diff --git a/codev/plans/789-vscode-inject-file-hunk-refere.md b/codev/plans/789-vscode-inject-file-hunk-refere.md new file mode 100644 index 000000000..143b6cdb8 --- /dev/null +++ b/codev/plans/789-vscode-inject-file-hunk-refere.md @@ -0,0 +1,100 @@ +# PIR Plan: Inject file/hunk reference into builder PTY from the unified-diff editor (codelens) + +## Understanding + +Reviewing a builder's changes is a per-file retype loop: open `codev.viewDiff`, read a file, switch to the builder terminal, manually retype the file path (and line numbers), add feedback, submit. The path-retyping is the bottleneck. + +The fix mirrors the proven `codev.referenceIssueInArchitect` pattern (`extension.ts:785`): an inline action injects a reference token into a terminal's prompt buffer **without** pressing Enter, then the human types their feedback and submits. That command targets the *architect* terminal with `# `; we want the same mechanic targeting the *builder* terminal with a file/hunk reference, surfaced as CodeLenses inside the diff editor `codev.viewDiff` opens. + +Key facts established from the code: + +- `codev.viewDiff` (`commands/view-diff.ts:301`) opens VSCode's multi-file diff editor via `vscode.changes`. For each changed file the **right** (modified) side is a plain `file:` URI at `/` (added/modified/renamed); deleted/binary right sides are `codev-diff:` placeholders. The right side carries the new-side line numbers we need. +- `getBuilderChanges(wt)` (`view-diff.ts:261`) already resolves `defaultBranch`, the merge-base `baseRef`, the `ChangeEntry[]`, and the binary-path set — everything except the per-file hunk line ranges. +- `terminal-manager.ts` has `injectArchitectText(text)` (`:146`): look up `architect:` terminal, `show()`, `sendText(text, false)` (no newline). There is no builder equivalent yet. `openBuilderByRoleOrId(roleOrId, focus)` (`:192`) resolves a builder and opens/reveals its terminal, but returns `void` and resolves `roleOrId` → a *canonical* `builder.id` that may differ from the input (e.g. `153` → `builder-spir-153`); the terminal is keyed `builder-`. +- `buildArchitectReferenceInjection` (`architect-reference-injection.ts`) is the pure, unit-tested string builder precedent — palette-hidden command, no `vscode` import, tested in `__tests__/architect-reference-injection.test.ts`. +- Palette suppression: a command is kept out of the Command Palette either by a `commandPalette` `when:"false"` entry (e.g. `referenceIssueInArchitect`, `openBuilderFileDiff`) **or** by not declaring it in `contributes.commands` at all. CodeLens-backing commands are invoked programmatically and need no package.json declaration; no test enforces registered-vs-declared parity. + +## Proposed Change + +Add a `CodeLensProvider` that renders inject actions on the **right-side `file:` documents** shown in the multi-file diff editor, plus the builder-terminal injection plumbing. + +**1. Pure helpers (new file `packages/vscode/src/diff-inject-ref.ts`, no `vscode` import — mirrors `architect-reference-injection.ts`):** +- `parseHunkRanges(patch: string): Array<{ newStart: number; newEnd: number }>` — parse `@@ -a,b +c,d @@` headers from a single file's unified diff. New-side start = `c`; length = `d` (absent → 1). `newEnd = newStart + max(length, 1) - 1`. A pure-deletion hunk (`+c,0`) clamps to a single anchor line at `c` (or `c+1`), so a click still references a sane location. +- `buildBuilderFileRef(relPath): string` → `` `${relPath} ` `` (trailing space, no Enter). +- `buildBuilderHunkRef(relPath, start, end): string` → `` `${relPath}:L${start}-L${end} ` ``. + +**2. CodeLens provider (new file `packages/vscode/src/diff-inject-codelens.ts`):** +- `DiffInjectCodeLensProvider implements vscode.CodeLensProvider`, holding a registry `Map` and an `onDidChangeCodeLenses` emitter. +- `provideCodeLenses(document)`: look up `document.uri.fsPath`. If present, emit one **file-level** lens (range at line 0) titled `Send to builder PTY` and one lens **per hunk** (range at `newStart-1`) titled `Send to builder PTY (lines -)`. Each lens's `command` is `codev.injectBuilderFileRef` with args `[builderId, refText]`. +- `setSession(entries)` replaces the registry (one active diff session per `viewDiff` invocation) and fires the change event; provider registered once at activation for `{ scheme: 'file' }`. + +**3. `viewDiff` populates the registry:** after `getBuilderChanges`, run one `git -C diff -M --unified=3 ` (full multi-file patch), split it per file on `diff --git`, map each file's hunks to its new path, and call `provider.setSession(...)` keyed by the right-side fs path (`path.join(wt, resourcePath)`) before opening the editor. Deleted/binary files are skipped (no right-side file document). This is one extra git call alongside the two `getBuilderChanges` already runs. + +**4. `terminal-manager.ts`:** +- Add `injectBuilderText(builderId, text): boolean` — look up `builder-`, `show()`, `sendText(text, false)`; return false if absent (exact mirror of `injectArchitectText`). +- Change `openBuilderByRoleOrId` to **return** the resolved canonical `builder.id` (`Promise`); existing callers ignore the return (non-breaking). This closes the id-mismatch gap so the inject targets the same terminal key that was opened. + +**5. `extension.ts`:** register `codev.injectBuilderFileRef` via the existing `reg(...)` helper, **without** declaring it in `contributes.commands` (palette-hidden by omission). Handler: +```ts +reg('codev.injectBuilderFileRef', async (builderId: string, text: string) => { + const resolvedId = await terminalManager?.openBuilderByRoleOrId(builderId, true); // open + focus, fallback flow + const ok = resolvedId ? terminalManager?.injectBuilderText(resolvedId, text) : false; + if (!ok) { vscode.window.showWarningMessage('Codev: Builder terminal not available'); } +}); +``` +`openBuilderByRoleOrId` already contains the "no active terminal" recovery flow (`promptNoTerminalRecovery`), satisfying the fallback acceptance criterion. Wire provider registration through a new `activateDiffInjectCodeLens(context)` called alongside `activateDiffView(context)`. + +## Files to Change + +- `packages/vscode/src/diff-inject-ref.ts` — **new**: pure helpers `parseHunkRanges`, `buildBuilderFileRef`, `buildBuilderHunkRef`. +- `packages/vscode/src/diff-inject-codelens.ts` — **new**: `DiffInjectCodeLensProvider` + `activateDiffInjectCodeLens(context)` returning the provider instance (so `viewDiff` can populate it). +- `packages/vscode/src/commands/view-diff.ts:261` / `:301` — fetch + parse the per-file patch in `getBuilderChanges` (or a sibling helper) and call `provider.setSession(...)` in `viewDiff` before `vscode.changes`. +- `packages/vscode/src/terminal-manager.ts:146` (add `injectBuilderText`), `:192` (return resolved id from `openBuilderByRoleOrId`). +- `packages/vscode/src/extension.ts:806` area — register `codev.injectBuilderFileRef`; call `activateDiffInjectCodeLens` near `activateDiffView` (`:884`); hold the provider so `viewDiff` reaches it (pass via the command registration / a module singleton, matching `view-diff`'s existing module-level `provider`). +- `packages/vscode/src/__tests__/diff-inject-ref.test.ts` — **new**: unit tests for hunk parsing + ref builders. + +## Risks & Alternatives Considered + +- **Primary risk — CodeLenses may not render inside `vscode.changes` (the multi-file diff editor).** CodeLens providers are document-scoped, and the multi-diff editor embeds standard diff editors; lenses normally render on the modified side, but multi-diff embedding has historically had gaps. **This is exactly what the `dev-approval` gate validates** — the reviewer runs the worktree and confirms lenses appear. Fallback if they don't render in the multi-diff editor: surface the same lenses in the per-file `vscode.diff` editor (`codev.openBuilderFileDiff`), which definitively renders CodeLenses — same provider, same registry, narrower entry point. I'll flag the outcome explicitly at the gate. +- **Over-trigger:** because the provider matches `{ scheme: 'file' }`, lenses also appear if the reviewer opens that exact worktree file *normally* (outside the diff) after running `viewDiff`. Accepted as benign — the injected reference is still correct and useful. The registry is replaced per `viewDiff` run, so stale sessions don't accumulate. (Optional tightening: scope the selector to a worktree glob so `provideCodeLenses` isn't even invoked for unrelated `file:` documents — functionally inert either way since non-registry paths return `[]`.) +- **Coexistence with existing editor chrome — no collision, purely additive.** The extension registers *no* `CodeLensProvider` today, and the `viewDiff` editor shows zero codelenses by default (left side is the no-language `codev-diff:` scheme; right side is a `file:` source doc, and TS reference/implementation lenses are off by default and unset in this repo). Ours are the first lenses in that editor. They render as *viewzone* lines anchored above a code line, so they (a) do **not** remove, reorder, or override the multi-diff editor's built-in chrome — the per-file header (filename, collapse/expand, open-file) and the diff gutter are untouched; (b) do **not** shift line numbers or desync hunk mapping (viewzones aren't real document lines; the editor inserts a matching filler on the opposite side to keep left/right aligned); and (c) coexist with any future language-service lenses — VSCode joins multiple providers' lenses on the same anchor with ` | ` separators rather than suppressing either. The only real cost is added vertical space (one lens line per file + per hunk), beyond the primary multi-diff-rendering risk above. The inline plan-review Comments API (`activateReviewComments`) is scoped to `codev/plans|specs|reviews/*.md` documents, not the diffed source files, so it's unaffected. +- **Id mismatch (builderId in registry vs terminal key):** resolved by returning the canonical id from `openBuilderByRoleOrId` and injecting against *that*, not the raw arg. +- **Deleted files:** no right-side `file:` document, so no lenses — you can't reference new lines of a deleted file. Acceptable and out of the issue's intent. +- **Alternative rejected — render lenses on the left `codev-diff:` side** (fully scoped to the diff editor, never over-triggers): rejected because the left side holds *old* content with old-side line numbers and is empty for added files, so it can't carry new-side hunk ranges. +- **Alternative rejected — replace the side-by-side editor with a single unified-diff text document** (file/hunk headers literally in text): rejected as a UX regression — reviewers rely on the side-by-side multi-file editor, and the issue asks to add lenses *to the existing `viewDiff` editor*, not replace it. + +## Revision (dev-approval gate): pivot from hunk lenses to symbol lenses + selection forward + +Testing at the `dev-approval` gate showed the approved hunk-driven approach is **not usable**: a newly-created file is a single whole-file git hunk, so it gets exactly one lens regardless of size — you can't forward a specific function/interface in a new file. Hunk granularity is hostage to where the *edit* is, not where you want to *comment*. (Also confirmed earlier: CodeLens doesn't render in the multi-file `vscode.changes` editor, so lenses live on the per-file `vscode.diff` / normal tab with `diffEditor.codeLens` enabled.) + +Revised approach (architect-directed at the gate): + +1. **Symbol-level lenses** — drive lenses off VSCode's Document Symbol provider (`vscode.executeDocumentSymbolProvider`) instead of git hunks, so granularity follows the *code*, not the diff. Level-2 depth + a kind allowlist: + - **Depth 0 (top-level):** Function, Class, Interface, Enum, Struct, Namespace, Module; plus **multi-line** top-level Variable/Constant (catches arrow-function components/handlers reported as Variable). + - **Depth 1 (into Class/Struct only):** Method, Constructor. + - Excluded: Property, Field, EnumMember, TypeParameter, Package, value kinds, one-line/non-top-level Variable/Constant. + - A symbol lens that anchors on line 0 is skipped (the file-level lens covers it). +2. **File-level lens** retained (whole file). +3. **Hunk lenses coexist with symbol lenses** (option B, after a round of review): the symbol lenses (bare "Forward to Builder" on declarations) are layered with per-hunk lenses ("Forward to Builder (lines N-M)" on each changed region) via `buildAllLensDescriptors`. A hunk lens is skipped when its anchor collides with a symbol/file lens, so nothing stacks. (`parseHunkRanges`/`parseUnifiedDiff` retained and fed into the registry entry's `hunks`.) +4. **Right-click "Forward Selection to Builder"** — an `editor/context` command on a non-empty selection injects `:L-L ` for the exact selected range. Context menus are *not* suppressed in the multi-file View Diff editor, so this is the granular path that works in the scan view. Scoped via a `codev.activeEditorIsBuilderFile` context key (set when the active editor is a tracked builder-diff file) + the built-in `editorHasSelection`. + +Surfaces: symbol/file lenses → per-file diff + normal tab (CodeLens limitation). Selection context-menu → works in the multi-file View Diff editor too (to be validated at the gate). + +Deferred (future issue): hover-triggered forward action; deeper symbol nesting; per-kind density settings. + +## Test Plan + +**Unit (`diff-inject-ref.test.ts`, vitest — `pnpm --filter codev-vscode test:unit`):** +- `parseHunkRanges` on a multi-hunk patch → correct new-side `{newStart,newEnd}` per hunk; single-line hunk (`@@ -10 +11 @@`) → `{11,11}`; pure-deletion hunk (`+c,0`) clamps to one line; multiple files isolated correctly after the per-file split. +- `buildBuilderFileRef('a/b.ts')` → `'a/b.ts '`; `buildBuilderHunkRef('a/b.ts', 10, 20)` → `'a/b.ts:L10-L20 '`. + +**Manual (at the `dev-approval` gate — the reviewer runs the worktree):** +1. `pnpm build` the vscode package; launch the Extension Development Host (or sideload). +2. With a builder that has changes, run **Codev: View Diff**. +3. Confirm a `Send to builder PTY` lens appears above each file and a `Send to builder PTY (lines N-M)` lens above each hunk. **(Validates the primary risk.)** +4. Click a file lens → builder terminal is revealed/focused and ` ` is typed into the prompt with **no Enter**. +5. Click a hunk lens → `:L-L ` typed, no Enter; line range matches the hunk's new-side range. +6. Confirm `codev.injectBuilderFileRef` does **not** appear in the Command Palette. +7. Close the builder terminal, click a lens → the no-terminal recovery flow opens the terminal, then injects. + +**Build/CI:** `pnpm --filter codev-vscode check-types`, `pnpm --filter codev-vscode lint`, and `pnpm --filter codev-vscode test:unit` must pass. diff --git a/codev/projects/789-vscode-inject-file-hunk-refere/status.yaml b/codev/projects/789-vscode-inject-file-hunk-refere/status.yaml new file mode 100644 index 000000000..1a9e75ad6 --- /dev/null +++ b/codev/projects/789-vscode-inject-file-hunk-refere/status.yaml @@ -0,0 +1,30 @@ +id: '789' +title: vscode-inject-file-hunk-refere +protocol: pir +phase: review +plan_phases: [] +current_plan_phase: null +gates: + plan-approval: + status: approved + requested_at: '2026-06-09T22:26:05.749Z' + approved_at: '2026-06-09T22:58:51.188Z' + dev-approval: + status: approved + requested_at: '2026-06-09T23:04:34.034Z' + approved_at: '2026-06-10T07:46:32.265Z' + pr: + status: approved + requested_at: '2026-06-10T08:35:47.462Z' + approved_at: '2026-06-10T08:43:33.506Z' +iteration: 1 +build_complete: true +history: [] +started_at: '2026-06-09T22:20:14.795Z' +updated_at: '2026-06-10T08:43:33.508Z' +pr_history: + - phase: review + pr_number: 1023 + branch: builder/pir-789 + created_at: '2026-06-10T08:25:20.462Z' +pr_ready_for_human: false diff --git a/codev/resources/lessons-learned.md b/codev/resources/lessons-learned.md index fd8975066..8145cea56 100644 --- a/codev/resources/lessons-learned.md +++ b/codev/resources/lessons-learned.md @@ -316,6 +316,9 @@ Generalizable wisdom extracted from review documents, ordered by impact. Updated - [From #983] Gate a remedy prompt on the condition the remedy actually fixes, not on any divergence. The Tower "Restart Tower" toast first fired on `running < max(installedCLI, extensionVersion)`, but a restart only reloads the *installed* binary — so when the extension was newer than an up-to-date CLI, it prompted a restart that reloaded the same version (a no-op) and named a version that wasn't installed. Narrowing to `running < installedCLI` made every prompt actionable and every word true. When two conditions can trigger one action, fire only on the sub-condition the action resolves; route the others to their real owners (here: "CLI behind extension" is the separate `codev --version` preflight's job). - [From #983] To detect that a long-running daemon is serving stale code after an in-place upgrade, probe the **running process's** in-memory version over its own API — the on-disk binary version (`--version`) only reports what's installed, not what's executing. The two diverge for the whole window between `npm install -g` and the next restart. Expose it from request-context state (not a fresh disk read) so the value is provably the running process's. - [From #983] When a decision needs two independently-fetched async inputs (here: installed-CLI version from a ~400ms spawn, running-Tower version from a connect-triggered probe), nothing orders them — whichever loses the race leaves the decision running on partial data (a null input silently read as "fine"). Re-run the decision when the *later* input resolves, so the comparison always runs with both values present. A one-shot "fire on event X" is a latent bug when X can beat the data it depends on. +- [From #789] CodeLens placement in diff editors is doubly constrained, and it dictated a whole feature's design. (1) VSCode's **multi-file diff editor** (`vscode.changes` — what `codev.viewDiff` opens) renders **no** CodeLenses at all; (2) the **single-file** diff editor hides them unless `diffEditor.codeLens` is enabled (off by default — microsoft/vscode#97640). So a "lens on the diff" feature can only live in the per-file `vscode.diff` / normal editor tabs, and must prompt to enable the setting (at **Global** scope — it's a personal editor preference; writing Workspace would edit the repo's shared `.vscode/settings.json` and force it on collaborators). **Context menus are NOT suppressed in the multi-file editor**, so a right-click/keybinding action is the only affordance that works in the scan view. Validate the rendering surface with a running-worktree spike *before* building on it — unit tests assert lens *shape*, never whether the host editor displays them (sibling to [From 799]). +- [From #789] For annotating a diff, drive granularity off the **code (document symbols), not git hunks**. A brand-new file is a single whole-file hunk, so hunk-driven lenses give it exactly one lens regardless of size; symbol-driven lenses (via `vscode.executeDocumentSymbolProvider`) give every function/class/interface its own affordance, so new and modified files are equally addressable. Also: emit one lens per **contiguous changed run** (split each hunk on context lines; deletions don't break a run), not per git hunk — git coalesces adjacent edits into one hunk, which would collapse several distinct changes onto one lens. +- [From #789] Don't add a "performance" cache before confirming the hot path. A per-version document-symbol cache was added to fix a ~100% CPU spike, but the spike was an unrelated newly-installed extension (the **Extension Test Runner**) walking the `.builders/` worktree farm (`rg --no-ignore --follow` over pnpm symlinks) — the extension-host CPU profile was ~99% idle. `provideCodeLenses` is not a hot path (VSCode caches lenses and only re-queries on invalidation), so the cache bought nothing and risked pinning an empty result when the language server was still warming up on first open. Profile first; cache only a proven-hot, proven-pure path. ## Documentation diff --git a/codev/reviews/789-vscode-inject-file-hunk-refere.md b/codev/reviews/789-vscode-inject-file-hunk-refere.md new file mode 100644 index 000000000..02801b69a --- /dev/null +++ b/codev/reviews/789-vscode-inject-file-hunk-refere.md @@ -0,0 +1,78 @@ +# PIR Review: Forward file/symbol/hunk/selection references into the builder PTY (CodeLens) + +Fixes #789 + +## Summary + +Adds a one-click way to forward a code reference from a builder's diff into that builder's terminal prompt — no path retyping. In a builder file diff (or a normal editor tab on a builder worktree file) a `Forward to Builder` CodeLens appears at the file top, above each declaration (function/class/interface/enum/struct/namespace/method/constructor + multi-line top-level const), and above each changed hunk; clicking injects `` or `:L-L` into the builder's prompt **without** pressing Enter, mirroring the existing architect-reference pattern. A right-click **"Codev: Forward Selection to Builder"** (and `Cmd/Ctrl+K B`) forwards any selected range, and works inside the multi-file View Diff editor too. + +The issue's original mechanism (CodeLens on hunks in the `codev.viewDiff` editor) proved unworkable and the design pivoted during the `dev-approval` gate — see Lessons Learned. The implementation that shipped is symbol-driven (granularity follows the code, so new files are as forwardable as modified ones), with per-hunk lenses layered on, plus the selection action as the path that survives in the multi-file editor. + +## Files Changed + +- `packages/vscode/src/diff-inject-ref.ts` (+247 / new) — pure helpers: symbol selection (`buildSymbolLensDescriptors`), changed-run parsing (`parseHunkRanges`/`parseUnifiedDiff`), `buildAllLensDescriptors` (symbol + hunk lenses, deduped by anchor line), ref builders. +- `packages/vscode/src/diff-inject-codelens.ts` (+156 / new) — `CodeLensProvider` over a per-diff registry; resolves document symbols; backs the `codev.activeEditorIsBuilderFile` context key. +- `packages/vscode/src/ensure-diff-codelens.ts` (+38 / new) — one-click prompt to enable `diffEditor.codeLens` (Global scope) when off. +- `packages/vscode/src/commands/view-diff.ts` (+79 / -…) — populate the registry on `viewDiff` and the per-file diff; open the editor before computing hunks. +- `packages/vscode/src/terminal-manager.ts` (+32) — `injectBuilderText`; `openBuilderByRoleOrId` returns the resolved canonical id. +- `packages/vscode/src/extension.ts` (+62) — register `codev.forwardToBuilder` (palette-hidden) and `codev.forwardSelectionToBuilder`; activate the provider. +- `packages/vscode/package.json` (+21) — selection command + `editor/context` menu + `Cmd/Ctrl+K B` keybinding (all scoped + palette-hidden). +- `packages/vscode/src/__tests__/diff-inject-ref.test.ts` (+181 / new) — unit tests for the pure helpers. + +## Commits + +Full list via `git log main..HEAD --oneline` (26 feature commits). Highlights: + +- `7ee8068a` Add diff-inject ref helpers + CodeLens provider +- `4c6af3ee` terminal-manager: injectBuilderText + resolved-id return +- `fecb9658` Option A: lenses on per-file diff + diffEditor.codeLens prompt +- `9984f8c4` Drive lenses off document symbols, not git hunks +- `75c03eb5` Right-click "Forward Selection to Builder" editor/context action +- `aea085f4` Restore per-hunk lenses alongside symbol lenses (option B) +- `a9bbb478` One change-lens per contiguous changed run, not per git hunk +- `29f04083` Open diffs before computing hunks (fix open-delay regression) +- `a93d6ac5` Remove symbol cache (misdiagnosed; risked pinning empty symbols) +- `aa96ba8f` Enable diffEditor.codeLens at Global scope + +## Test Results + +- `pnpm --filter codev-vscode check-types`: ✓ pass +- `pnpm --filter codev-vscode lint`: ✓ pass +- `pnpm --filter codev-vscode test:unit`: ✓ pass (375 tests, ~20 new for symbol selection / changed-run parsing / lens building) +- `node esbuild.js` bundle: ✓ +- Manual (human, at `dev-approval` in the Extension Dev Host): file/symbol/hunk lenses render in per-file diff and normal tabs with `diffEditor.codeLens` on; clicking injects the ref into the builder terminal without Enter; right-click "Forward Selection to Builder" + `Cmd+K B` forward a selection (incl. in the multi-file View Diff editor); new-file lenses carry line ranges; the enable-prompt writes the setting. + +## Architecture Updates + +No `arch.md` changes needed — this is a self-contained VSCode UI feature (a CodeLens provider + two commands + terminal injection) that doesn't alter module boundaries or introduce a cross-cutting pattern. The durable platform constraints it surfaced are captured in `lessons-learned.md` instead (see below). + +## Lessons Learned Updates + +Added one entry to `codev/resources/lessons-learned.md` (UI/UX): CodeLens does **not** render in VSCode's multi-file `vscode.changes` editor, and is hidden by default in single diff editors (`diffEditor.codeLens`) — the constraint that forced this feature's whole design pivot. The full design-process lesson (symbol- vs hunk-driven granularity, and surfacing the multi-file-editor limit early) is recorded in this review. + +## Things to Look At During PR Review + +- **`buildAllLensDescriptors` dedup** (`diff-inject-ref.ts`): a hunk lens is suppressed when its anchor line already has a symbol/file lens, so declaration-line changes show the structural lens and body changes show the hunk lens — no stacking. Worth a careful read. +- **`parseHunkRanges`** now emits one range per *contiguous changed run* (broken by context lines; deletions don't break a run), not per git hunk — this is what makes adjacent edits get individual lenses. +- **Surface limits**: symbol/hunk CodeLenses live in per-file diff + normal tabs (CodeLens is suppressed in the multi-file `vscode.changes` editor); the selection context-menu is the path that works in the multi-file editor. This is a VSCode constraint, not a bug. +- **No 3-way consult iteration** (PIR single-pass): if the consult flags anything, it's addressed/rebutted here, not re-reviewed. + +### Consultation outcome (PR-creation, single advisory pass) + +- **Claude: APPROVE** (HIGH) — no issues. +- **Codex: REQUEST_CHANGES** (HIGH) — **real defect, fixed.** The `codev.activeEditorIsBuilderFile` context key (gating the right-click "Forward Selection to Builder" + `Cmd/Ctrl+K B`) was synced only on `onDidChangeActiveTextEditor`. Because `openBuilderFileDiff` opens the diff (making it active) *before* registering its file, the key was left `false` on the just-opened diff until focus changed, so the selection entry point was dead. **Fix** (`diff-inject-codelens.ts`): re-sync the key on every registry change by subscribing to the provider's `onDidChangeCodeLenses` (fired by `setSession`/`upsert`). **Regression test**: `src/__tests__/diff-inject-context-key.test.ts` asserts the key flips `true` after the active file is registered (and back to `false` when a new session drops it) — it fails without the re-sync. Since PIR is single-pass, this fix was **not** independently re-reviewed; flagging for human verification at the `pr` gate. +- **Gemini: skipped** — the agy/Gemini path returned "the current workspace is empty" on both attempts (it didn't ingest the diff). Best-effort per the consult design; no verdict. + +## How to Test Locally + +This is a VSCode **extension** change, so `afx dev` won't exercise it — load the extension: + +- **Extension Dev Host**: open `packages/vscode` (or the repo) and press **F5** ("Run Codev Extension"). +- Ensure `diffEditor.codeLens` is on (the feature prompts to enable it on first file-diff open). +- Builders tree → click a changed file → confirm `Forward to Builder` lenses (file, symbols, hunks); click one → the builder terminal focuses and the ref is typed with no Enter. +- Select lines → right-click **Codev: Forward Selection to Builder** (or `Cmd/Ctrl+K B`); confirm it works in the multi-file **View Diff** editor too. +- Verify `codev.forwardToBuilder` / `codev.forwardSelectionToBuilder` are absent from the Command Palette. + +## Related + +- #1022 — workspace `files.watcherExclude`/`search.exclude` for `.builders/` + `node_modules`, and dropping the Extension Test Runner recommendation (a CPU storm discovered while reviewing this feature; not part of this change). diff --git a/codev/state/pir-789_thread.md b/codev/state/pir-789_thread.md new file mode 100644 index 000000000..c5210ae36 --- /dev/null +++ b/codev/state/pir-789_thread.md @@ -0,0 +1,73 @@ +# PIR #789 — inject file/hunk ref into builder PTY from the unified-diff editor (codelens) + +## Plan phase (iteration 1) + +Investigated the vscode extension. Key findings: +- `codev.viewDiff` opens VSCode's multi-file diff editor (`vscode.changes`); right side of each file is a plain `file:` URI at `/` — carries new-side line numbers. +- `injectArchitectText` (`terminal-manager.ts:146`) is the inject-without-Enter precedent; no builder equivalent exists yet. +- `openBuilderByRoleOrId` opens/reveals a builder terminal but returns void and resolves roleOrId → a *canonical* id that can differ from the input → terminal key mismatch risk. Plan returns the resolved id to fix this. +- Palette suppression: just don't declare the codelens-backing command in `contributes.commands`. + +### Design chosen +CodeLensProvider on the right-side `file:` documents. New pure helpers (`diff-inject-ref.ts`) for hunk parsing + ref strings; provider (`diff-inject-codelens.ts`) reads a per-`viewDiff`-run registry; `viewDiff` parses one extra `git diff --unified` to populate hunk ranges; `terminal-manager` gets `injectBuilderText` + id-returning `openBuilderByRoleOrId`; `extension.ts` registers palette-hidden `codev.injectBuilderFileRef`. + +### Primary risk (to validate at dev-approval gate) +CodeLenses may not render inside the `vscode.changes` multi-diff editor (document-scoped lenses + multi-diff embedding has historical gaps). Fallback if so: same provider/registry on the per-file `vscode.diff` (`openBuilderFileDiff`). This is exactly what PIR's dev-approval gate (run the worktree) is for. + +Plan written to `codev/plans/789-vscode-inject-file-hunk-refere.md`, committed. Awaiting `plan-approval`. + +## Implement phase (iteration 1) + +plan-approval approved. Implemented per plan, 3 commits: +- `diff-inject-ref.ts` (pure helpers: parseHunkRanges/parseUnifiedDiff/buildBuilderFileRef/buildBuilderHunkRef/buildLensDescriptors) + `diff-inject-codelens.ts` (provider, `{scheme:'file'}`, per-viewDiff-run registry) + `diff-inject-ref.test.ts` (12 tests). +- `terminal-manager.ts`: added `injectBuilderText`; `openBuilderByRoleOrId` now returns the resolved canonical id (non-breaking). +- `view-diff.ts`: one extra `git diff -M --unified=3` parsed into the registry before `vscode.changes` (patch failure non-fatal). `extension.ts`: palette-hidden `codev.injectBuilderFileRef` + `activateDiffInjectCodeLens`. + +Validation: `pnpm test:unit` 372 pass (had to build core+types dist first — the 7 import-resolution failures were pre-existing unbuilt-dep, not my diff), `check-types` clean, `lint` clean, esbuild bundles. + +**Primary risk still unvalidated**: whether CodeLenses render inside the `vscode.changes` multi-diff editor. Needs the human to run the worktree at the dev-approval gate. Fallback documented in plan if they don't render. + +Awaiting `dev-approval`. + +## Dev-approval gate — primary risk materialized, pivoted to Option A + +Human tested in the Extension Dev Host. Lenses did NOT appear in the `codev.viewDiff` multi-file editor, even with `diffEditor.codeLens` enabled. Confirmed via VSCode issues #97640 / #156707: CodeLens is hidden in diff editors by default; the setting re-enables it for the **single-file** diff editor, but the **multi-file `vscode.changes` editor doesn't render CodeLens at all**. So the issue's literal surface (lenses in the viewDiff multi-file editor) is infeasible. + +**Provider proven correct**: opening a changed file in a plain editor tab shows the lens on line 1. So registration + fsPath matching + rendering all work — only the multi-diff host is the blocker. + +**Pivot (human chose Option A)**: surface the lenses on the **single-file `vscode.diff`** opened from the Builders tree (`codev.openBuilderFileDiff`), which honors `diffEditor.codeLens`. Changes: +- `openBuilderFileDiff` now populates the registry for its file (new `registerFileInjectSession` in view-diff.ts, `upsert` on the provider) so lenses appear without a prior View Diff run. +- New `ensure-diff-codelens.ts`: when opening a file diff with `diffEditor.codeLens` off, one-click prompt to enable it (with "Don't ask again" in globalState). Avoids shipping a feature that silently needs a hidden setting. +- viewDiff still populates the registry (harmless; future-proofs if multi-diff ever supports lenses; also powers the normal-tab case). + +Validation: check-types/lint/test:unit (372) all green; esbuild bundles. **Needs human to re-test Option A**: F5 relaunch → click a changed file row in the Builders tree → single-file diff → lenses render (prompt offers to enable the setting first). + +Note for review file: the spec'd surface was infeasible; should reflect the per-file-diff entry point back to the issue. + +## Major pivot (dev-approval): symbol lenses + selection forward, hunk lenses removed + +Hunk-driven lenses proved unusable: a new file is one whole-file hunk → one lens regardless of size, no way to forward a specific function/interface. Architect directed a redesign (captured in the plan's "Revision" section): +- **Lenses now driven by document symbols** (`vscode.executeDocumentSymbolProvider`), not git hunks. Level-2 policy: file-level lens + top-level Function/Class/Interface/Enum/Struct/Namespace/Module + multi-line top-level Variable/Constant; descend one level into Class/Struct for Method/Constructor. Line-0 collision with the file lens skipped. +- **Hunk parsing removed** (`parseHunkRanges`/`parseUnifiedDiff`/`HunkRange` gone); registry entry simplified to `{fsPath, builderId, relPath}`; viewDiff/registerFileInjectSession no longer touch git for lenses. +- **Right-click "Forward Selection to Builder"** (`editor/context`) added for arbitrary ranges — context menus DO work in the multi-file View Diff editor (unlike CodeLens), so this is the in-scan-view path. Scoped via `codev.activeEditorIsBuilderFile` context key + `editorHasSelection`. + +Label still "Forward to Builder" (lenses) / "Forward Selection to Builder" (menu). Command ids: `codev.forwardToBuilder` (lens, palette-hidden) + `codev.forwardSelectionToBuilder` (menu, palette-hidden, declared for the context menu). + +Validation: 368 unit tests, check-types, lint, bundle all green. **Needs human re-test (F5)**: symbol lenses on per-file diff/normal tab; right-click selection in View Diff editor. Surfaces caveat unchanged: CodeLens not in multi-file editor; selection menu is. + +Deferred to future issue(s): hover-triggered forward; deeper nesting; per-kind density settings. + +## Review phase + +dev-approval approved. During final testing found a ~100% CPU spike — traced (via extension-host CPU profile = 99% idle, then live `ps` of the rg processes) to a newly-installed **Extension Test Runner** extension walking the `.builders/` worktree farm, NOT this feature. Filed **#1022** (workspace excludes + drop the recommendation). Also: removed the misdiagnosed symbol cache; fixed the diffEditor.codeLens enable-toast to write Global scope (personal pref, not Workspace); identified a stray uncommitted workspace `false` (from diagnostics) shadowing it — user reverts. + +Review written (`codev/reviews/789-...md`), lessons-learned updated (3 entries: diff-editor CodeLens constraints, symbol- vs hunk-driven granularity, profile-before-caching). Opening PR. + +## Follow-ups during dev-approval +- Renamed lens label → "Forward to Builder"; command ids `codev.forwardToBuilder` / `codev.forwardSelectionToBuilder`. +- Added `Cmd/Ctrl+K B` keybinding for Forward Selection (when matched to the menu so the hint renders); menu title prefixed "Codev:". +- **Option B**: restored per-hunk lenses to coexist with symbol lenses (`buildAllLensDescriptors`, dedup by anchor line so hunk lenses don't stack on symbol/file lenses). Hunk parsing (`parseHunkRanges`/`parseUnifiedDiff`) re-added and fed into the registry entry. 373 tests green. + +## Dev-approval iteration: hunk-lens placement fix + +Human observed hunk lenses landing on the wrong function's signature. Root cause: `--unified=3` reports each hunk's new-side start up to 3 *context* lines above the first real change, so near a function boundary the anchor lands on the preceding `def`/`return`. Fixed: `parseHunkRanges` now walks the hunk body to find the first/last actually-added (`+`) new-side lines (`changeStart`/`changeEnd`); `buildLensDescriptors` anchors AND labels on those instead of the header span. Deviation from issue's literal "hunk header range" — intentional, more accurate. 374 tests green. Needs human re-test after F5 relaunch. diff --git a/packages/vscode/package.json b/packages/vscode/package.json index bc933aff2..e547da4e6 100644 --- a/packages/vscode/package.json +++ b/packages/vscode/package.json @@ -50,6 +50,10 @@ "command": "codev.helloWorld", "title": "Codev: Show Connection State" }, + { + "command": "codev.forwardSelectionToBuilder", + "title": "Codev: Forward Selection to Builder" + }, { "command": "codev.openArchitectTerminal", "title": "Codev: Open Architect Terminal" @@ -295,6 +299,10 @@ ], "menus": { "commandPalette": [ + { + "command": "codev.forwardSelectionToBuilder", + "when": "false" + }, { "command": "codev.openBuilderById", "when": "false" @@ -388,6 +396,13 @@ "when": "false" } ], + "editor/context": [ + { + "command": "codev.forwardSelectionToBuilder", + "when": "codev.activeEditorIsBuilderFile && editorHasSelection", + "group": "codev@1" + } + ], "view/item/context": [ { "command": "codev.recheckCli", @@ -634,6 +649,12 @@ "key": "ctrl+k g", "mac": "cmd+k g" }, + { + "command": "codev.forwardSelectionToBuilder", + "key": "ctrl+k b", + "mac": "cmd+k b", + "when": "codev.activeEditorIsBuilderFile && editorHasSelection" + }, { "command": "workbench.view.extension.codev", "key": "ctrl+alt+c", diff --git a/packages/vscode/src/__tests__/diff-inject-context-key.test.ts b/packages/vscode/src/__tests__/diff-inject-context-key.test.ts new file mode 100644 index 000000000..b46b2a259 --- /dev/null +++ b/packages/vscode/src/__tests__/diff-inject-context-key.test.ts @@ -0,0 +1,99 @@ +/** + * Regression (#789, raised by the PR consultation): the + * `codev.activeEditorIsBuilderFile` context key — which gates the right-click + * "Forward Selection to Builder" menu and the Cmd/Ctrl+K B keybinding — must + * re-sync when the diff registry changes, not only on editor-focus changes. + * + * `openBuilderFileDiff` opens the diff (making it the active editor) BEFORE + * registering its file, so the active-editor event fires while the registry is + * still empty. Without a registry-change re-sync the key stays `false` on the + * just-opened diff until focus changes, leaving the selection action dead. + */ + +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 = { + activeEditor: undefined as unknown, + setContextCalls: [] as Array<{ key: string; value: unknown }>, + }; + return { EventEmitter, state }; +}); + +vi.mock('vscode', () => ({ + EventEmitter: h.EventEmitter, + Range: class {}, + CodeLens: class {}, + languages: { registerCodeLensProvider: () => ({ 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(); + }, + }, + window: { + get activeTextEditor() { return h.state.activeEditor; }, + onDidChangeActiveTextEditor: () => ({ dispose() {} }), + }, +})); + +const { + activateDiffInjectCodeLens, + setDiffInjectSession, + upsertDiffInjectEntry, + BUILDER_FILE_CONTEXT_KEY, +} = await import('../diff-inject-codelens.js'); + +function editorFor(fsPath: string): unknown { + return { document: { uri: { fsPath, toString: () => `file://${fsPath}` } } }; +} +function lastContext(): { key: string; value: unknown } | undefined { + return h.state.setContextCalls[h.state.setContextCalls.length - 1]; +} + +const ENTRY = { fsPath: '/wt/pkg/src/a.ts', builderId: 'b1', relPath: 'pkg/src/a.ts', hunks: [] }; + +describe('#789 — activeEditorIsBuilderFile re-syncs on registry change', () => { + beforeEach(() => { + h.state.activeEditor = undefined; + setDiffInjectSession([]); // clear the singleton registry + h.state.setContextCalls.length = 0; + }); + + it('sets the key true after the active file is registered (not only on focus change)', () => { + h.state.activeEditor = editorFor(ENTRY.fsPath); + + activateDiffInjectCodeLens({ subscriptions: [] } as never); + // Diff is active but not yet registered → key false. + expect(lastContext()).toEqual({ key: BUILDER_FILE_CONTEXT_KEY, value: false }); + + // openBuilderFileDiff order: open the diff, THEN register its file. + upsertDiffInjectEntry(ENTRY); + + // The registry-change re-sync must flip the key true (the bug left it false). + expect(lastContext()).toEqual({ key: BUILDER_FILE_CONTEXT_KEY, value: true }); + }); + + it('clears the key when a new session no longer tracks the active file', () => { + h.state.activeEditor = editorFor(ENTRY.fsPath); + activateDiffInjectCodeLens({ subscriptions: [] } as never); + upsertDiffInjectEntry(ENTRY); + expect(lastContext()?.value).toBe(true); + + setDiffInjectSession([]); // e.g. a fresh viewDiff run without this file + expect(lastContext()?.value).toBe(false); + }); +}); diff --git a/packages/vscode/src/__tests__/diff-inject-ref.test.ts b/packages/vscode/src/__tests__/diff-inject-ref.test.ts new file mode 100644 index 000000000..00a7ca967 --- /dev/null +++ b/packages/vscode/src/__tests__/diff-inject-ref.test.ts @@ -0,0 +1,181 @@ +/** + * Unit tests for the pure symbol/ref helpers behind the "Forward to Builder" + * CodeLens actions (#789). No `vscode` dependency, so the live implementation + * is imported directly (same pattern as `architect-reference-injection.test.ts`). + */ + +import { describe, it, expect } from 'vitest'; +import { + buildBuilderFileRef, + buildBuilderRangeRef, + buildSymbolLensDescriptors, + buildAllLensDescriptors, + parseHunkRanges, + type SymbolNode, +} from '../diff-inject-ref.js'; + +// Numeric vscode.SymbolKind values used by the tests. +const K = { + Class: 4, + Method: 5, + Property: 6, + Constructor: 8, + Enum: 9, + Interface: 10, + Function: 11, + Variable: 12, + Constant: 13, +} as const; + +function sym(kind: number, startLine: number, endLine: number, children: SymbolNode[] = []): SymbolNode { + return { kind, startLine, endLine, children }; +} + +describe('ref builders', () => { + it('builds a file ref with a trailing space and no newline', () => { + expect(buildBuilderFileRef('packages/vscode/src/extension.ts')) + .toBe('packages/vscode/src/extension.ts '); + }); + + it('builds a range ref with the L-L range', () => { + expect(buildBuilderRangeRef('a/b.ts', 10, 20)).toBe('a/b.ts:L10-L20 '); + }); +}); + +describe('buildSymbolLensDescriptors', () => { + it('always emits a file-level lens at line 0', () => { + expect(buildSymbolLensDescriptors('a/b.ts', [])).toEqual([ + { line: 0, title: 'Forward to Builder', refText: 'a/b.ts ' }, + ]); + }); + + it('lenses top-level structural declarations with their full range', () => { + const symbols = [ + sym(K.Function, 4, 9), // function → line 4, L5-L10 + sym(K.Interface, 12, 18), // interface → line 12, L13-L19 + sym(K.Enum, 20, 24), // enum → line 20, L21-L25 + ]; + 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 ' }, + ]); + }); + + it('descends one level into a class for methods and the constructor', () => { + const cls = sym(K.Class, 3, 40, [ + sym(K.Constructor, 5, 8), + sym(K.Method, 10, 20), + sym(K.Property, 22, 22), // excluded + ]); + 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 + ]); + }); + + it('lenses a top-level multi-line Variable/Constant but skips one-line ones', () => { + const symbols = [ + sym(K.Variable, 4, 12), // multi-line const (e.g. arrow component) → lensed + sym(K.Constant, 14, 14), // one-line scalar → skipped + ]; + 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 ' }, + ]); + }); + + it('skips a symbol that anchors on line 0 (collides with the file-level lens)', () => { + // A file whose first declaration starts at line 0. + const symbols = [sym(K.Function, 0, 30)]; + expect(buildSymbolLensDescriptors('a/b.ts', symbols)).toEqual([ + { line: 0, title: 'Forward to Builder', refText: 'a/b.ts ' }, + ]); + }); + + it('does not lens excluded top-level kinds (Property) or recurse past one level', () => { + const cls = sym(K.Class, 2, 50, [ + sym(K.Class, 10, 40, [ // nested class: not lensed, not recursed + sym(K.Method, 12, 20), + ]), + ]); + 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 ' }, + ]); + }); +}); + +describe('parseHunkRanges (one range per contiguous changed run)', () => { + it('splits a hunk into separate runs broken by context lines', () => { + // Three `return;` → `return undefined;` edits separated by context, in one + // git hunk — each is its own run, so each gets its own range. + const patch = [ + '@@ -10,12 +10,12 @@', + ' a', // new 10 (ctx) + '-return;', // old only + '+return undefined;', // new 11 ← run 1 + ' b', // new 12 (ctx) — breaks run + ' c', // new 13 + '-return;', + '+return undefined;', // new 14 ← run 2 + ' d', // new 15 (ctx) — breaks run + '-return;', + '+return undefined;', // new 16 ← run 3 + ].join('\n'); + expect(parseHunkRanges(patch)).toEqual([ + { start: 11, end: 11 }, + { start: 14, end: 14 }, + { start: 16, end: 16 }, + ]); + }); + + it('groups consecutive added lines into one run; deletions do not break it', () => { + const patch = ['@@ -1,3 +1,4 @@', ' a', '+b', '-x', '+c', ' d'].join('\n'); + // new 1=a(ctx), 2=b(+), 3=c(+) [x is old-only], 4=d(ctx) → one run 2-3 + expect(parseHunkRanges(patch)).toEqual([{ start: 2, end: 3 }]); + }); + + it('yields no range for a pure-deletion hunk', () => { + expect(parseHunkRanges('@@ -5,3 +4,0 @@\n-gone1\n-gone2')).toEqual([]); + }); + + it('separates runs across multiple hunks', () => { + const patch = ['@@ -1,1 +1,2 @@', '+a', '@@ -20,1 +22,2 @@', '+z'].join('\n'); + expect(parseHunkRanges(patch)).toEqual([ + { start: 1, end: 1 }, + { start: 22, end: 22 }, + ]); + }); +}); + +describe('buildAllLensDescriptors (symbol + change lenses)', () => { + it('adds one lens per changed run below the symbol/file lenses', () => { + const symbols = [sym(K.Function, 4, 30)]; // function lens at line 4 + 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 ' }, + ]); + }); + + it('skips a change lens that collides with a symbol lens line', () => { + const symbols = [sym(K.Function, 9, 30)]; // function lens at line 9 + 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 ' }, + ]); + }); + + it('skips a change run at line 1 that collides with the file-level lens', () => { + expect(buildAllLensDescriptors('a/b.ts', [], [{ start: 1, end: 17 }])).toEqual([ + { line: 0, title: 'Forward to Builder', refText: 'a/b.ts ' }, + ]); + }); +}); diff --git a/packages/vscode/src/commands/view-diff.ts b/packages/vscode/src/commands/view-diff.ts index 7d865c53e..d173aabe8 100644 --- a/packages/vscode/src/commands/view-diff.ts +++ b/packages/vscode/src/commands/view-diff.ts @@ -28,6 +28,8 @@ import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; import * as path from 'node:path'; import type { ConnectionManager } from '../connection-manager.js'; +import { parseHunkRanges, parseUnifiedDiff } from '../diff-inject-ref.js'; +import { setDiffInjectSession, upsertDiffInjectEntry, type DiffInjectSessionEntry } from '../diff-inject-codelens.js'; const execFileAsync = promisify(execFile); @@ -348,10 +350,9 @@ export async function viewDiff( return; } - const resources: Array<[vscode.Uri, vscode.Uri, vscode.Uri]> = planResources( - changes, - binaryPaths, - ).map(plan => { + const plans = planResources(changes, binaryPaths); + + const resources: Array<[vscode.Uri, vscode.Uri, vscode.Uri]> = plans.map(plan => { const { left, right } = diffUrisForChange(plan, { wt, ref: baseRef }); return [ vscode.Uri.file(path.join(wt, plan.resourcePath)), @@ -360,11 +361,81 @@ export async function viewDiff( ]; }); + // CodeLens "Forward to Builder" actions (#789): register the right-side fs + // paths + owning builder now (no hunks yet) so symbol/file lenses and the + // selection menu work as soon as a file is opened — without blocking the + // editor on a git call. + const filePlans = plans.filter(plan => plan.right.kind === 'file'); + setDiffInjectSession( + filePlans.map(plan => ({ + fsPath: path.join(wt, plan.resourcePath), + builderId: builder.id, + relPath: plan.resourcePath, + hunks: [], + })), + ); + await vscode.commands.executeCommand( 'vscode.changes', `Reviewing #${builder.issueId ?? builder.id} (${defaultBranch} ↔ HEAD)`, resources, ); + + // Fill per-hunk ranges after the editor is open; the provider's change event + // refreshes the lenses. Non-fatal on git failure — symbol/file lenses remain. + try { + const { stdout: patch } = await execFileAsync( + 'git', + ['-C', wt, 'diff', '-M', '--unified=3', baseRef], + { maxBuffer: 64 * 1024 * 1024 }, + ); + const hunksByPath = parseUnifiedDiff(patch); + setDiffInjectSession( + filePlans.map(plan => ({ + fsPath: path.join(wt, plan.resourcePath), + builderId: builder.id, + relPath: plan.resourcePath, + hunks: hunksByPath.get(plan.resourcePath) ?? [], + })), + ); + } catch { + // keep the hunk-less entries already registered + } +} + +/** + * Register the inject-codelens entry for a single changed file — the per-file + * `vscode.diff` opened from the Builders tree (`openBuilderFileDiff`). Mirrors + * what `viewDiff` does for the whole delta, but for one file, so the "Forward + * to Builder" lenses appear even when the reviewer opens a file diff without + * first running View Diff. Symbol lenses resolve lazily in the provider; the + * per-hunk ranges are computed here (non-fatal on git failure). + */ +export async function registerFileInjectSession(args: { + worktreePath: string; + baseRef: string; + builderId: string; + plan: ResourcePlan; +}): Promise { + // Deleted/binary files have no right-side `file:` document to host a lens. + if (args.plan.right.kind !== 'file') { return; } + const relPath = args.plan.resourcePath; + const fsPath = path.join(args.worktreePath, relPath); + // Register immediately with no hunks so the symbol/file lenses render right + // away — the git hunk computation below must not gate them. + upsertDiffInjectEntry({ fsPath, builderId: args.builderId, relPath, hunks: [] }); + try { + const { stdout } = await execFileAsync( + 'git', + ['-C', args.worktreePath, 'diff', '-M', '--unified=3', args.baseRef, '--', relPath], + { maxBuffer: 64 * 1024 * 1024 }, + ); + // Re-upsert with hunks; the provider's change event refreshes the lenses, + // adding the per-hunk ones. + upsertDiffInjectEntry({ fsPath, builderId: args.builderId, relPath, hunks: parseHunkRanges(stdout) }); + } catch { + // keep the symbol/file lenses already registered + } } interface BuilderLike { diff --git a/packages/vscode/src/diff-inject-codelens.ts b/packages/vscode/src/diff-inject-codelens.ts new file mode 100644 index 000000000..33877ffad --- /dev/null +++ b/packages/vscode/src/diff-inject-codelens.ts @@ -0,0 +1,163 @@ +/** + * CodeLens provider backing the "Forward to Builder" actions in the builder + * diff (#789). + * + * Lenses are driven by **document symbols** (functions/classes/interfaces/ + * methods), not git hunks, so granularity follows the code and a brand-new + * file is as forwardable as a modified one. The right side of each file in the + * diff is a plain `file:` document at `/`, so we + * register against `{ scheme: 'file' }` and emit lenses only for documents + * whose fs path is in the *active diff session* registry. `viewDiff` and the + * per-file diff replace/extend that registry, so lenses are scoped to the + * files the reviewer opened for diffing; any other `file:` document returns + * `[]`. + * + * Clicking a lens runs `codev.forwardToBuilder` (a palette-hidden command + * registered in `extension.ts`), which opens/reveals the builder terminal and + * types the reference into its prompt without pressing Enter. + * + * The registry also backs the `codev.activeEditorIsBuilderFile` context key + * (set on active-editor change) that scopes the right-click "Forward Selection + * to Builder" menu item to tracked builder-diff files. + * + * NOTE: CodeLens is suppressed in the multi-file `vscode.changes` editor, so + * these lenses render in the per-file `vscode.diff` and normal editor tabs + * (with `diffEditor.codeLens` enabled). The selection context-menu action is + * the path that works inside the multi-file editor. + */ + +import * as vscode from 'vscode'; +import { buildAllLensDescriptors, type ChangedRange, type SymbolNode } from './diff-inject-ref.js'; + +/** Command id the lenses invoke. Registered in `extension.ts`, NOT declared in + * `contributes.commands`, so it never appears in the Command Palette. */ +export const FORWARD_TO_BUILDER_COMMAND = 'codev.forwardToBuilder'; + +/** 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'; + +/** 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. */ + fsPath: string; + /** Builder that owns this diff (canonical id used for the terminal lookup). */ + builderId: string; + /** Repo-relative path injected into the prompt. */ + relPath: string; + /** Changed new-side line runs for the per-change lenses (empty = file/symbol lenses only). */ + hunks: ChangedRange[]; +} + +/** Map a `vscode.DocumentSymbol` tree to the pure `SymbolNode` shape. */ +function toSymbolNode(s: vscode.DocumentSymbol): SymbolNode { + return { + kind: s.kind as number, + startLine: s.range.start.line, + endLine: s.range.end.line, + children: s.children?.map(toSymbolNode) ?? [], + }; +} + +class DiffInjectCodeLensProvider implements vscode.CodeLensProvider { + private registry = new Map(); + private readonly _onDidChangeCodeLenses = new vscode.EventEmitter(); + readonly onDidChangeCodeLenses = this._onDidChangeCodeLenses.event; + + setSession(entries: DiffInjectSessionEntry[]): void { + this.registry = new Map(entries.map(e => [e.fsPath, e])); + this._onDidChangeCodeLenses.fire(); + } + + /** Add/replace a single file's entry without clearing the rest — used by the + * per-file diff path (`codev.openBuilderFileDiff`), which can be opened + * without a prior `viewDiff` run. */ + upsert(entry: DiffInjectSessionEntry): void { + this.registry.set(entry.fsPath, entry); + this._onDidChangeCodeLenses.fire(); + } + + get(fsPath: string): DiffInjectSessionEntry | undefined { + return this.registry.get(fsPath); + } + + async provideCodeLenses( + document: vscode.TextDocument, + token: vscode.CancellationToken, + ): Promise { + const entry = this.registry.get(document.uri.fsPath); + if (!entry) { return []; } + + let symbols: vscode.DocumentSymbol[] = []; + try { + symbols = + (await vscode.commands.executeCommand( + 'vscode.executeDocumentSymbolProvider', + document.uri, + )) ?? []; + } catch { + symbols = []; + } + if (token.isCancellationRequested) { return []; } + + const nodes = symbols.map(toSymbolNode); + const lastLine = Math.max(document.lineCount - 1, 0); + return buildAllLensDescriptors(entry.relPath, nodes, entry.hunks).map(d => { + const line = Math.min(Math.max(d.line, 0), lastLine); + const range = new vscode.Range(line, 0, line, 0); + return new vscode.CodeLens(range, { + title: d.title, + command: FORWARD_TO_BUILDER_COMMAND, + arguments: [entry.builderId, d.refText], + }); + }); + } + + dispose(): void { + this._onDidChangeCodeLenses.dispose(); + } +} + +const provider = new DiffInjectCodeLensProvider(); + +/** Register the provider for `file:` documents and keep the builder-file + * 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); + + context.subscriptions.push( + vscode.languages.registerCodeLensProvider({ scheme: 'file' }, provider), + vscode.window.onDidChangeActiveTextEditor(syncContextKey), + // The registry often changes AFTER the diff editor is already active — + // `openBuilderFileDiff` opens the diff, then registers its file — so the + // active-editor event fires while the registry is still empty. Re-sync the + // key whenever the registry changes (the provider fires this on + // 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)), + provider, + ); +} + +/** Replace the active diff session — called by `viewDiff` before it opens the + * multi-file diff editor. */ +export function setDiffInjectSession(entries: DiffInjectSessionEntry[]): void { + provider.setSession(entries); +} + +/** Add/replace one file's entry without clearing the rest — called by the + * per-file diff path so its lenses appear without a prior `viewDiff` run. */ +export function upsertDiffInjectEntry(entry: DiffInjectSessionEntry): void { + provider.upsert(entry); +} + +/** Look up the tracked builder-diff entry for a file path (the selection + * command uses this to resolve the owning builder + repo-relative path). */ +export function getDiffInjectEntry(fsPath: string): DiffInjectSessionEntry | undefined { + return provider.get(fsPath); +} diff --git a/packages/vscode/src/diff-inject-ref.ts b/packages/vscode/src/diff-inject-ref.ts new file mode 100644 index 000000000..fa6dd8adf --- /dev/null +++ b/packages/vscode/src/diff-inject-ref.ts @@ -0,0 +1,247 @@ +/** + * Pure helpers for the "Forward to Builder" CodeLens actions in the builder + * diff (#789). No `vscode` import — same precedent as + * `architect-reference-injection.ts`, so the symbol-selection and ref-string + * logic is unit-tested directly without mocking the editor API. + * + * Lenses are driven by **document symbols**, not git hunks: granularity follows + * the code (functions/classes/interfaces/methods), so a brand-new file is just + * as forwardable as a modified one. The provider + * (`diff-inject-codelens.ts`) resolves symbols via VSCode and adapts them to + * the `SymbolNode` shape below; all selection/anchor logic lives here. + */ + +/** + * Editor-agnostic description of one CodeLens to render: the 0-based line to + * anchor it on, the label, and the text to inject into the builder prompt. + */ +export interface LensDescriptor { + /** 0-based anchor line (the provider clamps to the document bounds). */ + line: number; + title: string; + /** Text typed into the builder terminal — always ends with a space, no Enter. */ + refText: string; +} + +/** + * Minimal, `vscode`-free projection of `vscode.DocumentSymbol` — just the + * fields the selection logic needs. `kind` is the numeric `vscode.SymbolKind` + * value; `startLine`/`endLine` are the symbol's full range (0-based). + */ +export interface SymbolNode { + kind: number; + startLine: number; + endLine: number; + children: SymbolNode[]; +} + +/** + * Numeric `vscode.SymbolKind` values (stable API enum). Kept here so the pure + * module needs no `vscode` import; the provider passes `symbol.kind` straight + * through. + */ +const KIND = { + Module: 1, + Namespace: 2, + Class: 4, + Method: 5, + Constructor: 8, + Enum: 9, + Interface: 10, + Function: 11, + Variable: 12, + Constant: 13, + Struct: 22, +} as const; + +/** Top-level structural declarations that always get a lens. */ +const TOP_LEVEL_KINDS = new Set([ + KIND.Function, + KIND.Class, + KIND.Interface, + KIND.Enum, + KIND.Struct, + KIND.Namespace, + KIND.Module, +]); + +/** + * A contiguous run of added new-side lines (1-based, inclusive) — a single + * visible change block. git groups nearby edits into one `@@` hunk, so each + * hunk is split into one `ChangedRange` per run of `+` lines (broken by + * unchanged context lines), and each run gets its own lens. `-` lines don't + * break a run (they don't occupy a new-side line); a pure-deletion hunk yields + * no range (nothing to point a new-side lens at). + */ +export interface ChangedRange { + start: number; + end: number; +} + +const HUNK_HEADER = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/; + +/** + * Parse a unified diff into one `ChangedRange` per contiguous run of added + * (`+`) new-side lines, tracking the new-side line number across each hunk. + * Context lines break a run; deletions don't (they have no new-side line). + */ +export function parseHunkRanges(patch: string): ChangedRange[] { + const ranges: ChangedRange[] = []; + let inHunk = false; + let cur = 0; // next new-side line number + let runStart = -1; + let runEnd = -1; + const flush = (): void => { + if (runStart !== -1) { + ranges.push({ start: runStart, end: runEnd }); + runStart = -1; + runEnd = -1; + } + }; + + for (const l of patch.split('\n')) { + const m = HUNK_HEADER.exec(l); + if (m) { + flush(); + cur = Number(m[1]); + inHunk = true; + continue; + } + if (!inHunk) { continue; } + if (l.startsWith('diff --git')) { flush(); inHunk = false; continue; } + if (l.startsWith('\\')) { continue; } // "\ No newline at end of file" + if (l.startsWith('+')) { + if (runStart === -1) { runStart = cur; } + runEnd = cur; + cur++; + } else if (l.startsWith('-')) { + // old-side only — does not advance the new-side counter, does not break the run + } else { + flush(); // context line ends the current run + cur++; + } + } + flush(); + return ranges; +} + +/** + * Split a multi-file unified diff (`git diff -M --unified=N `) into a map + * from each file's **new** path to its changed ranges. The new path is read + * from the `+++ b/` line; deleted files (`+++ /dev/null`) are omitted. + */ +export function parseUnifiedDiff(patch: string): Map { + const out = new Map(); + const sections = patch.split(/^diff --git .*$/m).slice(1); + for (const section of sections) { + const newPath = newPathFromSection(section); + if (!newPath) { continue; } + out.set(newPath, parseHunkRanges(section)); + } + return out; +} + +/** Extract the new-side path from a single file section's `+++ b/` line. */ +function newPathFromSection(section: string): string | null { + for (const line of section.split('\n')) { + if (!line.startsWith('+++ ')) { continue; } + const target = line.slice(4).trim(); + if (target === '/dev/null') { return null; } + return target.startsWith('b/') ? target.slice(2) : target; + } + return null; +} + +/** The whole file: ` ` (trailing space, no Enter). */ +export function buildBuilderFileRef(relPath: string): string { + return `${relPath} `; +} + +/** A line range: `:L-L `, or `:L ` for a single line + * (trailing space, no Enter). */ +export function buildBuilderRangeRef(relPath: string, start: number, end: number): string { + return start === end ? `${relPath}:L${start} ` : `${relPath}:L${start}-L${end} `; +} + +/** Label suffix for a line range: `(line N)` for one line, `(lines N-M)` otherwise. */ +function rangeLabel(start: number, end: number): string { + return start === end ? `(line ${start})` : `(lines ${start}-${end})`; +} + +/** + * Build the lens descriptors for a file from its document symbols: + * + * - A **file-level** lens at line 0 (forward the whole file). + * - A lens on each **top-level** structural declaration (the allowlist above), + * plus **top-level multi-line** Variable/Constant (catches arrow-function + * components/handlers reported as Variable, while skipping scalar consts). + * - One level into **Class/Struct** for Method/Constructor lenses, so a single + * method can be forwarded. No deeper recursion. + * + * A symbol lens that would anchor on line 0 is skipped — the file-level lens + * already occupies that line. + */ +export function buildSymbolLensDescriptors(relPath: string, symbols: SymbolNode[]): LensDescriptor[] { + const lenses: LensDescriptor[] = [ + { line: 0, title: 'Forward to Builder', refText: buildBuilderFileRef(relPath) }, + ]; + + const addLens = (s: SymbolNode): void => { + const line = Math.max(s.startLine, 0); + if (line === 0) { return; } // collides with the file-level lens + const start = s.startLine + 1; + const end = s.endLine + 1; + lenses.push({ + line, + title: `Forward to Builder ${rangeLabel(start, end)}`, + refText: buildBuilderRangeRef(relPath, start, end), + }); + }; + + for (const s of symbols) { + if (TOP_LEVEL_KINDS.has(s.kind)) { + addLens(s); + if (s.kind === KIND.Class || s.kind === KIND.Struct) { + for (const child of s.children) { + if (child.kind === KIND.Method || child.kind === KIND.Constructor) { + addLens(child); + } + } + } + } else if ( + (s.kind === KIND.Variable || s.kind === KIND.Constant) && + s.endLine > s.startLine + ) { + addLens(s); + } + } + + return lenses; +} + +/** + * Combine the symbol lenses (file-level + declarations, bare "Forward to + * Builder") with per-hunk lenses (labeled "Forward to Builder (lines N-M)" on + * each changed region). A hunk lens is skipped when its anchor line already has + * a symbol/file lens — declaration-line changes show the structural lens; body + * changes show the hunk lens — so no two lenses stack on one line. + */ +export function buildAllLensDescriptors( + relPath: string, + symbols: SymbolNode[], + ranges: ChangedRange[], +): LensDescriptor[] { + const lenses = buildSymbolLensDescriptors(relPath, symbols); + const usedLines = new Set(lenses.map(l => l.line)); + for (const r of ranges) { + const line = Math.max(r.start - 1, 0); + if (usedLines.has(line)) { continue; } // file-level (line 0) or a symbol lens already here + usedLines.add(line); + lenses.push({ + line, + title: `Forward to Builder ${rangeLabel(r.start, r.end)}`, + refText: buildBuilderRangeRef(relPath, r.start, r.end), + }); + } + return lenses; +} diff --git a/packages/vscode/src/ensure-diff-codelens.ts b/packages/vscode/src/ensure-diff-codelens.ts new file mode 100644 index 000000000..5d54bf35e --- /dev/null +++ b/packages/vscode/src/ensure-diff-codelens.ts @@ -0,0 +1,38 @@ +/** + * The "Forward to Builder" CodeLens actions (#789) only render in a diff + * editor when `diffEditor.codeLens` is on — VS Code hides CodeLens in diff + * editors by default (microsoft/vscode#97640). When a reviewer opens a builder + * file diff with the setting off, offer a one-click enable rather than letting + * the feature look silently broken. A "Don't ask again" choice is remembered in + * globalState so it never nags. + */ + +import * as vscode from 'vscode'; + +const DISMISS_KEY = 'codev.diffCodeLensPromptDismissed'; + +export async function ensureDiffEditorCodeLens( + context: vscode.ExtensionContext, +): Promise { + const cfg = vscode.workspace.getConfiguration('diffEditor'); + if (cfg.get('codeLens')) { return; } + if (context.globalState.get(DISMISS_KEY)) { return; } + + const ENABLE = 'Enable'; + const DISMISS = "Don't ask again"; + const choice = await vscode.window.showInformationMessage( + 'Codev’s "Forward to Builder" actions need CodeLens in diff editors, ' + + 'which VS Code hides by default. Enable it?', + ENABLE, + DISMISS, + ); + if (choice === ENABLE) { + // `diffEditor.codeLens` is a personal editor-behavior preference, so write + // it at the user (Global) level. Deliberately NOT Workspace — that would + // edit the repo's shared, committed `.vscode/settings.json` and force the + // choice on every collaborator. + await cfg.update('codeLens', true, vscode.ConfigurationTarget.Global); + } else if (choice === DISMISS) { + await context.globalState.update(DISMISS_KEY, true); + } +} diff --git a/packages/vscode/src/extension.ts b/packages/vscode/src/extension.ts index 0e6d40e2c..8bcf62fd7 100644 --- a/packages/vscode/src/extension.ts +++ b/packages/vscode/src/extension.ts @@ -7,7 +7,10 @@ import { sendMessage } from './commands/send.js'; import { approveGate } from './commands/approve.js'; import { cleanupBuilder } from './commands/cleanup.js'; import { openWorktreeWindow } from './commands/open-worktree-window.js'; -import { viewDiff, activateDiffView, diffUrisForChange } from './commands/view-diff.js'; +import { viewDiff, activateDiffView, diffUrisForChange, registerFileInjectSession } from './commands/view-diff.js'; +import { activateDiffInjectCodeLens, getDiffInjectEntry } from './diff-inject-codelens.js'; +import { ensureDiffEditorCodeLens } from './ensure-diff-codelens.js'; +import { buildBuilderRangeRef } from './diff-inject-ref.js'; import { runWorktreeDev } from './commands/run-worktree-dev.js'; import { stopWorktreeDev } from './commands/stop-worktree-dev.js'; import { runWorkspaceDev, stopWorkspaceDev } from './commands/run-workspace-dev.js'; @@ -805,11 +808,62 @@ export async function activate(context: vscode.ExtensionContext) { openWorktreeWindow(connectionManager!, extractBuilderId(arg))), reg('codev.viewDiff', (arg: vscode.TreeItem | string | undefined) => viewDiff(connectionManager!, extractBuilderId(arg))), + // CodeLens-only inject (#789): open + focus the builder terminal, then + // type the file/hunk reference into its prompt without submitting, so + // the reviewer keeps typing feedback before hitting Enter. Mirrors + // `codev.referenceIssueInArchitect`. Not declared in + // `contributes.commands` → never appears in the Command Palette. + reg('codev.forwardToBuilder', async (builderId: string, text: string) => { + // openBuilderByRoleOrId resolves to the canonical id and runs the + // no-terminal recovery flow on a miss; inject against that id so the + // terminal lookup hits the same key that was just opened. + const resolvedId = await terminalManager?.openBuilderByRoleOrId(builderId, true); + if (resolvedId && !terminalManager?.injectBuilderText(resolvedId, text)) { + vscode.window.showWarningMessage('Codev: Builder terminal not available'); + } + }), + // Right-click "Forward Selection to Builder" (#789): forward an arbitrary + // 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. + 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; + 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'); + } + }), reg('codev.openBuilderFileDiff', async (arg: unknown) => { if (!(arg instanceof BuilderFileTreeItem)) { return; } + // Open the diff FIRST so it appears instantly. Lens registration and + // the git hunk computation happen after — the entry registers + // synchronously (symbol/file lenses render right away) and the hunk + // lenses refresh in once git resolves (#789). Doing this before the + // open used to block the diff on a git subprocess. const { left, right } = diffUrisForChange(arg.plan, { wt: arg.worktreePath, ref: arg.baseRef }); const title = `${arg.plan.resourcePath} (#${arg.builderId})`; await vscode.commands.executeCommand('vscode.diff', left, right, title); + await registerFileInjectSession({ + worktreePath: arg.worktreePath, + baseRef: arg.baseRef, + builderId: arg.builderId, + plan: arg.plan, + }); + // Offer to enable diffEditor.codeLens (off by default — VS Code hides + // CodeLens in diff editors). After the open, so it never delays it. + await ensureDiffEditorCodeLens(context); }), regCli('codev.runWorktreeDev', (arg: vscode.TreeItem | string | undefined) => runWorktreeDev(connectionManager!, terminalManager!, extractBuilderId(arg))), @@ -883,6 +937,12 @@ export async function activate(context: vscode.ExtensionContext) { // without relying on the Git extension's worktree discovery. activateDiffView(context); + // CodeLens "Forward to Builder" actions inside the View Diff editor (#789). + // The backing command `codev.forwardToBuilder` is registered below and + // deliberately NOT declared in `contributes.commands`, so it stays out of + // the Command Palette (codelens-only entry point). + activateDiffInjectCodeLens(context); + // Review comment decorations activateReviewDecorations(context); diff --git a/packages/vscode/src/terminal-manager.ts b/packages/vscode/src/terminal-manager.ts index 827afa0ad..dce37976f 100644 --- a/packages/vscode/src/terminal-manager.ts +++ b/packages/vscode/src/terminal-manager.ts @@ -152,6 +152,22 @@ export class TerminalManager { return true; } + /** + * Type `text` into a builder terminal's input *without* a trailing newline + * (no submit) — the builder-side analogue of `injectArchitectText`, backing + * the "Forward to Builder" CodeLens (#789). Returns false if no terminal is + * registered for `builderId`; callers ensure it's open first (via + * `openBuilderByRoleOrId`, whose resolved id keys the lookup). + */ + injectBuilderText(builderId: string, text: string): boolean { + const key = `builder-${builderId}`; + const entry = this.terminals.get(key); + if (!entry) { return false; } + entry.terminal.show(); + entry.terminal.sendText(text, false); + return true; + } + /** * Open a builder terminal. If a terminal already exists for this builder * but points at a different (stale) Tower session, dispose it before @@ -188,13 +204,19 @@ export class TerminalManager { * Tower's `/api/state` can momentarily omit a live builder while its session * registry rehydrates/reconnects, and that transient miss self-heals on the * next call (PIR #982). Only a persistent miss surfaces the recovery toast. + * + * Returns the resolved **canonical** builder id on success (the key under + * which the terminal is registered), or `undefined` when the builder can't + * be opened. The "Forward to Builder" inject path (#789) uses this to target + * the same terminal that was opened, even when called with a bare numeric id; + * other callers ignore the return value. */ - async openBuilderByRoleOrId(roleOrId: string, focus = false): Promise { + async openBuilderByRoleOrId(roleOrId: string, focus = false): Promise { const client = this.connectionManager.getClient(); const workspacePath = this.connectionManager.getWorkspacePath(); if (!client || !workspacePath) { vscode.window.showErrorMessage('Codev: Not connected to Tower'); - return; + return undefined; } try { // resolveBuilderTerminal uses resolveAgentName internally so the bare @@ -212,17 +234,19 @@ export class TerminalManager { vscode.window.showWarningMessage( `Codev: Multiple builders match "${roleOrId}": ${outcome.matches.map(b => b.name).join(', ')}`, ); - return; + return undefined; } if (outcome.kind === 'missing') { await this.promptNoTerminalRecovery(roleOrId, workspacePath, focus); - return; + return undefined; } const { builder, terminalId } = outcome; await this.openBuilder(terminalId, builder.id, `Codev: ${builder.name}`, focus); + return builder.id; } catch (err) { this.log('ERROR', `Failed to open builder ${roleOrId}: ${(err as Error).message}`); vscode.window.showErrorMessage(`Codev: Failed to open ${roleOrId}`); + return undefined; } }