mirror of
https://github.com/stablyai/orca.git
synced 2026-09-29 08:03:20 +00:00
fix(native-chat): only read a patch where a patch was actually run
Recovering the patch envelope from any value of a tool's payload meant a write's own content was searched for one. A file documenting the patch format rendered a card for the file its example names, while the file actually written never appeared at all — the call and its result were consumed by that card, so nothing was left to correct it. Two changes: the envelope is recovered only for the tools that run one, never for a file edit whose payload is content; and only patch- or command-bearing arguments are searched, still including the words of an argument vector, which is where the envelope sits when a command tool applies it. The call payload is decoded back where it is needed rather than at the transcript decoder. Decoding it there changed the shape every reader of a tool's input sees, including the surface that recognises a question payload from any tool by shape alone: a tool whose arguments happened to carry that shape raised a question card pinned over the composer. That decode now happens inside the envelope recovery, the one consumer that needs the structure. A card also states an edit as made, so it now takes evidence that it landed — the provider reporting the call complete, or a result that is not an error. A turn that stopped before its call was answered reported an edit that may never have applied. This replaces the working-turn heuristic in the view, so the rule lives in one place. Two files still went missing. A multi-file patch has no per-file split, so it rendered as one card under the first file's name, with the later files' rows and their gutter numbers beneath it — a card asserting a false file position. Patch text is now split on its file boundaries, one card per file, each named by its own header, with a rename and a `/dev/null` side read from the same headers. And an envelope section that names a file but carries no body was dropped rather than reported, which is the same silent loss the delete case was fixed for.
This commit is contained in:
@@ -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<string, unknown>): 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<string, unknown> | null {
|
||||
try {
|
||||
const parsed: unknown = JSON.parse(value)
|
||||
return typeof parsed === 'object' && parsed !== null && !Array.isArray(parsed)
|
||||
? (parsed as Record<string, unknown>)
|
||||
: null
|
||||
} catch {
|
||||
return null
|
||||
}
|
||||
}
|
||||
|
||||
function codexToolResult(output: unknown): NativeChatBlock {
|
||||
const record = asRecord(output)
|
||||
const isError = record?.success === false || record?.is_error === true
|
||||
|
||||
@@ -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()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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')
|
||||
|
||||
@@ -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: [
|
||||
{
|
||||
|
||||
@@ -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))
|
||||
|
||||
@@ -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<string, unknown>)
|
||||
: 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<string, unknown>)
|
||||
: null
|
||||
}
|
||||
|
||||
function jsonRecord(value: string): Record<string, unknown> | 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<string, unknown>)
|
||||
: null
|
||||
} catch {
|
||||
return null
|
||||
}
|
||||
}
|
||||
|
||||
/** A command tool's argument is a vector, so the envelope sits one level in. */
|
||||
function envelopeArgument(record: Record<string, unknown>): 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
|
||||
})
|
||||
]
|
||||
})
|
||||
|
||||
@@ -7,6 +7,13 @@ import { unwrapBeginPatch } from './native-chat-begin-patch'
|
||||
const gutter = (files: ReturnType<typeof editFilesFromToolPair>): (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<typeof editFilesFromToolPair>[0]
|
||||
): ReturnType<typeof editFilesFromToolPair> =>
|
||||
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)
|
||||
})
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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/<path>` / `+++ b/<path>`, 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,
|
||||
|
||||
Reference in New Issue
Block a user