From 41b1cb4f3b0844721d0a1406970d42e744d55eac Mon Sep 17 00:00:00 2001 From: Ben Stull Date: Wed, 10 Jun 2026 11:00:28 -0700 Subject: [PATCH] F3 hardening: observable seam misses, cheaper disk-compare, E2E ordering notes (#6) Co-Authored-By: Claude Opus 4.8 (1M context) --- src/attributionController.ts | 33 +++++++++++++++++++++++++++--- src/pendingEdits.ts | 10 ++++++--- test/e2e/suite/attribution.test.ts | 3 +++ test/pendingEdits.test.ts | 9 ++++++++ 4 files changed, 49 insertions(+), 6 deletions(-) diff --git a/src/attributionController.ts b/src/attributionController.ts index 5e2b2bb..09fd2c0 100644 --- a/src/attributionController.ts +++ b/src/attributionController.ts @@ -169,10 +169,24 @@ export class AttributionController implements vscode.Disposable { this.render(e.document); } - /** True when the document buffer is byte-identical to the file on disk. */ + /** + * True when the document buffer is byte-identical to the file on disk. + * Fires only on `!isDirty` change events — first change after every clean + * state (frequent under `files.autoSave: afterDelay`) and reverts/reloads; + * O(file size) sync read. A size pre-check avoids the full read in the + * common case: if the byte lengths differ (accounting for a possible 3-byte + * UTF-8 BOM that `document.getText()` never includes) we return early. When + * a BOM is present the disk read is stripped before comparing so a genuine + * revert does not clobber attribution. + */ private matchesDisk(document: vscode.TextDocument): boolean { try { - return fs.readFileSync(document.uri.fsPath, "utf8") === document.getText(); + const bufLen = Buffer.byteLength(document.getText(), "utf8"); + const stat = fs.statSync(document.uri.fsPath); + // Allow size === bufLen (no BOM) OR size === bufLen + 3 (UTF-8 BOM). + if (stat.size !== bufLen && stat.size !== bufLen + 3) return false; + const disk = fs.readFileSync(document.uri.fsPath, "utf8").replace(/^/, ""); + return disk === document.getText(); } catch { // Unreadable/missing file: treat as a real edit (attribute), per // fail-open honesty — misclassifying a sync as an edit is recoverable. @@ -190,6 +204,8 @@ export class AttributionController implements vscode.Disposable { * `match` already consumed it), so a no-op edit never leaks a registration. * The replace is self-minimized (common prefix/suffix trimmed) to mirror the * host's WorkspaceEdit diff-minimization, so registered == applied == delivered. + * Callers issuing concurrent edits on the same document must serialize them or + * pass `expectedVersion` (offsets are computed against the call-time snapshot). */ async applyAgentEdit( document: vscode.TextDocument, @@ -225,7 +241,18 @@ export class AttributionController implements vscode.Disposable { const we = new vscode.WorkspaceEdit(); we.replace(document.uri, new vscode.Range(document.positionAt(minStart), document.positionAt(minEnd)), minText); const ok = await vscode.workspace.applyEdit(we); - this.pending.unregister(pendingEdit); + const removed = this.pending.unregister(pendingEdit); + if (ok && removed) { + // workspace.applyEdit resolves AFTER onDidChangeTextDocument is dispatched + // synchronously to listeners, so a matching change event should have already + // consumed the registration before we reach here. If it is still present, + // the host minimized the diff differently than we predicted — attribution + // may be wrong for this edit (INV-9). + this.output.appendLine( + "WARN: seam edit applied but its change event never matched the registration " + + "(host minimized differently?) — the edit may be mis-attributed (INV-9).", + ); + } return ok; } diff --git a/src/pendingEdits.ts b/src/pendingEdits.ts index 7318f83..5760eb8 100644 --- a/src/pendingEdits.ts +++ b/src/pendingEdits.ts @@ -55,11 +55,15 @@ export class PendingEditRegistry { /** * Remove a registration that failed to apply. Identity-based (`===`), so - * callers must keep the exact registered object; it is a no-op when `match` - * already consumed the registration. + * callers must keep the exact registered object. Returns `true` if the + * registration was still pending and was removed, `false` if it was already + * consumed by `match` (or was never registered). This lets callers detect + * seam misses for diagnosability (INV-9). */ - unregister(edit: PendingEdit): void { + unregister(edit: PendingEdit): boolean { + const before = this.pending.length; this.pending = this.pending.filter((p) => p !== edit); + return this.pending.length < before; } /** diff --git a/test/e2e/suite/attribution.test.ts b/test/e2e/suite/attribution.test.ts index 7ae26cb..386cd0e 100644 --- a/test/e2e/suite/attribution.test.ts +++ b/test/e2e/suite/attribution.test.ts @@ -35,6 +35,9 @@ async function externalWriteAndReload(uri: vscode.Uri, content: string): Promise } const settle = () => new Promise((r) => setTimeout(r, 300)); +// Tests are ORDER-DEPENDENT: later tests consume earlier tests' document/sidecar +// state by design, mirroring the F2 suite. This suite owns docs/attrib.md; +// the threads suite owns docs/sample.md — keep fixtures disjoint. suite("F3 live attribution (host E2E — seam-driven, no LLM)", () => { const TYPED = "A human-typed sentence. "; diff --git a/test/pendingEdits.test.ts b/test/pendingEdits.test.ts index 77c3c88..81d241d 100644 --- a/test/pendingEdits.test.ts +++ b/test/pendingEdits.test.ts @@ -29,6 +29,15 @@ describe("PendingEditRegistry (INV-9)", () => { reg.unregister(p); expect(reg.match("d.md", { start: 0, end: 0, text: "x" })).toBeNull(); }); + it("unregister reports whether the registration was still pending", () => { + const reg = new PendingEditRegistry(); + const p = { docPath: "d.md", start: 0, end: 0, newText: "x", provenance: AGENT }; + reg.register(p); + expect(reg.unregister(p)).toBe(true); + reg.register(p); + reg.match("d.md", { start: 0, end: 0, text: "x" }); + expect(reg.unregister(p)).toBe(false); + }); }); describe("minimizeReplace (host diff-minimization mirror)", () => {