diff options
Diffstat (limited to 'src')
| -rw-r--r-- | src/core/chunks/groups.test.ts | 73 | ||||
| -rw-r--r-- | src/core/chunks/groups.ts | 8 | ||||
| -rw-r--r-- | src/features/chat/ui/ChatView.svelte | 2 | ||||
| -rw-r--r-- | src/features/chat/ui/ChatView.test.ts | 132 |
4 files changed, 214 insertions, 1 deletions
diff --git a/src/core/chunks/groups.test.ts b/src/core/chunks/groups.test.ts index c8b0fa2..db632a3 100644 --- a/src/core/chunks/groups.test.ts +++ b/src/core/chunks/groups.test.ts @@ -122,4 +122,77 @@ describe("groupRenderedChunks", () => { expect(singles[0]?.kind === "single" && singles[0].chunk.chunk.type).toBe("tool-call"); expect(singles[1]?.kind === "single" && singles[1].chunk.chunk.type).toBe("tool-result"); }); + + // --- Duplicate toolCallId hardening (fix-toolcall-duplicate-keys) --- + // A provider double-emission (e.g. openrouter/stealth/ox-alpha) can persist the + // SAME toolCallId twice within one step. The renderer must NEVER crash on that + // (Svelte `each_key_duplicate` aborts the reactive flush — the tab appears dead). + + it("dedupes duplicate toolCallIds within a batch", () => { + // The PII_REDACTED_CONVERSATION_SHORT_01 pattern: every id emitted twice, in order (seq 1,2 then 3,4). + const groups = groupRenderedChunks([ + call(1, "a", "s1"), + call(2, "b", "s1"), + call(3, "a", "s1"), + call(4, "b", "s1"), + ]); + expect(groups).toHaveLength(1); + const g = groups[0]; + if (g?.kind !== "tool-batch") throw new Error("expected a tool-batch group"); + expect(g.entries).toHaveLength(2); + expect(g.entries.map((e) => e.call.toolCallId)).toEqual(["a", "b"]); + }); + + it("dedupes duplicate toolCallIds and pairs each unique call with its result", () => { + const groups = groupRenderedChunks([ + call(1, "a", "s1"), + call(2, "b", "s1"), + call(3, "a", "s1"), + call(4, "b", "s1"), + result(5, "a", "s1"), + result(6, "b", "s1"), + result(7, "a", "s1"), + result(8, "b", "s1"), + ]); + expect(groups).toHaveLength(1); + const g = groups[0]; + if (g?.kind !== "tool-batch") throw new Error("expected a tool-batch group"); + expect(g.entries).toHaveLength(2); + expect(g.entries[0]?.call.toolCallId).toBe("a"); + expect(g.entries[0]?.result?.content).toBe("result-a"); + expect(g.entries[1]?.call.toolCallId).toBe("b"); + expect(g.entries[1]?.result?.content).toBe("result-b"); + }); + + it("keeps the first occurrence of a duplicate toolCallId and skips the second", () => { + const firstA = call(1, "a", "s1"); // first occurrence of a + const firstB = call(2, "b", "s1"); // first occurrence of b + const groups = groupRenderedChunks([ + firstA, + firstB, + call(3, "a", "s1"), // duplicate of a — skipped + call(4, "b", "s1"), // duplicate of b — skipped + ]); + const g = groups[0]; + if (g?.kind !== "tool-batch") throw new Error("expected a tool-batch group"); + expect(g.entries).toHaveLength(2); + // The FIRST occurrence (by stream order) is the one kept — referential + // equality against the seq-1 / seq-2 chunks (the entry stores the inner + // chunk; the outer `seq` lives on the RenderedChunk wrapper). + expect(g.entries[0]?.call).toBe(firstA.chunk); + expect(g.entries[1]?.call).toBe(firstB.chunk); + }); + + it("keeps the first (provisional) occurrence of a duplicate and marks the batch provisional", () => { + // One provisional + one committed chunk with the SAME id: the first + // (provisional) occurrence wins, and the batch stays provisional. + const prov = call(1, "a", "s1", true); + const groups = groupRenderedChunks([prov, call(2, "a", "s1")]); + const g = groups[0]; + if (g?.kind !== "tool-batch") throw new Error("expected a tool-batch group"); + expect(g.provisional).toBe(true); + expect(g.entries).toHaveLength(1); + expect(g.entries[0]?.call).toBe(prov.chunk); + expect(g.entries[0]?.result).toBeNull(); + }); }); diff --git a/src/core/chunks/groups.ts b/src/core/chunks/groups.ts index 53a2873..c1c80c9 100644 --- a/src/core/chunks/groups.ts +++ b/src/core/chunks/groups.ts @@ -72,9 +72,17 @@ export function groupRenderedChunks(rendered: readonly RenderedChunk[]): readonl emittedSteps.add(stepId); const entries: ToolBatchEntry[] = []; + // Dedupe by toolCallId within the batch: a provider double-emission (or any + // corrupted transcript) can carry the SAME call id on multiple tool-call + // chunks in one step. The first occurrence (stream order) wins; later ones + // are skipped so the keyed `{#each}` in the UI never sees duplicate keys + // (Svelte `each_key_duplicate` aborts the reactive flush — tab appears dead). + const seenCallIds = new Set<string>(); let provisional = false; for (const inner of rendered) { if (inner.chunk.type === "tool-call" && inner.chunk.stepId === stepId) { + if (seenCallIds.has(inner.chunk.toolCallId)) continue; + seenCallIds.add(inner.chunk.toolCallId); const result = resultByCallId.get(inner.chunk.toolCallId) ?? null; entries.push({ call: inner.chunk, result }); if (inner.provisional) provisional = true; diff --git a/src/features/chat/ui/ChatView.svelte b/src/features/chat/ui/ChatView.svelte index e67ca5b..6805b20 100644 --- a/src/features/chat/ui/ChatView.svelte +++ b/src/features/chat/ui/ChatView.svelte @@ -301,7 +301,7 @@ <div class="chat chat-start [&>.chat-bubble]:max-w-5xl [&>.chat-bubble]:p-0"> <div class="chat-bubble w-full bg-transparent"> <div class="flex flex-col gap-1"> - {#each row.group.entries as entry (entry.call.toolCallId)} + {#each row.group.entries as entry, entryIndex (entryIndex)} <div class="collapse w-full rounded-box bg-base-200 text-sm"> <input type="checkbox" aria-label="Toggle tool call" /> <div class="collapse-title flex min-h-0 items-center gap-2 py-2 font-medium"> diff --git a/src/features/chat/ui/ChatView.test.ts b/src/features/chat/ui/ChatView.test.ts new file mode 100644 index 0000000..eb807c1 --- /dev/null +++ b/src/features/chat/ui/ChatView.test.ts @@ -0,0 +1,132 @@ +import type { StepId } from "@dispatch/wire"; +import { render, screen } from "@testing-library/svelte"; +import { describe, expect, it } from "vitest"; +import type { RenderedChunk } from "../../../core/chunks"; +import ChatView from "./ChatView.svelte"; + +// Defensive rendering: a conversation transcript can contain duplicate +// `toolCallId`s (provider double-emission, persisted — e.g. PII_REDACTED_CONVERSATION_SHORT_01). The +// grouped render path MUST dedupe them so the keyed {#each} never throws +// Svelte `each_key_duplicate` (which aborts the reactive flush and makes the +// tab appear dead). See fix-toolcall-duplicate-keys plan Part 1. +describe("ChatView — duplicate toolCallIds", () => { + it("renders a tool batch with duplicated toolCallIds without throwing, showing one card per unique call", () => { + const chunks: RenderedChunk[] = [ + { + seq: 1, + role: "assistant", + chunk: { + type: "tool-call", + toolCallId: "a", + toolName: "read_file", + input: { path: "/a" }, + stepId: "t1#0" as StepId, + }, + provisional: false, + }, + { + seq: 2, + role: "assistant", + chunk: { + type: "tool-call", + toolCallId: "b", + toolName: "list_dir", + input: { path: "/b" }, + stepId: "t1#0" as StepId, + }, + provisional: false, + }, + // The PII_REDACTED_CONVERSATION_SHORT_01 pattern: the same ids re-emitted immediately after. + { + seq: 3, + role: "assistant", + chunk: { + type: "tool-call", + toolCallId: "a", + toolName: "read_file", + input: { path: "/a" }, + stepId: "t1#0" as StepId, + }, + provisional: false, + }, + { + seq: 4, + role: "assistant", + chunk: { + type: "tool-call", + toolCallId: "b", + toolName: "list_dir", + input: { path: "/b" }, + stepId: "t1#0" as StepId, + }, + provisional: false, + }, + { + seq: 5, + role: "tool", + chunk: { + type: "tool-result", + toolCallId: "a", + toolName: "read_file", + content: "contents-of-a", + isError: false, + stepId: "t1#0" as StepId, + }, + provisional: false, + }, + { + seq: 6, + role: "tool", + chunk: { + type: "tool-result", + toolCallId: "b", + toolName: "list_dir", + content: "contents-of-b", + isError: false, + stepId: "t1#0" as StepId, + }, + provisional: false, + }, + // Duplicate results too — absorbed into the single deduped batch. + { + seq: 7, + role: "tool", + chunk: { + type: "tool-result", + toolCallId: "a", + toolName: "read_file", + content: "contents-of-a", + isError: false, + stepId: "t1#0" as StepId, + }, + provisional: false, + }, + { + seq: 8, + role: "tool", + chunk: { + type: "tool-result", + toolCallId: "b", + toolName: "list_dir", + content: "contents-of-b", + isError: false, + stepId: "t1#0" as StepId, + }, + provisional: false, + }, + ]; + + const { container } = render(ChatView, { props: { chunks } }); + + // No each_key_duplicate crash. Batched calls render as collapsible cards — + // exactly ONE per unique toolCallId (deduped), not 4. + const collapses = container.querySelectorAll(".collapse"); + expect(collapses).toHaveLength(2); + + // Both unique tool names shown; each result rendered once (deduped). + expect(screen.getByText("read_file")).toBeInTheDocument(); + expect(screen.getByText("list_dir")).toBeInTheDocument(); + expect(screen.getByText("contents-of-a")).toBeInTheDocument(); + expect(screen.getByText("contents-of-b")).toBeInTheDocument(); + }); +}); |
