OLS-3719, OTA-2090: Check needsRevision() in terminal phases before early return#366
OLS-3719, OTA-2090: Check needsRevision() in terminal phases before early return#366jhadvig wants to merge 4 commits into
Conversation
The reconciler's terminal phase handler (Completed, Denied, Escalated, EmergencyStopped) returned immediately without checking needsRevision(). This meant patching spec.revisionFeedback on a completed AgenticRun had no effect — the operator never re-entered the analysis loop. The NoActionRequired phase already had the correct pattern: guard the early return with !needsRevision() and fall through to the phase routing switch where handleRevision() is called. This commit applies the same pattern to the other terminal phases. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@jhadvig: This pull request references OTA-2090 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCompleted ChangesCompleted Run Revision
🚥 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}
Verify that patching revisionFeedback on a completed advisory-only AgenticRun triggers re-analysis. This is the scenario that was broken before the needsRevision() guard was added to terminal phases. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/handlers_test.go`:
- Around line 734-736: Update the assertion in the Completed-phase revision test
to require analyzed.ObservedGeneration to equal p.Generation, while retaining
the nil-condition guard and failure message. This verifies handleRevision
processed the current generation rather than accepting any nonzero stale value.
🪄 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: d618756f-0f3a-4a40-993c-667ec5f56a53
📒 Files selected for processing (1)
controller/agenticrun/handlers_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
harche
left a comment
harche
left a comment
There was a problem hiding this comment.
Thanks for the thorough writeup. The root cause analysis is spot on, and the Completed path (the actual OTA-2090 scenario) is correct and covered by a good test.
However, I would ask that we restrict the new fall through to Completed only and leave Denied, Escalated, and EmergencyStopped out of both switches. Two reasons:
1. Two of the four phases are mechanically broken. handleRevision() only clears the Executed/Verified/Escalated conditions. It never removes Denied or EmergencyStopped, and DerivePhase() checks those two first. So a revision from Denied or EmergencyStopped runs the full reanalysis (a real LLM call, a new AnalysisResult CR, Analyzed flips to RevisionComplete) and then the run snaps straight back to the same terminal phase. The revised analysis never surfaces as Proposed. I verified this with a quick test against this branch (details in the inline comment).
2. It matches the documented contract of the field. revisionFeedback is defined as feedback on the analysis:
lightspeed-agentic-operator/api/v1alpha1/agenticrun_types.go
Lines 370 to 376 in 6180ad2
A user denial or the emergency kill switch is not feedback about the analysis, and revision should not be a path to resurrect a killed run. If we later want "deny with feedback, then reanalyse," that is a deliberate feature: handleRevision() would need to clear the Denied condition too, plus tests.
Notes that do not block:
- The comment at
reconciler.go:150saying that only runs in nonterminal phases reach the suspension guard is now stale: a terminal run that needs revision reaches it too. Worth updating while you are here. - Narrow race worth a follow up (the shape predates this PR, but this PR widens the exposure): if
revisionFeedbacklands before the terminal reconcile has released sandboxes, the release at the top of the switch is skipped, andhandleRevision()callsresetExecutionAndVerification()which wipes the claim names from status. After that, nothing (including the deletion finalizer) can find them to release. TheNoActionRequiredpath never had this exposure since analysis only runs carry no execution or verification sandboxes.
| agenticv1alpha1.AgenticRunPhaseDenied, | ||
| agenticv1alpha1.AgenticRunPhaseEscalated, | ||
| agenticv1alpha1.AgenticRunPhaseEmergencyStopped: | ||
| if hasSandboxClaims(&run) { | ||
| if err := r.Agent.ReleaseSandboxes(ctx, &run); err != nil { | ||
| log.Error(err, "sandbox cleanup failed at terminal phase") | ||
| if !needsRevision(&run) { |
There was a problem hiding this comment.
Suggest restricting this to AgenticRunPhaseCompleted only, keeping Denied, Escalated, and EmergencyStopped on the unconditional early return.
For Denied and EmergencyStopped the fall through cannot actually work: handleRevision() only removes these three conditions:
lightspeed-agentic-operator/controller/agenticrun/handlers.go
Lines 157 to 161 in 6180ad2
while DerivePhase() checks EmergencyStopped and Denied before anything else:
lightspeed-agentic-operator/api/v1alpha1/agenticrun_types.go
Lines 67 to 78 in 6180ad2
Walking through a denied run with this patch applied:
- User patches
spec.revisionFeedback, generation becomes 2,needsRevision()returns true - Falls through to phase routing,
handleRevision()reruns analysis: agent invoked,AnalysisResultcreated,AnalyzedbecomesRevisionComplete (generation 2) - But
Denied=Truewas never removed, soDerivePhase()still returnsDenied - Next reconcile:
needsRevision()is now false (generation caught up), so the run takes the terminal cleanup path and sandboxes are released
Net effect: one paid LLM analysis, one orphaned AnalysisResult, run still terminally Denied, and every later revision attempt repeats the cycle. I confirmed this with a unit test on this branch: seeded a Denied run, patched feedback, reconciled 3 times:
phase after revision: Denied
analyzed condition: {Type:Analyzed Status:True ObservedGeneration:2 Reason:RevisionComplete
Message:Revision complete (generation 2) with 1 option(s)}
Escalated does route correctly (the condition is cleared), but semantically revisionFeedback is documented as feedback on the analysis; an escalation, a denial, or the kill switch is not that. Scoping this PR to Completed keeps it aligned with OTA-2090 and the contract of the field; the other phases can be deliberate follow ups if a use case shows up.
| case agenticv1alpha1.AgenticRunPhaseNoActionRequired, | ||
| agenticv1alpha1.AgenticRunPhaseCompleted, | ||
| agenticv1alpha1.AgenticRunPhaseDenied, | ||
| agenticv1alpha1.AgenticRunPhaseEscalated, | ||
| agenticv1alpha1.AgenticRunPhaseEmergencyStopped: |
There was a problem hiding this comment.
Same restriction here. With only Completed added, this becomes:
case agenticv1alpha1.AgenticRunPhaseNoActionRequired,
agenticv1alpha1.AgenticRunPhaseCompleted:
if needsRevision(&run) {
return r.handleRevision(ctx, &run, resolved)
}
return ctrl.Result{}, nilOptional tightening to consider: Completed on a run that actually executed means the cluster was already remediated. Revision would reanalyse the post remediation state and could lead to a second approved execution. If we only intend the advisory flow (the OTA-2090 case), gate on the established idiom for "no execution step":
case agenticv1alpha1.AgenticRunPhaseCompleted:
if run.Spec.Execution.IsZero() && needsRevision(&run) {
return r.handleRevision(ctx, &run, resolved)
}
return ctrl.Result{}, nil!run.Spec.Execution.IsZero() is already how the codebase detects that execution is configured:
If you gate here, mirror the same condition in the first switch so the two stay consistent. Also: the two case bodies in the first switch are now byte for byte identical, so it is worth merging NoActionRequired and Completed into a single case there too.
| // Submit revision on the completed run | ||
| reviseAgenticRun(t, fc, "fix-crash", "re-analyse with different focus") | ||
|
|
||
| // Reconcile 2: Completed → revision → Proposed |
There was a problem hiding this comment.
nit: copy paste, this is the third reconcile, but the comment here and the error message on line 728 both say "Reconcile 2".
| if agenticv1alpha1.DerivePhase(p.Status.Conditions) != agenticv1alpha1.AgenticRunPhaseProposed { | ||
| t.Fatalf("expected Proposed after revision from Completed, got %s", agenticv1alpha1.DerivePhase(p.Status.Conditions)) | ||
| } | ||
| if analyzed := meta.FindStatusCondition(p.Status.Conditions, agenticv1alpha1.AgenticRunConditionAnalyzed); analyzed == nil || analyzed.ObservedGeneration == 0 { |
There was a problem hiding this comment.
nit: analyzed.ObservedGeneration == 0 would pass even if the revision never observed the new generation (the stale value is 1). Asserting analyzed.ObservedGeneration == p.Generation directly proves the fix. (I know the == 0 shape mirrors the existing revision tests, so it is fine to leave for consistency, but since observedGeneration staleness is the exact symptom in the Jira, the stronger assertion earns its keep here.)
e33823a to
7ab3703
Compare
Per harche's review, restrict the needsRevision() fall-through to Completed only. Denied, Escalated, and EmergencyStopped are excluded because handleRevision() doesn't clear their conditions, so DerivePhase() would snap right back to the same terminal state. Also tighten the test assertion to verify ObservedGeneration equals the current Generation (not just non-zero), and update the suspension guard comment that was made stale by the Completed fall-through. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7ab3703 to
7f38a67
Compare
|
LGTM, tagging @onmete @blublinsky @xrajesh in case they want to review it as well and apply the tag. |
|
@jhadvig is going to update this PR for handling Failed state as well. /hold remove it once the PR is updated. |
openshift-ci
Bot
added
the
do-not-merge/hold
Per Harche's approval, also allow revision from Failed advisory-only runs. When analysis fails (e.g. LLM timeout), the user should be able to retry via revisionFeedback. handleRevision() resets the Analyzed condition from False/Failed to Unknown/Revising, clearing the failure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@jhadvig: 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. |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
harche
left a comment
harche
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. Verified the latest head locally: all package tests pass, and the Failed addition is mechanically sound in a way Denied was not. There is no dedicated Failed condition for DerivePhase() to get stuck on; Failed derives from a step condition being False, and handleRevision() resets exactly those, so the run genuinely lands at Proposed. Also confirmed the no loop property: failStep() stamps ObservedGeneration with the current generation, so a second failure makes needsRevision() false and the next reconcile takes the normal handleFailed() path instead of retrying forever.
Two observations, neither blocking.
| case agenticv1alpha1.AgenticRunPhaseFailed: | ||
| return r.handleFailed(ctx, &run) | ||
| if !(run.Spec.Execution.IsZero() && needsRevision(&run)) { | ||
| return r.handleFailed(ctx, &run) | ||
| } |
There was a problem hiding this comment.
Observation (not blocking): revision from Failed is really "retry analysis with feedback" (the new test even models an LLM timeout) rather than the documented "feedback requesting changes to the analysis." For advisory only runs the distinction is harmless, and it is arguably a UX win: the Re-analyse button now also recovers from transient analysis failures. Just flagging it so the extension of the revisionFeedback contract is a deliberate choice rather than an accident. If it is intended, a one line addition to the field's doc comment in a follow up would keep the contract honest.
| } | ||
|
|
||
| // --- Suspension guard (only non-terminal runs reach here) --- | ||
| // --- Suspension guard (non-terminal runs and advisory-only Completed runs needing revision reach here) --- |
There was a problem hiding this comment.
nit: this comment is stale again now that Failed runs also reach the suspension guard. Suggest "non-terminal runs and advisory-only Completed/Failed runs needing revision reach here".
|
Well, terminal state means terminal. Should we revive the terminal AgenticRun instead of spawning a new one? |
|
Fair concern from @onmete, and honestly there is no objectively right answer here; both models (revive in place vs. spawn a fresh run) are defensible. I am fine with the PR as it stands, but ultimately happy to go with whichever direction @onmete feels is right. A few data points that shaped my thinking, for whatever they are worth: The spec seems designed for in place revision. lightspeed-agentic-operator/api/v1alpha1/agenticrun_types.go Lines 370 to 380 in d0a2f39 The precedent already exists on main. lightspeed-agentic-operator/controller/agenticrun/helpers.go Lines 156 to 162 in d0a2f39 and it has supported revision since before this PR; this change mirrors that existing pattern rather than inventing revival: lightspeed-agentic-operator/controller/agenticrun/reconciler.go Lines 115 to 127 in d0a2f39 The CRD already models multiple rounds. Each step keeps an array of result CRs, one per round, so revision history accumulates on the same object: and the revision context handed to the agent embeds the generation number, so each round is traceable: lightspeed-agentic-operator/controller/agenticrun/helpers.go Lines 284 to 299 in d0a2f39 Where @onmete's instinct is exactly right, and the PR agrees: for a run that actually executed, Completed genuinely is terminal; the cluster was changed, and reviving it would be misleading. That case stays terminal here. The fall through is gated on lightspeed-agentic-operator/controller/agenticrun/reconciler.go Lines 129 to 142 in d0a2f39 On spawning a new run instead: totally workable too, it would just need a clone and link mechanism (carry over the request, reference the predecessor, re create the approval), and the console's Re-analyse flow would span two objects instead of one thread. That felt like a bigger feature than this bug fix, and it could still be added later without conflicting with this change if run lineage as a first class concept is ever wanted. Also worth noting: "re analyzing just after analysis" (Proposed) already works on main. OTA-2090 is specifically about the advisory flow where the run has already reached Completed, which is why the narrow gate above is the shape of the fix. Happy to defer to @onmete on the final call. |
jhadvig
changed the title
|
@jhadvig: This pull request references OLS-3719 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. This pull request references OTA-2090 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. |
4 participants
Summary
Fixes a bug where patching
spec.revisionFeedbackon a completed AgenticRun has no effect. The "Re-analyse" button in the cluster-update-console-plugin triggers this patch, but the operator never re-enters the analysis loop.Jira: https://redhat.atlassian.net/browse/OTA-2090
Root cause
The reconciler's first switch block handles terminal phases before the suspension guard. The
NoActionRequiredphase correctly guards its early return with!needsRevision(&run), falling through to the phase routing switch when a revision is needed. However, theCompleted,Denied,Escalated, andEmergencyStoppedphases return unconditionally without checkingneedsRevision():Fix
Two changes in
controller/agenticrun/reconciler.go:First switch (terminal phases): wrap the early return in
if !needsRevision(&run), mirroring the existingNoActionRequiredpattern. When revision is needed, execution falls through to the phase routing switch.Second switch (phase routing): add
Completed/Denied/Escalated/EmergencyStoppedto theNoActionRequiredcase, which already callshandleRevision()whenneedsRevision()returns true.How to reproduce
spec.revisionFeedback:metadata.generationincrements butAnalyzed.observedGenerationstays staleEvidence