diff --git a/docs/superpowers/plans/2026-06-10-f4-propose-accept.md b/docs/superpowers/plans/2026-06-10-f4-propose-accept.md index 1a008a5..af10294 100644 --- a/docs/superpowers/plans/2026-06-10-f4-propose-accept.md +++ b/docs/superpowers/plans/2026-06-10-f4-propose-accept.md @@ -1153,6 +1153,21 @@ git commit -m "F4: document propose/accept — manual smoke, README, playground --- +## Amendments (during execution) + +- **AMENDMENT 1 (Task 7):** the F4 accept E2E exposed a latent F3 seam + limitation — the host word-diffs one applied WorkspaceEdit into SEVERAL + minimal hunks when old/new share interior tokens (F4's realistic + replacements do; F3's test strings never did), so the registry's per-hunk + exact match missed and the edit fell through as fragmented HUMAN spans. + Fix: `PendingEditRegistry.match` (per-change equality) is replaced by + `matchEvent` (event-level net-effect match: every hunk inside the + registered full pre-edit range + equal net length delta — sound because one + applyEdit is one change event), and `AttributionController.onDidChange` + matches the event once, applying the full intended replacement as ONE + algebra edit on a hit. Unit tests rewritten accordingly + (`test/pendingEdits.test.ts`); INV-9's miss-warning path unchanged. + ## Self-review notes - **Spec coverage:** §6.3 model → Task 1/3; store prune → Task 2; §6.4 controller contracts (propose/acceptById/rejectById/getRendered/getStaleCount, hidden command) → Tasks 4/5; §6.5 PUC-1..5 → Tasks 4/6/7; §5 UX (amber deco, second comments controller, ✓/✗ title actions, status-bar stale count) → Tasks 4/5; INV-10..13 asserted in E2E (Task 7); §6.8 testing strategy → Tasks 1–3 (unit) + 7 (host E2E) + 8 (manual smoke); §7.3 done-bar → Task 8 gate + smoke. diff --git a/src/attributionController.ts b/src/attributionController.ts index 245d5e2..dd74390 100644 --- a/src/attributionController.ts +++ b/src/attributionController.ts @@ -146,24 +146,39 @@ export class AttributionController implements vscode.Disposable { return; } const s = this.state(docPath); - // Sort descending by offset so earlier changes don't invalidate later offsets - // (VS Code's order is undocumented; defensive sort is the safe guarantee). - for (const change of [...e.contentChanges].sort((a, b) => b.rangeOffset - a.rangeOffset)) { - const edit = { - start: change.rangeOffset, - end: change.rangeOffset + change.rangeLength, - newLength: change.text.length, - }; - const hit = this.pending.match(docPath, { start: edit.start, end: edit.end, text: change.text }); - const author = hit ? hit.provenance : this.currentAuthor(); - // On a seam hit, attribute the agent's FULL intended replacement (the - // registered edit is diff-minimized for transport only): same delta, - // wider span — the agent owns every char it asserted (INV-9). - s.spans = applyChange(s.spans, hit?.full ?? edit, author, { + // 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, + })), + ); + if (hit) { + const full = hit.full ?? { start: hit.start, end: hit.end, newLength: hit.newText.length }; + s.spans = applyChange(s.spans, full, hit.provenance, { newId: () => newId("at"), now: () => new Date().toISOString(), - turnId: hit?.turnId, + turnId: hit.turnId, }); + } else { + // Sort descending by offset so earlier changes don't invalidate later offsets + // (VS Code's order is undocumented; defensive sort is the safe guarantee). + for (const change of [...e.contentChanges].sort((a, b) => b.rangeOffset - a.rangeOffset)) { + const edit = { + start: change.rangeOffset, + end: change.rangeOffset + change.rangeLength, + newLength: change.text.length, + }; + s.spans = applyChange(s.spans, edit, this.currentAuthor(), { + newId: () => newId("at"), + now: () => new Date().toISOString(), + }); + } } if (s.spans.length > 0) s.hadAttributions = true; this.render(e.document); diff --git a/src/pendingEdits.ts b/src/pendingEdits.ts index 5760eb8..a646786 100644 --- a/src/pendingEdits.ts +++ b/src/pendingEdits.ts @@ -67,17 +67,28 @@ export class PendingEditRegistry { } /** - * Find-and-consume the registration exactly matching a change event - * (same doc, same replaced range, same inserted text). Null → human edit. + * Find-and-consume the registration matching a change EVENT's net effect. + * The host may deliver one applied WorkspaceEdit as SEVERAL minimal hunks + * (word-level diffing — observed on the F4 accept path), so per-hunk + * equality misses real seam edits and they fall through as "human typing". + * A pending edit matches an event when EVERY hunk lies inside its full + * pre-edit range and the event's net length delta equals the edit's + * (one applyEdit = one change event, so a seam event is never mixed with + * human hunks). Null → human edit. */ - match(docPath: string, change: { start: number; end: number; text: string }): PendingEdit | null { - const i = this.pending.findIndex( - (p) => - p.docPath === docPath && - p.start === change.start && - p.end === change.end && - p.newText === change.text, - ); + matchEvent( + docPath: string, + changes: ReadonlyArray<{ start: number; end: number; newLength: number }>, + ): PendingEdit | null { + if (changes.length === 0) return null; + const i = this.pending.findIndex((p) => { + if (p.docPath !== docPath) return false; + const full = p.full ?? { start: p.start, end: p.end, newLength: p.newText.length }; + const delta = full.newLength - (full.end - full.start); + const eventDelta = changes.reduce((d, c) => d + (c.newLength - (c.end - c.start)), 0); + if (delta !== eventDelta) return false; + return changes.every((c) => c.start >= full.start && c.end <= full.end); + }); if (i === -1) return null; const [hit] = this.pending.splice(i, 1); return hit; diff --git a/test/e2e/fixtures/workspace/docs/proposal.md b/test/e2e/fixtures/workspace/docs/proposal.md new file mode 100644 index 0000000..7a6cb36 --- /dev/null +++ b/test/e2e/fixtures/workspace/docs/proposal.md @@ -0,0 +1,9 @@ +# Proposal fixture + +A stable opening paragraph. + +The propose target sentence lives here. + +A second target for coexistence checks. + +A stable closing paragraph. diff --git a/test/e2e/suite-no-workspace/noWorkspace.test.ts b/test/e2e/suite-no-workspace/noWorkspace.test.ts index 96b9d07..b3748f0 100644 --- a/test/e2e/suite-no-workspace/noWorkspace.test.ts +++ b/test/e2e/suite-no-workspace/noWorkspace.test.ts @@ -25,6 +25,9 @@ suite("no-workspace activation (#8)", () => { "cowriting.editSelection", "cowriting.toggleAttribution", "cowriting.applyAgentEdit", + "cowriting.acceptProposal", + "cowriting.rejectProposal", + "cowriting.proposeAgentEdit", ]) { assert.ok(all.includes(command), `${command} is registered`); } diff --git a/test/e2e/suite/proposals.test.ts b/test/e2e/suite/proposals.test.ts new file mode 100644 index 0000000..0ba7c9c --- /dev/null +++ b/test/e2e/suite/proposals.test.ts @@ -0,0 +1,163 @@ +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"; +import type { Artifact } from "../../../src/model"; + +const WS = process.env.E2E_WORKSPACE!; +const DOC_REL = "docs/proposal.md"; + +function sidecarPath(): string { + return path.join(WS, ".threads", "docs", "proposal.md.json"); +} +function readSidecar(): Artifact { + return JSON.parse(fs.readFileSync(sidecarPath(), "utf8")) as Artifact; +} +async function openDoc(): Promise { + const uri = vscode.Uri.file(path.join(WS, DOC_REL)); + const doc = await vscode.workspace.openTextDocument(uri); + await vscode.window.showTextDocument(doc); + return doc; +} +async function getApi(): Promise { + const ext = vscode.extensions.getExtension("benstull.vscode-cowriting-plugin")!; + const api = (await ext.activate()) as CowritingApi; + assert.ok(api?.proposalController, "extension exports proposalController"); + return api; +} +async function proposeViaCommand( + doc: vscode.TextDocument, + target: string, + newText: string, + turnId: string, +): Promise { + const start = doc.getText().indexOf(target); + assert.ok(start >= 0, `fixture contains "${target}"`); + const id = await vscode.commands.executeCommand("cowriting.proposeAgentEdit", { + uri: doc.uri.toString(), + start, + end: start + target.length, + newText, + model: "sonnet", + sessionId: "e2e-prop", + turnId, + instruction: "e2e instruction", + }); + assert.ok(id, "propose returns the proposal id"); + return id!; +} +async function externalWriteAndReload(uri: vscode.Uri, content: string): Promise { + fs.writeFileSync(uri.fsPath, content, "utf8"); + const doc = await vscode.workspace.openTextDocument(uri); + await vscode.window.showTextDocument(doc); + await vscode.commands.executeCommand("workbench.action.files.revert"); + return doc; +} +const settle = () => new Promise((r) => setTimeout(r, 300)); + +// Tests are ORDER-DEPENDENT (F2/F3 pattern): later tests consume earlier state. +// This suite OWNS docs/proposal.md; threads owns docs/sample.md, attribution +// owns docs/attrib.md — fixtures stay disjoint. +suite("F4 propose/accept (host E2E — programmatic ingress, no LLM)", () => { + const TARGET = "The propose target sentence lives here."; + const REPLACEMENT = "PROPOSED-BY-CLAUDE replacement sentence."; + + test("propose records + renders a pending proposal and does NOT touch the document (INV-10)", async () => { + const doc = await openDoc(); + const api = await getApi(); + const before = doc.getText(); + await proposeViaCommand(doc, TARGET, REPLACEMENT, "turn-p1"); + await settle(); + assert.strictEqual(doc.getText(), before, "document text unchanged (INV-10)"); + const rendered = api.proposalController.getRendered(DOC_REL); + assert.strictEqual(rendered.length, 1); + assert.strictEqual(rendered[0].pending, true); + assert.strictEqual(rendered[0].turnId, "turn-p1"); + const art = readSidecar(); + assert.strictEqual(art.proposals.length, 1, "proposal persisted at propose time"); + assert.strictEqual(art.anchors[art.proposals[0].anchorId].fingerprint.text, TARGET); + }); + + test("accept applies via the seam: text replaced, Claude-attributed, proposal removed (INV-9/11/13)", async () => { + const doc = await openDoc(); + const api = await getApi(); + const id = api.proposalController.getRendered(DOC_REL)[0].id; + const ok = await api.proposalController.acceptById(DOC_REL, id); + assert.strictEqual(ok, true, "accept applies"); + await settle(); + assert.ok(doc.getText().includes(REPLACEMENT), "replacement landed"); + assert.ok(!doc.getText().includes(TARGET), "target gone"); + const spans = api.attributionController.getSpans(DOC_REL); + const agent = spans.find((s) => s.turnId === "turn-p1"); + assert.ok(agent, `accepted text is Claude-attributed with the proposal's turnId — got spans: ${JSON.stringify(spans)}`); + assert.strictEqual(agent!.authorKind, "agent"); + assert.strictEqual(api.proposalController.getRendered(DOC_REL).length, 0, "proposal gone (INV-13)"); + assert.strictEqual(readSidecar().proposals.length, 0, "removed from the sidecar"); + }); + + test("reject leaves the document untouched and removes the proposal (PUC-3)", async () => { + const doc = await openDoc(); + const api = await getApi(); + const before = doc.getText(); + const id = await proposeViaCommand(doc, "A second target for coexistence checks.", "WOULD-BE replacement.", "turn-p2"); + await settle(); + assert.strictEqual(api.proposalController.rejectById(DOC_REL, id), true); + await settle(); + assert.strictEqual(doc.getText(), before, "document untouched"); + assert.strictEqual(api.proposalController.getRendered(DOC_REL).length, 0); + assert.strictEqual(readSidecar().proposals.length, 0); + }); + + test("a pending proposal persists, survives reload, and re-anchors after an external move (PUC-4)", async () => { + let doc = await openDoc(); + const api = await getApi(); + const anchor = "A stable closing paragraph."; + await proposeViaCommand(doc, anchor, "A PROPOSED closing paragraph.", "turn-p3"); + await settle(); + const uri = vscode.Uri.file(path.join(WS, DOC_REL)); + doc = await externalWriteAndReload(uri, "PREPENDED LINE\n\n" + doc.getText()); + await settle(); + api.proposalController.renderAll(doc); + const rendered = api.proposalController.getRendered(DOC_REL); + assert.strictEqual(rendered.length, 1, "proposal survived reload"); + assert.strictEqual(rendered[0].pending, true, "still decidable"); + const moved = doc.getText().indexOf(anchor); + assert.strictEqual(rendered[0].range.start, moved, "re-anchored after the move"); + }); + + test("editing the target text makes the proposal stale: flagged, accept refused, doc untouched (INV-11)", async () => { + let doc = await openDoc(); + const api = await getApi(); + const id = api.proposalController.getRendered(DOC_REL)[0].id; + const mangled = doc.getText().replace("A stable closing paragraph.", "A reworded closing paragraph."); + doc = await externalWriteAndReload(vscode.Uri.file(path.join(WS, DOC_REL)), mangled); + await settle(); + api.proposalController.renderAll(doc); + assert.strictEqual(api.proposalController.getStaleCount(DOC_REL), 1, "flagged stale"); + const before = doc.getText(); + const ok = await api.proposalController.acceptById(DOC_REL, id); + assert.strictEqual(ok, false, "accept refused (INV-11)"); + assert.strictEqual(doc.getText(), before, "document untouched"); + assert.strictEqual(readSidecar().proposals.length, 1, "proposal still pending (recoverable)"); + // discard the husk so later tests see a clean sidecar + assert.strictEqual(api.proposalController.rejectById(DOC_REL, id), true); + }); + + test("multiple pending proposals coexist and are decidable out of order (PUC-5)", async () => { + const doc = await openDoc(); + const api = await getApi(); + const id1 = await proposeViaCommand(doc, "A stable opening paragraph.", "An ACCEPTED opening paragraph.", "turn-p4"); + const id2 = await proposeViaCommand(doc, "PROPOSED-BY-CLAUDE replacement sentence.", "A REPLACED-AGAIN sentence.", "turn-p5"); + await settle(); + assert.strictEqual(api.proposalController.getRendered(DOC_REL).length, 2); + // decide the SECOND first, then the first — order independence + assert.strictEqual(await api.proposalController.acceptById(DOC_REL, id2), true); + await settle(); + assert.strictEqual(await api.proposalController.acceptById(DOC_REL, id1), true); + await settle(); + assert.ok(doc.getText().includes("An ACCEPTED opening paragraph.")); + assert.ok(doc.getText().includes("A REPLACED-AGAIN sentence.")); + assert.strictEqual(api.proposalController.getRendered(DOC_REL).length, 0); + }); +}); diff --git a/test/pendingEdits.test.ts b/test/pendingEdits.test.ts index 81d241d..2df9372 100644 --- a/test/pendingEdits.test.ts +++ b/test/pendingEdits.test.ts @@ -8,26 +8,53 @@ const AGENT: Provenance = { }; describe("PendingEditRegistry (INV-9)", () => { - it("matches and consumes an exact registration", () => { + it("matches and consumes a single-hunk event equal to the registration", () => { const reg = new PendingEditRegistry(); - reg.register({ docPath: "d.md", start: 3, end: 7, newText: "new", provenance: AGENT, turnId: "t1" }); - const hit = reg.match("d.md", { start: 3, end: 7, text: "new" }); + reg.register({ + docPath: "d.md", start: 3, end: 7, newText: "new", provenance: AGENT, turnId: "t1", + full: { start: 3, end: 7, newLength: 3 }, + }); + const hit = reg.matchEvent("d.md", [{ start: 3, end: 7, newLength: 3 }]); expect(hit?.turnId).toBe("t1"); - expect(reg.match("d.md", { start: 3, end: 7, text: "new" })).toBeNull(); + expect(reg.matchEvent("d.md", [{ start: 3, end: 7, newLength: 3 }])).toBeNull(); }); - it("does not match a different doc, range, or text (human typing stays human)", () => { + it("matches a HOST-SPLIT event: several hunks inside the full range with the same net delta", () => { + // The host word-diffs one applied replace into multiple minimal hunks + // (observed on F4 accept) — per-hunk equality misses; net-effect matches. + const reg = new PendingEditRegistry(); + reg.register({ + docPath: "d.md", start: 10, end: 50, newText: "x".repeat(45), provenance: AGENT, turnId: "t2", + full: { start: 10, end: 50, newLength: 45 }, + }); + const hit = reg.matchEvent("d.md", [ + { start: 12, end: 20, newLength: 9 }, + { start: 25, end: 30, newLength: 6 }, + { start: 40, end: 48, newLength: 11 }, + ]); + expect(hit?.turnId).toBe("t2"); + }); + it("does not match a different doc, hunks outside the full range, or a different delta", () => { + const reg = new PendingEditRegistry(); + reg.register({ + docPath: "d.md", start: 3, end: 7, newText: "new", provenance: AGENT, + full: { start: 3, end: 7, newLength: 3 }, + }); + expect(reg.matchEvent("other.md", [{ start: 3, end: 7, newLength: 3 }])).toBeNull(); + expect(reg.matchEvent("d.md", [{ start: 3, end: 8, newLength: 4 }])).toBeNull(); + expect(reg.matchEvent("d.md", [{ start: 3, end: 7, newLength: 4 }])).toBeNull(); + expect(reg.matchEvent("d.md", [])).toBeNull(); + }); + it("falls back to the minimized range when no full extent was registered", () => { const reg = new PendingEditRegistry(); reg.register({ docPath: "d.md", start: 3, end: 7, newText: "new", provenance: AGENT }); - expect(reg.match("other.md", { start: 3, end: 7, text: "new" })).toBeNull(); - expect(reg.match("d.md", { start: 3, end: 8, text: "new" })).toBeNull(); - expect(reg.match("d.md", { start: 3, end: 7, text: "neww" })).toBeNull(); + expect(reg.matchEvent("d.md", [{ start: 3, end: 7, newLength: 3 }])).not.toBeNull(); }); it("unregister removes a failed application", () => { const reg = new PendingEditRegistry(); const p = { docPath: "d.md", start: 0, end: 0, newText: "x", provenance: AGENT }; reg.register(p); reg.unregister(p); - expect(reg.match("d.md", { start: 0, end: 0, text: "x" })).toBeNull(); + expect(reg.matchEvent("d.md", [{ start: 0, end: 0, newLength: 1 }])).toBeNull(); }); it("unregister reports whether the registration was still pending", () => { const reg = new PendingEditRegistry(); @@ -35,7 +62,7 @@ describe("PendingEditRegistry (INV-9)", () => { reg.register(p); expect(reg.unregister(p)).toBe(true); reg.register(p); - reg.match("d.md", { start: 0, end: 0, text: "x" }); + reg.matchEvent("d.md", [{ start: 0, end: 0, newLength: 1 }]); expect(reg.unregister(p)).toBe(false); }); });