From c3107ae44b338626046592a7557b6eda227e43fc Mon Sep 17 00:00:00 2001 From: Yash Datta Date: Mon, 6 Jul 2026 16:43:43 +0800 Subject: [PATCH 1/2] fix: correlate tool calls by the SDK's toolUseID and keep subagent text out of canonical history MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/daemon/providers/canonical.ts | 12 +- src/daemon/providers/claude/index.ts | 60 ++++----- src/daemon/providers/interface.ts | 15 ++- src/daemon/session.ts | 13 ++ src/tests/provider-claude.test.ts | 155 +++++++++++++++++++++--- src/tests/provider-switch.test.ts | 63 ++++++++++ src/tests/session-stream-commit.test.ts | 36 ++++++ 7 files changed, 301 insertions(+), 53 deletions(-) diff --git a/src/daemon/providers/canonical.ts b/src/daemon/providers/canonical.ts index f7d2d36..07ffac8 100644 --- a/src/daemon/providers/canonical.ts +++ b/src/daemon/providers/canonical.ts @@ -266,10 +266,20 @@ export class CanonicalHistoryAccumulator { handleEvent(event: ProviderEvent): void { switch (event.type) { case "text_done": - this.#currentText = event.content; + // Subagent text (parentToolUseId set) is not primary conversation + // content — recording it would corrupt cross-provider history (#82). + if (event.parentToolUseId != null) break; + // A turn can span several assistant messages (text → tool → text → + // final text); each fires its own text_done. Append every block — + // assigning would keep only the last one and drop all interleaved + // reasoning from the canonical history (#82). + this.#currentText = this.#currentText + ? `${this.#currentText}\n\n${event.content}` + : event.content; break; case "thinking_delta": + if (event.parentToolUseId != null) break; this.#currentThinking += event.content; break; diff --git a/src/daemon/providers/claude/index.ts b/src/daemon/providers/claude/index.ts index 999a3b3..9f2fa95 100644 --- a/src/daemon/providers/claude/index.ts +++ b/src/daemon/providers/claude/index.ts @@ -97,10 +97,6 @@ export class ClaudeProvider implements SessionProvider { #currentCanUseTool: TurnOpts["canUseTool"] | null = null; #currentSender: AuthContext | null = null; - // PreToolUse hook data — queued by hook, consumed by canUseTool - // Maps tool_name → queue of { toolUseId, agentId } for FIFO matching - #pendingToolUse: Map> = new Map(); - #init: ClaudeProviderInit; constructor(init: ClaudeProviderInit) { @@ -127,7 +123,6 @@ export class ClaudeProvider implements SessionProvider { this.#hasQueried = false; this.#backingRecoveryAttempted = false; this.#lastPushedContent = null; - this.#pendingToolUse.clear(); } // ── AgentProvider interface ─────────────────────────────────────────────── @@ -190,7 +185,6 @@ export class ClaudeProvider implements SessionProvider { } async teardown(): Promise { - this.#pendingToolUse.clear(); // clear before closing so stale entries don't survive a model switch this.#inputQueue?.close(); this.#abortController?.abort(); if (this.#consumerTask) { @@ -290,20 +284,13 @@ export class ClaudeProvider implements SessionProvider { hooks: { PreToolUse: [{ hooks: [async (rawInput) => { - const input = rawInput as PreToolUseHookInput & { agent_id?: string }; + const input = rawInput as PreToolUseHookInput; init.store.audit( this.#currentSender?.sub ?? "unknown", "session.tool_call", sessionId, `tool=${input.tool_name}`, ); - // Capture tool_use_id + agent_id so canUseTool can correlate them. - if (input.tool_use_id) { - const entry = { toolUseId: input.tool_use_id, agentId: input.agent_id }; - const queue = this.#pendingToolUse.get(input.tool_name) ?? []; - queue.push(entry); - this.#pendingToolUse.set(input.tool_name, queue); - } // Compression rewrite. if (init.config && init.compressionRegistry) { const rewritten = rewriteBashToolInput({ @@ -345,21 +332,22 @@ export class ClaudeProvider implements SessionProvider { }], }, - canUseTool: async (toolName, input) => { + canUseTool: async (toolName, input, options) => { const toolId = randomUUID(); const approvalId = randomUUID(); const inputObj = input as Record; - // Pop the PreToolUse-captured data for this tool (FIFO by name). - const pending = this.#pendingToolUse.get(toolName); - const captured = pending?.shift(); - if (pending && pending.length === 0) this.#pendingToolUse.delete(toolName); - - if (!captured?.toolUseId) { + // Correlate by the SDK's own tool_use_id, passed directly to this + // callback. Never reconstruct it from a PreToolUse-fed name-keyed + // FIFO: the SDK skips canUseTool for auto-allowed tools + // (allowedTools / project permissions.allow), so any allow rule + // desyncs such a queue and mis-correlates every later tool call + // in the session (issue #81). + const sdkToolUseId = options?.toolUseID; + if (!sdkToolUseId) { return { behavior: "deny" as const, message: "Unable to correlate tool use id" }; } - const sdkToolUseId = captured.toolUseId; - const sdkAgentId = captured.agentId; + const sdkAgentId = options?.agentID; // Emit tool_start — Session creates the SessionMessage. this.#emit({ @@ -495,6 +483,8 @@ export function translateSDKMessage( } // Text content (tool_use blocks are handled via canUseTool → tool_start). + // Tag with parent_tool_use_id so subagent text is never mistaken for + // primary assistant output downstream (issue #82). const content = msg.message.content as unknown as Array>; const textParts: string[] = []; for (const block of content) { @@ -503,39 +493,49 @@ export function translateSDKMessage( } } if (textParts.length > 0) { - emit({ type: "text_done", content: textParts.join("") }); + emit({ + type: "text_done", + content: textParts.join(""), + parentToolUseId: assistantMsg.parent_tool_use_id ?? null, + }); } break; } case "stream_event": { - const event = (msg as { + const streamMsg = msg as { event?: { type?: string; index?: number; content_block?: { type?: string }; delta?: { type?: string; text?: string; thinking?: string }; }; - }).event; + parent_tool_use_id?: string | null; + }; + const event = streamMsg.event; if (!event) break; + // Subagent stream events carry the spawning tool call's id — tag every + // text/thinking emission so consumers can keep them out of the primary + // conversation (issue #82). + const parentToolUseId = streamMsg.parent_tool_use_id ?? null; if (event.type === "content_block_start" && event.content_block?.type === "thinking") { // Signal a new thinking block — Session creates the message. - emit({ type: "thinking_delta", content: "", blockIndex: event.index }); + emit({ type: "thinking_delta", content: "", blockIndex: event.index, parentToolUseId }); break; } if (event.type === "content_block_delta" && event.delta) { if (event.delta.type === "text_delta" && event.delta.text) { - emit({ type: "text_delta", content: event.delta.text }); + emit({ type: "text_delta", content: event.delta.text, parentToolUseId }); } else if (event.delta.type === "thinking_delta" && event.delta.thinking) { - emit({ type: "thinking_delta", content: event.delta.thinking, blockIndex: event.index }); + emit({ type: "thinking_delta", content: event.delta.thinking, blockIndex: event.index, parentToolUseId }); } break; } if (event.type === "content_block_stop") { - emit({ type: "thinking_done", blockIndex: event.index }); + emit({ type: "thinking_done", blockIndex: event.index, parentToolUseId }); } break; } diff --git a/src/daemon/providers/interface.ts b/src/daemon/providers/interface.ts index 16030fd..f6c310a 100644 --- a/src/daemon/providers/interface.ts +++ b/src/daemon/providers/interface.ts @@ -90,12 +90,17 @@ export interface NormalizedTurnResult { // ── Provider event stream ───────────────────────────────────────────────────── -/** Normalized event emitted by any provider. Session maps these to SessionMessages. */ +/** Normalized event emitted by any provider. Session maps these to SessionMessages. + * + * Text/thinking events carry `parentToolUseId` when they were produced by a + * subagent (the id of the tool call that spawned it). `null`/absent = primary + * agent. Consumers must not record non-primary text as primary conversation + * content — see issue #82. */ export type ProviderEvent = - | { type: "text_delta"; content: string } - | { type: "text_done"; content: string } - | { type: "thinking_delta"; content: string; blockIndex?: number } - | { type: "thinking_done"; blockIndex?: number } + | { type: "text_delta"; content: string; parentToolUseId?: string | null } + | { type: "text_done"; content: string; parentToolUseId?: string | null } + | { type: "thinking_delta"; content: string; blockIndex?: number; parentToolUseId?: string | null } + | { type: "thinking_done"; blockIndex?: number; parentToolUseId?: string | null } /** Fired when a tool call starts (from the provider's canUseTool gate). * Carries the provider-internal tool_use_id so Session can correlate messages. */ | { diff --git a/src/daemon/session.ts b/src/daemon/session.ts index 1f30a89..ae29314 100644 --- a/src/daemon/session.ts +++ b/src/daemon/session.ts @@ -2000,6 +2000,19 @@ export class Session { } async #handleProviderEvent(event: ProviderEvent): Promise { + // Subagent text/thinking (parentToolUseId set) is not part of the primary + // conversation: streaming it into the primary assistant message corrupts + // both the visible transcript and the canonical history, and a subagent + // text_done would clobber the primary message mid-stream (#82). The + // subagent's work still surfaces via its tool_call messages and the + // spawning tool's result. + if ( + (event.type === "text_delta" || event.type === "text_done" || + event.type === "thinking_delta" || event.type === "thinking_done") && + event.parentToolUseId != null + ) { + return; + } switch (event.type) { case "text_delta": { if (!this.#activeAssistantMsg) { diff --git a/src/tests/provider-claude.test.ts b/src/tests/provider-claude.test.ts index 36af51f..e3935a4 100644 --- a/src/tests/provider-claude.test.ts +++ b/src/tests/provider-claude.test.ts @@ -26,6 +26,9 @@ let sdkMessages: SDKMsg[] = []; let sdkThrowError: Error | null = null; /** Captures the options passed to query() so tests can invoke callbacks. */ let capturedQueryOpts: Record | null = null; +/** When set, the mock loop blocks before finishing so tests can invoke captured + * callbacks (canUseTool, hooks) while the turn queue is still open. */ +let sdkGate: Promise | null = null; function makeMockQuery() { const err = sdkThrowError; @@ -35,7 +38,10 @@ function makeMockQuery() { return { async next(): Promise<{ done: boolean; value: SDKMsg | undefined }> { if (err) throw err; - if (i >= sdkMessages.length) return { done: true, value: undefined }; + if (i >= sdkMessages.length) { + if (sdkGate) await sdkGate; + return { done: true, value: undefined }; + } return { done: false, value: sdkMessages[i++] }; }, }; @@ -138,6 +144,30 @@ describe("translateSDKMessage – assistant", () => { }); expect(events.find((e) => e.type === "text_done")).toMatchObject({ content: "AB" }); }); + + it("tags text_done with parentToolUseId null for primary messages (#82)", () => { + const events = collectEmits({ + type: "assistant", + message: { content: [{ type: "text", text: "primary" }] }, + parent_tool_use_id: null, + }); + expect(events.find((e) => e.type === "text_done")).toMatchObject({ + content: "primary", + parentToolUseId: null, + }); + }); + + it("tags text_done with the spawning tool id for subagent messages (#82)", () => { + const events = collectEmits({ + type: "assistant", + message: { content: [{ type: "text", text: "subagent commentary" }] }, + parent_tool_use_id: "tu-task-1", + }); + expect(events.find((e) => e.type === "text_done")).toMatchObject({ + content: "subagent commentary", + parentToolUseId: "tu-task-1", + }); + }); }); describe("translateSDKMessage – stream_event", () => { @@ -178,6 +208,38 @@ describe("translateSDKMessage – stream_event", () => { const events = collectEmits({ type: "stream_event", event: null }); expect(events).toHaveLength(0); }); + + it("tags text_delta and thinking events with parentToolUseId for subagent streams (#82)", () => { + const textEvents = collectEmits({ + type: "stream_event", + event: { type: "content_block_delta", delta: { type: "text_delta", text: "sub chunk" } }, + parent_tool_use_id: "tu-task-2", + }); + expect(textEvents[0]).toMatchObject({ type: "text_delta", content: "sub chunk", parentToolUseId: "tu-task-2" }); + + const thinkEvents = collectEmits({ + type: "stream_event", + event: { type: "content_block_delta", index: 1, delta: { type: "thinking_delta", thinking: "sub hmm" } }, + parent_tool_use_id: "tu-task-2", + }); + expect(thinkEvents[0]).toMatchObject({ type: "thinking_delta", content: "sub hmm", parentToolUseId: "tu-task-2" }); + + const stopEvents = collectEmits({ + type: "stream_event", + event: { type: "content_block_stop", index: 1 }, + parent_tool_use_id: "tu-task-2", + }); + expect(stopEvents[0]).toMatchObject({ type: "thinking_done", blockIndex: 1, parentToolUseId: "tu-task-2" }); + }); + + it("tags primary stream events with parentToolUseId null", () => { + const events = collectEmits({ + type: "stream_event", + event: { type: "content_block_delta", delta: { type: "text_delta", text: "chunk" } }, + parent_tool_use_id: null, + }); + expect(events[0]).toMatchObject({ type: "text_delta", content: "chunk", parentToolUseId: null }); + }); }); describe("translateSDKMessage – result", () => { @@ -379,7 +441,7 @@ describe("extractToolResultText", () => { // ── ClaudeProvider lifecycle (mocked SDK) ───────────────────────────────────── describe("ClaudeProvider – runTurn with mocked SDK", () => { - beforeEach(() => { sdkMessages = []; sdkThrowError = null; capturedQueryOpts = null; }); + beforeEach(() => { sdkMessages = []; sdkThrowError = null; capturedQueryOpts = null; sdkGate = null; }); it("emits text_done + turn_done from mocked assistant + result messages", async () => { sdkMessages = [ @@ -489,32 +551,91 @@ describe("ClaudeProvider – runTurn with mocked SDK", () => { for await (const _ of run.events) { /* drain */ } }); - it("canUseTool callback emits tool_start and returns allow", async () => { - // The SDK fires PreToolUse *before* canUseTool — we must simulate that order - // so #pendingToolUse has the tool_use_id before canUseTool tries to pop it. + it("canUseTool emits tool_start correlated by the SDK's own toolUseID (#81)", async () => { sdkMessages = [{ type: "result", modelUsage: {} }]; capturedQueryOpts = null; + let release!: () => void; + sdkGate = new Promise((r) => { release = r; }); const provider = makeProvider(); - provider.runTurn({ history: [], userMessage: "hi", workdir: "/tmp", canUseTool: async () => ({ behavior: "allow" as const }) }); + const run = provider.runTurn({ history: [], userMessage: "hi", workdir: "/tmp", canUseTool: async () => ({ behavior: "allow" as const }) }); + await Promise.resolve(); + const opts = capturedQueryOpts as { + options: { + canUseTool: (name: string, input: unknown, o: { toolUseID: string; agentID?: string; signal: AbortSignal }) => Promise; + }; + } | null; + expect(opts?.options?.canUseTool).toBeDefined(); + const result = await opts!.options.canUseTool("Read", { file_path: "/tmp/x.ts" }, { + toolUseID: "tu-real-1", agentID: "agent-7", signal: new AbortController().signal, + }); + expect((result as { behavior: string }).behavior).toBe("allow"); + release(); + const events: ProviderEvent[] = []; + for await (const e of run.events) events.push(e); + const toolStart = events.find((e) => e.type === "tool_start") as Extract | undefined; + expect(toolStart).toBeDefined(); + expect(toolStart!.sdkToolUseId).toBe("tu-real-1"); + expect(toolStart!.sdkAgentId).toBe("agent-7"); + expect(toolStart!.name).toBe("Read"); + expect(toolStart!.input).toEqual({ file_path: "/tmp/x.ts" }); + }); + + it("auto-allowed tools do not desync later correlation (#81 regression)", async () => { + // Repro from the issue: an auto-allowed tool fires PreToolUse but the SDK + // skips canUseTool for it. The next GATED tool must still correlate to its + // own tool_use_id — with the old name-keyed FIFO it popped the stale + // auto-allowed entry instead. + sdkMessages = [{ type: "result", modelUsage: {} }]; + capturedQueryOpts = null; + let release!: () => void; + sdkGate = new Promise((r) => { release = r; }); + const provider = makeProvider(); + const run = provider.runTurn({ history: [], userMessage: "hi", workdir: "/tmp", canUseTool: async () => ({ behavior: "allow" as const }) }); await Promise.resolve(); const opts = capturedQueryOpts as { options: { hooks: { PreToolUse: Array<{ hooks: Array<(input: unknown) => Promise> }> }; - canUseTool: (name: string, input: unknown) => Promise; + canUseTool: (name: string, input: unknown, o: { toolUseID: string; agentID?: string; signal: AbortSignal }) => Promise; }; } | null; - if (opts?.options?.hooks?.PreToolUse?.[0]?.hooks?.[0] && opts.options.canUseTool) { - // 1. Fire PreToolUse so the provider registers "Read" → "tu-abc" - await opts.options.hooks.PreToolUse[0].hooks[0]({ - tool_name: "Read", tool_use_id: "tu-abc", tool_input: {}, agent_id: undefined, - }); - // 2. Now canUseTool can pop the pending entry and emit tool_start - const result = await opts.options.canUseTool("Read", { file_path: "/tmp/x.ts" }); - expect((result as { behavior: string }).behavior).toBe("allow"); - } + expect(opts?.options?.hooks?.PreToolUse?.[0]?.hooks?.[0]).toBeDefined(); + // 1. Auto-allowed call: PreToolUse fires, canUseTool never does. + await opts!.options.hooks.PreToolUse[0]!.hooks[0]!({ + tool_name: "Bash", tool_use_id: "tu-auto-allowed", tool_input: { command: "git status" }, + }); + // 2. Gated call of the SAME tool name. + await opts!.options.canUseTool("Bash", { command: "rm -rf build" }, { + toolUseID: "tu-gated", signal: new AbortController().signal, + }); + release(); + const events: ProviderEvent[] = []; + for await (const e of run.events) events.push(e); + const toolStarts = events.filter((e) => e.type === "tool_start") as Array>; + expect(toolStarts).toHaveLength(1); + expect(toolStarts[0]!.sdkToolUseId).toBe("tu-gated"); + expect(toolStarts[0]!.input).toEqual({ command: "rm -rf build" }); + }); + + it("canUseTool denies when the SDK provides no toolUseID", async () => { + sdkMessages = [{ type: "result", modelUsage: {} }]; + capturedQueryOpts = null; + let release!: () => void; + sdkGate = new Promise((r) => { release = r; }); + const provider = makeProvider(); + const run = provider.runTurn({ history: [], userMessage: "hi", workdir: "/tmp", canUseTool: async () => ({ behavior: "allow" as const }) }); + await Promise.resolve(); + const opts = capturedQueryOpts as { + options: { canUseTool: (name: string, input: unknown, o?: unknown) => Promise }; + } | null; + const result = await opts!.options.canUseTool("Read", {}, { signal: new AbortController().signal }); + expect(result).toMatchObject({ behavior: "deny" }); + release(); + const events: ProviderEvent[] = []; + for await (const e of run.events) events.push(e); + expect(events.find((e) => e.type === "tool_start")).toBeUndefined(); }); - it("PreToolUse hook captures tool_use_id", async () => { + it("PreToolUse hook runs (audit + compression) and returns a hook result", async () => { sdkMessages = [{ type: "result", modelUsage: {} }]; capturedQueryOpts = null; const provider = makeProvider(); diff --git a/src/tests/provider-switch.test.ts b/src/tests/provider-switch.test.ts index 7560b80..5c9ef49 100644 --- a/src/tests/provider-switch.test.ts +++ b/src/tests/provider-switch.test.ts @@ -154,6 +154,69 @@ describe("CanonicalHistoryAccumulator", () => { acc.reset(); expect(acc.history).toHaveLength(0); }); + + it("concatenates every text_done of a turn — interleaved text → tool → text (#82)", async () => { + // A real agentic turn fires one text_done per assistant message: + // commentary → tool call → final answer. All blocks must survive; the old + // assign-behavior kept only the last one. + const acc = new CanonicalHistoryAccumulator(); + const provider = new MockProvider("claude", [[ + { type: "text_done", content: "Let me check the file." }, + { type: "tool_start", toolId: "t1", sdkToolUseId: "sdk-t1", name: "Read", input: { file_path: "a.ts" }, approvalId: "a1" }, + { type: "tool_complete", sdkToolUseId: "sdk-t1", output: "export {}", success: true }, + { type: "text_done", content: "It is empty — done." }, + { type: "turn_done", result: mockResult({ providerId: "claude", model: "claude-opus-4-5" }) }, + ]]); + + await runTurn(provider, acc, "Check a.ts"); + + const turn = acc.history[1]; + expect(turn.role).toBe("assistant"); + if (turn.role === "assistant") { + expect(turn.content).toBe("Let me check the file.\n\nIt is empty — done."); + } + }); + + it("drops subagent text and thinking from the canonical history (#82)", async () => { + const acc = new CanonicalHistoryAccumulator(); + const provider = new MockProvider("claude", [[ + { type: "text_done", content: "Spawning a subagent.", parentToolUseId: null }, + { type: "thinking_delta", content: "sub thinking", parentToolUseId: "tu-task" }, + { type: "text_done", content: "SUBAGENT COMMENTARY", parentToolUseId: "tu-task" }, + { type: "text_done", content: "The subagent finished." }, + { type: "turn_done", result: mockResult({ providerId: "claude", model: "claude-opus-4-5" }) }, + ]]); + + await runTurn(provider, acc, "Delegate this"); + + const turn = acc.history[1]; + expect(turn.role).toBe("assistant"); + if (turn.role === "assistant") { + expect(turn.content).toBe("Spawning a subagent.\n\nThe subagent finished."); + expect(turn.content).not.toContain("SUBAGENT COMMENTARY"); + expect(turn.thinking).toBeUndefined(); + } + }); + + it("text_done accumulation resets across turns", async () => { + const acc = new CanonicalHistoryAccumulator(); + const provider = new MockProvider("claude", [ + [ + { type: "text_done", content: "First turn." }, + { type: "turn_done", result: mockResult({ providerId: "claude", model: "claude-opus-4-5" }) }, + ], + [ + { type: "text_done", content: "Second turn." }, + { type: "turn_done", result: mockResult({ providerId: "claude", model: "claude-opus-4-5" }) }, + ], + ]); + + await runTurn(provider, acc, "One"); + await runTurn(provider, acc, "Two"); + + expect(acc.history[1]).toMatchObject({ role: "assistant", content: "First turn." }); + expect(acc.history[3]).toMatchObject({ role: "assistant", content: "Second turn." }); + }); }); describe("Provider-switch: history forwarding", () => { diff --git a/src/tests/session-stream-commit.test.ts b/src/tests/session-stream-commit.test.ts index d04d23d..67e359b 100644 --- a/src/tests/session-stream-commit.test.ts +++ b/src/tests/session-stream-commit.test.ts @@ -201,6 +201,42 @@ describe("C1 – text_delta → text_done commits exactly one scrollback entry", }); }); +// ── C8: subagent text stays out of the primary stream (#82) ────────────────── + +describe("C8 – subagent text/thinking (parentToolUseId set) never reaches the primary stream", () => { + it("subagent text_done cannot clobber the streaming primary message", async () => { + const provider = new MockSessionProvider("claude", [ + [ + { type: "text_delta", content: "Primary " }, + // Subagent output interleaves mid-stream — must be ignored entirely. + { type: "text_delta", content: "SUB DELTA", parentToolUseId: "tu-task" }, + { type: "text_done", content: "SUBAGENT FINAL", parentToolUseId: "tu-task" }, + { type: "thinking_delta", content: "sub think", blockIndex: 0, parentToolUseId: "tu-task" }, + { type: "thinking_done", blockIndex: 0, parentToolUseId: "tu-task" }, + { type: "text_delta", content: "answer" }, + { type: "text_done", content: "Primary answer" }, + turnDone, + ], + ]); + const session = makeSession(provider); + + await session.send("delegate", TEST_AUTH); + await waitForIdle(session); + + const replay = replayFor(session); + assertNoDuplicates(replay); + const assistant = replay.filter((m) => m.role === "assistant"); + expect(assistant).toHaveLength(1); + expect(assistant[0]!.content).toBe("Primary answer"); + const thinking = replay.filter((m) => m.role === "thinking"); + expect(thinking).toHaveLength(0); + for (const m of replay) { + expect(m.content).not.toContain("SUB DELTA"); + expect(m.content).not.toContain("SUBAGENT FINAL"); + } + }); +}); + // ── C2: thinking blocks ─────────────────────────────────────────────────────── describe("C2 – thinking stream commits exactly one scrollback entry", () => { From b887fc2514cef5cacc438150fd52366ff28eb05f Mon Sep 17 00:00:00 2001 From: Yash Datta Date: Mon, 6 Jul 2026 17:22:32 +0800 Subject: [PATCH 2/2] fix: extract shared isSubagentEvent guard (CodeRabbit) 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 --- src/daemon/providers/canonical.ts | 10 +++++----- src/daemon/providers/interface.ts | 17 +++++++++++++++++ src/daemon/session.ts | 12 +++--------- 3 files changed, 25 insertions(+), 14 deletions(-) diff --git a/src/daemon/providers/canonical.ts b/src/daemon/providers/canonical.ts index 07ffac8..3f61ea3 100644 --- a/src/daemon/providers/canonical.ts +++ b/src/daemon/providers/canonical.ts @@ -11,7 +11,7 @@ * CanonicalToolCall type already captures everything needed. */ -import type { ProviderEvent } from "./interface.js"; +import { type ProviderEvent, isSubagentEvent } from "./interface.js"; // ── Types ───────────────────────────────────────────────────────────────────── @@ -264,11 +264,12 @@ export class CanonicalHistoryAccumulator { * On turn_done, the completed assistant turn is appended to history. */ handleEvent(event: ProviderEvent): void { + // Subagent text/thinking is not primary conversation content — recording + // it would corrupt cross-provider history (#82). Session already filters + // these before feeding the accumulator; this guards standalone callers. + if (isSubagentEvent(event)) return; switch (event.type) { case "text_done": - // Subagent text (parentToolUseId set) is not primary conversation - // content — recording it would corrupt cross-provider history (#82). - if (event.parentToolUseId != null) break; // A turn can span several assistant messages (text → tool → text → // final text); each fires its own text_done. Append every block — // assigning would keep only the last one and drop all interleaved @@ -279,7 +280,6 @@ export class CanonicalHistoryAccumulator { break; case "thinking_delta": - if (event.parentToolUseId != null) break; this.#currentThinking += event.content; break; diff --git a/src/daemon/providers/interface.ts b/src/daemon/providers/interface.ts index f6c310a..ab84747 100644 --- a/src/daemon/providers/interface.ts +++ b/src/daemon/providers/interface.ts @@ -124,6 +124,23 @@ export type ProviderEvent = | { type: "turn_done"; result: NormalizedTurnResult } | { type: "error"; message: string }; +/** + * True when a text/thinking ProviderEvent was produced by a subagent + * (`parentToolUseId` set). Such events must never be recorded as primary + * conversation content — see issue #82. Centralised so the canonical + * accumulator and Session's event consumer can't drift as new subagent-aware + * event types are added. + */ +export function isSubagentEvent(event: ProviderEvent): boolean { + return ( + (event.type === "text_delta" || + event.type === "text_done" || + event.type === "thinking_delta" || + event.type === "thinking_done") && + event.parentToolUseId != null + ); +} + // ── TurnRun ─────────────────────────────────────────────────────────────────── export interface TurnRun { diff --git a/src/daemon/session.ts b/src/daemon/session.ts index ae29314..dc63952 100644 --- a/src/daemon/session.ts +++ b/src/daemon/session.ts @@ -11,7 +11,7 @@ import { ClaudeProvider } from "./providers/claude/index.js"; import { CanonicalHistoryAccumulator } from "./providers/canonical.js"; -import type { ProviderEvent, NormalizedTurnResult, TurnRun, ToolApprovalFn, SessionProvider } from "./providers/interface.js"; +import { type ProviderEvent, type NormalizedTurnResult, type TurnRun, type ToolApprovalFn, type SessionProvider, isSubagentEvent } from "./providers/interface.js"; import { randomUUID } from "node:crypto"; import type { AuthContext, @@ -2005,14 +2005,8 @@ export class Session { // both the visible transcript and the canonical history, and a subagent // text_done would clobber the primary message mid-stream (#82). The // subagent's work still surfaces via its tool_call messages and the - // spawning tool's result. - if ( - (event.type === "text_delta" || event.type === "text_done" || - event.type === "thinking_delta" || event.type === "thinking_done") && - event.parentToolUseId != null - ) { - return; - } + // spawning tool's result. Shared with the canonical accumulator's guard. + if (isSubagentEvent(event)) return; switch (event.type) { case "text_delta": { if (!this.#activeAssistantMsg) {