diff --git a/src/proposalController.ts b/src/proposalController.ts index 1e66b42..ba9649c 100644 --- a/src/proposalController.ts +++ b/src/proposalController.ts @@ -315,6 +315,17 @@ export class ProposalController implements vscode.Disposable { const proposal = state.artifact.proposals.find((p) => p.id === proposalId); const fp = proposal ? state.artifact.anchors[proposal.anchorId]?.fingerprint : undefined; if (!proposal || !fp) return false; + // Reload-safety (INV-51/54): a proposal that already carries `original` was + // optimistically applied in a PRIOR session — the buffer holds the applied text + // and `fp` points at it, but this (fresh) controller's in-memory `applied` set is + // empty. Re-applying would recapture `original` from the already-applied buffer + // (= the replacement) and CLOBBER the true revert target, breaking Reject. Mark it + // applied in memory and stop — `original` is captured exactly once, on first apply. + if (proposal.original !== undefined) { + state.applied.add(proposalId); + this.renderAll(document); + return true; + } const resolved = resolve(document.getText(), fp); if (resolved === "orphaned") return false; const original = document.getText( diff --git a/src/trackChangesModel.ts b/src/trackChangesModel.ts index abec916..930772e 100644 --- a/src/trackChangesModel.ts +++ b/src/trackChangesModel.ts @@ -791,6 +791,26 @@ function renderReviewOp( * INV-34). Deterministic: proposals in the same block are ordered by anchorStart * then id; trailing proposals keep input order. */ +/** + * F12/#64 (INV-50): `currentText` with every resolved pending proposal's applied + * span reverted to its original (`replaced`) — the "landed" text the baseline diff + * should run against, so a pending proposal renders ONCE (as a proposal), never also + * as a landed change. Reverts high→low so earlier offsets stay valid. Pure. The + * preview's summary tally diffs against this too, so the toolbar count matches the + * body (a pending change is not double-counted as both a landed add/remove and a + * proposal). + */ +export function landedTextOf(currentText: string, proposals: ProposalView[]): string { + const pendingApplied = proposals + .filter((p) => p.anchorStart !== null) + .sort((a, b) => b.anchorStart! - a.anchorStart!); + let landedText = currentText; + for (const p of pendingApplied) { + landedText = landedText.slice(0, p.anchorStart!) + p.replaced + landedText.slice(p.anchorEnd!); + } + return landedText; +} + export function renderReview( baselineText: string, currentText: string, @@ -802,16 +822,12 @@ export function renderReview( // F12/#64 (INV-50): with optimistic apply the proposed text is already in // `currentText`, so a naive baseline→current diff would render each proposed - // change BOTH as a landed diff and as its proposal block. Diff against - // `currentText` with every resolved pending proposal reverted to its original, - // so the landed diff excludes pending proposals; they render once, as proposals. + // change BOTH as a landed diff and as its proposal block. Diff against the + // "landed" text (current minus pending proposals) so they render once. const pendingApplied = proposals .filter((p) => p.anchorStart !== null) .sort((a, b) => b.anchorStart! - a.anchorStart!); - let landedText = currentText; - for (const p of pendingApplied) { - landedText = landedText.slice(0, p.anchorStart!) + p.replaced + landedText.slice(p.anchorEnd!); - } + const landedText = landedTextOf(currentText, proposals); const ranges = splitBlocksWithRanges(landedText); const ops = diffBlocks(baselineText, landedText); diff --git a/src/trackChangesPreview.ts b/src/trackChangesPreview.ts index 42f8be0..bf8006d 100644 --- a/src/trackChangesPreview.ts +++ b/src/trackChangesPreview.ts @@ -13,7 +13,7 @@ import * as vscode from "vscode"; import type { DiffViewController } from "./diffViewController"; import type { AttributionController } from "./attributionController"; import type { ProposalController } from "./proposalController"; -import { renderReview, renderPlain, diffBlocks, diffToBlockHunks, type BlockOp } from "./trackChangesModel"; +import { renderReview, renderPlain, diffBlocks, diffToBlockHunks, landedTextOf, type BlockOp } from "./trackChangesModel"; import { buildFingerprint } from "./anchorer"; import { isAuthorable } from "./workspacePath"; import type { EditTurnResult, RunEditTurnOptions } from "./liveTurn"; @@ -382,9 +382,13 @@ export class TrackChangesPreviewController implements vscode.Disposable { } const spans = this.attribution.spansFor(document); const proposals = this.proposals.listProposals(document); + // F12/#64 (INV-50): count added/removed against the LANDED text (current minus + // pending proposals), matching the body — a pending change shows once, as a + // proposal, and is not also tallied as a landed add/remove. + const landedOps = diffBlocks(baselineText, landedTextOf(current, proposals)); const summary = { - added: ops.filter((o) => o.kind === "added").length, - removed: ops.filter((o) => o.kind === "removed").length, + added: landedOps.filter((o) => o.kind === "added").length, + removed: landedOps.filter((o) => o.kind === "removed").length, proposals: proposals.length, }; void panel.webview.postMessage({ diff --git a/test/e2e/suite/f12InlineDiff.test.ts b/test/e2e/suite/f12InlineDiff.test.ts index 711baf2..3fe8599 100644 --- a/test/e2e/suite/f12InlineDiff.test.ts +++ b/test/e2e/suite/f12InlineDiff.test.ts @@ -3,6 +3,8 @@ import * as fs from "fs"; import * as path from "path"; import * as vscode from "vscode"; import type { CowritingApi } from "../../../src/extension"; +import { addProposal, setProposalApplied } from "../../../src/proposalModel"; +import { buildFingerprint } from "../../../src/anchorer"; const WS = process.env.E2E_WORKSPACE!; const settle = () => new Promise((r) => setTimeout(r, 400)); @@ -197,3 +199,53 @@ suite("F12 inline diff — control parity (#64, INV-53)", () => { assert.ok(entry && /editorLangId == markdown/.test(entry.when ?? ""), "palette-guarded on markdown"); }); }); + +// Reload-safety: a proposal that was optimistically applied in a PRIOR session +// persists with `original` set and its fp re-anchored to the applied text. A fresh +// controller (empty in-memory `applied` set) must NOT re-capture `original` from the +// already-applied buffer — doing so would clobber the true revert target and break +// Reject. (Regression for the final-review CRITICAL; INV-51/54.) +suite("F12 inline diff — reload-safety (#64, INV-51/54)", () => { + test("optimisticApply does not clobber a previously-persisted original", async () => { + const TRUE_ORIGINAL = "The original sentence here."; + const APPLIED = "The APPLIED sentence here."; + const { doc } = await freshDoc("docs/f12-reload.md", `# R\n\n${TRUE_ORIGINAL}\n`); + const api = await getApi(); + const p = api.proposalController; + const key = p.keyFor(doc); + + // 1) The buffer holds the APPLIED text (as the saved-while-pending file would). + const at = doc.getText().indexOf(TRUE_ORIGINAL); + const we = new vscode.WorkspaceEdit(); + we.replace(doc.uri, new vscode.Range(doc.positionAt(at), doc.positionAt(at + TRUE_ORIGINAL.length)), APPLIED); + assert.ok(await vscode.workspace.applyEdit(we), "apply the prior-session applied text"); + await settle(); + + // 2) Record the proposal directly in the sidecar exactly as a prior session left + // it: fp anchored to the APPLIED text + `original` = the TRUE original. We do + // NOT call optimisticApply, so this controller's in-memory `applied` stays empty. + const appliedAt = doc.getText().indexOf(APPLIED); + const appliedFp = buildFingerprint(doc.getText(), { start: appliedAt, end: appliedAt + APPLIED.length }); + let id = ""; + api.sidecarRouter.update(key, (a) => { + id = addProposal(a, appliedFp, APPLIED, { kind: "agent", id: "claude", agent: { sdk: "x", model: "m", sessionId: "s" } }, { granularity: "block" }).proposalId; + setProposalApplied(a, id, appliedFp, TRUE_ORIGINAL); + }); + p.renderAll(doc); + await settle(); + + // 3) Re-entry as a reload would trigger (EditorProposalController re-applies on + // onDidChangeProposals because `applied` is empty). The guard must preserve original. + await p.optimisticApply(doc, id); + await settle(); + const view = p.listProposals(doc).find((v) => v.id === id); + assert.ok(view, "proposal still present"); + assert.strictEqual(view!.original, TRUE_ORIGINAL, "true original preserved, NOT clobbered with the applied text"); + + // 4) Reject restores the TRUE original (not the applied text). + assert.ok(await p.revertInPlace(key, id), "revert"); + await settle(); + assert.ok(doc.getText().includes(TRUE_ORIGINAL), "reject restored the true original"); + assert.ok(!doc.getText().includes(APPLIED), "applied text removed on reject"); + }); +});