diff --git a/src/main/native-chat/transcript-line-decoders-codex.ts b/src/main/native-chat/transcript-line-decoders-codex.ts index 8c55ec4a69f..d70bd7a7c80 100644 --- a/src/main/native-chat/transcript-line-decoders-codex.ts +++ b/src/main/native-chat/transcript-line-decoders-codex.ts @@ -202,29 +202,17 @@ function codexTurnItemBlocks(content: unknown): NativeChatBlock[] { return blocks } -/** A call's arguments arrive as a string holding JSON, so decode them once here - * rather than leaving every consumer to unescape the payload for itself. The - * transcript is untrusted, so anything that is not a JSON object stays exactly - * as it arrived. */ +/** The argument payload is passed through exactly as it arrived. Decoding it + * here would change the shape every `.input` consumer sees — including the ask + * surface, which reads a question shape out of any tool's input — so the one + * consumer that needs structure decodes it for itself. */ function codexCallInput(payload: Record): unknown { - const args = payload.arguments - if (args !== undefined) { - return typeof args === 'string' ? (parsedJsonObject(args) ?? args) : args + if (payload.arguments !== undefined) { + return payload.arguments } return payload.input ?? payload.action ?? null } -function parsedJsonObject(value: string): Record | null { - try { - const parsed: unknown = JSON.parse(value) - return typeof parsed === 'object' && parsed !== null && !Array.isArray(parsed) - ? (parsed as Record) - : null - } catch { - return null - } -} - function codexToolResult(output: unknown): NativeChatBlock { const record = asRecord(output) const isError = record?.success === false || record?.is_error === true diff --git a/src/main/native-chat/transcript-reader-codex-history-mode.test.ts b/src/main/native-chat/transcript-reader-codex-history-mode.test.ts index 23e503ae0a4..5d18bec8c85 100644 --- a/src/main/native-chat/transcript-reader-codex-history-mode.test.ts +++ b/src/main/native-chat/transcript-reader-codex-history-mode.test.ts @@ -2,6 +2,7 @@ import { mkdtemp, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, describe, expect, it } from 'vitest' +import { extractPendingAsk } from '../../shared/native-chat-ask' import { decodeCodexTranscriptLine } from './transcript-line-decoders-codex' import { readNativeChatTranscript } from './transcript-reader' import { readNativeChatTranscriptTail } from './transcript-tail-reader' @@ -248,7 +249,7 @@ describe('Codex transcript history modes', () => { }) }) - it('decodes a call argument payload once, so consumers see real values', () => { + it('passes an argument payload through untouched, so no consumer changes shape', () => { const call = decodeCodexTranscriptLine( JSON.stringify({ type: 'response_item', @@ -256,7 +257,7 @@ describe('Codex transcript history modes', () => { type: 'function_call', id: 'call-2', name: 'shell', - arguments: JSON.stringify({ command: ['bash', '-lc', 'echo one\necho two'] }) + arguments: '{"command":["bash","-lc","echo hi"]}' } }), 'fallback-args' @@ -265,19 +266,25 @@ describe('Codex transcript history modes', () => { expect(call?.blocks[0]).toMatchObject({ type: 'tool-call', name: 'shell', - input: { command: ['bash', '-lc', 'echo one\necho two'] } + input: '{"command":["bash","-lc","echo hi"]}' }) }) - it('leaves an argument payload that is not an object exactly as it arrived', () => { + it('does not raise a question card from an unrelated tool that carries a questions payload', () => { const call = decodeCodexTranscriptLine( JSON.stringify({ type: 'response_item', - payload: { type: 'function_call', id: 'call-3', name: 'shell', arguments: '{ not json' } + payload: { + type: 'function_call', + id: 'call-3', + name: 'some_mcp_tool', + arguments: '{"questions":[{"question":"Which branch?","options":["main","dev"]}]}' + } }), - 'fallback-bad-args' + 'fallback-questions' ) - expect(call?.blocks[0]).toMatchObject({ type: 'tool-call', input: '{ not json' }) + expect(call).not.toBeNull() + expect(extractPendingAsk(call ? [call] : [])).toBeNull() }) }) diff --git a/src/main/native-chat/transcript-reader.test.ts b/src/main/native-chat/transcript-reader.test.ts index 8de4e8892ec..ff48548804d 100644 --- a/src/main/native-chat/transcript-reader.test.ts +++ b/src/main/native-chat/transcript-reader.test.ts @@ -246,11 +246,10 @@ describe('readNativeChatTranscript (codex)', () => { expect(roles).toEqual(['user', 'reasoning', 'assistant', 'tool', 'assistant']) const call = result.messages.find((m) => m.blocks[0]?.type === 'tool-call') - // The argument payload is decoded once here rather than by every consumer. expect(call?.blocks[0]).toEqual({ type: 'tool-call', name: 'shell', - input: { command: ['bash', '-lc', 'make'] } + input: '{"command":["bash","-lc","make"]}' }) const toolResult = result.messages.find((m) => m.blocks[0]?.type === 'tool-result') diff --git a/src/renderer/src/components/native-chat/NativeChatToolRun.test.tsx b/src/renderer/src/components/native-chat/NativeChatToolRun.test.tsx index 82e6806d845..9fc0ff16788 100644 --- a/src/renderer/src/components/native-chat/NativeChatToolRun.test.tsx +++ b/src/renderer/src/components/native-chat/NativeChatToolRun.test.tsx @@ -32,6 +32,9 @@ describe('NativeChatToolRun', () => { { type: 'tool-call', name: 'apply_patch', + // The patch lives on the call in this lane, so the provider's own + // completion is what says the edit landed. + state: 'completed', input: { changes: [ { diff --git a/src/renderer/src/components/native-chat/NativeChatToolRun.tsx b/src/renderer/src/components/native-chat/NativeChatToolRun.tsx index 914faa9361c..be961aa9fcc 100644 --- a/src/renderer/src/components/native-chat/NativeChatToolRun.tsx +++ b/src/renderer/src/components/native-chat/NativeChatToolRun.tsx @@ -166,14 +166,10 @@ type EditCardModel = { const NO_EDIT_CARDS: EditCardModel = { editCards: new Map(), consumedResults: new Set() } -/** An edit renders as one card, so its result block is folded into the call. A - * call still in flight keeps the generic tool view rather than a card that - * states the edit as made; a failed one is filtered out by the model itself, so - * its result stays visible as the provider's error. */ -function buildEditCards( - blocks: NativeChatBlock[], - activeTurnIsWorking: boolean | undefined -): EditCardModel { +/** An edit renders as one card, so its result block is folded into the call. The + * model decides which calls have landed; a call that has not keeps the generic + * tool view, its result still visible as the provider's own error. */ +function buildEditCards(blocks: NativeChatBlock[]): EditCardModel { const editCards: EditCardModel['editCards'] = new Map() const consumedResults: EditCardModel['consumedResults'] = new Set() for (const [index, pair] of pairToolBlocks(blocks).entries()) { @@ -181,10 +177,6 @@ function buildEditCards( if (!call || !isEditToolName(call.name)) { continue } - // No lifecycle and no result yet, on a turn still working: not landed. - if (call.state == null && activeTurnIsWorking === true && pair.result === undefined) { - continue - } const files = editFilesFromToolPair({ name: call.name, input: call.input, @@ -251,8 +243,8 @@ export function NativeChatToolRun({ // Diffing every edit is the run's most expensive work, so a collapsed run — // which renders none of it — never pays for it. const { editCards, consumedResults } = useMemo( - () => (open ? buildEditCards(blocks, activeTurnIsWorking) : NO_EDIT_CARDS), - [open, blocks, activeTurnIsWorking] + () => (open ? buildEditCards(blocks) : NO_EDIT_CARDS), + [open, blocks] ) const ActiveToolIcon = latestActiveCall && COMMAND_TOOL_NAMES.has(normalizedToolName(latestActiveCall.name)) diff --git a/src/shared/native-chat-begin-patch.ts b/src/shared/native-chat-begin-patch.ts index f409108d188..4598157288b 100644 --- a/src/shared/native-chat-begin-patch.ts +++ b/src/shared/native-chat-begin-patch.ts @@ -8,15 +8,12 @@ const MOVE_HEADER = /^\*\*\* Move to: (.+)$/ /** Envelope structure that carries no file content of its own. */ const CONTROL_LINE = /^\*\*\* (?:End of File|Environment ID:)/ -/** The envelope reaches a tool call as one of its argument values, either whole - * or as an element of the argument vector the agent runs. Recover its text. */ +/** The envelope reaches a command tool as one of its patch or command + * arguments, either whole or as one word of the argument vector it runs. + * Recover its text. Callers must gate this on the tool being one that runs a + * patch: a file's own contents may quote an envelope. */ export function unwrapBeginPatch(input: unknown): string | null { - const source = - typeof input === 'string' - ? input - : typeof input === 'object' && input !== null - ? envelopeArgument(input as Record) - : null + const source = envelopeSource(input) if (!source) { return null } @@ -34,10 +31,43 @@ export function unwrapBeginPatch(input: unknown): string | null { return source.slice(start, end + END.length) } -/** Any argument value may hold the envelope, including one word of an argument - * vector, so look at the values rather than guessing at key names. */ +/** The arguments that carry a patch or the command line that applies one. Only + * these are searched: any other value is data the tool operates on, and a file + * whose own contents quote an envelope would otherwise be read as a patch + * against some other file entirely. */ +const ENVELOPE_ARGUMENTS = ['input', 'command', 'patch', 'arguments', 'script'] as const + +/** The call payload may itself be a string holding JSON. Decoding it here, in + * the one consumer that needs its structure, keeps every other reader of the + * call input seeing exactly what the provider sent. */ +function envelopeSource(input: unknown): string | null { + if (typeof input === 'string') { + const record = jsonRecord(input) + return record ? envelopeArgument(record) : input + } + return typeof input === 'object' && input !== null + ? envelopeArgument(input as Record) + : null +} + +function jsonRecord(value: string): Record | null { + if (!value.trimStart().startsWith('{')) { + return null + } + try { + const parsed: unknown = JSON.parse(value) + return typeof parsed === 'object' && parsed !== null && !Array.isArray(parsed) + ? (parsed as Record) + : null + } catch { + return null + } +} + +/** A command tool's argument is a vector, so the envelope sits one level in. */ function envelopeArgument(record: Record): string | null { - for (const value of Object.values(record)) { + for (const key of ENVELOPE_ARGUMENTS) { + const value = record[key] if (typeof value === 'string' && value.includes(BEGIN)) { return value } @@ -104,21 +134,19 @@ export function editFilesFromBeginPatch(envelope: string): NativeChatEditFile[] }) ] } - // The first chunk of an update may carry no hunk header at all, and a file - // whose body cannot be read as a hunk would otherwise vanish from a - // multi-file envelope with nothing to say it was dropped. + // The first chunk of an update may carry no hunk header at all, and a + // section may carry no body either. The envelope named the file, so it is + // reported with whatever rows it has rather than dropped from a multi-file + // envelope with nothing to say it went missing. const parsed = editLinesFromUnifiedPatch(body, { implicitFirstHunk: true }) - if (!parsed) { - return [] - } return [ finalizeEditFile({ path: moved ?? section.path, oldPath: moved ? section.path : null, changeKind: moved ? 'renamed' : 'edited', - lines: parsed.lines, - lineNumbersKnown: parsed.lineNumbersKnown, - truncated: parsed.truncated + lines: parsed?.lines ?? [], + lineNumbersKnown: parsed?.lineNumbersKnown ?? false, + truncated: parsed?.truncated ?? false }) ] }) diff --git a/src/shared/native-chat-edit-normalize.test.ts b/src/shared/native-chat-edit-normalize.test.ts index 0fd64b1ab02..4f834185bbc 100644 --- a/src/shared/native-chat-edit-normalize.test.ts +++ b/src/shared/native-chat-edit-normalize.test.ts @@ -7,6 +7,13 @@ import { unwrapBeginPatch } from './native-chat-begin-patch' const gutter = (files: ReturnType): (number | null)[] => (files ?? []).flatMap((file) => file.lines.map((line) => unifiedLineNumber(line))) +/** A card takes evidence the edit landed, so these cases report the call as + * complete. Cases about the lifecycle itself pass their own state. */ +const settledFiles = ( + pair: Parameters[0] +): ReturnType => + editFilesFromToolPair({ state: 'completed', ...pair }) + describe('editLinesFromUnifiedPatch', () => { it('numbers deletes from the old side and adds from the new side', () => { const parsed = editLinesFromUnifiedPatch('@@ -12,3 +12,3 @@\n ctx\n-was\n+now\n tail') @@ -115,13 +122,13 @@ describe('unwrapBeginPatch', () => { it('declines an envelope with no closing marker rather than swallowing the command line', () => { const command = 'bash -c "*** Begin Patch\n*** Update File: a.ts\n@@\n-x\n+y" && echo ok' expect(unwrapBeginPatch(command)).toBeNull() - expect(editFilesFromToolPair({ name: 'shell', input: command })).toBeNull() + expect(settledFiles({ name: 'shell', input: command })).toBeNull() }) }) describe('editFilesFromToolPair', () => { it('renders an apply_patch run through a command tool, which produced no diff', () => { - const files = editFilesFromToolPair({ + const files = settledFiles({ name: 'exec', input: { command: [ @@ -141,7 +148,7 @@ describe('editFilesFromToolPair', () => { }) it('numbers an added file from 1', () => { - const files = editFilesFromToolPair({ + const files = settledFiles({ name: 'exec', input: '*** Begin Patch\n*** Add File: new.ts\n+one\n+two\n*** End Patch' }) @@ -151,7 +158,7 @@ describe('editFilesFromToolPair', () => { }) it('keeps a file whose update body carries no hunk header', () => { - const files = editFilesFromToolPair({ + const files = settledFiles({ name: 'apply_patch', input: { input: @@ -164,7 +171,7 @@ describe('editFilesFromToolPair', () => { }) it('does not render envelope control lines as file content', () => { - const files = editFilesFromToolPair({ + const files = settledFiles({ name: 'apply_patch', input: { input: @@ -175,7 +182,7 @@ describe('editFilesFromToolPair', () => { }) it('reports a delete that names the file and carries no body', () => { - const files = editFilesFromToolPair({ + const files = settledFiles({ name: 'apply_patch', input: { input: '*** Begin Patch\n*** Delete File: gone.ts\n*** End Patch' } }) @@ -186,7 +193,7 @@ describe('editFilesFromToolPair', () => { }) it('reads a CRLF envelope, whose markers otherwise match nothing', () => { - const files = editFilesFromToolPair({ + const files = settledFiles({ name: 'apply_patch', input: { input: @@ -199,7 +206,7 @@ describe('editFilesFromToolPair', () => { }) it('marks the break between resolved hunks that sit far apart', () => { - const files = editFilesFromToolPair({ + const files = settledFiles({ name: 'Edit', input: { file_path: '/repo/a.ts' }, result: { @@ -220,7 +227,7 @@ describe('editFilesFromToolPair', () => { }) it('reads a move header as a rename', () => { - const files = editFilesFromToolPair({ + const files = settledFiles({ name: 'exec', input: '*** Begin Patch\n*** Update File: old.ts\n*** Move to: new.ts\n@@\n-a\n+b\n*** End Patch' @@ -231,7 +238,7 @@ describe('editFilesFromToolPair', () => { }) it('interleaves a Claude snippet pair without claiming line positions', () => { - const files = editFilesFromToolPair({ + const files = settledFiles({ name: 'Edit', input: { file_path: '/repo/a.ts', @@ -244,7 +251,7 @@ describe('editFilesFromToolPair', () => { }) it('prefers the resolved hunks on the result over the snippet pair', () => { - const files = editFilesFromToolPair({ + const files = settledFiles({ name: 'Edit', input: { file_path: '/repo/a.ts', old_string: 'was', new_string: 'now' }, result: { @@ -267,7 +274,7 @@ describe('editFilesFromToolPair', () => { }) it('treats a Write the provider reported as a creation as an added file', () => { - const files = editFilesFromToolPair({ + const files = settledFiles({ name: 'Write', input: { file_path: '/repo/new.ts', content: 'one\ntwo\n' }, result: { output: 'File created successfully at: /repo/new.ts' } @@ -277,14 +284,14 @@ describe('editFilesFromToolPair', () => { }) it('does not claim a creation for a Write over an existing file', () => { - const overwrite = editFilesFromToolPair({ + const overwrite = settledFiles({ name: 'Write', input: { file_path: '/repo/a.ts', content: 'one\ntwo\n' }, result: { output: 'The file /repo/a.ts has been updated.' } }) expect(overwrite?.[0]?.changeKind).toBe('edited') // With no result at all there is no evidence of a creation either. - const unreported = editFilesFromToolPair({ + const unreported = settledFiles({ name: 'Write', input: { file_path: '/repo/a.ts', content: 'one\n' } }) @@ -292,7 +299,7 @@ describe('editFilesFromToolPair', () => { }) it('reads a MultiEdit, whose snippet pairs sit in edits[]', () => { - const files = editFilesFromToolPair({ + const files = settledFiles({ name: 'MultiEdit', input: { file_path: '/repo/a.ts', @@ -321,7 +328,7 @@ describe('editFilesFromToolPair', () => { }) it('drops the gutter numbers whenever they locate a snippet rather than the file', () => { - const files = editFilesFromToolPair({ + const files = settledFiles({ name: 'Edit', input: { file_path: '/repo/a.ts', old_string: 'keep\nwas', new_string: 'keep\nnow' } }) @@ -335,14 +342,14 @@ describe('editFilesFromToolPair', () => { it('renders no card for an edit the provider rejected or has not landed', () => { const failedInput = { file_path: '/repo/a.ts', old_string: 'missing', new_string: 'now' } expect( - editFilesFromToolPair({ + settledFiles({ name: 'Edit', input: failedInput, result: { output: 'String to replace not found in file.', isError: true } }) ).toBeNull() expect( - editFilesFromToolPair({ + settledFiles({ name: 'apply_patch', input: { changes: [{ path: 'a.ts', kind: { type: 'update' }, diff: '@@ -1 +1 @@\n-a\n+b' }] @@ -350,20 +357,20 @@ describe('editFilesFromToolPair', () => { state: 'failed' }) ).toBeNull() - expect(editFilesFromToolPair({ name: 'Edit', input: failedInput, state: 'running' })).toBeNull() + expect(settledFiles({ name: 'Edit', input: failedInput, state: 'running' })).toBeNull() }) it('does not read a command tool result as a file edit', () => { const patch = 'diff --git a/a.ts b/a.ts\n--- a/a.ts\n+++ b/a.ts\n@@ -1 +1 @@\n-was\n+now' expect( - editFilesFromToolPair({ + settledFiles({ name: 'exec', input: { command: 'git diff' }, result: { output: patch } }) ).toBeNull() // The structured journal's `Diff` item carries its patch only on the result. - const diffed = editFilesFromToolPair({ + const diffed = settledFiles({ name: 'Diff', input: { path: '/repo/a.ts' }, result: { output: patch } @@ -373,7 +380,7 @@ describe('editFilesFromToolPair', () => { }) it('reports truncation when the content runs past the character cap', () => { - const files = editFilesFromToolPair({ + const files = settledFiles({ name: 'Write', input: { file_path: '/repo/a.ts', content: `${'x'.repeat(MAX_EDIT_CHARS)}\nlast\n` } }) @@ -383,7 +390,7 @@ describe('editFilesFromToolPair', () => { }) it('reads Codex structured changes, stripping the move marker from the body', () => { - const files = editFilesFromToolPair({ + const files = settledFiles({ name: 'apply_patch', input: { changes: [ @@ -401,7 +408,7 @@ describe('editFilesFromToolPair', () => { }) it('reads a Codex add change, which arrives as raw content with no hunk header', () => { - const files = editFilesFromToolPair({ + const files = settledFiles({ name: 'apply_patch', input: { changes: [{ path: 'new.ts', kind: { type: 'add' }, diff: 'one\ntwo' }] } }) @@ -409,8 +416,67 @@ describe('editFilesFromToolPair', () => { expect(files?.[0]?.added).toBe(2) }) + it('renders the file a write actually wrote, not one its content quotes', () => { + const files = settledFiles({ + name: 'Write', + input: { + file_path: 'docs/patch-format.md', + content: + 'Example:\n\n*** Begin Patch\n*** Update File: src/victim.ts\n@@\n-a\n+b\n*** End Patch\n' + }, + result: { output: 'File created successfully at: docs/patch-format.md' } + }) + expect(files?.map((file) => file.path)).toEqual(['docs/patch-format.md']) + expect(files?.[0]?.lines.some((line) => line.text.includes('Begin Patch'))).toBe(true) + }) + + it('finds an envelope in a command payload that arrived as JSON text', () => { + const envelope = '*** Begin Patch\n*** Update File: src/a.ts\n@@\n-was\n+now\n*** End Patch' + const files = settledFiles({ + name: 'shell', + input: JSON.stringify({ command: ['bash', '-lc', envelope], workdir: '/repo' }) + }) + expect(files?.[0]?.path).toBe('src/a.ts') + expect(files?.[0]?.added).toBe(1) + }) + + it('renders no card for a call the turn never answered', () => { + const input = { file_path: '/repo/a.ts', old_string: 'was', new_string: 'now' } + // No lifecycle and no result: nothing says the edit was applied. + expect(editFilesFromToolPair({ name: 'Edit', input })).toBeNull() + expect(editFilesFromToolPair({ name: 'Edit', input, state: 'completed' })).toHaveLength(1) + expect(editFilesFromToolPair({ name: 'Edit', input, result: { output: 'ok' } })).toHaveLength(1) + }) + + it('splits a multi-file patch into one card per file', () => { + const files = settledFiles({ + name: 'Diff', + input: { path: '/repo/one.ts' }, + result: { + output: + 'diff --git a/one.ts b/one.ts\n--- a/one.ts\n+++ b/one.ts\n@@ -1,1 +1,1 @@\n-first\n+FIRST\n' + + 'diff --git a/two.ts b/two.ts\n--- a/two.ts\n+++ b/two.ts\n@@ -10,1 +10,1 @@\n-second\n+SECOND' + } + }) + expect(files?.map((file) => file.path)).toEqual(['one.ts', 'two.ts']) + expect(files?.[0]?.lines.map((line) => line.text)).toEqual(['first', 'FIRST']) + expect(gutter(files?.slice(1))).toEqual([10, 10]) + }) + + it('keeps a file whose envelope section carries no body at all', () => { + const files = settledFiles({ + name: 'apply_patch', + input: { + input: + '*** Begin Patch\n*** Update File: first.ts\n@@\n-a\n+b\n*** Update File: second.ts\n*** Update File: third.ts\n@@\n-c\n+d\n*** End Patch' + } + }) + expect(files?.map((file) => file.path)).toEqual(['first.ts', 'second.ts', 'third.ts']) + expect(files?.[1]?.lines).toEqual([]) + }) + it('returns null for a tool that did not edit a file', () => { - expect(editFilesFromToolPair({ name: 'Bash', input: { command: 'ls' } })).toBeNull() + expect(settledFiles({ name: 'Bash', input: { command: 'ls' } })).toBeNull() expect(isEditToolName('Bash')).toBe(false) expect(isEditToolName('Edit')).toBe(true) }) diff --git a/src/shared/native-chat-edit-normalize.ts b/src/shared/native-chat-edit-normalize.ts index 6e2f8b610c0..e25d43a79bf 100644 --- a/src/shared/native-chat-edit-normalize.ts +++ b/src/shared/native-chat-edit-normalize.ts @@ -6,7 +6,11 @@ import { type NativeChatEditFile, type NativeChatEditLine } from './native-chat-edit-model' -import { editLinesFromUnifiedPatch, editLinesFromWholeFile } from './native-chat-unified-patch' +import { + editLinesFromUnifiedPatch, + editLinesFromWholeFile, + unifiedPatchSections +} from './native-chat-unified-patch' import type { NativeChatEditPatch } from './native-chat-types' // `NotebookEdit` is deliberately absent: its input carries only the new cell @@ -66,7 +70,10 @@ function linesFromEditPatch(patch: NativeChatEditPatch): NativeChatEditLine[] { } /** A whole-content write looks identical whether it created the file or - * overwrote one, so only positive evidence may claim a creation. */ + * overwrote one, so only positive evidence may claim a creation. With no + * evidence either way this errs toward the weaker claim: calling a creation an + * edit is imprecise, while calling an overwrite a creation is false and paints + * an existing file as wholly new. */ const CREATED_FILE_RESULT = /^\s*File created successfully/ function wholeContentChangeKind( @@ -210,11 +217,16 @@ export function editFilesFromToolPair(pair: { state?: 'running' | 'completed' | 'failed' result?: { output?: string; isError?: boolean; editPatch?: NativeChatEditPatch } }): NativeChatEditFile[] | null { - // A card states the edit as made. An edit that failed or has not landed yet - // must keep the generic tool view, which shows the provider's own error. + // A card states the edit as made, so it takes evidence that it landed: the + // provider reporting the call complete, or a result that is not an error. + // Anything else — failed, still running, or a turn that stopped before the + // call was answered — keeps the generic tool view and its error body. if (pair.state === 'failed' || pair.state === 'running' || pair.result?.isError === true) { return null } + if (pair.state !== 'completed' && pair.result === undefined) { + return null + } const input = record(pair.input) const patch = pair.result?.editPatch if (patch && patch.hunks.length > 0) { @@ -229,9 +241,12 @@ export function editFilesFromToolPair(pair: { ] } - const envelope = unwrapBeginPatch(pair.input) - if (envelope) { - const files = editFilesFromBeginPatch(envelope) + // Only a tool that runs a patch may be searched for an envelope: a file's own + // contents can quote one, and scanning a write's payload rendered a card for + // the quoted file while the file actually written never appeared. + if (PATCH_ENVELOPE_TOOLS.has(pair.name)) { + const envelope = unwrapBeginPatch(pair.input) + const files = envelope ? editFilesFromBeginPatch(envelope) : [] if (files.length > 0) { return files } @@ -259,18 +274,31 @@ export function editFilesFromToolPair(pair: { if (!patchText) { return null } - const parsed = editLinesFromUnifiedPatch(patchText) - if (!parsed) { - return null - } - return [ - finalizeEditFile({ - path: text(input?.path) ?? text(input?.file_path) ?? 'file', - oldPath: null, - changeKind: 'edited', - lines: parsed.lines, - lineNumbersKnown: parsed.lineNumbersKnown, - truncated: parsed.truncated - }) - ] + // One card per file the patch touches: run together, the later files' rows + // and gutter numbers sit under the first file's name. + const split = unifiedPatchSections(patchText) + const callerPath = text(input?.path) ?? text(input?.file_path) + // For a single-file patch the call names the file it is reporting on, which + // is the provider's own path. A multi-file patch has no one path, so each + // section is named by its own header. + const named = (section: { path: string | null }): string => + (split.sections.length === 1 ? (callerPath ?? section.path) : (section.path ?? callerPath)) ?? + 'file' + const files = split.sections.flatMap((section) => { + const parsed = editLinesFromUnifiedPatch(section.body) + if (!parsed && section.path === null) { + return [] + } + return [ + finalizeEditFile({ + path: named(section), + oldPath: section.oldPath, + changeKind: section.changeKind, + lines: parsed?.lines ?? [], + lineNumbersKnown: parsed?.lineNumbersKnown ?? false, + truncated: split.truncated || (parsed?.truncated ?? false) + }) + ] + }) + return files.length > 0 ? files : null } diff --git a/src/shared/native-chat-unified-patch.ts b/src/shared/native-chat-unified-patch.ts index b2eb6df73c6..d00000c69c0 100644 --- a/src/shared/native-chat-unified-patch.ts +++ b/src/shared/native-chat-unified-patch.ts @@ -105,6 +105,131 @@ export function editLinesFromUnifiedPatch( return { lines, lineNumbersKnown: ranged, truncated: source.truncated } } +const GIT_DIFF_HEADER = 'diff --git ' + +export type UnifiedPatchSection = { + /** Null when the patch text named no file, leaving it to the caller. */ + path: string | null + oldPath: string | null + changeKind: 'added' | 'deleted' | 'edited' | 'renamed' + body: string +} + +type Section = { + rows: string[] + oldPath: string | null + newPath: string | null + named: boolean + /** A `--- `/`+++ ` pair already named this section, so the next one is a new file. */ + hasHeaderPair: boolean +} + +/** Splits patch text into one section per file it touches. Without this a + * multi-file patch renders as a single card under the first file's name, with + * the later files' rows and gutter numbers beneath it. */ +export function unifiedPatchSections(text: string): { + sections: UnifiedPatchSection[] + truncated: boolean +} { + const source = splitEditContent(text) + const rows = source.lines + const sections: Section[] = [] + let current: Section | null = null + let inHunk = false + + const open = (): Section => { + const section: Section = { + rows: [], + oldPath: null, + newPath: null, + named: false, + hasHeaderPair: false + } + sections.push(section) + current = section + return section + } + + for (let index = 0; index < rows.length; index += 1) { + const raw = rows[index] ?? '' + if (raw.startsWith(GIT_DIFF_HEADER)) { + const paths = gitHeaderPaths(raw) + const section = open() + section.oldPath = paths.oldPath + section.newPath = paths.newPath + section.named = true + inHunk = false + continue + } + // The same rule the parser uses: a header pair is structure only outside a + // hunk, where `--- ` would otherwise be a removed line beginning with `--`. + if (!inHunk && isFileHeaderPair(rows, index)) { + // The pair names the section a `diff --git` just opened; a second pair in + // the same section is the next file of a patch written without them. + const section = current && !current.hasHeaderPair ? current : open() + section.oldPath = sourceHeaderPath(rows[index] ?? '') + section.newPath = sourceHeaderPath(rows[index + 1] ?? '') + section.named = true + section.hasHeaderPair = true + index += 1 + continue + } + if (raw.startsWith('@@')) { + inHunk = true + } else if (FILE_SECTION.test(raw)) { + inHunk = false + } + ;(current ?? open()).rows.push(raw) + } + + return { + sections: sections.map((section) => ({ + path: section.newPath ?? section.oldPath, + oldPath: + section.oldPath && section.newPath && section.oldPath !== section.newPath + ? section.oldPath + : null, + changeKind: sectionChangeKind(section), + body: section.rows.join('\n') + })), + truncated: source.truncated + } +} + +function sectionChangeKind(section: Section): UnifiedPatchSection['changeKind'] { + if (!section.named) { + return 'edited' + } + if (section.newPath === null) { + return 'deleted' + } + if (section.oldPath === null) { + return 'added' + } + return section.oldPath === section.newPath ? 'edited' : 'renamed' +} + +/** `--- a/` / `+++ b/`, where the absent side is `/dev/null` and a + * trailing tab introduces the timestamp some producers append. */ +function sourceHeaderPath(line: string): string | null { + const value = (line.slice(4).split('\t')[0] ?? '').trim() + return value === '' || value === '/dev/null' ? null : value.replace(/^[ab]\//, '') +} + +function gitHeaderPaths(line: string): { oldPath: string | null; newPath: string | null } { + const rest = line.slice(GIT_DIFF_HEADER.length) + // Both halves carry the same path unless the file moved, so the second one + // starts at the last ` b/` rather than at the first space. + const split = rest.lastIndexOf(' b/') + if (split === -1) { + return { oldPath: null, newPath: null } + } + return { + oldPath: rest.slice(0, split).replace(/^a\//, ''), + newPath: rest.slice(split + 1).replace(/^b\//, '') + } +} + /** Rows for a whole-file add or delete, which legitimately number from 1. */ export function editLinesFromWholeFile( content: string,