From 469c8c11fc4dc6096a3d3ffadcd107a51585356a Mon Sep 17 00:00:00 2001 From: Ben Stull Date: Fri, 12 Jun 2026 03:37:22 -0700 Subject: [PATCH] fix #38: don't mis-attribute undo/redo as fresh human authorship in the preview MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Undo in the editor rendered wrong marks in the F10 review preview: restored baseline text (and reverted Claude text) was colored as human-authored. Root cause — attributionController.onDidChange attributed every non-seam document change to currentAuthor() (always human) and ignored e.reason, so an undo/redo that re-inserts text created a fresh human span over it. renderReview is pure; the wrong marks came from these false author spans. Fix: on e.reason === Undo|Redo, reconcile span geometry but do NOT attribute the re-inserted chars — an undo is history navigation, not authorship, so restored text stays neutral (unattributed) rather than falsely claimed by the human. applyChange gains an `attributeInserted` flag (default true; false on undo/redo); seam matching is also skipped on undo/redo (the seam only applies forward edits). Restored text becomes neutral rather than recovering its exact prior provenance (e.g. undoing a deletion of Claude's text shows it unattributed, not blue) — perfect restoration would need an attribution history stack synced to the editor undo stack (fragile due to edit coalescing); filed mentally as a follow-up. The neutral behavior removes the misleading marks, which is the reported defect. Tests: E2E reproduces the mid-edit-undo case (buffer stays dirty so the disk-sync guard doesn't mask it) and asserts restored text is unattributed; +3 unit tests for the geometric-only applyChange path. 197 unit + 50 E2E green; typecheck clean. Closes #38 Co-Authored-By: Claude Opus 4.8 (1M context) --- src/attributionController.ts | 40 ++++++++++++----- src/attributionTracker.ts | 9 +++- test/attributionTracker.test.ts | 19 ++++++++ test/e2e/suite/undoMarks.test.ts | 76 ++++++++++++++++++++++++++++++++ 4 files changed, 131 insertions(+), 13 deletions(-) create mode 100644 test/e2e/suite/undoMarks.test.ts diff --git a/src/attributionController.ts b/src/attributionController.ts index 1bb13b9..a9f4947 100644 --- a/src/attributionController.ts +++ b/src/attributionController.ts @@ -152,18 +152,28 @@ export class AttributionController implements vscode.Disposable { return; } const s = this.state(docPath); + // An undo/redo is history navigation, NOT authorship (#38): reconcile span + // geometry but never freshly attribute the re-inserted text to the current + // author — otherwise restored baseline text (or reverted Claude text) is + // falsely colored human in the preview. A seam edit is always a forward + // apply, so undo/redo also bypasses seam matching. + const isUndoRedo = + e.reason === vscode.TextDocumentChangeReason.Undo || + e.reason === vscode.TextDocumentChangeReason.Redo; // One applyEdit = one change event, but the host may deliver a seam edit // as SEVERAL minimal hunks (word-level diffing). Match the EVENT's net // effect against the registry; on a hit the agent owns its FULL intended // replacement (INV-9) — apply it as ONE algebra edit, not per hunk. - const hit = this.pending.matchEvent( - docPath, - e.contentChanges.map((c) => ({ - start: c.rangeOffset, - end: c.rangeOffset + c.rangeLength, - newLength: c.text.length, - })), - ); + const hit = isUndoRedo + ? null + : this.pending.matchEvent( + docPath, + e.contentChanges.map((c) => ({ + start: c.rangeOffset, + end: c.rangeOffset + c.rangeLength, + newLength: c.text.length, + })), + ); if (hit) { const full = hit.full ?? { start: hit.start, end: hit.end, newLength: hit.newText.length }; s.spans = applyChange(s.spans, full, hit.provenance, { @@ -180,10 +190,16 @@ export class AttributionController implements vscode.Disposable { end: change.rangeOffset + change.rangeLength, newLength: change.text.length, }; - s.spans = applyChange(s.spans, edit, this.currentAuthor(), { - newId: () => newId("at"), - now: () => new Date().toISOString(), - }); + s.spans = applyChange( + s.spans, + edit, + this.currentAuthor(), + { + newId: () => newId("at"), + now: () => new Date().toISOString(), + }, + !isUndoRedo, + ); } } if (s.spans.length > 0) s.hadAttributions = true; diff --git a/src/attributionTracker.ts b/src/attributionTracker.ts index 0b95a23..7dfef46 100644 --- a/src/attributionTracker.ts +++ b/src/attributionTracker.ts @@ -49,12 +49,19 @@ export function coalesce(spans: LiveSpan[]): LiveSpan[] { * Apply one document edit (the half-open range [start,end) replaced by * newLength chars) authored by `author`. Existing spans shift/split/clip; * inserted chars become a new span of `author` (INV-7). + * + * `attributeInserted` (default true) controls whether inserted chars get a new + * span. Pass `false` for an UNDO/REDO change (#38): the geometry of existing + * spans is still reconciled, but re-inserted text is NOT freshly attributed — + * an undo is history navigation, not authorship, so restored text stays neutral + * rather than being falsely claimed by the current author. */ export function applyChange( spans: LiveSpan[], edit: TextEdit, author: Provenance, ctx: TrackerCtx, + attributeInserted = true, ): LiveSpan[] { const delta = edit.newLength - (edit.end - edit.start); const out: LiveSpan[] = []; @@ -71,7 +78,7 @@ export function applyChange( if (right) out.push(left ? { ...right, id: ctx.newId() } : right); } } - if (edit.newLength > 0) { + if (attributeInserted && edit.newLength > 0) { out.push({ id: ctx.newId(), start: edit.start, diff --git a/test/attributionTracker.test.ts b/test/attributionTracker.test.ts index de0c9e8..c213008 100644 --- a/test/attributionTracker.test.ts +++ b/test/attributionTracker.test.ts @@ -107,6 +107,25 @@ describe("multi-edit sequences", () => { }); }); +describe("applyChange — geometric-only (undo/redo, #38)", () => { + it("attributeInserted=false adds NO span for re-inserted text (restored text stays neutral)", () => { + // Undo re-inserts 'bravo ' at offset 6 into an empty span list → no span. + const r = applyChange([], { start: 6, end: 6, newLength: 6 }, HUMAN, ctx(), false); + expect(r).toEqual([]); + }); + it("attributeInserted=false still SHIFTS existing spans by the edit delta", () => { + // A real human span sits after the re-insert point; it must shift right by 6, + // but the re-inserted chars themselves get no new span. + const r = applyChange([span("tail", 20, 30, HUMAN)], { start: 6, end: 6, newLength: 6 }, HUMAN, ctx(), false); + expect(ranges(r)).toEqual([[26, 36]]); + expect(authors(r)).toEqual(["human"]); + }); + it("attributeInserted=false still reconciles geometry of a deletion (removes a covered span)", () => { + const r = applyChange([span("a", 3, 6, AGENT)], { start: 0, end: 10, newLength: 0 }, HUMAN, ctx(), false); + expect(r).toEqual([]); + }); +}); + describe("coalesce", () => { it("merges adjacent same-author same-turn spans, keeps earliest createdAt", () => { const a = { ...span("a", 0, 3, HUMAN), createdAt: "2026-06-09T00:00:00.000Z" }; diff --git a/test/e2e/suite/undoMarks.test.ts b/test/e2e/suite/undoMarks.test.ts new file mode 100644 index 0000000..ddeff88 --- /dev/null +++ b/test/e2e/suite/undoMarks.test.ts @@ -0,0 +1,76 @@ +import * as assert from "assert"; +import * as fs from "fs"; +import * as path from "path"; +import * as vscode from "vscode"; +import type { CowritingApi } from "../../../src/extension"; + +const WS = process.env.E2E_WORKSPACE!; +const settle = () => new Promise((r) => setTimeout(r, 400)); + +async function getApi(): Promise { + const ext = vscode.extensions.getExtension("benstull.vscode-cowriting-plugin")!; + const api = (await ext.activate()) as CowritingApi; + assert.ok(api?.attributionController && api?.trackChangesPreviewController, "exports attribution + preview"); + return api; +} + +async function freshDoc(rel: string, body: string): Promise<{ doc: vscode.TextDocument; key: string }> { + const abs = path.join(WS, rel); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, body, "utf8"); + const uri = vscode.Uri.file(abs); + const doc = await vscode.workspace.openTextDocument(uri); + await vscode.window.showTextDocument(doc); + await settle(); + return { doc, key: uri.toString() }; +} + +// #38 (P1): undo in the editor renders WRONG marks in the F10 review preview. +// Root cause: attribution attributes every non-seam change to the human and +// ignores e.reason, so an undo that re-inserts text falsely colors it human. +// We drive a MID-EDIT undo (buffer stays dirty, so the disk-sync guard doesn't +// mask it) and assert the restored baseline text is NOT re-attributed. +suite("F10 #38 — undo does not mis-attribute restored text (host E2E, no LLM)", () => { + const DOC_REL = "docs/undo38.md"; + const BASE = "Alpha bravo charlie.\n"; + + test("undo of a deletion of baseline text leaves it unattributed (not human)", async () => { + const { doc, key } = await freshDoc(DOC_REL, BASE); + const api = await getApi(); + + // Forward edit 1 (human): append a tail so a LATER undo of edit 2 keeps the + // buffer dirty (≠ disk) → the attribution branch runs, not the disk-sync one. + const e1 = new vscode.WorkspaceEdit(); + e1.insert(doc.uri, doc.positionAt(doc.getText().length), "\nHuman tail.\n"); + assert.ok(await vscode.workspace.applyEdit(e1), "edit 1 applied"); + await settle(); + + // Forward edit 2 (human): delete the baseline word "bravo " (offsets 6..12). + const e2 = new vscode.WorkspaceEdit(); + e2.delete(doc.uri, new vscode.Range(doc.positionAt(6), doc.positionAt(12))); + assert.ok(await vscode.workspace.applyEdit(e2), "edit 2 applied"); + await settle(); + assert.ok(!doc.getText().includes("bravo"), "bravo deleted"); + + // Undo edit 2 → "bravo " is re-inserted. It is RESTORED baseline text, not + // freshly authored — it must NOT become a human-attributed span. + await vscode.commands.executeCommand("undo"); + await settle(); + assert.ok(doc.getText().includes("Alpha bravo charlie."), "undo restored 'bravo '"); + assert.ok(doc.isDirty, "buffer still dirty (mid-edit undo → attribution branch, not disk-sync)"); + + const bravoStart = doc.getText().indexOf("bravo"); + const spans = api.attributionController.spansFor(doc); + const overBravo = spans.filter((s) => s.start < bravoStart + 5 && s.end > bravoStart); + assert.deepStrictEqual( + overBravo, + [], + `restored baseline text 'bravo' must be unattributed, got spans: ${JSON.stringify(overBravo)}`, + ); + + // And the on-state render must not color 'bravo' as human-authored. + const html = api.trackChangesPreviewController.renderHtmlFor(key); + const bravoColoredHuman = /[^<]*bravo/.test(html); + assert.ok(!bravoColoredHuman, "restored 'bravo' is not colored cw-by-human in the preview"); + }); +});