fix(acp): relativize the completed diff card title
The result-time diff card sent view.title raw, so a completed edit/write of an absolute in-workspace path flipped the card header back from the relativized `Edit src/b.ts` to the absolute path — the pending card relativizes, the result did not, and tool_call_update.title replaces the header. Apply displayTitle to the result diff arm using the diff path, mirroring the call-side card. Regression test proven red on the unfixed arm. Also record the overwrite diff-basis pre-read as a bounded follow-up (TODO(overwrite-diff-bound) + RFC non-goal): overwriting a large file reads the whole prior text into memory for a UI-only diff.
This commit is contained in:
@@ -46,6 +46,7 @@ Computing hunks-with-context is a solved problem with sharp edge cases (grouping
|
|||||||
- **Live incremental diff streaming.** The hunk is computed once, after the mutation completes; there is no per-keystroke diff.
|
- **Live incremental diff streaming.** The hunk is computed once, after the mutation completes; there is no per-keystroke diff.
|
||||||
- **Diffing a binary/non-UTF-8 overwrite.** `before` is `null` for such a file (it has no text diff basis); the write still succeeds and the result renders a whole-file diff (`oldText: null`) rather than a contextual hunk.
|
- **Diffing a binary/non-UTF-8 overwrite.** `before` is `null` for such a file (it has no text diff basis); the write still succeeds and the result renders a whole-file diff (`oldText: null`) rather than a contextual hunk.
|
||||||
- **Rename/move diffs.** Only content diffs of a single resolved path.
|
- **Rename/move diffs.** Only content diffs of a single resolved path.
|
||||||
|
- **Bounding the overwrite diff basis.** An overwrite reads the whole prior file into memory to compute the contextual hunk (on top of the new content already held), so a very large text overwrite allocates both texts for a UI-only diff. A future refinement can bound the pre-read and fall back to a whole-file / no contextual diff above a size threshold; tracked as `TODO(overwrite-diff-bound)` at the read site.
|
||||||
|
|
||||||
## Related
|
## Related
|
||||||
|
|
||||||
|
|||||||
@@ -151,6 +151,9 @@ export class LocalFileSystem extends FileSystem {
|
|||||||
// file (binary/invalid-UTF-8) — a null `before` gives no contextual-hunk
|
// file (binary/invalid-UTF-8) — a null `before` gives no contextual-hunk
|
||||||
// basis, so a consumer falls back to a whole-file diff (the tool still
|
// basis, so a consumer falls back to a whole-file diff (the tool still
|
||||||
// renders a result-time diff card, not the raw result text).
|
// renders a result-time diff card, not the raw result text).
|
||||||
|
// TODO(overwrite-diff-bound): this reads the whole prior file into memory
|
||||||
|
// for a UI-only diff; bound the pre-read and fall back to no contextual
|
||||||
|
// basis above a size threshold (see the applied-hunk-diffs RFC non-goals).
|
||||||
const before = existing ? await readTextForDiff(target.targetKey, signal) : null
|
const before = existing ? await readTextForDiff(target.targetKey, signal) : null
|
||||||
await writeFileAtomic(target.targetKey, content, existing?.mode, signal, this.internals)
|
await writeFileAtomic(target.targetKey, content, existing?.mode, signal, this.internals)
|
||||||
const after = await probe(target.targetKey)
|
const after = await probe(target.targetKey)
|
||||||
|
|||||||
@@ -1178,12 +1178,17 @@ function toolResultUpdate(callId: CallId, view: ToolResultView, isError: boolean
|
|||||||
// the diff the pending card installed (and keeps the model-facing result
|
// the diff the pending card installed (and keeps the model-facing result
|
||||||
// text from clobbering it).
|
// text from clobbering it).
|
||||||
const content: AcpToolCallContent[] = view.diffs.map(d => ({ type: 'diff', path: d.path, oldText: d.oldText, newText: d.newText }))
|
const content: AcpToolCallContent[] = view.diffs.map(d => ({ type: 'diff', path: d.path, oldText: d.oldText, newText: d.newText }))
|
||||||
|
// Relativize the replacement title against the session cwd from the diff
|
||||||
|
// path, exactly as the call-side card does — `tool_call_update.title`
|
||||||
|
// replaces the card header, so a raw absolute path here would undo the
|
||||||
|
// pending card's relativized title.
|
||||||
|
const title = view.title !== undefined ? displayTitle(view.title, view.diffs[0]?.path, terminal.cwd) : undefined
|
||||||
return {
|
return {
|
||||||
sessionUpdate: 'tool_call_update',
|
sessionUpdate: 'tool_call_update',
|
||||||
toolCallId: callId,
|
toolCallId: callId,
|
||||||
status,
|
status,
|
||||||
...content.length > 0 ? { content } : {},
|
...content.length > 0 ? { content } : {},
|
||||||
...view.title !== undefined ? { title: view.title } : {},
|
...title !== undefined ? { title } : {},
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
default:
|
default:
|
||||||
|
|||||||
@@ -675,6 +675,32 @@ describe('result-time diff card (REAL fs edit tool → tool_call_update diff blo
|
|||||||
await ctx.fiber.dispose()
|
await ctx.fiber.dispose()
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('the completed diff TITLE relativizes against the session cwd (the result title replaces the card header)', async () => {
|
||||||
|
// A `tool_call_update.title` replaces the card header, so the result-side
|
||||||
|
// diff must relativize its title exactly as the pending card did — otherwise
|
||||||
|
// a completed absolute-path edit flips `Edit src/b.ts` back to the raw
|
||||||
|
// absolute path. The diff/location paths stay absolute (the editor opens the
|
||||||
|
// real path). Drive the REAL fs edit tool with an absolute in-workspace path.
|
||||||
|
const ctx = await fsCtx()
|
||||||
|
const presenter = new ToolPresenter(ctx.tools)
|
||||||
|
const args = JSON.stringify({ file_path: '/work/proj/src/b.ts', old_string: 'OLD', new_string: 'NEW' })
|
||||||
|
const meta = { diffs: [{ path: '/work/proj/src/b.ts', oldText: 'a\nOLD\nb', newText: 'a\nNEW\nb' }] }
|
||||||
|
const out: SessionNotification['update'][] = []
|
||||||
|
const rendering = { enabled: false, cwd: '/work/proj' }
|
||||||
|
for (const event of [
|
||||||
|
evt('tool/call', { turn: 1, step: 1, callId: CallId('e1'), name: 'edit', arguments: args }),
|
||||||
|
evt('tool/result', { turn: 1, step: 1, callId: CallId('e1'), content: [{ type: 'text', text: 'ok' }], isError: false, meta }),
|
||||||
|
]) streamSessionEventUpdate(SessionId('s1'), event, n => out.push(n.update), presenter, rendering)
|
||||||
|
expect(out[1]).toEqual({
|
||||||
|
sessionUpdate: 'tool_call_update',
|
||||||
|
toolCallId: 'e1',
|
||||||
|
status: 'completed',
|
||||||
|
title: 'Edit src/b.ts',
|
||||||
|
content: [{ type: 'diff', path: '/work/proj/src/b.ts', oldText: 'a\nOLD\nb', newText: 'a\nNEW\nb' }],
|
||||||
|
})
|
||||||
|
await ctx.fiber.dispose()
|
||||||
|
})
|
||||||
|
|
||||||
it('a diff result with an EMPTY diffs array and no title omits both keys (nothing to send)', () => {
|
it('a diff result with an EMPTY diffs array and no title omits both keys (nothing to send)', () => {
|
||||||
// A synthetic tool whose presentResult yields a `diff` card with no hunks and
|
// A synthetic tool whose presentResult yields a `diff` card with no hunks and
|
||||||
// no title — the shipping fs tools never emit this (edit always has a hunk;
|
// no title — the shipping fs tools never emit this (edit always has a hunk;
|
||||||
|
|||||||
Reference in New Issue
Block a user