Merge PR #6934: fix(hashline,coding-agent): key snapshot tag on actually-persisted content after ACP bridge writes (@marton78)
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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<boolean> {
|
||||
|
||||
@@ -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<boolean> {
|
||||
if (!shouldRouteWriteThroughBridge(session, requestedPath, absolutePath)) return false;
|
||||
): Promise<BridgeWriteResult | undefined> {
|
||||
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 };
|
||||
}
|
||||
|
||||
@@ -1267,9 +1267,15 @@ export class WriteTool implements AgentTool<typeof writeSchema, WriteToolDetails
|
||||
|
||||
// Try ACP bridge first for editor-visible filesystem paths. Internal
|
||||
// artifacts such as local:// plans are owned by OMP, not the editor.
|
||||
if (await routeWriteThroughBridge(this.session, path, absolutePath, cleanContent, signal)) {
|
||||
const madeExecutable = await maybeMarkExecutableForShebang(absolutePath, cleanContent);
|
||||
const header = maybeWriteSnapshotHeader(this.session, absolutePath, cleanContent);
|
||||
const bridgeWrite = await routeWriteThroughBridge(this.session, path, absolutePath, cleanContent, signal);
|
||||
if (bridgeWrite) {
|
||||
// `write` always replaces the whole file, so (unlike hashline's
|
||||
// hunk-scoped diff) there's no size cost to keying the header/
|
||||
// executable-bit check on the verified post-write content —
|
||||
// use it so a drifted write (e.g. client format-on-save) still
|
||||
// hands back a tag that matches what's actually on disk.
|
||||
const madeExecutable = await maybeMarkExecutableForShebang(absolutePath, bridgeWrite.text);
|
||||
const header = maybeWriteSnapshotHeader(this.session, absolutePath, bridgeWrite.text);
|
||||
const writeLine = `Successfully wrote ${cleanContent.length} bytes to ${displayPath}`;
|
||||
let resultText = header ? `${header}\n${writeLine}` : writeLine;
|
||||
if (stripped) {
|
||||
|
||||
@@ -2,8 +2,17 @@ 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 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, executePatchSingle, executeReplaceSingle } from "@oh-my-pi/pi-coding-agent/edit";
|
||||
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";
|
||||
import type { WritethroughCallback } from "@oh-my-pi/pi-coding-agent/lsp";
|
||||
@@ -57,6 +66,23 @@ function makeBridge() {
|
||||
return { bridge, spy };
|
||||
}
|
||||
|
||||
/**
|
||||
* Stand-in for an ACP client whose save pipeline reformats content before it
|
||||
* settles on disk (e.g. Zed's `format_on_save` rewriting indentation). Unlike
|
||||
* `makeBridge`, the bytes actually persisted differ from what was requested —
|
||||
* exercising the read-back/drift-detection path in `routeWriteThroughBridge`.
|
||||
*/
|
||||
function makeDriftingBridge() {
|
||||
const bridge: ClientBridge = {
|
||||
capabilities: { writeTextFile: true },
|
||||
writeTextFile: async ({ path: p, content: c }) => {
|
||||
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<EditToolDetails, typeof hashlineEditParamsSchema>): 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 ─────────────────────────────────────────────────────
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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,
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -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<WriteResult> {
|
||||
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"]]);
|
||||
|
||||
Reference in New Issue
Block a user