fix(agent): prevent plan execution from stalling after update_plan#670
fix(agent): prevent plan execution from stalling after update_plan#670gnanam1990 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (12)
WalkthroughThe agent now applies completion gating to pending plans, stops non-productive stalled runs as incomplete, and propagates incomplete status to the TUI and session transcripts. Tests cover nudges, cancellation, max turns, reasoning-only turns, and unfinished plan rendering. ChangesPlan-aware run completion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Provider
participant AgentLoop
participant GuardState
participant TUI
participant SessionStore
Provider->>AgentLoop: Emits a provider turn
AgentLoop->>GuardState: Observes turn and pending plan items
GuardState-->>AgentLoop: Requests continuation or stops incomplete
AgentLoop->>TUI: Sends incomplete result and reason
TUI->>TUI: Marks active steps failed and preserves pending steps
TUI->>SessionStore: Records run_incomplete event
SessionStore-->>TUI: Hydrates incomplete system transcript row
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/tui/model_test.go (1)
1354-1377: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTighten the assertion to lock in "active step only" semantics.
The test only checks that some step is
failed; it wouldn't catch a regression where the pending stepcis also markedfailed(or completed), contradicting the stated "mark the active step failed without completing pending steps" behavior.♻️ Suggested tightened assertion
next := updated.(model) if next.plan.isComplete() { t.Fatal("incomplete turn must not force-complete the plan") } - foundFailed := false - for _, step := range next.plan.steps { - if step.status == "failed" { - foundFailed = true - } - } - if !foundFailed { - t.Fatalf("expected a failed plan step, got %+v", next.plan.steps) - } + if next.plan.steps[1].status != "failed" { + t.Fatalf("expected the active step to be marked failed, got %+v", next.plan.steps) + } + if next.plan.steps[2].status != "pending" { + t.Fatalf("expected the remaining pending step untouched, got %+v", next.plan.steps) + }🤖 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 `@internal/tui/model_test.go` around lines 1354 - 1377, Strengthen the assertions in the “incomplete turn marks the active plan step failed” test to verify only the active step is failed. Confirm the active step’s status is failed, while pending step c remains pending and is not completed or failed; retain the existing assertion that the plan is not complete.
🤖 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 `@internal/tui/model.go`:
- Around line 5178-5200: Update the incomplete-run handling in the
result-processing flow to persist a distinct replay event instead of
sessions.EventError. Use the existing dedicated incomplete-run event type or add
the appropriate separate tag, while preserving the current message payload and
transcript notice so resume replay represents the run as incomplete rather than
failed.
In `@internal/tui/plan_panel.go`:
- Around line 169-192: Update markIncompleteRemaining to set
planPanelState.completedAt to now when, after marking active steps failed, no
pending steps remain and the plan is complete; preserve it unset when pending
work remains. Extend TestPlanMarkIncompleteRemaining with coverage for the
no-pending terminal path.
---
Nitpick comments:
In `@internal/tui/model_test.go`:
- Around line 1354-1377: Strengthen the assertions in the “incomplete turn marks
the active plan step failed” test to verify only the active step is failed.
Confirm the active step’s status is failed, while pending step c remains pending
and is not completed or failed; retain the existing assertion that the plan is
not complete.
🪄 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: Pro
Run ID: 88b11d69-d0a6-45dd-bf2d-789ebd5fc856
📒 Files selected for processing (9)
internal/agent/completion_gate_test.gointernal/agent/guardrails.gointernal/agent/guardrails_test.gointernal/agent/loop.gointernal/agent/types.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/plan_panel.gointernal/tui/plan_panel_progress_test.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Preserve pending-plan incompleteness at the max-turns exit
internal/agent/loop.go:739
The new plan-aware gate applies to interactive runs even whenRequireCompletionSignalis false, but both max-turns exits still setResult.Incompleteonly when that option is true. A TUI run can therefore repeatedly make tool calls (includingupdate_planwith an in-progress item) untilMaxTurns, receive the forced final summary, and be treated as successful;model.updateModelthen callscompleteRemainingand marks every untouched step complete. This recreates the false-completion state the PR is fixing. Mark a max-turn cutoff incomplete when pending plan items remain as well, and assert the result/TUI state in the repeated-plan regression. -
[P2] Keep the incomplete-run event in resumed prompt context
internal/sessions/store.go:40
EventRunIncompleteis rendered on TUI resume, but the session prompt filter inexec_session.godoes not retain this new event type. Consequently, the next TUI orexec --resumeagent invocation receives the final assistant message but not the reason that its prior run stopped unfinished, so it can reason from an apparent successful completion. Include the new event inpromptContextEventsand cover resumed/forked prompt construction. -
[P2] Persist the final answer before its incomplete notice
internal/tui/model.go:5184
Live rendering appends the assistant's final answer and then theRun incompletenotice, but persistence appendsEventRunIncompletebeforeEventMessage. Session hydration replays events in stored order, so reopening the session reverses the visible chronology and presents the warning before the answer it qualifies. Store the assistant message first (or otherwise preserve the live ordering) and add an ordered replay test.
Apply the plan-aware completion gate to interactive runs while update_plan still has pending items, count reasoning-only turns as non-productive during an active plan, surface Incomplete results in the TUI, and mark the active plan step failed instead of force-completing a stalled run.
Use EventRunIncomplete instead of EventError for stalled plan runs so resume replays a system notice rather than a hard error. Stamp completedAt when markIncompleteRemaining leaves a terminal plan, tighten incomplete plan assertions, and add hydration coverage.
gnanam1990
force-pushed
the
fix/issue-666-plan-stall
branch
from
e0baf80 to
f009491
Compare
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Preserve pending-plan incompleteness at the max-turns exit
internal/agent/loop.go:739
The new plan-aware gate applies to interactive/TUI runs even whenRequireCompletionSignalis false, but both max-turns exits still setResult.Incompleteonly when that option is true. A TUI run can keep making tool calls (includingupdate_planwith an in-progress item) untilMaxTurns, receive the forced final summary, and be treated as successful;model.updateModelthen callscompleteRemainingand marks every untouched step complete. This recreates the false-completion state this PR is intended to prevent. Mark a max-turn cutoff incomplete when pending plan items remain as well, and assert the result/TUI state in the repeated-plan regression. -
[P1] Derive the completion gate from a successfully applied plan
internal/agent/guardrails.go:541
observeTurnupdatesplanItemsPendingfrom rawupdate_planarguments before the tool is executed. For example, with an existing pending TUI plan, a call such as{"plan":[{"status":"completed"}]}clears the guard count, butupdate_planrejects the missingcontentand leaves the actual plan unchanged. The next text turn bypasses the new interactive completion gate andcompleteRemainingfalsely completes that still-pending panel. This also diverges from the dispatcher’s concatenated-JSON recovery path. Only replace guard plan state after a successful, normalized update (or retain the prior state on a failed call), and cover the failed-update regression. -
[P2] Keep the incomplete-run event in resumed prompt context
internal/sessions/exec_session.go:194
EventRunIncompleteis rendered on TUI resume, but the session prompt filter does not retain the new event type. Consequently, the next TUI orexec --resumeinvocation receives the final assistant message but not the reason that its prior run stopped unfinished, so it can reason from an apparent successful completion. Include the event inpromptContextEventsand cover resumed/forked prompt construction. -
[P2] Persist the final answer before its incomplete notice
internal/tui/model.go:5184
Live rendering appends the assistant's final answer and then theRun incompletenotice, but persistence appendsEventRunIncompletebeforeEventMessage. Session hydration replays events in stored order, so reopening the session reverses the visible chronology and presents the warning before the answer it qualifies. Store the assistant message first (or otherwise preserve the live ordering) and add an ordered replay test.
2 participants
Summary
Fixes an agent-loop failure where Zero could remain indefinitely on the first active plan step after
update_plan.When a model returned continuation-cue text or reasoning-only responses without productive tool calls, interactive runs could accept the response as complete or repeatedly consume turns without reporting a visible failure.
Root cause
RequireCompletionSignalwas explicitly enabled.update_planstate, leaving the first step active.Fix
Tests
Added regression coverage for:
RequireCompletionSignal,update_planhitting max turns,Verification
go fmt ./...go vet ./...go test ./...make buildgo run golang.org/x/vuln/cmd/govulncheck@v1.3.0 ./...Lint comparison (base
80c39aavs fix6360909)golangci-lintwith--enable-only unused,ineffassign,staticcheckreports 36 issues on both base and fix branches. No new failures in changed production files (internal/agent/loop.go,internal/agent/guardrails.go,internal/tui/plan_panel.go,internal/tui/model.go). Pre-existing failures remain in unrelated packages (internal/sandbox,internal/tuihelpers,internal/cli, etc.).Smoke test
Live HY3 smoke test not performed — no HY3/OpenGateway credentials configured locally (active provider is
xai). Deterministic fake-provider tests are the primary reproduction.Fixes #666
Summary by CodeRabbit