diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index bb87143c3..d198760d0 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Improved reliability of edits when file snapshots share identical 16-bit hash tags + ## [16.3.2] - 2026-07-02 ### Breaking Changes diff --git a/packages/coding-agent/src/edit/hashline/diff.ts b/packages/coding-agent/src/edit/hashline/diff.ts index 22ac8e9bc..d38e493eb 100644 --- a/packages/coding-agent/src/edit/hashline/diff.ts +++ b/packages/coding-agent/src/edit/hashline/diff.ts @@ -180,8 +180,7 @@ function resolvePreviewEdits(args: { }): readonly Edit[] { const { section, absolutePath, normalized, snapshots, expected, liveMatches, edits } = args; if (!hasBlockEdit(edits)) return edits; - const baseText = - expected === undefined || liveMatches ? normalized : snapshots.byHashExact(absolutePath, expected)?.text; + const baseText = expected === undefined || liveMatches ? normalized : snapshots.byHash(absolutePath, expected)?.text; if (baseText === undefined) { throw createMismatchError(section, absolutePath, normalized, snapshots, expected ?? ""); } @@ -200,17 +199,9 @@ function applyPreviewEdits(args: { if (!options.skipHashValidation && expected === undefined) { throw new Error(missingSnapshotTagMessage(section.path)); } - // A 16-bit tag can collide across two different file states, so hash - // equality alone does not prove the live text IS the snapshot the model's - // anchors were minted against (mirrors Patcher's apply-time guard). When - // the store retains text for the tag, require it to be unambiguous and - // byte-identical to live; otherwise fall through to recovery/reject below - // exactly as if the hash had not matched. - const liveMatches = - expected !== undefined && - computeFileHash(normalized) === expected && - (snapshots.byHash(absolutePath, expected) === null || - snapshots.byHashExact(absolutePath, expected)?.text === normalized); + // The 4-hex tag is content-derived: when the live text hashes to it, trust + // the match and preview directly (mirrors Patcher's apply-time behavior). + const liveMatches = expected !== undefined && computeFileHash(normalized) === expected; const edits = parsePreviewEdits(section, options.streaming); const resolved = resolvePreviewEdits({ section, absolutePath, normalized, snapshots, expected, liveMatches, edits }); if (options.skipHashValidation || expected === undefined || liveMatches) return applyEdits(normalized, resolved); diff --git a/packages/coding-agent/test/edit-diff.test.ts b/packages/coding-agent/test/edit-diff.test.ts index 0e558e50d..10cbace63 100644 --- a/packages/coding-agent/test/edit-diff.test.ts +++ b/packages/coding-agent/test/edit-diff.test.ts @@ -300,9 +300,10 @@ describe("computeHashlineDiff", () => { }); // A 16-bit snapshot tag can collide across two different file states. The - // preview must mirror Patcher's apply-time guard: hash equality alone never - // proves the live text IS the snapshot the anchors were minted against. - test("rejects the no-drift path when the live text is a colliding ambiguous tag", async () => { + // preview mirrors Patcher's apply-time behavior: tag equality with the + // live content is trusted as-is — a colliding retained snapshot must not + // reject the preview, since a forced re-read would mint the very same tag. + test("previews onto live content when the tag matches a colliding retained snapshot", async () => { // Both texts hash to `1D84` (pinned in hashline's collision tests). const SNAPSHOT_TEXT = "line one 263\nline two 4471\n"; const LIVE_TEXT = "line one 410\nline two 6970\n"; @@ -311,48 +312,19 @@ describe("computeHashlineDiff", () => { const snapshotStore = new InMemorySnapshotStore(); // Anchors were minted against SNAPSHOT_TEXT; the live file is the - // colliding LIVE_TEXT, also retained (e.g. read after an external - // write). The tag is ambiguous — previewing the SWAP against live - // would show the model's payload landing on unrelated content. + // colliding LIVE_TEXT. The tag still matches live, so the SWAP lands + // on live line 2. const tag = snapshotStore.record(sourcePath, SNAPSHOT_TEXT); - snapshotStore.record(sourcePath, LIVE_TEXT); const result = await computeHashlineDiff( - { input: `${formatHashlineHeader(sourcePath, tag)}\nSWAP 2.=2:\n+edited from snapshot` }, + { input: `${formatHashlineHeader(sourcePath, tag)}\nSWAP 2.=2:\n+edited live` }, tempDir, snapshotStore, ); - expect("error" in result).toBe(true); - if ("error" in result) { - expect(result.error).toContain("file changed between read and edit"); - } - }); - - test("rejects the no-drift path when the live text collides with the single retained snapshot", async () => { - const SNAPSHOT_TEXT = "line one 263\nline two 4471\n"; - const LIVE_TEXT = "line one 410\nline two 6970\n"; - const sourcePath = path.join(tempDir, "source.txt"); - await Bun.write(sourcePath, LIVE_TEXT); - - const snapshotStore = new InMemorySnapshotStore(); - // Only SNAPSHOT_TEXT is retained; live drifted to a colliding text the - // store never saw. computeFileHash(live) === tag, but the retained - // text differs — recovery (3-way merge from SNAPSHOT_TEXT) must run - // instead of anchoring directly onto the collider. Here the merge - // cannot apply (every line differs), so the preview surfaces the - // drift error rather than a bogus diff. - const tag = snapshotStore.record(sourcePath, SNAPSHOT_TEXT); - - const result = await computeHashlineDiff( - { input: `${formatHashlineHeader(sourcePath, tag)}\nSWAP 2.=2:\n+edited from snapshot` }, - tempDir, - snapshotStore, - ); - - expect("error" in result).toBe(true); - if ("error" in result) { - expect(result.error).toContain("file changed between read and edit"); + expect("diff" in result).toBe(true); + if ("diff" in result) { + expect(result.diff).toContain("edited live"); } }); }); diff --git a/packages/hashline/CHANGELOG.md b/packages/hashline/CHANGELOG.md index 322c86b4f..dffa9b0e2 100644 --- a/packages/hashline/CHANGELOG.md +++ b/packages/hashline/CHANGELOG.md @@ -2,6 +2,14 @@ ## [Unreleased] +### Breaking Changes + +- Removed `SnapshotStore.byHashExact`; consumers resolve tags via `byHash`, which returns the most recently recorded version on a collision. + +### Changed + +- Improved robustness of patch application by resolving 16-bit snapshot tag collisions to the most recent version rather than rejecting them. + ## [16.3.0] - 2026-07-02 ### Changed diff --git a/packages/hashline/src/patcher.ts b/packages/hashline/src/patcher.ts index 2e3bc272d..f59cca7ca 100644 --- a/packages/hashline/src/patcher.ts +++ b/packages/hashline/src/patcher.ts @@ -521,23 +521,13 @@ export class Patcher { }): ApplyResult { const { section, canonicalPath, exists, normalized, edits } = args; const expected = exists ? section.fileHash : undefined; - // A 16-bit tag can collide across two different file states, so equality - // on `computeFileHash(normalized) === expected` alone is not enough to - // prove the live text IS the snapshot the tag names. Also require that, - // when a snapshot for `(path, expected)` is retained, exactly one stored - // version carries the tag and its full text matches the live text. If - // multiple versions share the tag, the header is ambiguous: there is no - // safe way to know which stored text the model's line anchors came from. - const storedSnapshotsForTag = - expected === undefined - ? [] - : this.snapshots.findByHash(expected).filter(snapshot => snapshot.path === canonicalPath); - const ambiguousStoredTag = storedSnapshotsForTag.length > 1; + // The 4-hex tag is content-derived: when the live text hashes to it, + // trust the match and apply directly. `storedSnapshotForTag` feeds the + // drift paths below (block resolution, 3-way recovery); on a 16-bit + // tag collision it resolves to the most-recently recorded text. const storedSnapshotForTag = expected === undefined ? null : this.snapshots.byHash(canonicalPath, expected); - const hashMatches = expected !== undefined && computeFileHash(normalized) === expected; - const matchedSnapshot = hashMatches ? this.snapshots.byContent(canonicalPath, normalized) : null; - const liveMatches = - hashMatches && !ambiguousStoredTag && (storedSnapshotForTag === null || matchedSnapshot !== null); + const liveMatches = expected !== undefined && computeFileHash(normalized) === expected; + const matchedSnapshot = liveMatches ? this.snapshots.byContent(canonicalPath, normalized) : null; // Resolve `replace_block N:` edits to concrete ranges before recovery // runs. Block anchors are expressed against the snapshot the section tag @@ -552,9 +542,6 @@ export class Patcher { const resolveWarnings: string[] = []; let resolved: readonly Edit[] = edits; if (hasBlockEdit(edits)) { - if (ambiguousStoredTag) { - throw this.#mismatchError(section, canonicalPath, normalized, expected ?? "", true); - } const baseText = expected === undefined || liveMatches ? normalized : storedSnapshotForTag?.text; if (baseText === undefined) { throw this.#mismatchError(section, canonicalPath, normalized, expected ?? "", false); @@ -590,9 +577,6 @@ export class Patcher { const result = applyEdits(normalized, resolved); return withResolveWarnings({ ...result, warnings: [HEADTAIL_DRIFT_WARNING, ...(result.warnings ?? [])] }); } - if (ambiguousStoredTag) { - throw this.#mismatchError(section, canonicalPath, normalized, expected ?? "", true); - } // File drifted: try to replay the edit against the version the tag // names and 3-way-merge it onto the live content. const recovered = this.recovery.tryRecover({ diff --git a/packages/hashline/src/recovery.ts b/packages/hashline/src/recovery.ts index ee7691fa1..b3e1b9ff4 100644 --- a/packages/hashline/src/recovery.ts +++ b/packages/hashline/src/recovery.ts @@ -392,11 +392,10 @@ export class Recovery { */ tryRecover(args: RecoveryArgs): RecoveryResult | null { const { path, currentText, fileHash, edits } = args; - // Collision-safe lookup: when two retained texts share the 16-bit tag - // there is no way to know which one the model's anchors were minted - // against — replaying against the wrong collider would land the edit - // on unrelated content. Refuse and let the caller reject (re-read). - const snapshot = this.store.byHashExact(path, fileHash); + // When two retained texts collide on the 16-bit tag, resolve to the + // most-recently recorded one; a wrong pick can only land if one of the + // merge/remap/session-chain strategies below applies it cleanly. + const snapshot = this.store.byHash(path, fileHash); if (!snapshot) return null; const isHead = isHeadSnapshot(this.store.head(path), snapshot); const recoveryWarning = isHead ? RECOVERY_EXTERNAL_WARNING : RECOVERY_SESSION_CHAIN_WARNING; diff --git a/packages/hashline/src/snapshots.ts b/packages/hashline/src/snapshots.ts index cb30b89e5..97189ec3b 100644 --- a/packages/hashline/src/snapshots.ts +++ b/packages/hashline/src/snapshots.ts @@ -11,7 +11,7 @@ * {@link SnapshotStore.record} with the full normalized text they observed. * The store hashes it, dedups against the per-path history, and returns the * tag. Consumers (recovery, the patcher) resolve a stale tag back to the - * recorded full text via {@link SnapshotStore.byHashExact} and 3-way-merge the + * recorded full text via {@link SnapshotStore.byHash} and 3-way-merge the * would-be edit onto the live content. * * The abstract base class lets callers plug in whatever storage they like @@ -49,8 +49,8 @@ export interface Snapshot { /** * Storage seam for full-file version snapshots. The patcher calls {@link head} - * for the latest version of a path and {@link byHashExact} when it needs the - * specific historical version a section's stale tag names. + * for the latest version of a path and {@link byHash} when it needs the + * historical version a section's stale tag names. */ export abstract class SnapshotStore { /** Most-recently recorded version for `path`, or `null` if none. */ @@ -59,27 +59,14 @@ export abstract class SnapshotStore { /** * Recorded version for `path` whose tag equals `hash`, or `null`. When two * distinct texts collide on the 16-bit tag, returns the most-recently - * recorded one; callers that treat the tag as content identity must use - * {@link byHashExact} (or verify {@link Snapshot.text} via {@link byContent}). + * recorded one. */ abstract byHash(path: string, hash: string): Snapshot | null; - /** - * Collision-safe {@link byHash}: the single retained version for `path` - * whose tag equals `hash`, or `null` when none is retained OR when two or - * more distinct texts collide on the tag. In the collision case there is - * no way to know which retained text the model's line anchors were minted - * against, so consumers that replay anchors (recovery, previews) must - * refuse rather than pick one. - */ - abstract byHashExact(path: string, hash: string): Snapshot | null; - /** * Recorded version for `path` whose {@link Snapshot.text} equals `fullText`, - * or `null`. Disambiguates hash collisions where two distinct file states - * share the same 4-hex tag: the patcher consults this before taking the - * no-drift path so a colliding live text is never accepted as the exact - * snapshot the model's line anchors were minted against. + * or `null`. The patcher uses it on the no-drift path to attach seen-line + * provenance to the exact text the model read. */ abstract byContent(path: string, fullText: string): Snapshot | null; @@ -189,20 +176,6 @@ export class InMemorySnapshotStore extends SnapshotStore { return history?.find(version => version.hash === hash) ?? null; } - byHashExact(path: string, hash: string): Snapshot | null { - const history = this.#versions.get(path); - if (history === undefined) return null; - let match: Snapshot | null = null; - for (const version of history) { - if (version.hash !== hash) continue; - // Two retained versions with one tag are distinct texts by - // construction (record() dedups on full-text equality) — ambiguous. - if (match !== null) return null; - match = version; - } - return match; - } - byContent(path: string, fullText: string): Snapshot | null { const history = this.#versions.get(path); return history?.find(version => version.text === fullText) ?? null; diff --git a/packages/hashline/test/patcher.test.ts b/packages/hashline/test/patcher.test.ts index f07ab03a2..d3efc1755 100644 --- a/packages/hashline/test/patcher.test.ts +++ b/packages/hashline/test/patcher.test.ts @@ -127,13 +127,11 @@ describe("Patcher snapshot tag integrity", () => { expect(fs.get(PATH)).toBe("current\n"); }); - // A 16-bit snapshot tag can collide across two different file states. - // When the model authored line-anchored edits against snapshot A but the - // live file is a colliding text B, `computeFileHash` alone cannot tell them - // apart. The patcher must NOT take the no-drift path against B: it either - // recovers against the stored A (3-way merge) or rejects as stale. - // Regression for issue #4075. - it("refuses to accept a colliding live text as the tagged snapshot", async () => { + // A 16-bit snapshot tag can collide across two different file states. Tag + // equality with the live content is trusted as-is: the model did nothing + // wrong, and a forced re-read would mint the very same tag. Line anchors + // therefore index the live text, colliding retained snapshots notwithstanding. + it("applies onto live content when the tag matches, even against a retained colliding snapshot", async () => { // These two texts both hash to `1D84`. const SNAPSHOT_TEXT = "line one 263\nline two 4471\n"; const LIVE_TEXT = "line one 410\nline two 6970\n"; @@ -145,59 +143,8 @@ describe("Patcher snapshot tag integrity", () => { const tag = snapshots.record(PATH, SNAPSHOT_TEXT, [1, 2]); const patcher = new Patcher({ fs, snapshots }); - try { - await patcher.apply(Patch.parse(`[${PATH}#${tag}]\nSWAP 2.=2:\n+edited from snapshot`)); - throw new Error("expected MismatchError"); - } catch (error) { - expect(error).toBeInstanceOf(MismatchError); - const message = (error as MismatchError).displayMessage; - // The tag IS a known snapshot, so we land on the drift branch. - expect(message).toMatch(/file changed between read and edit/); - } - // Live file untouched: line 2 must NOT have been overwritten with the - // model's edit anchored against the collider. - expect(fs.get(PATH)).toBe(LIVE_TEXT); - }); - - it("rejects an ambiguous colliding tag even when live text matches one retained collider", async () => { - const SNAPSHOT_TEXT = "line one 263\nline two 4471\n"; - const LIVE_TEXT = "line one 410\nline two 6970\n"; - expect(computeFileHash(SNAPSHOT_TEXT)).toBe(computeFileHash(LIVE_TEXT)); - - const snapshots = new InMemorySnapshotStore(); - const tag = snapshots.record(PATH, SNAPSHOT_TEXT, [1, 2]); - snapshots.record(PATH, LIVE_TEXT, [1, 2]); - - const fs = new InMemoryFilesystem([[PATH, LIVE_TEXT]]); - const patcher = new Patcher({ fs, snapshots }); - try { - await patcher.apply(Patch.parse(`[${PATH}#${tag}]\nSWAP 2.=2:\n+edited from snapshot`)); - throw new Error("expected MismatchError"); - } catch (error) { - expect(error).toBeInstanceOf(MismatchError); - } - expect(fs.get(PATH)).toBe(LIVE_TEXT); - }); - - it("rejects an ambiguous colliding tag before stale recovery can target the wrong snapshot", async () => { - const SNAPSHOT_TEXT = "line one 263\nline two 4471\n"; - const COLLIDING_TEXT = "line one 410\nline two 6970\n"; - const LIVE_TEXT = "line one 410\nline two 6970\nlive-added\n"; - expect(computeFileHash(SNAPSHOT_TEXT)).toBe(computeFileHash(COLLIDING_TEXT)); - - const snapshots = new InMemorySnapshotStore(); - const tag = snapshots.record(PATH, SNAPSHOT_TEXT, [1, 2]); - snapshots.record(PATH, COLLIDING_TEXT, [1, 2]); - - const fs = new InMemoryFilesystem([[PATH, LIVE_TEXT]]); - const patcher = new Patcher({ fs, snapshots }); - try { - await patcher.apply(Patch.parse(`[${PATH}#${tag}]\nSWAP 2.=2:\n+edited from snapshot`)); - throw new Error("expected MismatchError"); - } catch (error) { - expect(error).toBeInstanceOf(MismatchError); - } - expect(fs.get(PATH)).toBe(LIVE_TEXT); + await patcher.apply(Patch.parse(`[${PATH}#${tag}]\nSWAP 2.=2:\n+edited live`)); + expect(fs.get(PATH)).toBe("line one 410\nedited live\n"); }); }); diff --git a/packages/hashline/test/recovery-session-chain.test.ts b/packages/hashline/test/recovery-session-chain.test.ts index d95f526f9..08db6634e 100644 --- a/packages/hashline/test/recovery-session-chain.test.ts +++ b/packages/hashline/test/recovery-session-chain.test.ts @@ -203,7 +203,7 @@ function findCollidingTexts(): { older: string; newer: string } { } describe("Recovery — colliding snapshot tags", () => { - it("refuses recovery when two retained texts share the section's tag", () => { + it("recovers against the most-recently retained text when two colliders share the tag", () => { const { older, newer } = findCollidingTexts(); const tag = computeFileHash(older); expect(computeFileHash(newer)).toBe(tag); @@ -213,12 +213,9 @@ describe("Recovery — colliding snapshot tags", () => { store.record(PATH, older); store.record(PATH, newer); - // Live file drifted away from both colliders, so recovery cannot - // shortcut via live==snapshot; it must pick a base text for the tag. - // The model's edit was authored against `older` (line 2 = its unique - // payload). Resolving the tag to the most-recent collider would 3-way - // merge the stale payload onto `newer`'s unrelated line 2 — silent - // corruption. The ambiguous tag must refuse instead. + // Live drifted away from both colliders, so recovery cannot shortcut + // via live==snapshot. The tag cannot name a unique base; it resolves + // to the most-recently recorded collider and 3-way merges from there. const currentText = `${newer}drifted trailer\n`; const recovered = new Recovery(store).tryRecover({ path: PATH, @@ -227,13 +224,12 @@ describe("Recovery — colliding snapshot tags", () => { edits: parsePatch("SWAP 2.=2:\n+model payload").edits, }); - expect(recovered).toBeNull(); + expect(recovered?.text).toBe(lines("shared head", "model payload", "shared tail", "drifted trailer")); }); it("still recovers when exactly one retained text carries the tag", () => { - // Same drift scenario minus the collision: the unambiguous tag keeps - // recovering via 3-way merge, proving the collision gate above does - // not overreach. + // Same drift scenario with a single retained text for the tag: the + // plain 3-way merge path. const { older } = findCollidingTexts(); const store = new InMemorySnapshotStore(); const tag = store.record(PATH, older); diff --git a/packages/hashline/test/snapshots.test.ts b/packages/hashline/test/snapshots.test.ts index af2dc0f57..58ba324e8 100644 --- a/packages/hashline/test/snapshots.test.ts +++ b/packages/hashline/test/snapshots.test.ts @@ -151,25 +151,5 @@ describe("InMemorySnapshotStore", () => { expect(store.byContent(PATH, COLLIDE_A)?.seenLines).toEqual(new Set([1, 2])); expect(store.byContent(PATH, COLLIDE_B)).toBeNull(); }); - - it("byHashExact returns the single retained collider, or null when the tag is ambiguous", () => { - const store = new InMemorySnapshotStore(); - expect(store.byHashExact(PATH, computeFileHash(COLLIDE_A))).toBeNull(); - - const tag = store.record(PATH, COLLIDE_A); - // Exactly one retained text for the tag → safe to resolve. - expect(store.byHashExact(PATH, tag)?.text).toBe(COLLIDE_A); - - store.record(PATH, COLLIDE_B); - // Two distinct texts now share the tag: there is no way to know - // which one a section's anchors were minted against, so the - // collision-safe lookup must refuse while byHash still surfaces - // the most-recent collider. - expect(store.byHashExact(PATH, tag)).toBeNull(); - expect(store.byHash(PATH, tag)?.text).toBe(COLLIDE_B); - // Other paths and unknown tags stay null. - expect(store.byHashExact(OTHER, tag)).toBeNull(); - expect(store.byHashExact(PATH, tag === "0000" ? "FFFF" : "0000")).toBeNull(); - }); }); });