From cf8512700cbaff16afd8bd6ff784b36eb9158643 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Thu, 6 Aug 2026 09:21:46 +1000 Subject: [PATCH 01/27] chore(porch): 1037 init pir --- .../status.yaml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 codev/projects/1037-vscode-codelens-driven-review-/status.yaml diff --git a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml new file mode 100644 index 000000000..96e5d4db6 --- /dev/null +++ b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml @@ -0,0 +1,18 @@ +id: '1037' +title: vscode-codelens-driven-review- +protocol: pir +phase: plan +plan_phases: [] +current_plan_phase: null +gates: + plan-approval: + status: pending + dev-approval: + status: pending + pr: + status: pending +iteration: 1 +build_complete: false +history: [] +started_at: '2026-08-05T23:21:45.893Z' +updated_at: '2026-08-05T23:21:45.893Z' From 76bb96457e892fdcc6a16cb46d571ec29f07b7eb Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Thu, 6 Aug 2026 09:29:21 +1000 Subject: [PATCH 02/27] [PIR #1037] Plan draft --- .../1037-vscode-codelens-driven-review-.md | 170 ++++++++++++++++++ codev/state/pir-1037_thread.md | 11 ++ 2 files changed, 181 insertions(+) create mode 100644 codev/plans/1037-vscode-codelens-driven-review-.md create mode 100644 codev/state/pir-1037_thread.md diff --git a/codev/plans/1037-vscode-codelens-driven-review-.md b/codev/plans/1037-vscode-codelens-driven-review-.md new file mode 100644 index 000000000..594906f05 --- /dev/null +++ b/codev/plans/1037-vscode-codelens-driven-review-.md @@ -0,0 +1,170 @@ +# PIR Plan: Codelens-Driven Review Comments in the Builder Diff (#1037) + +## Understanding + +#789 (PR #1023) shipped fire-and-forget PTY injection from the builder diff: codelenses on each changed file (file-level, per-symbol, per-changed-run) type a `path:L42-L58 ` reference into the builder's prompt buffer, and the reviewer keeps typing. This issue layers a second, structured surface on top: the reviewer composes paragraph-length comments in inline comment threads (VSCode `comments` API), the comments accumulate in a per-builder queue persisted at `.builders//.codev/pending-comments.json`, and a single `Submit Review` action packages the whole queue into one batched message written to the builder PTY's prompt buffer (no Enter; the human reviews and submits). + +**Scope boundary (per architect instruction at plan time):** this PIR is #1037 only. The capture surface (codelens + inline threads), the persistence layer, and the batched submit (palette command + status-bar button). The bottom-panel display surface belongs to #1049 and is NOT built here; no panel views, no `PanelModeContributor`. The deliverable is fully usable standalone. The queue store exposes a clean read + event API as the seam #1049 will consume later. + +Key codebase facts the design builds on: + +- The diff surfaces are `viewDiff` (multi-file `vscode.changes` editor) and `openBuilderFileDiff` (per-file `vscode.diff`), both in `apps/vscode/src/commands/view-diff.ts`. Right sides are plain `file:` docs inside the worktree, registered in the diff-inject registry (`apps/vscode/src/diff-inject-codelens.ts`) keyed by fsPath with `{builderId, relPath, hunks}`. +- Lens anchors come from pure helpers in `apps/vscode/src/diff-inject-ref.ts` (`buildAllLensDescriptors`): a file-level lens at line 0, per-symbol lenses, per-changed-run lenses. The issue's "file/hunk header" language maps onto these existing anchors; no new anchor computation is needed. +- CodeLens is suppressed by VSCode in the multi-file `vscode.changes` editor (pre-existing #789 limitation). There, the context menu (and the comment gutter "+") are the working affordances. Documented, unchanged. +- PTY injection is `TerminalManager.injectBuilderText` → `Terminal.sendText(text, false)` → `terminal-adapter.ts handleInput` → raw bytes over WebSocket to the Tower PTY. Raw `\n` bytes act as Enter in the Claude REPL, so multi-line submit needs bracketed-paste wrapping (design locked below, verified at dev-approval). +- `apps/vscode/src/comments/plan-review.ts` already uses `vscode.comments.createCommentController` with the edit/save/cancel/delete command pattern (#1055). The new controller mirrors it. Comments API surface used (`createCommentController`, `CommentThread`, `commentingRangeProvider`, `CommentMode`) is long-stable and already exercised in this codebase against the pinned engine (`^1.105`), so no newer API surface is required. + +## Locked Plan-Gate Decisions + +### 1. Codelens surface (locked by issue + sub-decisions) + +- Exactly one lens per anchor, matching `codev.diffCodelensMode`: `Comment for Builder (…)` in comment mode (default), `Forward to Builder (…)` in forward mode. +- **Setting**: `codev.diffCodelensMode`, enum `"comment" | "forward"`, default `"comment"`, declared in `package.json` configuration, written with `ConfigurationTarget.Workspace` (persists per workspace as the issue requires). +- **Title-bar toggle**: single action button following the established Issue-1104 pattern (VSCode has no pressed-state for toolbar buttons): two commands, exactly one visible via a `when` clause on a `codev.diffCodelensMode` context key kept in sync with the setting; each shows the icon + title of the mode clicking switches TO. Icons: `$(comment)` for "switch to comment mode", `$(terminal)` for "switch to forward mode". Shown only when `codev.activeEditorIsBuilderFile`. +- **No sidebar duplication** of the toggle: title-bar button + settings UI only (single canonical entry point). +- **Context menu**: `editor/context` on builder-diff files always shows BOTH `Codev: Comment for Builder` (new; uses selection if present, else cursor line) and the existing forward actions, regardless of mode. +- **Gutter "+"**: the comments API's native `commentingRangeProvider` is enabled on registered builder-diff right-side docs in comment mode. It is the line-precise entry the lens anchors can't give. In forward mode the provider returns no ranges so the two modes stay visually distinct. +- **Keybinding**: none added in v1 (`Cmd/Ctrl+K B` keeps injecting regardless of mode, untouched). A comment keybinding is a follow-up once verified unbound against the bundled defaults. + +### 2. Submit format: one markdown-sectioned message + +``` +Review feedback (3 comments): + +### packages/vscode/src/views/builders.ts:L42-L58 +the early return here is wrong because Y; suggest Z + +### apps/vscode/src/extension.ts:L1140 +... +``` + +Written to the prompt buffer as a single injection, wrapped in bracketed-paste escapes (`\x1b[200~` … `\x1b[201~`, with inner `\n` converted to `\r` to mimic a terminal paste) so the Claude REPL treats it as pasted buffer content rather than Enter presses. No trailing Enter: the human reviews and submits (deliberate human-in-the-loop step, per issue and architect instruction). + +Rejected: JSON envelope (requires builder-side parsing); multiple sequential PTY writes (N interleaving opportunities, and the REPL prompt state between writes is unknowable). + +### 3. Edit / delete of queued comments + +Per-comment **edit** and **delete** via the native comment UI, mirroring the #1055 pattern from `plan-review.ts` (edit/save/cancel/delete commands, `contextValue`-scoped menus). Plus a `Codev: Discard Review Comments` palette command that clears the active builder's queue (with confirm). No reorder (queue order = `createdAt`). + +### 4. No active terminal at submit time + +Fall through to the existing `openBuilderByRoleOrId(builderId, true)` open/recovery flow, then inject; warning toast on failure. Exactly matches #789's `codev.forwardToBuilder`. + +### 5. Replies + +None in v1. `thread.canReply = false` once a comment is queued; one comment per thread; changes go through edit. Replies-as-separate-comments is a follow-up. + +### 6. Thread persistence across reloads + +Re-mount when the diff is re-opened for that builder: a reconciler listens to the diff-inject registry change event (`onDidChangeDiffInjectRegistry`) plus queue-change events, and creates/disposes threads so that every queued comment whose file is currently registered has exactly one visible thread (keyed by comment id; anchor line clamped to document bounds). Queued comments for unopened files stay invisible until their diff opens. + +### 7. Sync mechanism: hybrid (in-process events + file watcher) + +- In-process: the store fires `onDidChangeQueue(builderId)` on every mutation it performs. Same-window surfaces (threads, status bar) update instantly. +- Cross-window / external: a `FileSystemWatcher` on `RelativePattern(workspaceRoot, '.builders/*/.codev/pending-comments.json')` with a **200ms debounce** catches other VSCode windows' writes and external deletes (`afx cleanup` removing the worktree). Watcher events re-read the file and fire the same in-process event. +- Own-write echo suppression: the store compares file content mtime/hash before firing from the watcher path. + +### 8. Gitignore mechanism (architect-flagged decision) + +**Chosen: a managed block in `$GIT_COMMON_DIR/info/exclude`, written idempotently by the extension when the store first creates a queue file.** Block content: + +``` +# codev: builder worktree local state (managed block) +.builder-* +.codev/pending-comments.json +``` + +Weighing the two options the architect named: + +- Committed `.gitignore` entry: one visible, versioned line. But codev is a framework: every adopter repo would need the same entry, which means a `codev update` migration touching user `.gitignore` files, and until adopted the file shows as untracked noise in every builder's `git status` (and trips "keep worktree clean" checks). Heavier for the same result. +- Spawn-time `.git/info/exclude` write: reaches every adopter automatically with zero committed footprint, and `info/exclude` lives in the shared common dir so ONE write covers all worktrees, past and future. But wiring it into spawn means the fix only applies at the next spawn with a new codev version, and this PR is extension-side. + +The chosen variant takes the info/exclude approach but writes from the extension at queue-file creation (the moment the file is born), so it works for already-spawned builders and keeps this PR self-contained. It also adopts the family glob `.builder-*`, which retroactively silences the accumulating scaffolding-file class (`.builder-prompt.txt`, `.builder-role.md`, `.builder-session-id`, `.builder-start.sh`; the glob does NOT match the `.builders/` directory itself). Downsides accepted: invisibility (mitigated by the comment header) and non-survival of a fresh clone (rewritten on next use). Open question for the gate: whether to also include `.claude/hooks/` (spawn-written, currently untracked in worktrees); left out for now to keep the exclude scope tight. + +### 9. Schema (v1) + +```jsonc +{ + "version": 1, + "builderId": "pir-859", + "comments": [ + { + "id": "", + "createdAt": "2026-08-06T10:00:00Z", + "file": "packages/vscode/src/views/builders.ts", // repo-relative + "lineRange": { "start": 42, "end": 58 }, // 1-based inclusive + "body": "markdown text" + } + ] +} +``` + +Changes vs the issue sketch: `diffContext` dropped for v1 (nothing consumes it; the parser tolerates unknown fields so it can return later without a version bump). Corrupt/unparseable file reads as empty without destroying the bytes; the file is only rewritten on the next mutation. + +## Proposed Change + +New modules (following the repo's pure-logic-vs-vscode-shell split, precedent `diff-inject-ref.ts`): + +1. **`apps/vscode/src/review-queue/queue.ts`** (pure, no `vscode` import): schema types, tolerant parse + serialize, mutation helpers (add/edit/remove/clear returning new state), `buildSubmitMessage(comments)` (the exact packaging format), `wrapBracketedPaste(text)`, and exclude-block helpers (`mergeExcludeBlock(existing)` idempotent). +2. **`apps/vscode/src/review-queue/store.ts`**: `ReviewQueueStore`. Resolves each builder's queue path from its `worktreePath` (authoritative, from the overview; not synthesized from workspace root). Read/write, `ensureExcludeEntry` on first write, `onDidChangeQueue` emitter, file watcher + debounce wiring, pending-count reads. Disposal-safe. +3. **`apps/vscode/src/comments/builder-review.ts`**: comment controller `codev-builder-review` ("Codev Builder Review"). `commentingRangeProvider` over diff-inject-registry files (comment mode only). Thread mount/reconcile against queue + registry state. Commands: submit-from-thread (queue add, thread flips to Preview, `canReply = false`), start-edit / save-edit / cancel-edit / delete (mirroring `plan-review.ts` #1055). +4. **`apps/vscode/src/review-queue/submit.ts`**: `codev.submitReview` implementation. Resolve target builder (active builder-diff file's owner; else, if exactly one builder has pending comments use it, else QuickPick among builders with non-empty queues). Package → open terminal via `openBuilderByRoleOrId` → multi-line inject → remove exactly the submitted comment ids from the queue (comments added mid-flight survive) → dispose their threads. `codev.discardReviewComments` with confirm lives here too. +5. **Status bar** (small module or in `extension.ts`): item `$(comment) Submit Review (N)`, command `codev.submitReview`, visible when the active editor is a builder-diff file whose builder has N > 0 pending comments; updates on queue events and active-editor changes. + +Changes to existing files: + +- **`apps/vscode/src/diff-inject-ref.ts`**: extend `LensDescriptor` with the underlying line range so the provider can build either command from one descriptor set (pure change, existing tests keep passing). +- **`apps/vscode/src/diff-inject-codelens.ts`**: provider reads `codev.diffCodelensMode`; comment mode emits the same anchors titled `Comment for Builder (…)` invoking `codev.commentForBuilder(builderId, fsPath, relPath, start, end)`; forward mode unchanged. Lens refresh on configuration change. +- **`apps/vscode/src/extension.ts`**: register new commands (`codev.commentForBuilder`, `codev.commentSelectionForBuilder`, `codev.submitReview`, `codev.discardReviewComments`, the two mode-toggle commands), sync the `codev.diffCodelensMode` context key, instantiate store + controller + status bar, activate the reconciler. +- **`apps/vscode/src/terminal-manager.ts`**: add `injectBuilderTextMultiline(builderId, text)` (bracketed-paste wrap; single-line callers untouched). +- **`apps/vscode/package.json`**: the setting; command declarations; `editor/title` toggle buttons; `editor/context` comment action (unconditional on mode, scoped by `codev.activeEditorIsBuilderFile`); `comments/*` menus scoped `commentController == codev-builder-review`; `commandPalette` scoping (toggles hidden, submit/discard visible). +- **`apps/vscode/CHANGELOG.md` + `docs/releases/UNRELEASED.md`**: user-facing entry, explicitly noting the #789 behavior change: the inject codelens is no longer shown by default (comment mode is the new default; forward mode is one title-bar toggle or context-menu click away; `Cmd/Ctrl+K B` unchanged). + +Explicitly NOT changed: `codev.forwardToBuilder` and all #789 flows (they never touch the queue), spawn code, porch, Tower, the committed `.gitignore`, anything panel-related (#1049). + +## Files to Change + +- `apps/vscode/src/review-queue/queue.ts` (new, pure) +- `apps/vscode/src/review-queue/store.ts` (new) +- `apps/vscode/src/review-queue/submit.ts` (new) +- `apps/vscode/src/comments/builder-review.ts` (new) +- `apps/vscode/src/diff-inject-ref.ts` (extend `LensDescriptor`) +- `apps/vscode/src/diff-inject-codelens.ts` (mode-aware lenses) +- `apps/vscode/src/terminal-manager.ts` (multi-line inject) +- `apps/vscode/src/extension.ts` (wiring, commands, status bar, context key) +- `apps/vscode/package.json` (setting, commands, menus) +- `apps/vscode/src/__tests__/…` (new tests, see Test Plan) +- `apps/vscode/CHANGELOG.md`, `docs/releases/UNRELEASED.md` + +## Risks & Alternatives Considered + +- **Risk: bracketed-paste injection through the Tower PTY chain is unverified against the live Claude REPL.** Mitigation: spike this first in the implement phase against a real builder terminal; it is also the first item in the dev-approval script. Fallback if it fails: write the packaged message to a worktree-local file and inject a one-line reference to it (would be raised at the gate as a deviation, since the issue asks for the message itself in the buffer). +- **Risk: CodeLens absent in the multi-file `vscode.changes` editor** (pre-existing VSCode behavior). The comment entry inherits it. Context menu + gutter "+" work there; per-file diffs get the full surface. Documented, matches #789. +- **Risk: line drift.** Queued anchors go stale as the builder keeps committing. v1 clamps to document bounds on re-mount and does not verify content (no `diffContext`). The submit message carries the recorded range; a drifted range is still useful prose context for the builder. +- **Risk: controller overlap.** Worktree files under `codev/plans|specs/` are eligible for BOTH the existing plan-review controller and the new one when opened via a builder diff. Both inputs may appear on "+". Accepted for v1; noted for a follow-up if confusing. +- **Risk: `editor/title` button visibility on diff editors** keys off the context key (window-scoped), so it can appear on a non-diff editor focused while a builder file was last active. The context key already drives #789's menus with the same semantics; accepted. +- **Alternative rejected: workspaceState (Memento) persistence** instead of the worktree-local file. Loses cross-window sync, doesn't travel with the worktree, and dies with the window's storage; the file matches the per-builder state convention (`codev/state/_thread.md`) and cleans up with `afx cleanup` for free. +- **Alternative rejected: extending #789's inject path to optionally queue.** The issue is explicit that the surfaces never merge state; a mode flag on one path invites exactly that. + +## Test Plan + +Unit (vitest, `apps/vscode/src/__tests__/`, mocked `vscode` where needed; pure modules tested directly): + +- `queue.ts`: parse/serialize round-trip; tolerant parse (unknown fields, corrupt JSON reads as empty); add/edit/remove/clear; `buildSubmitMessage` exact-string format; `wrapBracketedPaste` (escapes + `\n`→`\r`); exclude-block merge idempotence (`.builder-*` glob present once). +- `store.ts` (real fs in temp dirs): create-on-first-write; per-builder isolation (two builders, no cross-talk); missing/removed worktree dir handled (stale-builder queue); `ensureExcludeEntry` appended exactly once; own-write echo suppression. +- Codelens: provider emits exactly one entry per anchor with the mode-matching title/command; flips on configuration change. +- Contributes assertions (pattern: `contributes-commands.test.ts`): setting declared with default `"comment"`; both context-menu actions present without mode conditions; toggle buttons' `when` clauses mutually exclusive; comment menus scoped to `codev-builder-review`. +- Submit: resolves the active builder; calls open-then-inject in order; clears only submitted ids; warning path when no terminal materializes. +- Reconciler: mounts threads on registry event; no duplicate threads across repeated events; disposes on delete; clamps out-of-bounds anchors. + +Manual at dev-approval (the running-flow demonstration; UI change, so per the testing guide expectations the reviewer exercises the real flow): + +1. Spawn/attach two builders; open both diffs. Add 3 comments to each via codelens, gutter "+", and context menu. Confirm queues in the two `.builders//.codev/pending-comments.json` files stay isolated. +2. `git status` inside a builder worktree: pending-comments file AND the `.builder-*` scaffolding files no longer show as untracked. +3. Toggle to forward mode: lens flips to `Forward to Builder`, gutter "+" disappears, context menu still shows both actions, `Cmd/Ctrl+K B` still injects. Forward clicks never appear in any queue. Toggle persists across reload. +4. Reload the window; re-open one builder's diff: queued threads re-mount at their anchors. The other builder's queue survives untouched. +5. Edit one queued comment, delete another; confirm file + status bar counter follow. +6. Submit builder A via the status-bar button, builder B via the palette command. Verify each PTY receives the packaged multi-line message in the prompt buffer WITHOUT submitting (bracketed-paste check), terminal is focused, and the queue + threads + counter clear after the corresponding submit. +7. Close builder A's terminal, submit again with a fresh comment: the open-terminal recovery flow kicks in (decision 4). +8. Second VSCode window on the same workspace: add a comment in window 1, see window 2's counter update within the watcher debounce. +9. `afx cleanup` a finished builder: its queue file disappears with the worktree; no stale UI remains. diff --git a/codev/state/pir-1037_thread.md b/codev/state/pir-1037_thread.md new file mode 100644 index 000000000..2f9f2638a --- /dev/null +++ b/codev/state/pir-1037_thread.md @@ -0,0 +1,11 @@ +# Builder thread: pir-1037 + +Issue #1037: codelens-driven review comments in the unified diff editor (per-builder queue, batched submit to PTY). Protocol: PIR (strict). + +## 2026-08-06 Plan phase + +- Investigated #789's actual surface: lenses are file/symbol/changed-run anchored (`diff-inject-ref.ts`), not literal hunk headers; multi-file `vscode.changes` editor suppresses codelens (context menu is the affordance there). Comments API already used in `comments/plan-review.ts` (#1055 edit pattern) against the pinned engine, so no new API surface needed. +- Architect instruction (mid-turn, 2026-08-05): scope is #1037 ONLY, no #1049 panel views; deliverable standalone via palette command + status bar. Weigh gitignore mechanism explicitly. Preserve #789 semantics exactly. Submit stays human-in-the-loop (no auto-Enter). Verify comments API vs engines ^1.105. Plan for a running-flow demo at dev-approval. +- Key design find: PTY injection is raw bytes (`sendText` → `handleInput` → WebSocket), so a multi-line batched message would submit on every `\n` in the Claude REPL. Locked design: bracketed-paste wrapping (`\x1b[200~…\x1b[201~`, `\n`→`\r`); flagged as the top risk, spike first in implement, first item in the dev-approval script. +- Gitignore decision: managed block in `$GIT_COMMON_DIR/info/exclude` written by the extension at first queue write; family glob `.builder-*` also silences the scaffolding-file class; committed-.gitignore alternative weighed and documented in the plan. +- Plan written to `codev/plans/1037-vscode-codelens-driven-review-.md`; sitting at plan-approval gate. From ad164233c3b72ec6b2aa4a14f8658da0ab7abaf7 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Thu, 6 Aug 2026 09:29:29 +1000 Subject: [PATCH 03/27] chore(porch): 1037 plan-approval gate-requested --- codev/projects/1037-vscode-codelens-driven-review-/status.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml index 96e5d4db6..433c4b471 100644 --- a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml +++ b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml @@ -7,6 +7,7 @@ current_plan_phase: null gates: plan-approval: status: pending + requested_at: '2026-08-05T23:29:29.459Z' dev-approval: status: pending pr: @@ -15,4 +16,4 @@ iteration: 1 build_complete: false history: [] started_at: '2026-08-05T23:21:45.893Z' -updated_at: '2026-08-05T23:21:45.893Z' +updated_at: '2026-08-05T23:29:29.460Z' From 36bdc2ad05fee8b762bc141dc59238baf467b506 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Thu, 6 Aug 2026 10:15:41 +1000 Subject: [PATCH 04/27] chore(porch): 1037 plan-approval gate-approved --- .../projects/1037-vscode-codelens-driven-review-/status.yaml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml index 433c4b471..07fe48f86 100644 --- a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml +++ b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml @@ -6,8 +6,9 @@ plan_phases: [] current_plan_phase: null gates: plan-approval: - status: pending + status: approved requested_at: '2026-08-05T23:29:29.459Z' + approved_at: '2026-08-06T00:15:41.906Z' dev-approval: status: pending pr: @@ -16,4 +17,4 @@ iteration: 1 build_complete: false history: [] started_at: '2026-08-05T23:21:45.893Z' -updated_at: '2026-08-05T23:29:29.460Z' +updated_at: '2026-08-06T00:15:41.906Z' From a2ab7bdc095aca04f5885a2704a016d33b8a51e5 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Thu, 6 Aug 2026 10:15:49 +1000 Subject: [PATCH 05/27] chore(porch): 1037 implement phase-transition --- .../projects/1037-vscode-codelens-driven-review-/status.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml index 07fe48f86..f1dc48a0d 100644 --- a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml +++ b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml @@ -1,7 +1,7 @@ id: '1037' title: vscode-codelens-driven-review- protocol: pir -phase: plan +phase: implement plan_phases: [] current_plan_phase: null gates: @@ -17,4 +17,4 @@ iteration: 1 build_complete: false history: [] started_at: '2026-08-05T23:21:45.893Z' -updated_at: '2026-08-06T00:15:41.906Z' +updated_at: '2026-08-06T00:15:49.217Z' From 35b79381148210504c42f342498e4c1c4f9ea957 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Thu, 6 Aug 2026 10:19:10 +1000 Subject: [PATCH 06/27] [PIR #1037] Pure review-queue module: schema, packaging, bracketed paste, exclude block --- .../src/__tests__/review-queue-queue.test.ts | 164 +++++++++++++++++ apps/vscode/src/review-queue/queue.ts | 169 ++++++++++++++++++ 2 files changed, 333 insertions(+) create mode 100644 apps/vscode/src/__tests__/review-queue-queue.test.ts create mode 100644 apps/vscode/src/review-queue/queue.ts diff --git a/apps/vscode/src/__tests__/review-queue-queue.test.ts b/apps/vscode/src/__tests__/review-queue-queue.test.ts new file mode 100644 index 000000000..27efe06d6 --- /dev/null +++ b/apps/vscode/src/__tests__/review-queue-queue.test.ts @@ -0,0 +1,164 @@ +/** + * Pure queue helpers for the pending review-comment queue (#1037): schema + * round-trip and tolerant parsing, immutable mutations, the exact submit + * packaging format (plan decision 2), bracketed-paste wrapping (a raw `\n` on + * the PTY stdin would submit the builder's prompt mid-message), and the + * idempotent `info/exclude` managed block (plan decision 8). + */ + +import { describe, it, expect } from 'vitest'; +import { + addComment, + buildSubmitMessage, + editComment, + formatCommentRef, + mergeExcludeBlock, + parseQueueFile, + QUEUE_FILE_RELPATH, + removeComments, + serializeQueueFile, + wrapBracketedPaste, + type PendingComment, +} from '../review-queue/queue.js'; + +function comment(overrides: Partial = {}): PendingComment { + return { + id: 'id-1', + createdAt: '2026-08-06T10:00:00Z', + file: 'packages/foo/src/bar.ts', + lineRange: { start: 42, end: 58 }, + body: 'the early return here is wrong', + ...overrides, + }; +} + +describe('parse/serialize round-trip', () => { + it('round-trips a queue through serialize + parse', () => { + const comments = [comment(), comment({ id: 'id-2', lineRange: null, body: 'whole-file note' })]; + const raw = serializeQueueFile('pir-859', comments); + expect(parseQueueFile(raw)).toEqual(comments); + }); + + it('records version and builderId in the on-disk shape', () => { + const parsed = JSON.parse(serializeQueueFile('pir-859', [])); + expect(parsed.version).toBe(1); + expect(parsed.builderId).toBe('pir-859'); + expect(parsed.comments).toEqual([]); + }); +}); + +describe('tolerant parsing', () => { + it('reads corrupt JSON as empty', () => { + expect(parseQueueFile('{not json')).toEqual([]); + expect(parseQueueFile('')).toEqual([]); + }); + + it('reads wrong top-level shapes as empty', () => { + expect(parseQueueFile('null')).toEqual([]); + expect(parseQueueFile('[]')).toEqual([]); + expect(parseQueueFile('{"comments": "nope"}')).toEqual([]); + }); + + it('drops malformed entries but keeps valid ones', () => { + const raw = JSON.stringify({ + version: 1, + builderId: 'x', + comments: [comment(), { id: '', body: 'missing everything' }, 42], + }); + expect(parseQueueFile(raw)).toEqual([comment()]); + }); + + it('ignores unknown fields on entries (forward compatibility)', () => { + const raw = JSON.stringify({ + version: 1, + builderId: 'x', + comments: [{ ...comment(), diffContext: '@@ -1 +1 @@' }], + }); + const parsed = parseQueueFile(raw); + expect(parsed).toHaveLength(1); + expect(parsed[0]!.id).toBe('id-1'); + }); +}); + +describe('mutations', () => { + it('add appends without mutating the input', () => { + const base = [comment()]; + const next = addComment(base, comment({ id: 'id-2' })); + expect(next.map(c => c.id)).toEqual(['id-1', 'id-2']); + expect(base).toHaveLength(1); + }); + + it('edit replaces only the matching body', () => { + const base = [comment(), comment({ id: 'id-2' })]; + const next = editComment(base, 'id-2', 'revised'); + expect(next[0]!.body).toBe('the early return here is wrong'); + expect(next[1]!.body).toBe('revised'); + }); + + it('removeComments drops exactly the given ids', () => { + const base = [comment(), comment({ id: 'id-2' }), comment({ id: 'id-3' })]; + const next = removeComments(base, ['id-1', 'id-3']); + expect(next.map(c => c.id)).toEqual(['id-2']); + }); +}); + +describe('submit packaging', () => { + it('formats range, single-line, and whole-file refs', () => { + expect(formatCommentRef('a/b.ts', { start: 42, end: 58 })).toBe('a/b.ts:L42-L58'); + expect(formatCommentRef('a/b.ts', { start: 7, end: 7 })).toBe('a/b.ts:L7'); + expect(formatCommentRef('a/b.ts', null)).toBe('a/b.ts'); + }); + + it('builds the exact sectioned message', () => { + const msg = buildSubmitMessage([ + comment({ body: 'first note\nwith a second line' }), + comment({ id: 'id-2', file: 'apps/x.ts', lineRange: { start: 7, end: 7 }, body: 'second note' }), + ]); + expect(msg).toBe( + 'Review feedback (2 comments):\n\n' + + '### packages/foo/src/bar.ts:L42-L58\nfirst note\nwith a second line\n\n' + + '### apps/x.ts:L7\nsecond note\n', + ); + }); + + it('uses the singular for one comment', () => { + expect(buildSubmitMessage([comment()])).toMatch(/^Review feedback \(1 comment\):/); + }); +}); + +describe('wrapBracketedPaste', () => { + it('wraps in paste escapes and converts newlines to carriage returns', () => { + expect(wrapBracketedPaste('a\nb\r\nc')).toBe('\x1b[200~a\rb\rc\x1b[201~'); + }); +}); + +describe('mergeExcludeBlock', () => { + it('appends the managed block to existing content', () => { + const merged = mergeExcludeBlock('node_modules/\n'); + expect(merged).toBe( + 'node_modules/\n' + + '# codev: builder worktree local state (managed block)\n' + + '.builder-*\n' + + `${QUEUE_FILE_RELPATH}\n`, + ); + }); + + it('adds a separating newline when existing content lacks one', () => { + expect(mergeExcludeBlock('node_modules/')).toMatch(/^node_modules\/\n# codev/); + }); + + it('is idempotent: returns null when the block is already present', () => { + const merged = mergeExcludeBlock(''); + expect(merged).not.toBeNull(); + expect(mergeExcludeBlock(merged!)).toBeNull(); + }); + + it('the family glob does not cover the .builders directory itself', () => { + // `.builder-*` requires a dash after "builder": scaffolding files match, + // the worktree parent directory `.builders` must not. + const glob = /^\.builder-.*$/; + expect('.builder-prompt.txt').toMatch(glob); + expect('.builder-session-id').toMatch(glob); + expect('.builders').not.toMatch(glob); + }); +}); diff --git a/apps/vscode/src/review-queue/queue.ts b/apps/vscode/src/review-queue/queue.ts new file mode 100644 index 000000000..5cbf75f59 --- /dev/null +++ b/apps/vscode/src/review-queue/queue.ts @@ -0,0 +1,169 @@ +/** + * Pure helpers for the per-builder pending review-comment queue (#1037). No + * `vscode` import, same precedent as `diff-inject-ref.ts`, so the schema, + * packaging, and exclude-block logic are unit-tested directly. + * + * The queue is the structured counterpart to #789's fire-and-forget PTY + * injection: comments composed in inline threads accumulate here and reach the + * builder only via a batched Submit Review. The two surfaces never merge + * state: nothing in this module is touched by the forward flow. + * + * On-disk location: `/.codev/pending-comments.json`, one file per + * builder, living inside the builder's worktree so it survives reloads, + * cannot mix with another builder's queue, and is removed with the worktree + * by `afx cleanup`. + */ + +/** 1-based inclusive line range a comment is anchored to. */ +export interface LineRange { + start: number; + end: number; +} + +export interface PendingComment { + /** Stable identity (crypto.randomUUID) used for edit/remove and thread keys. */ + id: string; + /** ISO timestamp; also the queue's ordering key (creation order, no reorder). */ + createdAt: string; + /** Repo-relative path of the commented file. */ + file: string; + /** Anchor range; null means the comment is about the whole file. */ + lineRange: LineRange | null; + /** Markdown body as typed in the comment thread. */ + body: string; +} + +/** On-disk shape of `/.codev/pending-comments.json`. */ +export interface PendingCommentsFile { + version: 1; + builderId: string; + comments: PendingComment[]; +} + +/** Queue file path relative to the builder's worktree root. */ +export const QUEUE_FILE_RELPATH = '.codev/pending-comments.json'; + +/** + * Parse a queue file's raw bytes into its comment list. Tolerant by design: + * corrupt JSON, a wrong top-level shape, or malformed entries read as an + * empty/partial list rather than throwing, and unknown fields are ignored + * (so a future field like `diffContext` can appear without a version bump). + * The caller never rewrites the file on a failed parse — only the next + * mutation writes, so bad bytes are preserved for inspection until then. + */ +export function parseQueueFile(raw: string): PendingComment[] { + let data: unknown; + try { + data = JSON.parse(raw); + } catch { + return []; + } + if (typeof data !== 'object' || data === null) { return []; } + const comments = (data as { comments?: unknown }).comments; + if (!Array.isArray(comments)) { return []; } + return comments.filter(isValidComment); +} + +function isValidComment(value: unknown): value is PendingComment { + if (typeof value !== 'object' || value === null) { return false; } + const c = value as Record; + if (typeof c.id !== 'string' || c.id === '') { return false; } + if (typeof c.createdAt !== 'string') { return false; } + if (typeof c.file !== 'string' || c.file === '') { return false; } + if (typeof c.body !== 'string') { return false; } + if (c.lineRange === null || c.lineRange === undefined) { return true; } + const r = c.lineRange as Record; + return typeof r.start === 'number' && typeof r.end === 'number'; +} + +export function serializeQueueFile(builderId: string, comments: PendingComment[]): string { + const file: PendingCommentsFile = { version: 1, builderId, comments }; + return JSON.stringify(file, null, 2) + '\n'; +} + +// ── Queue mutations (immutable — return a new list) ───────────────────── + +export function addComment(comments: PendingComment[], comment: PendingComment): PendingComment[] { + return [...comments, comment]; +} + +export function editComment(comments: PendingComment[], id: string, body: string): PendingComment[] { + return comments.map(c => { + if (c.id === id) { return { ...c, body }; } + return c; + }); +} + +export function removeComments(comments: PendingComment[], ids: readonly string[]): PendingComment[] { + const gone = new Set(ids); + return comments.filter(c => !gone.has(c.id)); +} + +// ── Submit packaging ──────────────────────────────────────────────────── + +/** `path:L42-L58`, `path:L42` for a single line, or bare `path` for whole-file. */ +export function formatCommentRef(file: string, range: LineRange | null): string { + if (!range) { return file; } + if (range.start === range.end) { return `${file}:L${range.start}`; } + return `${file}:L${range.start}-L${range.end}`; +} + +/** + * Package the queue into the single batched message written to the builder's + * prompt buffer (plan decision 2): a count header, then one `###` section per + * comment in creation order. The caller wraps it for the PTY with + * `wrapBracketedPaste` — this function returns plain markdown. + */ +export function buildSubmitMessage(comments: readonly PendingComment[]): string { + let noun = 'comments'; + if (comments.length === 1) { noun = 'comment'; } + const sections = comments.map( + c => `### ${formatCommentRef(c.file, c.lineRange)}\n${c.body.trim()}`, + ); + return `Review feedback (${comments.length} ${noun}):\n\n${sections.join('\n\n')}\n`; +} + +/** + * Wrap a multi-line message in bracketed-paste escapes so the builder's REPL + * treats it as pasted buffer content instead of typed keys — a raw `\n` on the + * PTY's stdin acts as Enter and would submit the prompt mid-message. Inner + * newlines become `\r` to match what a terminal emulator emits when pasting + * (xterm.js converts `\n` to `\r` on paste; Claude Code's REPL expects that + * form inside a paste block). + */ +export function wrapBracketedPaste(text: string): string { + return `\x1b[200~${text.replace(/\r?\n/g, '\r')}\x1b[201~`; +} + +// ── git info/exclude managed block (plan decision 8) ──────────────────── + +/** + * The managed ignore block appended to `$GIT_COMMON_DIR/info/exclude` when the + * first queue file is created. `info/exclude` lives in the shared common dir, + * so one write covers every worktree of the repo without a committed + * `.gitignore` change in adopter repos. The `.builder-*` family glob also + * silences the spawn scaffolding files (`.builder-prompt.txt`, + * `.builder-role.md`, `.builder-session-id`, `.builder-start.sh`); it does NOT + * match the `.builders/` directory (the glob requires a dash after "builder"). + */ +export const EXCLUDE_BLOCK_LINES = [ + '# codev: builder worktree local state (managed block)', + '.builder-*', + QUEUE_FILE_RELPATH, +] as const; + +/** + * Merge the managed block into an existing `info/exclude` content. Returns the + * new content to write, or null when the block (keyed on the queue-file line) + * is already present — the idempotence check, so repeated queue writes never + * duplicate the block. + */ +export function mergeExcludeBlock(existing: string): string | null { + const present = existing + .split('\n') + .some(line => line.trim() === QUEUE_FILE_RELPATH); + if (present) { return null; } + let prefix = existing; + if (prefix !== '' && !prefix.endsWith('\n')) { prefix += '\n'; } + return prefix + EXCLUDE_BLOCK_LINES.join('\n') + '\n'; +} From 9e15670a6ff3e1603d12700c97fcd9b0679ea2fc Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Thu, 6 Aug 2026 10:20:54 +1000 Subject: [PATCH 07/27] [PIR #1037] ReviewQueueStore: per-builder fs persistence, watcher sync, info/exclude block --- .../src/__tests__/review-queue-store.test.ts | 197 ++++++++++++++ apps/vscode/src/review-queue/store.ts | 252 ++++++++++++++++++ 2 files changed, 449 insertions(+) create mode 100644 apps/vscode/src/__tests__/review-queue-store.test.ts create mode 100644 apps/vscode/src/review-queue/store.ts diff --git a/apps/vscode/src/__tests__/review-queue-store.test.ts b/apps/vscode/src/__tests__/review-queue-store.test.ts new file mode 100644 index 000000000..b302fcb8f --- /dev/null +++ b/apps/vscode/src/__tests__/review-queue-store.test.ts @@ -0,0 +1,197 @@ +/** + * ReviewQueueStore against a real filesystem (temp dirs): create-on-first- + * write, per-builder isolation, stale/missing worktree handling, the + * `info/exclude` managed-block write (once, idempotent), and watcher echo + * suppression (#1037). `vscode` is mocked minimally; fs and git are real — + * each temp worktree is a real `git init` repo so the exclude path resolves + * through `git rev-parse --git-common-dir` exactly as in production. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'node:fs/promises'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { execFileSync } from 'node:child_process'; + +vi.mock('vscode', () => { + 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 {} + } + class RelativePattern { + constructor(public base: string, public pattern: string) {} + } + return { + EventEmitter, + RelativePattern, + workspace: { + createFileSystemWatcher: vi.fn(() => ({ + onDidCreate: vi.fn(() => ({ dispose() {} })), + onDidChange: vi.fn(() => ({ dispose() {} })), + onDidDelete: vi.fn(() => ({ dispose() {} })), + dispose: vi.fn(), + })), + }, + }; +}); + +const { ReviewQueueStore } = await import('../review-queue/store.js'); +const { QUEUE_FILE_RELPATH } = await import('../review-queue/queue.js'); + +let tmpRoot: string; + +function makeComment(id: string, file = 'src/a.ts') { + return { + id, + createdAt: '2026-08-06T10:00:00Z', + file, + lineRange: { start: 1, end: 2 }, + body: `note ${id}`, + }; +} + +/** A real git repo posing as a builder worktree. */ +async function makeWorktree(name: string): Promise { + const dir = path.join(tmpRoot, name); + await fs.mkdir(dir, { recursive: true }); + execFileSync('git', ['-C', dir, 'init', '-q']); + return dir; +} + +beforeEach(async () => { + tmpRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'review-queue-')); +}); + +afterEach(async () => { + await fs.rm(tmpRoot, { recursive: true, force: true }); +}); + +describe('ReviewQueueStore', () => { + it('creates the queue file on first add and round-trips through load', async () => { + const wt = await makeWorktree('pir-1'); + const store = new ReviewQueueStore(undefined); + store.registerWorktree('pir-1', wt); + + await store.add('pir-1', makeComment('c1')); + const onDisk = JSON.parse(await fs.readFile(path.join(wt, QUEUE_FILE_RELPATH), 'utf8')); + expect(onDisk.builderId).toBe('pir-1'); + expect(onDisk.comments).toHaveLength(1); + + const fresh = new ReviewQueueStore(undefined); + fresh.registerWorktree('pir-1', wt); + expect(await fresh.load('pir-1')).toEqual([makeComment('c1')]); + store.dispose(); + fresh.dispose(); + }); + + it('keeps two builders’ queues isolated', async () => { + const wtA = await makeWorktree('pir-a'); + const wtB = await makeWorktree('pir-b'); + const store = new ReviewQueueStore(undefined); + store.registerWorktree('pir-a', wtA); + store.registerWorktree('pir-b', wtB); + + await store.add('pir-a', makeComment('a1')); + await store.add('pir-b', makeComment('b1')); + await store.add('pir-a', makeComment('a2')); + + expect(store.getComments('pir-a').map(c => c.id)).toEqual(['a1', 'a2']); + expect(store.getComments('pir-b').map(c => c.id)).toEqual(['b1']); + expect(store.buildersWithPending().sort()).toEqual(['pir-a', 'pir-b']); + store.dispose(); + }); + + it('edit and remove persist through the file', async () => { + const wt = await makeWorktree('pir-1'); + const store = new ReviewQueueStore(undefined); + store.registerWorktree('pir-1', wt); + await store.add('pir-1', makeComment('c1')); + await store.add('pir-1', makeComment('c2')); + + await store.edit('pir-1', 'c1', 'revised'); + await store.remove('pir-1', ['c2']); + + const fresh = new ReviewQueueStore(undefined); + fresh.registerWorktree('pir-1', wt); + const loaded = await fresh.load('pir-1'); + expect(loaded).toHaveLength(1); + expect(loaded[0]!.body).toBe('revised'); + store.dispose(); + fresh.dispose(); + }); + + it('loads a vanished worktree (afx cleanup) as an empty queue', async () => { + const wt = await makeWorktree('pir-1'); + const store = new ReviewQueueStore(undefined); + store.registerWorktree('pir-1', wt); + await store.add('pir-1', makeComment('c1')); + + await fs.rm(wt, { recursive: true, force: true }); + expect(await store.load('pir-1')).toEqual([]); + expect(store.count('pir-1')).toBe(0); + store.dispose(); + }); + + it('mutation without a registered worktree throws instead of writing nowhere', async () => { + const store = new ReviewQueueStore(undefined); + await expect(store.add('ghost', makeComment('c1'))).rejects.toThrow(/no worktree/i); + store.dispose(); + }); + + it('writes the info/exclude managed block exactly once', async () => { + const wt = await makeWorktree('pir-1'); + const store = new ReviewQueueStore(undefined); + store.registerWorktree('pir-1', wt); + + await store.add('pir-1', makeComment('c1')); + await store.add('pir-1', makeComment('c2')); + + const exclude = await fs.readFile(path.join(wt, '.git', 'info', 'exclude'), 'utf8'); + const hits = exclude.split('\n').filter(l => l.trim() === QUEUE_FILE_RELPATH); + expect(hits).toHaveLength(1); + expect(exclude).toContain('.builder-*'); + + // The block makes git actually ignore both the queue file and the family. + await fs.writeFile(path.join(wt, '.builder-prompt.txt'), 'x'); + const status = execFileSync('git', ['-C', wt, 'status', '--porcelain'], { encoding: 'utf8' }); + expect(status).not.toContain(QUEUE_FILE_RELPATH); + expect(status).not.toContain('.builder-prompt.txt'); + store.dispose(); + }); + + it('a second session merges idempotently into an existing exclude file', async () => { + const wt = await makeWorktree('pir-1'); + const first = new ReviewQueueStore(undefined); + first.registerWorktree('pir-1', wt); + await first.add('pir-1', makeComment('c1')); + first.dispose(); + + const second = new ReviewQueueStore(undefined); + second.registerWorktree('pir-1', wt); + await second.add('pir-1', makeComment('c2')); + second.dispose(); + + const exclude = await fs.readFile(path.join(wt, '.git', 'info', 'exclude'), 'utf8'); + expect(exclude.split('\n').filter(l => l.includes('managed block'))).toHaveLength(1); + }); + + it('fires onDidChangeQueue on mutations with the builder id', async () => { + const wt = await makeWorktree('pir-1'); + const store = new ReviewQueueStore(undefined); + store.registerWorktree('pir-1', wt); + const events: string[] = []; + store.onDidChangeQueue(id => events.push(id)); + + await store.add('pir-1', makeComment('c1')); + await store.remove('pir-1', ['c1']); + expect(events).toEqual(['pir-1', 'pir-1']); + store.dispose(); + }); +}); diff --git a/apps/vscode/src/review-queue/store.ts b/apps/vscode/src/review-queue/store.ts new file mode 100644 index 000000000..4f71368e5 --- /dev/null +++ b/apps/vscode/src/review-queue/store.ts @@ -0,0 +1,252 @@ +/** + * ReviewQueueStore — the single owner of `.codev/pending-comments.json` reads + * and writes for every builder (#1037). All surfaces (inline threads, status + * bar, submit command) go through this store; none touch the files directly. + * + * Sync model (plan decision 7, hybrid): + * - Every mutation this store performs fires `onDidChangeQueue(builderId)` + * immediately, so same-window surfaces update with no watcher latency. + * - A `FileSystemWatcher` on each `.builders//.codev/pending-comments.json` + * (relative to the workspace root) catches other VSCode windows' writes and + * external deletes (`afx cleanup` removing the worktree), debounced 200ms + * per file. Own writes are echo-suppressed by comparing the file content + * against the last bytes this store wrote. + * + * Worktree paths are registered by callers from authoritative sources (the + * diff-inject registry entry or the Tower overview), never synthesized from + * the workspace root — a builder's worktree can live anywhere. + * + * On the first write of a queue file the store also appends a managed ignore + * block to `$GIT_COMMON_DIR/info/exclude` (plan decision 8) so the file — and + * the `.builder-*` spawn scaffolding family — never shows as untracked noise + * in any worktree's `git status`. + */ + +import * as vscode from 'vscode'; +import * as fs from 'node:fs/promises'; +import * as path from 'node:path'; +import { execFile } from 'node:child_process'; +import { promisify } from 'node:util'; +import { + addComment, + editComment, + parseQueueFile, + QUEUE_FILE_RELPATH, + removeComments, + serializeQueueFile, + mergeExcludeBlock, + type PendingComment, +} from './queue.js'; + +const execFileAsync = promisify(execFile); + +const WATCHER_DEBOUNCE_MS = 200; + +export class ReviewQueueStore implements vscode.Disposable { + private readonly changeEmitter = new vscode.EventEmitter(); + /** Fires with the builderId whose queue changed (own mutation or external). */ + readonly onDidChangeQueue = this.changeEmitter.event; + + private readonly worktreeById = new Map(); + /** In-memory queue cache, keyed by builderId; refreshed on external events. */ + private readonly cache = new Map(); + /** Last serialized bytes written per queue path — the watcher echo filter. */ + private readonly lastWritten = new Map(); + /** Worktrees whose info/exclude has been ensured this session. */ + private readonly excludeEnsured = new Set(); + private readonly debounceTimers = new Map>(); + private readonly disposables: vscode.Disposable[] = []; + + constructor(workspaceRoot: string | undefined) { + if (workspaceRoot) { + const watcher = vscode.workspace.createFileSystemWatcher( + new vscode.RelativePattern(workspaceRoot, `.builders/*/${QUEUE_FILE_RELPATH}`), + ); + const onEvent = (uri: vscode.Uri): void => { this.scheduleExternalRead(uri.fsPath); }; + this.disposables.push( + watcher, + watcher.onDidCreate(onEvent), + watcher.onDidChange(onEvent), + watcher.onDidDelete(onEvent), + ); + } + } + + /** Remember a builder's worktree root (idempotent; callers pass authoritative paths). */ + registerWorktree(builderId: string, worktreePath: string): void { + this.worktreeById.set(builderId, worktreePath); + } + + getWorktreePath(builderId: string): string | undefined { + return this.worktreeById.get(builderId); + } + + /** All builder ids with at least one pending comment loaded this session. */ + buildersWithPending(): string[] { + const ids: string[] = []; + for (const [id, comments] of this.cache) { + if (comments.length > 0) { ids.push(id); } + } + return ids; + } + + /** Cached queue for a builder (empty until `load` has run for it). */ + getComments(builderId: string): PendingComment[] { + return this.cache.get(builderId) ?? []; + } + + count(builderId: string): number { + return this.getComments(builderId).length; + } + + /** + * Read the queue file from disk into the cache. Missing file (never + * created, or the worktree was cleaned up) reads as empty. Fires the change + * event only when the loaded content differs from the cache. + */ + async load(builderId: string): Promise { + const filePath = this.queuePath(builderId); + if (!filePath) { return []; } + let comments: PendingComment[] = []; + try { + comments = parseQueueFile(await fs.readFile(filePath, 'utf8')); + } catch { + // Missing file or unreadable — an empty queue, not an error. + } + const before = JSON.stringify(this.cache.get(builderId) ?? []); + this.cache.set(builderId, comments); + if (JSON.stringify(comments) !== before) { + this.changeEmitter.fire(builderId); + } + return comments; + } + + async add(builderId: string, comment: PendingComment): Promise { + await this.mutate(builderId, comments => addComment(comments, comment)); + } + + async edit(builderId: string, id: string, body: string): Promise { + await this.mutate(builderId, comments => editComment(comments, id, body)); + } + + async remove(builderId: string, ids: readonly string[]): Promise { + await this.mutate(builderId, comments => removeComments(comments, ids)); + } + + async clear(builderId: string): Promise { + await this.mutate(builderId, () => []); + } + + private queuePath(builderId: string): string | undefined { + const worktree = this.worktreeById.get(builderId); + if (!worktree) { return undefined; } + return path.join(worktree, QUEUE_FILE_RELPATH); + } + + /** + * Load-mutate-write. Loads from disk first so concurrent writers (another + * window) are folded in rather than clobbered, then persists and fires. + */ + private async mutate( + builderId: string, + fn: (comments: PendingComment[]) => PendingComment[], + ): Promise { + const filePath = this.queuePath(builderId); + if (!filePath) { + throw new Error(`No worktree registered for builder "${builderId}"`); + } + const current = await this.load(builderId); + const next = fn(current); + const serialized = serializeQueueFile(builderId, next); + await fs.mkdir(path.dirname(filePath), { recursive: true }); + await fs.writeFile(filePath, serialized, 'utf8'); + this.lastWritten.set(filePath, serialized); + this.cache.set(builderId, next); + await this.ensureExclude(this.worktreeById.get(builderId)!); + this.changeEmitter.fire(builderId); + } + + /** + * Append the managed ignore block to the repo's shared + * `$GIT_COMMON_DIR/info/exclude` (once per session per worktree; the merge + * itself is idempotent across sessions). Failures are swallowed: an + * unignored queue file is cosmetic noise, never worth failing a comment + * write over. + */ + private async ensureExclude(worktreePath: string): Promise { + if (this.excludeEnsured.has(worktreePath)) { return; } + this.excludeEnsured.add(worktreePath); + try { + const { stdout } = await execFileAsync('git', [ + '-C', worktreePath, 'rev-parse', '--path-format=absolute', '--git-common-dir', + ]); + const commonDir = stdout.trim(); + if (!commonDir) { return; } + const excludePath = path.join(commonDir, 'info', 'exclude'); + let existing = ''; + try { + existing = await fs.readFile(excludePath, 'utf8'); + } catch { + // No info/exclude yet — start from empty. + } + const merged = mergeExcludeBlock(existing); + if (merged === null) { return; } + await fs.mkdir(path.dirname(excludePath), { recursive: true }); + await fs.writeFile(excludePath, merged, 'utf8'); + } catch { + // Not a git repo / git unavailable — skip silently. + } + } + + /** Debounced handler for watcher events on a queue file path. */ + private scheduleExternalRead(filePath: string): void { + const existing = this.debounceTimers.get(filePath); + if (existing) { clearTimeout(existing); } + this.debounceTimers.set(filePath, setTimeout(() => { + this.debounceTimers.delete(filePath); + this.handleExternalEvent(filePath); + }, WATCHER_DEBOUNCE_MS)); + } + + private async handleExternalEvent(filePath: string): Promise { + const builderId = this.builderIdForQueuePath(filePath); + let content: string | null = null; + try { + content = await fs.readFile(filePath, 'utf8'); + } catch { + // Deleted (afx cleanup) — treat as empty below. + } + if (content !== null && content === this.lastWritten.get(filePath)) { + return; // Echo of our own write. + } + let comments: PendingComment[] = []; + if (content !== null) { comments = parseQueueFile(content); } + const before = JSON.stringify(this.cache.get(builderId) ?? []); + this.cache.set(builderId, comments); + if (JSON.stringify(comments) !== before) { + this.changeEmitter.fire(builderId); + } + } + + /** + * Map a watched queue-file path back to a builder id: a registered worktree + * match wins; otherwise fall back to the worktree directory basename (the + * `.builders//` convention), registering it so subsequent reads work. + */ + private builderIdForQueuePath(filePath: string): string { + const worktree = path.dirname(path.dirname(filePath)); + for (const [id, wt] of this.worktreeById) { + if (wt === worktree) { return id; } + } + const id = path.basename(worktree); + this.worktreeById.set(id, worktree); + return id; + } + + dispose(): void { + for (const timer of this.debounceTimers.values()) { clearTimeout(timer); } + this.debounceTimers.clear(); + for (const d of this.disposables) { d.dispose(); } + this.changeEmitter.dispose(); + } +} From ca3ee865be5fdb62932af5fd9b73d60b9aa05137 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Thu, 6 Aug 2026 10:26:16 +1000 Subject: [PATCH 08/27] [PIR #1037] Mode-aware diff codelenses: diffCodelensMode setting, title-bar toggle, always-on context menu --- apps/vscode/package.json | 132 +++++++++++++- .../contributes-review-queue.test.ts | 99 +++++++++++ .../src/__tests__/diff-codelens-mode.test.ts | 166 ++++++++++++++++++ .../__tests__/diff-inject-context-key.test.ts | 4 + .../src/__tests__/diff-inject-ref.test.ts | 24 +-- apps/vscode/src/diff-inject-codelens.ts | 63 ++++++- apps/vscode/src/diff-inject-ref.ts | 27 ++- 7 files changed, 495 insertions(+), 20 deletions(-) create mode 100644 apps/vscode/src/__tests__/contributes-review-queue.test.ts create mode 100644 apps/vscode/src/__tests__/diff-codelens-mode.test.ts diff --git a/apps/vscode/package.json b/apps/vscode/package.json index ffabf3c45..ad99d1c52 100644 --- a/apps/vscode/package.json +++ b/apps/vscode/package.json @@ -376,6 +376,52 @@ { "command": "codev.cancelEditReviewComment", "title": "Codev: Cancel Edit" + }, + { + "command": "codev.diffCodelensUseForward", + "title": "Codev: Switch Diff Codelens to Forward Mode", + "icon": "$(terminal)" + }, + { + "command": "codev.diffCodelensUseComment", + "title": "Codev: Switch Diff Codelens to Comment Mode", + "icon": "$(comment)" + }, + { + "command": "codev.commentSelectionForBuilder", + "title": "Codev: Comment for Builder" + }, + { + "command": "codev.submitReview", + "title": "Codev: Submit Review" + }, + { + "command": "codev.discardReviewComments", + "title": "Codev: Discard Review Comments" + }, + { + "command": "codev.submitBuilderComment", + "title": "Codev: Queue Comment for Builder", + "enablement": "!commentIsEmpty" + }, + { + "command": "codev.deleteBuilderComment", + "title": "Codev: Delete Pending Comment", + "icon": "$(trash)" + }, + { + "command": "codev.startEditBuilderComment", + "title": "Codev: Edit Pending Comment", + "icon": "$(edit)" + }, + { + "command": "codev.saveEditBuilderComment", + "title": "Codev: Save Pending Comment", + "enablement": "!commentIsEmpty" + }, + { + "command": "codev.cancelEditBuilderComment", + "title": "Codev: Cancel Pending Comment Edit" } ], "menus": { @@ -384,6 +430,16 @@ "command": "codev.openMarkdownPreview", "when": "resourceLangId == markdown && resourcePath =~ /\\/codev\\/(plans|specs|reviews)\\//", "group": "navigation" + }, + { + "command": "codev.diffCodelensUseForward", + "when": "codev.activeEditorIsBuilderFile && codev.diffCodelensMode == 'comment'", + "group": "navigation" + }, + { + "command": "codev.diffCodelensUseComment", + "when": "codev.activeEditorIsBuilderFile && codev.diffCodelensMode == 'forward'", + "group": "navigation" } ], "commandPalette": [ @@ -407,6 +463,38 @@ "command": "codev.forwardSelectionToBuilder", "when": "false" }, + { + "command": "codev.commentSelectionForBuilder", + "when": "false" + }, + { + "command": "codev.diffCodelensUseForward", + "when": "false" + }, + { + "command": "codev.diffCodelensUseComment", + "when": "false" + }, + { + "command": "codev.submitBuilderComment", + "when": "false" + }, + { + "command": "codev.deleteBuilderComment", + "when": "false" + }, + { + "command": "codev.startEditBuilderComment", + "when": "false" + }, + { + "command": "codev.saveEditBuilderComment", + "when": "false" + }, + { + "command": "codev.cancelEditBuilderComment", + "when": "false" + }, { "command": "codev.openBuilderById", "when": "false" @@ -506,9 +594,14 @@ ], "editor/context": [ { - "command": "codev.forwardSelectionToBuilder", - "when": "codev.activeEditorIsBuilderFile && editorHasSelection", + "command": "codev.commentSelectionForBuilder", + "when": "codev.activeEditorIsBuilderFile", "group": "codev@1" + }, + { + "command": "codev.forwardSelectionToBuilder", + "when": "codev.activeEditorIsBuilderFile", + "group": "codev@2" } ], "view/item/context": [ @@ -740,6 +833,11 @@ "command": "codev.submitReviewComment", "group": "inline", "when": "commentController == codev-review && commentThreadIsEmpty" + }, + { + "command": "codev.submitBuilderComment", + "group": "inline", + "when": "commentController == codev-builder-review && commentThreadIsEmpty" } ], "comments/commentThread/title": [ @@ -747,6 +845,11 @@ "command": "codev.deleteReviewComment", "group": "inline@1", "when": "commentController == codev-review && commentThread == inline-review" + }, + { + "command": "codev.deleteBuilderComment", + "group": "inline@1", + "when": "commentController == codev-builder-review && commentThread == pending-builder-comment" } ], "comments/comment/title": [ @@ -754,6 +857,11 @@ "command": "codev.startEditReviewComment", "group": "group@1", "when": "commentController == codev-review && comment == inline-review" + }, + { + "command": "codev.startEditBuilderComment", + "group": "group@1", + "when": "commentController == codev-builder-review && comment == pending-builder-comment" } ], "comments/comment/context": [ @@ -766,6 +874,16 @@ "command": "codev.cancelEditReviewComment", "group": "inline@1", "when": "commentController == codev-review" + }, + { + "command": "codev.saveEditBuilderComment", + "group": "inline@2", + "when": "commentController == codev-builder-review" + }, + { + "command": "codev.cancelEditBuilderComment", + "group": "inline@1", + "when": "commentController == codev-builder-review" } ] }, @@ -915,6 +1033,16 @@ "default": "editor", "description": "Where to open Codev terminals (editor area or bottom panel)" }, + "codev.diffCodelensMode": { + "type": "string", + "enum": ["comment", "forward"], + "enumDescriptions": [ + "Codelenses compose a review comment in an inline thread; comments queue per builder and reach the PTY only via Submit Review.", + "Codelenses type the file/line reference straight into the builder terminal (the original fire-and-forget flow)." + ], + "default": "comment", + "description": "Which single action the codelenses in a builder diff offer. The right-click context menu always offers both actions regardless of mode. Toggle via the diff editor's title-bar button; the choice persists per workspace." + }, "codev.maxTerminals": { "type": "number", "default": 25, diff --git a/apps/vscode/src/__tests__/contributes-review-queue.test.ts b/apps/vscode/src/__tests__/contributes-review-queue.test.ts new file mode 100644 index 000000000..38f0cc2ca --- /dev/null +++ b/apps/vscode/src/__tests__/contributes-review-queue.test.ts @@ -0,0 +1,99 @@ +/** + * Manifest invariants for the review-comment queue surface (#1037): + * + * - `codev.diffCodelensMode` is declared with default `"comment"`. + * - The editor context menu offers BOTH the comment and the forward action on + * builder-diff files with no mode condition — the non-default flow must + * always be one right-click away, whatever the codelens shows. + * - The `editor/title` toggle buttons swap on the mode context key and are + * mutually exclusive (exactly one visible per mode). + * - The builder-review comment menus are scoped to the + * `codev-builder-review` controller so they never leak onto the + * plan-review controller's threads (and vice versa). + * - The #789 keybinding (Ctrl/Cmd+K B) is untouched by the mode. + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; + +const PKG = JSON.parse( + readFileSync(resolve(__dirname, '../../package.json'), 'utf8'), +); + +interface MenuEntry { command: string; when?: string; group?: string } + +const menus: Record = PKG.contributes.menus; + +function entry(section: string, command: string): MenuEntry | undefined { + return (menus[section] ?? []).find(m => m.command === command); +} + +describe('codev.diffCodelensMode setting', () => { + it('is declared with enum comment|forward and default comment', () => { + const prop = PKG.contributes.configuration.properties['codev.diffCodelensMode']; + expect(prop).toBeDefined(); + expect(prop.enum).toEqual(['comment', 'forward']); + expect(prop.default).toBe('comment'); + }); +}); + +describe('editor context menu', () => { + it('always offers both actions on builder-diff files, independent of mode', () => { + const comment = entry('editor/context', 'codev.commentSelectionForBuilder'); + const forward = entry('editor/context', 'codev.forwardSelectionToBuilder'); + expect(comment?.when).toBe('codev.activeEditorIsBuilderFile'); + expect(forward?.when).toBe('codev.activeEditorIsBuilderFile'); + for (const e of [comment, forward]) { + expect(e!.when).not.toContain('diffCodelensMode'); + } + }); +}); + +describe('editor/title mode toggle', () => { + it('shows exactly one toggle per mode via mutually exclusive when clauses', () => { + const toForward = entry('editor/title', 'codev.diffCodelensUseForward'); + const toComment = entry('editor/title', 'codev.diffCodelensUseComment'); + expect(toForward?.when).toBe("codev.activeEditorIsBuilderFile && codev.diffCodelensMode == 'comment'"); + expect(toComment?.when).toBe("codev.activeEditorIsBuilderFile && codev.diffCodelensMode == 'forward'"); + }); +}); + +describe('builder-review comment menus', () => { + it('scopes every entry to the codev-builder-review controller', () => { + const sections = [ + 'comments/commentThread/context', + 'comments/commentThread/title', + 'comments/comment/title', + 'comments/comment/context', + ]; + const builderCommands = [ + 'codev.submitBuilderComment', + 'codev.deleteBuilderComment', + 'codev.startEditBuilderComment', + 'codev.saveEditBuilderComment', + 'codev.cancelEditBuilderComment', + ]; + const found = new Set(); + for (const section of sections) { + for (const e of menus[section] ?? []) { + if (builderCommands.includes(e.command)) { + found.add(e.command); + expect(e.when, `${section}:${e.command}`).toContain('commentController == codev-builder-review'); + } + } + } + expect([...found].sort()).toEqual([...builderCommands].sort()); + }); +}); + +describe('#789 keybinding is mode-independent', () => { + it('keeps Ctrl/Cmd+K B on forwardSelectionToBuilder with its original when clause', () => { + const binding = (PKG.contributes.keybindings as Array<{ command: string; key: string; mac?: string; when?: string }>) + .find(k => k.command === 'codev.forwardSelectionToBuilder'); + expect(binding?.key).toBe('ctrl+k b'); + expect(binding?.mac).toBe('cmd+k b'); + expect(binding?.when).toBe('codev.activeEditorIsBuilderFile && editorHasSelection'); + expect(binding?.when).not.toContain('diffCodelensMode'); + }); +}); diff --git a/apps/vscode/src/__tests__/diff-codelens-mode.test.ts b/apps/vscode/src/__tests__/diff-codelens-mode.test.ts new file mode 100644 index 000000000..ed2c0b2e4 --- /dev/null +++ b/apps/vscode/src/__tests__/diff-codelens-mode.test.ts @@ -0,0 +1,166 @@ +/** + * Mode-aware diff codelenses (#1037): exactly one lens per anchor, whose + * title + command follow `codev.diffCodelensMode` — `Comment for Builder` / + * `codev.commentForBuilder` in comment mode (the default), `Forward to + * Builder` / `codev.forwardToBuilder` in forward mode. A configuration change + * must re-emit lenses (and re-sync the mode context key) so the flip is live. + */ + +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, + configListeners: [] as Array<(e: unknown) => void>, + setContextCalls: [] as Array<{ key: string; value: unknown }>, + capturedProvider: undefined as unknown, + }; + return { EventEmitter, state }; +}); + +vi.mock('vscode', () => ({ + EventEmitter: h.EventEmitter, + Range: class { + constructor( + public startLine: number, + public startChar: number, + public endLine: number, + public endChar: number, + ) {} + }, + CodeLens: class { + constructor(public range: unknown, public command: { title: string; command: string; arguments: unknown[] }) {} + }, + languages: { + registerCodeLensProvider: (_sel: unknown, prov: unknown) => { + h.state.capturedProvider = prov; + return { dispose() {} }; + }, + }, + commands: { + executeCommand: (cmd: string, ...args: unknown[]) => { + if (cmd === 'setContext') { + h.state.setContextCalls.push({ key: args[0] as string, value: args[1] }); + } + return Promise.resolve(undefined); + }, + }, + window: { + activeTextEditor: undefined, + onDidChangeActiveTextEditor: () => ({ dispose() {} }), + }, + workspace: { + getConfiguration: () => ({ get: () => h.state.mode }), + onDidChangeConfiguration: (fn: (e: unknown) => void) => { + h.state.configListeners.push(fn); + return { dispose() {} }; + }, + }, +})); + +const { + activateDiffInjectCodeLens, + setDiffInjectSession, + DIFF_CODELENS_MODE_KEY, +} = await import('../diff-inject-codelens.js'); + +interface Lens { command: { title: string; command: string; arguments: unknown[] } } + +const ENTRY = { + fsPath: '/wt/pkg/src/a.ts', + builderId: 'pir-9', + relPath: 'pkg/src/a.ts', + hunks: [{ start: 5, end: 8 }], +}; + +const DOC = { uri: { fsPath: ENTRY.fsPath, toString: () => `file://${ENTRY.fsPath}` }, lineCount: 100 }; +const TOKEN = { isCancellationRequested: false }; + +async function lenses(): Promise { + const provider = h.state.capturedProvider as { + provideCodeLenses(doc: unknown, token: unknown): Promise; + }; + return provider.provideCodeLenses(DOC, TOKEN); +} + +beforeEach(() => { + h.state.mode = 'comment'; + h.state.configListeners.length = 0; + h.state.setContextCalls.length = 0; + activateDiffInjectCodeLens({ subscriptions: [] } as never); + setDiffInjectSession([ENTRY]); +}); + +describe('diff codelens mode (#1037)', () => { + it('comment mode (default) emits Comment for Builder lenses with the comment command', async () => { + const all = await lenses(); + expect(all.length).toBeGreaterThan(0); + for (const lens of all) { + expect(lens.command.command).toBe('codev.commentForBuilder'); + expect(lens.command.title).toMatch(/^Comment for Builder/); + } + // File-level lens carries a null range (whole-file comment); the hunk lens + // carries its 1-based range for the queued comment's anchor. + expect(all[0]!.command.arguments).toEqual(['pir-9', ENTRY.fsPath, ENTRY.relPath, null]); + const hunkLens = all.find(l => l.command.title.includes('lines 5-8')); + expect(hunkLens?.command.arguments).toEqual(['pir-9', ENTRY.fsPath, ENTRY.relPath, { start: 5, end: 8 }]); + }); + + it('forward mode emits the original #789 lenses untouched', async () => { + h.state.mode = 'forward'; + const all = await lenses(); + for (const lens of all) { + expect(lens.command.command).toBe('codev.forwardToBuilder'); + expect(lens.command.title).toMatch(/^Forward to Builder/); + } + expect(all[0]!.command.arguments).toEqual(['pir-9', 'pkg/src/a.ts ']); + }); + + it('exactly one lens per anchor line in either mode', async () => { + for (const mode of ['comment', 'forward']) { + h.state.mode = mode; + const all = (await lenses()) as Array; + const anchorLines = all.map(l => l.range.startLine); + expect(new Set(anchorLines).size).toBe(anchorLines.length); + } + }); + + it('a configuration change re-emits lenses and re-syncs the mode context key', () => { + const provider = h.state.capturedProvider as { + onDidChangeCodeLenses(fn: () => void): { dispose(): void }; + }; + let fired = 0; + provider.onDidChangeCodeLenses(() => { fired += 1; }); + + h.state.mode = 'forward'; + for (const listener of h.state.configListeners) { + listener({ affectsConfiguration: (section: string) => section === DIFF_CODELENS_MODE_KEY }); + } + expect(fired).toBeGreaterThan(0); + const modeSyncs = h.state.setContextCalls.filter(c => c.key === DIFF_CODELENS_MODE_KEY); + expect(modeSyncs[modeSyncs.length - 1]!.value).toBe('forward'); + }); + + it('an unrelated configuration change does not re-emit lenses', () => { + const provider = h.state.capturedProvider as { + onDidChangeCodeLenses(fn: () => void): { dispose(): void }; + }; + let fired = 0; + provider.onDidChangeCodeLenses(() => { fired += 1; }); + for (const listener of h.state.configListeners) { + listener({ affectsConfiguration: () => false }); + } + expect(fired).toBe(0); + }); +}); diff --git a/apps/vscode/src/__tests__/diff-inject-context-key.test.ts b/apps/vscode/src/__tests__/diff-inject-context-key.test.ts index b46b2a259..998d08551 100644 --- a/apps/vscode/src/__tests__/diff-inject-context-key.test.ts +++ b/apps/vscode/src/__tests__/diff-inject-context-key.test.ts @@ -48,6 +48,10 @@ vi.mock('vscode', () => ({ get activeTextEditor() { return h.state.activeEditor; }, onDidChangeActiveTextEditor: () => ({ dispose() {} }), }, + workspace: { + getConfiguration: () => ({ get: () => 'comment' }), + onDidChangeConfiguration: () => ({ dispose() {} }), + }, })); const { diff --git a/apps/vscode/src/__tests__/diff-inject-ref.test.ts b/apps/vscode/src/__tests__/diff-inject-ref.test.ts index 00a7ca967..7395c954c 100644 --- a/apps/vscode/src/__tests__/diff-inject-ref.test.ts +++ b/apps/vscode/src/__tests__/diff-inject-ref.test.ts @@ -57,9 +57,9 @@ describe('buildSymbolLensDescriptors', () => { ]; expect(buildSymbolLensDescriptors('a/b.ts', symbols)).toEqual([ { line: 0, title: 'Forward to Builder', refText: 'a/b.ts ' }, - { line: 4, title: 'Forward to Builder (lines 5-10)', refText: 'a/b.ts:L5-L10 ' }, - { line: 12, title: 'Forward to Builder (lines 13-19)', refText: 'a/b.ts:L13-L19 ' }, - { line: 20, title: 'Forward to Builder (lines 21-25)', refText: 'a/b.ts:L21-L25 ' }, + { line: 4, title: 'Forward to Builder (lines 5-10)', refText: 'a/b.ts:L5-L10 ', range: { start: 5, end: 10 } }, + { line: 12, title: 'Forward to Builder (lines 13-19)', refText: 'a/b.ts:L13-L19 ', range: { start: 13, end: 19 } }, + { line: 20, title: 'Forward to Builder (lines 21-25)', refText: 'a/b.ts:L21-L25 ', range: { start: 21, end: 25 } }, ]); }); @@ -71,9 +71,9 @@ describe('buildSymbolLensDescriptors', () => { ]); expect(buildSymbolLensDescriptors('a/b.ts', [cls])).toEqual([ { line: 0, title: 'Forward to Builder', refText: 'a/b.ts ' }, - { line: 3, title: 'Forward to Builder (lines 4-41)', refText: 'a/b.ts:L4-L41 ' }, // class - { line: 5, title: 'Forward to Builder (lines 6-9)', refText: 'a/b.ts:L6-L9 ' }, // constructor - { line: 10, title: 'Forward to Builder (lines 11-21)', refText: 'a/b.ts:L11-L21 ' }, // method + { line: 3, title: 'Forward to Builder (lines 4-41)', refText: 'a/b.ts:L4-L41 ', range: { start: 4, end: 41 } }, // class + { line: 5, title: 'Forward to Builder (lines 6-9)', refText: 'a/b.ts:L6-L9 ', range: { start: 6, end: 9 } }, // constructor + { line: 10, title: 'Forward to Builder (lines 11-21)', refText: 'a/b.ts:L11-L21 ', range: { start: 11, end: 21 } }, // method ]); }); @@ -84,7 +84,7 @@ describe('buildSymbolLensDescriptors', () => { ]; expect(buildSymbolLensDescriptors('a/b.ts', symbols)).toEqual([ { line: 0, title: 'Forward to Builder', refText: 'a/b.ts ' }, - { line: 4, title: 'Forward to Builder (lines 5-13)', refText: 'a/b.ts:L5-L13 ' }, + { line: 4, title: 'Forward to Builder (lines 5-13)', refText: 'a/b.ts:L5-L13 ', range: { start: 5, end: 13 } }, ]); }); @@ -104,7 +104,7 @@ describe('buildSymbolLensDescriptors', () => { ]); expect(buildSymbolLensDescriptors('a/b.ts', [cls])).toEqual([ { line: 0, title: 'Forward to Builder', refText: 'a/b.ts ' }, - { line: 2, title: 'Forward to Builder (lines 3-51)', refText: 'a/b.ts:L3-L51 ' }, + { line: 2, title: 'Forward to Builder (lines 3-51)', refText: 'a/b.ts:L3-L51 ', range: { start: 3, end: 51 } }, ]); }); }); @@ -158,9 +158,9 @@ describe('buildAllLensDescriptors (symbol + change lenses)', () => { const ranges = [{ start: 10, end: 12 }, { start: 18, end: 18 }]; expect(buildAllLensDescriptors('a/b.ts', symbols, ranges)).toEqual([ { line: 0, title: 'Forward to Builder', refText: 'a/b.ts ' }, - { line: 4, title: 'Forward to Builder (lines 5-31)', refText: 'a/b.ts:L5-L31 ' }, - { line: 9, title: 'Forward to Builder (lines 10-12)', refText: 'a/b.ts:L10-L12 ' }, - { line: 17, title: 'Forward to Builder (line 18)', refText: 'a/b.ts:L18 ' }, + { line: 4, title: 'Forward to Builder (lines 5-31)', refText: 'a/b.ts:L5-L31 ', range: { start: 5, end: 31 } }, + { line: 9, title: 'Forward to Builder (lines 10-12)', refText: 'a/b.ts:L10-L12 ', range: { start: 10, end: 12 } }, + { line: 17, title: 'Forward to Builder (line 18)', refText: 'a/b.ts:L18 ', range: { start: 18, end: 18 } }, ]); }); @@ -169,7 +169,7 @@ describe('buildAllLensDescriptors (symbol + change lenses)', () => { const ranges = [{ start: 10, end: 12 }]; // anchor line 9 → collides → skipped expect(buildAllLensDescriptors('a/b.ts', symbols, ranges)).toEqual([ { line: 0, title: 'Forward to Builder', refText: 'a/b.ts ' }, - { line: 9, title: 'Forward to Builder (lines 10-31)', refText: 'a/b.ts:L10-L31 ' }, + { line: 9, title: 'Forward to Builder (lines 10-31)', refText: 'a/b.ts:L10-L31 ', range: { start: 10, end: 31 } }, ]); }); diff --git a/apps/vscode/src/diff-inject-codelens.ts b/apps/vscode/src/diff-inject-codelens.ts index e302067a7..3275dbe56 100644 --- a/apps/vscode/src/diff-inject-codelens.ts +++ b/apps/vscode/src/diff-inject-codelens.ts @@ -33,10 +33,28 @@ import { buildAllLensDescriptors, type ChangedRange, type SymbolNode } from './d * `contributes.commands`, so it never appears in the Command Palette. */ export const FORWARD_TO_BUILDER_COMMAND = 'codev.forwardToBuilder'; +/** Command id the comment-mode lenses invoke (#1037). Like the forward + * command, registered in `extension.ts` and kept out of the Command Palette. */ +export const COMMENT_FOR_BUILDER_COMMAND = 'codev.commentForBuilder'; + /** Context key (true when the active editor is a tracked builder-diff file) that * scopes the `editor/context` "Forward Selection to Builder" item. */ export const BUILDER_FILE_CONTEXT_KEY = 'codev.activeEditorIsBuilderFile'; +/** Setting + context key carrying the diff codelens mode (#1037). The setting + * persists per workspace; the same-named context key mirrors it so the + * `editor/title` toggle buttons swap via `when` clauses. */ +export const DIFF_CODELENS_MODE_KEY = 'codev.diffCodelensMode'; + +export type DiffCodelensMode = 'comment' | 'forward'; + +/** Read the current mode from configuration (default: comment). */ +export function getDiffCodelensMode(): DiffCodelensMode { + const value = vscode.workspace.getConfiguration('codev').get('diffCodelensMode'); + if (value === 'forward') { return 'forward'; } + return 'comment'; +} + /** One changed file in the active diff session, keyed by its right-side fs path. */ export interface DiffInjectSessionEntry { /** Absolute fs path of the right-side (worktree) document. */ @@ -81,6 +99,16 @@ class DiffInjectCodeLensProvider implements vscode.CodeLensProvider { return this.registry.get(fsPath); } + /** All entries in the active diff session (the thread reconciler iterates these). */ + entries(): DiffInjectSessionEntry[] { + return [...this.registry.values()]; + } + + /** Re-emit lenses without changing the registry (mode/config changes). */ + refresh(): void { + this._onDidChangeCodeLenses.fire(); + } + async provideCodeLenses( document: vscode.TextDocument, token: vscode.CancellationToken, @@ -102,9 +130,22 @@ class DiffInjectCodeLensProvider implements vscode.CodeLensProvider { const nodes = symbols.map(toSymbolNode); const lastLine = Math.max(document.lineCount - 1, 0); - return buildAllLensDescriptors(entry.relPath, nodes, entry.hunks).map(d => { + // Exactly one lens per anchor, matching the current mode (#1037): comment + // mode (default) mounts an inline thread; forward mode keeps #789's + // fire-and-forget injection. Same anchors either way. + const mode = getDiffCodelensMode(); + let label = 'Comment for Builder'; + if (mode === 'forward') { label = 'Forward to Builder'; } + return buildAllLensDescriptors(entry.relPath, nodes, entry.hunks, label).map(d => { const line = Math.min(Math.max(d.line, 0), lastLine); const range = new vscode.Range(line, 0, line, 0); + if (mode === 'comment') { + return new vscode.CodeLens(range, { + title: d.title, + command: COMMENT_FOR_BUILDER_COMMAND, + arguments: [entry.builderId, document.uri.fsPath, entry.relPath, d.range ?? null], + }); + } return new vscode.CodeLens(range, { title: d.title, command: FORWARD_TO_BUILDER_COMMAND, @@ -130,6 +171,14 @@ export function activateDiffInjectCodeLens(context: vscode.ExtensionContext): vo }; syncContextKey(vscode.window.activeTextEditor); + // Mirror the diffCodelensMode setting into a context key so the + // `editor/title` toggle buttons swap on it (#1037), and refresh the lenses + // when the setting changes so their titles/commands flip live. + const syncModeKey = (): void => { + void vscode.commands.executeCommand('setContext', DIFF_CODELENS_MODE_KEY, getDiffCodelensMode()); + }; + syncModeKey(); + context.subscriptions.push( vscode.languages.registerCodeLensProvider({ scheme: 'file' }, provider), vscode.window.onDidChangeActiveTextEditor(syncContextKey), @@ -140,6 +189,12 @@ export function activateDiffInjectCodeLens(context: vscode.ExtensionContext): vo // setSession/upsert), or the selection menu + Cmd/Ctrl+K B would stay // disabled on the just-opened diff until focus changes (#789). provider.onDidChangeCodeLenses(() => syncContextKey(vscode.window.activeTextEditor)), + vscode.workspace.onDidChangeConfiguration(e => { + if (e.affectsConfiguration(DIFF_CODELENS_MODE_KEY)) { + syncModeKey(); + provider.refresh(); + } + }), provider, ); } @@ -162,6 +217,12 @@ export function getDiffInjectEntry(fsPath: string): DiffInjectSessionEntry | und return provider.get(fsPath); } +/** All entries in the active diff session — the builder-review thread + * reconciler (#1037) uses this to know which files can host visible threads. */ +export function getDiffInjectEntries(): DiffInjectSessionEntry[] { + return provider.entries(); +} + /** * Subscribe to registry changes (a `setSession` / `upsert`). The registry is * frequently populated *after* the diff editor is already active — diff --git a/apps/vscode/src/diff-inject-ref.ts b/apps/vscode/src/diff-inject-ref.ts index fa6dd8adf..30e7a0101 100644 --- a/apps/vscode/src/diff-inject-ref.ts +++ b/apps/vscode/src/diff-inject-ref.ts @@ -21,6 +21,12 @@ export interface LensDescriptor { title: string; /** Text typed into the builder terminal — always ends with a space, no Enter. */ refText: string; + /** + * 1-based inclusive line range the lens denotes; absent on the file-level + * lens (whole file). Comment mode (#1037) anchors its queued comment to this + * range; forward mode only needs `refText`. + */ + range?: ChangedRange; } /** @@ -180,10 +186,18 @@ function rangeLabel(start: number, end: number): string { * * A symbol lens that would anchor on line 0 is skipped — the file-level lens * already occupies that line. + * + * `label` is the verb rendered in each title — `Forward to Builder` (#789, + * default) or `Comment for Builder` (#1037). The anchors are identical in both + * modes; only the title and the command the provider attaches differ. */ -export function buildSymbolLensDescriptors(relPath: string, symbols: SymbolNode[]): LensDescriptor[] { +export function buildSymbolLensDescriptors( + relPath: string, + symbols: SymbolNode[], + label = 'Forward to Builder', +): LensDescriptor[] { const lenses: LensDescriptor[] = [ - { line: 0, title: 'Forward to Builder', refText: buildBuilderFileRef(relPath) }, + { line: 0, title: label, refText: buildBuilderFileRef(relPath) }, ]; const addLens = (s: SymbolNode): void => { @@ -193,8 +207,9 @@ export function buildSymbolLensDescriptors(relPath: string, symbols: SymbolNode[ const end = s.endLine + 1; lenses.push({ line, - title: `Forward to Builder ${rangeLabel(start, end)}`, + title: `${label} ${rangeLabel(start, end)}`, refText: buildBuilderRangeRef(relPath, start, end), + range: { start, end }, }); }; @@ -230,8 +245,9 @@ export function buildAllLensDescriptors( relPath: string, symbols: SymbolNode[], ranges: ChangedRange[], + label = 'Forward to Builder', ): LensDescriptor[] { - const lenses = buildSymbolLensDescriptors(relPath, symbols); + const lenses = buildSymbolLensDescriptors(relPath, symbols, label); const usedLines = new Set(lenses.map(l => l.line)); for (const r of ranges) { const line = Math.max(r.start - 1, 0); @@ -239,8 +255,9 @@ export function buildAllLensDescriptors( usedLines.add(line); lenses.push({ line, - title: `Forward to Builder ${rangeLabel(r.start, r.end)}`, + title: `${label} ${rangeLabel(r.start, r.end)}`, refText: buildBuilderRangeRef(relPath, r.start, r.end), + range: { start: r.start, end: r.end }, }); } return lenses; From 9aafea0947683c718cf63565d73ed4791da6459b Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Thu, 6 Aug 2026 10:33:22 +1000 Subject: [PATCH 09/27] [PIR #1037] Builder-review comment controller: inline threads, reconcile, edit/delete --- .../__tests__/review-queue-reconcile.test.ts | 85 +++++ apps/vscode/src/comments/builder-review.ts | 311 ++++++++++++++++++ apps/vscode/src/review-queue/reconcile.ts | 75 +++++ 3 files changed, 471 insertions(+) create mode 100644 apps/vscode/src/__tests__/review-queue-reconcile.test.ts create mode 100644 apps/vscode/src/comments/builder-review.ts create mode 100644 apps/vscode/src/review-queue/reconcile.ts diff --git a/apps/vscode/src/__tests__/review-queue-reconcile.test.ts b/apps/vscode/src/__tests__/review-queue-reconcile.test.ts new file mode 100644 index 000000000..b4ea48921 --- /dev/null +++ b/apps/vscode/src/__tests__/review-queue-reconcile.test.ts @@ -0,0 +1,85 @@ +/** + * Thread reconcile planning (#1037, plan decision 6): a queued comment is + * visible exactly while its file is registered in the active diff session for + * its builder. Repeated reconciles are stable (no duplicate mounts), queue + * removals dispose, session swaps dispose the old builder's threads, and + * worktree derivation from a diff-inject entry never guesses. + */ + +import { describe, it, expect } from 'vitest'; +import { + planThreadReconcile, + deriveWorktreePath, + type RegisteredFile, +} from '../review-queue/reconcile.js'; +import type { PendingComment } from '../review-queue/queue.js'; + +function comment(id: string, file: string): PendingComment { + return { id, createdAt: '2026-08-06T10:00:00Z', file, lineRange: { start: 3, end: 4 }, body: id }; +} + +const FILE_A: RegisteredFile = { fsPath: '/wt/a/src/x.ts', builderId: 'A', relPath: 'src/x.ts' }; +const FILE_B: RegisteredFile = { fsPath: '/wt/b/src/y.ts', builderId: 'B', relPath: 'src/y.ts' }; + +describe('planThreadReconcile', () => { + it('mounts queued comments whose file is registered for their builder', () => { + const queues = new Map([ + ['A', [comment('a1', 'src/x.ts'), comment('a2', 'src/other.ts')]], + ['B', [comment('b1', 'src/y.ts')]], + ]); + const plan = planThreadReconcile([FILE_A, FILE_B], queues, new Set()); + expect(plan.toCreate.map(e => e.comment.id).sort()).toEqual(['a1', 'b1']); + expect(plan.toDispose).toEqual([]); + }); + + it('is stable across repeated reconciles (no duplicate mounts)', () => { + const queues = new Map([['A', [comment('a1', 'src/x.ts')]]]); + const plan = planThreadReconcile([FILE_A], queues, new Set(['a1'])); + expect(plan.toCreate).toEqual([]); + expect(plan.toDispose).toEqual([]); + }); + + it('does not leak one builder’s comments onto another’s same-named file', () => { + // Builder B has a file with the same repo-relative path as A's commented + // file; A's comment must only mount on A's fsPath. + const bSamePath: RegisteredFile = { fsPath: '/wt/b/src/x.ts', builderId: 'B', relPath: 'src/x.ts' }; + const queues = new Map([['A', [comment('a1', 'src/x.ts')]]]); + const plan = planThreadReconcile([bSamePath], queues, new Set()); + expect(plan.toCreate).toEqual([]); + }); + + it('disposes threads whose comment left the queue (submit / delete)', () => { + const queues = new Map([['A', [comment('a2', 'src/x.ts')]]]); + const plan = planThreadReconcile([FILE_A], queues, new Set(['a1', 'a2'])); + expect(plan.toDispose).toEqual(['a1']); + expect(plan.toCreate).toEqual([]); + }); + + it('disposes threads whose file left the diff session (session swap)', () => { + const queues = new Map([['A', [comment('a1', 'src/x.ts')]]]); + const plan = planThreadReconcile([FILE_B], queues, new Set(['a1'])); + expect(plan.toDispose).toEqual(['a1']); + }); + + it('mount target carries the registered fsPath (threads anchor on the right side)', () => { + const queues = new Map([['A', [comment('a1', 'src/x.ts')]]]); + const plan = planThreadReconcile([FILE_A], queues, new Set()); + expect(plan.toCreate[0]).toMatchObject({ fsPath: '/wt/a/src/x.ts', builderId: 'A' }); + }); +}); + +describe('deriveWorktreePath', () => { + it('strips the relPath suffix from the entry fsPath', () => { + expect(deriveWorktreePath('/repo/.builders/pir-9/src/x.ts', 'src/x.ts', '/')) + .toBe('/repo/.builders/pir-9'); + }); + + it('handles Windows separators', () => { + expect(deriveWorktreePath('C:\\repo\\.builders\\pir-9\\src\\x.ts', 'src/x.ts', '\\')) + .toBe('C:\\repo\\.builders\\pir-9'); + }); + + it('returns null instead of guessing when the suffix does not match', () => { + expect(deriveWorktreePath('/somewhere/else.ts', 'src/x.ts', '/')).toBeNull(); + }); +}); diff --git a/apps/vscode/src/comments/builder-review.ts b/apps/vscode/src/comments/builder-review.ts new file mode 100644 index 000000000..c5ff4ac14 --- /dev/null +++ b/apps/vscode/src/comments/builder-review.ts @@ -0,0 +1,311 @@ +/** + * Codev Builder Review — structured review comments on builder-diff files via + * VSCode's Comments API (#1037). + * + * The reviewer composes a comment in an inline thread (mounted by the + * comment-mode codelens, the gutter "+", or the context-menu action), submits + * it into the per-builder pending queue (`ReviewQueueStore`), and later + * flushes the whole queue to the builder PTY via Submit Review. This surface + * never touches #789's forward flow: a forward click reaches the PTY directly + * and never appears here. + * + * Thread lifecycle (plan decision 6): a queued comment renders as a visible + * thread exactly while its file is registered in the active diff-inject + * session. The reconciler diffs (registered files × queue) against mounted + * threads on every registry or queue change, so reload → re-open diff + * re-mounts, submit/delete disposes, and edits from another window update in + * place. Edit/delete mirror the plan-review controller's #1055 pattern. + */ + +import * as vscode from 'vscode'; +import * as path from 'node:path'; +import { randomUUID } from 'node:crypto'; +import { + getDiffCodelensMode, + getDiffInjectEntry, + getDiffInjectEntries, + onDidChangeDiffInjectRegistry, + COMMENT_FOR_BUILDER_COMMAND, +} from '../diff-inject-codelens.js'; +import { planThreadReconcile, deriveWorktreePath, type RegisteredFile } from '../review-queue/reconcile.js'; +import type { ReviewQueueStore } from '../review-queue/store.js'; +import type { LineRange, PendingComment } from '../review-queue/queue.js'; +import type { OverviewCache } from '../views/overview-data.js'; + +const CONTROLLER_ID = 'codev-builder-review'; + +/** contextValue on threads/comments — matched by the `comments/*` menu `when` clauses. */ +const PENDING_CONTEXT = 'pending-builder-comment'; + +/** A mounted queued comment. Carries its queue identity for edit/delete. */ +class BuilderReviewComment implements vscode.Comment { + public parent?: vscode.CommentThread; + public savedBody: string; + public mode = vscode.CommentMode.Preview; + public contextValue = PENDING_CONTEXT; + constructor( + public body: vscode.MarkdownString, + public author: vscode.CommentAuthorInformation, + public readonly commentId: string, + public readonly builderId: string, + ) { + this.savedBody = body.value; + } +} + +function bodyText(body: string | vscode.MarkdownString): string { + if (typeof body === 'string') { return body; } + return body.value; +} + +/** The 1-based inclusive range a thread's anchor denotes. */ +function threadLineRange(thread: vscode.CommentThread): LineRange { + const range = thread.range; + if (!range) { return { start: 1, end: 1 }; } + return { start: range.start.line + 1, end: range.end.line + 1 }; +} + +export function activateBuilderReviewComments( + context: vscode.ExtensionContext, + store: ReviewQueueStore, + overviewCache: OverviewCache, +): void { + 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', + }; + context.subscriptions.push(controller); + + // Gutter "+" on registered builder-diff files, comment mode only — in + // forward mode the comment surface recedes so the two modes stay distinct. + controller.commentingRangeProvider = { + provideCommentingRanges(document) { + if (getDiffCodelensMode() !== 'comment') { return []; } + if (!getDiffInjectEntry(document.uri.fsPath)) { return []; } + const lastLine = Math.max(0, document.lineCount - 1); + return [new vscode.Range(0, 0, lastLine, 0)]; + }, + }; + + /** Mounted threads keyed by queued-comment id. */ + const mounted = new Map(); + /** Codelens-supplied ranges for in-progress input threads (null = whole file). */ + const pendingRanges = new Map(); + /** Builders whose queue file has been loaded from disk this session. */ + const loaded = new Set(); + + function author(): vscode.CommentAuthorInformation { + return { name: overviewCache.getData()?.currentUser ?? 'architect' }; + } + + /** Register the entry's worktree with the store (derived, never guessed). */ + function registerEntryWorktree(entry: RegisteredFile): boolean { + if (store.getWorktreePath(entry.builderId)) { return true; } + const worktree = deriveWorktreePath(entry.fsPath, entry.relPath, path.sep); + if (!worktree) { return false; } + store.registerWorktree(entry.builderId, worktree); + return true; + } + + /** Anchor line for a queued comment, clamped to the open document if any. */ + function anchorLine(fsPath: string, range: LineRange | null): number { + let line = 0; + if (range) { line = Math.max(range.start - 1, 0); } + const doc = vscode.workspace.textDocuments.find(d => d.uri.fsPath === fsPath); + if (doc) { line = Math.min(line, Math.max(doc.lineCount - 1, 0)); } + return line; + } + + function mountQueuedComment(fsPath: string, builderId: string, comment: PendingComment): void { + const line = anchorLine(fsPath, comment.lineRange); + const thread = controller.createCommentThread( + vscode.Uri.file(fsPath), + new vscode.Range(line, 0, line, 0), + [], + ); + const rendered = new BuilderReviewComment( + new vscode.MarkdownString(comment.body), + author(), + comment.id, + builderId, + ); + rendered.parent = thread; + thread.comments = [rendered]; + thread.collapsibleState = vscode.CommentThreadCollapsibleState.Expanded; + thread.canReply = false; + thread.contextValue = PENDING_CONTEXT; + thread.label = 'Pending review comment'; + mounted.set(comment.id, thread); + } + + /** + * Diff desired vs mounted threads and apply. Also refreshes the body of an + * already-mounted comment when it changed underneath (an edit from another + * window arriving via the store's file watcher). + */ + async function reconcile(): Promise { + const entries = getDiffInjectEntries(); + const queues = new Map(); + for (const entry of entries) { + if (queues.has(entry.builderId)) { continue; } + if (!registerEntryWorktree(entry)) { continue; } + if (!loaded.has(entry.builderId)) { + loaded.add(entry.builderId); + await store.load(entry.builderId); + } + queues.set(entry.builderId, store.getComments(entry.builderId)); + } + + const plan = planThreadReconcile(entries, queues, new Set(mounted.keys())); + for (const id of plan.toDispose) { + mounted.get(id)?.dispose(); + mounted.delete(id); + } + for (const { comment, builderId, fsPath } of plan.toCreate) { + mountQueuedComment(fsPath, builderId, comment); + } + + // In-place body refresh for surviving threads. + for (const [builderId, comments] of queues) { + for (const comment of comments) { + const thread = mounted.get(comment.id); + if (!thread) { continue; } + const rendered = thread.comments[0] as BuilderReviewComment | undefined; + if (!rendered || rendered.builderId !== builderId) { continue; } + if (rendered.mode === vscode.CommentMode.Editing) { continue; } + if (bodyText(rendered.body) !== comment.body) { + rendered.body = new vscode.MarkdownString(comment.body); + rendered.savedBody = comment.body; + thread.comments = [...thread.comments]; + } + } + } + } + + /** Open an empty input thread at the given anchor (codelens / context menu). */ + function mountInputThread(fsPath: string, range: LineRange | null): void { + const line = anchorLine(fsPath, range); + const thread = controller.createCommentThread( + vscode.Uri.file(fsPath), + new vscode.Range(line, 0, line, 0), + [], + ); + thread.collapsibleState = vscode.CommentThreadCollapsibleState.Expanded; + thread.canReply = true; + pendingRanges.set(thread, range); + } + + const reg = (id: string, fn: (...args: never[]) => unknown): void => { + // eslint-disable-next-line no-restricted-syntax -- CLI-independent commands (local queue state), no regCli guard wanted (#791) + context.subscriptions.push(vscode.commands.registerCommand(id, fn)); + }; + + // Comment-mode codelens entry point. Args match the lens descriptors built + // in diff-inject-codelens.ts. + reg(COMMENT_FOR_BUILDER_COMMAND, ( + _builderId: string, + fsPath: string, + _relPath: string, + range: LineRange | null, + ) => { + mountInputThread(fsPath, range); + }); + + // Context-menu entry: selection if present, else the cursor line. Available + // in both modes (the menu always shows both actions). + reg('codev.commentSelectionForBuilder', () => { + const editor = vscode.window.activeTextEditor; + if (!editor) { return; } + const entry = getDiffInjectEntry(editor.document.uri.fsPath); + if (!entry) { return; } + const sel = editor.selection; + let start = sel.start.line + 1; + let end = sel.end.line + 1; + if (sel.isEmpty) { + start = editor.selection.active.line + 1; + end = start; + } else if (sel.end.character === 0 && sel.end.line > sel.start.line) { + // A selection ending at column 0 of a line doesn't include that line. + end = sel.end.line; + } + mountInputThread(entry.fsPath, { start, end }); + }); + + // 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) => { + const thread = reply.thread; + const entry = getDiffInjectEntry(thread.uri.fsPath); + if (!entry || !registerEntryWorktree(entry)) { + vscode.window.showWarningMessage('Codev: This file is not part of an active builder diff'); + return; + } + let lineRange: LineRange | null = threadLineRange(thread); + if (pendingRanges.has(thread)) { + lineRange = pendingRanges.get(thread)!; + } + const comment: PendingComment = { + id: randomUUID(), + createdAt: new Date().toISOString(), + file: entry.relPath, + lineRange, + body: reply.text, + }; + pendingRanges.delete(thread); + thread.dispose(); + await store.add(entry.builderId, comment); + }); + + // Edit flow (#1055 pattern): flip to VS Code's inline edit surface; + // reassigning `thread.comments` is required for a re-render. + reg('codev.startEditBuilderComment', (comment: BuilderReviewComment) => { + const thread = comment.parent; + if (!thread) { return; } + comment.savedBody = bodyText(comment.body); + comment.mode = vscode.CommentMode.Editing; + thread.comments = [...thread.comments]; + }); + + reg('codev.cancelEditBuilderComment', (comment: BuilderReviewComment) => { + const thread = comment.parent; + if (!thread) { return; } + comment.body = new vscode.MarkdownString(comment.savedBody); + comment.mode = vscode.CommentMode.Preview; + thread.comments = [...thread.comments]; + }); + + reg('codev.saveEditBuilderComment', async (comment: BuilderReviewComment) => { + const thread = comment.parent; + if (!thread) { return; } + const newBody = bodyText(comment.body); + comment.mode = vscode.CommentMode.Preview; + comment.savedBody = newBody; + thread.comments = [...thread.comments]; + await store.edit(comment.builderId, comment.commentId, newBody); + }); + + // Delete from the thread title bar: drop from the queue; the store event + // drives the reconciler, which disposes the thread. + reg('codev.deleteBuilderComment', async (thread: vscode.CommentThread) => { + const rendered = thread.comments[0] as BuilderReviewComment | undefined; + if (!rendered) { + thread.dispose(); + return; + } + await store.remove(rendered.builderId, [rendered.commentId]); + }); + + context.subscriptions.push( + onDidChangeDiffInjectRegistry(() => { reconcile(); }), + store.onDidChangeQueue(() => { reconcile(); }), + new vscode.Disposable(() => { + for (const thread of mounted.values()) { thread.dispose(); } + mounted.clear(); + }), + ); + reconcile(); +} diff --git a/apps/vscode/src/review-queue/reconcile.ts b/apps/vscode/src/review-queue/reconcile.ts new file mode 100644 index 000000000..ae1cfff3a --- /dev/null +++ b/apps/vscode/src/review-queue/reconcile.ts @@ -0,0 +1,75 @@ +/** + * Pure planning logic for mounting queued review comments as visible comment + * threads in the builder diff (#1037, plan decision 6). No `vscode` import — + * `comments/builder-review.ts` executes the returned plan against the real + * Comments API. + * + * A queued comment is visible exactly when its file is in the active + * diff-inject session for its builder (i.e. the reviewer has that builder's + * diff open). Comments for unopened files stay queued but invisible; threads + * whose comment left the queue (submitted / deleted) or whose file left the + * session are disposed. + */ + +import type { PendingComment } from './queue.js'; + +/** The subset of a diff-inject session entry the planner needs. */ +export interface RegisteredFile { + fsPath: string; + builderId: string; + relPath: string; +} + +/** A thread to create: the comment plus where it mounts. */ +export interface ThreadPlanEntry { + comment: PendingComment; + builderId: string; + fsPath: string; +} + +export interface ThreadReconcilePlan { + toCreate: ThreadPlanEntry[]; + /** Comment ids whose mounted thread must be disposed. */ + toDispose: string[]; +} + +/** + * Diff the desired thread set (registered files × queued comments) against the + * currently mounted comment ids. + */ +export function planThreadReconcile( + registered: readonly RegisteredFile[], + queues: ReadonlyMap, + mountedIds: ReadonlySet, +): ThreadReconcilePlan { + const desired = new Map(); + for (const file of registered) { + const comments = queues.get(file.builderId) ?? []; + for (const comment of comments) { + if (comment.file === file.relPath) { + desired.set(comment.id, { comment, builderId: file.builderId, fsPath: file.fsPath }); + } + } + } + const toCreate: ThreadPlanEntry[] = []; + for (const [id, entry] of desired) { + if (!mountedIds.has(id)) { toCreate.push(entry); } + } + const toDispose: string[] = []; + for (const id of mountedIds) { + if (!desired.has(id)) { toDispose.push(id); } + } + return { toCreate, toDispose }; +} + +/** + * Recover a worktree root from a diff-inject entry: `fsPath` is always + * `join(worktreePath, relPath)` (see `view-diff.ts`), so stripping the + * relative suffix yields the worktree. Returns null when the suffix doesn't + * match (defensive — never guess a root to write state under). + */ +export function deriveWorktreePath(fsPath: string, relPath: string, sep: string): string | null { + const suffix = sep + relPath.split('/').join(sep); + if (!fsPath.endsWith(suffix)) { return null; } + return fsPath.slice(0, fsPath.length - suffix.length); +} From f0383162eca55dc512e7d8f2eb2ff37f9e4ee244 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Thu, 6 Aug 2026 10:33:22 +1000 Subject: [PATCH 10/27] [PIR #1037] Submit Review: batched bracketed-paste flush to builder PTY, status-bar counter, wiring --- .../src/__tests__/submit-review.test.ts | 159 ++++++++++++++++++ apps/vscode/src/diff-inject-codelens.ts | 12 +- apps/vscode/src/extension.ts | 53 +++++- apps/vscode/src/review-queue/status-bar.ts | 45 +++++ apps/vscode/src/review-queue/submit.ts | 112 ++++++++++++ 5 files changed, 368 insertions(+), 13 deletions(-) create mode 100644 apps/vscode/src/__tests__/submit-review.test.ts create mode 100644 apps/vscode/src/review-queue/status-bar.ts create mode 100644 apps/vscode/src/review-queue/submit.ts diff --git a/apps/vscode/src/__tests__/submit-review.test.ts b/apps/vscode/src/__tests__/submit-review.test.ts new file mode 100644 index 000000000..1f5a89cd8 --- /dev/null +++ b/apps/vscode/src/__tests__/submit-review.test.ts @@ -0,0 +1,159 @@ +/** + * Submit Review flow (#1037): target-builder resolution (active diff owner → + * sole pending builder → QuickPick), open-terminal-then-inject ordering with + * the bracketed-paste wrapped message (no trailing Enter), clearing exactly + * the submitted ids, and the queue surviving a failed injection. + */ + +import { describe, it, expect, beforeEach, vi } from 'vitest'; + +const h = vi.hoisted(() => { + const state = { + activeEditorFsPath: undefined as string | undefined, + quickPickResult: undefined as { label: string } | undefined, + warnings: [] as string[], + statusMessages: [] as string[], + }; + return { state }; +}); + +vi.mock('vscode', () => ({ + EventEmitter: class { + event = (): { dispose(): void } => ({ dispose() {} }); + fire(): void {} + dispose(): void {} + }, + RelativePattern: class {}, + window: { + get activeTextEditor() { + if (!h.state.activeEditorFsPath) { return undefined; } + return { document: { uri: { fsPath: h.state.activeEditorFsPath } } }; + }, + showQuickPick: vi.fn(async () => h.state.quickPickResult), + showWarningMessage: vi.fn(async (msg: string) => { + h.state.warnings.push(msg); + return undefined; + }), + setStatusBarMessage: vi.fn((msg: string) => { h.state.statusMessages.push(msg); }), + }, + workspace: { + createFileSystemWatcher: vi.fn(), + getConfiguration: () => ({ get: () => 'comment' }), + onDidChangeConfiguration: () => ({ dispose() {} }), + }, + commands: { executeCommand: () => Promise.resolve(undefined) }, + languages: { registerCodeLensProvider: () => ({ dispose() {} }) }, + Range: class {}, + CodeLens: class {}, +})); + +const { submitReview, resolveTargetBuilder } = await import('../review-queue/submit.js'); +const { setDiffInjectSession, upsertDiffInjectEntry } = await import('../diff-inject-codelens.js'); +const { wrapBracketedPaste, buildSubmitMessage } = await import('../review-queue/queue.js'); + +function makeComment(id: string) { + return { id, createdAt: 't', file: 'src/a.ts', lineRange: { start: 1, end: 2 }, body: `b-${id}` }; +} + +/** Minimal in-memory stand-in for ReviewQueueStore. */ +function makeStore(queues: Record[]>) { + const removed: Array<{ builderId: string; ids: readonly string[] }> = []; + return { + removed, + getWorktreePath: () => '/wt', + registerWorktree: () => {}, + buildersWithPending: () => Object.keys(queues).filter(k => queues[k]!.length > 0), + count: (id: string) => (queues[id] ?? []).length, + load: async (id: string) => queues[id] ?? [], + getComments: (id: string) => queues[id] ?? [], + remove: async (builderId: string, ids: readonly string[]) => { removed.push({ builderId, ids }); }, + }; +} + +function makeTerminalManager(injectResult = true) { + const calls: Array<{ method: string; args: unknown[] }> = []; + return { + calls, + openBuilderByRoleOrId: vi.fn(async (id: string) => { + calls.push({ method: 'open', args: [id] }); + return id; + }), + injectBuilderText: vi.fn((id: string, text: string) => { + calls.push({ method: 'inject', args: [id, text] }); + return injectResult; + }), + }; +} + +const overviewCache = { getData: () => ({ builders: [], currentUser: 'amr' }) }; + +beforeEach(() => { + h.state.activeEditorFsPath = undefined; + h.state.quickPickResult = undefined; + h.state.warnings.length = 0; + h.state.statusMessages.length = 0; + setDiffInjectSession([]); +}); + +describe('resolveTargetBuilder', () => { + it('prefers the active builder-diff file’s owner', async () => { + upsertDiffInjectEntry({ fsPath: '/wt/src/a.ts', builderId: 'pir-1', relPath: 'src/a.ts', hunks: [] }); + h.state.activeEditorFsPath = '/wt/src/a.ts'; + const store = makeStore({ 'pir-2': [makeComment('x')] }); + expect(await resolveTargetBuilder(store as never)).toBe('pir-1'); + }); + + it('falls back to the sole builder with pending comments', async () => { + const store = makeStore({ 'pir-2': [makeComment('x')], 'pir-3': [] }); + expect(await resolveTargetBuilder(store as never)).toBe('pir-2'); + }); + + it('quick-picks when several builders have pending comments', async () => { + h.state.quickPickResult = { label: 'pir-3' }; + const store = makeStore({ 'pir-2': [makeComment('x')], 'pir-3': [makeComment('y')] }); + expect(await resolveTargetBuilder(store as never)).toBe('pir-3'); + }); + + it('reports and returns undefined when nothing is pending anywhere', async () => { + const store = makeStore({}); + expect(await resolveTargetBuilder(store as never)).toBeUndefined(); + expect(h.state.statusMessages.some(m => m.includes('No pending review comments'))).toBe(true); + }); +}); + +describe('submitReview', () => { + it('opens the terminal, injects the wrapped packaged message, clears submitted ids', async () => { + const comments = [makeComment('c1'), makeComment('c2')]; + const store = makeStore({ 'pir-1': comments }); + const tm = makeTerminalManager(); + + await submitReview({ store, terminalManager: tm, overviewCache } as never, 'pir-1'); + + expect(tm.calls.map(c => c.method)).toEqual(['open', 'inject']); + const injected = tm.calls[1]!.args[1] as string; + expect(injected).toBe(wrapBracketedPaste(buildSubmitMessage(comments))); + // Wrapped: bracketed-paste guarded, no raw newlines, no trailing Enter. + expect(injected.startsWith('\x1b[200~')).toBe(true); + expect(injected.endsWith('\x1b[201~')).toBe(true); + expect(injected).not.toContain('\n'); + expect(store.removed).toEqual([{ builderId: 'pir-1', ids: ['c1', 'c2'] }]); + }); + + it('keeps the queue intact when the terminal injection fails', async () => { + const store = makeStore({ 'pir-1': [makeComment('c1')] }); + const tm = makeTerminalManager(false); + + await submitReview({ store, terminalManager: tm, overviewCache } as never, 'pir-1'); + + expect(store.removed).toEqual([]); + expect(h.state.warnings.some(w => w.includes('kept in the queue'))).toBe(true); + }); + + it('does nothing but report when the target builder has no pending comments', async () => { + const store = makeStore({ 'pir-1': [] }); + const tm = makeTerminalManager(); + await submitReview({ store, terminalManager: tm, overviewCache } as never, 'pir-1'); + expect(tm.calls).toEqual([]); + expect(h.state.statusMessages.some(m => m.includes('No pending comments for pir-1'))).toBe(true); + }); +}); diff --git a/apps/vscode/src/diff-inject-codelens.ts b/apps/vscode/src/diff-inject-codelens.ts index 3275dbe56..855803fc7 100644 --- a/apps/vscode/src/diff-inject-codelens.ts +++ b/apps/vscode/src/diff-inject-codelens.ts @@ -165,12 +165,6 @@ const provider = new DiffInjectCodeLensProvider(); * context key in sync with the active editor. Called once at activation, * alongside `activateDiffView`. */ export function activateDiffInjectCodeLens(context: vscode.ExtensionContext): void { - const syncContextKey = (editor: vscode.TextEditor | undefined): void => { - const isBuilderFile = !!editor && provider.get(editor.document.uri.fsPath) !== undefined; - void vscode.commands.executeCommand('setContext', BUILDER_FILE_CONTEXT_KEY, isBuilderFile); - }; - syncContextKey(vscode.window.activeTextEditor); - // Mirror the diffCodelensMode setting into a context key so the // `editor/title` toggle buttons swap on it (#1037), and refresh the lenses // when the setting changes so their titles/commands flip live. @@ -179,6 +173,12 @@ export function activateDiffInjectCodeLens(context: vscode.ExtensionContext): vo }; syncModeKey(); + const syncContextKey = (editor: vscode.TextEditor | undefined): void => { + const isBuilderFile = !!editor && provider.get(editor.document.uri.fsPath) !== undefined; + void vscode.commands.executeCommand('setContext', BUILDER_FILE_CONTEXT_KEY, isBuilderFile); + }; + syncContextKey(vscode.window.activeTextEditor); + context.subscriptions.push( vscode.languages.registerCodeLensProvider({ scheme: 'file' }, provider), vscode.window.onDidChangeActiveTextEditor(syncContextKey), diff --git a/apps/vscode/src/extension.ts b/apps/vscode/src/extension.ts index d94a88d0d..5b82ebaaf 100644 --- a/apps/vscode/src/extension.ts +++ b/apps/vscode/src/extension.ts @@ -34,6 +34,10 @@ import { addReviewComment } from './commands/review.js'; import { activateGateToasts } from './notifications/gate-toast.js'; import { activateReviewDecorations } from './review-decorations.js'; import { activateReviewComments } from './comments/plan-review.js'; +import { activateBuilderReviewComments } from './comments/builder-review.js'; +import { ReviewQueueStore } from './review-queue/store.js'; +import { submitReview, discardReviewComments } from './review-queue/submit.js'; +import { activateSubmitReviewStatusBar } from './review-queue/status-bar.js'; import { MarkdownPreviewProvider } from './markdown-preview/preview-provider.js'; import { BuilderSpawnHandler } from './builder-spawn-handler.js'; import { BuilderTerminalLinkProvider, ReconnectTerminalLinkProvider } from './terminal-link-provider.js'; @@ -247,6 +251,14 @@ export async function activate(context: vscode.ExtensionContext) { terminalManager = new TerminalManager(connectionManager, outputChannel, context.extensionUri, overviewCache); context.subscriptions.push({ dispose: () => terminalManager?.dispose() }); + // Per-builder pending review-comment queues (#1037). The watcher root is + // this window's workspace folder — in the main checkout that covers every + // `.builders//` queue file for cross-window sync. + const reviewQueueStore = new ReviewQueueStore( + vscode.workspace.workspaceFolders?.[0]?.uri.fsPath, + ); + context.subscriptions.push(reviewQueueStore); + // Drive the `codev.terminalFocused` context key so the Cmd/Ctrl+V image // paste binding (#736) only applies when a Codev terminal is focused — // it must never shadow Cmd+V anywhere else. @@ -1150,25 +1162,46 @@ export async function activate(context: vscode.ExtensionContext) { // selected range when symbol/file lenses aren't granular enough. Unlike // the CodeLens, a context-menu action works inside the multi-file View // Diff editor too. Scoped via the `codev.activeEditorIsBuilderFile` - // context key + the built-in `editorHasSelection` in its `when` clause. + // context key. With no selection it forwards the cursor line (#1037: the + // context menu shows this action unconditionally so the forward flow is + // always one right-click away in comment mode; the Cmd/Ctrl+K B + // keybinding keeps its original `editorHasSelection` guard). reg('codev.forwardSelectionToBuilder', async () => { const editor = vscode.window.activeTextEditor; if (!editor) { return; } const entry = getDiffInjectEntry(editor.document.uri.fsPath); if (!entry) { return; } const sel = editor.selection; - if (sel.isEmpty) { return; } - const start = sel.start.line + 1; - // A selection ending at column 0 of a line doesn't include that line. - const end = sel.end.character === 0 && sel.end.line > sel.start.line - ? sel.end.line - : sel.end.line + 1; + let start = sel.start.line + 1; + let end = sel.end.line + 1; + if (sel.isEmpty) { + start = sel.active.line + 1; + end = start; + } else if (sel.end.character === 0 && sel.end.line > sel.start.line) { + // A selection ending at column 0 of a line doesn't include that line. + end = sel.end.line; + } const text = buildBuilderRangeRef(entry.relPath, start, end); const resolvedId = await terminalManager?.openBuilderByRoleOrId(entry.builderId, true); if (resolvedId && !terminalManager?.injectBuilderText(resolvedId, text)) { vscode.window.showWarningMessage('Codev: Builder terminal not available'); } }), + // Submit Review + Discard (#1037): flush / drop the per-builder pending + // comment queue. Builder resolution: active diff's owner → sole pending + // builder → QuickPick. + reg('codev.submitReview', () => + submitReview({ store: reviewQueueStore, terminalManager: terminalManager!, overviewCache })), + reg('codev.discardReviewComments', () => + discardReviewComments({ store: reviewQueueStore, terminalManager: terminalManager!, overviewCache })), + // 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 + // switches TO and persists the choice per workspace. + reg('codev.diffCodelensUseForward', () => + vscode.workspace.getConfiguration('codev').update('diffCodelensMode', 'forward', vscode.ConfigurationTarget.Workspace)), + reg('codev.diffCodelensUseComment', () => + vscode.workspace.getConfiguration('codev').update('diffCodelensMode', 'comment', vscode.ConfigurationTarget.Workspace)), // Forward the whole active diff file as a reference (the Forward File action). reg('codev.forwardCurrentFileToBuilder', async () => { const editor = vscode.window.activeTextEditor; @@ -1340,6 +1373,12 @@ export async function activate(context: vscode.ExtensionContext) { // format produced by `codev.addReviewComment` and review.json snippet. activateReviewComments(context, overviewCache); + // Builder review comments (#1037): inline threads on builder-diff files + // feeding the per-builder pending queue, plus the status-bar Submit Review + // counter. The batched submit itself is `codev.submitReview` above. + activateBuilderReviewComments(context, reviewQueueStore, overviewCache); + activateSubmitReviewStatusBar(context, reviewQueueStore); + // Codev Markdown Preview (#859): a read-only custom editor that renders a // spec/plan/review in the shared artifact-canvas and adds review comments // from the rendered surface. Opt-in via "Reopen With…" or diff --git a/apps/vscode/src/review-queue/status-bar.ts b/apps/vscode/src/review-queue/status-bar.ts new file mode 100644 index 000000000..ace47123e --- /dev/null +++ b/apps/vscode/src/review-queue/status-bar.ts @@ -0,0 +1,45 @@ +/** + * Status-bar Submit Review button (#1037): visible while the active editor is + * a builder-diff file whose builder has pending comments; shows the live + * count and triggers `codev.submitReview` for that builder. Hidden otherwise — + * the palette command remains the anytime entry point. + */ + +import * as vscode from 'vscode'; +import { getDiffInjectEntry, onDidChangeDiffInjectRegistry } from '../diff-inject-codelens.js'; +import type { ReviewQueueStore } from './store.js'; + +export function activateSubmitReviewStatusBar( + context: vscode.ExtensionContext, + store: ReviewQueueStore, +): void { + // Priority 97: just below the Tower connection item (100) and dev chip (99). + const item = vscode.window.createStatusBarItem(vscode.StatusBarAlignment.Left, 97); + item.command = 'codev.submitReview'; + + const update = (): void => { + const editor = vscode.window.activeTextEditor; + let entry; + if (editor) { entry = getDiffInjectEntry(editor.document.uri.fsPath); } + if (!entry) { + item.hide(); + return; + } + const count = store.count(entry.builderId); + if (count === 0) { + item.hide(); + return; + } + item.text = `$(comment) Submit Review (${count})`; + item.tooltip = `Send ${count} pending review comment(s) to builder ${entry.builderId}'s prompt`; + item.show(); + }; + + context.subscriptions.push( + item, + vscode.window.onDidChangeActiveTextEditor(update), + onDidChangeDiffInjectRegistry(update), + store.onDidChangeQueue(update), + ); + update(); +} diff --git a/apps/vscode/src/review-queue/submit.ts b/apps/vscode/src/review-queue/submit.ts new file mode 100644 index 000000000..c7d847e81 --- /dev/null +++ b/apps/vscode/src/review-queue/submit.ts @@ -0,0 +1,112 @@ +/** + * Submit Review + Discard (#1037): flush a builder's pending comment queue to + * its PTY as ONE batched message, or drop the queue without sending. + * + * Submit packages the queue (`buildSubmitMessage`), opens/reveals the builder + * terminal through the same open-and-recover flow as #789's forward command + * (plan decision 4), and types the message into the prompt buffer WITHOUT + * pressing Enter — the reviewer reads the packaged message and submits it + * themselves (deliberate human-in-the-loop step). The message is wrapped in + * bracketed-paste escapes because the PTY receives `sendText` bytes raw: an + * unwrapped `\n` would act as Enter and submit the prompt mid-message. + * + * Only the ids that were packaged are removed on success, so a comment queued + * while the message sits unsent in the prompt buffer survives to the next + * cycle rather than being silently flushed. + */ + +import * as vscode from 'vscode'; +import { buildSubmitMessage, wrapBracketedPaste } from './queue.js'; +import { getDiffInjectEntry } from '../diff-inject-codelens.js'; +import type { ReviewQueueStore } from './store.js'; +import type { TerminalManager } from '../terminal-manager.js'; +import type { OverviewCache } from '../views/overview-data.js'; + +export interface SubmitDeps { + store: ReviewQueueStore; + terminalManager: TerminalManager; + overviewCache: OverviewCache; +} + +/** + * Resolve which builder a queue action targets: the owner of the active + * builder-diff file wins; otherwise the sole builder with pending comments; + * otherwise a QuickPick over builders with non-empty queues. Returns + * undefined on cancel / nothing pending. + */ +export async function resolveTargetBuilder(store: ReviewQueueStore): Promise { + const editor = vscode.window.activeTextEditor; + if (editor) { + const entry = getDiffInjectEntry(editor.document.uri.fsPath); + if (entry) { return entry.builderId; } + } + const pending = store.buildersWithPending(); + if (pending.length === 0) { + vscode.window.setStatusBarMessage('Codev: No pending review comments', 3000); + return undefined; + } + if (pending.length === 1) { return pending[0]; } + const picked = await vscode.window.showQuickPick( + pending.map(id => ({ label: id, description: `${store.count(id)} pending` })), + { placeHolder: 'Select the builder whose review to act on' }, + ); + return picked?.label; +} + +export async function submitReview(deps: SubmitDeps, builderIdArg?: string): Promise { + let builderId = builderIdArg; + if (!builderId) { builderId = await resolveTargetBuilder(deps.store); } + if (!builderId) { return; } + + registerWorktreeFromOverview(deps, builderId); + const comments = await deps.store.load(builderId); + if (comments.length === 0) { + vscode.window.setStatusBarMessage(`Codev: No pending comments for ${builderId}`, 3000); + return; + } + + const message = buildSubmitMessage(comments); + const resolvedId = await deps.terminalManager.openBuilderByRoleOrId(builderId, true); + if (!resolvedId || !deps.terminalManager.injectBuilderText(resolvedId, wrapBracketedPaste(message))) { + vscode.window.showWarningMessage('Codev: Builder terminal not available — review comments kept in the queue'); + return; + } + await deps.store.remove(builderId, comments.map(c => c.id)); + vscode.window.setStatusBarMessage( + `Codev: ${comments.length} review comment(s) placed in ${builderId}'s prompt — press Enter there to send`, + 5000, + ); +} + +export async function discardReviewComments(deps: SubmitDeps, builderIdArg?: string): Promise { + let builderId = builderIdArg; + if (!builderId) { builderId = await resolveTargetBuilder(deps.store); } + if (!builderId) { return; } + + registerWorktreeFromOverview(deps, builderId); + const comments = await deps.store.load(builderId); + if (comments.length === 0) { + vscode.window.setStatusBarMessage(`Codev: No pending comments for ${builderId}`, 3000); + return; + } + const confirm = await vscode.window.showWarningMessage( + `Discard ${comments.length} pending review comment(s) for ${builderId}?`, + { modal: true }, + 'Discard', + ); + if (confirm !== 'Discard') { return; } + await deps.store.clear(builderId); +} + +/** + * Make sure the store knows the builder's worktree even when no diff has been + * opened this session (palette-driven submit after a reload): the Tower + * overview's `worktreePath` is authoritative. + */ +function registerWorktreeFromOverview(deps: SubmitDeps, builderId: string): void { + if (deps.store.getWorktreePath(builderId)) { return; } + const builder = deps.overviewCache.getData()?.builders.find(b => b.id === builderId); + if (builder?.worktreePath) { + deps.store.registerWorktree(builderId, builder.worktreePath); + } +} From 6111a7cca5f425da4b5741d8ded9be417bbcf048 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Thu, 6 Aug 2026 10:34:06 +1000 Subject: [PATCH 11/27] =?UTF-8?q?[PIR=20#1037]=20docs:=20thread=20?= =?UTF-8?q?=E2=80=94=20implement=20phase=20complete?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- codev/state/pir-1037_thread.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/codev/state/pir-1037_thread.md b/codev/state/pir-1037_thread.md index 2f9f2638a..398ab5f5e 100644 --- a/codev/state/pir-1037_thread.md +++ b/codev/state/pir-1037_thread.md @@ -9,3 +9,10 @@ Issue #1037: codelens-driven review comments in the unified diff editor (per-bui - Key design find: PTY injection is raw bytes (`sendText` → `handleInput` → WebSocket), so a multi-line batched message would submit on every `\n` in the Claude REPL. Locked design: bracketed-paste wrapping (`\x1b[200~…\x1b[201~`, `\n`→`\r`); flagged as the top risk, spike first in implement, first item in the dev-approval script. - Gitignore decision: managed block in `$GIT_COMMON_DIR/info/exclude` written by the extension at first queue write; family glob `.builder-*` also silences the scaffolding-file class; committed-.gitignore alternative weighed and documented in the plan. - Plan written to `codev/plans/1037-vscode-codelens-driven-review-.md`; sitting at plan-approval gate. + +## 2026-08-06 Implement phase + +- Plan approved; implemented in 5 commits: pure queue module (schema/packaging/bracketed-paste/exclude-block), ReviewQueueStore (fs + watcher + info/exclude managed block), mode-aware codelenses (`codev.diffCodelensMode` setting + title-bar toggle + always-on context menu), builder-review comment controller (mount/reconcile/edit/delete via Comments API), Submit Review (batched bracketed-paste flush + status-bar counter + palette commands). +- Deviations from plan, to raise at dev-approval: (1) changelog files NOT edited — apps/vscode/CHANGELOG.md and docs/releases/UNRELEASED.md are maintained on the docs/vscode-changelog branch per the template's per-PR workflow; suggested entry text handed to architect instead. (2) `lineRange` made nullable in the schema (null = whole-file comment) so the file-level lens produces `### path` instead of a misleading line ref. (3) #789's context-menu forward action: `when` relaxed from `editorHasSelection` to always-on-builder-files with a cursor-line fallback, to satisfy the issue's "context menu always exposes both" AC; the Cmd/Ctrl+K B keybinding keeps its original selection guard. +- All 694 vscode unit tests pass (37 new across 6 test files); check-types + eslint + esbuild clean. +- Bracketed-paste injection is the one thing unit tests cannot prove — first item in the dev-approval script. From e690d54d1fdc1ba9e26bf0be00790cb2a1fea5a1 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Thu, 6 Aug 2026 10:35:00 +1000 Subject: [PATCH 12/27] chore(porch): 1037 dev-approval gate-requested --- codev/projects/1037-vscode-codelens-driven-review-/status.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml index f1dc48a0d..e441300f9 100644 --- a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml +++ b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml @@ -11,10 +11,11 @@ gates: approved_at: '2026-08-06T00:15:41.906Z' dev-approval: status: pending + requested_at: '2026-08-06T00:35:00.821Z' pr: status: pending iteration: 1 build_complete: false history: [] started_at: '2026-08-05T23:21:45.893Z' -updated_at: '2026-08-06T00:15:49.217Z' +updated_at: '2026-08-06T00:35:00.821Z' From 67cb80d02e74bcf5b8e0f31bbc2b360ea4ea8687 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Mon, 10 Aug 2026 11:57:52 +1000 Subject: [PATCH 13/27] [PIR #1037] Focus comment input on codelens click via built-in addComment command --- apps/vscode/src/comments/builder-review.ts | 90 ++++++++++++++-------- 1 file changed, 56 insertions(+), 34 deletions(-) diff --git a/apps/vscode/src/comments/builder-review.ts b/apps/vscode/src/comments/builder-review.ts index c5ff4ac14..6779a5ed7 100644 --- a/apps/vscode/src/comments/builder-review.ts +++ b/apps/vscode/src/comments/builder-review.ts @@ -93,8 +93,13 @@ export function activateBuilderReviewComments( /** Mounted threads keyed by queued-comment id. */ const mounted = new Map(); - /** Codelens-supplied ranges for in-progress input threads (null = whole file). */ - const pendingRanges = new Map(); + /** + * Set when the file-level lens opened the input: the next submit on this + * file anchored at line 1 records `lineRange: null` (whole-file comment) + * instead of a misleading L1 ref. Short-lived — cleared on every new input + * open and every submit, with a time cap for abandoned inputs. + */ + let pendingWholeFile: { fsPath: string; at: number } | null = null; /** Builders whose queue file has been loaded from disk this session. */ const loaded = new Set(); @@ -186,17 +191,35 @@ export function activateBuilderReviewComments( } } - /** Open an empty input thread at the given anchor (codelens / context menu). */ - function mountInputThread(fsPath: string, range: LineRange | null): void { - const line = anchorLine(fsPath, range); - const thread = controller.createCommentThread( - vscode.Uri.file(fsPath), - new vscode.Range(line, 0, line, 0), - [], - ); - thread.collapsibleState = vscode.CommentThreadCollapsibleState.Expanded; - thread.canReply = true; - pendingRanges.set(thread, range); + /** + * Open the comment input at the given anchor via VS Code's own + * `workbench.action.addComment` ("Add Comment on Current Selection") — the + * same path the gutter "+" takes. A programmatically created thread renders + * expanded but does NOT focus its input (the stable API has no + * `CommentThread.reveal`), forcing a second click into the textbox; the + * built-in command both creates the thread (our range provider covers the + * file) and focuses the input. The thread's range comes from the selection, + * so the lens range is selected first; the eventual submit reads it back + * from `thread.range`. + */ + async function openCommentInput(fsPath: string, range: LineRange | null): Promise { + let editor = vscode.window.activeTextEditor; + if (!editor || editor.document.uri.fsPath !== fsPath) { + editor = vscode.window.visibleTextEditors.find(e => e.document.uri.fsPath === fsPath); + } + if (!editor) { return; } + const lastLine = Math.max(editor.document.lineCount - 1, 0); + let startLine = 0; + let endLine = 0; + if (range) { + startLine = Math.min(Math.max(range.start - 1, 0), lastLine); + endLine = Math.min(Math.max(range.end - 1, startLine), lastLine); + } + editor.selection = new vscode.Selection(startLine, 0, endLine, 0); + editor.revealRange(new vscode.Range(startLine, 0, endLine, 0)); + pendingWholeFile = null; + if (!range) { pendingWholeFile = { fsPath, at: Date.now() }; } + await vscode.commands.executeCommand('workbench.action.addComment'); } const reg = (id: string, fn: (...args: never[]) => unknown): void => { @@ -206,33 +229,24 @@ export function activateBuilderReviewComments( // Comment-mode codelens entry point. Args match the lens descriptors built // in diff-inject-codelens.ts. - reg(COMMENT_FOR_BUILDER_COMMAND, ( + reg(COMMENT_FOR_BUILDER_COMMAND, async ( _builderId: string, fsPath: string, _relPath: string, range: LineRange | null, ) => { - mountInputThread(fsPath, range); + await openCommentInput(fsPath, range); }); - // Context-menu entry: selection if present, else the cursor line. Available - // in both modes (the menu always shows both actions). - reg('codev.commentSelectionForBuilder', () => { + // Context-menu entry: the user's own selection (or cursor line) is already + // what `workbench.action.addComment` consumes, so no selection surgery is + // needed here. Available in both modes (the menu always shows both actions). + reg('codev.commentSelectionForBuilder', async () => { const editor = vscode.window.activeTextEditor; if (!editor) { return; } - const entry = getDiffInjectEntry(editor.document.uri.fsPath); - if (!entry) { return; } - const sel = editor.selection; - let start = sel.start.line + 1; - let end = sel.end.line + 1; - if (sel.isEmpty) { - start = editor.selection.active.line + 1; - end = start; - } else if (sel.end.character === 0 && sel.end.line > sel.start.line) { - // A selection ending at column 0 of a line doesn't include that line. - end = sel.end.line; - } - mountInputThread(entry.fsPath, { start, end }); + if (!getDiffInjectEntry(editor.document.uri.fsPath)) { return; } + pendingWholeFile = null; + await vscode.commands.executeCommand('workbench.action.addComment'); }); // Submit button on an input thread → queue the comment. The input thread is @@ -245,9 +259,18 @@ export function activateBuilderReviewComments( return; } let lineRange: LineRange | null = threadLineRange(thread); - if (pendingRanges.has(thread)) { - lineRange = pendingRanges.get(thread)!; + // File-level lens flow: the input was opened at line 1 purely as a mount + // point; record the comment as whole-file. The 30s cap keeps a marker from + // an abandoned input from reclassifying a genuine line-1 comment later. + if ( + pendingWholeFile && + pendingWholeFile.fsPath === thread.uri.fsPath && + Date.now() - pendingWholeFile.at < 30_000 && + lineRange.start === 1 && lineRange.end === 1 + ) { + lineRange = null; } + pendingWholeFile = null; const comment: PendingComment = { id: randomUUID(), createdAt: new Date().toISOString(), @@ -255,7 +278,6 @@ export function activateBuilderReviewComments( lineRange, body: reply.text, }; - pendingRanges.delete(thread); thread.dispose(); await store.add(entry.builderId, comment); }); From f58047ffe7b1b9bf8dca3425944e3e9b37ba193b Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Mon, 10 Aug 2026 12:07:26 +1000 Subject: [PATCH 14/27] [PIR #1037] Comment input: pass range args to addComment, refresh commenting ranges on registry change --- apps/vscode/src/comments/builder-review.ts | 105 ++++++++++++--------- 1 file changed, 58 insertions(+), 47 deletions(-) diff --git a/apps/vscode/src/comments/builder-review.ts b/apps/vscode/src/comments/builder-review.ts index 6779a5ed7..8a5746b8b 100644 --- a/apps/vscode/src/comments/builder-review.ts +++ b/apps/vscode/src/comments/builder-review.ts @@ -26,6 +26,7 @@ import { getDiffInjectEntries, onDidChangeDiffInjectRegistry, COMMENT_FOR_BUILDER_COMMAND, + DIFF_CODELENS_MODE_KEY, } from '../diff-inject-codelens.js'; import { planThreadReconcile, deriveWorktreePath, type RegisteredFile } from '../review-queue/reconcile.js'; import type { ReviewQueueStore } from '../review-queue/store.js'; @@ -82,24 +83,32 @@ export function activateBuilderReviewComments( // Gutter "+" on registered builder-diff files, comment mode only — in // forward mode the comment surface recedes so the two modes stay distinct. - controller.commentingRangeProvider = { + // `enableFileComments` lets `workbench.action.addComment` create a + // range-less file comment (the file-level lens flow). + const rangeProvider: vscode.CommentingRangeProvider = { provideCommentingRanges(document) { if (getDiffCodelensMode() !== 'comment') { return []; } if (!getDiffInjectEntry(document.uri.fsPath)) { return []; } const lastLine = Math.max(0, document.lineCount - 1); - return [new vscode.Range(0, 0, lastLine, 0)]; + return { enableFileComments: true, ranges: [new vscode.Range(0, 0, lastLine, 0)] }; }, }; + controller.commentingRangeProvider = rangeProvider; + + // VS Code caches each document's commenting ranges and only re-queries on + // its own triggers — none of which fire when the diff-inject registry + // registers a file AFTER its editor opened (openBuilderFileDiff opens the + // diff first; same ordering as the #789 context-key fix). Re-assigning the + // provider goes through the extension-host setter, which calls + // `$updateCommentingRanges` and makes the editor recompute — without this, + // the gutter "+" is missing and `workbench.action.addComment` rejects with + // "cursor must be within a commenting range" on a freshly opened diff. + const refreshCommentingRanges = (): void => { + controller.commentingRangeProvider = rangeProvider; + }; /** Mounted threads keyed by queued-comment id. */ const mounted = new Map(); - /** - * Set when the file-level lens opened the input: the next submit on this - * file anchored at line 1 records `lineRange: null` (whole-file comment) - * instead of a misleading L1 ref. Short-lived — cleared on every new input - * open and every submit, with a time cap for abandoned inputs. - */ - let pendingWholeFile: { fsPath: string; at: number } | null = null; /** Builders whose queue file has been loaded from disk this session. */ const loaded = new Set(); @@ -132,6 +141,11 @@ export function activateBuilderReviewComments( new vscode.Range(line, 0, line, 0), [], ); + if (!comment.lineRange) { + // Whole-file comment: render as a range-less file comment (the factory + // signature requires a Range, but the property accepts undefined). + thread.range = undefined; + } const rendered = new BuilderReviewComment( new vscode.MarkdownString(comment.body), author(), @@ -193,33 +207,33 @@ export function activateBuilderReviewComments( /** * Open the comment input at the given anchor via VS Code's own - * `workbench.action.addComment` ("Add Comment on Current Selection") — the - * same path the gutter "+" takes. A programmatically created thread renders - * expanded but does NOT focus its input (the stable API has no - * `CommentThread.reveal`), forcing a second click into the textbox; the - * built-in command both creates the thread (our range provider covers the - * file) and focuses the input. The thread's range comes from the selection, - * so the lens range is selected first; the eventual submit reads it back - * from `thread.range`. + * `workbench.action.addComment` — the same path the gutter "+" takes. A + * programmatically created thread renders expanded but does NOT focus its + * input (the stable API has no `CommentThread.reveal`), forcing a second + * click into the textbox; the built-in command both creates the thread (our + * range provider covers the file) and focuses the input. + * + * The command takes an explicit args object — `range` (1-based editor-core + * coordinates) or `fileComment` — so the anchor is passed directly instead + * of mutating the editor selection (which flashed a highlight). A + * `fileComment` thread materializes with `thread.range === undefined`, + * which the submit handler records as a whole-file comment. */ async function openCommentInput(fsPath: string, range: LineRange | null): Promise { - let editor = vscode.window.activeTextEditor; - if (!editor || editor.document.uri.fsPath !== fsPath) { - editor = vscode.window.visibleTextEditors.find(e => e.document.uri.fsPath === fsPath); - } - if (!editor) { return; } - const lastLine = Math.max(editor.document.lineCount - 1, 0); - let startLine = 0; - let endLine = 0; + const editor = vscode.window.activeTextEditor; + if (!editor || editor.document.uri.fsPath !== fsPath) { return; } if (range) { - startLine = Math.min(Math.max(range.start - 1, 0), lastLine); - endLine = Math.min(Math.max(range.end - 1, startLine), lastLine); + await vscode.commands.executeCommand('workbench.action.addComment', { + range: { + startLineNumber: range.start, + startColumn: 1, + endLineNumber: range.end, + endColumn: 1, + }, + }); + return; } - editor.selection = new vscode.Selection(startLine, 0, endLine, 0); - editor.revealRange(new vscode.Range(startLine, 0, endLine, 0)); - pendingWholeFile = null; - if (!range) { pendingWholeFile = { fsPath, at: Date.now() }; } - await vscode.commands.executeCommand('workbench.action.addComment'); + await vscode.commands.executeCommand('workbench.action.addComment', { fileComment: true }); } const reg = (id: string, fn: (...args: never[]) => unknown): void => { @@ -245,7 +259,6 @@ export function activateBuilderReviewComments( const editor = vscode.window.activeTextEditor; if (!editor) { return; } if (!getDiffInjectEntry(editor.document.uri.fsPath)) { return; } - pendingWholeFile = null; await vscode.commands.executeCommand('workbench.action.addComment'); }); @@ -258,19 +271,9 @@ export function activateBuilderReviewComments( vscode.window.showWarningMessage('Codev: This file is not part of an active builder diff'); return; } - let lineRange: LineRange | null = threadLineRange(thread); - // File-level lens flow: the input was opened at line 1 purely as a mount - // point; record the comment as whole-file. The 30s cap keeps a marker from - // an abandoned input from reclassifying a genuine line-1 comment later. - if ( - pendingWholeFile && - pendingWholeFile.fsPath === thread.uri.fsPath && - Date.now() - pendingWholeFile.at < 30_000 && - lineRange.start === 1 && lineRange.end === 1 - ) { - lineRange = null; - } - pendingWholeFile = null; + // A range-less thread is a file comment (the file-level lens flow). + let lineRange: LineRange | null = null; + if (thread.range) { lineRange = threadLineRange(thread); } const comment: PendingComment = { id: randomUUID(), createdAt: new Date().toISOString(), @@ -322,8 +325,16 @@ export function activateBuilderReviewComments( }); context.subscriptions.push( - onDidChangeDiffInjectRegistry(() => { reconcile(); }), + onDidChangeDiffInjectRegistry(() => { + refreshCommentingRanges(); + reconcile(); + }), store.onDidChangeQueue(() => { reconcile(); }), + // Mode flips change what provideCommentingRanges returns; force the + // recompute so the gutter "+" appears/recedes with the toggle. + vscode.workspace.onDidChangeConfiguration(e => { + if (e.affectsConfiguration(DIFF_CODELENS_MODE_KEY)) { refreshCommentingRanges(); } + }), new vscode.Disposable(() => { for (const thread of mounted.values()) { thread.dispose(); } mounted.clear(); From 24ba4610a38e3f2802b39445bd4d1cda2d271f86 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Mon, 10 Aug 2026 12:13:44 +1000 Subject: [PATCH 15/27] [PIR #1037] Mount queued threads on their full range so the widget sits after the last line --- apps/vscode/src/comments/builder-review.ts | 30 +++++++++++++--------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/apps/vscode/src/comments/builder-review.ts b/apps/vscode/src/comments/builder-review.ts index 8a5746b8b..836353b5e 100644 --- a/apps/vscode/src/comments/builder-review.ts +++ b/apps/vscode/src/comments/builder-review.ts @@ -125,22 +125,28 @@ export function activateBuilderReviewComments( return true; } - /** Anchor line for a queued comment, clamped to the open document if any. */ - function anchorLine(fsPath: string, range: LineRange | null): number { - let line = 0; - if (range) { line = Math.max(range.start - 1, 0); } + /** + * A queued comment's full anchor range (0-based, clamped to the open + * document if any). The thread must span the WHOLE recorded range, not just + * its first line — the widget renders after the range's last line, so a + * start-line-only thread would visually cut through the commented lines. + */ + function anchorRange(fsPath: string, range: LineRange): vscode.Range { + let start = Math.max(range.start - 1, 0); + let end = Math.max(range.end - 1, start); const doc = vscode.workspace.textDocuments.find(d => d.uri.fsPath === fsPath); - if (doc) { line = Math.min(line, Math.max(doc.lineCount - 1, 0)); } - return line; + if (doc) { + const lastLine = Math.max(doc.lineCount - 1, 0); + start = Math.min(start, lastLine); + end = Math.min(end, lastLine); + } + return new vscode.Range(start, 0, end, 0); } function mountQueuedComment(fsPath: string, builderId: string, comment: PendingComment): void { - const line = anchorLine(fsPath, comment.lineRange); - const thread = controller.createCommentThread( - vscode.Uri.file(fsPath), - new vscode.Range(line, 0, line, 0), - [], - ); + let range = new vscode.Range(0, 0, 0, 0); + if (comment.lineRange) { range = anchorRange(fsPath, comment.lineRange); } + const thread = controller.createCommentThread(vscode.Uri.file(fsPath), range, []); if (!comment.lineRange) { // Whole-file comment: render as a range-less file comment (the factory // signature requires a Range, but the property accepts undefined). From 1f8e3218673039301c69c67e072a51710c8c92ae Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Mon, 10 Aug 2026 12:18:30 +1000 Subject: [PATCH 16/27] [PIR #1037] Extend thread ranges to last-line content end so the range highlight covers every line --- apps/vscode/src/comments/builder-review.ts | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/apps/vscode/src/comments/builder-review.ts b/apps/vscode/src/comments/builder-review.ts index 836353b5e..5d9b96042 100644 --- a/apps/vscode/src/comments/builder-review.ts +++ b/apps/vscode/src/comments/builder-review.ts @@ -38,6 +38,10 @@ const CONTROLLER_ID = 'codev-builder-review'; /** contextValue on threads/comments — matched by the `comments/*` menu `when` clauses. */ const PENDING_CONTEXT = 'pending-builder-comment'; +/** Fallback end column when the document isn't open to measure the last + * line's length — far past any real line; the editor clamps to content. */ +const LAST_COLUMN = 1 << 20; + /** A mounted queued comment. Carries its queue identity for edit/delete. */ class BuilderReviewComment implements vscode.Comment { public parent?: vscode.CommentThread; @@ -130,17 +134,22 @@ export function activateBuilderReviewComments( * document if any). The thread must span the WHOLE recorded range, not just * its first line — the widget renders after the range's last line, so a * start-line-only thread would visually cut through the commented lines. + * The range ends at the last line's content end, NOT column 0 of that line: + * a column-0 end covers zero characters of the last line, so the comment + * range highlight would skip it (paint 129 but not 130 of a 129-130 span). */ function anchorRange(fsPath: string, range: LineRange): vscode.Range { let start = Math.max(range.start - 1, 0); let end = Math.max(range.end - 1, start); + let endColumn = LAST_COLUMN; const doc = vscode.workspace.textDocuments.find(d => d.uri.fsPath === fsPath); if (doc) { const lastLine = Math.max(doc.lineCount - 1, 0); start = Math.min(start, lastLine); end = Math.min(end, lastLine); + endColumn = doc.lineAt(end).text.length; } - return new vscode.Range(start, 0, end, 0); + return new vscode.Range(start, 0, end, endColumn); } function mountQueuedComment(fsPath: string, builderId: string, comment: PendingComment): void { @@ -229,12 +238,18 @@ export function activateBuilderReviewComments( const editor = vscode.window.activeTextEditor; if (!editor || editor.document.uri.fsPath !== fsPath) { return; } 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 + // highlight entirely. + let endColumn = LAST_COLUMN; + const endLine = Math.min(Math.max(range.end - 1, 0), editor.document.lineCount - 1); + if (endLine >= 0) { endColumn = editor.document.lineAt(endLine).text.length + 1; } await vscode.commands.executeCommand('workbench.action.addComment', { range: { startLineNumber: range.start, startColumn: 1, endLineNumber: range.end, - endColumn: 1, + endColumn, }, }); return; From a094352e34e0004eee68b2675d6d091105e8e5d6 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Mon, 10 Aug 2026 12:35:39 +1000 Subject: [PATCH 17/27] [PIR #1037] Flip diffCodelensMode default to forward (preserve #789 for existing users) --- apps/vscode/package.json | 2 +- .../src/__tests__/contributes-review-queue.test.ts | 4 ++-- apps/vscode/src/__tests__/diff-codelens-mode.test.ts | 11 ++++++++++- apps/vscode/src/diff-inject-codelens.ts | 9 +++++---- codev/state/pir-1037_thread.md | 9 +++++++++ 5 files changed, 27 insertions(+), 8 deletions(-) diff --git a/apps/vscode/package.json b/apps/vscode/package.json index ad99d1c52..adf0945ce 100644 --- a/apps/vscode/package.json +++ b/apps/vscode/package.json @@ -1040,7 +1040,7 @@ "Codelenses compose a review comment in an inline thread; comments queue per builder and reach the PTY only via Submit Review.", "Codelenses type the file/line reference straight into the builder terminal (the original fire-and-forget flow)." ], - "default": "comment", + "default": "forward", "description": "Which single action the codelenses in a builder diff offer. The right-click context menu always offers both actions regardless of mode. Toggle via the diff editor's title-bar button; the choice persists per workspace." }, "codev.maxTerminals": { diff --git a/apps/vscode/src/__tests__/contributes-review-queue.test.ts b/apps/vscode/src/__tests__/contributes-review-queue.test.ts index 38f0cc2ca..a77d00fa6 100644 --- a/apps/vscode/src/__tests__/contributes-review-queue.test.ts +++ b/apps/vscode/src/__tests__/contributes-review-queue.test.ts @@ -30,11 +30,11 @@ function entry(section: string, command: string): MenuEntry | undefined { } describe('codev.diffCodelensMode setting', () => { - it('is declared with enum comment|forward and default comment', () => { + it('is declared with enum comment|forward and default forward (preserves #789 for existing users)', () => { const prop = PKG.contributes.configuration.properties['codev.diffCodelensMode']; expect(prop).toBeDefined(); expect(prop.enum).toEqual(['comment', 'forward']); - expect(prop.default).toBe('comment'); + expect(prop.default).toBe('forward'); }); }); diff --git a/apps/vscode/src/__tests__/diff-codelens-mode.test.ts b/apps/vscode/src/__tests__/diff-codelens-mode.test.ts index ed2c0b2e4..df7648e76 100644 --- a/apps/vscode/src/__tests__/diff-codelens-mode.test.ts +++ b/apps/vscode/src/__tests__/diff-codelens-mode.test.ts @@ -103,7 +103,7 @@ beforeEach(() => { }); describe('diff codelens mode (#1037)', () => { - it('comment mode (default) emits Comment for Builder lenses with the comment command', async () => { + it('comment mode emits Comment for Builder lenses with the comment command', async () => { const all = await lenses(); expect(all.length).toBeGreaterThan(0); for (const lens of all) { @@ -127,6 +127,15 @@ describe('diff codelens mode (#1037)', () => { expect(all[0]!.command.arguments).toEqual(['pir-9', 'pkg/src/a.ts ']); }); + it('an unset or unrecognized setting value falls back to forward (the default)', async () => { + h.state.mode = undefined as never; + let all = await lenses(); + expect(all[0]!.command.command).toBe('codev.forwardToBuilder'); + h.state.mode = 'garbage'; + all = await lenses(); + expect(all[0]!.command.command).toBe('codev.forwardToBuilder'); + }); + it('exactly one lens per anchor line in either mode', async () => { for (const mode of ['comment', 'forward']) { h.state.mode = mode; diff --git a/apps/vscode/src/diff-inject-codelens.ts b/apps/vscode/src/diff-inject-codelens.ts index 855803fc7..479a4cd42 100644 --- a/apps/vscode/src/diff-inject-codelens.ts +++ b/apps/vscode/src/diff-inject-codelens.ts @@ -48,11 +48,12 @@ export const DIFF_CODELENS_MODE_KEY = 'codev.diffCodelensMode'; export type DiffCodelensMode = 'comment' | 'forward'; -/** Read the current mode from configuration (default: comment). */ +/** Read the current mode from configuration (default: forward — #789's + * original behavior is preserved for users who never touch the setting). */ export function getDiffCodelensMode(): DiffCodelensMode { const value = vscode.workspace.getConfiguration('codev').get('diffCodelensMode'); - if (value === 'forward') { return 'forward'; } - return 'comment'; + if (value === 'comment') { return 'comment'; } + return 'forward'; } /** One changed file in the active diff session, keyed by its right-side fs path. */ @@ -131,7 +132,7 @@ class DiffInjectCodeLensProvider implements vscode.CodeLensProvider { const nodes = symbols.map(toSymbolNode); const lastLine = Math.max(document.lineCount - 1, 0); // Exactly one lens per anchor, matching the current mode (#1037): comment - // mode (default) mounts an inline thread; forward mode keeps #789's + // mode mounts an inline thread; forward mode (default) keeps #789's // fire-and-forget injection. Same anchors either way. const mode = getDiffCodelensMode(); let label = 'Comment for Builder'; diff --git a/codev/state/pir-1037_thread.md b/codev/state/pir-1037_thread.md index 398ab5f5e..dbbe8bbb7 100644 --- a/codev/state/pir-1037_thread.md +++ b/codev/state/pir-1037_thread.md @@ -16,3 +16,12 @@ Issue #1037: codelens-driven review comments in the unified diff editor (per-bui - Deviations from plan, to raise at dev-approval: (1) changelog files NOT edited — apps/vscode/CHANGELOG.md and docs/releases/UNRELEASED.md are maintained on the docs/vscode-changelog branch per the template's per-PR workflow; suggested entry text handed to architect instead. (2) `lineRange` made nullable in the schema (null = whole-file comment) so the file-level lens produces `### path` instead of a misleading line ref. (3) #789's context-menu forward action: `when` relaxed from `editorHasSelection` to always-on-builder-files with a cursor-line fallback, to satisfy the issue's "context menu always exposes both" AC; the Cmd/Ctrl+K B keybinding keeps its original selection guard. - All 694 vscode unit tests pass (37 new across 6 test files); check-types + eslint + esbuild clean. - Bracketed-paste injection is the one thing unit tests cannot prove — first item in the dev-approval script. + +## 2026-08-10 Dev-approval gate iteration (live testing feedback) + +- Comment input required a second click to focus: replaced programmatic thread creation for input with the built-in `workbench.action.addComment` (args-based: `{range}` / `{fileComment: true}`), which creates + focuses natively. Whole-file marker hack deleted; provider now returns `enableFileComments: true`. +- "Cursor must be within a commenting range" on fresh diffs: VS Code caches commenting ranges per document and never re-queries when the registry registers a file post-open; re-assigning `commentingRangeProvider` on registry change (and mode change) forces the recompute via the ext-host setter. +- Re-mounted queued threads anchored at start line only (widget cut through the range) → mount on the full recorded range. +- Range highlight skipped the last line (range ended at column 0) → extend to last-line content end in both input and mount paths. +- Third report (widget "mid-hunk") verified NOT a bug via stored JSON: gutter/selection comment 133-150 rendered faithfully; hunk lens is the whole-hunk entry point. +- **Human decision at gate: default mode flipped to `forward`** — preserves #789 behavior for existing users; comment mode is opt-in via the title-bar toggle. Overrides the issue's "comment default"; decided by the human reviewer 2026-08-10. Changelog note about a behavior flip is now unnecessary. From db1ab3c6599ae39eab1d8b340cb6413120c36691 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Mon, 10 Aug 2026 12:37:07 +1000 Subject: [PATCH 18/27] chore(porch): 1037 dev-approval gate-approved --- .../projects/1037-vscode-codelens-driven-review-/status.yaml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml index e441300f9..65a3de54e 100644 --- a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml +++ b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml @@ -10,12 +10,13 @@ gates: requested_at: '2026-08-05T23:29:29.459Z' approved_at: '2026-08-06T00:15:41.906Z' dev-approval: - status: pending + status: approved requested_at: '2026-08-06T00:35:00.821Z' + approved_at: '2026-08-10T02:37:07.583Z' pr: status: pending iteration: 1 build_complete: false history: [] started_at: '2026-08-05T23:21:45.893Z' -updated_at: '2026-08-06T00:35:00.821Z' +updated_at: '2026-08-10T02:37:07.584Z' From 049ee199b1e5d392754816a5e563fe973d7a35f1 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Mon, 10 Aug 2026 12:38:21 +1000 Subject: [PATCH 19/27] chore(porch): 1037 review phase-transition --- .../projects/1037-vscode-codelens-driven-review-/status.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml index 65a3de54e..90878dbed 100644 --- a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml +++ b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml @@ -1,7 +1,7 @@ id: '1037' title: vscode-codelens-driven-review- protocol: pir -phase: implement +phase: review plan_phases: [] current_plan_phase: null gates: @@ -19,4 +19,4 @@ iteration: 1 build_complete: false history: [] started_at: '2026-08-05T23:21:45.893Z' -updated_at: '2026-08-10T02:37:07.584Z' +updated_at: '2026-08-10T02:38:20.975Z' From 68fde1394050d398c6719dfed4d9db89af9db3c7 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Mon, 10 Aug 2026 12:40:28 +1000 Subject: [PATCH 20/27] [PIR #1037] Review + retrospective --- codev/resources/arch.md | 1 + codev/resources/lessons-learned.md | 3 + .../1037-vscode-codelens-driven-review-.md | 76 +++++++++++++++++++ 3 files changed, 80 insertions(+) create mode 100644 codev/reviews/1037-vscode-codelens-driven-review-.md diff --git a/codev/resources/arch.md b/codev/resources/arch.md index 8d4bfecfa..c3e26597a 100644 --- a/codev/resources/arch.md +++ b/codev/resources/arch.md @@ -1197,6 +1197,7 @@ The VS Code extension (`apps/vscode`) is a thin client over Tower's existing API - **Builders diff-review: navigation + active-file sync (#1060/#1066)**: The "current builder/file" in a diff review is **derived from the active editor, not stored**. The diff-inject registry (`diff-inject-codelens.ts`) maps each right-side worktree fsPath → `{ builderId, relPath, hunks }`; because a worktree-absolute path is unique per builder, two builders that changed the same relative path stay distinct. Everything keys off `getDiffInjectEntry(activeEditor.fsPath)`: cross-file keyboard nav (`codev.diffNextFile`/`diffPreviousFile`, #1060) and the Builders-tree active-file reveal (#1066). Navigation walks **one builder's** changed-file list (it never crosses builders) in the **visible tree order** — depth-first via `flattenTreeOrder(buildFilePathTree(...))` in tree-view mode, raw git `--name-status` order in flat mode — and **wraps** at both ends (`computeNavTarget` modulo) to match VSCode's built-in hunk navigation, which also wraps. The reveal (#1066, Explorer-style, gated by `codev.buildersAutoReveal`) requires two pieces of TreeView groundwork: file rows carry a stable `::` id and `BuildersProvider.getParent` reconstructs the full chain (file → compacted folder(s) → builder → group), since `reveal` matches by id and walks parents while the subtree is still collapsed. It fires on **both** the active-editor change AND the diff-inject registry change, because a programmatic diff open registers its entry *after* the editor activates (same dual-trigger the lens context-key sync uses). - **Agents view + group-by axes (#1104)**: The sidebar's primary work view is **Agents** (view id `codev.agents`, formerly `codev.builders` — only the user-facing id/label changed; internal symbols like `BuildersProvider` and the `codev.buildersGroupBy` setting keep their names). It groups in-flight builders by **exactly one of three axes** (`stage` | `area` | `architect`), each a `BuilderGrouping` strategy in `views/builder-grouping.ts`; `architect` groups by `spawnedByArchitect` (null folds into `main`, matching the affinity router) and a childless architect produces no group, so the work view shows owners-of-work while the *full* roster stays in Workspace > Architects. The axis is a single toolbar button (see the no-`toggled` lesson). The architect roster the Add-Architect flow needs rides on **`/api/overview` as `architects: ArchitectState[]`** (additive wire field), built by a shared `liveArchitects(entry, manager)` helper in `tower-routes.ts` reused by the `/api/state` dashboard-state path so the two payloads can't drift — `liveArchitects` (running-session set, from `terminal_sessions`) is distinct from state.ts's `getArchitects` (persisted `architect` table). `Codev: Add Architect` is conversational: it resolves `main` from that roster and routes a request via `sendMessage('architect:main', …)` rather than creating the architect directly. - **IDE-mode dual activation + layer model (#1144)**: One extension artifact ships through two channels, the marketplace vsix (guest mode) and the same files baked into the Codev IDE fork as a built-in (IDE mode), with the split made at **runtime** via `vscode.env.appName === CODEV_IDE_APP_NAME` (`src/ide-mode.ts`; the constant is a **cross-repo contract** with the fork's product.json `nameLong` and the only place the string is spelled; a user-installed marketplace copy shadowing the built-in is safe because detection is appName-based, not channel-based). `activationEvents` includes `onStartupFinished`, so `activate()` runs in **every window of every install**; which side effects may fire is decided by a pure activation-tier model: `full` (a codev workspace is open), `ide-empty` (IDE with no codev workspace: Tower-level surfaces + empty-window onboarding), `dormant` (guest with no codev workspace: **provably inert**, meaning no Tower auto-start, no preflight, no status bar, no focus steal, no state writes; commands/providers still register so palette invocations degrade gracefully). `activationPolicy(tier)` is the unit-tested switchboard; the five gated side effects live in `extension.ts`. Three context keys drive the UI: `codev.ideMode`; `codev.hasWorkspace` (**codev-workspace-presence**: the opened folder itself contains `codev/`/`.codev/` or the `codev.workspacePath` override points at one; deliberately NOT bare folder-presence, and detection deliberately does **no ancestor walk**, so a codev-enabled home directory cannot leak into every window under it); and `codev.stateKnown` (set first-thing in `activate()`; unset context keys evaluate *false*, so `viewsWelcome`/`when` clauses gate on it to avoid asserting wrong states during the workbench-restore-to-activation gap, and a `!codev.stateKnown` "Loading Codev…" welcome covers the pre-provider window). Workspace-bound views carry `when: codev.hasWorkspace`; the Agents view is the ungated anchor carrying per-quadrant welcome content. The IDE empty-window onboarding (container focus, one-time globalState-gated first-run toast + the #791 walkthrough) is **runtime code by hard constraint**: extension-contributed `configurationDefaults` register asynchronously and race first render (fork-verified, roughly 1-in-3 leak), so nothing first-launch-visible may ride on them. +- **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`). ## Repository Dual Nature diff --git a/codev/resources/lessons-learned.md b/codev/resources/lessons-learned.md index b782adad5..8fc502581 100644 --- a/codev/resources/lessons-learned.md +++ b/codev/resources/lessons-learned.md @@ -338,6 +338,9 @@ Generalizable wisdom extracted from review documents, ordered by impact. Updated - [From 859] VS Code's built-in markdown preview has NO preview→host message channel for contributed `markdown.previewScripts` (`acquireVsCodeApi` is single-call and already consumed; the host's `onDidReceiveMessage` is a closed allowlist; `command:` URIs are blocked; the messaging-API request was closed out-of-scope). To get a hover-action → extension-host interaction on rendered markdown, render it yourself in an owned webview (`CustomTextEditor`); don't try to extend the built-in preview. - [From 859] An editor/title command `icon` given as an SVG *file path* is rendered as an image with no color context, so `fill="currentColor"` resolves to **black** (unlike activity-bar container icons, which VS Code inlines and tints monochrome). Supply `{ "light": ..., "dark": ... }` theme variants for title-bar / menu command icons. - [From 859] Hiding an annotation that lives in the markdown source by *blanking* its line splits a multi-line block (a blank line is a block separator); *removing* the line before parsing (with a cleaned→original line map to preserve source-line attribution) keeps the block intact. Hide source-embedded markers at parse time, not by post-hoc blanking. +- [From 1037] A programmatically created `CommentThread` renders expanded but never focuses its input (stable API has no `CommentThread.reveal`) — the user must click into the textbox. To open a comment input for the user, invoke the built-in `workbench.action.addComment`, which accepts an args object (`{ range }` in 1-based editor coordinates, or `{ fileComment: true }` for a range-less file comment) so no editor-selection mutation (and no selection flash) is needed. Reserve `createCommentThread` for rendering existing comments. +- [From 1037] VS Code caches each document's commenting ranges and re-queries only on its own triggers — if eligibility changes after the editor opened (e.g. a registry populated post-open), the gutter "+" stays missing and `addComment` rejects with "cursor must be within a commenting range". Re-assigning `controller.commentingRangeProvider` (same object) goes through the extension-host setter, which forces the renderer to recompute. +- [From 1037] A comment thread's widget renders after the LAST line of its range, and a range ending at column 0 of that line excludes it from the range highlight (zero characters covered). Anchor threads on the full range they describe, ending at the last line's content end — a start-line-only thread visually cuts through the code it annotates, and a column-0 end paints all lines but the last. - [From 0126] Building the overview endpoint to work in degraded mode (showing builders but empty PR/backlog sections with error messages when `gh` is unavailable) is a good default pattern for features that depend on external services. - [From 0364] Use `onPointerDown` with `preventDefault()` and `tabIndex={-1}` to prevent focus stealing from terminal widgets -- this pattern translates directly to any floating controls rendered alongside interactive widgets. - [From 1144] VS Code evaluates **unset** context keys as false, and views render before the owning extension activates. Any `when`/`viewsWelcome` clause built from negated keys (`!codev.hasWorkspace`) is therefore TRUE during the workbench-restore-to-activation gap and asserts a wrong state (we flashed "Open a folder to use Codev" inside a codev workspace for ~2s). Fix pattern: a `stateKnown` key set first-thing in `activate()`, required by every such clause, plus a `!stateKnown` "Loading…" welcome entry to replace VS Code's raw "no data provider registered" placeholder (welcome content also renders pre-provider-registration). diff --git a/codev/reviews/1037-vscode-codelens-driven-review-.md b/codev/reviews/1037-vscode-codelens-driven-review-.md new file mode 100644 index 000000000..7ecd82bf3 --- /dev/null +++ b/codev/reviews/1037-vscode-codelens-driven-review-.md @@ -0,0 +1,76 @@ +# PIR Review: Codelens-Driven Review Comments in the Builder Diff (per-builder queue + batched submit) + +Fixes #1037 + +## Summary + +The builder diff gains a structured review-comment surface layered on #789's fire-and-forget PTY injection: the reviewer composes comments in inline VSCode comment threads (codelens, gutter "+", or context menu), the comments persist in a per-builder queue at `.builders//.codev/pending-comments.json`, and a single `Submit Review` action (status-bar button + palette command) packages the whole queue into one markdown message typed into the builder PTY's prompt buffer, wrapped in bracketed-paste escapes, with no Enter pressed — the human reviews and sends. The codelens shows exactly one action per anchor, controlled by `codev.diffCodelensMode` (default `forward`, so existing #789 users see zero change; comment mode is a per-workspace opt-in via the diff title-bar toggle). The panel display surface for the queue is explicitly out of scope — it belongs to #1049, which will consume this PR's `ReviewQueueStore` read + event API. + +## Files Changed + +- `apps/vscode/package.json` (+132 / -0): setting, 10 commands, title-bar toggle, context-menu, comments menus +- `apps/vscode/src/review-queue/queue.ts` (+169, new): pure schema / packaging / bracketed paste / exclude block +- `apps/vscode/src/review-queue/store.ts` (+252, new): per-builder fs persistence, watcher sync, info/exclude write +- `apps/vscode/src/review-queue/reconcile.ts` (+75, new): pure thread-reconcile planning +- `apps/vscode/src/review-queue/submit.ts` (+112, new): Submit Review / Discard flows +- `apps/vscode/src/review-queue/status-bar.ts` (+45, new): `Submit Review (N)` counter +- `apps/vscode/src/comments/builder-review.ts` (+365, new): comment controller, input via built-in addComment, mount/reconcile, edit/delete +- `apps/vscode/src/diff-inject-codelens.ts` (+64 / -~10): mode-aware lenses, mode context key, registry entries accessor +- `apps/vscode/src/diff-inject-ref.ts` (+27 / -~10): `LensDescriptor.range` + label parameter +- `apps/vscode/src/extension.ts` (+53 / -~13): wiring, new commands, forward-selection cursor fallback +- 7 test files (+903 total: 6 new + 2 extended) +- `codev/plans/1037-vscode-codelens-driven-review-.md`, `codev/state/pir-1037_thread.md` + +## Commits + +- `35b79381` [PIR #1037] Pure review-queue module: schema, packaging, bracketed paste, exclude block +- `9e15670a` [PIR #1037] ReviewQueueStore: per-builder fs persistence, watcher sync, info/exclude block +- `ca3ee865` [PIR #1037] Mode-aware diff codelenses: diffCodelensMode setting, title-bar toggle, always-on context menu +- `9aafea09` [PIR #1037] Builder-review comment controller: inline threads, reconcile, edit/delete +- `f0383162` [PIR #1037] Submit Review: batched bracketed-paste flush to builder PTY, status-bar counter, wiring +- `67cb80d0` [PIR #1037] Focus comment input on codelens click via built-in addComment command +- `f58047ff` [PIR #1037] Comment input: pass range args to addComment, refresh commenting ranges on registry change +- `24ba4610` [PIR #1037] Mount queued threads on their full range so the widget sits after the last line +- `1f8e3218` [PIR #1037] Extend thread ranges to last-line content end so the range highlight covers every line +- `a094352e` [PIR #1037] Flip diffCodelensMode default to forward (preserve #789 for existing users) + +## Test Results + +- `pnpm build` (porch check): pass +- Extension unit suite (`vitest`): 695 tests / 60 files passing (38 new across 6 new test files + extensions to 2 existing) +- `pnpm check-types` + `eslint`: clean +- Manual verification at dev-approval: the human reviewer exercised the live flow in an extension host against a real builder over multiple iterations (comment capture via lens/gutter/menu, queue persistence, thread re-mount, submit-to-prompt-buffer with bracketed paste, mode toggle). Three UX defects found and fixed during the gate (input focus, stale commenting ranges, thread anchoring/highlight); one report verified not-a-bug against the stored queue JSON. Gate approved 2026-08-10. + +## Deviations From the Approved Plan + +1. **Default mode is `forward`, not `comment`** — human decision at the dev-approval gate (2026-08-10), deliberately overriding the issue's "comment is the default": existing #789 users see zero behavior change; comment mode is opt-in per workspace. This also removed the need for a behavior-change changelog warning. +2. **Comment input opens via the built-in `workbench.action.addComment`** (args-based) instead of programmatically created threads — the stable API cannot focus a created thread's input. Whole-file comments use the native `fileComment` concept (`thread.range === undefined`). +3. **`lineRange` is nullable in the schema** (null = whole-file comment) so the packaged section header reads `### path` instead of a fabricated line ref. +4. **#789's context-menu forward action** now shows without a selection (cursor-line fallback) to satisfy the "context menu always exposes both actions" criterion; the `Cmd/Ctrl+K B` keybinding keeps its original selection guard. +5. **Changelog files not edited on this branch** — `apps/vscode/CHANGELOG.md` and `docs/releases/UNRELEASED.md` are maintained on the `docs/vscode-changelog` branch per that workflow; suggested entry handed to the architect: "Builder diff review comments: compose queued review comments in inline threads (opt-in comment mode via the diff title-bar toggle), batch-submit to the builder's prompt via Submit Review." + +## Architecture Updates + +Routed COLD: added a "Builder review-comment queue (#1037)" entry to `codev/resources/arch.md` under VS Code Extension → Key Design Decisions (storage location + single-owner store + the #789/#1037 never-merge-state invariant + the #1049 seam). Nothing HOT: the feature is extension-scoped, not a cross-cutting system-shape fact that changes implementation choices elsewhere. + +## Lessons Learned Updates + +Routed COLD: three `[From 1037]` entries in `codev/resources/lessons-learned.md` under UI/UX — (1) programmatic `CommentThread` creation cannot focus its input; open inputs via `workbench.action.addComment` with `{range}`/`{fileComment}` args; (2) VS Code caches a document's commenting ranges and only re-queries on its own triggers; re-assign `commentingRangeProvider` to force a recompute when eligibility changes after the editor opened; (3) a thread range ending at column 0 excludes its last line from the range highlight, and the widget renders after the range's last line. Nothing HOT: all three are VS Code comments-API recipes, not cross-cutting rules. + +## Things to Look At During PR Review + +- **`review-queue/store.ts` mutate path**: load-mutate-write folds in concurrent writers from other windows; the watcher echo-suppression compares against last-written bytes. Worth a read for race assumptions. +- **Bracketed-paste injection** (`queue.ts wrapBracketedPaste` + `submit.ts`): `\n` → `\r` conversion inside `\x1b[200~ … \x1b[201~`. Verified live at dev-approval against a real Claude REPL; the wrapped payload travels `sendText` → Tower WebSocket → PTY stdin. +- **`info/exclude` managed block** (`store.ts ensureExclude`): writes to `$GIT_COMMON_DIR/info/exclude` resolved via `git rev-parse`; the `.builder-*` family glob also silences spawn scaffolding files. Failures are deliberately swallowed (cosmetic concern, never blocks a comment write). +- **`comments/builder-review.ts`**: the commenting-ranges refresh trick (provider re-assignment) and the reconciler's mount/dispose lifecycle are the least obvious parts; the pure planner (`reconcile.ts`) carries the tests. +- **#789 surface untouched**: `codev.forwardToBuilder` and the keybinding are unchanged; the only #789-adjacent edits are the context-menu `when` relaxation + cursor fallback (deviation 4) and lens titles/commands swapping by mode. + +## How to Test Locally + +- Pull the branch; `pnpm install`; launch the extension host from `apps/vscode` (F5) or `pnpm vsix` and install. +- Open a builder diff from the Agents view. Default (forward) mode: lenses read `Forward to Builder` and behave exactly as #789. +- Click the title-bar `$(comment)` toggle: lenses flip to `Comment for Builder`, gutter "+" appears. Add comments via lens, gutter, and right-click; watch the status-bar `Submit Review (N)` counter. +- Reload the window, re-open the diff: queued threads re-mount at their ranges. +- `Codev: Submit Review`: packaged message lands in the builder's prompt buffer without submitting; queue and threads clear; press Enter in the terminal to deliver. +- Two-builder isolation: queue comments on two builders' diffs; each `.builders//.codev/pending-comments.json` stays separate; each submit flushes only its builder. +- `git status` inside a builder worktree: neither the queue file nor `.builder-*` scaffolding shows as untracked. From edbdc1d8ae66578bf36d188dee33a029f32578aa Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Mon, 10 Aug 2026 12:40:49 +1000 Subject: [PATCH 21/27] chore(porch): 1037 record PR #1382 --- .../1037-vscode-codelens-driven-review-/status.yaml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml index 90878dbed..bb553b611 100644 --- a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml +++ b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml @@ -19,4 +19,9 @@ iteration: 1 build_complete: false history: [] started_at: '2026-08-05T23:21:45.893Z' -updated_at: '2026-08-10T02:38:20.975Z' +updated_at: '2026-08-10T02:40:49.148Z' +pr_history: + - phase: review + pr_number: 1382 + branch: builder/pir-1037 + created_at: '2026-08-10T02:40:49.147Z' From d7de7e6b3a1e1faf662b154f0e657d15addb2177 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Mon, 10 Aug 2026 12:41:01 +1000 Subject: [PATCH 22/27] chore(porch): 1037 review build-complete --- .../projects/1037-vscode-codelens-driven-review-/status.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml index bb553b611..3ca6f7d19 100644 --- a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml +++ b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml @@ -16,10 +16,10 @@ gates: pr: status: pending iteration: 1 -build_complete: false +build_complete: true history: [] started_at: '2026-08-05T23:21:45.893Z' -updated_at: '2026-08-10T02:40:49.148Z' +updated_at: '2026-08-10T02:41:01.039Z' pr_history: - phase: review pr_number: 1382 From b11dd87092feb12b3a8a6262355558991d41e936 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Mon, 10 Aug 2026 12:45:19 +1000 Subject: [PATCH 23/27] [PIR #1037] Address consultation notes: stale docblocks, document terminal-manager deviation, note reload gap --- apps/vscode/src/__tests__/contributes-review-queue.test.ts | 3 ++- apps/vscode/src/__tests__/diff-codelens-mode.test.ts | 7 ++++--- codev/reviews/1037-vscode-codelens-driven-review-.md | 2 ++ 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/apps/vscode/src/__tests__/contributes-review-queue.test.ts b/apps/vscode/src/__tests__/contributes-review-queue.test.ts index a77d00fa6..dec8f434f 100644 --- a/apps/vscode/src/__tests__/contributes-review-queue.test.ts +++ b/apps/vscode/src/__tests__/contributes-review-queue.test.ts @@ -1,7 +1,8 @@ /** * Manifest invariants for the review-comment queue surface (#1037): * - * - `codev.diffCodelensMode` is declared with default `"comment"`. + * - `codev.diffCodelensMode` is declared with default `"forward"` (existing + * #789 users see no change; comment mode is the per-workspace opt-in). * - The editor context menu offers BOTH the comment and the forward action on * builder-diff files with no mode condition — the non-default flow must * always be one right-click away, whatever the codelens shows. diff --git a/apps/vscode/src/__tests__/diff-codelens-mode.test.ts b/apps/vscode/src/__tests__/diff-codelens-mode.test.ts index df7648e76..fe6a2ef61 100644 --- a/apps/vscode/src/__tests__/diff-codelens-mode.test.ts +++ b/apps/vscode/src/__tests__/diff-codelens-mode.test.ts @@ -1,9 +1,10 @@ /** * Mode-aware diff codelenses (#1037): exactly one lens per anchor, whose * title + command follow `codev.diffCodelensMode` — `Comment for Builder` / - * `codev.commentForBuilder` in comment mode (the default), `Forward to - * Builder` / `codev.forwardToBuilder` in forward mode. A configuration change - * must re-emit lenses (and re-sync the mode context key) so the flip is live. + * `codev.commentForBuilder` in comment mode, `Forward to Builder` / + * `codev.forwardToBuilder` in forward mode (the default, preserving #789). + * A configuration change must re-emit lenses (and re-sync the mode context + * key) so the flip is live. */ import { describe, it, expect, beforeEach, vi } from 'vitest'; diff --git a/codev/reviews/1037-vscode-codelens-driven-review-.md b/codev/reviews/1037-vscode-codelens-driven-review-.md index 7ecd82bf3..be336e4cc 100644 --- a/codev/reviews/1037-vscode-codelens-driven-review-.md +++ b/codev/reviews/1037-vscode-codelens-driven-review-.md @@ -48,6 +48,7 @@ The builder diff gains a structured review-comment surface layered on #789's fir 3. **`lineRange` is nullable in the schema** (null = whole-file comment) so the packaged section header reads `### path` instead of a fabricated line ref. 4. **#789's context-menu forward action** now shows without a selection (cursor-line fallback) to satisfy the "context menu always exposes both actions" criterion; the `Cmd/Ctrl+K B` keybinding keeps its original selection guard. 5. **Changelog files not edited on this branch** — `apps/vscode/CHANGELOG.md` and `docs/releases/UNRELEASED.md` are maintained on the `docs/vscode-changelog` branch per that workflow; suggested entry handed to the architect: "Builder diff review comments: compose queued review comments in inline threads (opt-in comment mode via the diff title-bar toggle), batch-submit to the builder's prompt via Submit Review." +6. **`terminal-manager.ts` unchanged** — the plan listed an `injectBuilderTextMultiline` method; instead `submit.ts` wraps the message with `wrapBracketedPaste` at the call site and reuses the existing `injectBuilderText`, leaving the #789 injection path byte-identical. (Flagged by the PR consultation as an undocumented deviation; documented here.) ## Architecture Updates @@ -64,6 +65,7 @@ Routed COLD: three `[From 1037]` entries in `codev/resources/lessons-learned.md` - **`info/exclude` managed block** (`store.ts ensureExclude`): writes to `$GIT_COMMON_DIR/info/exclude` resolved via `git rev-parse`; the `.builder-*` family glob also silences spawn scaffolding files. Failures are deliberately swallowed (cosmetic concern, never blocks a comment write). - **`comments/builder-review.ts`**: the commenting-ranges refresh trick (provider re-assignment) and the reconciler's mount/dispose lifecycle are the least obvious parts; the pure planner (`reconcile.ts`) carries the tests. - **#789 surface untouched**: `codev.forwardToBuilder` and the keybinding are unchanged; the only #789-adjacent edits are the context-menu `when` relaxation + cursor fallback (deviation 4) and lens titles/commands swapping by mode. +- **Known minor gap (consultation finding, follow-up)**: after a window reload, the palette `Codev: Submit Review` doesn't see on-disk queues until a builder diff has been opened (the store's cache populates from the diff-inject registry and its own writes; nothing globs `.builders/*/.codev/pending-comments.json` at activation). No data loss — the diff-centric flow always works, and opening the diff is the natural path to reviewing before submitting. A one-time activation glob-and-load would close it; deliberately left as a follow-up rather than expanding this PR. ## How to Test Locally From 945e3b498be81c22877ed995424fa96c03f18f47 Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Mon, 10 Aug 2026 12:55:46 +1000 Subject: [PATCH 24/27] [PIR #1037] Fix consultation findings: mode-independent commenting ranges, queue preload at activation, echo-suppression + clamp tests --- .../__tests__/builder-review-ranges.test.ts | 131 ++++++++++++++++++ .../__tests__/review-queue-reconcile.test.ts | 24 ++++ .../src/__tests__/review-queue-store.test.ts | 76 +++++++++- apps/vscode/src/comments/builder-review.ts | 36 ++--- apps/vscode/src/extension.ts | 3 + apps/vscode/src/review-queue/reconcile.ts | 23 ++- apps/vscode/src/review-queue/store.ts | 30 +++- .../1037-vscode-codelens-driven-review-.md | 6 +- 8 files changed, 300 insertions(+), 29 deletions(-) create mode 100644 apps/vscode/src/__tests__/builder-review-ranges.test.ts diff --git a/apps/vscode/src/__tests__/builder-review-ranges.test.ts b/apps/vscode/src/__tests__/builder-review-ranges.test.ts new file mode 100644 index 000000000..db52d48f3 --- /dev/null +++ b/apps/vscode/src/__tests__/builder-review-ranges.test.ts @@ -0,0 +1,131 @@ +/** + * Regression (#1037, raised by the PR consultation): the builder-review + * commenting ranges must NOT depend on `codev.diffCodelensMode`. The + * `workbench.action.addComment` command — which backs the comment codelens, + * the gutter "+", AND the always-visible context-menu action — validates + * against these ranges, so a comment-mode-only provider silently breaks + * `Codev: Comment for Builder` from the context menu whenever the editor is + * in forward mode, which is the DEFAULT. Ranges must be provided for + * registered builder-diff files in every mode, with file comments enabled. + */ + +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: 'forward' as string, + controller: undefined as unknown, + }; + return { EventEmitter, state }; +}); + +vi.mock('vscode', () => ({ + EventEmitter: h.EventEmitter, + Range: class { + constructor( + public startLine: number, + public startChar: number, + public endLine: number, + public endChar: number, + ) {} + }, + Selection: class {}, + 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: (id: string, label: string) => { + const controller = { + id, + label, + options: undefined as unknown, + commentingRangeProvider: undefined as unknown, + createCommentThread: vi.fn(), + dispose: vi.fn(), + }; + h.state.controller = controller; + return controller; + }, + }, + commands: { + registerCommand: () => ({ dispose() {} }), + executeCommand: () => Promise.resolve(undefined), + }, + window: { + activeTextEditor: undefined, + visibleTextEditors: [], + onDidChangeActiveTextEditor: () => ({ dispose() {} }), + showWarningMessage: vi.fn(), + }, + workspace: { + textDocuments: [], + getConfiguration: () => ({ get: () => h.state.mode }), + onDidChangeConfiguration: () => ({ dispose() {} }), + }, + languages: { registerCodeLensProvider: () => ({ dispose() {} }) }, +})); + +const { activateBuilderReviewComments } = 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: [] }; +const DOC = { uri: { fsPath: ENTRY.fsPath }, lineCount: 40 }; + +const storeStub = { + onDidChangeQueue: () => ({ dispose() {} }), + getWorktreePath: () => '/wt', + registerWorktree: () => {}, + load: async () => [], + getComments: () => [], +} as never; + +const overviewStub = { getData: () => null } as never; + +interface Controller { + commentingRangeProvider: { + provideCommentingRanges(doc: unknown): { enableFileComments: boolean; ranges: unknown[] } | unknown[]; + }; +} + +beforeEach(() => { + setDiffInjectSession([]); + activateBuilderReviewComments({ subscriptions: [] } as never, storeStub, overviewStub); + setDiffInjectSession([ENTRY]); +}); + +describe('builder-review commenting ranges are mode-independent', () => { + it.each(['forward', 'comment', 'garbage'])( + 'provides ranges + file comments for a registered file in %s mode', + mode => { + h.state.mode = mode; + const provider = (h.state.controller as Controller).commentingRangeProvider; + const result = provider.provideCommentingRanges(DOC) as { enableFileComments: boolean; ranges: unknown[] }; + expect(result.enableFileComments).toBe(true); + expect(result.ranges.length).toBeGreaterThan(0); + }, + ); + + it('provides nothing for an unregistered file', () => { + const provider = (h.state.controller as Controller).commentingRangeProvider; + const result = provider.provideCommentingRanges({ uri: { fsPath: '/elsewhere.ts' }, lineCount: 5 }); + expect(result).toEqual([]); + }); +}); diff --git a/apps/vscode/src/__tests__/review-queue-reconcile.test.ts b/apps/vscode/src/__tests__/review-queue-reconcile.test.ts index b4ea48921..2bba3a0ab 100644 --- a/apps/vscode/src/__tests__/review-queue-reconcile.test.ts +++ b/apps/vscode/src/__tests__/review-queue-reconcile.test.ts @@ -10,6 +10,7 @@ import { describe, it, expect } from 'vitest'; import { planThreadReconcile, deriveWorktreePath, + clampAnchorLines, type RegisteredFile, } from '../review-queue/reconcile.js'; import type { PendingComment } from '../review-queue/queue.js'; @@ -68,6 +69,29 @@ describe('planThreadReconcile', () => { }); }); +describe('clampAnchorLines (stale anchors from a moving worktree)', () => { + it('maps a 1-based in-bounds range to 0-based lines', () => { + expect(clampAnchorLines({ start: 42, end: 58 }, 100)).toEqual({ startLine: 41, endLine: 57 }); + }); + + it('clamps a range whose end drifted past the end of file', () => { + expect(clampAnchorLines({ start: 42, end: 58 }, 50)).toEqual({ startLine: 41, endLine: 49 }); + }); + + it('clamps a fully out-of-bounds range onto the last line', () => { + expect(clampAnchorLines({ start: 90, end: 95 }, 10)).toEqual({ startLine: 9, endLine: 9 }); + }); + + it('applies no upper clamp when the document is not open', () => { + expect(clampAnchorLines({ start: 42, end: 58 }, undefined)).toEqual({ startLine: 41, endLine: 57 }); + }); + + it('never inverts: end is floored to start, start is floored to 0', () => { + expect(clampAnchorLines({ start: 10, end: 3 }, 100)).toEqual({ startLine: 9, endLine: 9 }); + expect(clampAnchorLines({ start: 0, end: 0 }, 100)).toEqual({ startLine: 0, endLine: 0 }); + }); +}); + describe('deriveWorktreePath', () => { it('strips the relPath suffix from the entry fsPath', () => { expect(deriveWorktreePath('/repo/.builders/pir-9/src/x.ts', 'src/x.ts', '/')) diff --git a/apps/vscode/src/__tests__/review-queue-store.test.ts b/apps/vscode/src/__tests__/review-queue-store.test.ts index b302fcb8f..be0ec81f1 100644 --- a/apps/vscode/src/__tests__/review-queue-store.test.ts +++ b/apps/vscode/src/__tests__/review-queue-store.test.ts @@ -13,6 +13,11 @@ import * as os from 'node:os'; import * as path from 'node:path'; import { execFileSync } from 'node:child_process'; +const w = vi.hoisted(() => ({ + // Captured FileSystemWatcher handlers so tests can simulate OS events. + handlers: [] as Array<(uri: { fsPath: string }) => void>, +})); + vi.mock('vscode', () => { class EventEmitter { private handlers: Array<(e: T) => void> = []; @@ -28,14 +33,18 @@ vi.mock('vscode', () => { class RelativePattern { constructor(public base: string, public pattern: string) {} } + const capture = (fn: (uri: { fsPath: string }) => void): { dispose(): void } => { + w.handlers.push(fn); + return { dispose() {} }; + }; return { EventEmitter, RelativePattern, workspace: { createFileSystemWatcher: vi.fn(() => ({ - onDidCreate: vi.fn(() => ({ dispose() {} })), - onDidChange: vi.fn(() => ({ dispose() {} })), - onDidDelete: vi.fn(() => ({ dispose() {} })), + onDidCreate: capture, + onDidChange: capture, + onDidDelete: capture, dispose: vi.fn(), })), }, @@ -67,6 +76,7 @@ async function makeWorktree(name: string): Promise { beforeEach(async () => { tmpRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'review-queue-')); + w.handlers.length = 0; }); afterEach(async () => { @@ -182,6 +192,66 @@ describe('ReviewQueueStore', () => { expect(exclude.split('\n').filter(l => l.includes('managed block'))).toHaveLength(1); }); + it('preloadFromDisk surfaces persisted queues before any diff is opened (reload gap)', async () => { + // Two builders under `.builders/` wrote queues in a previous session. + for (const id of ['pir-a', 'pir-b']) { + const dir = path.join(tmpRoot, '.builders', id, '.codev'); + await fs.mkdir(dir, { recursive: true }); + await fs.writeFile( + path.join(tmpRoot, '.builders', id, QUEUE_FILE_RELPATH), + JSON.stringify({ version: 1, builderId: id, comments: [makeComment(`${id}-c1`)] }), + 'utf8', + ); + } + // One worktree without a queue file must not register. + await fs.mkdir(path.join(tmpRoot, '.builders', 'pir-empty'), { recursive: true }); + + const store = new ReviewQueueStore(tmpRoot); + await store.preloadFromDisk(); + expect(store.buildersWithPending().sort()).toEqual(['pir-a', 'pir-b']); + expect(store.getComments('pir-a')[0]!.id).toBe('pir-a-c1'); + store.dispose(); + }); + + it('preloadFromDisk is a no-op without a .builders directory', async () => { + const store = new ReviewQueueStore(tmpRoot); + await store.preloadFromDisk(); + expect(store.buildersWithPending()).toEqual([]); + store.dispose(); + }); + + it('suppresses watcher echoes of its own writes but honors external changes', async () => { + // Real timers: the 200ms debounce chains into real fs I/O, so the test + // waits past the window instead of faking the clock. + const pastDebounce = (): Promise => new Promise(r => setTimeout(r, 300)); + const wt = await makeWorktree('pir-1'); + const store = new ReviewQueueStore(tmpRoot); + store.registerWorktree('pir-1', wt); + const events: string[] = []; + store.onDidChangeQueue(id => events.push(id)); + + await store.add('pir-1', makeComment('c1')); + expect(events).toEqual(['pir-1']); + + // The OS watcher reports our own write back — must not re-fire. + const queuePath = path.join(wt, QUEUE_FILE_RELPATH); + for (const fire of w.handlers) { fire({ fsPath: queuePath }); } + await pastDebounce(); + expect(events).toEqual(['pir-1']); + + // A genuinely external write (another window) must fire. + await fs.writeFile( + queuePath, + JSON.stringify({ version: 1, builderId: 'pir-1', comments: [makeComment('c1'), makeComment('c2')] }), + 'utf8', + ); + for (const fire of w.handlers) { fire({ fsPath: queuePath }); } + await pastDebounce(); + expect(events).toEqual(['pir-1', 'pir-1']); + expect(store.getComments('pir-1').map(c => c.id)).toEqual(['c1', 'c2']); + store.dispose(); + }); + it('fires onDidChangeQueue on mutations with the builder id', async () => { const wt = await makeWorktree('pir-1'); const store = new ReviewQueueStore(undefined); diff --git a/apps/vscode/src/comments/builder-review.ts b/apps/vscode/src/comments/builder-review.ts index 5d9b96042..dd32bb504 100644 --- a/apps/vscode/src/comments/builder-review.ts +++ b/apps/vscode/src/comments/builder-review.ts @@ -21,14 +21,12 @@ import * as vscode from 'vscode'; import * as path from 'node:path'; import { randomUUID } from 'node:crypto'; import { - getDiffCodelensMode, getDiffInjectEntry, getDiffInjectEntries, onDidChangeDiffInjectRegistry, COMMENT_FOR_BUILDER_COMMAND, - DIFF_CODELENS_MODE_KEY, } from '../diff-inject-codelens.js'; -import { planThreadReconcile, deriveWorktreePath, type RegisteredFile } from '../review-queue/reconcile.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'; import type { OverviewCache } from '../views/overview-data.js'; @@ -85,13 +83,16 @@ export function activateBuilderReviewComments( }; context.subscriptions.push(controller); - // Gutter "+" on registered builder-diff files, comment mode only — in - // forward mode the comment surface recedes so the two modes stay distinct. - // `enableFileComments` lets `workbench.action.addComment` create a - // range-less file comment (the file-level lens flow). + // Gutter "+" on registered builder-diff files, in BOTH codelens modes. The + // ranges must not depend on the mode: `workbench.action.addComment` (which + // backs the codelens, gutter, AND the always-visible context-menu action) + // validates against these ranges, so a comment-mode-only provider breaks + // `Codev: Comment for Builder` from the context menu whenever the editor is + // in forward mode — the default. The codelens stays the mode-distinct + // surface. `enableFileComments` lets the command create a range-less file + // comment (the file-level lens flow). const rangeProvider: vscode.CommentingRangeProvider = { provideCommentingRanges(document) { - if (getDiffCodelensMode() !== 'comment') { return []; } if (!getDiffInjectEntry(document.uri.fsPath)) { return []; } const lastLine = Math.max(0, document.lineCount - 1); return { enableFileComments: true, ranges: [new vscode.Range(0, 0, lastLine, 0)] }; @@ -139,17 +140,11 @@ export function activateBuilderReviewComments( * range highlight would skip it (paint 129 but not 130 of a 129-130 span). */ function anchorRange(fsPath: string, range: LineRange): vscode.Range { - let start = Math.max(range.start - 1, 0); - let end = Math.max(range.end - 1, start); - let endColumn = LAST_COLUMN; const doc = vscode.workspace.textDocuments.find(d => d.uri.fsPath === fsPath); - if (doc) { - const lastLine = Math.max(doc.lineCount - 1, 0); - start = Math.min(start, lastLine); - end = Math.min(end, lastLine); - endColumn = doc.lineAt(end).text.length; - } - return new vscode.Range(start, 0, end, endColumn); + const { startLine, endLine } = clampAnchorLines(range, doc?.lineCount); + let endColumn = LAST_COLUMN; + if (doc) { endColumn = doc.lineAt(endLine).text.length; } + return new vscode.Range(startLine, 0, endLine, endColumn); } function mountQueuedComment(fsPath: string, builderId: string, comment: PendingComment): void { @@ -351,11 +346,6 @@ export function activateBuilderReviewComments( reconcile(); }), store.onDidChangeQueue(() => { reconcile(); }), - // Mode flips change what provideCommentingRanges returns; force the - // recompute so the gutter "+" appears/recedes with the toggle. - vscode.workspace.onDidChangeConfiguration(e => { - if (e.affectsConfiguration(DIFF_CODELENS_MODE_KEY)) { refreshCommentingRanges(); } - }), new vscode.Disposable(() => { for (const thread of mounted.values()) { thread.dispose(); } mounted.clear(); diff --git a/apps/vscode/src/extension.ts b/apps/vscode/src/extension.ts index 5b82ebaaf..2159b3ee4 100644 --- a/apps/vscode/src/extension.ts +++ b/apps/vscode/src/extension.ts @@ -258,6 +258,9 @@ export async function activate(context: vscode.ExtensionContext) { vscode.workspace.workspaceFolders?.[0]?.uri.fsPath, ); context.subscriptions.push(reviewQueueStore); + // Load persisted queues so the palette Submit Review and the status-bar + // counter see them right after a reload, before any diff is opened. + reviewQueueStore.preloadFromDisk(); // Drive the `codev.terminalFocused` context key so the Cmd/Ctrl+V image // paste binding (#736) only applies when a Codev terminal is focused — diff --git a/apps/vscode/src/review-queue/reconcile.ts b/apps/vscode/src/review-queue/reconcile.ts index ae1cfff3a..f3693975e 100644 --- a/apps/vscode/src/review-queue/reconcile.ts +++ b/apps/vscode/src/review-queue/reconcile.ts @@ -11,7 +11,7 @@ * session are disposed. */ -import type { PendingComment } from './queue.js'; +import type { LineRange, PendingComment } from './queue.js'; /** The subset of a diff-inject session entry the planner needs. */ export interface RegisteredFile { @@ -62,6 +62,27 @@ export function planThreadReconcile( return { toCreate, toDispose }; } +/** + * Clamp a queued comment's 1-based inclusive anchor range to 0-based line + * indices within a document of `lineCount` lines (undefined = document not + * open, no upper clamp). Queued anchors go stale as the builder keeps + * committing — a range past the current end of file must still mount, on the + * last line, rather than throw or vanish. + */ +export function clampAnchorLines( + range: LineRange, + lineCount: number | undefined, +): { startLine: number; endLine: number } { + let startLine = Math.max(range.start - 1, 0); + let endLine = Math.max(range.end - 1, startLine); + if (lineCount !== undefined) { + const lastLine = Math.max(lineCount - 1, 0); + startLine = Math.min(startLine, lastLine); + endLine = Math.min(endLine, lastLine); + } + return { startLine, endLine }; +} + /** * Recover a worktree root from a diff-inject entry: `fsPath` is always * `join(worktreePath, relPath)` (see `view-diff.ts`), so stripping the diff --git a/apps/vscode/src/review-queue/store.ts b/apps/vscode/src/review-queue/store.ts index 4f71368e5..74a1c1b94 100644 --- a/apps/vscode/src/review-queue/store.ts +++ b/apps/vscode/src/review-queue/store.ts @@ -57,7 +57,7 @@ export class ReviewQueueStore implements vscode.Disposable { private readonly debounceTimers = new Map>(); private readonly disposables: vscode.Disposable[] = []; - constructor(workspaceRoot: string | undefined) { + constructor(private readonly workspaceRoot: string | undefined) { if (workspaceRoot) { const watcher = vscode.workspace.createFileSystemWatcher( new vscode.RelativePattern(workspaceRoot, `.builders/*/${QUEUE_FILE_RELPATH}`), @@ -72,6 +72,34 @@ export class ReviewQueueStore implements vscode.Disposable { } } + /** + * One-time scan of `.builders//.codev/pending-comments.json` under the + * workspace root: register each worktree (dir basename = builder id, the + * same fallback the watcher path uses) and load its queue into the cache. + * Called fire-and-forget at activation so the palette Submit Review and the + * status-bar counter see persisted queues after a window reload, before any + * diff has been opened. Best-effort: a missing `.builders/` dir or an + * unreadable file reads as empty, never throws. + */ + async preloadFromDisk(): Promise { + if (!this.workspaceRoot) { return; } + let names: string[] = []; + try { + names = await fs.readdir(path.join(this.workspaceRoot, '.builders')); + } catch { + return; // No worktrees — nothing to preload. + } + await Promise.all(names.map(async name => { + const queueFile = path.join(this.workspaceRoot!, '.builders', name, QUEUE_FILE_RELPATH); + try { + await fs.access(queueFile); + } catch { + return; // This builder never queued anything. + } + await this.load(this.builderIdForQueuePath(queueFile)); + })); + } + /** Remember a builder's worktree root (idempotent; callers pass authoritative paths). */ registerWorktree(builderId: string, worktreePath: string): void { this.worktreeById.set(builderId, worktreePath); diff --git a/codev/reviews/1037-vscode-codelens-driven-review-.md b/codev/reviews/1037-vscode-codelens-driven-review-.md index be336e4cc..ec7d3644a 100644 --- a/codev/reviews/1037-vscode-codelens-driven-review-.md +++ b/codev/reviews/1037-vscode-codelens-driven-review-.md @@ -49,6 +49,7 @@ The builder diff gains a structured review-comment surface layered on #789's fir 4. **#789's context-menu forward action** now shows without a selection (cursor-line fallback) to satisfy the "context menu always exposes both actions" criterion; the `Cmd/Ctrl+K B` keybinding keeps its original selection guard. 5. **Changelog files not edited on this branch** — `apps/vscode/CHANGELOG.md` and `docs/releases/UNRELEASED.md` are maintained on the `docs/vscode-changelog` branch per that workflow; suggested entry handed to the architect: "Builder diff review comments: compose queued review comments in inline threads (opt-in comment mode via the diff title-bar toggle), batch-submit to the builder's prompt via Submit Review." 6. **`terminal-manager.ts` unchanged** — the plan listed an `injectBuilderTextMultiline` method; instead `submit.ts` wraps the message with `wrapBracketedPaste` at the call site and reuses the existing `injectBuilderText`, leaving the #789 injection path byte-identical. (Flagged by the PR consultation as an undocumented deviation; documented here.) +7. **Gutter "+" is mode-independent** (plan said comment-mode-only) — forced by a real defect the PR consultation caught: `workbench.action.addComment` validates against the provider's commenting ranges, so a comment-mode-only provider broke the always-visible context-menu `Comment for Builder` in forward mode, the shipped default. Ranges are now provided for registered builder-diff files in every mode (regression-pinned in `builder-review-ranges.test.ts`); the codelens remains the mode-distinct surface. ## Architecture Updates @@ -65,7 +66,10 @@ Routed COLD: three `[From 1037]` entries in `codev/resources/lessons-learned.md` - **`info/exclude` managed block** (`store.ts ensureExclude`): writes to `$GIT_COMMON_DIR/info/exclude` resolved via `git rev-parse`; the `.builder-*` family glob also silences spawn scaffolding files. Failures are deliberately swallowed (cosmetic concern, never blocks a comment write). - **`comments/builder-review.ts`**: the commenting-ranges refresh trick (provider re-assignment) and the reconciler's mount/dispose lifecycle are the least obvious parts; the pure planner (`reconcile.ts`) carries the tests. - **#789 surface untouched**: `codev.forwardToBuilder` and the keybinding are unchanged; the only #789-adjacent edits are the context-menu `when` relaxation + cursor fallback (deviation 4) and lens titles/commands swapping by mode. -- **Known minor gap (consultation finding, follow-up)**: after a window reload, the palette `Codev: Submit Review` doesn't see on-disk queues until a builder diff has been opened (the store's cache populates from the diff-inject registry and its own writes; nothing globs `.builders/*/.codev/pending-comments.json` at activation). No data loss — the diff-centric flow always works, and opening the diff is the natural path to reviewing before submitting. A one-time activation glob-and-load would close it; deliberately left as a follow-up rather than expanding this PR. +- **PR consultation findings and dispositions** (single advisory pass; claude=APPROVE, codex=REQUEST_CHANGES — all three codex findings assessed as valid and fixed, since PIR does not auto-re-review): + 1. *Context-menu comment action broken in forward mode (real defect)* — commenting ranges were comment-mode-only while `workbench.action.addComment` validates against them; with forward as the shipped default, `Codev: Comment for Builder` from the context menu could not create a thread. Fixed by making the ranges mode-independent (deviation 7); regression test `builder-review-ranges.test.ts` pins ranges + `enableFileComments` in every mode. + 2. *Palette blind to persisted queues after reload* (also flagged by claude) — fixed: `ReviewQueueStore.preloadFromDisk()` scans `.builders/*/.codev/pending-comments.json` at activation, so the palette command and status-bar counter see queues before any diff opens. Covered by two new store tests. + 3. *Plan-promised tests missing* — added: watcher echo-suppression (own write suppressed, external write honored, via captured watcher handlers) and out-of-bounds anchor clamping (extracted as pure `clampAnchorLines` in `reconcile.ts`, 5 cases). ## How to Test Locally From 967a6f94023f7c2bf5bda620ee9d916719c4113c Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Mon, 10 Aug 2026 12:56:35 +1000 Subject: [PATCH 25/27] chore(porch): 1037 pr gate-requested --- .../projects/1037-vscode-codelens-driven-review-/status.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml index 3ca6f7d19..6f50226a1 100644 --- a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml +++ b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml @@ -15,13 +15,15 @@ gates: approved_at: '2026-08-10T02:37:07.583Z' pr: status: pending + requested_at: '2026-08-10T02:56:35.653Z' iteration: 1 build_complete: true history: [] started_at: '2026-08-05T23:21:45.893Z' -updated_at: '2026-08-10T02:41:01.039Z' +updated_at: '2026-08-10T02:56:35.654Z' pr_history: - phase: review pr_number: 1382 branch: builder/pir-1037 created_at: '2026-08-10T02:40:49.147Z' +pr_ready_for_human: true From e4725db9ec0b707c23d6f61ed053b47042e3cd3a Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Mon, 10 Aug 2026 12:58:09 +1000 Subject: [PATCH 26/27] chore(porch): 1037 pr gate-approved --- .../1037-vscode-codelens-driven-review-/status.yaml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml index 6f50226a1..feb9be63d 100644 --- a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml +++ b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml @@ -14,16 +14,17 @@ gates: requested_at: '2026-08-06T00:35:00.821Z' approved_at: '2026-08-10T02:37:07.583Z' pr: - status: pending + status: approved requested_at: '2026-08-10T02:56:35.653Z' + approved_at: '2026-08-10T02:58:09.013Z' iteration: 1 build_complete: true history: [] started_at: '2026-08-05T23:21:45.893Z' -updated_at: '2026-08-10T02:56:35.654Z' +updated_at: '2026-08-10T02:58:09.016Z' pr_history: - phase: review pr_number: 1382 branch: builder/pir-1037 created_at: '2026-08-10T02:40:49.147Z' -pr_ready_for_human: true +pr_ready_for_human: false From f17544e6b799ecbccdd64711c8abbe7c5fa385ff Mon Sep 17 00:00:00 2001 From: Amr Elsayed Date: Mon, 10 Aug 2026 12:59:21 +1000 Subject: [PATCH 27/27] chore(porch): 1037 protocol complete --- .../projects/1037-vscode-codelens-driven-review-/status.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml index feb9be63d..92b77061b 100644 --- a/codev/projects/1037-vscode-codelens-driven-review-/status.yaml +++ b/codev/projects/1037-vscode-codelens-driven-review-/status.yaml @@ -1,7 +1,7 @@ id: '1037' title: vscode-codelens-driven-review- protocol: pir -phase: review +phase: verified plan_phases: [] current_plan_phase: null gates: @@ -21,7 +21,7 @@ iteration: 1 build_complete: true history: [] started_at: '2026-08-05T23:21:45.893Z' -updated_at: '2026-08-10T02:58:09.016Z' +updated_at: '2026-08-10T02:59:21.759Z' pr_history: - phase: review pr_number: 1382