OLS-3793 spec: remove verification retry mechanism, escalate on failure - #421
Open
onmete wants to merge 1 commit into
Open
OLS-3793 spec: remove verification retry mechanism, escalate on failure#421onmete wants to merge 1 commit into
onmete wants to merge 1 commit into
Conversation
Verification failure now leads directly to escalation instead of re-executing remediation. Removes maxAttempts from ApprovalPolicy and AgenticRunApproval, removes retryIndex from result CRs, simplifies phase derivation. Convergence-dependent checks are handled within the verification agent prompt. Related: OLS-3781, OLS-3793 Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe specifications remove retry and attempt configuration. Execution runs once per analysis iteration. Verification failure sets failure state and enters escalation. Audit logging records verification completion for every created ChangesExecution lifecycle changes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 3
🤖 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 @.ai/spec/what/audit-logging.md:
- Line 44: Update the agenticrun.verification.completed emission described in
the audit-logging table to occur only when the operator first observes
VerificationResult status Completed=True, rather than when the
VerificationResult CR is created. Include both successful and failed completed
results, and ensure the event payload contains the fully populated status and
does not represent an in-progress verification.
In @.ai/spec/what/run-lifecycle.md:
- Line 23: Update the phase-derivation rule for Verified=False to remove the
obsolete dedicated retrying-execution exception; state that it derives Failed
unless an earlier escalation or denial rule takes precedence, and update the
referenced Rule 16 wording consistently.
In @.ai/spec/what/sandbox-execution.md:
- Line 63: Align the removed retryIndex contract across both specifications: in
.ai/spec/what/sandbox-execution.md lines 63-63, remove retryIndex from Rule 7a
line 14; in .ai/spec/what/crd-api.md lines 31-39, retain its removal and update
the execution and verification result-template definitions so neither requires
retryIndex.
🪄 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: Enterprise
Run ID: 50882a2b-c54d-4061-b4c0-69f42a1586cf
📒 Files selected for processing (5)
.ai/spec/what/approval.md.ai/spec/what/audit-logging.md.ai/spec/what/crd-api.md.ai/spec/what/run-lifecycle.md.ai/spec/what/sandbox-execution.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
| | `agenticrun.execution.completed` | `agenticrun.execute` | ExecutionResult CR created | `result.name`, `result.uid`, `actions_taken.count` + full ExecutionResult CR serialization | | ||
| | `agenticrun.verification.completed` | `agenticrun.verify` | VerificationResult CR created, checks passed | `result.name`, `result.uid`, `checks.count` + full VerificationResult CR serialization | | ||
| | `agenticrun.verification.retry` | `agenticrun.verify` | Verification failed, retrying execution+verification | `result.name`, `retry_count`, `checks.count` + full VerificationResult CR serialization | | ||
| | `agenticrun.verification.completed` | `agenticrun.verify` | VerificationResult CR created | `result.name`, `result.uid`, `checks.count` + full VerificationResult CR serialization | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Emit verification completion after the Result CR is complete.
The rule triggers agenticrun.verification.completed when the VerificationResult CR is created. The sandbox creates the CR first and patches status in a second API call. Emit the event when the operator first observes Completed=True, including failed results, so the event contains the full status and does not label an in-progress verification as completed.
🤖 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 @.ai/spec/what/audit-logging.md at line 44, Update the
agenticrun.verification.completed emission described in the audit-logging table
to occur only when the operator first observes VerificationResult status
Completed=True, rather than when the VerificationResult CR is created. Include
both successful and failed completed results, and ensure the event payload
contains the fully populated status and does not represent an in-progress
verification.
| - If `Verified` is `Unknown` → phase `Verifying`. | ||
| - If `Verified` is `False` AND reason indicates retrying execution → phase `Executing`. | ||
| - If `Verified` is `False` otherwise → phase `Failed`. | ||
| - If `Verified` is `False` → phase `Failed` (unless `Escalated` is set, which takes precedence per rule ordering). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the obsolete retry exception from phase derivation.
This change removes the retrying-execution phase. However, Rule 16, Line 51 still exempts a “dedicated retrying-execution reason” from the Failed phase. Delete that exception and state that Verified=False derives Failed unless an earlier escalation or denial rule takes precedence.
🤖 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 @.ai/spec/what/run-lifecycle.md at line 23, Update the phase-derivation rule
for Verified=False to remove the obsolete dedicated retrying-execution
exception; state that it derives Failed unless an earlier escalation or denial
rule takes precedence, and update the referenced Rule 16 wording consistently.
| 25. **Finalizers**: Non-deleted runs MUST gain a cleanup finalizer before leaving non-terminal phases so deletion can run RBAC and sandbox release hooks safely. | ||
| 26. **[OLS-3066] Result CR writes**: Result CRs are created by the **sandbox** (not the operator) via `oc` — see rules 7a and 8. The operator's role is to (a) pre-compute the result template in the input ConfigMap, and (b) process the Result CR after the sandbox creates it. The Result CR retains the same shape: immutable spec, owner reference to the `AgenticRun`, started/completed conditions in status, embedded outcome payload, sandbox reference, and optional `failureReason` for system errors. Console field paths on Result CR `.status` are unchanged. | ||
| 27. **Retry index**: `ExecutionResult` and `VerificationResult` MUST record the current execution retry index in spec for correlation with `status.steps.execution.retryCount`. | ||
| 27. [REMOVED] `retryIndex` on `ExecutionResult` and `VerificationResult` has been removed. Execution runs exactly once per analysis iteration; verification failure escalates directly. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Align the removed retryIndex contract across the specifications.
The CRD contract removes retryIndex, but the sandbox result-template rule still requires it.
.ai/spec/what/sandbox-execution.md#L63-L63: removeretryIndexfrom Rule 7a, Line 14..ai/spec/what/crd-api.md#L31-L39: retain the removal and align the execution and verification result-template definitions with it.
📍 Affects 2 files
.ai/spec/what/sandbox-execution.md#L63-L63(this comment).ai/spec/what/crd-api.md#L31-L39
🤖 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 @.ai/spec/what/sandbox-execution.md at line 63, Align the removed retryIndex
contract across both specifications: in .ai/spec/what/sandbox-execution.md lines
63-63, remove retryIndex from Rule 7a line 14; in .ai/spec/what/crd-api.md lines
31-39, retain its removal and update the execution and verification
result-template definitions so neither requires retryIndex.
Contributor
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Spec-only change, pre-push reviewed.
Verification failure now leads directly to escalation instead of re-executing remediation. Removes
maxAttemptsfromApprovalPolicyandAgenticRunApproval, removesretryIndexfrom result CRs, simplifies phase derivation logic. Convergence-dependent checks are handled within the verification agent prompt.Changes:
run-lifecycle.md: Rewrite rules 7–8 (verification failure → escalation, no retries), simplifyVerified=Falsederivationapproval.md: Remove rules 18–21 (maxAttempts), add single-execution-attempt rule, update CEL invariants and config surfacecrd-api.md: RemoveretryCount,retryIndex,maxAttemptsfrom CRD field descriptionsaudit-logging.md: Remove retry events section andretry_countattributesandbox-execution.md: RemoveretryIndexrule, simplify execution timeout ruleRelated: OLS-3781, OLS-3793
Made with Cursor