OLS-3864,OLS-3865: sanitize LLM analysis options before CRD write - #442
OLS-3864,OLS-3865: sanitize LLM analysis options before CRD write#442tremes wants to merge 1 commit into
Conversation
Add two-layer defense against LLM output that violates CRD constraints: 1. Schema enforcement: add maxLength to all string fields in AnalysisOutputSchema and MinimalAnalysisOutputSchema so the LLM is told the limits upfront. 2. Go-side sanitization: add sanitizeAnalysisOptions() called from Analyze() that zeros per-option Diagnosis structs with empty required fields (prevents MinLength rejection) and truncates strings exceeding CRD MaxLength limits (prevents Too Long rejection). Both the JSON schemas and the sanitization function share the same maxLen* constants to keep limits in sync. Add TestSchemasCoverCRDMaxLength drift test to catch future divergence between CRD and LLM schema maxLength values. Signed-off-by: Tomáš Remeš <tremes@redhat.com> Assisted-by: Claude Code:claude-opus-4-6
openshift-ci-robot
added
the
jira/valid-reference
|
@tremes: This pull request references OLS-3864 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. This pull request references OLS-3865 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesAnalysis output limits
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controller/agenticrun/sandbox_agent.go (1)
119-124: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSanitize the top-level diagnosis before returning it.
Line 119 sanitizes only
resp.Options. IfactionRequiredis false, an oversizedresp.Diagnosis.Summaryorresp.Diagnosis.RootCausereachesAnalysisOutputunchanged. The CRD limits can then reject the status patch.Proposed fix
sanitizeAnalysisOptions(log, run.Name, resp.Options) -if resp.Diagnosis != nil && (resp.Diagnosis.Summary == "" || resp.Diagnosis.RootCause == "") { - log.Info("ignoring empty top-level diagnosis (per-option diagnoses used instead)", "run", run.Name) - resp.Diagnosis = nil +if resp.Diagnosis != nil { + if resp.Diagnosis.Summary == "" || resp.Diagnosis.RootCause == "" { + log.Info("ignoring empty top-level diagnosis (per-option diagnoses used instead)", "run", run.Name) + resp.Diagnosis = nil + } else { + resp.Diagnosis.Summary = truncate(resp.Diagnosis.Summary, maxLenDiagnosisSummary) + resp.Diagnosis.RootCause = truncate(resp.Diagnosis.RootCause, maxLenDiagnosisRootCause) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/agenticrun/sandbox_agent.go` around lines 119 - 124, Sanitize the top-level diagnosis before returning the analysis output, not only resp.Options. Update the flow around sanitizeAnalysisOptions and the resp.Diagnosis empty-check to apply the existing diagnosis sanitization and enforce CRD length limits on Summary and RootCause, including when actionRequired is false, while preserving the current nil handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controller/agenticrun/sandbox_agent.go`:
- Around line 338-345: Update truncate to limit Unicode characters using
rune-aware length and slicing, preserving unchanged strings within the limit;
add coverage for non-ASCII input. Before storing the top-level resp.Diagnosis in
AnalysisResult.Status, pass it through the same truncation limit used for
diagnosis text.
---
Outside diff comments:
In `@controller/agenticrun/sandbox_agent.go`:
- Around line 119-124: Sanitize the top-level diagnosis before returning the
analysis output, not only resp.Options. Update the flow around
sanitizeAnalysisOptions and the resp.Diagnosis empty-check to apply the existing
diagnosis sanitization and enforce CRD length limits on Summary and RootCause,
including when actionRequired is false, while preserving the current nil
handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b2f045d0-73ec-491d-b9f1-f42302785d47
📒 Files selected for processing (4)
controller/agenticrun/sandbox_agent.gocontroller/agenticrun/sandbox_agent_test.gocontroller/agenticrun/schema_crd_drift_test.gocontroller/agenticrun/schemas.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
| // truncate returns s unchanged if its byte length is within max, | ||
| // otherwise it returns the first max bytes. | ||
| func truncate(s string, max int) string { | ||
| if len(s) <= max { | ||
| return s | ||
| } | ||
| return s[:max] | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- truncate definition and usages ---'
rg -n -C 6 '\btruncate\(|func truncate\b' controller/agenticrun
printf '%s\n' '--- relevant sanitizer and response construction ---'
rg -n -C 12 'sanitize|AnalysisOutput|Diagnosis|Options|maxLength|RootCause|rootCause' controller/agenticrun api
printf '%s\n' '--- tests ---'
rg -n -C 8 'truncate|Unicode|UTF|rune|analysis' controller/agenticrun/*_test.go apiRepository: openshift/lightspeed-agentic-operator
Length of output: 50394
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- implementation ---'
rg -n -C 18 'func truncate|truncate\(' controller/agenticrun/sandbox_agent.go
printf '%s\n' '--- analysis flow ---'
rg -n -C 20 'func .*Analyze|AnalysisOutput|analysisResponse|sanitize|Options' controller/agenticrun/sandbox_agent.go
printf '%s\n' '--- CRD length annotations ---'
rg -n -C 4 'MaxLength|maxLength|RootCause|Summary|Description' api/v1alpha1
printf '%s\n' '--- truncation test ---'
sed -n '455,515p' controller/agenticrun/sandbox_agent_test.goRepository: openshift/lightspeed-agentic-operator
Length of output: 50393
🏁 Script executed (no clone):
#!/bin/bash
set -e
python3 - <<'PY'
samples = ["é" * 10, "🙂" * 10, "a🙂b"]
for s in samples:
for max_len in (1, 2, 5, 10):
raw = s.encode("utf-8")
prefix = raw[:max_len]
try:
decoded = prefix.decode("utf-8")
valid = True
chars = len(decoded)
except UnicodeDecodeError:
decoded = None
valid = False
chars = None
print({
"input_chars": len(s),
"input_bytes": len(raw),
"max": max_len,
"byte_prefix_valid_utf8": valid,
"byte_prefix_chars": chars,
})
PY
Length of output: 1408
🏁 Script executed (no clone):
#!/bin/bash
set -e
python3 - <<'PY'
samples = ["é" * 10, "🙂" * 10, "a🙂b"]
for s in samples:
for max_len in (1, 2, 5, 10):
raw = s.encode("utf-8")
prefix = raw[:max_len]
try:
decoded = prefix.decode("utf-8")
valid = True
chars = len(decoded)
except UnicodeDecodeError:
valid = False
chars = None
print({
"input_chars": len(s),
"input_bytes": len(raw),
"max": max_len,
"byte_prefix_valid_utf8": valid,
"byte_prefix_chars": chars,
})
PY
Length of output: 1408
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- analysis status types ---'
sed -n '1,260p' api/v1alpha1/agenticrun_status_types.go
printf '%s\n' '--- analysis result types ---'
sed -n '1,180p' api/v1alpha1/analysisresult_types.go
printf '%s\n' '--- generated CRD analysis schema references ---'
rg -n -C 3 '"(title|summary|rootCause|description|command|justification)":|maxLength:' config/crd/bases config/crd 2>/dev/null | head -n 240
printf '%s\n' '--- existing truncation assertions ---'
sed -n '465,510p' controller/agenticrun/sandbox_agent_test.go
printf '%s\n' '--- top-level diagnosis consumers ---'
rg -n -C 10 'output\.Diagnosis|Analysis\.Diagnosis|Diagnosis:' controller apiRepository: openshift/lightspeed-agentic-operator
Length of output: 50393
🏁 Script executed (no clone):
#!/bin/bash
set -e
cat > /tmp/json_utf8_probe.go <<'EOF'
package main
import (
"encoding/json"
"fmt"
)
func main() {
s := "🙂🙂"
b := []byte(s)[:5]
out, err := json.Marshal(struct {
Value string `json:"value"`
}{string(b)})
fmt.Printf("raw_valid=%t json=%s err=%v\n", json.Valid(out), out, err)
}
EOF
go run /tmp/json_utf8_probe.go
rm -f /tmp/json_utf8_probe.go
Length of output: 176
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- exact analysis model definitions ---'
rg -n 'type (DiagnosisResult|RemediationOption|RemediationPlan|ProposedAction|VerificationPlan|RBAC)|MaxLength' api/v1alpha1/analysisresult_types.go api/v1alpha1/*.go
printf '%s\n' '--- definitions context ---'
for pat in 'type DiagnosisResult' 'type RemediationOption' 'type RemediationPlan' 'type ProposedAction'; do
line=$(rg -n "$pat" api/v1alpha1 | head -n 1 | cut -d: -f2)
file=$(rg -l "$pat" api/v1alpha1 | head -n 1)
if [ -n "$file" ] && [ -n "$line" ]; then
start=$((line-8)); [ "$start" -lt 1 ] && start=1
end=$((line+100))
echo "--- $pat in $file ---"
sed -n "${start},${end}p" "$file"
fi
done
printf '%s\n' '--- analysis result creation ---'
rg -n -C 25 'AnalysisResultStatus|createAnalysis|CreateAnalysis|result :=.*Analysis|Diagnosis' controller/agenticrun --glob '*.go' | head -n 260Repository: openshift/lightspeed-agentic-operator
Length of output: 42948
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- non-test analysis result writes ---'
rg -n -C 18 'AnalysisResult\{|Status:.*AnalysisResultStatus|Diagnosis:.*output|Diagnosis:.*analysis|output\.Diagnosis|analysis\.Diagnosis' controller/agenticrun --glob '*.go' --glob '!**/*_test.go'
printf '%s\n' '--- all AnalysisOutput field assignments ---'
rg -n -C 10 'AnalysisOutput\{|\.Diagnosis|Diagnosis:' controller/agenticrun --glob '*.go' --glob '!**/*_test.go'Repository: openshift/lightspeed-agentic-operator
Length of output: 23603
Truncate strings by Unicode characters, and sanitize the top-level diagnosis.
len and s[:max] operate on bytes. Non-ASCII output can be split and serialized as �, while retaining fewer characters than the CRD limit. Use rune-aware truncation and add a Unicode test. Apply the same limits to the top-level resp.Diagnosis before storing it in AnalysisResult.Status.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/agenticrun/sandbox_agent.go` around lines 338 - 345, Update
truncate to limit Unicode characters using rune-aware length and slicing,
preserving unchanged strings within the limit; add coverage for non-ASCII input.
Before storing the top-level resp.Diagnosis in AnalysisResult.Status, pass it
through the same truncation limit used for diagnosis text.
|
@tremes: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Add two-layer defense against LLM output that violates CRD constraints:
Schema enforcement: add maxLength to all string fields in AnalysisOutputSchema and MinimalAnalysisOutputSchema so the LLM is told the limits upfront.
Go-side sanitization: add sanitizeAnalysisOptions() called from Analyze() that zeros per-option Diagnosis structs with empty required fields (prevents MinLength rejection) and truncates strings exceeding CRD MaxLength limits (prevents Too Long rejection).
Both the JSON schemas and the sanitization function share the same maxLen* constants to keep limits in sync.
Add TestSchemasCoverCRDMaxLength drift test to catch future divergence between CRD and LLM schema maxLength values.
Assisted-by: Claude Code:claude-opus-4-6