fix(runner): preserve structured live turn history - #183
Conversation
|
Warning Review limit reached
Next review available in: 3 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe runner now returns flattened response text and structured assistant/tool messages. Command and web callers append the full message history. Session recording returns persisted tool output. Tests cover replay, parallel tools, truncation, and cancellation. ChangesStructured runner history
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Runner
participant Model
participant Session
Caller->>Runner: Submit run request
Runner->>Model: Generate response and tool calls
Model-->>Runner: Assistant text and structured calls
Runner->>Session: Record tool result
Session-->>Runner: Return persisted tool output
Runner-->>Caller: Return RunResult
Caller->>Caller: Append returned messages to history
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.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/session/session.go (1)
580-596: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReturn recorder write failures to the runner.
RecordToolResultdiscards ther.writeEntryerror and returnsoutputas if it were persisted. If session storage fails,internal/runner/runner.gostill appends that output to live history, but session replay lacks the tool result. A later resume can then contain an unmatched assistant tool call.Return a wrapped persistence error that includes the session path and
toolCallID. Propagate it through the runner completion path instead of claiming that the output was stored.As per coding guidelines, “Wrap non-tool errors with
fmt.Errorf("context: %w", err)and return descriptive errors containing relevant paths, line numbers, or command output.”🤖 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/session/session.go` around lines 580 - 596, The Recorder.RecordToolResult method must surface persistence failures instead of discarding r.writeEntry errors. Update its return contract to return the output and an error, wrapping write failures with session-path and toolCallID context, then propagate that error through the runner’s tool-completion path so live history is not updated as though the result was persisted.Source: Coding guidelines
🧹 Nitpick comments (2)
internal/runner/cancel_backfill_test.go (1)
223-231: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCompare cancellation results with session replay.
The backfill test validates only the result ID. The incomplete-call test validates only that tool entries are absent. The recorder can store different backfill content or omit
"partial", and both tests still pass. Compare reconstructed history with the matching liveRunResult.Messagesin both tests.
assertMessagesEqualalready provides this check ininternal/runner/history_result_test.goLines 261-271.Proposed test change
state := session.ReconstructState(entries) + assertMessagesEqual(t, state.History, outcome.result.Messages) for i, m := range state.History { // ... } for _, entry := range entries { // ... } + assertMessagesEqual(t, session.ReconstructState(entries).History, result.Messages)Also applies to: 313-321
🤖 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 223 - 231, Update both cancellation test assertions around the live history checks and the incomplete-call case to compare reconstructed history against the matching live RunResult.Messages using the existing assertMessagesEqual helper. Keep the current structural assertions where useful, but ensure the comparisons validate complete message content, including backfill fields such as “partial”, rather than only IDs, roles, or tool-entry absence.internal/runner/history_result_test.go (1)
190-201: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert tool-result pairing with the emitted tool calls.
The test checks the tool-call count, but it does not prove that each tool result answers a tool call in
result.Messages[0]. A transcript with mismatched call IDs can pass this test. BuildwantIDsfrom the assistant tool calls, then consume each tool-result ID from that set.Proposed test change
- wantIDs := map[string]bool{"call-a": true, "call-b": true} + wantIDs := make(map[string]bool, len(result.Messages[0].ToolCalls)) + for _, toolCall := range result.Messages[0].ToolCalls { + if toolCall.ID == "" || wantIDs[toolCall.ID] { + t.Fatalf("assistant tool calls = %#v, want two unique IDs", result.Messages[0].ToolCalls) + } + wantIDs[toolCall.ID] = true + }🤖 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/history_result_test.go` around lines 190 - 201, Update the assertions around the first assistant message to build wantIDs from its emitted ToolCalls rather than hard-coding call-a and call-b. Continue consuming each ToolCallID from result.Messages[1:3], rejecting IDs not present in the assistant calls and ensuring no expected call IDs remain.
🤖 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/command/interactive.go`:
- Around line 618-623: Update the plan-completion flow around
handlePlanCompletion and both runner.Run result-handling sites to detect
cancellation before submitting or approving a plan. Use the existing session
context cancellation state, or propagate an explicit cancellation indicator
through RunResult, and return without waiting on planRespCh when the run was
canceled; preserve normal plan approval for non-canceled runs.
In `@internal/runner/cancel_backfill_test.go`:
- Around line 294-295: Bound the cancellation test around runInner by executing
it in a goroutine that sends its result through a buffered channel, and add a
short local context deadline or timeout select. Update the test to fail promptly
if the handler is not invoked or runInner does not observe cancellation, while
preserving the existing result and completion assertions.
In `@internal/runner/runner.go`:
- Around line 312-320: Update the tool-stream error path around emitToolResult
and MessageStream.Recv so a receive failure before the first chunk reuses the
announced tool-call ID instead of an empty toolCallID when recording and
appending the result. Add a test covering first-receive failure and verify the
handler/session output remains paired with the announced ID.
In `@internal/web/chat.go`:
- Around line 315-320: Ensure result messages committed around runner.Run are
serialized in submit order, not merely protected by eng.emu; use the engine’s
existing sequencing mechanism or add per-turn ordering so an older run cannot
append after a newer submission or result. Update the append path near
eng.history and ensure ordering remains correct even when runner.Run calls
finish out of order; do not rely on the runGen cleanup check.
---
Outside diff comments:
In `@internal/session/session.go`:
- Around line 580-596: The Recorder.RecordToolResult method must surface
persistence failures instead of discarding r.writeEntry errors. Update its
return contract to return the output and an error, wrapping write failures with
session-path and toolCallID context, then propagate that error through the
runner’s tool-completion path so live history is not updated as though the
result was persisted.
---
Nitpick comments:
In `@internal/runner/cancel_backfill_test.go`:
- Around line 223-231: Update both cancellation test assertions around the live
history checks and the incomplete-call case to compare reconstructed history
against the matching live RunResult.Messages using the existing
assertMessagesEqual helper. Keep the current structural assertions where useful,
but ensure the comparisons validate complete message content, including backfill
fields such as “partial”, rather than only IDs, roles, or tool-entry absence.
In `@internal/runner/history_result_test.go`:
- Around line 190-201: Update the assertions around the first assistant message
to build wantIDs from its emitted ToolCalls rather than hard-coding call-a and
call-b. Continue consuming each ToolCallID from result.Messages[1:3], rejecting
IDs not present in the assistant calls and ensuring no expected call IDs remain.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 926f148e-ec2c-4697-a456-078def3774a3
📒 Files selected for processing (7)
internal/command/acp.gointernal/command/interactive.gointernal/runner/cancel_backfill_test.gointernal/runner/history_result_test.gointernal/runner/runner.gointernal/session/session.gointernal/web/chat.go
| result := runner.Run(hookCtx, agent, history, eng.eventHandler, recorder, eng.todoStore, eng.env.GoalStore, s.tracer, eng.tokenUsage) | ||
| if len(result.Messages) > 0 { | ||
| eng.emu.Lock() | ||
| eng.history = append(eng.history, &schema.Message{Role: schema.Assistant, Content: resp}) | ||
| eng.history = append(eng.history, result.Messages...) | ||
| eng.emu.Unlock() | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve Web turn order when committing result messages.
When a newer submitMessage call starts before an older runner.Run call returns, both goroutines append to eng.history when they finish. The eng.emu lock protects the slice but does not preserve submission order. An older result can appear after a newer user message or after a newer result. The next turn can then receive an invalid transcript. Serialize runs per engine or commit per-turn results in sequence order. The runGen cleanup check does not protect this append.
🤖 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/web/chat.go` around lines 315 - 320, Ensure result messages
committed around runner.Run are serialized in submit order, not merely protected
by eng.emu; use the engine’s existing sequencing mechanism or add per-turn
ordering so an older run cannot append after a newer submission or result.
Update the append path near eng.history and ensure ordering remains correct even
when runner.Run calls finish out of order; do not rely on the runGen cleanup
check.
![jcode-cloud-app[bot]](https://avatars.githubusercontent.com/in/4395162?s=60&v=4){kind=small}
There was a problem hiding this comment.
jcode review
Clean. The PR correctly restructures runner.Run/runInner to return a RunResult carrying the structured assistant+tool transcript, updates all four production call sites (acp.go, interactive.go x3, web/chat.go) consistently, and wires RecordToolResult's returned (truncated) output back into live history so live and replayed sessions match. The incomplete-streaming-tool-call handling on stream error is sound (announced tool calls only happen after a successful stream flush, so a mid-stream error leaves no dangling calls and text-only partial content is preserved), and the cancellation/backfill path preserves the assistant-tool-call -> tool-result invariant required by model APIs. gofmt and go vet are clean; go build ./... and the runner/session/command/web test packages all pass. No high-confidence defects introduced.
0 findings
Checks performed
- Read full diff at .git/jcode-review.diff
- Read internal/runner/runner.go (Run, runInner, streaming/non-streaming assistant + tool paths)
- Read internal/session/history.go ReconstructState/repairDanglingToolCalls
- Read internal/session/truncate.go TruncateToolOutput and RecordToolResult return wiring
- Read internal/agent/history.go SyncSummarization/findToolBoundary interaction
- Grepped all runner.Run/runInner callers (6 production + tests, all updated)
- Inspected acp.go Prompt, interactive.go handlePrompt/handlePlanCompletion/runEventLoop, web/chat.go submitMessage call sites
- go generate ./internal/model/... (registry_generated.go)
- Created internal/web/dist stub; go build ./... OK
- go test ./internal/runner/... ./internal/session/... ./internal/command/... ./internal/web/... all pass
- gofmt -l and go vet on changed files clean
jcode posts a non-blocking COMMENT review. Merge decisions remain with your team.
There was a problem hiding this comment.
jcode review
Clean. The PR refactors runner.Run/runInner to return a structured RunResult (Response + Messages + Err) instead of a flattened string, and threads structured assistant/tool messages into live history across the TUI, ACP, and Web callers. The full module builds, go vet is clean, and all tests pass for internal/runner, internal/session, internal/command, internal/agent, internal/handler, and internal/tools. All runInner return paths route through the finish() helper so Response/Err are always populated; the new errors.Join result is consumed via errors.Is in every handler; the caller's history slice is copied before continuation mutation (fixing a prior aliasing bug); incomplete streaming tool calls are correctly dropped; persistence failures append the replay-equivalent InterruptedOutput and surface a non-nil Err so plan submission is skipped on interruption. No correctness, security, reliability, or data-loss defect was verified with >=80% confidence.
0 findings
Checks performed
- go build ./... (full module, clean)
- go test ./internal/runner/... -count=1 (pass)
- go test ./internal/session/... -count=1 (pass)
- go test ./internal/command/... -run Plan (pass)
- go test ./internal/agent/... ./internal/handler/... ./internal/tools/... (pass)
- go vet on runner/session/command/web (clean)
- inspected runInner return paths all use finish()
- verified OnAgentDone errors.Is handling in web/acp handlers
- verified Run copies caller history before continuation append
- single RecordToolResult caller handles new (string,error) return
- read full diff incl. history_result_test.go, review_feedback_test.go, cancel_backfill_test.go
jcode posts a non-blocking COMMENT review. Merge decisions remain with your team.
1 participant
Summary
RunResult{Response, Messages}Design
Responseremains the flattened assistant text used by trace output and plan completion.Messagescontains the persistable assistant/tool delta for the turn. Internal continuation prompts remain transient, matching current session replay semantics.All transports now append
Messagesinstead of synthesizing one assistant message from a possibly empty string.Acceptance coverage
session.ReconstructStateVerification
go test ./...golangci-lint run --new-from-rev=origin/main— 0 issuesmake lint-webgit diff --checkFull
make lintremains blocked by 44 pre-existingSA5011findings in tests on the base branch; this PR introduces no new lint findings.Summary by CodeRabbit
Bug Fixes
Tests