From 027eaeb25b83a84ac6c0460606ddd5e1752738f9 Mon Sep 17 00:00:00 2001 From: Serhii Vecherenko Date: Sat, 13 Jun 2026 12:22:55 -0700 Subject: [PATCH] fix(claude): preserve real line numbers in structured patch diffs - thread Claude `structuredPatch` metadata through file change payloads - reuse shared unified-diff helpers for real hunk headers - add mapper and renderer tests for Edit and MultiEdit patch rendering --- .../parts/items/acpToolPayload.test.ts | 32 ++++ src/shared/lineUnifiedDiff.ts | 43 +++--- .../agents/claude/sdkCanonicalMapping.test.ts | 140 ++++++++++++++++++ .../agents/claude/sdkCanonicalMapping.ts | 88 +++++++++++ .../agents/claude/sdkCanonicalMappingState.ts | 20 +++ 5 files changed, 304 insertions(+), 19 deletions(-) diff --git a/src/renderer/components/thread/ChatPane/parts/items/acpToolPayload.test.ts b/src/renderer/components/thread/ChatPane/parts/items/acpToolPayload.test.ts index 26ec93d8..52270380 100644 --- a/src/renderer/components/thread/ChatPane/parts/items/acpToolPayload.test.ts +++ b/src/renderer/components/thread/ChatPane/parts/items/acpToolPayload.test.ts @@ -247,6 +247,38 @@ describe("acpToolPayload", () => { expect(part.text).toContain("+after"); }); + it("preserves Claude structuredPatch real line numbers from metadata.changes", () => { + // Shape emitted by the Claude supervisor from tool_use_result.structuredPatch: + // a full unified diff whose @@ header carries the real file line numbers. + const diff = [ + "diff --git a/src/app.ts b/src/app.ts", + "--- a/src/app.ts", + "+++ b/src/app.ts", + "@@ -11,3 +11,3 @@", + " line 11", + "-const oldValue = true;", + "+const oldValue = false;", + " line 13", + ].join("\n"); + + const payload = { + path: "src/app.ts", + args: { + file_path: "src/app.ts", + old_string: "const oldValue = true;", + new_string: "const oldValue = false;", + }, + result: "Edit applied.", + metadata: { changes: [{ path: "src/app.ts", kind: { type: "update" }, diff }] }, + }; + + const part = extractAcpDiffResultPart(payload); + expect(part.language).toBe("diff"); + expect(part.text).toContain("@@ -11,3 +11,3 @@"); + expect(part.text).not.toContain("@@ -1 +1 @@"); + expect(extractAcpDiffSummary(payload)).toEqual({ added: 1, removed: 1 }); + }); + it("synthesizes diffs and summaries from apply_patch patchText args", () => { const payload = { args: { diff --git a/src/shared/lineUnifiedDiff.ts b/src/shared/lineUnifiedDiff.ts index 4055f0a2..f2800ae2 100644 --- a/src/shared/lineUnifiedDiff.ts +++ b/src/shared/lineUnifiedDiff.ts @@ -35,6 +35,27 @@ export function countLineChangeStats(oldText: string, newText: string): LineChan return { added, removed }; } +/** + * The `diff --git` / `---` / `+++` header lines for a unified diff. `displayPath` + * must already be normalized via {@link normalizeDiffFilePath}. + */ +export function buildDiffHeaderLines( + displayPath: string, + isCreate: boolean, + isDelete: boolean, +): string[] { + return [ + `diff --git a/${displayPath} b/${displayPath}`, + isCreate ? "--- /dev/null" : `--- a/${displayPath}`, + isDelete ? "+++ /dev/null" : `+++ b/${displayPath}`, + ]; +} + +/** Git hunk ranges drop the `,count` suffix when the range covers a single line. */ +export function formatHunkRange(start: number, count: number): string { + return count === 1 ? String(start) : `${start},${count}`; +} + /** Build a minimal unified diff suitable for InlineDiffView / git-diff-view. */ export function buildLineUnifiedDiff(path: string, oldText: string, newText: string): string { const displayPath = normalizeDiffFilePath(path); @@ -42,21 +63,7 @@ export function buildLineUnifiedDiff(path: string, oldText: string, newText: str const isCreate = oldText.length === 0; const isDelete = newText.length === 0; const hunks = buildHunks(ops); - if (hunks.length === 0) { - return [ - `diff --git a/${displayPath} b/${displayPath}`, - isCreate ? "--- /dev/null" : `--- a/${displayPath}`, - isDelete ? "+++ /dev/null" : `+++ b/${displayPath}`, - "", - ].join("\n"); - } - return [ - `diff --git a/${displayPath} b/${displayPath}`, - isCreate ? "--- /dev/null" : `--- a/${displayPath}`, - isDelete ? "+++ /dev/null" : `+++ b/${displayPath}`, - ...hunks, - "", - ].join("\n"); + return [...buildDiffHeaderLines(displayPath, isCreate, isDelete), ...hunks, ""].join("\n"); } export function diffLineOps(oldText: string, newText: string): DiffOp[] { @@ -175,13 +182,11 @@ function splitLines(text: string): string[] { /** Git unified-diff old-file range (`-0,0` when the hunk only adds lines). */ function formatOldHunkRange(count: number, linesBefore: number): string { if (count === 0) return "0,0"; - const start = linesBefore + 1; - return count === 1 ? String(start) : `${start},${count}`; + return formatHunkRange(linesBefore + 1, count); } /** Git unified-diff new-file range. */ function formatNewHunkRange(count: number, linesBefore: number): string { if (count === 0) return "0,0"; - const start = linesBefore + 1; - return count === 1 ? String(start) : `${start},${count}`; + return formatHunkRange(linesBefore + 1, count); } diff --git a/src/supervisor/agents/claude/sdkCanonicalMapping.test.ts b/src/supervisor/agents/claude/sdkCanonicalMapping.test.ts index 005be0f1..9c1c8e8d 100644 --- a/src/supervisor/agents/claude/sdkCanonicalMapping.test.ts +++ b/src/supervisor/agents/claude/sdkCanonicalMapping.test.ts @@ -1151,6 +1151,146 @@ describe("sdkCanonicalMapping — tool use", () => { ]); }); + it("threads structuredPatch real line numbers onto Edit file changes", () => { + const state = createClaudeMapperState("thread-1"); + const args = { + file_path: "src/app.ts", + old_string: "const oldValue = true;", + new_string: "const oldValue = false;", + }; + + mapClaudeSdkMessage( + streamEvent({ + type: "content_block_start", + index: 0, + content_block: { type: "tool_use", id: "toolu_edit", name: "Edit", input: args }, + }), + state, + ); + + const completed = mapClaudeSdkMessage( + { + type: "user", + session_id: "claude-session", + parent_tool_use_id: null, + tool_use_result: { + filePath: "src/app.ts", + oldString: args.old_string, + newString: args.new_string, + originalFile: "line 11\nconst oldValue = true;\nline 13\n", + userModified: false, + replaceAll: false, + structuredPatch: [ + { + oldStart: 11, + oldLines: 3, + newStart: 11, + newLines: 3, + lines: [ + " line 11", + "-const oldValue = true;", + "+const oldValue = false;", + " line 13", + ], + }, + ], + }, + message: { + role: "user", + content: [{ type: "tool_result", tool_use_id: "toolu_edit", content: "Edit applied." }], + }, + } as unknown as SDKMessage, + state, + ); + + const updated = completed.find((event) => event.type === "item.updated"); + const payload = (updated as { payload?: Record }).payload; + const metadata = payload?.metadata as { changes?: Array<{ path?: string; diff?: string }> }; + expect(metadata.changes?.[0]?.path).toBe("src/app.ts"); + // Real file line numbers from structuredPatch, not the synthetic `@@ -1 +1 @@`. + expect(metadata.changes?.[0]?.diff).toContain("@@ -11,3 +11,3 @@"); + expect(metadata.changes?.[0]?.diff).toContain("-const oldValue = true;"); + expect(metadata.changes?.[0]?.diff).toContain("+const oldValue = false;"); + // The human-readable result text is still preserved for the accordion. + expect(payload?.result).toBe("Edit applied."); + }); + + it("emits every structuredPatch hunk for a MultiEdit at its real start line", () => { + const state = createClaudeMapperState("thread-1"); + const args = { file_path: "src/app.ts", edits: [] }; + + mapClaudeSdkMessage( + streamEvent({ + type: "content_block_start", + index: 0, + content_block: { type: "tool_use", id: "toolu_multi", name: "MultiEdit", input: args }, + }), + state, + ); + + const completed = mapClaudeSdkMessage( + { + type: "user", + session_id: "claude-session", + parent_tool_use_id: null, + tool_use_result: { + filePath: "src/app.ts", + structuredPatch: [ + { oldStart: 5, oldLines: 1, newStart: 5, newLines: 1, lines: ["-a", "+A"] }, + { oldStart: 40, oldLines: 1, newStart: 40, newLines: 2, lines: ["-b", "+B", "+C"] }, + ], + }, + message: { + role: "user", + content: [{ type: "tool_result", tool_use_id: "toolu_multi", content: "ok" }], + }, + } as unknown as SDKMessage, + state, + ); + + const updated = completed.find((event) => event.type === "item.updated"); + const diff = (updated as { payload?: { metadata?: { changes?: Array<{ diff?: string }> } } }) + .payload?.metadata?.changes?.[0]?.diff; + expect(diff).toContain("@@ -5 +5 @@"); + expect(diff).toContain("@@ -40 +40,2 @@"); + }); + + it("ignores a structuredPatch whose filePath does not match the edited file", () => { + const state = createClaudeMapperState("thread-1"); + const args = { file_path: "src/app.ts", old_string: "a", new_string: "b" }; + + mapClaudeSdkMessage( + streamEvent({ + type: "content_block_start", + index: 0, + content_block: { type: "tool_use", id: "toolu_edit", name: "Edit", input: args }, + }), + state, + ); + + const completed = mapClaudeSdkMessage( + { + type: "user", + session_id: "claude-session", + parent_tool_use_id: null, + tool_use_result: { + filePath: "src/other.ts", + structuredPatch: [ + { oldStart: 9, oldLines: 1, newStart: 9, newLines: 1, lines: ["-a", "+b"] }, + ], + }, + message: { + role: "user", + content: [{ type: "tool_result", tool_use_id: "toolu_edit", content: "ok" }], + }, + } as unknown as SDKMessage, + state, + ); + + const updated = completed.find((event) => event.type === "item.updated"); + expect((updated as { payload?: Record }).payload?.metadata).toBeUndefined(); + }); + it("counts Claude Write content lines as create diff summary", () => { const state = createClaudeMapperState("thread-1"); const args = { diff --git a/src/supervisor/agents/claude/sdkCanonicalMapping.ts b/src/supervisor/agents/claude/sdkCanonicalMapping.ts index 9e7bb609..b54643a1 100644 --- a/src/supervisor/agents/claude/sdkCanonicalMapping.ts +++ b/src/supervisor/agents/claude/sdkCanonicalMapping.ts @@ -17,6 +17,11 @@ import type { UserInputOption, } from "@/shared/contracts"; import { readDiffSummary, readFileChangePath } from "../fileChangeSummary"; +import { + buildDiffHeaderLines, + formatHunkRange, + normalizeDiffFilePath, +} from "@/shared/lineUnifiedDiff"; import { createContextUsageEvent, readNonNegativeInteger } from "../contextUsage"; import { buildQuestionAnswerEvents } from "../questionAnswerEvents"; import { @@ -40,6 +45,7 @@ import { import { createClaudeMapperState, type ClaudeMapperState, + type FileChangeMetadata, type PlanAggregatorRole, type TextItemState, type ToolItemState, @@ -758,6 +764,7 @@ function toolPayload( ...(diffSummary ? { diffSummary } : {}), args: tool.input, ...(result !== undefined ? { result } : {}), + ...(tool.fileChangeMetadata ? { metadata: tool.fileChangeMetadata } : {}), ...errorFields, }; } @@ -796,6 +803,80 @@ function inferFileChangeKind(toolName: string): "create" | "edit" | "delete" { return "edit"; } +interface StructuredPatchHunk { + oldStart: number; + oldLines: number; + newStart: number; + newLines: number; + lines: string[]; +} + +function readStructuredPatchHunks(toolUseResult: unknown): StructuredPatchHunk[] | undefined { + if (!toolUseResult || typeof toolUseResult !== "object") return undefined; + const patch = (toolUseResult as Record).structuredPatch; + if (!Array.isArray(patch) || patch.length === 0) return undefined; + const hunks: StructuredPatchHunk[] = []; + for (const entry of patch) { + if (!entry || typeof entry !== "object") continue; + const { oldStart, oldLines, newStart, newLines, lines } = entry as Record; + if ( + typeof oldStart !== "number" || + typeof oldLines !== "number" || + typeof newStart !== "number" || + typeof newLines !== "number" || + !Array.isArray(lines) + ) { + continue; + } + hunks.push({ + oldStart, + oldLines, + newStart, + newLines, + lines: lines.filter((line): line is string => typeof line === "string"), + }); + } + return hunks.length > 0 ? hunks : undefined; +} + +/** + * Build a `metadata.changes[]` entry from the Claude SDK's + * `tool_use_result.structuredPatch` (Edit / MultiEdit / Write output). The hunk + * headers carry the real file line numbers (`oldStart` / `newStart`), so a full + * unified diff assembled here flows through the renderer's existing structured- + * changes passthrough (the same path Codex uses) and InlineDiffView renders true + * line numbers instead of the synthetic `@@ -1 +1 @@` synthesized from + * `old_string` / `new_string`. + * + * `expectedPath` guards against a `tool_use_result` that belongs to a different + * tool_result block in the same user message (Claude emits one per message in + * practice, but the SDK field is untyped and shared across the message). + */ +function fileChangeMetadataFromToolResult( + toolUseResult: unknown, + expectedPath: string | undefined, +): FileChangeMetadata | undefined { + const hunks = readStructuredPatchHunks(toolUseResult); + if (!hunks) return undefined; + const record = toolUseResult as Record; + const filePath = typeof record.filePath === "string" ? record.filePath : undefined; + const resultPath = filePath && filePath.length > 0 ? filePath : expectedPath; + if (!resultPath) return undefined; + if (expectedPath && filePath !== undefined && filePath !== expectedPath) return undefined; + const isCreate = record.originalFile === null || record.type === "create"; + const displayPath = normalizeDiffFilePath(resultPath); + const body = hunks.flatMap((hunk) => [ + `@@ -${formatHunkRange(hunk.oldStart, hunk.oldLines)} +${formatHunkRange(hunk.newStart, hunk.newLines)} @@`, + ...hunk.lines, + ]); + const diff = [...buildDiffHeaderLines(displayPath, isCreate, false), ...body].join("\n"); + return { + changes: [ + { path: resultPath, kind: { type: isCreate ? "add" : "update", move_path: null }, diff }, + ], + }; +} + function extractPlanSteps( input: Record, ): Array<{ step: string; status: "pending" | "in_progress" | "completed" }> { @@ -1621,6 +1702,13 @@ function mapClaudeSdkMessageInner( continue; } const isError = obj.is_error === true; + if (tool.itemType === "file_change" && !isError) { + const metadata = fileChangeMetadataFromToolResult( + (message as { tool_use_result?: unknown }).tool_use_result, + readFileChangePath(tool.input), + ); + if (metadata) tool.fileChangeMetadata = metadata; + } const stream = tool.itemType === "command_execution" ? "command_output" diff --git a/src/supervisor/agents/claude/sdkCanonicalMappingState.ts b/src/supervisor/agents/claude/sdkCanonicalMappingState.ts index 6d8dcdbd..b9436f6d 100644 --- a/src/supervisor/agents/claude/sdkCanonicalMappingState.ts +++ b/src/supervisor/agents/claude/sdkCanonicalMappingState.ts @@ -11,6 +11,20 @@ export interface TextItemState { export type PlanAggregatorRole = "TodoWrite" | "TaskCreate" | "TaskUpdate" | "TaskStop"; +/** + * Structured per-file diff metadata built from the SDK's + * `tool_use_result.structuredPatch` (Edit/MultiEdit/Write output). Mirrors the + * `metadata.changes[]` shape that Codex/OpenCode emit so the renderer's + * existing structured-changes passthrough can render real hunk line numbers. + */ +export interface FileChangeMetadata { + changes: Array<{ + path?: string; + kind: { type: string; move_path: string | null }; + diff: string; + }>; +} + export interface ToolItemState { itemId: string; itemType: CanonicalItemType; @@ -26,6 +40,12 @@ export interface ToolItemState { * canonical `plan` item events instead. */ planAggregatorRole?: PlanAggregatorRole; + /** + * Real-line-number diff derived from the tool result's `structuredPatch`, + * attached to the `file_change` payload so InlineDiffView shows true file + * line numbers instead of a synthetic `@@ -1 +1 @@` header. + */ + fileChangeMetadata?: FileChangeMetadata; } export interface ClaudeMapperState {