Drop estimatedImpact; align verification schema with CRD (#162)

[harche]

Fixes the schema/CRD contract drift in #162.

Problem

ProposalResult.EstimatedImpact was +required in the AnalysisResult CRD 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 → AnalysisResult persisted with 0 options, the proposal was marked Failed, and the analysis sandbox pod was orphaned.

Changes

• Remove estimatedImpact from the API type (proposal_analysis_types.go), the LLM output schema (schemas.go), the generated CRD, and the mock agent (test/agent/main.go).
• Tighten the verification branch of the LLM schema to match the CRD (steps[] require name+type; verification requires description) — the same latent drift class noted in the issue.
• Add 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, full controller/proposal tests pass; guard test negative-tested (injected drift → fails with precise path).
• Deployed to a 4.22 cluster and ran a Default-mode analysis: the agent produced a proposal without estimatedImpact, the AnalysisResult persisted Succeeded with 1 option, and the proposal advanced to Proposed (no orphaned pod).

🤖 Generated with Claude Code

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[coderabbitai]

Warning

Review limit reached

@harche, we couldn't start this review because you've reached your PR review rate limit.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 44f6c397-9187-4053-b2b7-a2d1a4fb8835

📥 Commits

Reviewing files that changed from the base of the PR and between 4c889b5 and a192d1f.

📒 Files selected for processing (1)

• controller/proposal/schema_crd_drift_test.go

📝 Walkthrough

Walkthrough

Removes the EstimatedImpact field from ProposalResult and the estimatedImpact property from the AnalysisOutputSchema JSON schema. Adds required constraints to verification and verification.steps items. Drops the field from the mock agent response. Introduces a drift test that validates CRD required fields are covered by the LLM schema.

Changes

Remove estimatedImpact and tighten schema contracts

Layer / File(s)	Summary
Remove EstimatedImpact from API type and LLM schema api/v1alpha1/proposal_analysis_types.go, controller/proposal/schemas.go, test/agent/main.go	EstimatedImpact field deleted from ProposalResult; estimatedImpact property removed from AnalysisOutputSchema; verification and verification.steps items gain explicit required constraints; mock agent canned response drops the field.
CRD/LLM schema drift detection test controller/proposal/schema_crd_drift_test.go	TestAnalysisSchemaCoversCRDRequiredFields loads the CRD YAML, parses AnalysisOutputSchema, and recursively asserts that all CRD required fields covered by the LLM schema are also required there. Helpers digObject, asJSONObject, and requiredSet support the traversal.

Possibly related issues

• Analysis fails to persist when LLM omits proposal.estimatedImpact — schema/CRD contract drift #162: The changes directly resolve the schema-CRD contract mismatch described in this issue by removing estimatedImpact from required fields, tightening the verification schema, and adding the drift-detection test to prevent regressions.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main changes: removing estimatedImpact and aligning the verification schema with the CRD, matching the core objectives.
Description check	✅ Passed	The description clearly explains the schema-to-CRD drift problem, lists the specific changes made, and details verification steps, directly relating to the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign joshuawilson for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between d176dbd and 4c889b5.

⛔ Files ignored due to path filters (1)

• config/crd/bases/agentic.openshift.io_analysisresults.yaml is excluded by !config/crd/bases/**

📒 Files selected for processing (4)

• api/v1alpha1/proposal_analysis_types.go
• controller/proposal/schema_crd_drift_test.go
• controller/proposal/schemas.go
• test/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

[openshift-ci]

@harche: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.