vscode: honor QuickPick selection in architect reference injection

codev/plans/1139-vscode-backlog-reference-issue.md

@@ -0,0 +1,65 @@
+# PIR Plan: Backlog "Reference issue in architect" honors the QuickPick selection
+
+## Understanding
+
+In a multi-architect workspace, clicking the inline "Reference issue in architect" button on a Backlog row shows a QuickPick to choose the target architect, but the reference text is always injected into `architect:main` regardless of the pick. The picker is effectively a no-op for the injection. Single-architect workspaces are unaffected.
+
+Root cause (verified against the current source):
+
+1. `codev.openArchitectTerminal` (`packages/vscode/src/extension.ts:751`) resolves the target architect (explicit arg → QuickPick when >1 architects → default `'main'`) into a local `targetName` and opens that terminal. The handler returns nothing, so the resolution is lost to callers.
+2. `codev.referenceIssueInArchitect` (`extension.ts:1019`) awaits `executeCommand('codev.openArchitectTerminal')` (which may run the picker), then calls `injectArchitectText(text)` with no architect name (`extension.ts:1029`).
+3. `injectArchitectText` (`packages/vscode/src/terminal-manager.ts:146`) defaults `architectName` to `'main'` (a deliberate Spec 786 Phase 6 choice, before the Issue 841 Gap 2 picker existed), so injection always keys `architect:main`.
+4. `codev.referencePRInArchitect` (`extension.ts:1034`, the PR-sidebar mirror per #1043) has the identical shape at `extension.ts:1039`.
+
+Two independently-correct changes (786 Phase 6's default, 841 Gap 2's picker) crossed wires: the picker's selection is used for "which terminal to open" but never for "which terminal to inject into".
+
+## Proposed Change
+
+Follow the fix sketch from the issue — two surgical changes plus a docstring cleanup:
+
+1. **`codev.openArchitectTerminal` returns the resolved architect name** (`string | undefined`):
+   - Success path (terminal found and opened): return `targetName`.
+   - All failure paths return `undefined`: not connected, picker dismissed, no matching architect (warning shown), and the catch block.
+   - The `reg` helper (`extension.ts:732`) passes handler return values straight to `registerCommand`, so `executeCommand<string | undefined>(...)` receives the value with no plumbing changes.
+   - The single-architect path also returns the resolved name (`'main'`), so callers rely on the return value uniformly.
+
+2. **Both reference commands pass the resolved name through**:
+   - `codev.referenceIssueInArchitect`: capture `const resolvedName = await vscode.commands.executeCommand<string | undefined>('codev.openArchitectTerminal')`. If `resolvedName` is undefined (picker cancelled or open failed), return without injecting — the open path already surfaced its own error/warning where one is warranted, and a picker cancel is a deliberate user action that should be silent. Otherwise call `injectArchitectText(text, resolvedName)`.
+   - `codev.referencePRInArchitect`: same change.
+   - The existing "Codev: Architect terminal not available" warning stays for the residual case where open succeeded but the terminal lookup misses.
+
+3. **Docstring cleanup** at `terminal-manager.ts:139-144`: remove the "the Backlog button always targets `main` regardless of how many sibling architects exist" claim, note that reference-injection callers now pass the picker-resolved name, and that the `'main'` default remains for name-less callers. No behavior change in this file.
+
+## Files to Change
+
+- `packages/vscode/src/extension.ts:751-803` — `codev.openArchitectTerminal` handler returns `targetName` on success, `undefined` on every early-out (not connected, picker dismissed, architect not found, catch).
+- `packages/vscode/src/extension.ts:1019-1033` — `codev.referenceIssueInArchitect` captures the returned name; skips injection when undefined; passes the name to `injectArchitectText`.
+- `packages/vscode/src/extension.ts:1034-1043` — `codev.referencePRInArchitect` same change.
+- `packages/vscode/src/terminal-manager.ts:133-145` — docstring only: drop the "always targets main" design claim, document the new caller contract.
+- `packages/vscode/src/__tests__/extension-architect-commands.test.ts:145-159` — replace the "no name → defaults to 'main'" sentinel (which documents the old behavior) with assertions that (a) the resolved name is captured from `executeCommand` and passed as the second arg to `injectArchitectText`, and (b) injection is skipped when the resolved name is undefined. Add an assertion on the `codev.openArchitectTerminal` block that it `return targetName` on success (the new contract callers depend on).
+- `packages/vscode/src/__tests__/reference-pr-in-architect.test.ts` — add the mirrored assertions for `codev.referencePRInArchitect` (resolved name passed through, no-inject on undefined).
+
+Note on test style: this suite is deliberately source-level sentinel tests (reading `extension.ts` as text) because activating the extension requires mocking the whole `vscode` module. The updated tests keep that pattern — they anchor on the new source shape (`injectArchitectText(buildArchitectReferenceInjection(...), resolvedName)` and the early return) rather than executing the handlers.
+
+## Risks & Alternatives Considered
+
+- **Risk: double-picker UX.** None introduced — the picker still runs at most once, inside `openArchitectTerminal`; the reference commands only consume its result.
+- **Risk: other callers of `codev.openArchitectTerminal` observing a new return value.** Verified all in-repo invokers: the two reference commands (`extension.ts:1028`, `extension.ts:1038`) are the only `executeCommand` call sites; the Workspace view row (`views/workspace.ts:287`) and the Builders architect-group header (`views/builders.ts:317`) invoke via TreeItem `command` bindings with an explicit name and ignore the return; the command-relay mapping (`command-relay.ts:53`) likewise ignores it. Returning a value is additive for all of them.
+- **Risk: behavior change on failure.** Previously a failed/cancelled open still attempted injection and produced the "Architect terminal not available" warning. After the fix a cancelled picker exits silently. This is intentional (cancel is a deliberate user action) and matches the issue's fix sketch ("When the user cancels the picker, do not inject").
+- **Alternative: have `injectArchitectText` itself resolve "the last opened architect".** Rejected — introduces hidden state in the terminal manager and breaks the single-source-of-truth resolution that already lives in `openArchitectTerminal`.
+- **Alternative: duplicate the picker logic in the reference commands.** Rejected — two pickers to keep in sync; the return-value approach reuses the existing resolution untouched.
+- **Out of scope** (per the issue): the single-architect default behavior, PR sidebar semantics beyond the mirror injection, other Backlog inline actions, and the sibling `afx workspace recover` attribution bug.
+
+## Test Plan
+
+- **Unit** (`pnpm --filter codev-vscode test`, or vitest from `packages/vscode/`):
+  - Updated sentinel: `codev.referenceIssueInArchitect` passes the resolved architect name as the second argument to `injectArchitectText`, and early-returns when the resolved name is undefined.
+  - New sentinel: `codev.openArchitectTerminal` returns `targetName` on the success path.
+  - Mirrored sentinels for `codev.referencePRInArchitect`.
+  - Full existing suite passes (notably `extension-architect-commands.test.ts`, `reference-pr-in-architect.test.ts`, `terminal-manager.test.ts`).
+- **Manual** (at the dev-approval gate, in a workspace with ≥2 architects):
+  1. Click the "Reference issue in architect" inline button on a Backlog row → QuickPick appears → pick a non-main architect → verify the reference text (`#<id> "<title>" `) lands in *that* architect's terminal input, focused, not submitted.
+  2. Same flow, pick `main` → text lands in main.
+  3. Same flow, press Escape on the picker → no injection, no warning toast.
+  4. Repeat 1 with the PR sidebar's reference button → same routing.
+  5. Single-architect workspace: button injects into main with no picker (unchanged).

codev/projects/1139-vscode-backlog-reference-issue/status.yaml

@@ -0,0 +1,30 @@
+id: '1139'
+title: vscode-backlog-reference-issue
+protocol: pir
+phase: verified
+plan_phases: []
+current_plan_phase: null
+gates:
+  plan-approval:
+    status: approved
+    requested_at: '2026-07-06T04:18:06.892Z'
+    approved_at: '2026-07-07T02:55:42.842Z'
+  dev-approval:
+    status: approved
+    requested_at: '2026-07-07T03:00:50.307Z'
+    approved_at: '2026-07-08T05:39:22.972Z'
+  pr:
+    status: approved
+    requested_at: '2026-07-08T05:53:53.412Z'
+    approved_at: '2026-07-08T06:03:17.518Z'
+iteration: 1
+build_complete: true
+history: []
+started_at: '2026-07-06T04:14:56.991Z'
+updated_at: '2026-07-08T06:03:27.969Z'
+pr_history:
+  - phase: review
+    pr_number: 1157
+    branch: builder/pir-1139
+    created_at: '2026-07-08T05:49:39.187Z'
+pr_ready_for_human: false

codev/resources/lessons-learned.md

@@ -82,6 +82,7 @@ Generalizable wisdom extracted from review documents, ordered by impact. Updated
 - [From #1018] Against a *moving runtime*, only a deterministic guard holds — instructions, per-agent memory, and `git bisect` do not. The builder write-into-main-checkout bug is intrinsic model/CLI path-synthesis behavior (the model anchors a synthesized absolute path at the inferred repo root, dropping its `.builders/<id>/` worktree segment) that drifts across upgrades in both directions. The fix that survives version churn is a `PreToolUse` hook that converts a silent wrong-rooted write into a loud, correctable deny; the role-doc instruction is only a backstop. When a bug's root cause is "the model guessed wrong and got no corrective signal," reach for a runtime invariant, not a better prompt.
 - [From #1018] A guard's *surface* and its *blast radius* must match the actual hazard, not the role. The write-guard is builder-only and write-only by design: (a) the architect legitimately owns `main`, so the same hook there is a structural no-op (root resolves to the main checkout) and was deliberately not installed; (b) reads are left unguarded so codev's intentional cross-checkout reads (architect↔builder threads, sibling threads) keep working. Guarding "outside the worktree" symmetrically across roles or across read+write would have broken designed-in behavior. Scope the invariant to where the silent failure actually occurs.
 - [From #1018] `fs.writeFileSync` does not create missing parent dirs, and a git worktree only materializes directories that contain *tracked* files — git never checks out an empty dir. A path like `.claude/hooks/` (holding only a generated, intentionally-untracked file) therefore does not exist in a fresh worktree, and even `.claude/` may be absent in an adopter repo that tracks nothing under it. Any code that writes a generated file into a worktree subdir must `mkdir -p` its parent first; don't assume a dir exists just because a sibling tracked dir (e.g. `.claude/skills/`) does.
+- [From #1139] When you add an interactive resolution step (a picker, a prompt) in front of an API that has a documented defaulting parameter, the resolution must flow to every consumer of that default: return the resolved value from the command/function that owns the interaction and audit downstream callers. Two independently-correct changes composed into a silent no-op here. Spec 786 Phase 6 deliberately defaulted `injectArchitectText(architectName = 'main')` so the Backlog button kept working, and Issue 841 Gap 2 later added a QuickPick upstream in `codev.openArchitectTerminal`, but the picker's choice was consumed only for "which terminal to open," never returned, so the reference commands kept injecting into `main` no matter what the user picked. Neither change was wrong; the seam between them was. The tell to grep for: a `showQuickPick`/resolution whose result is used locally but not returned, sitting upstream of a call site that relies on a default the resolution was meant to supersede.
 - [From 810] The builder-overview shape is defined twice — the `OverviewBuilder` wire type (`packages/types`) and a structurally-identical local `BuilderOverview` interface in `overview.ts`, kept in sync by hand. Adding a field to only the wire type compiles for clients (vscode/dashboard) but breaks the codev build at the server-side `builders.push({...})` sites. Compounding footgun: the codev package has no `check-types` script, so the mismatch is invisible until a full `pnpm build` runs `tsc` over `codev/src` — vscode/dashboard type-checks pass meanwhile. When touching the overview projection, build the codev package, not just the client type-check.
 - [From 0395] Prompt-based instructions beat programmatic file manipulation for flexible document generation — the Builder already has context and can write natural responses, while code would need fragile parsing and placeholder logic
 - [From 0395] Keep specs and plans clean as forward-looking documents — append review history (consultation feedback, lessons learned) to review files, not to the documents being reviewed

codev/reviews/1139-vscode-backlog-reference-issue.md

@@ -0,0 +1,69 @@
+# PIR Review: Backlog "Reference issue in architect" honors the QuickPick selection
+
+Fixes #1139
+
+## Summary
+
+In multi-architect workspaces, the Backlog row's "Reference issue in architect" button (and its PR-sidebar mirror) showed a QuickPick to choose the target architect but always injected the reference text into `architect:main`, making the picker a no-op. The fix makes `codev.openArchitectTerminal` return the architect name it actually resolved (explicit arg, picker choice, or single-architect default), and both reference commands now pass that name through to `injectArchitectText`. A cancelled picker skips injection silently instead of falling back to main.
+
+## Files Changed
+
+- `packages/vscode/src/extension.ts` (+28 / -7): `openArchitectTerminal` returns `Promise<string | undefined>`; both reference commands capture and pass the resolved name, early-returning on undefined
+- `packages/vscode/src/terminal-manager.ts` (+6 / -6): docstring only, removed the stale "the Backlog button always targets main" design claim
+- `packages/vscode/src/__tests__/extension-architect-commands.test.ts` (+36 / -11): replaced the sentinel that codified the old always-main behavior with return-contract, name-pass-through, and cancel-path sentinels
+- `packages/vscode/src/__tests__/reference-pr-in-architect.test.ts` (+19 / -0): mirrored sentinels for the PR-sidebar command
+- `codev/plans/1139-vscode-backlog-reference-issue.md` (+65): plan artifact
+- `codev/state/pir-1139_thread.md` (+20): builder thread
+- `codev/resources/lessons-learned.md`: one new Architecture entry (see Lessons Learned Updates)
+
+## Commits
+
+- `8f34a761` [PIR #1139] Plan draft
+- `2e0ca369` [PIR #1139] Honor QuickPick selection in architect reference injection
+- `dd29bbbe` [PIR #1139] Thread: implement phase notes
+- plus porch state-transition commits (`chore(porch): ...`)
+
+## Test Results
+
+- `pnpm compile` (tsc + tsc webview + eslint + esbuild): pass
+- `pnpm test:unit` (vitest): pass, 47 files, 547 tests (4 sentinel tests new or rewritten)
+- Manual verification: approved by the human reviewer at the `dev-approval` gate, exercising the running worktree
+
+## Architecture Updates
+
+No arch changes: this PR fixes command wiring inside the VS Code extension (a return value threaded between two existing commands). No module boundaries, state, or cross-subsystem contracts changed. The `injectArchitectText` `'main'` default remains for name-less callers.
+
+## Lessons Learned Updates
+
+One COLD-tier entry added to `codev/resources/lessons-learned.md` (Architecture section): two independently-correct changes composed into this bug. Spec 786 Phase 6 deliberately defaulted `injectArchitectText` to `'main'`; Issue 841 Gap 2 later added a QuickPick upstream in `openArchitectTerminal`. The picker's resolution was consumed for "which terminal to open" but never returned, so downstream consumers of the default silently kept the pre-picker behavior. The lesson: when adding an interactive resolution step in front of an API with a documented default, return the resolution and audit every consumer of that default.
+
+Nothing HOT-tier: the rule is narrow to command-composition inside the extension, not a behavior-changing cross-cutting invariant.
+
+## Things to Look At During PR Review
+
+- The deliberate behavior change on cancel: previously a dismissed picker still attempted injection into main (surfacing a "terminal not available" warning when main's terminal wasn't registered); now a cancel exits silently. This matches the issue's fix sketch.
+- Every early-out of `codev.openArchitectTerminal` now explicitly `return undefined` (not connected, picker dismissed, architect not found, workspace-state fetch failure) so callers get a uniform contract.
+- The tests are source-level sentinels (the suite's established pattern, since activating the extension requires mocking the whole `vscode` module); they anchor on the new source shape rather than executing handlers.
+
+## Consultation Findings and Dispositions (single advisory pass)
+
+PIR runs one consultation pass with no automated re-review, so both codex findings are recorded here for the human at the `pr` gate:
+
+- **codex REQUEST_CHANGES: plan file lacks `approved`/`validated` YAML frontmatter.** Rebutted, no change made. The frontmatter rule (CLAUDE.md, "Approved specs/plans need YAML frontmatter") applies to artifacts the architect creates and approves on `main` *before spawning a builder*, so porch can no-op the corresponding phase. This plan was authored inside the porch-driven PIR flow; its approval is structured porch state (`plan-approval` gate approved, commit `684f32dd`, recorded in `status.yaml` history). Adding `validated: [gemini, codex, claude]` would be factually wrong: PIR's plan phase is human-only review by design.
+- **codex REQUEST_CHANGES: "How to Test Locally" didn't explain how to load the modified extension.** Accepted, documentation fix applied below (this is a review-file correction, not a code defect, so no regression test applies). The claude consult returned APPROVE with no findings.
+
+## How to Test Locally
+
+This is a VS Code extension change, so the dev server alone does not load the modified bundle. To exercise the fix:
+
+1. Open the worktree as its own window: VSCode sidebar, right-click builder pir-1139, **Open Worktree as Workspace** (or `code .builders/pir-1139`).
+2. In that window, press F5 ("Run Codev Extension", the repo's `.vscode/launch.json` config). The pre-launch task builds the extension and an Extension Development Host window opens running the modified extension.
+3. Alternative without the dev host: `cd packages/vscode && pnpm vsix`, then install the generated `.vsix` via the Extensions view ("Install from VSIX...") in your normal window, and reload.
+
+- **View diff**: VSCode sidebar, right-click builder pir-1139, **View Diff**
+- **What to verify** (needs a workspace with 2+ architects):
+  - Backlog row mention button: pick a non-main architect in the QuickPick; `#<id> "<title>" ` lands in that architect's terminal, focused, not submitted
+  - Same flow picking `main`: text lands in main
+  - Same flow pressing Escape: no injection, no warning
+  - PR sidebar mention button: same routing behavior
+  - Single-architect workspace: no picker, injects into main, unchanged