diff --git a/.changeset/fix-skill-read-duplicates.md b/.changeset/fix-skill-read-duplicates.md new file mode 100644 index 0000000..d12accd --- /dev/null +++ b/.changeset/fix-skill-read-duplicates.md @@ -0,0 +1,5 @@ +--- +"@jmfederico/pi-web": patch +--- + +Prevent live skill-loading cards from duplicating when the finalized transcript groups multiple skill reads. diff --git a/src/client/src/chatMessages.ts b/src/client/src/chatMessages.ts index ceec115..5294b14 100644 --- a/src/client/src/chatMessages.ts +++ b/src/client/src/chatMessages.ts @@ -162,9 +162,9 @@ function normalizeContent(content: unknown, message: unknown): ChatPart[] { if (type === "toolCall") { const toolName = getString(part, "name") ?? "tool"; const args = getProperty(part, "arguments"); - const skillRead = toolName === "read" ? parseSkillReadPath(getString(args, "path")) : undefined; - if (skillRead !== undefined) return [{ type: "skillRead", ...skillRead }]; const toolCallId = getString(part, "id"); + const skillRead = toolName === "read" ? parseSkillReadPath(getString(args, "path")) : undefined; + if (skillRead !== undefined) return [{ type: "skillRead", ...skillRead, ...(toolCallId === undefined ? {} : { toolCallId }) }]; return [{ type: "toolCall", ...(toolCallId === undefined ? {} : { toolCallId }), toolName, summary: summarizeArgs(args), ...(args === undefined ? {} : { args }) }]; } if (type === "image") { diff --git a/src/client/src/chatTranscript.test.ts b/src/client/src/chatTranscript.test.ts index a4454eb..09c349b 100644 --- a/src/client/src/chatTranscript.test.ts +++ b/src/client/src/chatTranscript.test.ts @@ -101,8 +101,9 @@ describe("applyTranscriptEvent", () => { ]); }); - it("replaces streamed skill reads when the finalized assistant message includes thinking", () => { + it("replaces streamed thinking and skill reads when the finalized assistant message includes thinking", () => { const streamed: ChatLine[] = [ + { role: "assistant", parts: [{ type: "thinking", text: "load skill" }] }, { role: "skill", parts: [{ type: "skillRead", name: "playwright", path: "/skills/playwright/SKILL.md" }] }, { role: "tool", parts: [{ type: "toolResult", toolName: "read", text: "skill content", isError: false }] }, ]; @@ -174,8 +175,8 @@ describe("applyTranscriptEvent", () => { messages = applyTranscriptEvent(messages, { type: "tool.start", toolName: "read", toolCallId: "2", summary: "", args: { path: "/skills/sentry-cli/SKILL.md" } }) ?? messages; expect(messages).toEqual([ - { role: "skill", parts: [{ type: "skillRead", name: "playwright", path: "/skills/playwright/SKILL.md" }] }, - { role: "skill", parts: [{ type: "skillRead", name: "sentry-cli", path: "/skills/sentry-cli/SKILL.md" }] }, + { role: "skill", parts: [{ type: "skillRead", name: "playwright", path: "/skills/playwright/SKILL.md", toolCallId: "1" }] }, + { role: "skill", parts: [{ type: "skillRead", name: "sentry-cli", path: "/skills/sentry-cli/SKILL.md", toolCallId: "2" }] }, ]); }); @@ -185,6 +186,74 @@ describe("applyTranscriptEvent", () => { messages = applyTranscriptEvent(messages, { type: "tool.start", toolName: "read", toolCallId: "1", summary: "", args: { path: "/skills/playwright/SKILL.md" } }) ?? messages; expect(messages).toEqual([ + { role: "skill", parts: [{ type: "skillRead", name: "playwright", path: "/skills/playwright/SKILL.md", toolCallId: "1" }] }, + ]); + }); + + it("replaces multiple streamed skill reads with the finalized grouped skill message", () => { + const firstTool: ChatLine = { role: "tool", parts: [{ type: "toolExecution", toolCallId: "read-1", toolName: "read", summary: "/skills/code-quality-architecture/SKILL.md", status: "success", resultText: "content" }] }; + const secondTool: ChatLine = { role: "tool", parts: [{ type: "toolExecution", toolCallId: "read-2", toolName: "read", summary: "/skills/relay/SKILL.md", status: "success", resultText: "content" }] }; + const thirdTool: ChatLine = { role: "tool", parts: [{ type: "toolExecution", toolCallId: "read-3", toolName: "read", summary: "/skills/skill-creator/SKILL.md", status: "success", resultText: "content" }] }; + const streamed: ChatLine[] = [ + { role: "skill", parts: [{ type: "skillRead", name: "code-quality-architecture", path: "/skills/code-quality-architecture/SKILL.md", toolCallId: "read-1" }] }, + firstTool, + { role: "skill", parts: [{ type: "skillRead", name: "relay", path: "/skills/relay/SKILL.md", toolCallId: "read-2" }] }, + secondTool, + { role: "skill", parts: [{ type: "skillRead", name: "skill-creator", path: "/skills/skill-creator/SKILL.md", toolCallId: "read-3" }] }, + thirdTool, + ]; + + expect(applyTranscriptEvent(streamed, { + type: "message.end", + message: { + role: "assistant", + content: [ + { type: "toolCall", id: "read-1", name: "read", arguments: { path: "/skills/code-quality-architecture/SKILL.md" } }, + { type: "toolCall", id: "read-2", name: "read", arguments: { path: "/skills/relay/SKILL.md" } }, + { type: "toolCall", id: "read-3", name: "read", arguments: { path: "/skills/skill-creator/SKILL.md" } }, + ], + timestamp: "2026-05-09T12:00:00.000Z", + }, + })).toEqual([ + { + role: "skill", + parts: [ + { type: "skillRead", name: "code-quality-architecture", path: "/skills/code-quality-architecture/SKILL.md", toolCallId: "read-1" }, + { type: "skillRead", name: "relay", path: "/skills/relay/SKILL.md", toolCallId: "read-2" }, + { type: "skillRead", name: "skill-creator", path: "/skills/skill-creator/SKILL.md", toolCallId: "read-3" }, + ], + meta: { timestamp: "2026-05-09T12:00:00.000Z" }, + }, + firstTool, + secondTool, + thirdTool, + ]); + }); + + it("ignores streamed skill read starts that are already in a finalized grouped skill message", () => { + const messages: ChatLine[] = [ + { + role: "skill", + parts: [ + { type: "skillRead", name: "code-quality-architecture", path: "/skills/code-quality-architecture/SKILL.md", toolCallId: "read-1" }, + { type: "skillRead", name: "relay", path: "/skills/relay/SKILL.md", toolCallId: "read-2" }, + ], + meta: { timestamp: "2026-05-09T12:00:00.000Z" }, + }, + { role: "tool", parts: [{ type: "toolExecution", toolCallId: "read-1", toolName: "read", summary: "/skills/code-quality-architecture/SKILL.md", status: "success", resultText: "content" }] }, + ]; + + expect(applyTranscriptEvent(messages, { type: "tool.start", toolName: "read", toolCallId: "read-2", summary: "", args: { path: "/skills/relay/SKILL.md" } })).toEqual(messages); + }); + + it("allows the same skill read after a user boundary", () => { + const messages: ChatLine[] = [ + { role: "skill", parts: [{ type: "skillRead", name: "playwright", path: "/skills/playwright/SKILL.md" }] }, + textMessage("user", "load it again"), + ]; + + expect(applyTranscriptEvent(messages, { type: "tool.start", toolName: "read", toolCallId: "", summary: "", args: { path: "/skills/playwright/SKILL.md" } })).toEqual([ + ...messages, { role: "skill", parts: [{ type: "skillRead", name: "playwright", path: "/skills/playwright/SKILL.md" }] }, ]); }); diff --git a/src/client/src/chatTranscript.ts b/src/client/src/chatTranscript.ts index 84e976d..bad38d5 100644 --- a/src/client/src/chatTranscript.ts +++ b/src/client/src/chatTranscript.ts @@ -35,8 +35,8 @@ function applyFinalMessage(messages: ChatLine[], rawMessage: unknown): ChatLine[ } function applyFinalLine(messages: ChatLine[], displayEnded: ChatLine): ChatLine[] { - const skillReadIndex = findMatchingSkillRead(messages, displayEnded); - if (skillReadIndex >= 0) return [...messages.slice(0, skillReadIndex), displayEnded, ...messages.slice(skillReadIndex + 1)]; + const skillReadIndexes = findMatchingSkillReadIndexes(messages, displayEnded); + if (skillReadIndexes.length > 0) return replaceSkillReadLines(messages, skillReadIndexes, displayEnded); const last = messages.at(-1); if (last?.role !== displayEnded.role) return [...messages, displayEnded]; if (displayEnded.role === "assistant" || sameMessageText(last, displayEnded)) return [...messages.slice(0, -1), displayEnded]; @@ -58,7 +58,9 @@ function parseSkillReadPath(path: string | undefined): { name: string; path: str function appendToolExecutionStart(messages: ChatLine[], event: Extract): ChatLine[] { const skillRead = event.toolName === "read" ? parseSkillReadPath(getString(event.args, "path")) : undefined; - if (skillRead !== undefined) return appendLine(messages, { role: "skill", parts: [{ type: "skillRead", ...skillRead }] }); + if (skillRead !== undefined) { + return appendLine(messages, { role: "skill", parts: [{ type: "skillRead", ...skillRead, ...(event.toolCallId === "" ? {} : { toolCallId: event.toolCallId }) }] }); + } const part: ToolExecutionPart = { type: "toolExecution", @@ -151,16 +153,51 @@ function stringifyToolContent(content: unknown): string { return ""; } -function findMatchingSkillRead(messages: ChatLine[], ended: ChatLine): number { +function findMatchingSkillReadIndexes(messages: ChatLine[], ended: ChatLine): number[] { const endedReads = skillReads(ended); - if (endedReads.length === 0) return -1; - for (let index = messages.length - 1; index >= 0; index--) { - const message = messages[index]; - if (message?.role !== "skill") continue; - const reads = skillReads(message); - if (sameSkillReads(reads, endedReads)) return index; + if (endedReads.length === 0) return []; + + const matchedIndexes: number[] = []; + let readEnd = endedReads.length; + const lowerBound = lastUserBoundaryIndex(messages) + 1; + + for (let index = messages.length - 1; index >= lowerBound; index--) { + const reads = skillReads(messages[index]); + if (reads.length === 0) continue; + const readStart = readEnd - reads.length; + if (readStart < 0) continue; + if (!sameSkillReads(reads, endedReads.slice(readStart, readEnd))) continue; + matchedIndexes.unshift(index); + readEnd = readStart; + if (readEnd === 0) return matchedIndexes; } - return -1; + + return []; +} + +function replaceSkillReadLines(messages: ChatLine[], indexes: number[], replacement: ChatLine): ChatLine[] { + const replacementIndexes = indexesWithAdjacentAssistantFragment(messages, indexes, replacement); + const insertIndex = replacementIndexes[0]; + if (insertIndex === undefined) return messages; + const replaced = new Set(replacementIndexes); + const next: ChatLine[] = []; + for (let index = 0; index < messages.length; index++) { + if (index === insertIndex) next.push(replacement); + const message = messages[index]; + if (message !== undefined && !replaced.has(index)) next.push(message); + } + return next; +} + +function indexesWithAdjacentAssistantFragment(messages: ChatLine[], indexes: number[], replacement: ChatLine): number[] { + const firstIndex = indexes[0]; + if (replacement.role !== "assistant" || firstIndex === undefined) return indexes; + const previousIndex = firstIndex - 1; + return isStreamedAssistantFragment(messages[previousIndex]) ? [previousIndex, ...indexes] : indexes; +} + +function isStreamedAssistantFragment(message: ChatLine | undefined): boolean { + return message?.role === "assistant" && message.parts.length > 0 && message.parts.every((part) => part.type === "text" || part.type === "thinking"); } function skillReads(message: ChatLine | undefined): SkillRead[] { @@ -176,6 +213,7 @@ function sameSkillReads(left: SkillRead[], right: SkillRead[]): boolean { function sameSkillRead(left: SkillRead, right: SkillRead | undefined): boolean { if (right === undefined) return false; + if (left.toolCallId !== undefined && right.toolCallId !== undefined) return left.toolCallId === right.toolCallId; return normalizeSkillPath(left.path) === normalizeSkillPath(right.path) || left.name === right.name; } @@ -201,11 +239,32 @@ function appendNewMessage(messages: ChatLine[], rawMessage: unknown): ChatLine[] function appendLine(messages: ChatLine[], line: ChatLine): ChatLine[] { const last = messages.at(-1); - if (line.role === "skill" && sameSkillReads(skillReads(last), skillReads(line))) return messages; + if (isDuplicateSkillLine(messages, line)) return messages; if (last?.role === line.role && line.role !== "skill") return [...messages.slice(0, -1), { ...last, parts: [...last.parts, ...line.parts] }]; return [...messages, line]; } +function isDuplicateSkillLine(messages: ChatLine[], line: ChatLine): boolean { + const reads = skillReads(line); + if (line.role !== "skill" || reads.length === 0) return false; + const lowerBound = lastUserBoundaryIndex(messages) + 1; + return reads.every((read) => hasMatchingSkillRead(messages, read, lowerBound)); +} + +function hasMatchingSkillRead(messages: ChatLine[], read: SkillRead, lowerBound: number): boolean { + for (let index = messages.length - 1; index >= lowerBound; index--) { + if (skillReads(messages[index]).some((candidate) => sameSkillRead(candidate, read))) return true; + } + return false; +} + +function lastUserBoundaryIndex(messages: ChatLine[]): number { + for (let index = messages.length - 1; index >= 0; index--) { + if (messages[index]?.role === "user") return index; + } + return -1; +} + function isRecord(value: unknown): value is Record { return typeof value === "object" && value !== null; } diff --git a/src/client/src/components/shared.ts b/src/client/src/components/shared.ts index a179efc..91165e5 100644 --- a/src/client/src/components/shared.ts +++ b/src/client/src/components/shared.ts @@ -24,7 +24,7 @@ export type ChatPart = | { type: "image"; mimeType: string; data: string } | { type: "thinking"; text: string } | { type: "skillInvocation"; name: string; location: string; content: string } - | { type: "skillRead"; name: string; path: string } + | { type: "skillRead"; name: string; path: string; toolCallId?: string } | { type: "toolCall"; toolCallId?: string; toolName: string; summary: string; args?: unknown } | ToolExecutionPart | { type: "toolResult"; toolCallId?: string; toolName: string; text: string; isError: boolean; content?: unknown; details?: unknown }