fix(f11): address code review — insertion anchoring, turnId, accept-all coverage (#43)

Code-review follow-up on the F11 branch (5966907..1564ef5). Three real findings:

- CRITICAL — pure-insertion hunks were born-orphaned. A document rewrite that
  INSERTS text produced a zero-width hunk (start==end) → buildFingerprint yields
  empty fp.text → anchorer.resolve orphans an empty needle → the proposal could
  never be accepted (silently). diffToHunks now anchors every zero-width
  insertion to an adjacent source token (anchorInsertion): the range absorbs the
  token and the replacement keeps it, so net text is identical but fp.text is
  non-empty and resolvable. New unit tests: applying hunks always reconstructs
  the rewrite (substitute/delete/insert/multi); insertions are never zero-width.
  New host E2E ACCEPTS a rewrite-with-insertion end to end and asserts accept-all
  reconstructs the intended document (also covers sequential multi-hunk accept).

- IMPORTANT — renderPlain cross-block fidelity. The per-block off-mode rendering
  (SLICE-2) can't resolve a reference-link definition in a separate block.
  Documented as a conscious tradeoff of the operator-locked block-level mapping
  (§6.7) — renderReview already rendered per-block, so this keeps both modes
  consistent — with a characterization test + a docstring note.

- MINOR — F11 proposals now carry a turnId (one per Ask-Claude gesture, shared
  across a document rewrite's N hunks), matching the editor-menu editSelection
  path so a rewrite groups as one agent turn.

208 unit + 9/9 F11 host E2E green. (The lone red E2E is a pre-existing,
F11-independent undoMarks timing flake — proven by isolation: it fails
identically with all F11 tests removed.)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ben Stull
2026-06-12 14:16:49 -07:00
parent 1564ef562b
commit 47cc733026
4 changed files with 138 additions and 3 deletions
+27
View File
@@ -144,6 +144,33 @@ suite("F11 preview toolbar (host E2E — message → seam wiring, no LLM)", () =
assert.strictEqual(ids.length, 0, "an unchanged replacement proposes nothing");
});
// SLICE-3 (review follow-up): a document rewrite that INSERTS text → an
// acceptable proposal (not a born-orphaned zero-width hunk), and accepting all
// hunks of a multi-hunk rewrite lands the intended document.
test("document rewrite with an insertion → acceptable proposals; accept-all reaches the rewrite", async () => {
const original = "# F11 accept\n\nThe brown fox sleeps.\n";
const rewrite = "# F11 accept\n\nThe brown fox QUIETLY sleeps today.\n";
const { doc } = await freshDoc("docs/f11accept.md", original);
const api = await getApi();
const ctl = api.trackChangesPreviewController;
await vscode.commands.executeCommand("cowriting.showTrackChangesPreview");
await settle();
ctl.setEditTurnForTest(async () => ({ replacement: rewrite, model: "sonnet", sessionId: "e2e-accept" }));
const ids = await ctl.runEditAndPropose(doc, { kind: "document" }, "expand the sentence");
await settle();
assert.ok(ids.length >= 1, "the rewrite produced at least one proposal");
// Every proposal must be acceptable (the insertion-anchoring fix): accept each.
for (const id of ids) {
const ok = await api.proposalController.acceptById("docs/f11accept.md", id);
assert.ok(ok, `proposal ${id} is acceptable (not born-orphaned)`);
await settle();
}
assert.strictEqual(doc.getText(), rewrite, "accepting all hunks reconstructs the intended rewrite");
assert.strictEqual(api.proposalController.listProposals(doc).length, 0, "no proposals left pending");
});
// SLICE-3: the document-scoped command exists for #42 reuse, guarded on markdown.
test("cowriting.editDocument is a registered command, palette-guarded on markdown", async () => {
const all = await vscode.commands.getCommands(true);
+59
View File
@@ -360,6 +360,46 @@ describe("F11 diffToHunks (INV-37)", () => {
const b = diffToHunks("a b c d", "a B c D");
expect(a).toEqual(b);
});
/** Apply hunks (right→left so earlier offsets stay valid) to reconstruct the rewrite. */
const applyHunks = (current: string, hunks: ReturnType<typeof diffToHunks>): string => {
let out = current;
for (const h of [...hunks].sort((a, b) => b.start - a.start)) {
out = out.slice(0, h.start) + h.replacement + out.slice(h.end);
}
return out;
};
test("applying the hunks always reconstructs the rewrite (substitute / delete / insert / multi)", () => {
const cases: Array<[string, string]> = [
["The quick brown fox.", "The quick red fox."],
["one two three four", "one TWO three FOUR"],
["alpha", "omega"],
["keep this and drop that", "keep this"],
["one two three", "one INSERTED two three"],
["start middle end", "PREFIX start middle end SUFFIX"],
["unchanged body", "unchanged body"],
];
for (const [current, rewrite] of cases) {
expect(applyHunks(current, diffToHunks(current, rewrite))).toBe(rewrite);
}
});
test("an inserted run anchors to adjacent text — never a zero-width, unacceptable hunk (INV-37)", () => {
// A pure insertion would otherwise produce start==end → an empty fingerprint
// → resolve() orphans it → the proposal can never be accepted. Each hunk must
// span real source text so its F4 fingerprint resolves.
for (const [current, rewrite] of [
["one two three", "one INSERTED two three"],
["tail anchor", "tail anchor APPENDED"],
["lead", "PREPENDED lead"],
] as Array<[string, string]>) {
for (const h of diffToHunks(current, rewrite)) {
expect(h.end).toBeGreaterThan(h.start); // non-zero-width
expect(current.slice(h.start, h.end).length).toBeGreaterThan(0); // real fp.text
}
}
});
});
// F11 SLICE-2 (INV-36): the pure render layer emits data-src-start/data-src-end
@@ -408,4 +448,23 @@ describe("F11 data-src emission (INV-36)", () => {
const r2 = renderReview("x", "x\n\ny", [], []);
expect(r1).toBe(r2);
});
// CHARACTERIZATION (conscious tradeoff of the locked block-level mapping, §6.7
// fork 1): both modes render markdown PER BLOCK so each block can carry its
// data-src offsets. A consequence is that markdown constructs whose parts span
// blank-line-separated blocks — a reference-link USE and its DEFINITION — do not
// resolve across blocks (markdown-it sees each block in isolation). renderReview
// already had this; F11 brings the off/clean preview into line with it (both
// per-block) rather than leaving the two modes rendering differently. If
// cross-block fidelity is wanted later, it is a follow-up (source-map driven
// wrapping), not a change to the locked block-level decision.
test("a reference-link definition in a separate block does not resolve (block-level rendering)", () => {
const doc = "See [the spec][ref] for details.\n\n[ref]: https://example.com/spec\n";
const html = renderPlain(doc);
// the link is NOT resolved to an <a href> — the [text][ref] is rendered literally.
expect(html).not.toContain('href="https://example.com/spec"');
expect(html).toContain("[the spec][ref]");
// both modes agree (renderReview is likewise per-block) — the consistency point.
expect(renderReview(doc, doc, [], [])).not.toContain('href="https://example.com/spec"');
});
});