From 7708f372b5e6288ae86543f9dcad7cf50914aabb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rton=20Dan=C3=B3czy?= Date: Tue, 28 Jul 2026 20:27:07 +0200 Subject: [PATCH 1/3] fix(hashline,coding-agent): key snapshot tag on actually-persisted content after ACP bridge writes Root cause of the reported "edit tool silently reformats the whole file" corruption: fs/write_text_file has no verbatim guarantee. When an ACP client (e.g. Zed with format_on_save: on) reformats a buffer on save, routeWriteThroughBridge reported the pre-write content as successfully written, and Patcher.commit keyed the returned snapshot tag on that same pre-write text instead of what actually landed on disk. The next edit anchored on that tag then resolved hunks against a baseline the file had already drifted away from, which is what produced whole-file "corruption" from single-line hunks -- reproduced live in this session against real Swift/JSON/TypeScript files with Zed as the ACP client. - routeWriteThroughBridge reads the file back after the bridge write and returns the verified content plus a drift flag (best-effort: ACP defines no ordering between the client acking the write and its own async format-on-save settling, so this degrades gracefully to the old stale-tag-on-next-read failure mode, never to corruption). - HashlineFilesystem.writeText propagates that verified content in view-space (the same space readText returns -- e.g. a notebook's editable cell text, not its raw JSON), not storage-space, so tag validation on the next edit compares like with like. - Patcher.commit keys fileHash/header/snapshot on the verified post-write content (normalized, so BOM/line-ending restoration never produces a false "drift") when it diverges from what was sent, and appends a warning naming the drift -- but deliberately leaves the returned `after` (and therefore the model-visible diff) scoped to the intended hunk. Diffing against the full drifted file would balloon the tool response to span every reformatted line (measured ~6.8x inflation on a 245-line file with one touched line); the warning is the correct O(1) channel for "your editor reformatted this," not an O(file-size) diff. - write.ts keys its own snapshot header on the verified bridge content too (no diff-size concern there since write always replaces the whole file). Caught via code review (dispatched against the first pass of this fix): a naive "just use the verified content everywhere" fix broke .ipynb editing outright (write-space vs read-space content mismatch, tag invalid on every notebook edit) and would have inflated every drifted edit response by ~6.8x. Both are now covered by regression tests that fail against the pre-fix code and pass against this one. (cherry picked from commit 35ab80e43be5800b2f48728e4400eb9fd7f7f7d2) --- packages/coding-agent/CHANGELOG.md | 4 + .../src/edit/hashline/filesystem.ts | 30 ++- packages/coding-agent/src/tools/acp-bridge.ts | 60 +++++- packages/coding-agent/src/tools/write.ts | 12 +- .../coding-agent/test/edit-acp-bridge.test.ts | 200 +++++++++++++++++- packages/hashline/CHANGELOG.md | 1 + packages/hashline/src/messages.ts | 17 ++ packages/hashline/src/patcher.ts | 36 +++- packages/hashline/test/patcher.test.ts | 51 +++++ 9 files changed, 393 insertions(+), 18 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index d97a1f633..84fe6b4f3 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- 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 ### Breaking Changes 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 +178,182 @@ 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: Awaited>): 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 read = await import("@oh-my-pi/hashline"); + const realTag = read.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 read = await import("@oh-my-pi/hashline"); + const realTag = read.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 read = await import("@oh-my-pi/hashline"); + const cellView = "# %% [code] cell:0\nprint('old')\n"; + const realTag = read.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"]]); From a5d01deb85f0ec265f10bf7db7e297c4d0a9f33d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rton=20Dan=C3=B3czy?= Date: Tue, 28 Jul 2026 22:08:43 +0200 Subject: [PATCH 2/3] test(coding-agent): hoist hashline import to module scope in ACP bridge test Replaces three inline `await import("@oh-my-pi/hashline")` calls with a single top-level `computeFileHash` import. AGENTS.md forbids dynamic imports. Review: https://github.com/can1357/oh-my-pi/pull/6934 (cherry picked from commit 9ffda46c94de0146a8e77ec816640918a565a0ea) --- packages/coding-agent/test/edit-acp-bridge.test.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/coding-agent/test/edit-acp-bridge.test.ts b/packages/coding-agent/test/edit-acp-bridge.test.ts index e8f000293..4946217a3 100644 --- a/packages/coding-agent/test/edit-acp-bridge.test.ts +++ b/packages/coding-agent/test/edit-acp-bridge.test.ts @@ -2,6 +2,7 @@ import { afterEach, beforeEach, describe, expect, it, spyOn } from "bun:test"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; +import { computeFileHash } from "@oh-my-pi/hashline"; import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { DEFAULT_FUZZY_THRESHOLD, @@ -250,8 +251,7 @@ describe("executeHashlineSingle model-visible payload under write-time drift", ( const { writethrough } = makeWritethroughMock(); const session = createSession(tmpDir, { bridge }); - const read = await import("@oh-my-pi/hashline"); - const realTag = read.computeFileHash(original); + const realTag = computeFileHash(original); const result = await executeHashlineSingle({ session, @@ -296,8 +296,7 @@ describe("executeHashlineSingle model-visible payload under write-time drift", ( const original = "\uFEFFhello\nworld\n"; await fs.writeFile(absPath, original); - const read = await import("@oh-my-pi/hashline"); - const realTag = read.computeFileHash("hello\nworld\n"); // tag hashes BOM-stripped content + const realTag = computeFileHash("hello\nworld\n"); // tag hashes BOM-stripped content const result = await executeHashlineSingle({ session, @@ -329,9 +328,8 @@ describe("executeHashlineSingle model-visible payload under write-time drift", ( // (full JSON) were being conflated regardless of any ACP client. const session = createSession(tmpDir); - const read = await import("@oh-my-pi/hashline"); const cellView = "# %% [code] cell:0\nprint('old')\n"; - const realTag = read.computeFileHash(cellView); + const realTag = computeFileHash(cellView); const result = await executeHashlineSingle({ session, From 5c0b52a1418191496c343ed793e73900e4779754 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rton=20Dan=C3=B3czy?= Date: Tue, 28 Jul 2026 22:09:40 +0200 Subject: [PATCH 3/3] test(coding-agent): use declared AgentToolResult type in ACP bridge test Replaces `Awaited>` with the function's declared `AgentToolResult` return type. AGENTS.md bans ReturnType<>. Review: https://github.com/can1357/oh-my-pi/pull/6934 (cherry picked from commit 59dbc5b79f8857bab67559656eae96cf7dfb6ac0) --- packages/coding-agent/test/edit-acp-bridge.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/coding-agent/test/edit-acp-bridge.test.ts b/packages/coding-agent/test/edit-acp-bridge.test.ts index 4946217a3..b5cc5cc22 100644 --- a/packages/coding-agent/test/edit-acp-bridge.test.ts +++ b/packages/coding-agent/test/edit-acp-bridge.test.ts @@ -3,12 +3,15 @@ import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; import { computeFileHash } from "@oh-my-pi/hashline"; +import type { AgentToolResult } from "@oh-my-pi/pi-agent-core"; import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { DEFAULT_FUZZY_THRESHOLD, + type EditToolDetails, executeHashlineSingle, executePatchSingle, executeReplaceSingle, + hashlineEditParamsSchema, } from "@oh-my-pi/pi-coding-agent/edit"; import { HashlineFilesystem } from "@oh-my-pi/pi-coding-agent/edit/hashline/filesystem"; import { resolveLocalUrlToPath } from "@oh-my-pi/pi-coding-agent/internal-urls"; @@ -210,7 +213,7 @@ describe("HashlineFilesystem ACP fs routing", () => { // ─── executeHashlineSingle end-to-end (model-visible payload) ──────────────── -function getText(result: Awaited>): string { +function getText(result: AgentToolResult): string { const first = result.content[0]; return first?.type === "text" ? first.text : ""; }