fix(#70): harden the tracked-span revert per branch review
- shiftTracked (pure, anchorer.ts + unit tests): interior/outside edits shift, boundary-straddling edits distrust; an insertion exactly at the span start lands BEFORE the span (never absorbed into the revert target) - clear tracked spans on doc close (a closed buffer can change on disk with no change events — a kept span could revert over unrelated text, INV-11) - renderAll only REBUILDS an absent span from an exact resolve; a present entry is continuously-tracked ground truth (duplicate-text resolve can't clobber it) - revertInPlace/finalizeInPlace gain the guard.isReadOnly check (INV-16) - the hard-fail warning offers 'Discard proposal (leave text)' so a reloaded session can still dismiss an unlocatable proposal record - ✓/✗ CodeLens pair anchors at the tracked span when a tweak orphans the exact anchor (the fixed reject path stays reachable from the primary gesture) - QuickPick batch menu routes through the reporting accept-all/reject-all commands (a discarded skip tally re-created the silent failure) - one clearProposal helper replaces the thrice-copied clear sequence; reject() now cleans applied/appliedSpans too Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -101,6 +101,30 @@ export function resolve(docText: string, fp: Fingerprint): OffsetRange | "orphan
|
||||
return "orphaned";
|
||||
}
|
||||
|
||||
/**
|
||||
* #70 (INV-5/INV-11): maintain a TRACKED applied span across an in-session edit.
|
||||
* Unlike `shift`, which clamps every point into a best-effort range, a tracked
|
||||
* span must stay *provably* meaningful — it is a revert target. So:
|
||||
* - an edit fully OUTSIDE or fully INSIDE the span shifts it (the span is
|
||||
* still "the applied block, as tweaked");
|
||||
* - an insertion exactly AT the span start lands BEFORE the span (both ends
|
||||
* shift right) — matching exact-resolve semantics, where text typed just
|
||||
* before the applied block is never part of it;
|
||||
* - any edit STRADDLING a span boundary returns "distrusted": the shifted
|
||||
* result would be a clamped guess, and stale text is never reverted by
|
||||
* guess (INV-11).
|
||||
*/
|
||||
export function shiftTracked(range: OffsetRange, edit: TextEdit): OffsetRange | "distrusted" {
|
||||
const delta = edit.newLength - (edit.end - edit.start);
|
||||
if (edit.start === edit.end && edit.start === range.start) {
|
||||
return { start: range.start + delta, end: range.end + delta };
|
||||
}
|
||||
const outside = edit.end <= range.start || edit.start >= range.end;
|
||||
const inside = edit.start >= range.start && edit.end <= range.end;
|
||||
if (!outside && !inside) return "distrusted";
|
||||
return shift(range, edit);
|
||||
}
|
||||
|
||||
/** Maintain a live range across an in-session edit (spec §6.4 `shift`). */
|
||||
export function shift(range: OffsetRange, edit: TextEdit): OffsetRange {
|
||||
const delta = edit.newLength - (edit.end - edit.start);
|
||||
|
||||
@@ -226,9 +226,16 @@ export class EditorProposalController implements vscode.Disposable, vscode.CodeL
|
||||
if (document.languageId !== "markdown") return [];
|
||||
if (!this.registry.isCoediting(document.uri)) return [];
|
||||
const key = this.proposals.keyFor(document);
|
||||
const applied = this.proposals
|
||||
.listProposals(document)
|
||||
.filter((v) => v.anchorStart !== null && this.proposals.isApplied(key, v.id));
|
||||
// #70: an interior tweak orphans the exact anchor (anchorStart null) but the
|
||||
// proposal stays fully decidable — Keep by id, Reject via the tracked span —
|
||||
// so the lens pair anchors at the tracked span instead of vanishing (which
|
||||
// made the fixed reject path unreachable from the primary gesture surface).
|
||||
const applied: { id: string; at: number }[] = [];
|
||||
for (const v of this.proposals.listProposals(document)) {
|
||||
if (!this.proposals.isApplied(key, v.id)) continue;
|
||||
const at = v.anchorStart ?? this.proposals.trackedSpan(key, v.id)?.start;
|
||||
if (at !== undefined) applied.push({ id: v.id, at });
|
||||
}
|
||||
const lenses: vscode.CodeLens[] = [];
|
||||
if (applied.length >= 2) {
|
||||
const top = new vscode.Range(0, 0, 0, 0);
|
||||
@@ -238,7 +245,7 @@ export class EditorProposalController implements vscode.Disposable, vscode.CodeL
|
||||
);
|
||||
}
|
||||
for (const v of applied) {
|
||||
const pos = document.positionAt(v.anchorStart!);
|
||||
const pos = document.positionAt(v.at);
|
||||
const line = new vscode.Range(pos.line, 0, pos.line, 0);
|
||||
lenses.push(
|
||||
new vscode.CodeLens(line, { title: "✓ Keep", command: "cowriting.proposalAcceptMenu", arguments: [v.id] }),
|
||||
@@ -261,10 +268,12 @@ export class EditorProposalController implements vscode.Disposable, vscode.CodeL
|
||||
if (!pick) return;
|
||||
const all = pick.includes("ALL");
|
||||
if (kind === "accept") {
|
||||
if (all) await this.proposals.acceptAllProposals(doc);
|
||||
// The batch commands own the applied/skipped reporting (#70: a silent:true
|
||||
// batch with a discarded tally re-creates the silent-skip failure mode).
|
||||
if (all) await vscode.commands.executeCommand("cowriting.acceptAllProposals");
|
||||
else await this.proposals.finalizeInPlace(key, id);
|
||||
} else {
|
||||
if (all) await this.proposals.rejectAll(doc);
|
||||
if (all) await vscode.commands.executeCommand("cowriting.rejectAllProposals");
|
||||
else await this.proposals.revertInPlace(key, id);
|
||||
}
|
||||
}
|
||||
|
||||
+53
-26
@@ -13,7 +13,7 @@
|
||||
import * as vscode from "vscode";
|
||||
import { SidecarRouter, docIdentity } from "./sidecarRouter";
|
||||
import { emptyArtifact, type Artifact, type Fingerprint, type Proposal, type Provenance } from "./model";
|
||||
import { resolve, shift, buildFingerprint, type OffsetRange } from "./anchorer";
|
||||
import { resolve, shift, shiftTracked, buildFingerprint, type OffsetRange } from "./anchorer";
|
||||
import { addProposal, removeProposal, setProposalApplied } from "./proposalModel";
|
||||
import type { AttributionController } from "./attributionController";
|
||||
import type { VersionGuard } from "./versionGuard";
|
||||
@@ -73,6 +73,13 @@ export class ProposalController implements vscode.Disposable {
|
||||
this.disposables.push(this.onDidChangeProposalsEmitter);
|
||||
this.disposables.push(
|
||||
vscode.workspace.onDidChangeTextDocument((e) => this.onDidChange(e)),
|
||||
// #70: a tracked applied span is only meaningful while its buffer is live —
|
||||
// a closed doc can change on disk with no change events, so a span kept
|
||||
// across close/reopen could revert over unrelated text (INV-11: never by
|
||||
// guess). Cleared here; a resolving anchor rebuilds it at the next renderAll.
|
||||
vscode.workspace.onDidCloseTextDocument((doc) => {
|
||||
this.docs.get(this.keyOf(doc))?.appliedSpans.clear();
|
||||
}),
|
||||
// PUC-7 restore-on-enter (mirrors ThreadController/AttributionController):
|
||||
// `renderAll` is the only path that recomputes state.live/unresolved AND
|
||||
// fires onDidChangeProposals — the event EditorProposalController listens
|
||||
@@ -335,6 +342,22 @@ export class ProposalController implements vscode.Disposable {
|
||||
return this.docs.get(docPath)?.applied.has(proposalId) ?? false;
|
||||
}
|
||||
|
||||
/** #70: the shift-tracked applied span (the revert-fallback bookkeeping) —
|
||||
* exposed so the editor surface can keep the decision gestures reachable
|
||||
* for an applied proposal whose exact anchor an interior tweak orphaned. */
|
||||
trackedSpan(docPath: string, proposalId: string): OffsetRange | undefined {
|
||||
return this.docs.get(docPath)?.appliedSpans.get(proposalId);
|
||||
}
|
||||
|
||||
/** The one proposal-clear sequence (record + in-memory bookkeeping + re-render)
|
||||
* shared by finalize/revert/reject so no path leaves a stale entry behind. */
|
||||
private clearProposal(state: DocState, proposalId: string, document?: vscode.TextDocument): void {
|
||||
this.store.update(state.docPath, (a) => removeProposal(a, proposalId));
|
||||
state.applied.delete(proposalId);
|
||||
state.appliedSpans.delete(proposalId);
|
||||
if (document) this.renderAll(document);
|
||||
}
|
||||
|
||||
/**
|
||||
* F12/#64 (INV-48): optimistically apply a pending proposal INTO the buffer so
|
||||
* the editor shows the would-be-accepted result (editable). Reuses the F4
|
||||
@@ -427,15 +450,13 @@ export class ProposalController implements vscode.Disposable {
|
||||
* purposes anymore (the `onDidApplyAgentEdit → advance` wire was removed).
|
||||
*/
|
||||
async finalizeInPlace(docPath: string, proposalId: string): Promise<boolean> {
|
||||
if (this.guard.isReadOnly(docPath)) return false;
|
||||
const hit = this.byId(docPath, proposalId);
|
||||
if (!hit) return false;
|
||||
const document = this.openDoc(hit.state);
|
||||
if (!document) return false;
|
||||
this.attribution.signalLanded(document);
|
||||
this.store.update(docPath, (a) => removeProposal(a, proposalId));
|
||||
hit.state.applied.delete(proposalId);
|
||||
hit.state.appliedSpans.delete(proposalId);
|
||||
this.renderAll(document);
|
||||
this.clearProposal(hit.state, proposalId, document);
|
||||
return true;
|
||||
}
|
||||
|
||||
@@ -450,6 +471,7 @@ export class ProposalController implements vscode.Disposable {
|
||||
* leaving the proposed text in the buffer. Undo (or ✓ Keep) remains the way out.
|
||||
*/
|
||||
async revertInPlace(docPath: string, proposalId: string, opts?: { silent?: boolean }): Promise<boolean> {
|
||||
if (this.guard.isReadOnly(docPath)) return false;
|
||||
const hit = this.byId(docPath, proposalId);
|
||||
if (!hit) return false;
|
||||
const document = this.openDoc(hit.state);
|
||||
@@ -457,10 +479,7 @@ export class ProposalController implements vscode.Disposable {
|
||||
// Never optimistically applied → nothing of ours is in the buffer; clearing
|
||||
// the record IS the reject (the legacy pending-only path).
|
||||
if (hit.proposal.original === undefined) {
|
||||
this.store.update(docPath, (a) => removeProposal(a, proposalId));
|
||||
hit.state.applied.delete(proposalId);
|
||||
hit.state.appliedSpans.delete(proposalId);
|
||||
this.renderAll(document);
|
||||
this.clearProposal(hit.state, proposalId, document);
|
||||
return true;
|
||||
}
|
||||
const fp = hit.state.artifact.anchors[hit.proposal.anchorId]?.fingerprint;
|
||||
@@ -468,9 +487,18 @@ export class ProposalController implements vscode.Disposable {
|
||||
const span = resolved !== "orphaned" ? resolved : hit.state.appliedSpans.get(proposalId);
|
||||
if (!span) {
|
||||
if (!opts?.silent) {
|
||||
void vscode.window.showWarningMessage(
|
||||
"Cowriting: this proposal's applied text can't be located (it changed or moved) — undo your edits, or Keep it to leave the buffer as-is (it is never reverted by guess).",
|
||||
);
|
||||
// The revert is refused, but the record must stay dismissable (a window
|
||||
// reload empties both the tracked spans and the undo stack): offer a
|
||||
// record-only discard that leaves the buffer exactly as it stands.
|
||||
const DISCARD = "Discard proposal (leave text)";
|
||||
void vscode.window
|
||||
.showWarningMessage(
|
||||
"Cowriting: this proposal's applied text can't be located (it changed or moved) — it is never reverted by guess. Undo your edits, Keep it, or discard the record as-is.",
|
||||
DISCARD,
|
||||
)
|
||||
.then((choice) => {
|
||||
if (choice === DISCARD) this.clearProposal(hit.state, proposalId, document);
|
||||
});
|
||||
}
|
||||
return false;
|
||||
}
|
||||
@@ -481,10 +509,7 @@ export class ProposalController implements vscode.Disposable {
|
||||
hit.proposal.original,
|
||||
);
|
||||
if (!(await vscode.workspace.applyEdit(we))) return false;
|
||||
this.store.update(docPath, (a) => removeProposal(a, proposalId));
|
||||
hit.state.applied.delete(proposalId);
|
||||
hit.state.appliedSpans.delete(proposalId);
|
||||
this.renderAll(document);
|
||||
this.clearProposal(hit.state, proposalId, document);
|
||||
return true;
|
||||
}
|
||||
|
||||
@@ -521,9 +546,7 @@ export class ProposalController implements vscode.Disposable {
|
||||
|
||||
private reject(state: DocState, proposal: Proposal): void {
|
||||
if (this.guard.isReadOnly(state.docPath)) return;
|
||||
this.store.update(state.docPath, (a) => removeProposal(a, proposal.id));
|
||||
const document = this.openDoc(state);
|
||||
if (document) this.renderAll(document);
|
||||
this.clearProposal(state, proposal.id, this.openDoc(state));
|
||||
}
|
||||
|
||||
// ---- PUC-4: load / external change / resolve-or-flag --------------------------------
|
||||
@@ -553,9 +576,14 @@ export class ProposalController implements vscode.Disposable {
|
||||
this.recordProposal(state, proposal, { start: off, end: off }, false);
|
||||
} else {
|
||||
this.recordProposal(state, proposal, resolved, true);
|
||||
// #70: an exact resolve is ground truth — (re)sync the applied-range
|
||||
// bookkeeping (covers a prior-session apply, whose in-memory map is empty).
|
||||
if (state.applied.has(proposal.id)) state.appliedSpans.set(proposal.id, resolved);
|
||||
// #70: REBUILD the applied-range bookkeeping from an exact resolve when
|
||||
// the map has no entry (a prior-session apply, or after a close/reopen
|
||||
// clear). An entry already present has been shift-maintained continuously
|
||||
// since apply and is ground truth — never clobbered by a resolve, which
|
||||
// can land on an exact duplicate of the applied text elsewhere.
|
||||
if (state.applied.has(proposal.id) && !state.appliedSpans.has(proposal.id)) {
|
||||
state.appliedSpans.set(proposal.id, resolved);
|
||||
}
|
||||
}
|
||||
}
|
||||
this.renderStatus(state);
|
||||
@@ -583,10 +611,9 @@ export class ProposalController implements vscode.Disposable {
|
||||
// external replace — makes the shifted range a clamped guess, so the span
|
||||
// is distrusted (deleted) rather than reverted-to by guess (INV-11).
|
||||
for (const [id, range] of state.appliedSpans) {
|
||||
const outside = edit.end <= range.start || edit.start >= range.end;
|
||||
const inside = edit.start >= range.start && edit.end <= range.end;
|
||||
if (outside || inside) state.appliedSpans.set(id, shift(range, edit));
|
||||
else state.appliedSpans.delete(id);
|
||||
const shifted = shiftTracked(range, edit);
|
||||
if (shifted === "distrusted") state.appliedSpans.delete(id);
|
||||
else state.appliedSpans.set(id, shifted);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user