fix: correlate tool calls by the SDK's toolUseID and keep subagent text out of canonical history - #118
Conversation
…xt out of canonical history Two related provider-layer correctness bugs: 1. Tool-call correlation (#81): canUseTool reconstructed the tool_use_id from a name-keyed FIFO fed by the PreToolUse hook. The SDK skips canUseTool for auto-allowed tools (allowedTools / project permissions.allow), so any allow rule left a stale queue head that the next gated call popped — mis-correlating every subsequent tool call — and entries for always-allowed tools grew the map unboundedly. The SDK passes the real toolUseID (and agentID) to canUseTool directly; use them and delete the FIFO entirely. 2. Canonical history corruption (#82): subagent assistant text was emitted as primary text, and CanonicalHistoryAccumulator assigned each text_done, keeping only the last block of a turn. Text/thinking provider events now carry parentToolUseId; Session and the accumulator drop non-primary text, and text_done blocks append across a turn so interleaved commentary (text → tool → text) survives provider switches. Fixes #81 Fixes #82 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughClaude tool-call correlation now uses SDK identifiers directly, and provider events carry subagent correlation metadata. Canonical history and session streaming now ignore subagent text/thinking while preserving multi-segment assistant text, with expanded tests covering the new behavior. ChangesSubagent/tool correlation and history isolation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #118 +/- ##
==========================================
+ Coverage 77.47% 77.49% +0.01%
==========================================
Files 85 86 +1
Lines 13396 13403 +7
==========================================
+ Hits 10379 10386 +7
Misses 3017 3017
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/daemon/providers/canonical.ts (1)
268-284: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSubagent filter duplicated across
canonical.tsandsession.ts.Both
CanonicalHistoryAccumulator.handleEventandSession#handleProviderEventindependently re-implementevent.parentToolUseId != nullto drop subagent text/thinking. SinceSessionalready filters these events before they ever reach#accumulator.handleEvent(persrc/daemon/session.tslines 2009-2015), the check here exists purely to protect standalone callers (e.g.provider-switch.test.ts). Consider extracting a sharedisSubagentEvent(event)helper (or aparentToolUseIdtype guard) so both call sites can't drift independently as more subagent-aware event types are added.🤖 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 `@src/daemon/providers/canonical.ts` around lines 268 - 284, The subagent-event filtering logic is duplicated in CanonicalHistoryAccumulator.handleEvent and Session#handleProviderEvent, both checking event.parentToolUseId != null separately. Extract a shared helper such as isSubagentEvent(event) or a parentToolUseId type guard in canonical.ts/session.ts and use it from both call sites so the filtering stays consistent for text_done, thinking_delta, and any future subagent-aware event types, while preserving the standalone accumulator behavior used by tests like provider-switch.test.ts.
🤖 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 `@src/daemon/providers/canonical.ts`:
- Around line 268-284: The subagent-event filtering logic is duplicated in
CanonicalHistoryAccumulator.handleEvent and Session#handleProviderEvent, both
checking event.parentToolUseId != null separately. Extract a shared helper such
as isSubagentEvent(event) or a parentToolUseId type guard in
canonical.ts/session.ts and use it from both call sites so the filtering stays
consistent for text_done, thinking_delta, and any future subagent-aware event
types, while preserving the standalone accumulator behavior used by tests like
provider-switch.test.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 50a2561d-6d11-48a1-96b1-09f307dd23d4
📒 Files selected for processing (7)
src/daemon/providers/canonical.tssrc/daemon/providers/claude/index.tssrc/daemon/providers/interface.tssrc/daemon/session.tssrc/tests/provider-claude.test.tssrc/tests/provider-switch.test.tssrc/tests/session-stream-commit.test.ts
The subagent text/thinking filter was re-implemented in both CanonicalHistoryAccumulator.handleEvent and Session#handleProviderEvent. Centralize it as isSubagentEvent in providers/interface.ts so the two call sites can't drift as new subagent-aware event types are added. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Fixes the two High-severity provider-layer correctness bugs from the July audit — both in the Claude provider's event pipeline, so they ship as one PR.
#81 — PreToolUse↔canUseTool FIFO desync
canUseToolreconstructed thetool_use_idby popping a name-keyed FIFO that the PreToolUse hook pushed for every tool call. The SDK skipscanUseToolfor auto-allowed tools (allowedTools, projectpermissions.allow), so any allow rule left a stale queue head. The next gated call popped the wrong entry, mis-correlating every later tool call in the session (tool_startwith the wrongsdkToolUseId→ orphanedtool_complete→ tools stuck "running" forever, output attached to the wrong message). Entries for always-allowed tools were never popped, so the map also grew unboundedly.The SDK passes the real
toolUseID(andagentID) directly tocanUseTool— this PR uses those and deletes the FIFO entirely. No queue, no desync, no unbounded growth.#82 — cross-provider canonical history corruption
Two halves:
text_donecould even clobber the primary message mid-stream).CanonicalHistoryAccumulatorassigned eachtext_done, so a normal agentic turn (text → tool → text → final text) kept only the last block — all interleaved reasoning vanished from the history handed to the next provider on a model switch.Text/thinking
ProviderEvents now carryparentToolUseId(from the SDK'sparent_tool_use_id). Session and the accumulator drop non-primary text/thinking, andtext_doneblocks append across the turn (reset at turn boundaries). The subagent's work still surfaces via its tool_call messages and the spawning Task tool's result.Tests
provider-claude.test.ts:canUseToolcorrelates by the SDK-provided id; regression repro for fix: PreToolUse↔canUseTool FIFO desync corrupts tool-call correlation when tools are auto-allowed #81 (auto-allowed PreToolUse followed by a gated call of the same tool name correlates to its own id); deny when notoolUseID; translate-layer tagging for assistant + stream_event messages (primary and subagent).provider-switch.test.ts: interleaved text → tool → text concatenates; subagent text/thinking dropped from canonical history; accumulation resets across turns.session-stream-commit.test.ts(C8): subagenttext_delta/text_done/thinking mid-stream never touch the primary message, thinking stream, or scrollback.bun x tsc --noEmit✅ ·bun run lint✅ ·bun run test1018 pass / 0 fail ✅Fixes #81
Fixes #82
🤖 Generated with Claude Code
Summary by CodeRabbit
text_donecontent instead of overwriting.