OLS-3654 Fix empty top-level diagnosis CRD validation failure#344
OLS-3654 Fix empty top-level diagnosis CRD validation failure#344vimalk78 wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[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 |
371eb58 to
d1b9129
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change filters incomplete top-level diagnoses during sandbox analysis and status construction. Regression tests verify that empty top-level diagnosis fields are omitted while per-option diagnoses remain preserved. ChangesDiagnosis validation and persistence
Sequence Diagram(s)sequenceDiagram
participant SandboxAgentCaller
participant AgenticRunReconciler
participant AnalysisResultStatus
SandboxAgentCaller->>SandboxAgentCaller: parse and clear incomplete top-level Diagnosis
SandboxAgentCaller->>AgenticRunReconciler: return analysis output with option diagnoses
AgenticRunReconciler->>AnalysisResultStatus: persist complete top-level Diagnosis
AgenticRunReconciler->>AnalysisResultStatus: preserve option-level diagnoses
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
vimalk78
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_test.go`:
- Around line 437-442: The incomplete-diagnosis tests cover only when both
required fields are empty; extend the cases around the sandbox diagnosis
assertion in controller/agenticrun/sandbox_agent_test.go:437-442 to cover
Summary-only and RootCause-only omissions, asserting the returned diagnosis is
nil. Add the corresponding one-sided empty-field cases around the persisted
status diagnosis assertions in controller/agenticrun/results_test.go:249-256,
asserting the status diagnosis is unset.
In `@controller/agenticrun/sandbox_agent.go`:
- Around line 109-112: Move the incomplete top-level diagnosis sanitization
block before the missing-diagnosis validation in the surrounding
response-handling flow. Ensure an incomplete diagnosis is set to nil before the
check that logs or handles missing diagnoses, so actionRequired=false responses
trigger the intended missing-diagnosis behavior; preserve the existing
per-option diagnosis handling.
🪄 Autofix (Beta)
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: Enterprise
Run ID: 888866e8-0efd-4aa9-bd03-e8aac7be5c47
📒 Files selected for processing (4)
controller/agenticrun/results.gocontroller/agenticrun/results_test.gocontroller/agenticrun/sandbox_agent.gocontroller/agenticrun/sandbox_agent_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
When the agent returns actionRequired=true with per-option diagnoses but an empty top-level diagnosis (summary="", rootCause="", confidence="Low"), the non-nil Diagnosis pointer causes the empty struct to be written to the AnalysisResult status. The CRD's MinLength=1 on summary and rootCause rejects the status patch. Fix: normalize empty diagnosis to nil in the parsing layer, and guard the assignment in createAnalysisResult to skip empty fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Vimal Kumar <vimal78@gmail.com>
d1b9129 to
ad29534
Compare
|
@vimalk78: 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. |
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
blublinsky
left a comment
blublinsky
left a comment
There was a problem hiding this comment.
should-fix: incomplete top-level diagnosis should also reject empty/missing confidence
OLS-3654 correctly drops a top-level Diagnosis when summary or rootCause is empty, which avoids CRD MinLength=1 failures on those fields. The CRD still requires all three:
required: [confidence, rootCause, summary]
# confidence enum: Low | Medium | HighWith a payload like:
{
"summary": "PostgresqlTooManyConnections firing",
"rootCause": "pool exhaustion",
"confidence": ""
}(or confidence omitted → Go zero value), both guards still treat the diagnosis as complete:
sandbox_agent.go: only checksSummary == "" || RootCause == ""results.go: only requiresSummary != "" && RootCause != ""
confidence is omitempty, so it is dropped from the status JSON and the API rejects the update for missing required confidence — same status-patch failure mode as OLS-3654.
Suggest treating the diagnosis as complete only when Summary, RootCause, and a valid Confidence (Low/Medium/High) are set, in both places, plus a table-driven test case for empty/missing confidence. A small shared helper would keep the parse-path and write-path checks aligned.
Happy to take this as a follow-up if you prefer to land the empty summary/rootCause fix first.
2 participants
Summary
actionRequired: truewith per-option diagnoses but an empty top-level diagnosis (summary="",rootCause="",confidence="Low"), the non-nilDiagnosispointer causes the empty struct to be written to the AnalysisResult status, which fails CRDMinLength=1validationTest plan
TestCreateAnalysisResult_EmptyTopLevelDiagnosis— verifiescreateAnalysisResultdoes not write empty diagnosis fields to statusTestSandboxAgentCaller_Analyze_EmptyTopLevelDiagnosis— verifiesAnalyze()normalizes empty diagnosis to nil🤖 Generated with Claude Code