Skip to content

OLS-3719, OTA-2090: Check needsRevision() in terminal phases before early return #366

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jhadvig wants to merge 4 commits into
base: main
Choose a base branch
from
+130 −4
Open

OLS-3719, OTA-2090: Check needsRevision() in terminal phases before early return #366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1bee693
OTA-2090: Check needsRevision() in terminal phases before early return
jhadvig Jul 23, 2026
6180ad2
Add test for revision from Completed phase
jhadvig Jul 23, 2026
7f38a67
Address review: narrow revision to Completed phase only
jhadvig Jul 23, 2026
d0a2f39
Add Failed phase to revision path
jhadvig Jul 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 104 additions & 0 deletions controller/agenticrun/handlers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,110 @@ 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 3: Completed → revision → Proposed
if _, err := reconcileOnce(r, "fix-crash"); err != nil {
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 != p.Generation {
t.Fatalf("expected observedGeneration to equal current generation %d after revision from Completed", p.Generation)
}
}

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{
Expand Down
30 changes: 26 additions & 4 deletions controller/agenticrun/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,21 @@ func (r *AgenticRunReconciler) Reconcile(ctx context.Context, req ctrl.Request)
return ctrl.Result{}, nil
}

case agenticv1alpha1.AgenticRunPhaseCompleted,
agenticv1alpha1.AgenticRunPhaseDenied,
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")
}
}
if r.Audit != nil {
r.Audit.EmitTerminalSpan(ctx, &run, string(phase), terminalReason(&run))
r.Audit.Cleanup(&run)
}
return ctrl.Result{}, nil
}

case agenticv1alpha1.AgenticRunPhaseDenied,
agenticv1alpha1.AgenticRunPhaseEscalated,
agenticv1alpha1.AgenticRunPhaseEmergencyStopped:
if hasSandboxClaims(&run) {
Expand All @@ -142,10 +155,12 @@ 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)
}
Comment on lines 157 to +160

@harche harche Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) ---

@harche harche Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

suspended, err := isSuspended(ctx, r.Client)
if err != nil {
return ctrl.Result{}, err
Expand Down Expand Up @@ -215,6 +230,13 @@ func (r *AgenticRunReconciler) Reconcile(ctx context.Context, req ctrl.Request)
}
return ctrl.Result{}, nil

case agenticv1alpha1.AgenticRunPhaseCompleted,
agenticv1alpha1.AgenticRunPhaseFailed:
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
Expand Down