From 497ea15bdfa9b36f5364287a74cf1218aa3f1933 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Fri, 3 Jul 2026 23:14:22 +0800 Subject: [PATCH] fix(acp): relativize the completed diff card title MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- ...26-07-02-result-time-applied-hunk-diffs.md | 1 + packages/fs/fs-local/src/index.ts | 3 +++ packages/ui/acp/src/index.ts | 7 ++++- packages/ui/acp/tests/stream-update.spec.ts | 26 +++++++++++++++++++ 4 files changed, 36 insertions(+), 1 deletion(-) diff --git a/docs/rfc/implemented/architecture/2026-07-02-result-time-applied-hunk-diffs.md b/docs/rfc/implemented/architecture/2026-07-02-result-time-applied-hunk-diffs.md index 8a4c49ff2c..9c95dff773 100644 --- a/docs/rfc/implemented/architecture/2026-07-02-result-time-applied-hunk-diffs.md +++ b/docs/rfc/implemented/architecture/2026-07-02-result-time-applied-hunk-diffs.md @@ -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. - **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. +- **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 diff --git a/packages/fs/fs-local/src/index.ts b/packages/fs/fs-local/src/index.ts index 8ce96fe49d..567c6277c6 100644 --- a/packages/fs/fs-local/src/index.ts +++ b/packages/fs/fs-local/src/index.ts @@ -151,6 +151,9 @@ export class LocalFileSystem extends FileSystem { // 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 // 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 await writeFileAtomic(target.targetKey, content, existing?.mode, signal, this.internals) const after = await probe(target.targetKey) diff --git a/packages/ui/acp/src/index.ts b/packages/ui/acp/src/index.ts index ab784d2efd..bcfc0de6e4 100644 --- a/packages/ui/acp/src/index.ts +++ b/packages/ui/acp/src/index.ts @@ -1178,12 +1178,17 @@ function toolResultUpdate(callId: CallId, view: ToolResultView, isError: boolean // the diff the pending card installed (and keeps the model-facing result // text from clobbering it). 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 { sessionUpdate: 'tool_call_update', toolCallId: callId, status, ...content.length > 0 ? { content } : {}, - ...view.title !== undefined ? { title: view.title } : {}, + ...title !== undefined ? { title } : {}, } } default: diff --git a/packages/ui/acp/tests/stream-update.spec.ts b/packages/ui/acp/tests/stream-update.spec.ts index 67cd95ccdd..b580a5fc3d 100644 --- a/packages/ui/acp/tests/stream-update.spec.ts +++ b/packages/ui/acp/tests/stream-update.spec.ts @@ -675,6 +675,32 @@ describe('result-time diff card (REAL fs edit tool → tool_call_update diff blo 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)', () => { // 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;