fix #38: undo/redo no longer mis-attributes restored text in the review preview #39
@@ -152,18 +152,28 @@ export class AttributionController implements vscode.Disposable {
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
const s = this.state(docPath);
|
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
|
// 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
|
// 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
|
// 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.
|
// replacement (INV-9) — apply it as ONE algebra edit, not per hunk.
|
||||||
const hit = this.pending.matchEvent(
|
const hit = isUndoRedo
|
||||||
docPath,
|
? null
|
||||||
e.contentChanges.map((c) => ({
|
: this.pending.matchEvent(
|
||||||
start: c.rangeOffset,
|
docPath,
|
||||||
end: c.rangeOffset + c.rangeLength,
|
e.contentChanges.map((c) => ({
|
||||||
newLength: c.text.length,
|
start: c.rangeOffset,
|
||||||
})),
|
end: c.rangeOffset + c.rangeLength,
|
||||||
);
|
newLength: c.text.length,
|
||||||
|
})),
|
||||||
|
);
|
||||||
if (hit) {
|
if (hit) {
|
||||||
const full = hit.full ?? { start: hit.start, end: hit.end, newLength: hit.newText.length };
|
const full = hit.full ?? { start: hit.start, end: hit.end, newLength: hit.newText.length };
|
||||||
s.spans = applyChange(s.spans, full, hit.provenance, {
|
s.spans = applyChange(s.spans, full, hit.provenance, {
|
||||||
@@ -180,10 +190,16 @@ export class AttributionController implements vscode.Disposable {
|
|||||||
end: change.rangeOffset + change.rangeLength,
|
end: change.rangeOffset + change.rangeLength,
|
||||||
newLength: change.text.length,
|
newLength: change.text.length,
|
||||||
};
|
};
|
||||||
s.spans = applyChange(s.spans, edit, this.currentAuthor(), {
|
s.spans = applyChange(
|
||||||
newId: () => newId("at"),
|
s.spans,
|
||||||
now: () => new Date().toISOString(),
|
edit,
|
||||||
});
|
this.currentAuthor(),
|
||||||
|
{
|
||||||
|
newId: () => newId("at"),
|
||||||
|
now: () => new Date().toISOString(),
|
||||||
|
},
|
||||||
|
!isUndoRedo,
|
||||||
|
);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
if (s.spans.length > 0) s.hadAttributions = true;
|
if (s.spans.length > 0) s.hadAttributions = true;
|
||||||
|
|||||||
@@ -49,12 +49,19 @@ export function coalesce(spans: LiveSpan[]): LiveSpan[] {
|
|||||||
* Apply one document edit (the half-open range [start,end) replaced by
|
* Apply one document edit (the half-open range [start,end) replaced by
|
||||||
* newLength chars) authored by `author`. Existing spans shift/split/clip;
|
* newLength chars) authored by `author`. Existing spans shift/split/clip;
|
||||||
* inserted chars become a new span of `author` (INV-7).
|
* 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(
|
export function applyChange(
|
||||||
spans: LiveSpan[],
|
spans: LiveSpan[],
|
||||||
edit: TextEdit,
|
edit: TextEdit,
|
||||||
author: Provenance,
|
author: Provenance,
|
||||||
ctx: TrackerCtx,
|
ctx: TrackerCtx,
|
||||||
|
attributeInserted = true,
|
||||||
): LiveSpan[] {
|
): LiveSpan[] {
|
||||||
const delta = edit.newLength - (edit.end - edit.start);
|
const delta = edit.newLength - (edit.end - edit.start);
|
||||||
const out: LiveSpan[] = [];
|
const out: LiveSpan[] = [];
|
||||||
@@ -71,7 +78,7 @@ export function applyChange(
|
|||||||
if (right) out.push(left ? { ...right, id: ctx.newId() } : right);
|
if (right) out.push(left ? { ...right, id: ctx.newId() } : right);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
if (edit.newLength > 0) {
|
if (attributeInserted && edit.newLength > 0) {
|
||||||
out.push({
|
out.push({
|
||||||
id: ctx.newId(),
|
id: ctx.newId(),
|
||||||
start: edit.start,
|
start: edit.start,
|
||||||
|
|||||||
@@ -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", () => {
|
describe("coalesce", () => {
|
||||||
it("merges adjacent same-author same-turn spans, keeps earliest createdAt", () => {
|
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" };
|
const a = { ...span("a", 0, 3, HUMAN), createdAt: "2026-06-09T00:00:00.000Z" };
|
||||||
|
|||||||
@@ -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<CowritingApi> {
|
||||||
|
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 = /<span class="cw-by-human">[^<]*bravo/.test(html);
|
||||||
|
assert.ok(!bravoColoredHuman, "restored 'bravo' is not colored cw-by-human in the preview");
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user