From 1bee6931894f91ba3e0478d5abc53838b92f25cd Mon Sep 17 00:00:00 2001 From: Jakub Hadvig Date: Thu, 23 Jul 2026 09:40:15 +0200 Subject: [PATCH 1/4] OTA-2090: Check needsRevision() in terminal phases before early return MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- controller/agenticrun/reconciler.go | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/controller/agenticrun/reconciler.go b/controller/agenticrun/reconciler.go index 81b7e0ac..2235bb44 100644 --- a/controller/agenticrun/reconciler.go +++ b/controller/agenticrun/reconciler.go @@ -130,16 +130,18 @@ func (r *AgenticRunReconciler) Reconcile(ctx context.Context, req ctrl.Request) 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) { + if hasSandboxClaims(&run) { + if err := r.Agent.ReleaseSandboxes(ctx, &run); err != nil { + log.Error(err, "sandbox cleanup failed at terminal phase") + } } + if r.Audit != nil { + r.Audit.EmitTerminalSpan(ctx, &run, string(phase), terminalReason(&run)) + r.Audit.Cleanup(&run) + } + return ctrl.Result{}, nil } - if r.Audit != nil { - r.Audit.EmitTerminalSpan(ctx, &run, string(phase), terminalReason(&run)) - r.Audit.Cleanup(&run) - } - return ctrl.Result{}, nil case agenticv1alpha1.AgenticRunPhaseFailed: return r.handleFailed(ctx, &run) @@ -209,7 +211,11 @@ func (r *AgenticRunReconciler) Reconcile(ctx context.Context, req ctrl.Request) } return r.handleEscalation(ctx, &run, resolved, approval, policy) - case agenticv1alpha1.AgenticRunPhaseNoActionRequired: + case agenticv1alpha1.AgenticRunPhaseNoActionRequired, + agenticv1alpha1.AgenticRunPhaseCompleted, + agenticv1alpha1.AgenticRunPhaseDenied, + agenticv1alpha1.AgenticRunPhaseEscalated, + agenticv1alpha1.AgenticRunPhaseEmergencyStopped: if needsRevision(&run) { return r.handleRevision(ctx, &run, resolved) } From 6180ad2e13508447d5cff92bd0d84c313e8e328d Mon Sep 17 00:00:00 2001 From: Jakub Hadvig Date: Thu, 23 Jul 2026 09:49:32 +0200 Subject: [PATCH 2/4] Add test for revision from Completed phase 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) --- controller/agenticrun/handlers_test.go | 54 ++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/controller/agenticrun/handlers_test.go b/controller/agenticrun/handlers_test.go index 1e0011fd..196e638e 100644 --- a/controller/agenticrun/handlers_test.go +++ b/controller/agenticrun/handlers_test.go @@ -682,6 +682,60 @@ func TestReconcile_RevisionWithFeedback(t *testing.T) { } } +func TestReconcile_RevisionFromCompleted(t *testing.T) { + scheme := testScheme() + // Advisory-only run (no Execution/Verification) reaches Completed in one reconcile + run := &agenticv1alpha1.AgenticRun{ + ObjectMeta: metav1.ObjectMeta{Name: "fix-crash", Namespace: "default"}, + Spec: agenticv1alpha1.AgenticRunSpec{ + Request: "Investigate issue", + Tools: testTools(), + TargetNamespaces: []string{"production"}, + Analysis: agenticv1alpha1.AgenticRunStep{Agent: "default"}, + }, + } + + objs := append([]client.Object{run}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...). + WithStatusSubresource(run, &agenticv1alpha1.AnalysisResult{}, &agenticv1alpha1.ExecutionResult{}, &agenticv1alpha1.VerificationResult{}, &agenticv1alpha1.EscalationResult{}).Build() + + r := &AgenticRunReconciler{Client: fc, Agent: newTestAgentCaller(), Namespace: "default"} + + // Reconcile 1: Pending → Proposed (analysis complete) + if _, err := reconcileOnce(r, "fix-crash"); err != nil { + t.Fatalf("reconcile 1: %v", err) + } + p, _ := getAgenticRun(r, "fix-crash") + if agenticv1alpha1.DerivePhase(p.Status.Conditions) != agenticv1alpha1.AgenticRunPhaseProposed { + t.Fatalf("expected Proposed, got %s", agenticv1alpha1.DerivePhase(p.Status.Conditions)) + } + + // Approve and reconcile 2: Proposed → Completed (advisory-only, no execution) + approveAgenticRun(t, fc, "fix-crash") + if _, err := reconcileOnce(r, "fix-crash"); err != nil { + t.Fatalf("reconcile 2: %v", err) + } + p, _ = getAgenticRun(r, "fix-crash") + if agenticv1alpha1.DerivePhase(p.Status.Conditions) != agenticv1alpha1.AgenticRunPhaseCompleted { + t.Fatalf("expected Completed, got %s", agenticv1alpha1.DerivePhase(p.Status.Conditions)) + } + + // Submit revision on the completed run + reviseAgenticRun(t, fc, "fix-crash", "re-analyse with different focus") + + // Reconcile 2: Completed → revision → Proposed + if _, err := reconcileOnce(r, "fix-crash"); err != nil { + t.Fatalf("reconcile 2 (revision from Completed): %v", err) + } + p, _ = getAgenticRun(r, "fix-crash") + 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 { + t.Fatal("observedGeneration not set after revision from Completed") + } +} + func TestReconcile_ExecutionRBACCreatedOnApproval(t *testing.T) { agent := newTestAgentCaller() agent.analyzeResult = &AnalysisOutput{ From 7f38a67b345d6a075d61219fe24d4980ee09017d Mon Sep 17 00:00:00 2001 From: Jakub Hadvig Date: Thu, 23 Jul 2026 18:08:07 +0200 Subject: [PATCH 3/4] Address review: narrow revision to Completed phase only 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) --- controller/agenticrun/handlers_test.go | 8 +++--- controller/agenticrun/reconciler.go | 35 ++++++++++++++++++-------- 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/controller/agenticrun/handlers_test.go b/controller/agenticrun/handlers_test.go index 196e638e..cc57c7de 100644 --- a/controller/agenticrun/handlers_test.go +++ b/controller/agenticrun/handlers_test.go @@ -723,16 +723,16 @@ func TestReconcile_RevisionFromCompleted(t *testing.T) { // Submit revision on the completed run reviseAgenticRun(t, fc, "fix-crash", "re-analyse with different focus") - // Reconcile 2: Completed → revision → Proposed + // Reconcile 3: Completed → revision → Proposed if _, err := reconcileOnce(r, "fix-crash"); err != nil { - t.Fatalf("reconcile 2 (revision from Completed): %v", err) + t.Fatalf("reconcile 3 (revision from Completed): %v", err) } p, _ = getAgenticRun(r, "fix-crash") 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 { - t.Fatal("observedGeneration not set after revision from Completed") + if analyzed := meta.FindStatusCondition(p.Status.Conditions, agenticv1alpha1.AgenticRunConditionAnalyzed); analyzed == nil || analyzed.ObservedGeneration != p.Generation { + t.Fatalf("expected observedGeneration to equal current generation %d after revision from Completed", p.Generation) } } diff --git a/controller/agenticrun/reconciler.go b/controller/agenticrun/reconciler.go index 2235bb44..502e5e91 100644 --- a/controller/agenticrun/reconciler.go +++ b/controller/agenticrun/reconciler.go @@ -126,11 +126,8 @@ func (r *AgenticRunReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, nil } - case agenticv1alpha1.AgenticRunPhaseCompleted, - agenticv1alpha1.AgenticRunPhaseDenied, - agenticv1alpha1.AgenticRunPhaseEscalated, - agenticv1alpha1.AgenticRunPhaseEmergencyStopped: - if !needsRevision(&run) { + case agenticv1alpha1.AgenticRunPhaseCompleted: + if !(run.Spec.Execution.IsZero() && needsRevision(&run)) { if hasSandboxClaims(&run) { if err := r.Agent.ReleaseSandboxes(ctx, &run); err != nil { log.Error(err, "sandbox cleanup failed at terminal phase") @@ -143,11 +140,25 @@ func (r *AgenticRunReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, nil } + case 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 r.Audit != nil { + r.Audit.EmitTerminalSpan(ctx, &run, string(phase), terminalReason(&run)) + r.Audit.Cleanup(&run) + } + return ctrl.Result{}, nil + case agenticv1alpha1.AgenticRunPhaseFailed: return r.handleFailed(ctx, &run) } - // --- Suspension guard (only non-terminal runs reach here) --- + // --- Suspension guard (non-terminal runs and advisory-only Completed runs needing revision reach here) --- suspended, err := isSuspended(ctx, r.Client) if err != nil { return ctrl.Result{}, err @@ -211,16 +222,18 @@ func (r *AgenticRunReconciler) Reconcile(ctx context.Context, req ctrl.Request) } return r.handleEscalation(ctx, &run, resolved, approval, policy) - case agenticv1alpha1.AgenticRunPhaseNoActionRequired, - agenticv1alpha1.AgenticRunPhaseCompleted, - agenticv1alpha1.AgenticRunPhaseDenied, - agenticv1alpha1.AgenticRunPhaseEscalated, - agenticv1alpha1.AgenticRunPhaseEmergencyStopped: + case agenticv1alpha1.AgenticRunPhaseNoActionRequired: if needsRevision(&run) { return r.handleRevision(ctx, &run, resolved) } return ctrl.Result{}, nil + case agenticv1alpha1.AgenticRunPhaseCompleted: + if run.Spec.Execution.IsZero() && needsRevision(&run) { + return r.handleRevision(ctx, &run, resolved) + } + return ctrl.Result{}, nil + default: log.V(1).Info("unhandled phase, no-op", LogKeyPhase, phase) return ctrl.Result{}, nil From d0a2f39e759e62df63280ee936cd7dbbe8e171fb Mon Sep 17 00:00:00 2001 From: Jakub Hadvig Date: Thu, 23 Jul 2026 21:46:52 +0200 Subject: [PATCH 4/4] Add Failed phase to revision path 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) --- controller/agenticrun/handlers_test.go | 50 ++++++++++++++++++++++++++ controller/agenticrun/reconciler.go | 7 ++-- 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/controller/agenticrun/handlers_test.go b/controller/agenticrun/handlers_test.go index cc57c7de..0b5c8bfa 100644 --- a/controller/agenticrun/handlers_test.go +++ b/controller/agenticrun/handlers_test.go @@ -736,6 +736,56 @@ func TestReconcile_RevisionFromCompleted(t *testing.T) { } } +func TestReconcile_RevisionFromFailed(t *testing.T) { + agent := newTestAgentCaller() + scheme := testScheme() + + // Advisory-only run + run := &agenticv1alpha1.AgenticRun{ + ObjectMeta: metav1.ObjectMeta{Name: "fix-crash", Namespace: "default"}, + Spec: agenticv1alpha1.AgenticRunSpec{ + Request: "Investigate issue", + Tools: testTools(), + TargetNamespaces: []string{"production"}, + Analysis: agenticv1alpha1.AgenticRunStep{Agent: "default"}, + }, + } + + objs := append([]client.Object{run}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...). + WithStatusSubresource(run, &agenticv1alpha1.AnalysisResult{}, &agenticv1alpha1.ExecutionResult{}, &agenticv1alpha1.VerificationResult{}, &agenticv1alpha1.EscalationResult{}).Build() + + r := &AgenticRunReconciler{Client: fc, Agent: agent, Namespace: "default"} + + // Make analysis fail + agent.analyzeErr = fmt.Errorf("LLM timeout") + + // Reconcile 1: Pending → Failed + if _, err := reconcileOnce(r, "fix-crash"); err != nil { + t.Fatalf("reconcile 1: %v", err) + } + p, _ := getAgenticRun(r, "fix-crash") + if agenticv1alpha1.DerivePhase(p.Status.Conditions) != agenticv1alpha1.AgenticRunPhaseFailed { + t.Fatalf("expected Failed, got %s", agenticv1alpha1.DerivePhase(p.Status.Conditions)) + } + + // Fix the agent and submit revision + agent.analyzeErr = nil + reviseAgenticRun(t, fc, "fix-crash", "retry after timeout") + + // Reconcile 2: Failed → revision → Proposed + if _, err := reconcileOnce(r, "fix-crash"); err != nil { + t.Fatalf("reconcile 2 (revision from Failed): %v", err) + } + p, _ = getAgenticRun(r, "fix-crash") + if agenticv1alpha1.DerivePhase(p.Status.Conditions) != agenticv1alpha1.AgenticRunPhaseProposed { + t.Fatalf("expected Proposed after revision from Failed, got %s", agenticv1alpha1.DerivePhase(p.Status.Conditions)) + } + if analyzed := meta.FindStatusCondition(p.Status.Conditions, agenticv1alpha1.AgenticRunConditionAnalyzed); analyzed == nil || analyzed.ObservedGeneration != p.Generation { + t.Fatalf("expected observedGeneration to equal current generation %d after revision from Failed", p.Generation) + } +} + func TestReconcile_ExecutionRBACCreatedOnApproval(t *testing.T) { agent := newTestAgentCaller() agent.analyzeResult = &AnalysisOutput{ diff --git a/controller/agenticrun/reconciler.go b/controller/agenticrun/reconciler.go index 502e5e91..5739435a 100644 --- a/controller/agenticrun/reconciler.go +++ b/controller/agenticrun/reconciler.go @@ -155,7 +155,9 @@ func (r *AgenticRunReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, nil case agenticv1alpha1.AgenticRunPhaseFailed: - return r.handleFailed(ctx, &run) + if !(run.Spec.Execution.IsZero() && needsRevision(&run)) { + return r.handleFailed(ctx, &run) + } } // --- Suspension guard (non-terminal runs and advisory-only Completed runs needing revision reach here) --- @@ -228,7 +230,8 @@ func (r *AgenticRunReconciler) Reconcile(ctx context.Context, req ctrl.Request) } return ctrl.Result{}, nil - case agenticv1alpha1.AgenticRunPhaseCompleted: + case agenticv1alpha1.AgenticRunPhaseCompleted, + agenticv1alpha1.AgenticRunPhaseFailed: if run.Spec.Execution.IsZero() && needsRevision(&run) { return r.handleRevision(ctx, &run, resolved) }