OLS-3223: Fix ProposalApproval creation for analysis-only proposals#55
OLS-3223: Fix ProposalApproval creation for analysis-only proposals#55onmete wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@onmete: This pull request references OLS-3223 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. |
|
Warning Review limit reached
More reviews will be available in 37 minutes and 36 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. 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 (4)
📝 WalkthroughWalkthroughThe PR conditionally skips seeding approval stages for workflow steps that a Proposal does not define, preventing invalid stage entries. The specification is clarified, the approval seeding logic implements guards for absent steps, and two tests validate the behavior at unit and integration levels. ChangesConditional approval stage seeding
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controller/proposal/approval.go (1)
60-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix Analysis/Escalation auto-stage seeding when the proposal agent is empty (and drop redundant Escalation seeding for automatic policies)
controller/proposal/approval.goseedsstage.Analysis = AnalysisApproval{Agent: proposal.Spec.Analysis.Agent}unconditionally and seedsstage.Escalation = EscalationApproval{Agent: proposal.Spec.Analysis.Agent}forSandboxStepEscalationautomatic policies; there’s noIsZero()guard for either path.Proposal.spechas no escalation field (so escalation can’t be “defined” by the proposal), and for escalation handling the reconciler defaults toresolved.Analysisunless an escalation stage override exists;isStageApproved()already auto-approves via the policy when the stage entry is absent. So seeding an Escalation stage forAutomaticis redundant.- Empty-agent risk is still real:
ApprovalStagediscriminated-union validation requireshas(self.analysis)/has(self.escalation)whentypematches, butanalysis/escalationarejson:",omitzero". Ifproposal.Spec.Analysis.Agentis omitted/empty, the seededAnalysisApproval{Agent: ""}/EscalationApproval{Agent: ""}becomes the zero struct and causes the entireanalysis/escalationfield to be omitted—reintroducing the same validation failure path you guarded for Execution/Verification.ProposalStep.Agentis optional (no CRD default annotation forProposalStep.Agent), so non-empty is not guaranteed.🤖 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/proposal/approval.go` around lines 60 - 75, The code seeds stage.Analysis and stage.Escalation unconditionally which can produce zero-value structs (empty Agent) and reintroduce validation failures; update the switch in controller/proposal/approval.go so that for SandboxStepAnalysis you only set stage.Analysis when proposal.Spec.Analysis.IsZero() is false (or proposal.Spec.Analysis.Agent is non-empty), for SandboxStepExecution and SandboxStepVerification keep the existing IsZero guards, and remove the automatic seeding of stage.Escalation entirely for SandboxStepEscalation (do not set EscalationApproval using proposal.Spec.Analysis.Agent); this ensures you only create non-empty Analysis/Execution/Verification entries and avoid redundant Escalation seeding for automatic policies.
🤖 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.
Outside diff comments:
In `@controller/proposal/approval.go`:
- Around line 60-75: The code seeds stage.Analysis and stage.Escalation
unconditionally which can produce zero-value structs (empty Agent) and
reintroduce validation failures; update the switch in
controller/proposal/approval.go so that for SandboxStepAnalysis you only set
stage.Analysis when proposal.Spec.Analysis.IsZero() is false (or
proposal.Spec.Analysis.Agent is non-empty), for SandboxStepExecution and
SandboxStepVerification keep the existing IsZero guards, and remove the
automatic seeding of stage.Escalation entirely for SandboxStepEscalation (do not
set EscalationApproval using proposal.Spec.Analysis.Agent); this ensures you
only create non-empty Analysis/Execution/Verification entries and avoid
redundant Escalation seeding for automatic policies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 02a86769-5a92-4b57-94ed-b51ff5cb1f13
📒 Files selected for processing (4)
.ai/spec/what/approval.mdcontroller/proposal/approval.gocontroller/proposal/approval_test.gocontroller/proposal/state_machine_test.go
…ages (OLS-3223) When the ApprovalPolicy auto-approves Verification or Execution but the proposal is analysis-only (or has no verification), the controller tried to create an ApprovalStage with an empty agent. The zero-value struct was omitted by omitzero serialization, causing the CEL discriminated-union validation to reject the Create call. The reconciler retried indefinitely. Fix: check ProposalStep.IsZero() before building auto-stage entries, aligning with the same guards resolveProposal() already uses. Co-authored-by: Cursor <cursoragent@cursor.com>
onmete
force-pushed
the
fix/ols-3223-advisory-approval-creation
branch
from
cb68eda to
1614dde
Compare
|
Re: CodeRabbit review — Analysis/Escalation empty-agent risk: Fixed in 1614dde:
|
|
@onmete: 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
Summary
ensureProposalApprovalcrashing on analysis-only proposals when theApprovalPolicyauto-approves Verification or Execution stagesIsZero()guard).ai/spec/what/approval.mdrule 2) to clarify absent steps must not be seededRoot Cause
The auto-stage loop iterated over all
Automaticpolicy stages unconditionally. For analysis-only proposals,proposal.Spec.Verificationis zero-value — producing aVerificationApproval{Agent: ""}that serializes as an empty struct, which the CEL discriminated-union validation rejects.Testing
make vet— passmake api-lint— passmake test— pass (all unit + state machine tests)TestEnsureProposalApproval_AnalysisOnly_SkipsAbsentStagesTestAutoApproval_AdvisoryOnly_SkipsAbsentStagesSpec
.ai/spec/what/approval.md(rule 2 — seeding constraint for absent steps)Made with Cursor
Summary by CodeRabbit
Bug Fixes
Tests
Documentation