Drop estimatedImpact; align verification schema with CRD (#162)#174
Drop estimatedImpact; align verification schema with CRD (#162)#174harche wants to merge 2 commits into
Conversation
estimatedImpact on ProposalResult was +required in the AnalysisResult CRD but only optional in the LLM output schema, so analyses where the agent omitted it were rejected by CRD validation at status-patch time — persisting 0 options, marking the proposal Failed, and orphaning the analysis sandbox pod. - Remove estimatedImpact from the API type, the LLM output schema, the generated CRD, and the mock agent. - Tighten the verification branch of the LLM schema to match the CRD (steps[] require name+type; verification requires description), fixing the same latent drift class. - Add a guard test asserting every CRD-required field is also required in the corresponding LLM output schema.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Warning Review limit reached
More reviews will be available in 32 minutes and 27 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoves the ChangesRemove estimatedImpact and tighten schema contracts
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
harche
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/proposal/schema_crd_drift_test.go`:
- Around line 91-99: The guard condition `if _, modeled := llmProps[req];
!modeled { continue }` causes the test to silently skip CRD-required fields that
are completely absent from the LLM schema, allowing real contract breaks to go
undetected. Instead of continuing when a CRD-required field is not found in
llmProps, add an error check that reports when a CRD-required field is missing
entirely from the LLM output schema before checking if it is marked as required.
This ensures the test catches cases where required fields are removed from the
LLM schema contract.
In `@controller/proposal/schemas.go`:
- Around line 79-84: The operator schema in controller/proposal/schemas.go has
been modified to remove the estimatedImpact field and enforce new required
constraints, but the openshift/lightspeed-agentic-sandbox repository still
contains the old schema definition that includes estimatedImpact and lacks these
required field constraints. You need to update the schema definition in the
sandbox repository to match the current operator schema exactly, removing the
estimatedImpact field and adding the same required field constraints that are
now present in the "required": ["description"] structure and any nested property
requirements to restore schema parity and ensure consistent validation behavior.
🪄 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: d6a7a000-bac7-45c8-b4a4-c2c1a7b46c7b
⛔ Files ignored due to path filters (1)
config/crd/bases/agentic.openshift.io_analysisresults.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (4)
api/v1alpha1/proposal_analysis_types.gocontroller/proposal/schema_crd_drift_test.gocontroller/proposal/schemas.gotest/agent/main.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
💤 Files with no reviewable changes (1)
- api/v1alpha1/proposal_analysis_types.go
Address review feedback: the drift guard silently skipped CRD-required fields that were not modeled at all in the LLM output schema, so removing such a field could pass undetected. A required field that the agent is never asked for is rejected at status-patch time, which is exactly the drift this guard exists to catch — so error in that case too, not only when a modeled field lacks the required marker.
|
@harche: 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. |
joshuawilson
left a comment
joshuawilson
left a comment
There was a problem hiding this comment.
Adversarial Review
The core change (dropping estimatedImpact, tightening the analysis verification schema, adding a drift guard test) is correct and well-motivated. The fix-up commit addressing CodeRabbit's feedback is also sound.
However, the PR introduces a guard test that only protects one of four LLM output schemas, while live instances of the exact same drift class exist in the other schemas. These are confirmed by cross-referencing the CRD YAML required lists against the LLM schema required lists:
Two confirmed live drifts (same bug class as estimatedImpact)
| Schema | LLM required |
CRD required |
Missing |
|---|---|---|---|
VerificationOutputSchema checks[] |
["name", "result"] |
["name", "result", "source", "value"] |
source, value |
ExecutionOutputSchema verification |
(none) | ["conditionOutcome", "summary"] |
conditionOutcome, summary |
Both can cause the same runtime failure: LLM omits a field → status patch rejected by CRD validation → result marked Failed → sandbox pod orphaned.
Recommendation
- Fix the two confirmed drifts (add missing
requiredentries inschemas.go). - Extend the drift guard test to cover VerificationResult, ExecutionResult, and EscalationResult CRDs — otherwise the stated goal ("catch this class of drift automatically") is incomplete.
These can be in this PR or a fast-follow, but the drifts themselves are live bugs of the same class this PR fixes.
| assertRequiredCoverage(t, "options[]", options.Items.Schema, llmOption) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
MAJOR — Guard test only covers AnalysisResult, leaving live drift in other schemas
The test achieves its stated goal for AnalysisResult, but the same drift class this PR fixed for estimatedImpact exists right now in at least two other schemas:
1. VerificationOutputSchema (schemas.go:199) — LLM schema requires ["name", "result"], but the VerificationResult CRD (agentic.openshift.io_verificationresults.yaml:119-123) requires ["name", "result", "source", "value"]. If the LLM omits source or value, the VerificationResult status patch will be rejected by CRD validation — the same failure mode as estimatedImpact.
2. ExecutionOutputSchema (schemas.go:172-179) — The verification sub-object has no required array at all, but the ExecutionResult CRD (agentic.openshift.io_executionresults.yaml:253-255) requires ["conditionOutcome", "summary"]. Same failure mode.
Extending this test to cover all four result CRDs (AnalysisResult, VerificationResult, ExecutionResult, EscalationResult) against their respective *OutputSchema variables would catch these immediately and would prevent the stated goal — "catch this class of drift automatically" — from being incomplete on merge.
| }, | ||
| "required": ["name", "type"] | ||
| } | ||
| } |
There was a problem hiding this comment.
MAJOR — Tightening here is correct, but the same fix is needed in two other schemas
Good catch adding "required": ["name", "type"] for verification steps and "required": ["description"] for the verification object in the Analysis schema.
The same tightening is needed elsewhere:
-
VerificationOutputSchema(line 199 in this file): Currently"required": ["name", "result"]but the CRD requires["name", "result", "source", "value"]. Add"source"and"value"to the required array. -
ExecutionOutputSchema(lines 172-179 in this file): Theverificationsub-object has norequiredarray, but the CRD requires["conditionOutcome", "summary"]. Add"required": ["conditionOutcome", "summary"]to the verification object.
These are the same latent drift class noted in the issue description and would cause the exact same runtime failure (status patch rejected → result marked Failed → sandbox pod orphaned).
| for _, req := range crd.Required { | ||
| // A field the CRD requires must be both modeled and required in the LLM | ||
| // output schema. If it is absent entirely the agent is never asked for | ||
| // it, so the status patch is rejected at runtime — the exact drift this | ||
| // guard exists to catch. | ||
| if _, modeled := llmProps[req]; !modeled { | ||
| t.Errorf("schema/CRD drift at %s: CRD requires %q but the LLM output schema does not model the field", path, req) | ||
| continue | ||
| } | ||
| if !llmRequired[req] { | ||
| t.Errorf("schema/CRD drift at %s: CRD requires %q but the LLM output schema does not mark it required", path, req) | ||
| } |
There was a problem hiding this comment.
MINOR — Guard only checks one direction (CRD→LLM); reverse direction (LLM→CRD) is unchecked
The test asserts every CRD-required field is also required in the LLM schema. This is the right check for the estimatedImpact bug class (CRD rejects an LLM response).
However, the reverse is also worth guarding: if the LLM schema models a property that does not exist in the CRD at all (e.g., a typo like "descrption" instead of "description"), that field would be silently pruned by the API server on the status patch, causing silent data loss. A reverse check — asserting every property in the LLM schema exists as a property in the CRD — would catch this. Not a blocker, but would complete the guard.
| func TestAnalysisSchemaCoversCRDRequiredFields(t *testing.T) { | ||
| crdPath := filepath.Join("..", "..", "config", "crd", "bases", "agentic.openshift.io_analysisresults.yaml") | ||
| raw, err := os.ReadFile(crdPath) | ||
| if err != nil { |
There was a problem hiding this comment.
MINOR — Fragile relative path to CRD file
This relies on go test setting CWD to the package directory, which is correct, but if the package or CRD path is ever restructured, the test fails with a confusing "read CRD" error rather than a clear message about the expected layout. Consider a small clarifying comment, or alternatively using runtime.Caller(0) to resolve the path relative to the source file rather than CWD.
2 participants
Fixes the schema/CRD contract drift in #162.
Problem
ProposalResult.EstimatedImpactwas+requiredin theAnalysisResultCRD but only optional in the LLM output schema (controller/proposal/schemas.go). When the analysis agent omitted it (non-deterministic), the operator's status patch was rejected by CRD validation →AnalysisResultpersisted with 0 options, the proposal was marked Failed, and the analysis sandbox pod was orphaned.Changes
estimatedImpactfrom the API type (proposal_analysis_types.go), the LLM output schema (schemas.go), the generated CRD, and the mock agent (test/agent/main.go).verificationbranch of the LLM schema to match the CRD (steps[]requirename+type;verificationrequiresdescription) — the same latent drift class noted in the issue.schema_crd_drift_test.go: a guard test asserting every CRD-required field is also required in the corresponding LLM output schema, to catch this class of drift automatically.Verification
make manifests,make api-lint(0 issues),make vet/fmt-check, fullcontroller/proposaltests pass; guard test negative-tested (injected drift → fails with precise path).estimatedImpact, theAnalysisResultpersisted Succeeded with 1 option, and the proposal advanced toProposed(no orphaned pod).🤖 Generated with Claude Code