diff --git a/apps/vscode/package.json b/apps/vscode/package.json index 44b35976b5..72b8e74f63 100644 --- a/apps/vscode/package.json +++ b/apps/vscode/package.json @@ -411,9 +411,18 @@ }, { "command": "codev.submitBuilderComment", - "title": "Codev: Queue Comment for Builder", + "title": "Queue Comment for Builder", "enablement": "!commentIsEmpty" }, + { + "command": "codev.forwardBuilderComment", + "title": "Forward to Builder", + "enablement": "!commentIsEmpty" + }, + { + "command": "codev.cancelBuilderComment", + "title": "Cancel" + }, { "command": "codev.deleteBuilderComment", "title": "Codev: Delete Pending Comment", @@ -516,6 +525,14 @@ "command": "codev.submitBuilderComment", "when": "false" }, + { + "command": "codev.forwardBuilderComment", + "when": "false" + }, + { + "command": "codev.cancelBuilderComment", + "when": "false" + }, { "command": "codev.deleteBuilderComment", "when": "false" @@ -840,7 +857,17 @@ }, { "command": "codev.submitBuilderComment", - "group": "inline", + "group": "inline@1", + "when": "commentController == codev-builder-review && commentThreadIsEmpty && codev.diffCodelensMode == 'comment'" + }, + { + "command": "codev.forwardBuilderComment", + "group": "inline@1", + "when": "commentController == codev-builder-review && commentThreadIsEmpty && codev.diffCodelensMode != 'comment'" + }, + { + "command": "codev.cancelBuilderComment", + "group": "inline@2", "when": "commentController == codev-builder-review && commentThreadIsEmpty" } ], diff --git a/apps/vscode/src/__tests__/builder-review-submit.test.ts b/apps/vscode/src/__tests__/builder-review-submit.test.ts new file mode 100644 index 0000000000..e16d3bf9c6 --- /dev/null +++ b/apps/vscode/src/__tests__/builder-review-submit.test.ts @@ -0,0 +1,252 @@ +/** + * Builder-review Submit is the single authoring surface's delivery point + * (#1552). The diff codelens mode decides what Submit does with the authored + * prose: + * - comment mode → enqueue a PendingComment (the batched Submit Review path) + * - forward mode → inject " " into the builder PTY immediately + * An empty / whitespace-only submit leaves NOTHING behind (no queue entry, no + * forward, no orphan thread) — the same net result as Escape/Cancel. + */ + +import { describe, it, expect, beforeEach, vi } from 'vitest'; + +const h = vi.hoisted(() => { + class EventEmitter { + private handlers: Array<(e: T) => void> = []; + event = (fn: (e: T) => void): { dispose(): void } => { this.handlers.push(fn); return { dispose() {} }; }; + fire(value: T): void { for (const fn of this.handlers) { fn(value); } } + dispose(): void {} + } + const state = { + mode: 'comment' as string, + activeFsPath: undefined as string | undefined, + handlers: new Map unknown>(), + executed: [] as Array<{ command: string; args: unknown[] }>, + }; + return { EventEmitter, state }; +}); + +vi.mock('vscode', () => ({ + EventEmitter: h.EventEmitter, + Range: class { constructor(public a: number, public b: number, public c: number, public d: number) {} }, + Uri: { file: (fsPath: string) => ({ fsPath, toString: () => `file://${fsPath}` }) }, + Disposable: class { constructor(private fn: () => void) {} dispose(): void { this.fn(); } }, + MarkdownString: class { constructor(public value: string) {} }, + CommentMode: { Preview: 1, Editing: 0 }, + CommentThreadCollapsibleState: { Collapsed: 0, Expanded: 1 }, + comments: { + createCommentController: () => ({ + options: undefined as unknown, + commentingRangeProvider: undefined as unknown, + createCommentThread: vi.fn(() => ({ comments: [], dispose() {} })), + dispose: vi.fn(), + }), + }, + commands: { + registerCommand: (id: string, fn: (...args: never[]) => unknown) => { h.state.handlers.set(id, fn); return { dispose() {} }; }, + executeCommand: vi.fn(async (command: string, ...args: unknown[]) => { h.state.executed.push({ command, args }); }), + }, + window: { + get activeTextEditor() { + if (!h.state.activeFsPath) { return undefined; } + return { document: { uri: { fsPath: h.state.activeFsPath }, lineCount: 40, lineAt: () => ({ text: '' }) } }; + }, + visibleTextEditors: [], + onDidChangeActiveTextEditor: () => ({ dispose() {} }), + showWarningMessage: vi.fn(), + }, + workspace: { + textDocuments: [], + getConfiguration: () => ({ get: () => h.state.mode }), + onDidChangeConfiguration: () => ({ dispose() {} }), + }, + languages: { registerCodeLensProvider: () => ({ dispose() {} }) }, +})); + +const { + activateBuilderReviewComments, + isBuilderComposerOpen, + submitActiveBuilderComposer, +} = await import('../comments/builder-review.js'); +const { setDiffInjectSession } = await import('../diff-inject-codelens.js'); + +const ENTRY = { fsPath: '/wt/pkg/src/a.ts', builderId: 'pir-9', relPath: 'pkg/src/a.ts', hunks: [], baseRef: 'main', worktreePath: '/wt' }; + +const added: Array<{ builderId: string; comment: { file: string; lineRange: unknown; body: string } }> = []; +const storeStub = { + onDidChangeQueue: () => ({ dispose() {} }), + getWorktreePath: () => '/wt', + registerWorktree: () => {}, + load: async () => [], + getComments: () => [], + add: async (builderId: string, comment: { file: string; lineRange: unknown; body: string }) => { added.push({ builderId, comment }); }, +} as never; +const overviewStub = { getData: () => null } as never; + +/** A pending in-progress reply thread. `range` undefined → a file comment. */ +function makeThread(range?: { start: { line: number }; end: { line: number } }) { + return { uri: { fsPath: ENTRY.fsPath }, range, dispose: vi.fn() }; +} + +function submit() { + return h.state.handlers.get('codev.submitBuilderComment') as (reply: unknown) => Promise; +} +function forwardCalls() { + return h.state.executed.filter(e => e.command === 'codev.forwardToBuilder'); +} + +beforeEach(() => { + h.state.handlers.clear(); + h.state.executed = []; + h.state.activeFsPath = undefined; + added.length = 0; + setDiffInjectSession([]); + // activate resets composerOpen to false each run. + activateBuilderReviewComments({ subscriptions: [] } as never, storeStub, overviewStub); + setDiffInjectSession([ENTRY]); +}); + +/** Open a box via the authoring entry point the codelens + feedback gestures use. */ +function openBox() { + return h.state.handlers.get('codev.commentForBuilder') as (...a: unknown[]) => Promise; +} +function builtinCalls() { + return h.state.executed.map(e => e.command); +} + +describe('builder-review Submit delivery (#1552)', () => { + it('comment mode: enqueues the authored prose with the thread range', async () => { + h.state.mode = 'comment'; + const thread = makeThread({ start: { line: 4 }, end: { line: 8 } }); // 1-based 5..9 + await submit()({ thread, text: 'please rename this' }); + expect(added).toHaveLength(1); + expect(added[0].builderId).toBe('pir-9'); + expect(added[0].comment.file).toBe('pkg/src/a.ts'); + expect(added[0].comment.lineRange).toEqual({ start: 5, end: 9 }); + expect(added[0].comment.body).toBe('please rename this'); + expect(forwardCalls()).toHaveLength(0); + expect(thread.dispose).toHaveBeenCalled(); + }); + + it('forward mode: injects " " into the builder PTY and enqueues nothing', async () => { + h.state.mode = 'forward'; + const thread = makeThread({ start: { line: 4 }, end: { line: 8 } }); + await submit()({ thread, text: 'please rename this' }); + expect(forwardCalls()).toEqual([ + { command: 'codev.forwardToBuilder', args: ['pir-9', 'pkg/src/a.ts:L5-L9 please rename this'] }, + ]); + expect(added).toHaveLength(0); + expect(thread.dispose).toHaveBeenCalled(); + }); + + it('codev.forwardBuilderComment shares the handler: in forward mode it forwards ref + prose', async () => { + h.state.mode = 'forward'; + const forwardCmd = h.state.handlers.get('codev.forwardBuilderComment') as (reply: unknown) => Promise; + expect(forwardCmd).toBeTypeOf('function'); + const thread = makeThread({ start: { line: 4 }, end: { line: 8 } }); + await forwardCmd({ thread, text: 'rename this' }); + expect(forwardCalls()).toEqual([ + { command: 'codev.forwardToBuilder', args: ['pir-9', 'pkg/src/a.ts:L5-L9 rename this'] }, + ]); + expect(added).toHaveLength(0); + }); + + it('forward mode, whole-file comment: forwards the file ref + prose', async () => { + h.state.mode = 'forward'; + const thread = makeThread(undefined); // file comment + await submit()({ thread, text: 'overall this file needs work' }); + expect(forwardCalls()).toEqual([ + { command: 'codev.forwardToBuilder', args: ['pir-9', 'pkg/src/a.ts overall this file needs work'] }, + ]); + expect(added).toHaveLength(0); + }); + + it('empty / whitespace submit leaves nothing behind in either mode', async () => { + for (const mode of ['comment', 'forward']) { + h.state.mode = mode; + const thread = makeThread({ start: { line: 4 }, end: { line: 8 } }); + await submit()({ thread, text: ' \n\t ' }); + expect(added).toHaveLength(0); + expect(forwardCalls()).toHaveLength(0); + expect(thread.dispose).toHaveBeenCalled(); + } + }); + + it('trims surrounding whitespace from the authored body (comment mode)', async () => { + h.state.mode = 'comment'; + const thread = makeThread({ start: { line: 4 }, end: { line: 8 } }); + await submit()({ thread, text: ' trailing spaces kept out ' }); + expect(added[0].comment.body).toBe('trailing spaces kept out'); + }); +}); + +describe('builder-review composer state + deck submit/cancel executors (#1552)', () => { + it('opening a box marks the composer open; Submit clears it', async () => { + expect(isBuilderComposerOpen()).toBe(false); + h.state.activeFsPath = ENTRY.fsPath; // openCommentInput requires the active editor to be the file + await openBox()('pir-9', ENTRY.fsPath, 'pkg/src/a.ts', null); + expect(isBuilderComposerOpen()).toBe(true); + // VS Code focused the reply box via the built-in add-comment command. + expect(builtinCalls()).toContain('workbench.action.addComment'); + + const thread = makeThread(undefined); + await submit()({ thread, text: 'a real comment' }); + expect(isBuilderComposerOpen()).toBe(false); + }); + + it('submitActiveBuilderComposer drives the VERIFIED built-in editor.action.submitComment and clears the flag', async () => { + h.state.activeFsPath = ENTRY.fsPath; + await openBox()('pir-9', ENTRY.fsPath, 'pkg/src/a.ts', null); + expect(isBuilderComposerOpen()).toBe(true); + h.state.executed = []; + await submitActiveBuilderComposer(); + // Must be editor.action.* — workbench.action.submitComment does not exist. + expect(builtinCalls()).toContain('editor.action.submitComment'); + expect(builtinCalls()).not.toContain('workbench.action.submitComment'); + expect(isBuilderComposerOpen()).toBe(false); + }); + + it('the Cancel BUTTON (codev.cancelBuilderComment) disposes the box and clears the flag — nothing queued/forwarded', async () => { + h.state.activeFsPath = ENTRY.fsPath; + await openBox()('pir-9', ENTRY.fsPath, 'pkg/src/a.ts', null); + expect(isBuilderComposerOpen()).toBe(true); + added.length = 0; + const cancelButton = h.state.handlers.get('codev.cancelBuilderComment') as (reply: unknown) => void; + const thread = makeThread({ start: { line: 4 }, end: { line: 8 } }); + cancelButton({ thread, text: 'half-typed prose' }); + expect(thread.dispose).toHaveBeenCalled(); + expect(added).toHaveLength(0); + expect(forwardCalls()).toHaveLength(0); + expect(isBuilderComposerOpen()).toBe(false); + }); + + it('does NOT treat a foreign comment box as ours: a focused commentinput with our flag clear reads composer-CLOSED (CMAP #1552 cross-controller)', () => { + // A plan/spec review box (the codev-review controller) also opens + // commentinput-… editors. With OUR flag clear, that must not read as our + // composer — else a diff-review dial would submit the unrelated plan comment. + h.state.activeFsPath = '/cluesmith.codev-vscode/commentinput-planreview-1.md'; + expect(isBuilderComposerOpen()).toBe(false); + }); + + it('opening OUR box sets the flag; it does not depend on which editor is focused', async () => { + h.state.activeFsPath = ENTRY.fsPath; + expect(isBuilderComposerOpen()).toBe(false); + await openBox()('pir-9', ENTRY.fsPath, 'pkg/src/a.ts', null); + expect(isBuilderComposerOpen()).toBe(true); + }); + + it('self-heal: a submit executor on a stale-open flag still runs only the (no-op) built-in and clears — never resurrects prose', async () => { + // Simulate a stale flag: a box was opened then dismissed by native Escape + // (unobservable), leaving composerOpen true with no focused editor. + h.state.activeFsPath = ENTRY.fsPath; + await openBox()('pir-9', ENTRY.fsPath, 'pkg/src/a.ts', null); + expect(isBuilderComposerOpen()).toBe(true); + h.state.executed = []; + added.length = 0; + await submitActiveBuilderComposer(); // built-in is a no-op host-side when nothing is focused + expect(builtinCalls()).toEqual(['editor.action.submitComment']); // nothing else fired + expect(added).toHaveLength(0); // no queue write, no phantom submit + expect(forwardCalls()).toHaveLength(0); + expect(isBuilderComposerOpen()).toBe(false); // healed + }); +}); diff --git a/apps/vscode/src/__tests__/contributes-review-queue.test.ts b/apps/vscode/src/__tests__/contributes-review-queue.test.ts index dec8f434f9..eb5f6384b1 100644 --- a/apps/vscode/src/__tests__/contributes-review-queue.test.ts +++ b/apps/vscode/src/__tests__/contributes-review-queue.test.ts @@ -60,6 +60,21 @@ describe('editor/title mode toggle', () => { }); }); +describe('builder-review submit button is mode-labelled', () => { + it('shows Queue in comment mode and Forward in forward mode via mutually exclusive when clauses (CMAP #1552)', () => { + const queue = entry('comments/commentThread/context', 'codev.submitBuilderComment'); + const forward = entry('comments/commentThread/context', 'codev.forwardBuilderComment'); + // Same inline slot so exactly one is the primary Submit button per mode. + expect(queue?.group).toBe('inline@1'); + expect(forward?.group).toBe('inline@1'); + expect(queue?.when).toContain("codev.diffCodelensMode == 'comment'"); + expect(forward?.when).toContain("codev.diffCodelensMode != 'comment'"); + const cmds = PKG.contributes.commands as Array<{ command: string; title: string }>; + expect(cmds.find(c => c.command === 'codev.submitBuilderComment')?.title).toBe('Queue Comment for Builder'); + expect(cmds.find(c => c.command === 'codev.forwardBuilderComment')?.title).toBe('Forward to Builder'); + }); +}); + describe('builder-review comment menus', () => { it('scopes every entry to the codev-builder-review controller', () => { const sections = [ @@ -70,6 +85,8 @@ describe('builder-review comment menus', () => { ]; const builderCommands = [ 'codev.submitBuilderComment', + 'codev.forwardBuilderComment', + 'codev.cancelBuilderComment', 'codev.deleteBuilderComment', 'codev.startEditBuilderComment', 'codev.saveEditBuilderComment', diff --git a/apps/vscode/src/__tests__/feedback.test.ts b/apps/vscode/src/__tests__/feedback.test.ts index 056bc1d649..3e73aacee2 100644 --- a/apps/vscode/src/__tests__/feedback.test.ts +++ b/apps/vscode/src/__tests__/feedback.test.ts @@ -1,14 +1,20 @@ /** - * Mode-neutral review feedback (#1410): the diff/scroll dial verbs route each - * chunk forward-now (immediate PTY inject) or into the queue, following the - * `codev.diffCodelensMode` setting, deriving both branches from the same anchor. + * Mode-neutral review feedback (#1410, #1552): the diff/scroll dial verbs turn + * each chunk (whole file / hunk-under-cursor / selection) into an AUTHORING + * gesture — they open the native comment reply box at the anchor via + * `codev.commentForBuilder`. There is no promptless path. + * + * Deck composer parity (#1552): while a builder-review box is OPEN, VS Code (the + * diff-mode owner) interprets the SAME verbs to drive it — hunk & selection = + * open-or-submit, file = no-op (dial cancel was dropped, ruling B; discard is the + * visible Cancel button). The queue-vs-forward decision lives in the box's Submit, + * not here. */ import { describe, it, expect, beforeEach, vi } from 'vitest'; const h = vi.hoisted(() => { const state = { - mode: 'forward' as 'forward' | 'comment', activeFsPath: undefined as string | undefined, selection: { active: { line: 0 }, start: { line: 0, character: 0 }, end: { line: 0, character: 0 }, isEmpty: true }, executed: [] as Array<{ command: string; args: unknown[] }>, @@ -17,6 +23,9 @@ const h = vi.hoisted(() => { // Stdout the press helper's fresh `git diff` returns; null → the call rejects, // which makes `resolvePressCursorRef` fall back to the frozen `entry.hunks`. gitStdout: null as string | null, + // Builder-review composer state + spies (mocked module below). + composerOpen: false, + submitCalls: 0, }; return { state }; }); @@ -30,6 +39,12 @@ vi.mock('node:child_process', () => ({ }, })); +// The composer owner: feedback.ts reads its open-state and drives submit. +vi.mock('../comments/builder-review.js', () => ({ + isBuilderComposerOpen: () => h.state.composerOpen, + submitActiveBuilderComposer: vi.fn(async () => { h.state.submitCalls++; }), +})); + vi.mock('vscode', () => ({ EventEmitter: class { event = (): { dispose(): void } => ({ dispose() {} }); fire(): void {} dispose(): void {} }, RelativePattern: class {}, @@ -43,7 +58,7 @@ vi.mock('vscode', () => ({ }, workspace: { createFileSystemWatcher: vi.fn(), - getConfiguration: () => ({ get: () => h.state.mode }), + getConfiguration: () => ({ get: () => 'forward' }), onDidChangeConfiguration: () => ({ dispose() {} }), }, commands: { executeCommand: vi.fn(async (command: string, ...args: unknown[]) => { h.state.executed.push({ command, args }); }) }, @@ -52,101 +67,111 @@ vi.mock('vscode', () => ({ CodeLens: class {}, })); -const { feedbackFile, feedbackHunk, feedbackSelection } = await import('../review-queue/feedback.js'); +const { feedbackFile, feedbackHunk, feedbackSelection, decideFeedbackAction } = await import('../review-queue/feedback.js'); const { setDiffInjectSession } = await import('../diff-inject-codelens.js'); const FS_PATH = '/w/alpha/.builders/pir-1/src/a.ts'; -/** Minimal in-memory ReviewQueueStore stand-in capturing worktree + queue writes. */ -function makeStore() { - const worktrees = new Map(); - const added: Array<{ builderId: string; comment: { file: string; lineRange: unknown; body: string } }> = []; - return { - store: { - getWorktreePath: (id: string) => worktrees.get(id), - registerWorktree: (id: string, wt: string) => { worktrees.set(id, wt); }, - add: async (builderId: string, comment: { file: string; lineRange: unknown; body: string }) => { added.push({ builderId, comment }); }, - }, - worktrees, - added, - }; +/** The single `codev.commentForBuilder` invocation an OPEN gesture is expected to make. */ +function commentCalls() { + return h.state.executed.filter(e => e.command === 'codev.commentForBuilder'); } -describe('feedback mode-router (#1410)', () => { +describe('decideFeedbackAction — pure composer state machine (#1552)', () => { + it('with NO box open, every axis opens a comment at that axis', () => { + expect(decideFeedbackAction('file', false)).toEqual({ kind: 'open', axis: 'file' }); + expect(decideFeedbackAction('hunk', false)).toEqual({ kind: 'open', axis: 'hunk' }); + expect(decideFeedbackAction('selection', false)).toEqual({ kind: 'open', axis: 'selection' }); + }); + + it('with a box open, hunk & selection submit (open-or-submit); the file dial is a defined no-op (cancel dropped, ruling B)', () => { + expect(decideFeedbackAction('hunk', true)).toEqual({ kind: 'submit' }); + expect(decideFeedbackAction('selection', true)).toEqual({ kind: 'submit' }); + expect(decideFeedbackAction('file', true)).toEqual({ kind: 'noop' }); + }); + + it('a stale-open flag decides submit for hunk — never a phantom cancel/open; the built-in no-op is what makes it safe', () => { + // The function cannot know the flag is stale; it decides `submit`. Safety + // comes from the executor: SUBMIT runs the built-in submit-comment, a no-op + // when nothing is focused (asserted in builder-review-submit.test.ts). + expect(decideFeedbackAction('hunk', true)).toEqual({ kind: 'submit' }); + }); +}); + +describe('feedback gesture routing (#1410, #1552)', () => { beforeEach(() => { - h.state.mode = 'forward'; h.state.activeFsPath = FS_PATH; h.state.selection = { active: { line: 0 }, start: { line: 0, character: 0 }, end: { line: 0, character: 0 }, isEmpty: true }; h.state.executed = []; h.state.warnings = []; h.state.statusMessages = []; h.state.gitStdout = null; // default: git rejects → fall back to the frozen entry.hunks + h.state.composerOpen = false; + h.state.submitCalls = 0; setDiffInjectSession([{ fsPath: FS_PATH, builderId: 'pir-1', relPath: 'src/a.ts', hunks: [{ start: 5, end: 9 }], baseRef: 'main', worktreePath: '/w/alpha/.builders/pir-1' }]); }); - it('forward mode: a file press injects immediately via forwardToBuilder', async () => { - const { store, added } = makeStore(); - await feedbackFile({ store: store as never }); - expect(h.state.executed).toEqual([{ command: 'codev.forwardToBuilder', args: ['pir-1', 'src/a.ts '] }]); - expect(added).toHaveLength(0); // nothing queued in forward mode - }); - - it('comment mode: a file press enqueues a whole-file comment through the store', async () => { - h.state.mode = 'comment'; - const { store, added, worktrees } = makeStore(); - await feedbackFile({ store: store as never }); - expect(h.state.executed).toHaveLength(0); // no immediate forward - expect(added).toHaveLength(1); - expect(added[0].builderId).toBe('pir-1'); - expect(added[0].comment.file).toBe('src/a.ts'); - expect(added[0].comment.lineRange).toBeNull(); // whole file - expect(added[0].comment.body).toContain('Stream Deck'); - // worktree derived from the diff entry (never guessed) - expect(worktrees.get('pir-1')).toBe('/w/alpha/.builders/pir-1'); + it('no box open: a file press opens the comment reply box at the whole-file anchor', async () => { + await feedbackFile(); + expect(commentCalls()).toEqual([ + { command: 'codev.commentForBuilder', args: ['pir-1', FS_PATH, 'src/a.ts', null] }, + ]); }); - it('comment mode: a hunk press enqueues the changed-hunk range under the cursor', async () => { - h.state.mode = 'comment'; + it('no box open: a hunk press opens the input anchored to the changed-hunk range under the cursor', async () => { h.state.selection = { active: { line: 6 }, start: { line: 6, character: 0 }, end: { line: 6, character: 0 }, isEmpty: true }; // line 7 ∈ [5,9] - const { store, added } = makeStore(); - await feedbackHunk({ store: store as never }); - expect(added[0].comment.lineRange).toEqual({ start: 5, end: 9 }); + await feedbackHunk(); + expect(commentCalls()[0].args[3]).toEqual({ start: 5, end: 9 }); }); - it('a hunk press resolves against the FRESH git parse, not the stale frozen ranges (#1534)', async () => { - // Frozen entry.hunks is [{5,9}] and does NOT cover line 20; the live diff does. + it('no box open: a hunk press resolves against the FRESH git parse, not the stale frozen ranges (#1534)', async () => { h.state.gitStdout = '@@ -0,0 +20,2 @@\n+const added = 1;\n+const more = 2;\n'; h.state.selection = { active: { line: 19 }, start: { line: 19, character: 0 }, end: { line: 19, character: 0 }, isEmpty: true }; // line 20 - const { store } = makeStore(); - await feedbackHunk({ store: store as never }); // forward mode (default) - expect(h.state.executed).toContainEqual({ command: 'codev.forwardToBuilder', args: ['pir-1', 'src/a.ts:L20-L21 '] }); + await feedbackHunk(); + expect(commentCalls()[0].args[3]).toEqual({ start: 20, end: 21 }); expect(h.state.statusMessages).toHaveLength(0); }); - it('a hunk press with no changed lines at the cursor anchors the whole file with an honest note, never the old error (#1534)', async () => { - h.state.mode = 'comment'; + it('no box open: a hunk press with no changed lines anchors the whole file with an honest note (#1534)', async () => { h.state.gitStdout = ''; // a fresh parse that records no ranges h.state.selection = { active: { line: 0 }, start: { line: 0, character: 0 }, end: { line: 0, character: 0 }, isEmpty: true }; // line 1 ∉ any hunk - const { store, added } = makeStore(); - await feedbackHunk({ store: store as never }); - expect(added[0].comment.lineRange).toBeNull(); // whole file, not an error + await feedbackHunk(); + expect(commentCalls()[0].args[3]).toBeNull(); expect(h.state.statusMessages.join('\n')).toContain('no changed lines at the cursor'); expect(h.state.statusMessages.join('\n')).not.toContain('place the cursor in a changed hunk'); }); - it('comment mode: a selection press enqueues the selected range', async () => { - h.state.mode = 'comment'; + it('no box open: a selection press opens the input anchored to the selected range', async () => { h.state.selection = { active: { line: 2 }, start: { line: 2, character: 0 }, end: { line: 5, character: 4 }, isEmpty: false }; - const { store, added } = makeStore(); - await feedbackSelection({ store: store as never }); - expect(added[0].comment.lineRange).toEqual({ start: 3, end: 6 }); + await feedbackSelection(); + expect(commentCalls()[0].args[3]).toEqual({ start: 3, end: 6 }); }); - it('does nothing when the focused editor is not a tracked builder diff', async () => { + it('no box open: warns instead of doing nothing when the focused editor is not a builder diff', async () => { h.state.activeFsPath = '/some/unrelated/file.ts'; - const { store, added } = makeStore(); - await feedbackFile({ store: store as never }); - expect(h.state.executed).toHaveLength(0); - expect(added).toHaveLength(0); + await feedbackFile(); + expect(commentCalls()).toHaveLength(0); + expect(h.state.warnings.join('\n')).toContain('focus a builder diff first'); + }); + + it('box open: a hunk press SUBMITS the open composer and opens nothing new', async () => { + h.state.composerOpen = true; + await feedbackHunk(); + expect(h.state.submitCalls).toBe(1); + expect(commentCalls()).toHaveLength(0); + }); + + it('box open: a file press is a defined NO-OP — no submit, no open (dial cancel dropped; the Cancel button discards)', async () => { + h.state.composerOpen = true; + await feedbackFile(); + expect(h.state.submitCalls).toBe(0); + expect(commentCalls()).toHaveLength(0); + }); + + it('box open: a selection press SUBMITS (open-or-submit) — so the dial that opened the box can also submit it', async () => { + h.state.composerOpen = true; + await feedbackSelection(); + expect(h.state.submitCalls).toBe(1); + expect(commentCalls()).toHaveLength(0); }); }); diff --git a/apps/vscode/src/comments/builder-review.ts b/apps/vscode/src/comments/builder-review.ts index dd32bb5040..ba29ce791d 100644 --- a/apps/vscode/src/comments/builder-review.ts +++ b/apps/vscode/src/comments/builder-review.ts @@ -24,8 +24,10 @@ import { getDiffInjectEntry, getDiffInjectEntries, onDidChangeDiffInjectRegistry, + getDiffCodelensMode, COMMENT_FOR_BUILDER_COMMAND, } from '../diff-inject-codelens.js'; +import { buildBuilderFileRef, buildBuilderRangeRef } from '../diff-inject-ref.js'; import { planThreadReconcile, deriveWorktreePath, clampAnchorLines, type RegisteredFile } from '../review-queue/reconcile.js'; import type { ReviewQueueStore } from '../review-queue/store.js'; import type { LineRange, PendingComment } from '../review-queue/queue.js'; @@ -61,6 +63,59 @@ function bodyText(body: string | vscode.MarkdownString): string { return body.value; } +/** + * VS Code built-in that submits the FOCUSED native comment reply box (#1552), + * so a deck submit gesture can flush an open composer. `editor.action.submitComment` + * is the submit id (the `editor.*` id — NOTE: `workbench.action.submitComment` + * does NOT exist; it would be a silent no-op). It only acts on a FOCUSED comment + * editor, which is why the submit path is a no-op when the box has lost focus. + */ +const SUBMIT_FOCUSED_COMMENT = 'editor.action.submitComment'; + +/** + * Whether OUR builder-review comment box is currently open — tracked here, in the + * composer's owner, so the deck feedback router (#1552) can drive it. Set true + * only when WE open a box (`openCommentInput`); cleared when it submits. + * + * This is deliberately OUR flag, not a "focused-comment-input" probe: a second + * native comment controller (`codev-review`, plan/spec review) also opens + * `commentinput-…` editors, so keying off the focused comment URI would let a + * diff-review dial submit an unrelated plan/spec comment (CMAP #1552). The flag + * is scoped to this controller, so `isBuilderComposerOpen()` never mistakes a + * foreign comment box for ours. + * + * A native Escape dismissal is NOT observable via the stable comment API, so the + * flag can stale-stick `true`. That stays cancel-biased: a stale flag can only + * cost a submit no-op or an extra open, never a phantom submit — SUBMIT runs the + * built-in, which no-ops when no comment editor is focused, and the submit + * executor clears the flag, so it self-heals on the next gesture. + */ +let composerOpen = false; + +/** True while OUR builder-review comment box is open (the deck router reads this). */ +export function isBuilderComposerOpen(): boolean { + return composerOpen; +} + +/** + * Submit the focused composer via VS Code's built-in (#1552). The built-in is a + * no-op when no comment editor is focused, so a stale `composerOpen` can never + * resurrect cancelled prose. Our `codev.submitBuilderComment` handler also + * clears the flag on a real submit; clearing here self-heals the no-op case. + */ +export async function submitActiveBuilderComposer(): Promise { + await vscode.commands.executeCommand(SUBMIT_FOCUSED_COMMENT); + composerOpen = false; +} + +// NOTE (#1552, ruling B): there is intentionally NO dial-cancel executor. A +// native comment DRAFT has no safe programmatic discard — hideComment keeps the +// draft, closeActiveEditor closes the HOST editor (observed: focus jumps windows), +// and submit-empty is blocked by the submit button's `!commentIsEmpty` enablement. +// VS Code hands us the thread only on a click, so the discard is the visible +// Cancel button (`codev.cancelBuilderComment`, which disposes the thread). The +// thread-owning rework that would restore a dial cancel is the #1560 spike. + /** The 1-based inclusive range a thread's anchor denotes. */ function threadLineRange(thread: vscode.CommentThread): LineRange { const range = thread.range; @@ -73,13 +128,19 @@ export function activateBuilderReviewComments( store: ReviewQueueStore, overviewCache: OverviewCache, ): void { + // Reset composer state on (re)activation, so a reload never starts thinking a + // box is open (#1552). + composerOpen = false; const controller = vscode.comments.createCommentController( CONTROLLER_ID, 'Codev Builder Review', ); controller.options = { prompt: 'Comment for builder', - placeHolder: 'Type review feedback for the builder, then Queue Comment', + // Neutral hint (CMAP #1552): the submit button's label is mode-specific + // (Queue in comment mode, Forward in the default forward mode), so the + // placeholder stays generic rather than naming one delivery. + placeHolder: 'Type review feedback for the builder, then submit', }; context.subscriptions.push(controller); @@ -232,6 +293,9 @@ export function activateBuilderReviewComments( async function openCommentInput(fsPath: string, range: LineRange | null): Promise { const editor = vscode.window.activeTextEditor; if (!editor || editor.document.uri.fsPath !== fsPath) { return; } + // A box is about to open and take focus — mark the composer open so the deck + // feedback gestures drive it (submit) instead of stacking threads (#1552). + composerOpen = true; if (range) { // endColumn spans the last line's content (clamped by the editor); // ending at column 1 would exclude the last line from the range @@ -278,27 +342,75 @@ export function activateBuilderReviewComments( await vscode.commands.executeCommand('workbench.action.addComment'); }); - // Submit button on an input thread → queue the comment. The input thread is - // disposed; the reconciler re-creates the canonical thread from the queue. - reg('codev.submitBuilderComment', async (reply: vscode.CommentReply) => { + // Submit button on an input thread → deliver the authored comment. The diff + // codelens mode decides delivery (#1552): forward mode injects the ref + + // prose into the builder PTY now (the #789 forward path every forward verb + // uses), comment mode enqueues it for the batched Submit Review. This is the + // SINGLE authoring surface for both — so the gutter "+", the context-menu + // action, the comment codelens, and the deck flag gestures all deliver per + // the current mode (owner-approved at the #1552 plan gate). The input thread + // is disposed either way; in comment mode the reconciler re-creates the + // canonical thread from the queue. + // + // Two command ids share this ONE handler so the button can carry a mode-accurate + // label (CMAP #1552): `codev.submitBuilderComment` ("Queue Comment for Builder", + // shown in comment mode) and `codev.forwardBuilderComment` ("Forward to Builder", + // shown in forward mode) — the menu `when` clauses gate which is visible, the + // handler's own mode branch does the real delivery, so the two can't drift. + const deliverBuilderComment = async (reply: vscode.CommentReply): Promise => { const thread = reply.thread; + // Any Submit — empty or not — ends the composer, so the next deck gesture + // opens a fresh box rather than trying to drive a closed one (#1552). + composerOpen = false; + // Empty / whitespace submit leaves nothing behind: no queue entry, no + // forward, no orphan thread. (Escape/Cancel already disposes the in-progress + // thread; this covers a Submit with a blank body.) + const body = reply.text.trim(); + if (!body) { thread.dispose(); return; } const entry = getDiffInjectEntry(thread.uri.fsPath); - if (!entry || !registerEntryWorktree(entry)) { + if (!entry) { vscode.window.showWarningMessage('Codev: This file is not part of an active builder diff'); return; } // A range-less thread is a file comment (the file-level lens flow). let lineRange: LineRange | null = null; if (thread.range) { lineRange = threadLineRange(thread); } + + if (getDiffCodelensMode() === 'forward') { + const ref = lineRange + ? buildBuilderRangeRef(entry.relPath, lineRange.start, lineRange.end) + : buildBuilderFileRef(entry.relPath); + // The ref carries a trailing space, so `ref + body` reads " ". + thread.dispose(); + await vscode.commands.executeCommand('codev.forwardToBuilder', entry.builderId, ref + body); + return; + } + + if (!registerEntryWorktree(entry)) { + vscode.window.showWarningMessage('Codev: This file is not part of an active builder diff'); + return; + } const comment: PendingComment = { id: randomUUID(), createdAt: new Date().toISOString(), file: entry.relPath, lineRange, - body: reply.text, + body, }; thread.dispose(); await store.add(entry.builderId, comment); + }; + reg('codev.submitBuilderComment', deliverBuilderComment); + reg('codev.forwardBuilderComment', deliverBuilderComment); + + // Cancel button on an input thread → discard the in-progress box, leaving + // nothing queued or forwarded (#1552). VS Code hands us the thread, so a click + // disposes it directly (no dependency on a built-in). This is the VISIBLE + // counterpart to the deck Files-dial cancel and to the canvas composer's + // explicit Cancel — the box was missing a labelled discard next to Submit. + reg('codev.cancelBuilderComment', (reply: vscode.CommentReply) => { + composerOpen = false; + reply.thread.dispose(); }); // Edit flow (#1055 pattern): flip to VS Code's inline edit surface; diff --git a/apps/vscode/src/extension.ts b/apps/vscode/src/extension.ts index 8067094b4e..b64b126ff6 100644 --- a/apps/vscode/src/extension.ts +++ b/apps/vscode/src/extension.ts @@ -1255,11 +1255,13 @@ export async function activate(context: vscode.ExtensionContext) { )), reg('codev.discardReviewComments', () => discardReviewComments({ store: reviewQueueStore, terminalManager: terminalManager!, overviewCache })), - // Mode-neutral review feedback (#1410): the deck diff/scroll dials press - // these; each forwards immediately or enqueues per `codev.diffCodelensMode`. - reg('codev.feedbackCurrentFileToBuilder', () => feedbackFile({ store: reviewQueueStore })), - reg('codev.feedbackCurrentHunkToBuilder', () => feedbackHunk({ store: reviewQueueStore })), - reg('codev.feedbackSelectionToBuilder', () => feedbackSelection({ store: reviewQueueStore })), + // Mode-neutral review feedback (#1410, #1552): the deck diff/scroll dials + // press these; each opens the native comment reply box at the anchor so the + // reviewer authors the comment, which Submit then forwards or enqueues per + // `codev.diffCodelensMode` (see review-queue/feedback.ts). No promptless path. + reg('codev.feedbackCurrentFileToBuilder', () => feedbackFile()), + reg('codev.feedbackCurrentHunkToBuilder', () => feedbackHunk()), + reg('codev.feedbackSelectionToBuilder', () => feedbackSelection()), // Diff codelens mode toggle (#1037): a single title-bar button per mode // (VS Code toolbar buttons have no pressed state — same pattern as the // Agents group-by cycle above); each command shows the mode clicking diff --git a/apps/vscode/src/review-queue/feedback.ts b/apps/vscode/src/review-queue/feedback.ts index 5c1acdf62f..d9af8fe257 100644 --- a/apps/vscode/src/review-queue/feedback.ts +++ b/apps/vscode/src/review-queue/feedback.ts @@ -1,41 +1,92 @@ /** - * Mode-neutral review feedback (#1410): the Stream Deck diff dials and Scroll - * dial press a single `feedback-*` verb, and this module routes each chunk - * (whole file / hunk-under-cursor / selection) EITHER as an immediate PTY - * forward OR into the per-builder pending-comment queue, following the - * workspace's `codev.diffCodelensMode` setting — so the deck never infers the - * mode. Both branches derive their anchor from the SAME resolver, so a given - * dial press references the same file/range in either mode. + * Mode-neutral review feedback (#1410, #1552): the Stream Deck diff dials and + * Scroll dial press a single `feedback-*` verb, and this module turns each + * chunk (whole file / hunk-under-cursor / selection) into an **authoring** + * gesture — it opens the native inline comment reply box at the anchor, the + * same surface as spec/plan comment authoring, so the reviewer types or + * dictates the actual comment. There is no promptless path: a gesture never + * stamps a placeholder body or force-forwards a bare ref (#1552 removed the old + * promptless deck default). * - * The queue branch mutates ONLY through `ReviewQueueStore` (the queue's single - * source of truth, #1037): the status bar, inline threads, and Tower's - * per-builder queued-feedback count all reflect a deck-driven enqueue for free. + * Deck composer parity (#1552, mirroring the artifact-canvas composer #1425): + * VS Code is the *diff-mode owner*, so it interprets the SAME verbs contextually + * — while a builder-review comment box is open, the content dials drive it: * - * The feedback always targets the builder whose diff is FOCUSED (the diff-inject - * entry's owner), never a separately-selected builder — a review comment must - * attach to the file in front of the reviewer. + * - hunk press → open-or-submit (open a hunk comment; press again to SUBMIT) + * - selection press → open-or-submit (open a selection comment; press again to SUBMIT) + * - file press → no-op while a box is open (open a whole-file comment when none is) + * + * DIAL CANCEL was DROPPED (ruling B, #1552): unlike the canvas composer — a React + * component that owns its draft and unmounts it on cancel — a native VS Code + * comment DRAFT has no safe programmatic discard (hideComment keeps the draft, + * closeActiveEditor closes the host editor, submit-empty is enablement-blocked). + * VS Code hands us the thread only on a click, so the discard is the visible + * **Cancel button** (`codev.cancelBuilderComment`). The thread-owning approach + * that would restore a dial cancel is the #1560 spike. + * + * The queue-vs-forward decision is separate and lives in the reply box's Submit + * (see `comments/builder-review.ts`): the box enqueues (comment mode) or forwards + * ref + prose (forward mode) per `codev.diffCodelensMode`. So the deck never + * infers either the composer action or the delivery mode; both are owned VS + * Code-side. + * + * The feedback always targets the builder whose diff is FOCUSED (the anchor is + * read from the active editor), never a separately-selected builder — a review + * comment must attach to the file in front of the reviewer. When no builder + * diff is focused, an OPEN gesture surfaces a clear message instead of a silent + * no-op, so a dial press over the wrong editor is legible. */ import * as vscode from 'vscode'; -import { randomUUID } from 'node:crypto'; -import * as path from 'node:path'; import { getDiffInjectEntry, - getDiffCodelensMode, + COMMENT_FOR_BUILDER_COMMAND, type DiffInjectSessionEntry, } from '../diff-inject-codelens.js'; -import { buildBuilderFileRef, buildBuilderRangeRef } from '../diff-inject-ref.js'; +import { + isBuilderComposerOpen, + submitActiveBuilderComposer, +} from '../comments/builder-review.js'; import { resolvePressCursorRef } from '../commands/press-cursor-ref.js'; -import { deriveWorktreePath } from './reconcile.js'; import type { LineRange } from './queue.js'; -import type { ReviewQueueStore } from './store.js'; -/** Body attached to a chunk flagged from the deck — a dial press carries no - * typed prose, so the comment's file + range are its substance. */ -const DECK_FLAG_BODY = 'Flagged for review from Stream Deck.'; +/** The three feedback gestures, one per Stream Deck axis. */ +export type FeedbackAxis = 'file' | 'hunk' | 'selection'; -export interface FeedbackDeps { - store: ReviewQueueStore; +/** What a feedback gesture does, given the axis and whether a composer box is + * already open. A discriminated union so the caller dispatches exhaustively. + * `cancel` is deliberately NOT in the dial vocabulary (ruling B, #1552): the + * native comment draft has no safe programmatic discard, so cancel is the + * visible Cancel button only. */ +export type FeedbackAction = + | { kind: 'open'; axis: FeedbackAxis } + | { kind: 'submit' } + | { kind: 'noop' }; + +/** + * Pure composer state machine (#1552, modeled on `decideApprovalRelay`): given + * the gesture's axis and whether a builder-review comment box is currently open, + * decide what the press does. No `vscode`, so the four branches are unit-tested + * directly — including the self-heal edge below. + * + * With NO box open, every axis simply opens a comment at that axis. With a box + * open, the content dials (hunk, selection) submit — so whichever dial opened + * the box, a second press submits — and the FILE dial is a defined no-op (its + * old cancel role was dropped, ruling B). No open runs while a box is open, so + * threads never stack. + * + * CANCEL-BIASED / never a phantom submit: this function only *names* the action; + * SUBMIT is executed via VS Code's built-in submit-comment, which is a no-op + * when no comment editor is focused. So if `composerOpen` is stale (a native + * Escape dismissed the box without notifying us), a press decides `submit` but + * the built-in no-ops — cancelled text is never resurrected — and the caller + * clears the flag, so the next press opens. A stale flag can cost a no-op or an + * extra open, never a phantom submit. + */ +export function decideFeedbackAction(axis: FeedbackAxis, composerOpen: boolean): FeedbackAction { + if (!composerOpen) { return { kind: 'open', axis }; } + if (axis === 'file') { return { kind: 'noop' }; } // dial cancel dropped (#1560 spike); Cancel button discards + return { kind: 'submit' }; // hunk or selection: open-or-submit } /** Where a feedback gesture points: the owning diff entry + range (null = whole file). */ @@ -97,38 +148,33 @@ function selectionAnchor(): Anchor | undefined { return { entry, lineRange: { start, end } }; } -/** Route one anchor per the workspace mode: forward now (PTY) or enqueue. */ -async function route(deps: FeedbackDeps, anchor: Anchor | undefined): Promise { - if (!anchor) { return; } - const { entry, lineRange } = anchor; - if (getDiffCodelensMode() === 'forward') { - // Immediate: the same low-level inject the forward CodeLens / commands use. - const ref = lineRange - ? buildBuilderRangeRef(entry.relPath, lineRange.start, lineRange.end) - : buildBuilderFileRef(entry.relPath); - await vscode.commands.executeCommand('codev.forwardToBuilder', entry.builderId, ref); +/** Resolve the anchor for an OPEN gesture (only the open branch needs one). */ +async function resolveAnchor(axis: FeedbackAxis): Promise { + if (axis === 'file') { return fileAnchor(); } + if (axis === 'hunk') { return hunkAnchor(); } + return selectionAnchor(); +} + +/** Run one feedback gesture: submit the open composer, or open the native comment + * reply box at the anchor for the reviewer to author. */ +async function gesture(axis: FeedbackAxis): Promise { + const action = decideFeedbackAction(axis, isBuilderComposerOpen()); + if (action.kind === 'submit') { await submitActiveBuilderComposer(); return; } + if (action.kind === 'noop') { return; } + const anchor = await resolveAnchor(axis); + if (!anchor) { + vscode.window.showWarningMessage('Codev: focus a builder diff first to flag it for review'); return; } - // Queue: register the builder's worktree from the diff entry (derived, never - // guessed) so the write lands in the right worktree even when nothing has been - // queued this session, then mutate through the store. - if (!deps.store.getWorktreePath(entry.builderId)) { - const worktree = deriveWorktreePath(entry.fsPath, entry.relPath, path.sep); - if (!worktree) { - vscode.window.showWarningMessage('Codev: could not locate the builder worktree for this diff'); - return; - } - deps.store.registerWorktree(entry.builderId, worktree); - } - await deps.store.add(entry.builderId, { - id: randomUUID(), - createdAt: new Date().toISOString(), - file: entry.relPath, - lineRange, - body: DECK_FLAG_BODY, - }); + const { entry, lineRange } = anchor; + // The same authoring entry point the comment codelens uses: it creates AND + // focuses the reply box at the anchor (the active editor is `entry.fsPath`, + // since the anchor was read from it) and marks the composer open. Submit + // delivers per the current mode. + await vscode.commands.executeCommand( + COMMENT_FOR_BUILDER_COMMAND, entry.builderId, entry.fsPath, entry.relPath, lineRange); } -export const feedbackFile = (deps: FeedbackDeps): Promise => route(deps, fileAnchor()); -export const feedbackHunk = async (deps: FeedbackDeps): Promise => route(deps, await hunkAnchor()); -export const feedbackSelection = (deps: FeedbackDeps): Promise => route(deps, selectionAnchor()); +export const feedbackFile = (): Promise => gesture('file'); +export const feedbackHunk = (): Promise => gesture('hunk'); +export const feedbackSelection = (): Promise => gesture('selection'); diff --git a/codev/plans/1552-vscode-review-flag-gestures-mu.md b/codev/plans/1552-vscode-review-flag-gestures-mu.md new file mode 100644 index 0000000000..22d17c9b81 --- /dev/null +++ b/codev/plans/1552-vscode-review-flag-gestures-mu.md @@ -0,0 +1,230 @@ +# PIR Plan: Review-flag gestures must author prose (native inline thread), no promptless default + +> ## Scope delta — owner-ruled at dev-approval (2026-08-26) +> +> **The scope below is SUPERSEDED in one respect.** The original plan assumed the native comment +> box's own Submit/Cancel (keyboard `Cmd+Enter` / `Escape`, like spec/plan authoring) was enough. +> Dev-approval testing showed it is not for the **deck-driven, dictation** workflow this feature +> exists for: a Stream Deck dial can *open* the box but nothing on the deck can submit or cancel it, +> because the diff-mode review dials — unlike the artifact-canvas composer (#1425) — have no +> submit/cancel gesture. Amr (owner), testing at the deck, ruled in-session at the dev gate: +> +> > "we need to achieve parity first, the implementation is currently unusable." +> +> **Approved expansion (Option A, architect-ruled with three conditions):** VS Code becomes the +> diff-mode *composer owner* and interprets the SAME `feedback-*` verbs contextually — while a +> builder-review box is open, **hunk press = open-or-submit, file press = cancel, selection = +> inert** — exactly mirroring the canvas composer. No deck or command-relay change; parity by +> architecture. The state machine is a pure, unit-tested function (`decideFeedbackAction`), it is +> cancel-biased (a stale-open flag can only cost a no-op or an extra open, never a phantom submit, +> because SUBMIT runs VS Code's built-in `editor.action.submitComment`, a no-op when nothing is +> focused), and the native-Escape staleness edge is documented as a bounded known limitation. +> Built-in ids were verified against the bundled workbench source: submit = `editor.action.submitComment` +> (NOT `workbench.action.submitComment`, which does not exist), cancel = `workbench.action.hideComment`. +> +> **Not built (recorded follow-up):** Option B — dedicated `submit-comment`/`cancel-comment` relay +> verbs + deck-lane wiring — is the cleaner long-term shape if Option A's Escape edge proves real in +> use. Deferred, not implemented here. +> +> No re-gate (precedent: pir-1494's owner-redirect at dev-approval): the dev gate itself is the +> checkpoint and stays pending Amr's re-test of the expanded build. Files stayed in-fence: +> `review-queue/feedback.ts` + `comments/builder-review.ts` (+ the 3-line `extension.ts` wiring). +> +> ### Delta 2 — Option A itself was corrected during implementation (ruling B, shipped) +> +> Option A above is **superseded in two respects** by what dev-approval testing forced (full record +> in `codev/reviews/1552-*.md`): +> 1. **`cancel = workbench.action.hideComment` is wrong** — it only *collapses* the widget (draft +> survives); `closeActiveEditor` closes the host editor. There is **no native discard**. Dial +> cancel was therefore **dropped** (ruling B): `decideFeedbackAction` is now `open | submit | noop` +> — with a box open the **file dial is a no-op**, not cancel. Discard is the visible **Cancel +> button** (`codev.cancelBuilderComment`, disposes the thread). Thread-owning dial-cancel = **#1560**. +> 2. **selection = inert** became **selection = open-or-submit** (mirrors the hunk dial, so whichever +> dial opened the box can also submit it). +> +> Composer-open state is tracked by OUR own flag (`composerOpen`, set only when this controller opens +> a box), **not** a focused-comment-input probe — a CMAP finding: keying off the focused comment URI +> would let a diff dial submit the plan/spec (`codev-review`) comment box. + +## Understanding + +The three review-flag gestures — `codev.feedbackCurrentFileToBuilder`, `-CurrentHunkToBuilder`, +`-SelectionToBuilder` (all in `review-queue/feedback.ts`, backing the Stream Deck `feedback-*` +verbs and any keybinding) — currently attach a **fixed, promptless body** to the flagged chunk and +never ask the reviewer for the actual comment: + +- **Queue mode** (`codev.diffCodelensMode === 'comment'`): `route()` enqueues a `PendingComment` + whose body is the hard-coded `DECK_FLAG_BODY = 'Flagged for review from Stream Deck.'` + (`feedback.ts:35`, used at `:128`) — a placeholder, and mislabeled "from Stream Deck" even when + the trigger is a keybinding. +- **Forward mode** (`'forward'`, the default): `route()` immediately injects a bare file/range ref + into the builder PTY via `codev.forwardToBuilder` with **no prose at all** (`feedback.ts:104-110`). + +Owner ruling (settled during the 1049 dev-review session, relayed in the architect kickoff): +**promptless flagging must not exist at all.** Every flag gesture must open a comment-authoring +input at the anchor, and the reviewer's typed/dictated text becomes the comment body. There is no +keep-the-default option. + +The authoring surface the codebase already uses for this is the **native inline comment thread +reply box** driven by `comments/builder-review.ts` (`createCommentController` + VS Code's +`workbench.action.addComment` + the `codev.submitBuilderComment` reply handler) — the same UX as +spec/plan comment authoring in `comments/plan-review.ts`. It is multi-line and dictation-friendly, +with Submit / Cancel / Edit (#1055) / Delete (#1037). Crucially, `builder-review.ts` already +exposes an authoring entry point — `COMMENT_FOR_BUILDER_COMMAND` (`codev.commentForBuilder`), whose +handler `openCommentInput(fsPath, range)` creates **and focuses** the reply box at a given anchor +(the comment-mode codelens already invokes it). What's missing is: (a) the flag gestures don't call +it, and (b) the submit handler always enqueues, so forward mode has no authored-prose path. + +## Proposed Change + +Move the **mode decision** out of `feedback.ts` (which currently pre-populates a placeholder or +force-forwards) and into the **submit** of the authoring thread. The flag gestures become a thin +"resolve anchor → open the native comment input" step, identical in both modes; the reviewer's +Submit then either enqueues (queue mode) or forwards ref + authored prose (forward mode). + +This reuses the exact authoring path the issue points at, keeps a single authoring surface, and +deletes `DECK_FLAG_BODY` and every promptless branch. + +### 1. `review-queue/feedback.ts` — flag gestures open the input, never stamp/force-forward + +`route(anchor)` collapses to: + +- **No anchor** (the focused editor is not a tracked builder diff — `activeEntry()` returns + `undefined`): show `vscode.window.showWarningMessage('Codev: focus a builder diff first to flag it + for review')` instead of the current **silent no-op** (Scenario 7 — a dial press over a + non-diff editor gives a clear message, not nothing). +- **Anchor present**: invoke the existing authoring entry point + `vscode.commands.executeCommand(COMMENT_FOR_BUILDER_COMMAND, entry.builderId, entry.fsPath, + entry.relPath, lineRange)`. That opens **and focuses** the native reply box at the anchor + (`openCommentInput` requires the active editor to be `fsPath` — always true here, since the anchor + was derived from the active editor, so the deck-focus/dictation ergonomics of Scenario 7 hold). + +The anchor resolvers are unchanged: `fileAnchor()` (whole file), `hunkAnchor()` (Scenario 4 — keeps +`resolvePressCursorRef`'s #1534 behavior: fresh single-file re-parse, hunk→symbol→file, and a +cursor on no changed line **degrades to whole-file with the existing status note**, never the old +"place the cursor in a changed hunk" error), and `selectionAnchor()`. All three route the same way, +so file / hunk / selection all prompt (Scenario 5). + +Because the mode branch and the store write leave this module, `feedback.ts` sheds `DECK_FLAG_BODY`, +`getDiffCodelensMode`, `buildBuilderFileRef`/`buildBuilderRangeRef`, `deriveWorktreePath`, +`randomUUID`, `path`, and the `FeedbackDeps { store }` dependency. The three exported functions lose +their `deps` parameter. + +### 2. `comments/builder-review.ts` — `codev.submitBuilderComment` becomes mode-aware + empty-guarded + +The reply handler (currently `builder-review.ts:283-302`, which always enqueues) gains: + +- **Empty / whitespace guard** (Scenario 3): `const text = reply.text.trim(); if (!text) { + reply.thread.dispose(); return; }` — an empty or whitespace-only submit queues and forwards + nothing and leaves no mounted thread. (Escape/Cancel already disposes the in-progress thread via + VS Code, so Cancel leaves no artifact for free.) +- **Forward mode** (`getDiffCodelensMode() === 'forward'`): build the ref from the thread's file + + range — `buildBuilderRangeRef(entry.relPath, start, end)` for a ranged thread, else + `buildBuilderFileRef(entry.relPath)` — and forward `ref + text` via + `vscode.commands.executeCommand('codev.forwardToBuilder', entry.builderId, ref + text)`, then + `thread.dispose()`. No queue entry (Scenario 2). The ref helpers already emit a trailing space + (`'src/a.ts:L5-L9 '`), so `ref + text` reads `@…:L5-L9 ` — exactly the "ref then prose" + shape the forward inject path was built for. Delivery uses the sanctioned #789 inject path + (`codev.forwardToBuilder` → `injectBuilderText`, no auto-Enter), matching every other forward verb. +- **Queue mode** (default `'comment'`): unchanged behavior — `registerEntryWorktree(entry)` then + `store.add(entry.builderId, { …, body: text })` (Scenario 1). The queued comment is a first-class + entry: editable (#1055) and deletable (#1037) before the batched "Submit Review (N)" flush + (Scenario 8 — no code change, it already is; we simply stop pre-seeding a placeholder). + +Multi-line + dictation (Scenario 6) is inherent to the comment reply box; no `showInputBox`. + +### 3. `extension.ts` — drop the now-unused `store` arg from the three registrations + +`feedback.ts:1260-1262` change from `() => feedbackFile({ store: reviewQueueStore })` to +`() => feedbackFile()` (and likewise hunk/selection). `reviewQueueStore` stays wired to the +builder-review controller (unchanged) and to `submitReview`/`discardReviewComments`. + +## Files to Change + +- `apps/vscode/src/review-queue/feedback.ts` — delete `DECK_FLAG_BODY`; rewrite `route()` to + warn-on-no-anchor + invoke `COMMENT_FOR_BUILDER_COMMAND`; drop the `store`/ref/uuid/path imports + and the `FeedbackDeps` param from `feedbackFile`/`feedbackHunk`/`feedbackSelection`. Anchor + resolvers unchanged. Update the module header comment (it currently describes the forward/enqueue + split that is moving out). +- `apps/vscode/src/comments/builder-review.ts` — `codev.submitBuilderComment`: add the + empty/whitespace guard and the forward-mode branch; import `getDiffCodelensMode` and the two ref + builders. Update the handler's doc comment. +- `apps/vscode/src/extension.ts:1260-1262` — drop the `{ store }` arg from the three `feedback*` + registrations. +- `apps/vscode/src/__tests__/feedback.test.ts` — rewrite: both modes and all three verbs now assert + a single `codev.commentForBuilder` invocation with the resolved anchor args (no direct + forward/enqueue); the no-builder-diff case asserts the warning; the #1534 fresh-parse and + degrade-to-file-with-note cases assert the anchor/range passed to `commentForBuilder` (and the + status note) rather than a queued body. `DECK_FLAG_BODY`/"Stream Deck" assertions removed. +- `apps/vscode/src/__tests__/builder-review-ranges.test.ts` (or a sibling + `builder-review-submit.test.ts` reusing its vscode mock) — add submit coverage: forward mode + forwards `ref + prose` and disposes with no `store.add`; queue mode `store.add`s the typed body; + empty/whitespace disposes with neither forward nor `store.add`. + +## Risks & Alternatives Considered + +- **Risk — the mode-aware submit also changes the gutter "+" / context-menu / comment-codelens + submit in forward mode** (they share the one `codev.submitBuilderComment`). Today those enqueue in + every mode. After this change, an authored comment submitted while in forward mode is *forwarded* + instead of queued. This is intentional and consistent with the mode contract ("forward mode = + deliver now; comment mode = queue"), and it keeps exactly one authoring surface with + mode-determined delivery. It is confined to the two in-scope files. If the architect wants the + gutter/context-menu paths to stay queue-only regardless of mode, the alternative below applies. + Called out here for the plan gate. +- **Alternative — tag only feedback-gesture threads as "forward intent"** so the gutter/context-menu + submit stays queue-only. Rejected: VS Code's stable Comments API gives no handle to the thread + `workbench.action.addComment` creates, so distinguishing gesture-opened threads at submit time + requires a fragile pending-anchor handshake. Reading the current mode at submit is deterministic + and simpler. +- **Alternative — `showInputBox` for the prose.** Rejected by the issue (Scenario 6): single-line, + truncates dictated paragraphs. The native thread reply is the required surface. +- **Risk — forward-mode delivery semantics.** Forward uses the existing no-auto-Enter inject + (`codev.forwardToBuilder`), so the ref + prose lands in the PTY prompt for the reviewer to send, + matching every other forward verb and the mailbox discipline (no direct PTY write, no forced + submit). Not a behavior regression; documented so the reviewer knows Enter is theirs to press. +- **Scope note.** No `apps/streamdeck` / `command-relay.ts` change — the deck already presses the + existing `feedback-*` verbs; the behavior change is entirely VS Code-side. No + `contextual-panel/*` or `OverviewCache` change (the pir-1553 sibling lane's set). No + `packages/types` / Tower reach. If implementation forces any of these, I stop and tell the + architect before proceeding. + +## Test Plan + +**Unit (vitest, run from the worktree):** + +- `apps/vscode/src/__tests__/feedback.test.ts` (rewritten): + - forward mode + comment mode, for file / hunk / selection: exactly one + `codev.commentForBuilder` executed with `[builderId, fsPath, relPath, expectedRange]` + (`null` for whole-file); nothing forwarded, nothing enqueued directly by `feedback.ts`. + - no active builder diff → the "focus a builder diff first" warning, and no + `commentForBuilder` invocation. + - #1534 preserved: a hunk press resolves against the fresh git parse (range reflected in the + `commentForBuilder` args); a cursor on no changed line degrades to a whole-file anchor + (`range === null`) with the "no changed lines at the cursor" status note and never the old + "place the cursor in a changed hunk" error. +- builder-review submit tests (new/extended): forward mode forwards `ref + prose` via + `codev.forwardToBuilder` and disposes the thread with no `store.add`; queue mode `store.add`s the + typed body; empty/whitespace submit disposes with neither forward nor `store.add`. +- `pnpm --filter @cluesmith/codev-vscode test` and `… check-types` green from the worktree. + +**Manual (Extension Development Host — this is the dev-approval evidence, run in the worktree):** + +1. Open a builder diff (`Codev: View Diff` / a builder file). Confirm the flag command path with + the Command Palette (`Codev: Flag Current File to Builder` / hunk / selection) or the bound key. +2. **Queue mode** (`codev.diffCodelensMode = 'comment'`): run each of file / hunk / selection → + the **native inline comment thread reply box opens and is focused** at the anchor (no + placeholder text). Type multi-line prose → **Submit** → it appears as a pending review comment + (status bar count +1, inline thread with Edit/Delete). **Cancel / empty submit** → nothing + queued, no thread left mounted. +3. **Forward mode** (`'forward'`): repeat → the same reply box opens; Submit injects `ref + prose` + into the builder PTY prompt; Cancel/empty injects nothing. +4. **No builder diff focused**: run a flag command over a plain file → the "focus a builder diff + first" warning, not a silent no-op. +5. **Promptless default gone**: `grep -rn "DECK_FLAG_BODY\|Flagged for review from Stream Deck" + apps/vscode/src` returns nothing; no gesture produces a comment without typed prose. + +**Cannot drive from the builder shell (named plainly per the evidence bar):** a *physical* Stream +Deck dial press. The dial only presses the `feedback-*` VS Code commands via `command-relay.ts`, +which are exactly what steps 1-4 exercise directly, so the VS Code-side behavior under test is fully +covered; the hardware-to-command relay is unchanged and out of scope. The reviewer runs steps 1-5 +against the running worktree at the dev-approval gate. diff --git a/codev/projects/1552-vscode-review-flag-gestures-mu/1552-review-iter1-rebuttals.md b/codev/projects/1552-vscode-review-flag-gestures-mu/1552-review-iter1-rebuttals.md new file mode 100644 index 0000000000..6e8ad0f73e --- /dev/null +++ b/codev/projects/1552-vscode-review-flag-gestures-mu/1552-review-iter1-rebuttals.md @@ -0,0 +1,58 @@ +# Rebuttal — review iteration 1 (#1552) + +Verdicts: **Gemini APPROVE**, **Codex REQUEST_CHANGES**, **Claude COMMENT**. Codex and Claude converged +on the same substantive findings; all four are legitimate and I **fixed all of them** (no +disagreement). Summary below, then per-point. + +## Codex REQUEST_CHANGES + +**1. `isBuilderComposerOpen()` treats any focused `comment`/`commentinput` editor as a builder +composer → a hunk/selection gesture could submit a plan-review / PR / other-extension comment via +`editor.action.submitComment`. "Track builder-owned composer state specifically and add a +cross-controller regression test."** + +FIXED. `isBuilderComposerOpen()` now reads **only this controller's own `composerOpen` flag** (set +solely when *we* open a box in `openCommentInput`). Removed the `isCommentInputFocused()` focus probe +from the union — the URI (`commentinput-` under the shared extension authority) cannot +distinguish our `codev-builder-review` controller from `plan-review.ts`'s `codev-review` controller, so +the probe was the leak. Submit stays reliable: the normal open→dictate→submit flow keeps the box +focused, and `editor.action.submitComment` is focus-gated host-side (a stale flag yields a no-op submit +or an extra open — never a phantom submit). Added a regression test in `builder-review-submit.test.ts`: +a focused `commentinput-…` editor with our flag clear now reads composer-CLOSED (a diff dial will not +submit a foreign plan/spec box). Residual, documented: an extremely narrow sequence (our box opened +*then* dismissed by native Escape leaving the flag stale-true, *then* a plan box focused, *then* a diff +dial pressed) could still submit the focused box — non-destructive (commits the user's own prose to its +intended place) and the same bounded native-Escape edge already recorded; #1560's thread-owning rework +removes it entirely. + +**2. Artifacts contradictory (plan says file=cancel/selection=inert; impl ships file=no-op/ +selection=submit; obsolete cancel comments; forward-mode button still says "Queue Comment for +Builder"). "Update the plan/review/source text and use a mode-neutral Submit label."** + +FIXED across the board: +- **Plan** (`codev/plans/1552-*.md`): added **Delta 2** to the scope-delta block recording ruling B — + dial cancel dropped (`open | submit | noop`, file dial = no-op), selection = open-or-submit, and the + `hideComment`/`closeActiveEditor` dead-ends. The plan no longer contradicts what shipped. +- **Source comments**: `feedback.test.ts` header rewritten (it still read "file = cancel, selection = + inert"); the `SUBMIT_FOCUSED_COMMENT` doc block no longer references the removed + `isCommentInputFocused()`; no obsolete cancel-executor references remain (grep-clean). +- **Button label**: the old single `"Queue Comment for Builder"` was misleading in the default + `forward` mode (Submit forwards to the PTY, it doesn't queue). Owner chose **mode-accurate labels** + over a neutral word: a second command `codev.forwardBuilderComment` ("Forward to Builder") shares + the one handler, and mutually-exclusive `when` clauses on `codev.diffCodelensMode` show **"Forward + to Builder"** in forward mode and **"Queue Comment for Builder"** in comment mode — matching the + codelens vocabulary exactly. The placeholder is neutral ("…then submit"). + +## Claude COMMENT (non-blocking; all addressed anyway) + +1. **Submit mislabeled in default forward mode** — FIXED (mode-neutral "Send to Builder" + placeholder). +2. **`isBuilderComposerOpen()` can't tell which comment controller is focused** — FIXED (Codex #1 above). +3. **Stale doc comment `feedback.test.ts:8-9`** — FIXED (header rewritten to the shipped vocabulary). +4. **Plan file doesn't record ruling B** — FIXED (plan Delta 2 above). + +## Verification + +`pnpm check-types` ✓, `eslint` ✓, `node esbuild.js` (build) ✓, `pnpm test:unit` ✓ (951 tests; +1 the new +cross-controller regression). `grep` for the removed symbols (`isCommentInputFocused`, +`cancelActiveBuilderComposer`, `DECK_FLAG_BODY`, `Queue Comment for Builder`) is empty. Gemini's APPROVE +stands; Codex's REQUEST_CHANGES and Claude's four items are all resolved in code and artifacts. diff --git a/codev/projects/1552-vscode-review-flag-gestures-mu/status.yaml b/codev/projects/1552-vscode-review-flag-gestures-mu/status.yaml new file mode 100644 index 0000000000..6c5fd41160 --- /dev/null +++ b/codev/projects/1552-vscode-review-flag-gestures-mu/status.yaml @@ -0,0 +1,30 @@ +id: '1552' +title: vscode-review-flag-gestures-mu +protocol: pir +phase: review +plan_phases: [] +current_plan_phase: null +gates: + plan-approval: + status: approved + requested_at: '2026-08-25T08:25:07.506Z' + approved_at: '2026-08-25T21:26:11.009Z' + dev-approval: + status: approved + requested_at: '2026-08-25T21:32:23.598Z' + approved_at: '2026-08-26T02:51:11.330Z' + pr: + status: approved + requested_at: '2026-08-26T03:08:24.783Z' + approved_at: '2026-08-26T03:22:50.801Z' +iteration: 1 +build_complete: true +history: [] +started_at: '2026-08-25T08:19:40.765Z' +updated_at: '2026-08-26T03:22:50.803Z' +pr_history: + - phase: review + pr_number: 1561 + branch: builder/pir-1552 + created_at: '2026-08-26T02:55:37.586Z' +pr_ready_for_human: false diff --git a/codev/resources/arch.md b/codev/resources/arch.md index dffe535a5e..1e4f5b8ba0 100644 --- a/codev/resources/arch.md +++ b/codev/resources/arch.md @@ -1216,6 +1216,8 @@ The VS Code extension (`apps/vscode`) is a thin client over Tower's existing API - **Builder review-comment queue (#1037)**: The builder diff carries two feedback surfaces that **never merge state**: #789's fire-and-forget forward-to-PTY injection, and a structured per-builder comment queue. Queued comments persist in `.builders//.codev/pending-comments.json` (worktree-local, so the queue survives reloads, cannot mix across builders, and dies with `afx cleanup`; the file is git-invisible via a managed block the extension appends to the repo's shared `$GIT_COMMON_DIR/info/exclude`, whose `.builder-*` family glob also silences spawn scaffolding files). `ReviewQueueStore` (`review-queue/store.ts`) is the single owner of those files — every surface (inline `codev-builder-review` comment threads, the status-bar `Submit Review (N)` counter, the submit/discard commands) reads through it and reacts to its `onDidChangeQueue` event; cross-window sync rides a debounced FileSystemWatcher with own-write echo suppression. Which surface the diff codelens offers is `codev.diffCodelensMode` (`forward` default preserves #789; the context menu always offers both). `Submit Review` packages the queue into one markdown message and types it into the builder PTY wrapped in bracketed-paste escapes (raw `\n` on the PTY would submit the prompt), deliberately without Enter — the human reviews and sends. The store's read + event API is the seam #1049's contextual panel modes will render from. - **Running-Tower version probe (#983)**: A second preflight dimension catches the case the CLI check structurally can't — an `npm install -g` upgrade that updated the on-disk binary but left **Tower running stale in-memory code**. Tower exposes read-only `GET /api/version` (`{ version, startedAt }`, wire type `TowerVersionInfo` in `codev-types`, served from `RouteContext` so it reports the *running* process's version, not the disk binary; unauthenticated like `/health`). On each `connected` transition the extension probes it (`TowerClient.getVersion()`, returning the raw `{ status }` so the preflight distinguishes a 404 "Tower too old to report" from an unreachable Tower). Divergence fires **only on `running < installedCLI`** — the case a restart actually fixes; running-vs-extension is left to #791 (a restart can't load code that isn't installed). The toast offers a `Restart Tower` action (`afx tower stop && afx tower start`, local host only — safe to self-invoke because #991 scoped `afx tower stop` to the listening process; remote hosts get informational wording). The two async inputs (installed-CLI version, running-Tower version) are reconciled against the startup race by re-probing once the CLI check resolves. Decision/wording logic is pure + unit-tested in `preflight-core.ts` (`decideTowerStatus`, `towerDivergenceMessage`). +- **Review-comment composer ownership — two surfaces, two owners (#1552)**: The two review-comment composers differ in who owns the *draft*, which decides what a remote (Stream Deck) driver can do to it. The **artifact-canvas** composer (spec/plan, `overlays/CommentComposer.tsx`) is our own React component holding the draft in local state — cancel unmounts it and destroys the draft, so the canvas composer verbs (`composer-open-or-submit` / `composer-cancel`) are **view-scoped** (they act regardless of editor focus). The **builder-diff review box** (`comments/builder-review.ts`) is VS Code's **native Comments widget**: the extension opens it via `workbench.action.addComment` (which focuses the reply input but returns **no thread handle**), and VS Code owns the draft. Consequence for deck "composer parity" (#1552): open and **submit** can mirror (submit via `editor.action.submitComment` — the `editor.*` id; `workbench.action.submitComment` does not exist — which is **focus-gated**, so it no-ops unless the box is the focused editor), but a dial **cancel cannot** — there is no native draft discard (`workbench.action.hideComment` only *collapses* the widget, the draft survives; `workbench.action.closeActiveEditor` closes the **host** editor). The reliable discard is the visible **Cancel button** (`codev.cancelBuilderComment`, disposing the thread VS Code hands us on click). So #1552 dropped dial-cancel (Files-dial is a defined no-op) and the deck vocabulary is `open | submit | noop`; **#1560** tracks the thread-owning rework (`createCommentThread` so the extension holds the handle → dispose on cancel) that would restore a deck cancel by making the box our-owned like the canvas one. **Composer-open state (`isBuilderComposerOpen`) is keyed off THIS controller's own flag, not a "focused comment input" probe** — `plan-review.ts` registers a *second* native comment controller (`codev-review`) that also opens `commentinput-…` editors, so a focus-based probe would let a diff-review dial submit an unrelated plan/spec comment (CMAP-caught in #1552). + ## Repository Dual Nature This repository has a unique dual structure: diff --git a/codev/resources/lessons-learned.md b/codev/resources/lessons-learned.md index bb52c1e68f..ed69e7b31b 100644 --- a/codev/resources/lessons-learned.md +++ b/codev/resources/lessons-learned.md @@ -660,5 +660,7 @@ Generalizable wisdom extracted from review documents, ordered by impact. Updated --- +- [From #1552] A built-in command id present in bundled source proves it **exists**, not what it **does** to your widget/state — bundle-presence is not behaviour-presence. Two architect seats endorsed a native-comment discard built on `workbench.action.closeActiveEditor` ("the only true discard — keep it"), reasoning from source reads; the owner's live re-test refuted it — it closed the *host editor* (focus jumped to another window), not the draft. What broke the deadlock was a captured runtime tracer (a scoped output channel logging each Stream Deck dial press) that recorded the actual behaviour: `editor.action.submitComment` no-ops unless the comment box is the *focused* editor; `hideComment` keeps the draft; there is no native discard. Verify a built-in's runtime behaviour against the live host before building a design on it, and treat two reviewers agreeing as *still unverified* until something actually runs it. Sibling to [From 799] — read the source, but the running host is ground truth; assert behaviour at the layer that actually fails. + *Last updated: 2026-04-17 (Maintenance run 0007 — v3.0.0 pre-release)* *Source: codev/reviews/* diff --git a/codev/reviews/1552-vscode-review-flag-gestures-mu.md b/codev/reviews/1552-vscode-review-flag-gestures-mu.md new file mode 100644 index 0000000000..709e0d4c6d --- /dev/null +++ b/codev/reviews/1552-vscode-review-flag-gestures-mu.md @@ -0,0 +1,178 @@ +# PIR Review: Review-flag gestures author prose in a native comment thread (no promptless default) + +Fixes #1552 + +## Summary + +The VS Code review-flag gestures (`codev.feedbackCurrentFileToBuilder` / `-CurrentHunkToBuilder` / +`-SelectionToBuilder`, driven by keybindings and Stream Deck dials) used to stamp a fixed placeholder +body (`DECK_FLAG_BODY = "Flagged for review from Stream Deck."`) and never ask the reviewer for the +actual comment. Owner-ruled that promptless flagging must not exist. This PR makes every gesture open +the **native inline comment reply box** at the anchor so the reviewer types/dictates real prose, which +becomes the comment body — enqueued (comment mode) or forwarded with the range ref (forward mode). It +then went further, under an owner ruling made during dev-approval testing: **deck composer parity**, so +a bare dial sequence (open → dictate → same dial submits) works hands-free, with cancel via the box's +visible **Cancel** button. + +## Files Changed + +(`git diff --stat` vs merge-base `b0c3b755`) + +- `apps/vscode/src/review-queue/feedback.ts` (+~90 / -~55) — gestures resolve an anchor then open the + native comment box; pure `decideFeedbackAction` composer state machine (open | submit | noop). +- `apps/vscode/src/comments/builder-review.ts` (+~110 / -~10) — mode-aware/empty-guarded Submit; + composer-open tracking (flag ∪ focused-comment-input detection); submit executor; visible Cancel + button command; `DECK_FLAG_BODY` deleted. +- `apps/vscode/src/extension.ts` (12) — drop the now-unused `store` arg from the three registrations; + refresh the block comment. +- `apps/vscode/package.json` (17) — Cancel button + menu wiring (Submit primary/right, Cancel + secondary/left); dropped the redundant `Codev:` title prefix on the two comment-box buttons. +- `apps/vscode/src/__tests__/feedback.test.ts`, `builder-review-submit.test.ts` (new), + `builder-review-ranges.test.ts`, `contributes-review-queue.test.ts` — decision machine, gesture + routing, composer lifecycle, verified built-in ids, menu scoping. + +## Commits + +Substantive commits (per-phase narrative lives in `codev/state/pir-1552_thread.md`): + +- `d75b96efe` Flag gestures author prose via native comment thread; mode-aware Submit +- `e23973940` Deck composer parity: context-aware feedback verbs drive the open box +- `9ed92b843` Add visible Cancel button to the builder-review comment box +- `2da54ca32` Fix reversed comment-box buttons + let selection dial submit +- `36f4aaa73` Fix dial submit/cancel: detect focused comment input; close (not hideComment) to cancel +- `c535b8f1e` Dial cancel: neutralize harmful closeActiveEditor → safe no-op +- `f6ee2e5d2` Ruling B: drop dial cancel from the vocabulary; button-only discard +- `82de2b748` Drop redundant "Codev:" prefix from the comment box buttons +- `135ddd3e3` Remove the dial-diag diagnostic tracer (root-cause complete) + +## Test Results + +- `pnpm check-types`: ✓ pass +- `pnpm exec eslint`: ✓ pass +- `node esbuild.js` (build): ✓ pass +- `pnpm test:unit`: ✓ pass (950 tests; ~30 new/rewritten across the four files above) +- Manual verification (owner, at the dev-approval gate, across the full parity arc on his Stream Deck + reviewing a live builder diff): flag gesture opens the native thread; typed prose becomes the comment + body and queues (verified 6 real comments in `/.codev/pending-comments.json`, no + placeholder); a bare dial open→dictate→same-dial-submit works; the Files dial is a harmless no-op; + the visible **Cancel** button discards leaving nothing queued; buttons read cleanly (Submit + primary/right, Cancel secondary/left). + +## The Design Journey (read this before the diff) + +This lane's value is as much in what was *rejected* as in what shipped. The arc, with its decision +points and refutations: + +**1. The core fix (plan-gate approved).** Gestures open the native comment reply box (reusing +`comments/builder-review.ts`' `codev.commentForBuilder` authoring entry). The queue-vs-forward decision +moved into the box's Submit. A flagged concern — the **mode-aware Submit unification** (in forward mode +the gutter "+"/context-menu/codelens Submit now forward too, not just the deck gestures) — was shown to +the owner with its concrete consequence and **approved at the plan gate** as a deliberate unification, +not a side effect. + +**2. Scope expanded at dev-approval (owner ruling).** Testing at the deck, the owner found the box +*opened* via a dial but nothing on the deck could submit or cancel it — unlike the artifact-canvas +composer (#1425), the diff-mode review dials had no submit/cancel gesture. His verbatim ruling: *"we +need to achieve parity first, the implementation is currently unusable."* The plan carries a +superseded-marker delta recording this. Architect approved **Option A** (VS Code, as the diff-mode +composer owner, interprets the same `feedback-*` verbs contextually) with three conditions: mirror the +canvas semantics; a pure cancel-biased state machine; document the native-Escape staleness edge. + +**3. Root-causing the dial via captured data.** Submit-then-cancel still failed. A temporary tracer +(routed to a "Codev Feedback Debug" output channel; since removed) captured the actual dial-press +sequence and established the failure was **in-fence** (commands reach the host; routing decides +correctly) — the VS Code **built-ins** were the problem. Three facts, all later bundle-confirmed: +`editor.action.submitComment` no-ops unless the comment box is the *focused* editor (and +`workbench.action.submitComment` does not exist); a focused comment box *is* the active editor as a +`commentinput-…` document (the detection lever); and there is **no native discard** for a draft. + +**4. The three dead ends for cancel (record for posterity).** Discarding a native comment draft +programmatically is impossible with the stable API: +- `workbench.action.hideComment` (what Esc binds to) only *collapses* the widget — the draft survives, + and a survived draft could later be resurrected and submitted = a phantom submit of cancelled text, + which the cancel-biased design forbids. +- `workbench.action.closeActiveEditor` closes the **host editor**, not the draft (observed: focus + jumped to a different VS Code window). Harmful; neutralized in `c535b8f1e`. +- submit-empty-as-discard (clear the input, then submit → our empty-body guard disposes) is blocked by + the submit button's `enablement: !commentIsEmpty` — an empty box has no enabled submit action to fire. + +**5. Two refutation ownerships (both architect seats were wrong; the empirical test was right).** The +`closeActiveEditor` discard was endorsed by both architect seats as "the only true discard — keep it," +built on treating **bundle-presence** of a command id as **behaviour**-presence. The owner's live +re-test refuted a claim two seats endorsed and none had run. Recorded exactly that way: the +three-dead-ends evidence plus the live re-test outranked both seats; the neutralization (`c535b8f1e`) +was the correct call. + +**6. Ruling B (shipped).** Cancel is removed from the *dial* vocabulary — `decideFeedbackAction` is now +purely `open | submit | noop`; with a box open, hunk & selection are open-or-submit and the Files dial +is a defined no-op. The reliable discard is the visible **Cancel** button (VS Code hands us the thread +only on a click → `codev.cancelBuilderComment` disposes it). Three grounds: the acceptance criteria are +met by prompting; the risk asymmetry of the thread-owning rework against a now-working submit/dictation +path; and the API offers no native discard today. Draft-survival (hide) was considered and rejected on +the phantom-submit ground. The thread-owning rework that would restore a dial cancel is filed as the +**#1560** spike. + +## Architecture Updates + +Routed one COLD arch fact to `codev/resources/arch.md` (§ VS Code Extension): the **two-composer +ownership asymmetry** across the review-comment surfaces — the artifact-canvas composer (spec/plan) is +our own React component that owns its draft (so the deck can open/submit/**cancel** it, view-scoped), +while the builder-diff review box is VS Code's native Comments widget whose draft VS Code owns (opened +via `addComment`, no thread handle returned → no programmatic discard; `editor.action.submitComment` is +focus-gated). This is the durable "why" behind ruling B and #1560. Not hot-tier: it's reference detail +for anyone touching those surfaces, not an always-injected invariant that would displace a capped hot +fact. + +## Lessons Learned Updates + +Routed one COLD lesson to `codev/resources/lessons-learned.md` (§ Debugging and Root Cause Analysis): +**a built-in command id present in bundled source proves it EXISTS, not what it DOES to your +widget/state** — verify a built-in's *runtime behaviour* (a captured tracer beat source-reading here) +before building a design on it; two architect seats endorsed a discard design on bundle-presence-as- +behaviour and the owner's live re-test refuted it. A refinement of the existing hot lessons ("captured +raw data beats speculation"; "verify claims against the actual file"), narrow enough for the cold tier +rather than displacing a capped hot lesson. + +## Things to Look At During PR Review + +- **`decideFeedbackAction` purity + the composer-open signal.** The dial vocabulary is `open | submit | + noop` (no cancel). `isBuilderComposerOpen()` reads **this controller's own `composerOpen` flag** (set + only when we open a box). It deliberately does **not** probe "is a comment input focused" — CMAP + (#1552) caught that a focus probe would let a diff-review dial submit the *plan/spec* review box + (`codev-review`, a second native comment controller on the same extension whose `commentinput-…` + URIs are indistinguishable). Submit stays reliable because `editor.action.submitComment` is + focus-gated host-side and the normal open→dictate→submit flow keeps the box focused; a stale flag + (native Escape) yields a no-op submit or an extra open, never a phantom submit. +- **Mode-aware Submit unification** (owner-approved at the plan gate): in forward mode the gutter "+" / + context-menu / codelens Submit forward too, not just the deck gestures. Intentional; called out so it + reads as design, not drift. +- **Native-Escape staleness edge** (documented, bounded): if the reviewer dismisses the box with the + keyboard Escape (not the Cancel button), our `composerOpen` flag can stale-stick true; the union with + `isCommentInputFocused()` and the no-op-on-unfocused submit built-in make the worst case a no-op or an + extra open — never a phantom submit. +- **Button ordering is host-behavioural.** Submit is `inline@1`, Cancel `inline@2`; empirically the + lower order renders rightmost/primary (the blue button that Enter/the submit dial fire). If a future + VS Code changes that ordering semantics, re-verify Submit stays primary. + +## How to Test Locally + +- **View diff**: VS Code sidebar → right-click builder `pir-1552` → **Review Diff**. +- **Run dev**: VS Code sidebar → **Run Dev**, or `afx dev pir-1552`. (Note: the Extension Development + Host loads the compiled `dist/extension.js` as-is — rebuild + reload the window to pick up changes.) +- **What to verify** (maps to the plan's Test Plan + the parity arc): + - Open a builder diff. Cursor on a changed hunk → press the flag gesture (palette `Codev: Flag …`, a + keybinding, or a Stream Deck dial) → the native inline reply box opens and is focused. + - Type multi-line prose → Submit → it queues as a pending review comment (status-bar `Submit Review + (N)` +1; inline thread with Edit/Delete). In forward mode it injects `ref + prose` into the builder + PTY instead. + - **Deck**: a bare open → dictate → **same dial** press submits; the **Files** dial is a no-op; the + visible **Cancel** button discards (box vanishes, nothing queued). + - No builder diff focused → a flag gesture shows "focus a builder diff first," not a silent no-op. + - `grep -rn "DECK_FLAG_BODY\|Flagged for review from Stream Deck" apps/vscode/src` → empty. + +## Follow-ups (not in scope; filed) + +- **#1560** — thread-owning dial-cancel spike (would restore a deck cancel by owning the comment thread + so it can be disposed, the way the canvas unmounts its composer). Deliberate decision-behind-spike. +- **#1559** — wire the contextual-panel "Code Review" surface to render the pending-comment queue; that + panel is currently a stub ("… will appear here (#1037)") and belongs to the contextual-panel lane. diff --git a/codev/state/pir-1552_thread.md b/codev/state/pir-1552_thread.md new file mode 100644 index 0000000000..2cc217219e --- /dev/null +++ b/codev/state/pir-1552_thread.md @@ -0,0 +1,274 @@ +# Builder thread — pir-1552 + +Issue #1552 — vscode: review-flag gestures must prompt for comment prose (native inline thread), +remove the promptless deck default. Protocol: PIR (strict). Files in scope: +`apps/vscode/src/review-queue/feedback.ts` + `apps/vscode/src/comments/builder-review.ts`. + +## Plan phase (2026-08-25) + +Investigated the flag-gesture path end to end: + +- `feedback.ts` `route()` today either force-forwards a bare ref (forward mode) or enqueues a + placeholder `DECK_FLAG_BODY = 'Flagged for review from Stream Deck.'` (:35, used :128). Both are + promptless. Owner ruled promptless flagging must not exist (relayed by architect; settled in the + 1049 dev-review session — no keep-the-default option). +- The authoring surface already exists: `builder-review.ts` exposes `COMMENT_FOR_BUILDER_COMMAND` + (`codev.commentForBuilder`) → `openCommentInput(fsPath, range)`, which creates + focuses the + native inline comment reply box at an anchor (the comment-mode codelens already uses it). Same UX + as spec/plan authoring (`plan-review.ts`). + +Design chosen: move the mode decision OUT of `feedback.ts` and INTO the submit handler. +- `feedback.ts`: resolve anchor → if none, warn "focus a builder diff first" (was a silent no-op); + else invoke `COMMENT_FOR_BUILDER_COMMAND`. Delete `DECK_FLAG_BODY` + store/ref/uuid deps. Anchor + resolvers unchanged (keeps #1534 `resolvePressCursorRef` degrade-to-file-with-note in `hunkAnchor` + — pir-1534 merged 93874894d touched this; read its role before editing). +- `builder-review.ts` `codev.submitBuilderComment`: add empty/whitespace guard (dispose, no + artifact) + forward-mode branch (build ref, forward `ref + prose` via `codev.forwardToBuilder`, + dispose, no queue) ; queue mode unchanged (enqueue typed body). +- `extension.ts:1260-1262`: drop the now-unused `{ store }` arg from the 3 registrations. + +Risk flagged at plan gate: the mode-aware submit also affects gutter "+" / context-menu submit in +forward mode (they share `codev.submitBuilderComment`) — intentional unification, called out for +Amr's decision. Alternative (tag only gesture threads) rejected: no stable-API handle to the +addComment-created thread. + +Scope fences respected: VS Code side only; no `apps/streamdeck`/`command-relay.ts`; no +`contextual-panel/*`/`OverviewCache` (pir-1553 sibling lane); no `packages/types`/Tower. + +Evidence bar (UX-rule change): dev-approval needs it seen running — native thread opens on gesture, +Submit queues typed prose, Cancel/empty queues nothing, promptless default provably gone (grep + +behaviour). Cannot drive a PHYSICAL Stream Deck dial from the builder shell; the dial only presses +the `feedback-*` VS Code commands (via `command-relay.ts`), which the manual EDH steps exercise +directly — named plainly in the plan. + +Plan written to `codev/plans/1552-vscode-review-flag-gestures-mu.md`, committed. Awaiting +plan-approval (Amr's gate; architect relays; I run porch approve). + +## Plan gate APPROVED + implement done (2026-08-26) + +Amr approved the plan as presented, including the flagged unification (gutter+/context-menu/codelens +Submit forwards in forward mode). Convention resolved: builder runs `porch approve` — I ran +`porch approve 1552 plan-approval`. Recorded the unification as owner-approved-at-plan-gate; will +note it in the review artifact (not a side effect). + +Implemented exactly as planned: +- `review-queue/feedback.ts`: `route()` → warn "focus a builder diff first" on no anchor, else + `executeCommand(COMMENT_FOR_BUILDER_COMMAND, ...)`. Deleted DECK_FLAG_BODY + store/ref/uuid deps; + gestures take no args. Anchor resolvers unchanged (kept #1534 hunkAnchor degrade-to-file-with-note). +- `comments/builder-review.ts` `codev.submitBuilderComment`: empty/whitespace → dispose no artifact; + forward mode → `forwardToBuilder(builderId, ref + body)` (ref has trailing space); comment mode → + enqueue trimmed body. Imports getDiffCodelensMode + buildBuilderFileRef/RangeRef. +- `extension.ts:1260-1262`: dropped `{ store }` arg; refreshed the block comment. + +Tests: rewrote feedback.test.ts (7 tests), added builder-review-submit.test.ts (5 tests). Affected +files: 4 files / 22 tests pass. check-types ✓, eslint ✓ on all changed files. AC grep for +DECK_FLAG_BODY / "Flagged for review from Stream Deck" is empty. + +RED RESOLVED (was stale-install, now green): the 20 import-time failures for +`@cluesmith/codev-sdk/reconnect-policy` were a stale worktree node_modules link (installed before +that subpath export landed on main) — same class as pir-1494's "Cannot find module three". Architect +diagnosed it; `pnpm install --frozen-lockfile` at the worktree root cleared it. Full apps/vscode +unit suite now 80 files / 935 tests all pass. Not a code issue; nothing in my diff changed for it. + +Fences held: only feedback.ts + builder-review.ts + 3-line extension.ts wiring + tests. No streamdeck, +no contextual-panel/OverviewCache, no types/Tower. + +Now at dev-approval gate (Amr's gate; evidence = native thread seen running on the gesture, Submit +queues/forwards typed prose per mode, Cancel/empty nothing, DECK_FLAG_BODY gone). + +## Scope EXPANDED at dev-approval — deck composer parity (2026-08-26) + +Amr, testing at the deck, found the box opens via a dial but nothing on the deck can submit/cancel +it (diff-mode review dials have no submit/cancel gesture, unlike the canvas composer #1425). He +ruled: "we need to achieve parity first, the implementation is currently unusable." Architect +approved Option A (VS Code = diff-mode composer owner, interprets the same feedback-* verbs +contextually) with 3 conditions: (a) mirror canvas exactly — hunk=open-or-submit, file=cancel, +selection=inert; (b) pure unit-tested cancel-biased state machine, never a phantom submit; (c) +document the Escape-staleness edge. No re-gate (pir-1494 precedent). Fences held (feedback.ts + +builder-review.ts only; no deck/relay change — parity by architecture). + +Key correction from architect (verified vs bundled workbench source): SUBMIT built-in is +`editor.action.submitComment` (editor.*), NOT `workbench.action.submitComment` (does NOT exist); +CANCEL is `workbench.action.hideComment` (confirmed). Both behind named constants, flagged for EDH +confirmation (bundle presence proves id exists, not exact focused-comment behaviour). + +Implemented: +- feedback.ts: pure `decideFeedbackAction(axis, composerOpen)` -> open|submit|cancel|noop + (modeled on decideApprovalRelay). gesture() reads isBuilderComposerOpen() and dispatches. +- builder-review.ts: module `composerOpen` (single source), set true in openCommentInput, cleared + in submit handler + both executors. Exports isBuilderComposerOpen / submitActiveBuilderComposer + (editor.action.submitComment) / cancelActiveBuilderComposer (workbench.action.hideComment). +- Tests: decideFeedbackAction (6 combos + stale-flag), gesture routing (open + submit/cancel/noop + branches), composer lifecycle + verified built-in ids + self-heal. Full apps/vscode suite 80 + files / 949 tests pass; check-types ✓, eslint ✓; AC grep empty. + +Plan-delta (superseded-marker) recorded at top of plan file per architect. Option B +(dedicated relay verbs + deck lane) noted as the cleaner follow-up, NOT built. + +Residual EDH-only evidence (named): the exact focused-comment behaviour of the two built-in ids, +and end-to-end dial submit/cancel — only the running host (VS Code EDH or the new Codev Desktop.app) +can confirm. Everything else unit-tested headlessly. dev gate stays pending Amr's re-test. + +## Visible Cancel button added (2026-08-26) + +Amr, testing, reported "the cancel button is still not there" (screenshot: box has Submit + trash +icon, no labelled Cancel). Root: my deck-parity work made the Files DIAL cancel (invisible), never +added a UI button. Added codev.cancelBuilderComment: click disposes the in-progress thread + clears +composerOpen (nothing queued/forwarded). package.json: command def + palette-hide + inline menu with +cancel@1 / submit@2 so Submit stays primary/last (the button Enter + deck-submit trigger). Updated +contributes-review-queue.test (builderCommands list) + builder-review-submit.test (cancel-button +handler). Pushed 9ed92b843. Full suite 80 files / 950 tests; check-types + eslint + build clean. + +HOST-VERIFY (named, can't drive headlessly): that VS Code renders Submit (not Cancel) as the primary +button so Enter/Changes-dial still SUBMIT after adding the second inline button — flagged to Amr. + +## Button reversal + selection-dial fixes (2026-08-26, commit 2da54ca32) + +Amr's screenshots: builder box rendered [Queue Comment grey/left] [Cancel blue/PRIMARY/right] — the +REVERSE of the target [Cancel white/left] [Comment blue/right]. My inline@N guess was backwards: +empirically @1 = rightmost+primary. Root-cause insight: because Cancel was primary, the deck submit +(editor.action.submitComment fires the PRIMARY action) was triggering CANCEL — explaining "press +again to submit doesn't work." Fixed: submit@1 (primary/right), cancel@2 (secondary/left). + +Also: Amr opens with the 3rd (Scroll/selection) dial and expects a 2nd press to submit; I'd made +selection inert-when-open. Changed decideFeedbackAction so the FILE dial is the sole cancel and every +other open dial (hunk, selection) is open-or-submit — whichever dial opened the box, 2nd press submits. +Updated feedback.test (selection now submits). Full suite 80 files / 950 tests; check-types+eslint+build clean. + +STILL TO VERIFY at deck (Amr): (1) Submit now blue/right, Enter+Changes/Scroll dial submit; (2) Files +dial cancel — uses workbench.action.hideComment; if it still doesn't discard, the reliable fallback is +the visible Cancel BUTTON (click = dispose, works), and I'll get the exact Esc-bound cancel command id +from the architect (bundle-verified) rather than guess again. + +## Queue path VERIFIED working; dial submit/cancel still failing → added transmission diagnostic (2026-08-26) + +Amr reviewing shannon builder 4392: pending-comments.json has 6 comments, each with real typed prose +(NO placeholder) — so the #1552 queue path WORKS end-to-end (submitted via button/Cmd+Enter). But deck +DIAL submit/cancel still fails; earlier he saw "focus a builder diff first" on the 2nd dial press = +gesture went to OPEN = composerOpen read false. Couldn't resolve by static reading (trace says it should +be true), and repeated build/reload confusion (EDH loads dist/extension.js as-is; package.json refreshes +on reload but compiled JS needs rebuild). + +Architect (both, via #1406 misroute + main relay): root-cause the transmission BEFORE proposing changes; +report WHERE it is — if command-relay.ts or apps/streamdeck, that's out of fence (route, don't reach). +Panel-body gap filed as #1559 (not this lane); contextual-panel fence re-confirmed. + +Added TEMP diagnostic (commit 5c5b5dbd2, revert before PR): traces every gesture + composer transition to +a "Codev Feedback Debug" output channel, tagged [dial-diag-v1] (also proves build freshness). + +## ROOT CAUSE FOUND from the trace (2026-08-26, fix 36f4aaa73) + +VS Code persists output channels to disk (…/Code/logs/…/exthost/output_logging_…/1-Codev Feedback Debug.log), +so I read Amr's actual dial-press trace. WHERE: IN MY FENCE (not relay/streamdeck — commands reach the host, +routing decides correctly). The VS Code BUILT-INS were the problem: +1. workbench.action.hideComment does NOT discard an in-progress comment box (trace: box stayed open, + activeEditor kept = …/commentinput-…md after exec). +2. editor.action.submitComment no-ops unless the comment box is the FOCUSED editor (trace: submit fired + with activeEditor=a non-box file → no codev.submitBuilderComment FIRED after). +3. Lever discovered: a focused comment box IS the active editor, as a `commentinput-…` document. + +FIX (36f4aaa73): isBuilderComposerOpen() = flag OR isCommentInputFocused() (live signal → stale-flag +recovery + submit only fires while box focused so the built-in hits it). Cancel closes the focused +comment-input editor via workbench.action.closeActiveEditor, GATED on isCommentInputFocused() so it can +never close a real file editor. Full suite 952 pass; check-types+eslint clean. + +Residual: closeActiveEditor-as-discard inferred from trace (gated, safe), awaits Amr's re-test to confirm +it visibly discards; asked architect to bundle-verify the exact Esc-bound discard id as the definitive +option. Reported root cause to architect. Waiting on Amr to restart debug session + re-capture the tracer. + +## Architect DESIGN RULING — discard stands (2026-08-26) [RECORD IN REVIEW] + +Main bundle-verified all three trace facts at source and established there is NO native discard command: +Esc IS workbench.action.hideComment and even used correctly only COLLAPSES the widget, draft surviving. +So closeActiveEditor-gated-on-comment-input-focus is not a workaround — it is the ONLY true discard. KEEP it. +Two verified reasons discard is correct (record in review; note hide-with-draft-survival as +considered-and-rejected, reason 2 the decider): + 1. Canvas parity: canvas composer-cancel → cancelComposer → setComposingLine(null) UNMOUNTS the composer, + destroying the draft (local React state). Same gesture, same meaning — dial-cancel discards in both modes. + 2. Condition (b) safety: a hidden-but-surviving draft could be resurrected + SUBMITTED by a later + open-or-submit press = phantom submit of cancelled text. Discard makes that structurally impossible. + +Canvas precedent: canvas verbs are VIEW-scoped, not focus-scoped ("a remote driver never moved focus into +the textarea", ArtifactCanvas.tsx:982-984). Native built-ins ARE focus-gated (submitComment no-ops without +comment-editor focus). So the OPEN path must leave focus IN the input for a bare open→submit dial sequence. +My open uses workbench.action.addComment, which focuses the reply input for RANGE comments (trace press 4: +activeEditor=commentinput after a range open). The focus-detection fix then makes submit fire only while +focused. Edge to watch: a hunk press that degrades to a whole-FILE comment (fileComment:true) — confirm on +re-test whether it also leaves focus in the input; if not, that whole-file deck case needs a focus nudge. +Amr can overrule discard at re-test (one-function swap to draft-survival) but discard is the presented design. + +## Re-test: SUBMIT works; CANCEL hits a hard native-API limit (2026-08-26, c535b8f1e) + +Amr re-tested: dial SUBMIT now works (focus-detection fix landed). Dial CANCEL via closeActiveEditor was +HARMFUL — closed the HOST editor, focus jumped to a different VS Code window. Neutralized to a harmless +no-op. All three discard routes are dead ends: hideComment collapses (draft survives, unsafe), closeActiveEditor +closes host (wrong), submit-empty blocked by the submit button's `enablement: !commentIsEmpty` (no enabled +submit action on an empty box). The ONE reliable discard is the visible Cancel BUTTON (VS Code hands us the +thread on click → codev.cancelBuilderComment disposes it); the deck can't invoke it (no thread arg), and +addComment never returns the thread (no onDidCreateCommentThread, no CommentThread.reveal). + +Asked architect to rule: (A) thread-owning rework — open via createCommentThread to get the handle for +dial-cancel dispose, but createCommentThread doesn't auto-focus (would regress dictation + focus-gated submit); +mitigation createCommentThread+addComment is unverified. (B) button-only cancel — ship dial open+submit + +Cancel BUTTON discard (fully working, zero risk), file thread-owning dial-cancel as a follow-up spike. My lean: B. +Awaiting ruling. Current state: submit-via-dial + Cancel-button-discard both work; dial-cancel is a safe no-op. + +## RULING B — dial cancel dropped, button-only discard (2026-08-26, f6ee2e5d2) + +Architect ruled B: ship button-only cancel. Applied — `decideFeedbackAction` is now purely open|submit|noop +(cancel removed from the dial vocabulary). Box open: hunk & selection = open-or-submit (verified load-bearing +path, untouched); FILE dial = defined no-op. Deleted the dead cancelActiveBuilderComposer executor. Visible +Cancel button (codev.cancelBuilderComment → dispose thread) is the sole discard. Confirmed the architect's +one point: Files-dial = defined no-op, never hide/close. Thread-owning dial-cancel = #1560 spike. 950 tests pass. + +REVIEW ARTIFACT must record (architect directive): the three-dead-ends dead-ends record; BOTH refutation +ownerships (main owned the closeActiveEditor-closes-host implication; the reviewer/relaying architect owned +building the discard ruling on bundle-presence-as-behaviour — Amr's live re-test refuted a claim two seats +endorsed and none ran; the empirical test + neutralization c535b8f1e outranked both seats); B's three grounds +(AC met by prompting; risk asymmetry of the thread-owning rework; API offers no native discard today); and +#1560 as deliberate-decision-behind-spike. + +Tracer REMOVED (135ddd3e3): logFeedbackDebug + "Codev Feedback Debug" channel + all call sites + test-mock +stubs stripped; grep for logFeedbackDebug/feedbackDbg/dial-diag empty; 950 tests pass. Also dropped the +redundant "Codev:" prefix from the comment-box buttons (82de2b748) → "Cancel" / "Queue Comment for Builder". +Production code now carries zero diagnostics. + +## DEV-APPROVAL approved → REVIEW phase (2026-08-26) + +Amr approved dev-approval (verbatim relayed by architect, ratifying the whole parity arc via his own deck +re-testing). Ran porch approve 1552 dev-approval (build+tests green). Wrote codev/reviews/1552-*.md with the +full arc (parity gap + verbatim ruling, superseded-plan delta, three-dead-ends + both refutation ownerships, +ruling B grounds, #1560/#1559). Routed 2 COLD governance updates: arch.md (VS Code Extension) two-composer +ownership asymmetry; lessons-learned.md (Debugging) bundle-presence≠behaviour. Review commit ec83838ad. +Next: push, open PR (--body-file the review), porch done --pr, then porch done triggers the single CMAP pass +(advisory, max_iterations 1) → pr gate. At pr gate: branch freezes before gate recorded, gate-record commit +is last write, silence through merge. If CI pends on known flakes (#1502-class browser job, #1558 Tower all-401) +tell architect, don't retry. + +## CMAP iter1 + fixes → PR GATE pending (2026-08-26, PR #1561) + +Verdicts: Gemini APPROVE, Codex REQUEST_CHANGES, Claude COMMENT. Both flagged the same 2 substantive +issues; I FIXED all 4 (rebuttal at 1552-review-iter1-rebuttals.md): +1. Cross-controller composer detection — isBuilderComposerOpen() unioned in isCommentInputFocused(), + so a diff dial could submit the plan/spec (codev-review) comment box. Now reads OUR composerOpen + flag only (removed the focus probe); added a cross-controller regression test. Residual: narrow + stale-flag+plan-box edge, non-destructive, documented (#1560 removes it). +2. Submit label "Queue Comment for Builder" misleading in default forward mode (forwards, not queues) + → mode-neutral "Send to Builder" + placeholder. +3. feedback.test.ts stale header vocabulary → rewritten. +4. Plan scope-delta still Option A (file=cancel/hideComment) → added Delta 2 recording ruling B. +Also updated arch.md (composer flag not a focus probe + cross-controller reason) + review Things-to-Look-At. +951 tests / check-types / eslint / build green. Commits f9880f77d; PR body refreshed. + +At PR GATE (pending). Notified architect with CMAP outcome. + +## Owner-directed label change at the gate (2026-08-26, c8bc36bff) + +Amr, reviewing at the gate, asked "is it send or forward?" — the neutral "Send to Builder" was too +vague. He chose mode-accurate labels (option B). Implemented: second command codev.forwardBuilderComment +("Forward to Builder") shares the one delivery handler; mutually-exclusive when clauses on +codev.diffCodelensMode show "Forward to Builder" in forward mode (default) and "Queue Comment for Builder" +in comment mode — matching the codelens vocabulary. Placeholder neutralized. +2 tests (mode-gating + +forward-command delivery); 953 pass, check-types/eslint/build green. Pushed, PR body refreshed, rebuttal +updated. Re-frozen. Still at pr gate pending Amr's merge decision. CI known-flaky watch: Artifact-Canvas +Browser + Tower Integration — will report, not retry.