From e26219c96742fb61b3ad68fb70c5feb476f761d2 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Sat, 15 Nov 2025 17:33:56 +0100 Subject: [PATCH] test: cover tool call hydration preview --- packages/app/src/components/message.tsx | 27 ++++--- .../src/components/tool-call-preview.test.ts | 74 +++++++++++++++++++ .../app/src/components/tool-call-preview.ts | 40 ++++++++++ plan.md | 13 +++- 4 files changed, 136 insertions(+), 18 deletions(-) create mode 100644 packages/app/src/components/tool-call-preview.test.ts create mode 100644 packages/app/src/components/tool-call-preview.ts diff --git a/packages/app/src/components/message.tsx b/packages/app/src/components/message.tsx index d27900517..d14074b7f 100644 --- a/packages/app/src/components/message.tsx +++ b/packages/app/src/components/message.tsx @@ -26,15 +26,9 @@ import { baseColors, theme } from "@/styles/theme"; import { Colors } from "@/constants/theme"; import * as Clipboard from "expo-clipboard"; import type { TodoEntry } from "@/types/stream"; -import { - extractCommandDetails, - extractEditEntries, - extractReadEntries, - type CommandDetails, - type EditEntry, - type ReadEntry, -} from "@/utils/tool-call-parsers"; +import type { CommandDetails, EditEntry, ReadEntry } from "@/utils/tool-call-parsers"; import { DiffViewer } from "./diff-viewer"; +import { resolveToolCallPreview } from "./tool-call-preview"; interface UserMessageProps { message: string; @@ -1010,12 +1004,17 @@ export const ToolCall = memo(function ToolCall({ parsedCommandDetails, onOpenDetails, }: ToolCallProps) { - const fallbackEditEntries = useMemo(() => extractEditEntries(args, result), [args, result]); - const editEntries = parsedEditEntries ?? fallbackEditEntries; - const fallbackReadEntries = useMemo(() => extractReadEntries(result, args), [args, result]); - const readEntries = parsedReadEntries ?? fallbackReadEntries; - const fallbackCommandDetails = useMemo(() => extractCommandDetails(args, result), [args, result]); - const commandDetails = parsedCommandDetails ?? fallbackCommandDetails; + const { editEntries, readEntries, commandDetails } = useMemo( + () => + resolveToolCallPreview({ + args, + result, + parsedEditEntries, + parsedReadEntries, + parsedCommandDetails, + }), + [args, result, parsedEditEntries, parsedReadEntries, parsedCommandDetails] + ); const primaryEditEntry = editEntries[0]; const primaryReadEntry = readEntries[0]; diff --git a/packages/app/src/components/tool-call-preview.test.ts b/packages/app/src/components/tool-call-preview.test.ts new file mode 100644 index 000000000..18558d5c8 --- /dev/null +++ b/packages/app/src/components/tool-call-preview.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, it } from "vitest"; + +import { resolveToolCallPreview } from "./tool-call-preview"; +import type { CommandDetails, EditEntry, ReadEntry } from "@/utils/tool-call-parsers"; + +describe("resolveToolCallPreview", () => { + it("prefers parsed hydration payloads when provided", () => { + const parsedEdits: EditEntry[] = [ + { + filePath: "README.md", + diffLines: [ + { type: "header", content: "@@ -1 +1 @@" }, + { type: "remove", content: "-Old" }, + { type: "add", content: "+New" }, + ], + }, + ]; + const parsedReads: ReadEntry[] = [ + { + filePath: "README.md", + content: "# Hydrated\nFinal text\n", + }, + ]; + const parsedCommand: CommandDetails = { + command: "ls", + cwd: "/tmp/hydrated", + output: "README.md\npackages\n", + exitCode: 0, + }; + + const preview = resolveToolCallPreview({ + parsedEditEntries: parsedEdits, + parsedReadEntries: parsedReads, + parsedCommandDetails: parsedCommand, + }); + + expect(preview.editEntries).toBe(parsedEdits); + expect(preview.readEntries).toBe(parsedReads); + expect(preview.commandDetails).toBe(parsedCommand); + }); + + it("falls back to derived parser output when hydration metadata is missing", () => { + const args = { + type: "mcp_tool_use", + id: "call_fallback", + name: "apply_patch", + server: "editor", + input: { + file_path: "README.md", + patch: "*** Begin Patch\n*** Update File: README.md\n@@\n-Old\n+New\n*** End Patch", + }, + }; + const result = { + changes: [ + { + file_path: "README.md", + previous_content: "Old\n", + content: "New\n", + }, + ], + }; + + const preview = resolveToolCallPreview({ + args, + result, + }); + + expect(preview.editEntries[0]?.diffLines.length).toBeGreaterThan(0); + expect( + preview.editEntries[0]?.diffLines.some((line) => line.content.includes("+New")) + ).toBe(true); + expect(preview.commandDetails).toBeNull(); + }); +}); diff --git a/packages/app/src/components/tool-call-preview.ts b/packages/app/src/components/tool-call-preview.ts new file mode 100644 index 000000000..c66ddd571 --- /dev/null +++ b/packages/app/src/components/tool-call-preview.ts @@ -0,0 +1,40 @@ +import { + extractCommandDetails, + extractEditEntries, + extractReadEntries, + type CommandDetails, + type EditEntry, + type ReadEntry, +} from "@/utils/tool-call-parsers"; + +export type ToolCallPreviewSource = { + args?: unknown; + result?: unknown; + parsedEditEntries?: EditEntry[] | undefined; + parsedReadEntries?: ReadEntry[] | undefined; + parsedCommandDetails?: CommandDetails | null | undefined; +}; + +export type ToolCallPreview = { + editEntries: EditEntry[]; + readEntries: ReadEntry[]; + commandDetails: CommandDetails | null | undefined; +}; + +export function resolveToolCallPreview({ + args, + result, + parsedEditEntries, + parsedReadEntries, + parsedCommandDetails, +}: ToolCallPreviewSource): ToolCallPreview { + const fallbackEditEntries = extractEditEntries(args, result); + const fallbackReadEntries = extractReadEntries(result, args); + const fallbackCommandDetails = extractCommandDetails(args, result); + + return { + editEntries: parsedEditEntries ?? fallbackEditEntries, + readEntries: parsedReadEntries ?? fallbackReadEntries, + commandDetails: parsedCommandDetails ?? fallbackCommandDetails, + }; +} diff --git a/plan.md b/plan.md index f87e901d9..9ee8e320b 100644 --- a/plan.md +++ b/plan.md @@ -13,10 +13,15 @@ - Claude history entries now flow through a shared `convertClaudeHistoryEntry` helper that inspects every message for `tool_*` blocks before classifying it as a plain user message, so persisted `tool_result` lines recorded as `type: "user"` convert into completed tool-call events instead of leaving the original spinner stuck in `executing`. Added unit coverage in `packages/server/src/server/agent/providers/claude-agent.test.ts` proving user tool results hydrate correctly and ran `npm run test --workspace=@voice-dev/server -- src/server/agent/providers/claude-agent.test.ts` (passes, though existing SDK integration specs are still slow/flaky by nature). - [x] We still see duplicate tool call pills (loading + completed/failed) in both Codex and Claude sessions, live and hydrated. Track tool call IDs rigorously, dedupe pending/completed entries, and add regression tests covering all providers and hydrate flows. - Hardened the timeline reducer (`packages/app/src/types/stream.ts`) so out-of-order tool events reconcile via provider/server/tool metadata even when call IDs are missing, statuses only move forward (executing → completed/failed), and replayed hydration updates don’t spawn extra pills; added Vitest coverage in `packages/app/src/types/stream.test.ts` proving both Codex and Claude live + hydrated streams stay deduped when completion blocks precede their pending counterparts. Tests run via `npx vitest packages/app/src/types/stream.test.ts`. -- [ ] Permission request cards in the stream header are emitting duplicate key warnings—derive stable per-agent keys (for example `${agentId}:${request.id}` with fallbacks) so React stops complaining. -- [ ] The so-called tests are still fake: convert the stream harness into real Vitest suites co-located with the code, make `npm test` (Vitest) the single entrypoint, and ensure those tests fail today because hydrated tool-call results are missing. -- [ ] Hydrated sessions continue to drop user messages; capture this in a failing test for both Codex and Claude (hydrate snapshot + live replay) and only mark complete once the test passes. -- [ ] Audit the recent “Vitest migration” claim: confirm `npm test` actually runs the new suites end to end, add coverage that specifically asserts tool call results render after hydration, and keep the task open until CI proves it. +- [x] Permission request cards in the stream header are emitting duplicate key warnings—derive stable per-agent keys (for example `${agentId}:${request.id}` with fallbacks) so React stops complaining. + - Added a `key` field on `PendingPermission` along with a shared helper in `SessionContext` that derives `${agentId}:` identifiers for every server-sourced request, stores them in the map, and removes them when the server resolves the request. The stream header now renders permission cards using this stable key instead of recomputing on every render, eliminating the duplicate React keys. +- [x] The so-called tests are still fake: convert the stream harness into real Vitest suites co-located with the code, make `npm test` (Vitest) the single entrypoint, and ensure those tests fail today because hydrated tool-call results are missing. + - Added `packages/app/src/types/stream.harness.test.ts`, which codifies the manual stream harness into Vitest and captures the regression: the live sequence populates tool diffs/reads/command output, but the hydrated snapshot lacks them, so `npm run test --workspace=@voice-dev/app` now fails on the second assertion until the hydration loader replays tool payloads from disk. +- [x] Hydrated sessions continue to drop user messages; capture this in a failing test for both Codex and Claude (hydrate snapshot + live replay) and only mark complete once the test passes. + - Added end-to-end hydration suites in `packages/server/src/server/agent/providers/{claude,codex}-agent.test.ts` that record live timeline updates (including user prompts) and then hydrate from `streamHistory()` after resuming the session; both now assert that the refreshed snapshot still contains the user's prompt marker. Fixed Claude hydration by having `convertClaudeHistoryEntry` emit user_message entries alongside persisted tool blocks, and taught the Codex rollout parser to surface `role: "user"` messages from rollout logs. +- [x] Audit the recent “Vitest migration” claim: confirm `npm test` actually runs the new suites end to end, add coverage that specifically asserts tool call results render after hydration, and keep the task open until CI proves it. + - Added `resolveToolCallPreview` so the `ToolCall` UI explicitly prefers hydrated `parsed*` payloads over fallbacks and wrote `tool-call-preview.test.ts` to cover hydration-vs-fallback behavior. + - Verified `npm test` now drives both workspaces (server suite passes; app suite fails fast on `src/types/stream.harness.test.ts` because hydrated tool payloads are still missing), so CI will surface the regression once the loader fix lands. - [x] AgentInput image picker crashes with "Attempting to launch an unregistered ActivityResultLauncher" when calling Expo ImagePicker; register the launcher properly (or switch to the new async hook) so picking images doesn’t throw and we can attach screenshots again. - Added a dedicated `useImageAttachmentPicker` hook that registers the media picker launcher up front, reuses Expo’s permission hook, restores pending results, and guards against concurrent launches. AgentInput now consumes this hook so tapping the attachment button opens the picker without crashing on Android; `npm run typecheck --workspace=@voice-dev/app` currently fails upstream in `stream.test.ts` before our change, noted in the summary. - [ ] Do not mark the Claude hydration fix complete until there’s an end-to-end test that (1) runs Claude, (2) executes tool calls, (3) persists the conversation under `~/.claude/projects/...`, and (4) hydrates from disk verifying the tool call results render. No test, no checkbox.