From c9975ba9e67c01c8190c2ed38f8023304fb294e8 Mon Sep 17 00:00:00 2001 From: BenStullsBets Date: Thu, 2 Jul 2026 09:04:32 -0700 Subject: [PATCH] fix: route document asks to comment-first askClaude (plan T6 Step 4); machine-author guard + offer cleanup Co-Authored-By: Claude Fable 5 --- src/editFlow.ts | 57 ++++++++++----------- src/extension.ts | 36 +++++++++++++- src/threadController.ts | 32 +++++++++--- test/e2e/suite/commentLoop.test.ts | 43 ++++++++++++++++ test/e2e/suite/f12Reach.test.ts | 79 +++++++++++++++++++----------- 5 files changed, 177 insertions(+), 70 deletions(-) diff --git a/src/editFlow.ts b/src/editFlow.ts index 1cf440d..9c18d29 100644 --- a/src/editFlow.ts +++ b/src/editFlow.ts @@ -1,13 +1,15 @@ /** * EditFlow — the host edit-turn flow (F11/F12), extracted from the review * webview controller ahead of its sunset (native-surfaces migration, spec - * §6.10). Owns the `cowriting.editDocument` command, the instruction prompt + - * progress-wrapped "ask Claude" gesture (INV-10 gate, INV-8 host-only LLM - * surface), and the low-level turn→proposal(s) cut (`runEditAndPropose`, - * INV-39/40). Never mutates the document directly — every turn lands as one or - * more F4 proposals via `ProposalController.propose` (INV-10). Reachable from - * both the review webview (delegates here) and, from Task 6 on, the thread - * controller — vscode-API-only, no webview state. + * §6.10). Owns the instruction prompt + progress-wrapped "ask Claude" gesture + * (INV-10 gate, INV-8 host-only LLM surface), and the low-level turn→proposal(s) + * cut (`runEditAndPropose`, INV-39/40). Never mutates the document directly — + * every turn lands as one or more F4 proposals via `ProposalController.propose` + * (INV-10). `runEditAndPropose` is reachable from the review webview (via this + * class's `askClaude`) and, from Task 6 on, the thread controller + * (`ThreadController.runMakeEdit`) — vscode-API-only, no webview state. The + * `cowriting.editDocument` command lives in extension.ts (Finding-1 fix) so it + * can hand off to `ThreadController.askClaude()`. */ import * as vscode from "vscode"; import type { ProposalController } from "./proposalController"; @@ -67,38 +69,29 @@ export class EditFlow implements vscode.Disposable { private readonly liveProgressUi: LiveProgressUi, private readonly registry: CoeditingRegistry, ) { - this.disposables.push( - // F11: document-scoped Ask-Claude (also reused by #42's reach gateways). - // Edits a markdown doc; the rewrite is diffed into F4 proposals. - // #42 (INV-38): the editor/title/context (tab) entry passes the clicked - // tab's resource Uri — target THAT document, opening it if it isn't already - // an open buffer (mirrors showTrackChangesPreview's #41 resolution); the - // palette / keybinding / editor/context pass nothing → the active editor. - vscode.commands.registerCommand("cowriting.editDocument", async (uri?: vscode.Uri) => { - const doc = uri - ? vscode.workspace.textDocuments.find((d) => d.uri.toString() === uri.toString()) ?? - (await vscode.workspace.openTextDocument(uri)) - : vscode.window.activeTextEditor?.document; - if (!doc || doc.languageId !== "markdown") { - void vscode.window.showWarningMessage("Cowriting: open a Markdown document to ask Claude to edit it."); - return; - } - void this.askClaude(doc, { kind: "document" }); - }), - ); + // Code-review fix (Finding 1, session native-surfaces-exec): the + // `cowriting.editDocument` command used to be registered here, calling + // `this.askClaude(doc, {kind:"document"})` — which prompts via the (now + // rejecting-stub) `askEditInstruction`, a silent dead end. That command is now + // registered in extension.ts, AFTER ThreadController exists, and hands off to + // `ThreadController.askClaude()` (the comments-first ask, D19) instead. This + // class's `askClaude`/`askEditInstruction` remain: the review webview's + // toolbar ask (`trackChangesPreview.ts`) still delegates to them, slated for + // deletion alongside that webview in Task 8. } /** * F11 (PUC-3/4): prompt host-side for the instruction (keeps the LLM/secret * surface out of the sealed webview, INV-8/35), run the edit turn, and surface - * the result as F4 proposal(s). UI wrapper around `runEditAndPropose`. Called - * both by the `cowriting.editDocument` command above and (via delegation) by - * the review webview's `askClaude` toolbar intent (either scope). + * the result as F4 proposal(s). UI wrapper around `runEditAndPropose`. As of the + * Finding-1 fix, its only production caller is the review webview's `askClaude` + * toolbar intent (either scope) — `cowriting.editDocument`/`cowriting.editSelection` + * now delegate to `ThreadController.askClaude()` instead (D19). Both die together + * in Task 8 when the webview is sunset. */ async askClaude(document: vscode.TextDocument, target: EditTarget): Promise { - // INV-10: the choke point for BOTH the editDocument command and the webview's - // askClaude toolbar intent (either scope) — a non-entered doc gets the warning, - // not a turn. + // INV-10: the choke point for the webview's askClaude toolbar intent (either + // scope) — a non-entered doc gets the warning, not a turn. if (!this.registry.isCoediting(document.uri)) { void vscode.window.showWarningMessage(NOT_COEDITING_WARNING); return; diff --git a/src/extension.ts b/src/extension.ts index d39c3a7..18883bf 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -323,11 +323,43 @@ export function activate(context: vscode.ExtensionContext): CowritingApi | undef }), ); + // Code-review fix (Finding 1, session native-surfaces-exec): `cowriting.editDocument` + // used to delegate to `EditFlow.askClaude(doc, {kind:"document"})`, which prompts via + // `askEditInstruction` — a rejecting stub since Task 6 sunset the input webview + // (spec §6.10). Reached with no selection (the common case for `cowriting.edit`), + // that was a SILENT dead end: the reject fired before EditFlow.askClaude's own try/ + // catch and was `void`'d, so no toast, no thread, nothing. This command now resolves + // its target document exactly as before (a tab's clicked URI, falling back to the + // active editor), focuses it, and hands off to the SAME comments-first ask as + // `cowriting.editSelection` — `ThreadController.askClaude()` top-anchors a + // whole-document ask when the (now-focused) editor's selection is empty (D19). Kept + // registered + hidden from the palette for the host E2E harness and #42's + // tab-targeting behavior; `EditFlow.runEditAndPropose` (untouched) stays the E2E seam + // for driving a turn without the native comment UI — see f12Reach.test.ts / + // f11Toolbar.test.ts / f12Accept.test.ts / f12Review.test.ts. + context.subscriptions.push( + vscode.commands.registerCommand("cowriting.editDocument", async (uri?: vscode.Uri) => { + const doc = uri + ? vscode.workspace.textDocuments.find((d) => d.uri.toString() === uri.toString()) ?? + (await vscode.workspace.openTextDocument(uri)) + : vscode.window.activeTextEditor?.document; + if (!doc || doc.languageId !== "markdown") { + void vscode.window.showWarningMessage("Cowriting: open a Markdown document to ask Claude to edit it."); + return; + } + if (vscode.window.activeTextEditor?.document.uri.toString() !== doc.uri.toString()) { + await vscode.window.showTextDocument(doc); + } + await threadController.askClaude(); + }), + ); + // The single user-facing "Ask Claude to Edit" gesture (one command, one // keybinding, one menu entry). It routes to the selection flow (editSelection) // or the whole-document flow (editDocument) by selection/context — see - // routeEdit. The two underlying commands stay registered for the host E2E - // harness and the internal seams, but are hidden from the palette. + // routeEdit. Both underlying commands now end at ThreadController.askClaude() + // (comments-first ask, D19); they stay registered for the host E2E harness and + // the internal seams, but are hidden from the palette. context.subscriptions.push( vscode.commands.registerCommand("cowriting.edit", async (uri?: vscode.Uri) => { const editor = vscode.window.activeTextEditor; diff --git a/src/threadController.ts b/src/threadController.ts index a329bbc..8e311e4 100644 --- a/src/threadController.ts +++ b/src/threadController.ts @@ -192,12 +192,17 @@ export class ThreadController implements vscode.Disposable { end: document.offsetAt(editor.selection.end), }; const fp = buildFingerprint(document.getText(), offsets); - const { threadId } = addThread(state.artifact, fp, { author: this.currentAuthor(), body: firstBody }); + const author = this.currentAuthor(); + const { threadId } = addThread(state.artifact, fp, { author, body: firstBody }); this.persist(state); const vsThread = this.renderThread(document, state, threadId, offsets, false); // D10: the first message of a new thread is also "a comment on a coedited - // doc" — run the same respond loop as a reply. - void this.respondInThread(vsThread, state, threadId, firstBody); + // doc" — run the same respond loop as a reply. Finding 2 guard: only for a + // human-authored message (never re-trigger the loop off the machine's own + // words — see reply() below for the matching guard). + if (author.kind !== "agent") { + void this.respondInThread(vsThread, state, threadId, firstBody); + } return threadId; } @@ -210,9 +215,10 @@ export class ThreadController implements vscode.Disposable { if (!document) return; const state = this.ensureState(document); if (this.guard.isReadOnly(state.docPath)) return; + const author = this.currentAuthor(); let threadId = this.threadIdOf(vsThread); if (threadId) { - appendMessage(state.artifact, threadId, { author: this.currentAuthor(), body: r.text }); + appendMessage(state.artifact, threadId, { author, body: r.text }); } else { // D10/D19: a brand-new thread created via the native "+"/comment-widget // gesture (not via createThreadOnSelection) — VS Code hands us a real, @@ -223,7 +229,7 @@ export class ThreadController implements vscode.Disposable { end: document.offsetAt(range.end), }; const fp = buildFingerprint(document.getText(), offsets); - const created = addThread(state.artifact, fp, { author: this.currentAuthor(), body: r.text }); + const created = addThread(state.artifact, fp, { author, body: r.text }); threadId = created.threadId; state.vsThreads.set(threadId, vsThread); state.live.set(threadId, offsets); @@ -231,8 +237,14 @@ export class ThreadController implements vscode.Disposable { } this.persist(state); this.refreshComments(vsThread, state, threadId); - // D10: every human comment on a coedited doc runs a turn. - void this.respondInThread(vsThread, state, threadId, r.text); + // D10: every HUMAN comment on a coedited doc runs a turn. Finding 2 guard: + // `cowriting.reply` is the ingress for both a genuine human reply and (in + // principle) a re-dispatched machine message — never loop the machine's own + // reply back through respondInThread (it would double-turn on Claude's words + // and re-flip an already-"offer" thread). + if (author.kind !== "agent") { + void this.respondInThread(vsThread, state, threadId, r.text); + } } /** @@ -310,6 +322,12 @@ export class ThreadController implements vscode.Disposable { const target = this.targetOf(doc, vsThread); const ids = await this.editFlow.runEditAndPropose(doc, target, offer.ask); vsThread.contextValue = "offer-done"; + // Finding 3 (seam hygiene): drop the offer once it's spent — otherwise the + // `makeThreadEdit(threadId, docPath)` test seam (or a stray second UI click, + // now dead per contextValue but still a valid direct call) could re-run the + // same edit a second time. A future offer on this thread is a fresh + // respondInThread() call, which re-populates the WeakMap entry. + this.offers.delete(vsThread); if (ids.length) { void vscode.window.showInformationMessage( `Cowriting: ${ids.length} pending change${ids.length === 1 ? "" : "s"} landed in the buffer — ✓ Keep / ✗ Reject there.`, diff --git a/test/e2e/suite/commentLoop.test.ts b/test/e2e/suite/commentLoop.test.ts index 4136ace..a3ab961 100644 --- a/test/e2e/suite/commentLoop.test.ts +++ b/test/e2e/suite/commentLoop.test.ts @@ -44,4 +44,47 @@ suite("comment → reply → offer → proposal (PUC-8, D10/D19)", () => { const id = await api.threadController.createThreadOnSelection("hello"); assert.strictEqual(id, undefined); // gate refuses thread creation entirely }); + + // Finding 1 (code review, session native-surfaces-exec): `cowriting.edit` with NO + // selection used to route through `cowriting.editDocument` → the removed + // `askEditInstruction` webview prompt — a rejecting stub `void`'d before any + // try/catch, i.e. a completely silent dead end (no toast, no thread). It must now + // reach ThreadController.askClaude()'s top-anchored whole-document path (D19) — + // the same comments-first ask the selection route already used. There is no API to + // drive VS Code's native comment-widget submission from a host E2E test (the actual + // comment→reply→offer→proposal round trip is exercised above via the + // createThreadOnSelection seam), so this asserts the routing itself: the gesture + // reaches ThreadController.askClaude (never the old broken stub) and does not throw. + test("cowriting.edit with no selection on a coedited doc reaches the top-anchored ask path, not a silent dead end", async () => { + const api = await activateApi(); + const doc = await vscode.workspace.openTextDocument({ + language: "markdown", + content: "# T\n\nA document-level ask with no selection.\n", + }); + const ed = await vscode.window.showTextDocument(doc); + await vscode.commands.executeCommand("cowriting.coeditDocument"); + // Defensive stubs, mirroring the suite's other test — no real turn should fire + // from this gesture alone (no comment text is ever submitted), but guard against + // a real @cline/sdk call if the routing regresses. + api.threadController.setTurnRunnerForTest(async () => ({ replacement: "stub", model: "stub", sessionId: "s1" })); + api.editFlow.setEditTurnForTest(async () => ({ replacement: "stub", model: "stub", sessionId: "s1" })); + ed.selection = new vscode.Selection(0, 0, 0, 0); // no selection + + let askClaudeCalls = 0; + const origAskClaude = api.threadController.askClaude.bind(api.threadController); + api.threadController.askClaude = async () => { + askClaudeCalls++; + return origAskClaude(); + }; + try { + await vscode.commands.executeCommand("cowriting.edit"); + } finally { + api.threadController.askClaude = origAskClaude; + } + assert.strictEqual( + askClaudeCalls, + 1, + "cowriting.edit (no selection) reached ThreadController.askClaude — not the removed editDocument prompt stub", + ); + }); }); diff --git a/test/e2e/suite/f12Reach.test.ts b/test/e2e/suite/f12Reach.test.ts index 4aa4468..7b3aff4 100644 --- a/test/e2e/suite/f12Reach.test.ts +++ b/test/e2e/suite/f12Reach.test.ts @@ -72,9 +72,17 @@ suite("F12 SLICE-1 — Ask-Claude reach (#42, INV-38)", () => { // PUC-3 behavior: editDocument invoked with a tab URI targets THAT document, // not whatever editor happens to be active (mirrors #41's clicked-doc resolution). + // Finding 1 fix (native-surfaces code review): editDocument no longer prompts via + // the removed askEditInstruction webview stub (that path threw a silent, `void`'d, + // unhandled rejection — no toast, no thread, no proposal). It now resolves/focuses + // its target document and hands off to ThreadController.askClaude() (D19), same as + // editSelection. The turn→proposal cut itself (EditFlow.runEditAndPropose against a + // {kind:"document"} target) is exercised directly, without the command, in + // f11Toolbar.test.ts / f12Accept.test.ts / f12Review.test.ts — this test's job is + // the URI-targeting/focus behavior, plus confirming the routing actually reaches + // askClaude (proving Finding 1's dead end is gone). test("editDocument(uri) targets the clicked tab's document, not the active editor", async () => { const api = await getApi(); - const editFlow = api.editFlow; // Doc A is the active editor; Doc B is the "clicked tab" we pass by URI. const a = await freshDoc("docs/f12-active.md", "# Active\n\nThe active editor paragraph.\n"); @@ -82,55 +90,68 @@ suite("F12 SLICE-1 — Ask-Claude reach (#42, INV-38)", () => { await vscode.window.showTextDocument(a.doc); await settle(); - // Stub the document instruction prompt (the webview can't run in CI) + the LLM turn. - const origPrompt = editFlow.askEditInstruction; - editFlow.askEditInstruction = async () => "rewrite it"; - editFlow.setEditTurnForTest(async () => ({ - replacement: "# Tab\n\nThe REWRITTEN tab paragraph.\n", - model: "sonnet", - sessionId: "e2e-f12-tab", - })); + let askClaudeCalls = 0; + // Capture the active-editor doc INSIDE the spy, before delegating to the real + // askClaude — askClaude's own `workbench.action.addComment` side effect moves + // focus to the ephemeral comment-input widget (a `comment://` URI), which would + // make a post-hoc `activeTextEditor` check meaningless. What we're proving here + // is editDocument's OWN resolve-and-focus step ran against the right document + // before handing off. + let focusedDocAtHandoff: string | undefined; + const origAskClaude = api.threadController.askClaude.bind(api.threadController); + api.threadController.askClaude = async () => { + askClaudeCalls++; + focusedDocAtHandoff = vscode.window.activeTextEditor?.document.uri.toString(); + return origAskClaude(); + }; try { await vscode.commands.executeCommand("cowriting.editDocument", b.doc.uri); await settle(); } finally { - editFlow.askEditInstruction = origPrompt; + api.threadController.askClaude = origAskClaude; } - // The proposal(s) landed on the TAB doc (B), and the ACTIVE doc (A) has none. - assert.ok(api.proposalController.listProposals(b.doc).length >= 1, "tab doc B received the document-edit proposal(s)"); assert.strictEqual( - api.proposalController.listProposals(a.doc).length, - 0, - "active doc A was NOT edited — editDocument honored the tab URI", + askClaudeCalls, + 1, + "editDocument(uri) reached ThreadController.askClaude — not the removed prompt stub", + ); + assert.strictEqual( + focusedDocAtHandoff, + b.doc.uri.toString(), + "editDocument(uri) focused the TAB doc B, not the previously-active doc A, before handing off to askClaude", ); - // F12 (INV-48): EditorProposalController optimistically applies proposals into the - // buffer, so the proposed text is now in the tab doc B. Confirm one of the proposal - // texts is present (the tab doc was edited, not A). - assert.ok(b.doc.getText().includes("REWRITTEN"), "F12 optimistic-apply: proposed text in tab doc B"); }); // No URI arg (palette / keybinding) → fall back to the active editor. test("editDocument() with no arg targets the active editor", async () => { const api = await getApi(); - const editFlow = api.editFlow; const a = await freshDoc("docs/f12-noarg.md", "# No arg\n\nThe active doc paragraph here.\n"); await vscode.window.showTextDocument(a.doc); await settle(); - const origPrompt = editFlow.askEditInstruction; - editFlow.askEditInstruction = async () => "rewrite it"; - editFlow.setEditTurnForTest(async () => ({ - replacement: "# No arg\n\nThe REWRITTEN active doc paragraph.\n", - model: "sonnet", - sessionId: "e2e-f12-noarg", - })); + let askClaudeCalls = 0; + // See the sibling test above for why this is captured inside the spy rather + // than after askClaude() returns. + let focusedDocAtHandoff: string | undefined; + const origAskClaude = api.threadController.askClaude.bind(api.threadController); + api.threadController.askClaude = async () => { + askClaudeCalls++; + focusedDocAtHandoff = vscode.window.activeTextEditor?.document.uri.toString(); + return origAskClaude(); + }; try { await vscode.commands.executeCommand("cowriting.editDocument"); await settle(); } finally { - editFlow.askEditInstruction = origPrompt; + api.threadController.askClaude = origAskClaude; } - assert.ok(api.proposalController.listProposals(a.doc).length >= 1, "active doc received the proposal(s) on no-arg"); + + assert.strictEqual(askClaudeCalls, 1, "editDocument() with no arg reached ThreadController.askClaude"); + assert.strictEqual( + focusedDocAtHandoff, + a.doc.uri.toString(), + "editDocument() with no arg kept the active editor's doc active before handing off to askClaude", + ); }); });