diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index e12783e12..3e2addfcc 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -12,6 +12,7 @@ - Fixed the assistant-facing available-resource summary omitting parameterized MCP resource templates, so a failed `mcp://` read now lists templates alongside concrete resources ([#6911](https://github.com/can1357/oh-my-pi/issues/6911)). - Fixed `xd://` mount notices re-announcing already-known devices on session resume / host reconnect: the notice was diff-gated only against the in-memory mount set, which reset each resume, so reconnecting MCP/RPC-host devices re-spliced a redundant developer message into history and busted the provider prompt-cache prefix (re-billing the whole suffix at full price on metered providers). Notices now carry a structured `{ added, removed }` payload and are gated against the devices persisted history already announced—including legacy rendered notices from before the structured payload—so a resume that re-establishes the same inventory emits nothing. The announced baseline is reset when the transcript is replaced (`/new`, `switchSession`, `branch`), so a device reconnecting into the fresh history announces again ([#6921](https://github.com/can1357/oh-my-pi/issues/6921)). - Fixed the model picker showing an extension provider's placeholder model list instead of the credential-aware catalog it resolves at registration, so models the account actually has could be missing while unavailable ones stayed listed. +- Fixed `edit`/`write` writes routed through the ACP client bridge (`fs/write_text_file`) trusting the requested content as "what's on disk" even when the client transforms it on save (e.g. Zed with `format_on_save: on` reformatting indentation the tool never touched). `routeWriteThroughBridge` now reads the file back after the bridge write and returns the verified content; `HashlineFilesystem.writeText` propagates it so the hashline snapshot tag matches the real file instead of the pre-write intent. Closes the reported "single-hunk `edit` call reformats the whole file" corruption, whose actual cause was every later edit resolving hunks against a stale snapshot the file had already drifted away from. ## [17.1.8] - 2026-07-28 diff --git a/packages/coding-agent/src/edit/hashline/filesystem.ts b/packages/coding-agent/src/edit/hashline/filesystem.ts index ca118dd42..b8743be2e 100644 --- a/packages/coding-agent/src/edit/hashline/filesystem.ts +++ b/packages/coding-agent/src/edit/hashline/filesystem.ts @@ -198,9 +198,33 @@ export class HashlineFilesystem extends Filesystem { const finalContent = await serializeEditFileText(absolutePath, relativePath, content); // Route through ACP bridge when available; skips internal artifacts. - if (await routeWriteThroughBridge(this.session, relativePath, absolutePath, finalContent, this.#signal)) { + // `finalContent` is storage-space (e.g. a notebook's full JSON); the + // bridge may also report content that diverges from it (e.g. the + // client reformatted on save). `WriteResult.text` must stay in + // view-space — the same space `readText` returns — so a follow-up + // `readText` sees exactly what this write reports. + const bridgeResult = await routeWriteThroughBridge( + this.session, + relativePath, + absolutePath, + finalContent, + this.#signal, + ); + if (bridgeResult) { this.#diagnosticsByPath.set(relativePath, undefined); - return { text: finalContent }; + if (!bridgeResult.driftedFromRequest) { + // No client-side transform: the view we sent is what's on disk. + return { text: content }; + } + // Drifted (e.g. format-on-save): re-derive the view from what + // actually landed on disk instead of assuming `content` still + // matches. Falls back to `content` if the drifted file can't be + // re-read as a valid view (e.g. a formatter broke notebook JSON). + try { + return { text: await readEditFileText(absolutePath, relativePath) }; + } catch { + return { text: content }; + } } const diagnostics = await this.#writethrough( @@ -213,7 +237,7 @@ export class HashlineFilesystem extends Filesystem { ); invalidateFsScanAfterWrite(absolutePath); this.#diagnosticsByPath.set(relativePath, diagnostics); - return { text: finalContent }; + return { text: content }; } async exists(relativePath: string): Promise { diff --git a/packages/coding-agent/src/tools/acp-bridge.ts b/packages/coding-agent/src/tools/acp-bridge.ts index a18f37099..b63b7984f 100644 --- a/packages/coding-agent/src/tools/acp-bridge.ts +++ b/packages/coding-agent/src/tools/acp-bridge.ts @@ -40,15 +40,39 @@ export function shouldRouteWriteThroughBridge( return absolutePath !== resolvePlanPath(session, state.planFilePath); } +/** + * Result of a bridge-routed write: the content actually verified on disk + * after the client processed the write, plus whether that content diverges + * from what the tool asked to persist. + * + * ACP's `fs/write_text_file` has no "verbatim, no side effects" guarantee — + * a client (e.g. Zed with `format_on_save: on`) may reformat the buffer as + * part of handling the write before it settles on disk. Silently trusting + * the requested `content` as "what's now on disk" lets that drift poison + * every snapshot/tag/hash a caller derives from the write, which then reads + * back as unrelated whole-file corruption on the *next* edit. Reading the + * file back and reporting what's actually there keeps callers honest. + */ +export interface BridgeWriteResult { + /** Content actually present on disk immediately after the bridge write. */ + text: string; + /** `true` when `text` differs from the content the tool asked to write. */ + driftedFromRequest: boolean; +} + /** * Try to route a file write through the ACP client bridge. * * Performs the full guard check, bridge call (wrapped in {@link ToolError}), - * FS-scan cache invalidation, and session mutation-version bump. + * a post-write read-back to detect client-side transformation (e.g. + * format-on-save), FS-scan cache invalidation, and session mutation-version + * bump. * - * Returns `true` when the bridge was used and the caller must skip the - * writethrough path. Returns `false` when the bridge is unavailable or the - * path should not be routed through it. + * Returns `undefined` when the bridge is unavailable or the path should not + * be routed through it — the caller must fall back to the writethrough path. + * Returns a {@link BridgeWriteResult} when the bridge was used; callers MUST + * use `result.text` (not the content they requested) for any snapshot, hash, + * or tag derived from this write. */ export async function routeWriteThroughBridge( session: ToolSession, @@ -56,11 +80,11 @@ export async function routeWriteThroughBridge( absolutePath: string, content: string, signal?: AbortSignal, -): Promise { - if (!shouldRouteWriteThroughBridge(session, requestedPath, absolutePath)) return false; +): Promise { + if (!shouldRouteWriteThroughBridge(session, requestedPath, absolutePath)) return undefined; const bridge = session.getClientBridge?.(); - if (!bridge?.capabilities.writeTextFile || !bridge.writeTextFile) return false; + if (!bridge?.capabilities.writeTextFile || !bridge.writeTextFile) return undefined; const changeType = (await Bun.file(absolutePath).exists()) ? FileChangeType.Changed : FileChangeType.Created; // The ACP protocol has no cancellation for fs writes; the most we can do is @@ -77,5 +101,25 @@ export async function routeWriteThroughBridge( } invalidateFsScanAfterWrite(absolutePath); session.bumpFileMutationVersion?.(absolutePath); - return true; + + // Best-effort verification: the client already flushed the write (that's + // the whole point of `fs/write_text_file`), so the file on disk reflects + // whatever the client actually persisted, formatter and all. If the + // read-back itself fails, fall back to trusting `content` rather than + // failing an otherwise-successful write. This is a best-effort signal, + // not a guarantee: ACP defines no ordering between a client acking the + // write and its own async format-on-save settling, so a client that acks + // before its formatter runs will look verbatim here. That degrades to + // the pre-fix behavior for THIS write, but never corrupts state: the + // next `readText` still observes whatever the client eventually settles + // on, and a real desync there surfaces as an honest stale-tag error + // instead of a silently wrong tag. + let actualText = content; + try { + actualText = await Bun.file(absolutePath).text(); + } catch { + // Unreadable right after a reported-successful write; nothing more we + // can verify here. + } + return { text: actualText, driftedFromRequest: actualText !== content }; } diff --git a/packages/coding-agent/src/tools/write.ts b/packages/coding-agent/src/tools/write.ts index f8e0edde7..f3c53b880 100644 --- a/packages/coding-agent/src/tools/write.ts +++ b/packages/coding-agent/src/tools/write.ts @@ -1267,9 +1267,15 @@ export class WriteTool implements AgentTool { + await Bun.write(p, c.replace(/^ /gm, "\t")); + }, + }; + const spy = spyOn(bridge, "writeTextFile"); + return { bridge, spy }; +} + function makeWritethroughMock(): { writethrough: WritethroughCallback; spy: { calledWith: string[] } } { const spy = { calledWith: [] as string[] }; // The writethrough must actually write to disk so post-write verification passes. @@ -156,6 +182,179 @@ describe("HashlineFilesystem ACP fs routing", () => { expect(bridgeSpy).not.toHaveBeenCalled(); expect(writeSpy.calledWith).toContain(sandboxAbs); }); + + it("returns the client's actually-persisted content, not the requested content, when the bridge reformats on save", async () => { + const { bridge } = makeDriftingBridge(); + const { writethrough } = makeWritethroughMock(); + const session = createSession(tmpDir, { bridge }); + + const filesystem = new HashlineFilesystem({ + session, + writethrough, + beginDeferredDiagnosticsForPath: noopBeginDeferred, + }); + + const requested = "function f() {\n return 1;\n}\n"; + const relPath = "output.ts"; + const absPath = path.join(tmpDir, relPath); + + const result = await filesystem.writeText(relPath, requested); + + // Ground truth: the "editor" reformatted spaces to tabs on save. + const onDisk = await fs.readFile(absPath, "utf8"); + expect(onDisk).toBe("function f() {\n\treturn 1;\n}\n"); + expect(onDisk).not.toBe(requested); + + // `writeText`'s result MUST reflect reality, not the pre-write intent — + // this is what the patcher keys the next snapshot tag on. + expect(result.text).toBe(onDisk); + }); +}); + +// ─── executeHashlineSingle end-to-end (model-visible payload) ──────────────── + +function getText(result: AgentToolResult): string { + const first = result.content[0]; + return first?.type === "text" ? first.text : ""; +} + +function extractTag(text: string): string { + const match = /#([0-9A-Fa-f]{4})\]/.exec(text); + if (!match) throw new Error(`no snapshot tag found in: ${text}`); + return match[1] ?? ""; +} + +describe("executeHashlineSingle model-visible payload under write-time drift", () => { + let tmpDir: string; + + beforeEach(async () => { + resetSettingsForTest(); + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-acp-hashline-e2e-")); + await Settings.init({ inMemory: true, cwd: tmpDir }); + }); + + afterEach(async () => { + resetSettingsForTest(); + await removeWithRetries(tmpDir); + }); + + it("keeps the model-visible diff scoped to the intended hunk, not the whole reformatted file, when the bridge drifts", async () => { + // Source-shaped content (short lines, closing braces at col 0) is exactly + // what defeats the compact-diff-preview's contiguous-run collapse, so this + // is the worst case for payload inflation, not a favorable one. + const lines = ["function f() {"]; + for (let i = 0; i < 60; i++) lines.push(` const v${i} = ${i};`); + lines.push("}", ""); + const original = lines.join("\n"); + const relPath = "big.ts"; + const absPath = path.join(tmpDir, relPath); + await fs.writeFile(absPath, original); + + const { bridge } = makeDriftingBridge(); + const { writethrough } = makeWritethroughMock(); + const session = createSession(tmpDir, { bridge }); + + const realTag = computeFileHash(original); + + const result = await executeHashlineSingle({ + session, + input: `[${relPath}#${realTag}]\nSWAP 2.=2:\n+ const v0 = 100;`, + writethrough, + beginDeferredDiagnosticsForPath: noopBeginDeferred, + }); + + const text = getText(result); + + // Ground truth: the "editor" reformatted every untouched indented line. + const onDisk = await fs.readFile(absPath, "utf8"); + expect(onDisk).not.toBe(original); + expect(onDisk.split("\n").filter(l => l.startsWith("\t")).length).toBeGreaterThan(50); + + // The model-visible response must stay small: a few lines around the + // intended hunk plus a short warning, not a diff spanning ~60 reformatted + // lines. This is the regression a naive "key the diff on the verified + // content" fix would introduce. + expect(text.length).toBeLessThan(600); + expect(text).toMatch(/reformatted it on save/); + expect(text).not.toContain("v59"); // an untouched, far-away line never appears + + // And the returned tag must still be valid for a follow-up edit. + const nextTag = extractTag(text); + const followUp = await executeHashlineSingle({ + session, + input: `[${relPath}#${nextTag}]\nSWAP 1.=1:\n+function g() {`, + writethrough, + beginDeferredDiagnosticsForPath: noopBeginDeferred, + }); + expect(getText(followUp)).not.toMatch(/mismatch|stale/i); + }); + + it("does not warn about drift for a byte-perfect (non-reformatting) bridge write on a BOM'd file", async () => { + const { bridge } = makeBridge(); // verbatim: writes exactly what it's given + const { writethrough } = makeWritethroughMock(); + const session = createSession(tmpDir, { bridge }); + + const relPath = "bom.txt"; + const absPath = path.join(tmpDir, relPath); + const original = "\uFEFFhello\nworld\n"; + await fs.writeFile(absPath, original); + + const realTag = computeFileHash("hello\nworld\n"); // tag hashes BOM-stripped content + + const result = await executeHashlineSingle({ + session, + input: `[${relPath}#${realTag}]\nSWAP 2.=2:\n+earth`, + writethrough, + beginDeferredDiagnosticsForPath: noopBeginDeferred, + }); + + const text = getText(result); + expect(text).not.toMatch(/reformatted it on save/); + }); + + it("propagates a notebook's editable view (not raw JSON) as the write result, so a follow-up edit's tag stays valid", async () => { + const relPath = "nb.ipynb"; + const absPath = path.join(tmpDir, relPath); + const notebook = { + cells: [ + { cell_type: "code", source: ["print('old')\n"], metadata: {}, outputs: [], execution_count: null }, + ], + metadata: {}, + nbformat: 4, + nbformat_minor: 5, + }; + await fs.writeFile(absPath, JSON.stringify(notebook)); + + const { writethrough } = makeWritethroughMock(); + // No bridge at all: this reproduces the bug on the plain writethrough + // path, where the notebook's view-space (cell text) and storage-space + // (full JSON) were being conflated regardless of any ACP client. + const session = createSession(tmpDir); + + const cellView = "# %% [code] cell:0\nprint('old')\n"; + const realTag = computeFileHash(cellView); + + const result = await executeHashlineSingle({ + session, + input: `[${relPath}#${realTag}]\nSWAP 2.=2:\n+print('new')`, + writethrough, + beginDeferredDiagnosticsForPath: noopBeginDeferred, + }); + const text = getText(result); + expect(text).not.toMatch(/mismatch|stale/i); + + const nextTag = extractTag(text); + const followUp = await executeHashlineSingle({ + session, + input: `[${relPath}#${nextTag}]\nSWAP 2.=2:\n+print('newer')`, + writethrough, + beginDeferredDiagnosticsForPath: noopBeginDeferred, + }); + expect(getText(followUp)).not.toMatch(/mismatch|stale/i); + + const updated = JSON.parse(await fs.readFile(absPath, "utf8")); + expect(updated.cells[0].source.join("")).toContain("newer"); + }); }); // ─── executeReplaceSingle ───────────────────────────────────────────────────── diff --git a/packages/hashline/CHANGELOG.md b/packages/hashline/CHANGELOG.md index 52d3c29ea..d12eb05f6 100644 --- a/packages/hashline/CHANGELOG.md +++ b/packages/hashline/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed - Rejected line anchors above `Number.MAX_SAFE_INTEGER` and ranges spanning more than 100,000 lines before expansion, preventing streaming previews from exhausting CPU and memory. +- Fixed the recorded snapshot tag desyncing from disk when the `Filesystem` transforms content on write (e.g. an ACP-connected editor reformatting on save via `format_on_save`). `Patcher.commit` now keys the returned `fileHash`/header/snapshot on `WriteResult.text` (what the adapter reports actually landed on disk) instead of the pre-write content whenever they diverge, and surfaces a warning naming the drift. Previously a single-line hunk against a drifting write path could look like the tool silently reformatted the entire file on a later, unrelated edit, since every subsequent hunk was resolved against a snapshot the file no longer matched. ## [17.1.5] - 2026-07-27 diff --git a/packages/hashline/src/messages.ts b/packages/hashline/src/messages.ts index 8298858de..d3a7e249a 100644 --- a/packages/hashline/src/messages.ts +++ b/packages/hashline/src/messages.ts @@ -289,6 +289,23 @@ export const RECOVERY_LINE_REMAP_WARNING = export const HEADTAIL_DRIFT_WARNING = "Applied the `INS.HEAD:`/`INS.TAIL:` edit despite a stale snapshot tag (file changed since your read) — head/tail position is content-independent. Re-read if the drift was unexpected."; +/** + * The `Filesystem` reported that what actually landed on disk differs from + * what was written (see `WriteResult.text`) — most commonly an ACP-connected + * editor reformatting the buffer on save (e.g. `format_on_save` with tab/space + * settings that don't match the file). The recorded snapshot is re-keyed on + * the real, post-write content so the next edit's tag validation matches + * reality instead of silently drifting. + */ +export function writeDriftWarning(path: string): string { + return ( + `${path}: the file on disk after this write differs from what was sent — the client ` + + "(editor/IDE) likely reformatted it on save (e.g. format-on-save, tab/space settings). " + + "The returned snapshot reflects the actual file; re-read before further edits if the " + + "extra changes were unexpected." + ); +} + /** * Section omitted the mandatory snapshot tag. Shared by the apply * ({@link Patcher.prepare}) and preview/diff paths so both stay in lockstep. diff --git a/packages/hashline/src/patcher.ts b/packages/hashline/src/patcher.ts index 14549d3fa..162aee2ad 100644 --- a/packages/hashline/src/patcher.ts +++ b/packages/hashline/src/patcher.ts @@ -35,6 +35,7 @@ import { pathRecoveredFromTagMessage, type RevealedLine, unseenLinesMessage, + writeDriftWarning, } from "./messages"; import { MismatchError } from "./mismatch"; import { detectLineEnding, type LineEnding, normalizeToLF, restoreLineEndings, stripBom } from "./normalize"; @@ -98,7 +99,13 @@ export interface PatchSectionResult { persisted: string; /** Final text that the {@link Filesystem} actually wrote (may differ if the FS transformed it). */ written: string; - /** 4-hex content-hash tag for `after`. Use to anchor follow-up edits. */ + /** + * 4-hex content-hash tag. Hashes the content the {@link Filesystem} + * reports actually landed on disk (see `written`), which normally equals + * `after` but can diverge when the write path transforms content (e.g. an + * ACP-bridge write reformatted by the client's format-on-save). Use to + * anchor follow-up edits. + */ fileHash: string; /** Hashline section header (`[path#tag]`) of the post-edit content. */ header: string; @@ -488,9 +495,32 @@ export class Patcher { } const write: WriteResult = await this.fs.writeText(section.path, persisted); - const fileHash = this.#recordFullSnapshot(canonicalPath, after); const op = exists ? "update" : "create"; + // `write.text` is the FS adapter's report of what actually landed on + // disk (see `WriteResult`), which for an ACP-bridge write can diverge + // from `after` when the client transforms content on save (e.g. + // format-on-save reformatting indentation the tool never touched). + // Keying the snapshot on `after` unconditionally would record a hash + // for content that no longer exists on disk: the next `read` sees the + // drifted file, tag validation misses, and hunk resolution proceeds + // against a baseline the file has already left — the mechanism behind + // "single-line edit reformats the whole file". Re-derive the recorded + // text from what was actually persisted and hash THAT. + // + // Deliberately does NOT touch `after` (or the diff/`newText` derived + // from it downstream): `after` stays the content this section asked + // for, so the model-visible diff stays scoped to the intended hunk + // instead of ballooning to a whole-file diff against a formatter's + // output on every drifted write. The drift itself is a warning, not a + // diff — an O(1) signal instead of an O(file-size) one. Comparing the + // normalized forms (rather than raw `write.text`/`persisted`) avoids a + // false "drift" purely from BOM/line-ending restoration asymmetry. + const recorded = normalizeToLF(stripBom(write.text).text); + const driftedOnWrite = recorded !== after; + const fileHash = this.#recordFullSnapshot(canonicalPath, recorded); + const allWarnings = driftedOnWrite ? [...warnings, writeDriftWarning(section.path)] : warnings; + return { path: section.path, canonicalPath, @@ -503,7 +533,7 @@ export class Patcher { header: formatHashlineHeader(section.path, fileHash), firstChangedLine: applyResult.firstChangedLine, blockResolutions: applyResult.blockResolutions, - warnings, + warnings: allWarnings, }; } diff --git a/packages/hashline/test/patcher.test.ts b/packages/hashline/test/patcher.test.ts index 8cc2c4b06..0ab353290 100644 --- a/packages/hashline/test/patcher.test.ts +++ b/packages/hashline/test/patcher.test.ts @@ -12,6 +12,7 @@ import { NodeFilesystem, Patch, Patcher, + type WriteResult, } from "@oh-my-pi/hashline"; const PATH = "a.ts"; @@ -148,6 +149,56 @@ describe("Patcher snapshot tag integrity", () => { }); }); +// A write-time transform outside the patcher's control (e.g. an ACP-connected +// editor's format-on-save rewriting indentation on every save) must never +// poison the next section's snapshot tag with content that no longer exists +// on disk. `DriftingFilesystem` stands in for that editor: every write is +// persisted verbatim to the backing store (so `fs.get` sees ground truth, +// like the file the reporter grepped with `cat`/`grep` right after the tool +// call returned), but `writeText` echoes back a *reformatted* copy — spaces +// turned into tabs, exactly the corruption reported against the ACP bridge. +class DriftingFilesystem extends InMemoryFilesystem { + async writeText(path: string, content: string): Promise { + const drifted = content.replace(/^ /gm, "\t"); + await super.writeText(path, drifted); + return { text: drifted }; + } +} + +describe("Patcher snapshot tag stays honest across a write-time content transform", () => { + it("keys the returned snapshot tag on what the Filesystem actually persisted, not the pre-write content", async () => { + const original = "function f() {\n return 1;\n}\n"; + const fs = new DriftingFilesystem([[PATH, original]]); + const snapshots = new InMemorySnapshotStore(); + const tag = snapshots.record(PATH, original); + const patcher = new Patcher({ fs, snapshots }); + + const result = await patcher.apply(Patch.parse(`[${PATH}#${tag}]\nSWAP 2.=2:\n+ return 2;`)); + const section = result.sections[0]; + if (!section) throw new Error("expected one section result"); + + // Ground truth: the Filesystem drifted the untouched line's indentation + // to tabs on write, exactly like a hostile format-on-save would. + const onDisk = fs.get(PATH); + expect(onDisk).toBe("function f() {\n\treturn 2;\n}\n"); + + // The returned tag MUST hash the drifted (real) content, not the + // pre-write text the patcher computed — otherwise the very next edit's + // tag validation is checked against content the file no longer has. + expect(section.fileHash).toBe(computeFileHash(onDisk ?? "")); + expect(section.header).toBe(formatHashlineHeader(PATH, computeFileHash(onDisk ?? ""))); + + // The drift is surfaced, not swallowed: silent divergence is exactly + // what turned a one-line edit into unexplained whole-file corruption. + expect(section.warnings.some(w => w.includes(PATH) && /reformatted it on save/.test(w))).toBe(true); + + // A follow-up edit anchored on the returned tag must succeed against + // the real (drifted) file instead of failing a stale-tag mismatch. + const followUp = await patcher.apply(Patch.parse(`[${PATH}#${section.fileHash}]\nSWAP 1.=1:\n+function g() {`)); + expect(fs.get(PATH)).toBe("function g() {\n\treturn 2;\n}\n"); + }); +}); + describe("Patcher mandatory snapshot tag policy", () => { it("rejects a hashless head/tail insert — the tag is required on every section", async () => { const fs = new InMemoryFilesystem([[PATH, "a\nb\n"]]);