fix: backfill interrupted tool results so stopped sessions stay resumable - #150
Merged
Conversation
…able A session stopped mid-tool (user stop, fatal tool-node failure) persisted tool_call entries without matching tool_result entries; resuming rebuilt a history the model API rejects with 'assistant message with tool_calls must be followed by tool messages'. - runner: drain dangling announced tool calls on every early-exit path, recording an '[Interrupted before result was recorded]' result. The backfill carries no error — an interrupted call is not a failed call, so front-ends render a calm row instead of a red 'context.Canceled' card. - session: ReconstructState repairs any remaining dangling tool_calls with placeholder tool messages (covers process kills and older recordings), and no longer skips entries inside subagent start/result windows — the main agent's parallel tool results land there and were being swallowed, leaving dangling calls (session-86e09b12). - web: /stop no longer emits its own agent_done (the runner reports the single context.Canceled); the web handler maps cancellation to a stopped:true payload and the timeline shows a muted 'Stopped by user' notice instead of a red error card.
📝 WalkthroughWalkthroughCancellation now produces a single stopped completion event, backfills missing tool results, preserves tool results across subagent markers, repairs reconstructed histories, and renders user stops as notices instead of errors. ChangesCancellation and tool-history lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant handleStop
participant runInner
participant WebHandler
participant WebSocket
participant chatStore
User->>handleStop: request stop
handleStop->>runInner: cancel context
runInner->>runInner: backfill pending tool result
runInner->>WebHandler: OnAgentDone(context.Canceled)
WebHandler->>WebSocket: agent_done stopped payload
WebSocket->>chatStore: dispatch stopped state
chatStore->>chatStore: append notice
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/runner/cancel_backfill_test.go (1)
156-163: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the interruption-specific result contract.
The test only checks count and ID, so it still passes if the framework emits
context.Canceledas the tool error—the red-row regression this PR intends to prevent. Assert the placeholder output,Err == nil, and preserved tool name.Proposed assertions
if h.results[0].ToolCallID != "call-block-1" { t.Errorf("result ToolCallID = %q, want call-block-1", h.results[0].ToolCallID) } +if h.results[0].Name != "block" { + t.Errorf("result Name = %q, want block", h.results[0].Name) +} +if h.results[0].Output != session.InterruptedToolOutput { + t.Errorf("result Output = %q, want %q", h.results[0].Output, session.InterruptedToolOutput) +} +if h.results[0].Err != nil { + t.Errorf("result Err = %v, want nil", h.results[0].Err) +}🤖 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/runner/cancel_backfill_test.go` around lines 156 - 163, Strengthen the assertions in the result validation for the announced call so it verifies the interruption-specific contract: assert the placeholder output, require Err to be nil, and confirm the original tool name is preserved, in addition to the existing count and ToolCallID checks.
🤖 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.
Nitpick comments:
In `@internal/runner/cancel_backfill_test.go`:
- Around line 156-163: Strengthen the assertions in the result validation for
the announced call so it verifies the interruption-specific contract: assert the
placeholder output, require Err to be nil, and confirm the original tool name is
preserved, in addition to the existing count and ToolCallID checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05bf867a-5c6f-4789-a7e1-092758ddee94
📒 Files selected for processing (9)
internal/handler/web.gointernal/runner/cancel_backfill_test.gointernal/runner/runner.gointernal/session/history.gointernal/session/history_test.gointernal/web/chat.goweb/src/app/store.tsweb/src/app/wsBridge.tsweb/src/lib/ws.ts
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.
Problem
Stopping a run mid-tool (or a fatal tool-node failure) persisted
tool_callentries without matchingtool_resultentries. Resuming such a session rebuilt a history the model API rejects:A second, related hole:
ReconstructStateskipped every entry betweensubagent_startandsubagent_result. When the main agent issued a parallel batch (e.g. grep + subagent), the fast tool's result landed inside that window and was swallowed — again leaving a dangling tool_call on resume (session-86e09b12,tool_call_ids did not have response messages: grep:53).On the web,
/stopadditionally emitted its ownagent_done("stopped by user")that raced the runner's terminal event, producing a red error card for a deliberate user action.Changes
internal/runner/runner.go):drainDanglingToolResultsbackfills a result for every announced tool call that never produced one, on every early-exit path (user stop ×2, fatal event error). The backfill carries no error — an interrupted call is not a failed call, so front-ends render a calm row instead of a redcontext.Canceledcard next to the stop notice. Tool-start tracking now also records the tool name so the backfill can attribute results correctly.internal/session/history.go):ReconstructStaterepairs any remaining dangling tool_calls with placeholder tool messages (covers process kills and older recordings), exported asInterruptedToolOutputand shared with the runner. The subagent start/result/async markers are now informational-only — subagent-internal calls are never persisted, so entries in that window are the main agent's own parallel results and must be kept.internal/web/chat.go,internal/handler/web.go):/stopno longer double-reports; the runner owns the singleOnAgentDone(context.Canceled), which the web handler maps toagent_done{stopped:true}.store.ts,wsBridge.ts,ws.ts): a stopped run shows a muted "Stopped by user" notice instead of a red error card.Tests
internal/runner/cancel_backfill_test.go: cancels a run mid-tool via a blocking tool; asserts exactly one result reaches the handler, the session pairs call/result on disk, and the reconstructed history satisfies the tool-call invariant.internal/session/history_test.go: invariant assertion helper + repro of the session-86e09b12 parallel-window failure, interrupted-call backfill, and backfill positioning when later turns exist.Verification
go test ./...— all passmake lint— golangci-lint 0 issues; web + packages typecheck cleanSummary by CodeRabbit
New Features
Bug Fixes
Tests