[Spec 761] Surface multiple architects in Tower dashboard (3.0.6 hotfix)

codev/projects/761-surface-multiple-architects-in/761-phase_1-iter1-rebuttals.md

@@ -0,0 +1,21 @@
+# Phase 1 Implementation — Iteration 1 Review Rebuttals
+
+## Verdicts
+
+| Model | Verdict |
+|-------|---------|
+| Gemini | APPROVE — "Phase 1 implementation flawlessly matches the plan with no out-of-scope changes." |
+| Codex | (unavailable — environment limitation, same as spec and plan phases) |
+| Claude | APPROVE — "Ready for Phase 2 to consume." |
+
+## Codex unavailable
+
+Same environmental issue as spec and plan phases. The architect has previously accepted the 2-of-3 result for this project.
+
+## No changes required
+
+Both available reviewers APPROVE with no blocking findings. Claude noted one minor coverage gap (no explicit `persistent: true` propagation test in the new spec-761 file) but explicitly said "not worth blocking on" since the same code path is covered by existing shell/builder loop tests.
+
+No code or test changes were made in response to this iteration.
+
+Ready to proceed to Phase 2.

codev/projects/761-surface-multiple-architects-in/761-phase_2-iter1-rebuttals.md

@@ -0,0 +1,70 @@
+# Phase 2 Implementation — Iteration 1 Review Rebuttals
+
+## Verdicts
+
+| Model | Verdict |
+|-------|---------|
+| Gemini | REQUEST_CHANGES (one critical issue, addressed) |
+| Codex | (unavailable — environment limitation) |
+| Claude | REQUEST_CHANGES (one critical bug + one minor, both addressed) |
+
+## Gemini REQUEST_CHANGES — addressed
+
+### Missing sync effect: deep links and auto-switch don't update the left pane
+
+Gemini correctly identified that the original implementation deviated from the plan in a sound way — using an independent `activeArchitectName` state in `App.tsx` (separate from `useTabs`'s `activeTabId`) to keep the right pane content intact when switching architects on the left. But the consequence was that `activeTabId` updates (from `?tab=architect:<name>` deep links and the auto-switch for newly-added architects) didn't propagate to the left pane.
+
+**Fix**: Added a one-way sync effect in `App.tsx`:
+
+```tsx
+useEffect(() => {
+  if (activeTab?.type === 'architect' && activeTab.architectName) {
+    setActiveArchitectName(activeTab.architectName);
+  }
+}, [activeTab?.id, activeTab?.type, activeTab?.architectName]);
+```
+
+This mirrors `useTabs`'s activeTab (when it lands on an architect) into the left pane's `activeArchitectName`. Strip clicks still update only `activeArchitectName` directly — they do not change `activeTabId`, preserving the right pane's content.
+
+Added a new test (`N=2: deep-link ?tab=architect:<name> syncs the left pane selection`) that asserts deep-linking to a non-main architect makes that architect's terminal visible on the left after render.
+
+## Claude REQUEST_CHANGES — both issues addressed
+
+### Critical bug: right pane blanks on reload after architect selection
+
+Claude traced the bug:
+1. User clicks `sibling` in the architect strip → `writeActiveArchitect('sibling')`.
+2. User reloads.
+3. `App.tsx` `activeArchitectName` initializes to `'sibling'` from localStorage — left pane correctly shows sibling.
+4. `useTabs` *also* reads localStorage and sets `activeTabId = 'architect:sibling'`. **This is the bug.**
+5. Right pane content checks `activeTab?.type === 'work'/'analytics'/'team'`, all false → entire right pane blanks until user clicks something.
+
+**Fix**: Removed `readActiveArchitect()` restoration and the architect-specific localStorage write from `useTabs.ts`. Persistence is now entirely owned by `App.tsx`'s strip-click handler (which already calls `writeActiveArchitect`). On reload, `useTabs` leaves `activeTabId` at the default `'work'`; the left pane independently restores via `App.tsx`'s `activeArchitectName` state.
+
+**Tradeoff acknowledged**: on mobile, where there is no left-pane strip, the architect tab is in the main TabBar. Mobile users lose architect-tab-restore-on-reload — `activeTabId` defaults to `'work'`. They can re-tap their architect tab. This is the smaller of two UX regressions; the desktop blank-right-pane issue affects the customer this hotfix is targeting.
+
+Added a new regression test (`N=2: reload with persisted architect keeps work view active on the right pane`) that asserts WorkView remains visible on the right pane while the left pane restores the persisted architect.
+
+### Minor: deploy-window fallback produced `undefined` names
+
+Claude flagged that `buildArchitectTabs`'s deploy-window fallback (scalar `state.architect` from older server) would produce `label: undefined` and `architectName: undefined` when the scalar lacked the new `name` field.
+
+**Fix**: `buildArchitectTabs` now defaults `a.name ?? 'main'` so a tab always has a non-undefined label/architectName. Single line change.
+
+## Tests updated
+
+Two test changes to reflect the corrected behaviour:
+- `useTabs.architects.test.ts`: "restores persisted active architect from localStorage" renamed to "does NOT restore active architect from localStorage into activeTabId" with rationale documenting the right-pane blanking bug it prevents.
+- `useTabs.architects.test.ts`: "writes localStorage when selectTab is called on an architect tab" renamed to "selectTab does NOT write localStorage from useTabs" — App.tsx now owns persistence.
+
+Added two new App-level tests:
+- `N=2: reload with persisted architect keeps work view active on the right pane (Claude iter-1 bug fix)`.
+- `N=2: deep-link ?tab=architect:<name> syncs the left pane selection` (Gemini iter-1 sync fix).
+
+Total dashboard tests: 22 passing (up from 20 in initial Phase 2 submission).
+
+## Codex unavailable
+
+Same environment issue as all previous phases.
+
+Ready for re-verification.

codev/projects/761-surface-multiple-architects-in/761-plan-iter1-rebuttals.md

@@ -0,0 +1,57 @@
+# Plan 761 — Iteration 1 Review Rebuttals
+
+## Verdicts
+
+| Model | Verdict |
+|-------|---------|
+| Gemini | REQUEST_CHANGES |
+| Codex | (unavailable — see below) |
+| Claude | COMMENT |
+
+## Gemini REQUEST_CHANGES — all three issues addressed
+
+### 1. Tab ID convention breaks N=1→N=2 transition
+
+> The plan decides to use `id: 'architect'` for N=1 and `id: 'architect:main'` for N>1. If a user adds an architect dynamically (N=1 → N=2), `main`'s ID will suddenly change. The `useTabs` auto-switch logic will see *both* the renamed `main` and the new `sibling` as "genuinely new tabs", leading to unpredictable focus stealing. It also unnecessarily clears `main` out of the `activatedTerminals` set.
+
+**Resolution**: Pin revised. The first architect (always `main` per Phase 1's main-first ordering) gets bare `id: 'architect'` *regardless* of N. Subsequent architects get `id: 'architect:<name>'`. N=1 keeps snapshot stability AND `main`'s ID is stable across the entire workspace lifecycle. Plan note 1 updated.
+
+### 2. `localStorage` key collision via `workspaceName`
+
+> I've audited `tower-routes.ts`: `workspaceName` is simply `path.basename(workspacePath)`. If a user has `~/work/codev` and `~/personal/codev`, both return `workspaceName: 'codev'` and will overwrite each other's active tabs.
+
+**Resolution**: Helper revised to read `window.location.pathname` (URL-encoded, globally unique per workspace). Plan's "localStorage helper" section updated with the audit rationale.
+
+### 3. `toolbarExtra` in extracted render helper
+
+> The `Terminal` component in the left pane requires the `toolbarExtra={architectToolbarExtra}` prop to render the collapse buttons. Ensure the refactored helper accepts an optional `toolbarExtra` argument.
+
+**Resolution**: Helper signature updated to `renderPersistentTerminals(tabsToRender, activeTabId, toolbarExtra?)`. Threading rule pinned: "on the active terminal only" — within the loop, pass `toolbarExtra` only when `tab.id === activeTabId`. (Both gemini and claude flagged this.) Plan note 3 added.
+
+## Claude COMMENT — main issue addressed
+
+### A. `DashboardState` import will fail due to `Annotation.parent` mismatch
+
+> The shared `Annotation` interface in `packages/types/src/api.ts:43-49` has a **required** `parent: { type: string; id?: string }` field. But the handler at `tower-routes.ts:1526-1533` builds annotations **without** `parent`. If you type `state` as `DashboardState`, TypeScript will error.
+
+**Resolution**: Phase 1 deliverables expanded to include a small companion type fix — make `Annotation.parent` optional in the shared type. **Plan-time audit done**: `grep -rn "annotation\.parent\|ann\.parent" packages/` returns only `agent-farm/state.ts:287-288` and `agent-farm/db/migrate.ts:99-100`, both of which use the *agent-farm-internal* `Annotation` type (`packages/codev/src/agent-farm/types.ts:29`), NOT the shared one. The shared `Annotation.parent` has zero callers. Making it optional aligns the shared type with reality and unblocks the clean `DashboardState` import. Plan note 4 added.
+
+A `satisfies Pick<DashboardState, 'architect' | 'architects'>` fallback is documented for any unforeseen future mismatch.
+
+### B. `architectToolbarExtra` prop threading unspecified
+
+Same as Gemini #3. Addressed.
+
+## Codex unavailable
+
+Same environment issue as spec phase. Architect previously accepted 2-of-3 result; same here.
+
+## Net change to plan from iter-1
+
+- Tab ID convention: revised from asymmetric-by-N to first-vs-rest.
+- localStorage key: pinned to `window.location.pathname`.
+- `toolbarExtra` threading: pinned (active-terminal only).
+- `Annotation.parent`: companion type fix added to Phase 1 (1-character change in shared type).
+- All decisions documented in plan's "Notes" section with iter-1 review provenance for traceability.
+
+Ready for plan-approval gate.

codev/projects/761-surface-multiple-architects-in/761-review-iter1-rebuttals.md

@@ -0,0 +1,26 @@
+# PR Review (Spec 761) — Iteration 1 Rebuttals
+
+## Verdicts
+
+| Model | Verdict |
+|-------|---------|
+| Gemini | APPROVE |
+| Codex | (unavailable — environment limitation, same as all prior phases) |
+| Claude | APPROVE |
+
+## No changes required
+
+Both available reviewers APPROVE the PR with no blocking issues.
+
+Claude noted three minor observations, none blocking:
+1. **Left pane N>1 mounts all architect terminals immediately** rather than lazy-mounting. The plan explicitly said this was acceptable for realistic N ≤ 5; documented as a small inconsistency with the right pane's lazy-mount.
+2. **N=1 → N=2 transition causes a one-time Terminal remount.** When the second architect is added while the dashboard is open, the React tree changes shape (bare `<Terminal>` → wrapped in `<div.terminal-tab-pane>`). One-time WebSocket reconnect for the `main` architect. Not during normal tab switching, so acceptable. A future follow-up could pre-wrap the N=1 terminal to eliminate the remount.
+3. **Dashboard tests not in porch's gated test runner.** Pre-existing gap (`pnpm test` from repo root only runs `@cluesmith/codev`). Already flagged in the review's "Technical Debt" and "Follow-up Items" sections.
+
+Gemini raised no issues.
+
+## Codex unavailable
+
+Same environment issue as all previous phases. Architect previously accepted 2-of-3.
+
+Ready for the PR gate.

codev/projects/761-surface-multiple-architects-in/761-spec-iter1-claude.md

@@ -0,0 +1,89 @@
+I'll read the key source files referenced in the spec to verify its claims and assess feasibility.Let me check a few more details to verify the spec's accuracy.Now I have a thorough understanding of the codebase. Let me compile my review.
+
+---
+
+## Specification Review: 761 — Surface Multiple Architects in Tower Dashboard, VSCode Extension, and `afx status`
+
+### Overall Assessment
+
+This is an exceptionally well-written specification. The Current State section accurately describes the codebase (I verified every line reference), the scope is tightly bounded, the backward-compatibility constraints are explicit, and the acceptance criteria are testable. The architect's framing from issue #761 has been faithfully translated into a structured spec with clear plan-phase decision points.
+
+### 1. Completeness
+
+**Strengths:**
+- Every collapse point (API → dashboard → VSCode → CLI) is identified with exact file/line references — all verified as accurate.
+- The "Out of scope" and "Explicitly NOT in scope" sections are unusually precise, preventing scope creep.
+- Open questions are properly triaged into "blocks progress" (none) vs "affects design" (deferred to plan phase) — this is the right call.
+
+**Minor gaps:**
+- **WebSocket lifecycle for multi-architect terminals.** The spec mentions `activatedTerminals` for persistence (test scenario NF-2) but doesn't discuss what happens when the user switches between architect tabs — specifically whether both terminals maintain active WebSocket connections or if only the visible one does. The current single-architect model has exactly one WS connection in the left pane. With N > 1, the plan phase should decide whether all N connections stay alive (simpler, more memory) or if inactive tabs tear down their WS (saves resources but adds reconnect latency). This is a plan-phase detail, but worth flagging in the spec's open questions since it affects the "no render flicker" non-functional requirement.
+- **`/api/state` inline type vs `packages/types/src/api.ts`.** The `handleWorkspaceState` function in `tower-routes.ts:1452-1461` defines its response shape as an *inline type literal* rather than importing `DashboardState` from `packages/types/src/api.ts`. Any change must update **both** the inline type and the shared interface. The spec references both files but doesn't call out this dual-definition pattern — the plan phase should ensure they stay in sync.
+
+### 2. Correctness
+
+**All codebase claims verified:**
+- ✅ `tower-routes.ts:1472-1486` — scalar architect emission from `entry.architects.get('main')` — confirmed at lines 1472-1487.
+- ✅ `tower-terminals.ts:928-940` — single `TerminalEntry` with hard-coded `id: 'architect'` — confirmed.
+- ✅ `tower-types.ts:41-46` — `architects: Map<string, string>` — confirmed at lines 41-42.
+- ✅ `useTabs.ts:27-29` — single architect tab push from `state.architect` — confirmed at lines 26-28.
+- ✅ `workspace.ts:23-34` — single `TreeItem('Open Architect')` — confirmed at lines 25-33.
+- ✅ `extension.ts:140-157` — `codev.openArchitectTerminal` reading scalar `state.architect.terminalId` — confirmed.
+- ✅ `status.ts:44-71` — Tower-running path printing terminals — confirmed.
+- ✅ `status.ts:82-92` — Tower-not-running path reading local state — confirmed at lines 82-91.
+- ✅ `schema.ts` — `architect.id TEXT PRIMARY KEY` (name) and `builders.spawned_by_architect TEXT` — confirmed at lines 18-25 and 44.
+- ✅ `state.ts` — `main`-only shim, querying `WHERE id = 'main'` — confirmed.
+
+**One minor inaccuracy:** The spec says `App.tsx:256` filters `t.type === 'architect'` from the right TabBar. The actual code at line 256 is `tabs.filter(t => t.type !== 'architect')` — same logic, but the spec uses `===` when it should say `!==` (or "filters out architect type"). This doesn't affect the spec's correctness — just a wording imprecision.
+
+### 3. Feasibility
+
+**Highly feasible.** The change is purely additive across well-separated layers:
+- The API layer adds a collection field alongside an existing scalar.
+- The dashboard React change is a conditional tab-strip inside a pane that already handles terminal persistence.
+- The VSCode extension change is a loop replacing a singleton.
+- The CLI change is a new flag + header line.
+
+**No architectural risk.** The foundation from spec #755 is solid — the `Map<string, string>` is already collection-shaped, the DB schema already stores names, and the routing layer is untouched.
+
+### 4. Edge Cases and Error Scenarios
+
+**Well covered:**
+- Dead architect terminal (scenario 6) — deferred to plan-phase but acknowledged.
+- Unknown deep-link name (scenario 8) — falls back gracefully.
+- `--architect nonexistent` error path (scenario 4) — clear error with listing.
+- Active-tab `localStorage` collision (scenario 7) — documented as last-write-wins.
+
+**One edge case not covered:**
+- **Architect registers while dashboard is open.** If a second architect is added (`afx workspace add-architect`) while the dashboard is already loaded, how does the dashboard discover it? The current `/api/state` polling (or SSE push — I didn't check which) should handle this naturally, but it's worth a sentence confirming that the existing refresh mechanism suffices. If the dashboard relies on initial load only, the new architect tab won't appear until manual refresh.
+
+### 5. Security Considerations
+
+Adequate. The spec correctly notes that names are not secrets, no new auth surfaces are introduced, and `/api/state` scoping is already per-workspace. No concerns.
+
+### 6. Testing Strategy
+
+**Thorough and well-structured.** The test matrix covers:
+- Single-architect regression (critical for the "zero visual change" constraint)
+- Multi-architect functional tests across all four surfaces
+- Deep-link backward compatibility
+- Non-functional render-flicker and payload-size parity
+
+**One suggestion:** The spec mentions "byte-identical" layout for N=1 but this is hard to assert literally in a React rendering context. The plan phase should interpret this as "DOM-structurally identical" or "snapshot-identical" rather than literal byte comparison of rendered HTML. The spec's later mention of "snapshot / DOM-presence tests" suggests this is already the intent, but the language could be clearer.
+
+### 7. VS Code Sidebar Label Change
+
+The spec proposes changing the N=1 label from `Open Architect` to `main`. This is a deliberate, small UX regression for solo-architect users — the label goes from action-oriented ("Open Architect") to identity-oriented ("main"). The spec acknowledges this as "the only delta" and justifies it for naming consistency. This is reasonable, but it's worth noting that `main` is less self-explanatory to new users than `Open Architect`. The plan phase might consider `Architect (main)` as a compromise, or deferring the label change until N > 1.
+
+---
+
+VERDICT: APPROVE
+SUMMARY: Exceptionally thorough spec with verified codebase references; two minor gaps (WebSocket lifecycle for N>1 tabs, inline-type dual-definition pattern) are plan-phase concerns, not blockers.
+CONFIDENCE: HIGH
+
+---
+
+KEY_ISSUES:
+- None that block progress. Two items for plan-phase awareness:
+  1. WebSocket lifecycle when switching between multiple architect tabs in the left pane (persist all connections vs teardown inactive ones) — should be noted as a plan-phase decision.
+  2. The inline type literal in `tower-routes.ts:1452-1461` must be updated alongside `DashboardState` in `packages/types/src/api.ts` — the plan should call this out explicitly to avoid drift.
+- Minor: consider whether renaming the VSCode sidebar label from "Open Architect" → "main" for N=1 users is the right default, or whether a less-jarring label like "Architect (main)" might serve both audiences.