diff --git a/codev/plans/761-surface-multiple-architects-in.md b/codev/plans/761-surface-multiple-architects-in.md new file mode 100644 index 000000000..5535d81e8 --- /dev/null +++ b/codev/plans/761-surface-multiple-architects-in.md @@ -0,0 +1,409 @@ +# Plan: Surface Multiple Architects in Tower Dashboard (3.0.6 hotfix) + +## Metadata +- **ID**: 761-surface-multiple-architects-in +- **Status**: draft +- **Specification**: [codev/specs/761-surface-multiple-architects-in.md](../specs/761-surface-multiple-architects-in.md) +- **Created**: 2026-05-18 +- **GitHub Issue**: #761 +- **Target release**: 3.0.6 (Ionic hotfix on top of 3.0.5) +- **Predecessor**: PR #757 / spec #755 + +## Executive Summary + +The spec calls for **dashboard-tabs-only** in v1 (architect's 2026-05-18T20:48Z slicing directive). Two coupled changes, ordered: + +1. **`/api/state` exposes the architects collection.** Add `architects: ArchitectState[]` to `DashboardState`; preserve the scalar `architect` field unchanged. The shared interface and the inline literal in `tower-routes.ts:handleWorkspaceState` must be kept in sync. +2. **Dashboard renders one tab per architect when N > 1.** Extend `useTabs` to push one tab per architect; restructure `App.tsx`'s left pane so it participates in the `activatedTerminals` lazy-mount pattern (preventing terminal remount on tab switch); persist active-architect name in `localStorage`; extend deep-link parsing to recognise `?tab=architect:`. + +Single-architect workspaces (the dominant population) must see a DOM-snapshot-identical dashboard. + +This plan splits the two scope items into two implementation phases. Each phase is one self-contained commit. Both ship in one PR. + +## Success Metrics + +All criteria from `codev/specs/761-surface-multiple-architects-in.md` apply. Roll-up: + +- [ ] `/api/state` returns `architects: ArchitectState[]` (with `name`) AND the preserved scalar `architect`. Both are consistent with `entry.architects`. +- [ ] `DashboardState` (shared) and the inline response type in `tower-routes.ts:handleWorkspaceState` cannot drift (compile-time import OR asserted test). +- [ ] Dashboard renders one tab per architect when N>1, in a tab strip inside the left pane (desktop) and inline in the main TabBar (mobile). +- [ ] Active-architect name persists per workspace in `localStorage`; reload restores it. +- [ ] Deep link `?tab=architect:` works; unknown name falls back to default; bare `?tab=architect` works as before. +- [ ] Switching between architect tabs does not unmount or remount any Terminal component (no WebSocket reconnect-flicker). +- [ ] N=1 dashboard is DOM-snapshot-identical to the pre-761 baseline. +- [ ] All existing tests pass; new tests cover the listed scenarios from spec. + +## Phases (Machine Readable) + +```json +{ + "phases": [ + {"id": "phase_1", "title": "API state: expose architects collection"}, + {"id": "phase_2", "title": "Dashboard: per-architect tabs + left-pane persistent rendering"} + ] +} +``` + +## Phase Breakdown + +### Phase 1: API state — expose architects collection + +**Dependencies**: None. + +#### Objectives + +- Extend the `/api/state` response to carry the full architects collection alongside the preserved scalar. +- Eliminate the dual-type-definition drift between `DashboardState` and `tower-routes.ts:handleWorkspaceState` so the change is structurally safe going forward. +- Zero user-visible change at this point — the dashboard still reads the scalar. + +#### Deliverables + +- [ ] `packages/types/src/api.ts`: + - Add `name: string` to `ArchitectState`. + - Add `architects: ArchitectState[]` to `DashboardState`. + - Existing scalar `architect: ArchitectState | null` stays exactly as it is today. + - **Also (small companion fix)**: change `Annotation.parent` from required to optional (`parent?: { type: string; id?: string }`) in the **shared** `packages/types/src/api.ts` interface. The handler's inline literal at `tower-routes.ts:1526-1533` builds annotations without it. 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` is currently consumed by zero callers. Making it optional aligns the shared type with reality and is a prerequisite for the clean `DashboardState` import. +- [ ] `packages/codev/src/agent-farm/servers/tower-routes.ts:1443-1537` (`handleWorkspaceState`): + - Replace the inline response type literal at lines 1452-1461 by importing `DashboardState` from `@cluesmith/codev-types` and typing the local `state` variable as `DashboardState`. (This is the lower-risk option than keeping two declarations and asserting equality at test time.) + - Build the `architects` array by iterating `entry.architects` (name → terminalId) and, for each, looking up the session via `manager.getSession(terminalId)`; emit `{ name, port: 0, pid, terminalId, persistent }` per entry. Skip entries whose session is unavailable. Entries are emitted in `Map` iteration order (insertion order); the `main` entry, when present, is moved to the front so it is always `architects[0]`. + - Continue populating the scalar `architect` exactly as today (`entry.architects.get('main') ?? entry.architects.values().next().value` → derive the same `ArchitectState` shape, now also carrying `name`). +- [ ] `packages/codev/src/agent-farm/servers/tower-types.ts:62-79`: + - **Not** touched in this phase. `InstanceStatus.architectUrl` stays scalar; `TerminalEntry` stays as-is. (Spec is explicit: `getTerminalsForWorkspace` is NOT modified in v1.) +- [ ] **No changes** to `packages/codev/src/agent-farm/servers/tower-terminals.ts` or `tower-instances.ts`. Explicitly out of scope for v1 (see spec, References → "Backend call site explicitly NOT touched in v1"). + +#### Implementation Details + +The `entry.architects: Map` is already populated by spec #755's machinery. No SQLite migration, no Tower-side data plumbing. The handler change is pure read-and-shape. + +**Front-of-array convention for `main`**: Iteration order for `Map` is insertion order; `main` may or may not be inserted first depending on workspace history. Surfacing `architects[0]` as a *reliable* "main pointer" simplifies the dashboard's "default-architect" picker. Implementation: build the array by appending non-`main` entries in iteration order, then `unshift` the `main` entry if present. Document this contract in the JSDoc on `DashboardState.architects`. + +**Type sync via import**: removing the inline literal eliminates the drift class entirely. The handler's local `state` variable becomes typed as `DashboardState`. The one type mismatch this exposes is `Annotation.parent`: the shared type marks it required but the handler does not populate it. The companion type fix above (making `parent` optional) resolves this. With that in place, `state: DashboardState` compiles cleanly. If a future field on `DashboardState` also drifts, the compiler will catch it at this seam — which is exactly the drift-prevention this seeks. + +If during implementation a different unforeseen mismatch surfaces, fall back to a typed Pick assertion at the end of the handler before serialisation: +```ts +const finalState = state satisfies Pick; +return res.end(JSON.stringify(finalState)); +``` +This still anchors the architect-related fields to the shared type while letting other inline-only fields stay loose. Document any such mismatch in the commit message; full alignment is the goal but partial alignment is acceptable if scope creep would otherwise grow. + +**Performance impact**: zero — same Map iteration that already happens for the scalar selection. The new array allocation is bounded by N (≤5 in practice). + +#### Acceptance Criteria + +- [ ] `tsc` passes after the change. +- [ ] `/api/state` for a single-architect workspace returns the new field `architects: [{ name: 'main', port: 0, pid: ..., terminalId: ..., persistent: ... }]` AND the preserved scalar `architect` populated identically. +- [ ] `/api/state` for a two-architect workspace returns `architects` with both entries; `main` is at index 0 if present; scalar `architect` matches `architects[0]`. +- [ ] Type-sync test (or simply the fact that the inline literal no longer exists) asserts no drift. +- [ ] All existing tests pass. + +#### Test Plan + +- **Unit / API tests** (`packages/codev/src/agent-farm/__tests__/spec-761-api-state.test.ts` — new file): + - Build a fake workspace entry with `entry.architects = new Map([['main', 'term-1']])` and assert the `/api/state` response. + - Same with two architects (`main` + `sibling`); assert array contents and ordering (`main` first). + - Same with three architects, custom name order, asserting `main`-first ordering. + - Workspace with no architects: `architects: []` and `architect: null`. +- **Type-sync guard**: removing the inline literal means the type is sourced from `DashboardState`; no additional test needed beyond `tsc`. +- **Regression**: existing `tower-routes.test.ts` tests for `/api/state` must continue to pass without modification (they assert the scalar shape and should not care about the new additive field). + +#### Rollback Strategy + +Revert the single commit; this phase introduces no migrations and no runtime side effects. The scalar `architect` field is unchanged, so consumers stuck on the previous build will continue to work. + +#### Risks + +- **Risk**: An existing test in `tower-routes.test.ts` asserts the response uses the old inline literal exactly. **Mitigation**: read the existing assertion before editing; if it asserts an absent `architects` key it must be updated to allow either presence (preferred — additive) or rewritten to test the new collection directly. +- **Risk**: The architect-session lookup fails for one entry in `entry.architects` (race, stale registration). **Mitigation**: skip silently (as the scalar handler does today for the single-architect case). Log via `_deps.log` if available. + +--- + +### Phase 2: Dashboard — per-architect tabs + left-pane persistent rendering + +**Dependencies**: Phase 1. + +#### Objectives + +- The dashboard renders one tab per architect when N > 1. When N = 1, the visual is DOM-snapshot-identical to today. +- The left pane participates in the `activatedTerminals` lazy-mount + keep-alive pattern so switching architect tabs does not unmount terminals. +- Active architect name persists per-workspace in `localStorage`; reload restores it. +- Deep link `?tab=architect:` selects the named architect; `?tab=architect` still works. + +#### Deliverables + +- [ ] `packages/types/src/api.ts` — already updated in Phase 1; no further changes. +- [ ] `packages/dashboard/src/lib/api.ts`: + - Re-export `ArchitectState` from `@cluesmith/codev-types` (or update the existing re-export to ensure `name: string` is available to consumers). The dashboard already re-exports the types it consumes from this module; just confirm the new field is visible. +- [ ] `packages/dashboard/src/hooks/useTabs.ts`: + - Extend the `Tab` interface (lines 4-15) with an optional `architectName?: string` field so tabs carry the architect identity (the `id` and `label` already carry it implicitly, but a typed field is cleaner for downstream consumers). + - Replace the existing scalar-driven push (lines 27-29) with a loop over `state.architects ?? []` (with a fallback to `state.architect` populated as a one-element array if `architects` is absent — handles a momentary deploy-window where dashboard.js is newer than the server). Push one tab per architect: `{ id, type: 'architect', label: a.name, closable: false, terminalId: a.terminalId, persistent: a.persistent, architectName: a.name }`. + - **Tab ID convention (revised per Gemini iter-1 review)**: The *first* architect tab always uses `id: 'architect'` (bare, no name suffix), regardless of N. Subsequent architect tabs use `id: 'architect:'`. Since Phase 1 emits `main` at `architects[0]` whenever it is registered, the first tab is `main` in essentially all real workspaces — its ID is stable across the N=1→N=2 transition. The earlier draft of this plan used asymmetric N-dependent IDs (bare for N=1, prefixed for N>1) which would have re-keyed `main`'s ID on transition, breaking `activatedTerminals` tracking and auto-switch logic. The first-vs-rest convention preserves DOM-snapshot stability for N=1 AND keeps the first tab's ID stable for every workspace lifecycle. + - Extend the `?tab=` deep-link parser in the `useEffect` at lines 79-99: add a check for `tabParam?.startsWith('architect:')` before the existing match-by-id-or-type. Extract the suffix as `name`; find the architect tab whose `architectName === name`; if not found, fall back to the existing match-by-type which lands on the first architect tab. + - Adjust the auto-switch skip at line 115 (`tab.type !== 'architect'`): this currently suppresses auto-selection of newly-appearing architect tabs (a deliberate choice when there was only one architect tab that existed at page-load). Now that architects can be added post-load via `afx workspace add-architect`, the plan decision is: **invert the skip for architect tabs**. New architect tabs auto-switch like builder tabs do today. This matches the spec's "important question" resolution and gives external customers a useful "appears in the dashboard immediately after `add-architect`" UX. + - **localStorage persistence**: a small helper module `packages/dashboard/src/lib/architectPersistence.ts` exporting `readActiveArchitect(workspacePath): string | null` and `writeActiveArchitect(workspacePath, name)`. Key namespace: `codev-active-architect:`. Plan-pinned key name — audit confirmed only `codev-web-key` and `TipBanner` date keys exist today. + - `useTabs` reads the persisted active architect on initial mount (in the same effect that handles the `?tab=` URL parameter, with URL parameter taking precedence). When the user clicks an architect tab (via `selectTab`), the helper persists the architect name. +- [ ] `packages/dashboard/src/components/App.tsx`: + - **The left pane must participate in `activatedTerminals` lazy-mount + keep-alive**. Currently `App.tsx:184` finds `const architectTab = tabs.find(t => t.type === 'architect')` and renders ONE `Terminal` component at `App.tsx:236-238`. New approach: + - Replace single-tab discovery with `const architectTabs = tabs.filter(t => t.type === 'architect')`. + - When N>1, render a small tab strip ABOVE the terminal area inside the left pane (above the existing `architectToolbarExtra` row, or as a new sibling — implementation detail decided in the React tree, not load-bearing). The strip uses the existing `tab` and `tab-active` CSS classes for visual consistency with the right-pane TabBar, but is its own React component (e.g., `ArchitectTabStrip`) to keep responsibility contained. + - When N=1, do NOT render the strip — the existing bare-terminal rendering is preserved exactly. (DOM-snapshot-identical constraint.) + - For the terminal content, render N hidden + 1 visible `Terminal` components using the same `display: none` + `activatedTerminals` pattern from `renderPersistentContent` (lines 113-149). Implementation: extract a `renderPersistentTerminals(tabs)` helper that takes a list and renders them with the persistence pattern; call it once for the left pane (architects) and once for the right pane (builders + shells, as today). + - The right pane's `TabBar` filter `t.type !== 'architect'` (line 256) is unchanged; architect tabs stay out of the right tab bar regardless of N. + - Mobile (`MobileLayout`, line 168-181): no change needed. `useTabs` already produces the right tab list; the existing `renderPersistentContent(['architect', 'builder', 'shell'])` call covers all architect tabs. + - `architectToolbarExtra` (the collapse buttons) is rendered on the currently-active architect terminal, exactly like today. When there is no active architect, the existing `"No architect terminal"` empty state shows. + - **Active-tab selection**: the active-tab ID for an architect comes from `useTabs`. When the user clicks a strip entry, `useTabs.selectTab(id)` runs; that change in `activeTabId` causes the corresponding terminal pane's `display` to flip from `none` to undefined. No remount; no WebSocket reconnect. +- [ ] New module: `packages/dashboard/src/components/ArchitectTabStrip.tsx`: + - Renders a horizontal strip of architect tab buttons. Props: `tabs: Tab[]`, `activeTabId: string`, `onSelectTab: (id) => void`. Visual style: same as the right-pane `TabBar` (reuse classes) but without close buttons (architect tabs are not closable). +- [ ] **NO changes** in this phase to `Tab` IDs of non-architect tabs, the right-pane `TabBar`, `MobileLayout`, `WorkView`, or any other surface. +- [ ] Single-architect DOM snapshot tests: + - `App.terminal-persistence.test.tsx` is the existing relevant test family. + - Add `packages/dashboard/__tests__/App.architect-tabs.test.tsx` with two cases: + - N=1: snapshot of left-pane DOM matches a baseline; no `architect-tab-strip` element present. + - N=2: left-pane DOM has an `ArchitectTabStrip` with two entries; clicking one toggles the visible terminal pane via the `display` style; both `Terminal` components remain mounted. + - Add `packages/dashboard/__tests__/useTabs.architects.test.ts` for unit-level tests of the hook: deep-link parsing, localStorage round-trip, auto-switch behaviour, N=0 / N=1 / N=2 / N=3 cases. + +#### Implementation Details + +**Persistent-terminal extraction**. The existing `renderPersistentContent` (`App.tsx:113-149`) does the lazy-mount + keep-alive pattern for the right-pane terminal tabs. The cleanest refactor is to: + +1. Extract a pure `renderPersistentTerminals(tabsToRender, activeTabId, toolbarExtra?): JSX.Element[]` helper. Signature includes an optional `toolbarExtra` prop that is threaded through to the **active** terminal only (passing it to all would render multiple copies of the collapse buttons; only the visible terminal needs them). No new logic beyond that — the existing inline loop is hoisted into a named function. +2. The right-pane caller maps to `renderPersistentTerminals(persistentRightTabs, activeTabId)` exactly as today (no `toolbarExtra`). +3. The left-pane caller maps to `renderPersistentTerminals(architectTabs, activeTabId, architectToolbarExtra)` when N > 1; when N = 1, falls through to the bare `` render at `App.tsx:236-238` (preserves DOM identity for snapshot stability). + +**`toolbarExtra` threading rule (pinned per Gemini iter-1 and Claude iter-1 reviews)**: on the active terminal only. Within `renderPersistentTerminals`, when `tab.id === activeTabId`, pass `toolbarExtra` as a prop; otherwise omit it. This matches the today's single-terminal behaviour (toolbar visible on the architect terminal) and keeps the collapse buttons correctly placed when switching architects. + +The `activatedTerminals` Set in `App.tsx:39` is keyed by `tab.id`; since architect tab IDs are now `architect:main` etc. (N>1 only), the Set will simply gain those keys as the user visits them. The existing `useEffect` at `App.tsx:76-87` that marks the active terminal as activated continues to work — it checks `activeTab.type === 'architect' || 'builder' || 'shell'`. + +**Deep-link parsing addition**: + +```ts +// inside the existing useEffect in useTabs.ts +const tabParam = urlParams.get('tab'); +if (tabParam) { + // New: handle architect: form + if (tabParam.startsWith('architect:')) { + const name = tabParam.slice('architect:'.length); + const archTab = tabs.find(t => t.architectName === name); + if (archTab) { + setActiveTabId(archTab.id); + urlTabHandled.current = true; + // ... existing URL cleanup + return; + } + } + // Existing: match by id or type (handles bare ?tab=architect → first architect tab) + const matchingTab = tabs.find(t => t.id === tabParam || t.type === tabParam); + if (matchingTab) { + setActiveTabId(matchingTab.id); + urlTabHandled.current = true; + // ... existing URL cleanup + } +} else { + urlTabHandled.current = true; +} +``` + +**localStorage helper**: + +```ts +// packages/dashboard/src/lib/architectPersistence.ts +const KEY_PREFIX = 'codev-active-architect:'; + +function workspaceKey(): string { + // Use window.location.pathname (e.g. "/workspace/Users%2Fmwk%2F.../"). + // Audit (Gemini iter-1): DashboardState.workspaceName = path.basename(workspacePath), + // which collides when two workspaces share a basename (e.g. ~/work/codev vs ~/personal/codev). + // pathname is URL-encoded and globally unique per workspace. + return window.location.pathname; +} + +export function readActiveArchitect(): string | null { + try { return localStorage.getItem(KEY_PREFIX + workspaceKey()); } + catch { return null; } +} + +export function writeActiveArchitect(name: string): void { + try { localStorage.setItem(KEY_PREFIX + workspaceKey(), name); } + catch { /* quota / SSR / private-mode — silently ignore */ } +} +``` + +**Pinned (revised per Gemini iter-1 review)**: use `window.location.pathname` for the key suffix. `DashboardState.workspaceName` is just `path.basename`, which is NOT globally unique. The pathname is the URL-encoded full workspace path the dashboard is served at, guaranteed unique per workspace within a Tower. + +**Auto-switch skip inversion**: + +```ts +// useTabs.ts line 115 — current: +if (!knownTabIds.current.has(tab.id) && tab.type !== 'architect') { + setActiveTabId(tab.id); +} +// new: +if (!knownTabIds.current.has(tab.id)) { + setActiveTabId(tab.id); +} +``` + +The original skip was added when there was only one architect tab and it was pre-existing at page-load (so a "new" architect tab was conceptually nonsense). Now that adding architects post-load is supported by spec #755's `afx workspace add-architect`, removing the skip gives users immediate visual feedback. Caveat: on initial page load, ALL architect tabs are "new" relative to the empty seed. The existing seed-on-non-null-state pattern (lines 105-112) already handles that — `knownTabIds.current` is set to the full current list once `state !== null` is observed; only then does auto-switch fire. So removing the skip is safe. + +**Dead architect tab handling** (spec open question): pinned here as **omit dead architects from the tab strip**. The `architects` array in `/api/state` is built from sessions where `manager.getSession(terminalId)` returns a live session (Phase 1's skip-silently rule). So a dead architect terminal won't appear in `state.architects` and its tab won't be rendered. No grey-out indicator. (Cleanest, smallest change. Follow-up issues can revisit.) + +#### Acceptance Criteria + +- [ ] `pnpm build` succeeds; `tsc` clean. +- [ ] N=1 dashboard DOM matches snapshot (no `ArchitectTabStrip` element). +- [ ] N=2 dashboard renders the tab strip with two entries; click toggles which terminal pane is visible; both `Terminal` components remain mounted via `display: none`. +- [ ] `localStorage` key `codev-active-architect:` is written when an architect tab is selected, and read on reload to restore the selection. +- [ ] `?tab=architect:sibling` opens `sibling` tab on initial load; `?tab=architect:ghost` falls back to the first architect tab. +- [ ] New architect added via `afx workspace add-architect --name foo` post-load appears in the strip on the next `/api/state` poll and auto-switches to. +- [ ] All existing tests pass, especially `App.terminal-persistence.test.tsx` and `architect-toolbar.test.tsx`. +- [ ] Mobile layout unchanged behaviourally; multi-architect appears as multiple tabs in the main TabBar. + +#### Test Plan + +- **`packages/dashboard/__tests__/useTabs.architects.test.ts`** (new): + - Hook unit test (using `@testing-library/react-hooks` or equivalent already in the suite). + - Cases: + - `state.architects: []` → no architect tab. + - `state.architects: [{name: 'main', ...}]` → one tab, `id: 'architect'`, `label: 'main'`. (Bare ID preserves DOM identity.) + - `state.architects: [{main}, {sibling}]` → two tabs, IDs `architect:main` and `architect:sibling`. + - Initial load with `?tab=architect:sibling` → `activeTabId === 'architect:sibling'`. + - Initial load with `?tab=architect:ghost` → falls back to `'architect:main'` (or the first one). + - Initial load with `?tab=architect` → falls back to first architect tab. + - `localStorage` round-trip: write `sibling`, simulate reload, hook reads `sibling` as initial active. + - URL `?tab=` takes precedence over `localStorage`. + - Post-load architect addition: `state.architects` grows from 1 to 2; `useTabs` returns 2 tabs; `activeTabId` auto-switches to the new one. +- **`packages/dashboard/__tests__/App.architect-tabs.test.tsx`** (new): + - Render `App` with mocked `useBuilderStatus` returning `state.architects` of various sizes. + - N=0: no architect tab, `"No architect terminal"` empty state, snapshot match. + - N=1: one architect, no tab strip in left pane, bare `Terminal` rendered, snapshot match for left pane DOM. + - N=2: tab strip present with two entries; clicking each toggles which terminal is visible (`display: none` style swap); both Terminals remain in the DOM throughout. + - WebSocket lifecycle: assert no remount of either `Terminal` across a click sequence (existing test patterns in `App.terminal-persistence.test.tsx` show how to assert this). +- **`packages/dashboard/__tests__/ArchitectTabStrip.test.tsx`** (new): + - Component-level tests: renders one button per tab; calls `onSelectTab` with the right ID on click; renders active-state styling for `activeTabId`. +- **Existing tests** that must still pass without modification: + - `App.terminal-persistence.test.tsx` + - `architect-toolbar.test.tsx` + - `TabBar.test.tsx` + - `SplitPane.test.tsx` + - `MobileLayout.test.tsx` +- **E2E (deferred unless trivial)**: an explicit Playwright test that drives a two-architect workspace through tab-switching and reload would be valuable; the codebase has Playwright infra (`codev/resources/testing-guide.md`). Implementation-phase to decide whether to add or defer to the follow-up issues. The unit + component tests above are the floor. + +#### Rollback Strategy + +Revert the phase commit. Phase 1's `/api/state` change continues to ship harmlessly (additive field that the dashboard chose to ignore). No data loss, no migration. + +#### Risks + +- **Risk**: The DOM-snapshot test for N=1 catches a tiny structural change introduced by the refactor (e.g., an extra wrapper `
`). **Mitigation**: design the refactor to either (a) keep the N=1 path bypassing the new `renderPersistentTerminals` entirely, or (b) emit identical DOM at N=1. Plan-pin (a) as the default — N=1 keeps the bare `` render at `App.tsx:236-238`, no new wrappers. +- **Risk**: localStorage write fails (quota, SSR, private mode). **Mitigation**: the helper is wrapped in try/catch; missed persistence is a usability degradation, not a crash. +- **Risk**: Removing the auto-switch skip causes a visible flash on initial load (the first architect tab steals focus from `'work'`). **Mitigation**: the existing seed-on-non-null pattern in `useTabs.ts:105-112` means the first non-null state populates `knownTabIds` with all current tabs; only post-seed changes trigger auto-switch. Test asserts initial active tab is `'work'` for a workspace that has architects at page-load time. +- **Risk**: The `useTabs.ts` `tabs.map(t => t.id).join(',')` dependency in the auto-switch `useEffect` (line 121) recomputes on every state poll, even when the tab list is unchanged. Adding `architectName` to `Tab` does not change this; just verify the dependency string still uniquely captures tab identity for the equality check. **Mitigation**: existing dependency is fine — IDs alone are unique. +- **Risk**: Reusing the `tab` and `tab-active` CSS classes from `TabBar` couples the strip's visual to the right pane's. **Mitigation**: acceptable for a hotfix; the visual consistency is a feature. Follow-up styling work can fork the classes if needed. + +--- + +## Dependency Map + +``` +Phase 1 (API) ──→ Phase 2 (Dashboard) +``` + +Both phases ship in one PR. Each is a single atomic commit. + +## Resource Requirements + +### Development resources + +- Single builder. Both phases are React + Node TypeScript; no new tooling. + +### Infrastructure + +- None new. No SQLite migration. No new services. No new dependencies in `package.json`. + +## Integration Points + +### Internal systems + +- Tower's `entry.architects: Map` — read-only consumer. +- Dashboard's existing `useBuilderStatus` polling — adds the new `architects` field to its consumed state; no change to fetch logic. +- Dashboard's existing `activatedTerminals` lazy-mount pattern — extended to the left pane. + +### External systems + +- None. + +## Risk Analysis + +### Technical risks + +| Risk | Probability | Impact | Mitigation | Owner | +|------|------------|--------|------------|-------| +| Inline-type drift returns in a future PR after Phase 1 removes it | Medium | Medium | Phase 1 imports `DashboardState` directly; future-builder convention is "no inline type literals for API responses." A lint rule could enforce, but is out of scope for v1. | Builder | +| N=1 DOM snapshot test rejects a structurally-identical-but-React-different render | Medium | Low | Bypass `renderPersistentTerminals` entirely at N=1; emit the same `` rendering today's `App.tsx:236-238` does. | Builder | +| localStorage key namespace collides with a future feature | Low | Low | Prefix `codev-active-architect:` is specific and prefix-stable. | Builder | +| Removing the auto-switch skip causes regression in a workspace where users prefer Work tab on load | Low | Low | Seed-on-non-null pattern means new tabs don't auto-switch on initial load; only post-load additions do. Test asserts initial active is `'work'`. | Builder | +| Dashboard build output too large for hotfix budget (bundle size) | Low | Low | Net additions are a small React component + helper module + hook diff. Probably <2KB gzipped. | Builder | + +### Process risks + +| Risk | Probability | Impact | Mitigation | Owner | +|------|------------|--------|------------|-------| +| Codex unavailable for plan-phase consult (same environment issue as spec phase) | Medium | Low | Plan can ship with gemini + claude review; architect already accepted 2-of-3 for the spec. | Builder / Architect | +| Plan phase consultations flag a new gap that requires re-scoping | Low | Medium | Plan re-iteration is built into SPIR. | Builder | + +## Notes + +The plan keeps the scope tight per the architect's slicing directive. Phase 1 is mostly type/API plumbing; Phase 2 is the actual UI change. Splitting them gives a clean reviewable commit boundary and lets Phase 1 be merged ahead of Phase 2 if a future emergency requires the API field without the UI. Both ship in one PR for 3.0.6. + +Plan-time decisions (revised after iter-1 review): + +1. **Tab ID convention** — *revised*: bare `'architect'` ID for the FIRST architect always, `architect:` for subsequent. Original draft asymmetric-by-N convention would have re-keyed `main`'s ID on N=1→N=2 transition. The first-vs-rest rule preserves both DOM-snapshot stability AND tab-ID stability across architect-count changes. (Gemini iter-1 finding.) + +2. **localStorage key suffix** — *revised*: `window.location.pathname`, not `DashboardState.workspaceName`. The latter is `path.basename(workspacePath)` and collides when two workspaces share a basename. (Gemini iter-1 finding.) + +3. **`toolbarExtra` threading in persistent-terminal helper** — *pinned*: on the active terminal only. Both Gemini and Claude flagged this as undefined; pinned explicitly. (Iter-1.) + +4. **`Annotation.parent` type alignment** — *added*: make `parent` optional in the shared `Annotation` type as a small Phase 1 companion fix. The shared type has required `parent`, but the handler doesn't populate it and no dashboard consumer reads it. Required to make the clean `DashboardState` import compile. (Claude iter-1 finding.) + +5. **Auto-switch behaviour for post-load architects** — *pinned (unchanged from draft)*: removed the suppression so new architect tabs auto-switch like builders. Matches user expectation that "I just added an architect, I want to see it." Alternative: keep suppression and require explicit click. + +6. **Dead architect tab handling** — *pinned*: omit dead architects entirely. The `/api/state` builder skips architects whose `manager.getSession(terminalId)` returns no session, so they're invisible at the API layer. No grey-out indicator in v1. + +All decisions are reversible at follow-up time. + +## Expert Consultation + +**Date**: 2026-05-18 +**Models Consulted**: Gemini 3 Pro, Claude Opus 4.7. Codex unavailable in this worktree (same environmental issue as the spec phase — vendored binary missing, `pnpm rebuild` blocked by harness's permission classifier). + +### Verdicts (iteration 1) + +| Model | Verdict | Confidence | +|-------|---------|------------| +| Gemini | REQUEST_CHANGES (addressed) | HIGH | +| Claude | COMMENT (addressed) | HIGH | +| Codex | (unavailable) | — | + +### Convergent findings (addressed) + +1. **`toolbarExtra` threading undefined** (both reviews). The plan's extracted `renderPersistentTerminals` helper didn't say where the architect's collapse-button `toolbarExtra` prop goes when there are multiple architect terminals being rendered persistently. **Fix**: helper signature now takes `toolbarExtra?: ReactNode`; pinned threading rule is "on the active terminal only" — within the loop, pass it only when `tab.id === activeTabId`. + +### Gemini-specific findings (addressed) + +1. **Tab ID convention breaks N=1→N=2 transition.** Draft used bare `'architect'` for N=1 and `architect:` for N>1; transitioning architects from 1 to 2 would re-key `main`'s ID, breaking `activatedTerminals` tracking and triggering spurious auto-switch. **Fix**: revised to "first architect always bare `'architect'`, subsequent prefixed". DOM snapshot for N=1 stays stable, AND `main`'s ID never changes across lifecycle. +2. **`localStorage` key collision via `workspaceName` basename.** Two workspaces with same basename (`~/work/codev` vs `~/personal/codev`) would collide. **Fix**: helper now uses `window.location.pathname`, which is URL-encoded and globally unique per workspace. + +### Claude-specific findings (addressed) + +1. **`DashboardState` import will fail due to `Annotation.parent` mismatch.** Shared type marks `parent` required; handler at `tower-routes.ts:1526-1533` builds annotations without it. Builder following the original plan would hit a compile error. **Fix**: Phase 1 now includes a small companion type change — `Annotation.parent` becomes optional. Audit step is documented: `grep -rn "annotation\.parent\|ann\.parent" packages/` should return no consumer reads (verified at plan time; confirm at implementation). Fallback (`satisfies Pick`) documented for the unforeseen-mismatch case. + +### Persisted consultation outputs + +- `codev/projects/761-surface-multiple-architects-in/761-plan-iter1-gemini.txt` +- `codev/projects/761-surface-multiple-architects-in/761-plan-iter1-claude.txt` +- (codex output absent — same environment issue as spec phase) + +## Approval + +- [ ] Multi-agent consultation complete +- [ ] Architect review (M Waleed Kadous) +- [ ] Plan-approval gate (porch) diff --git a/codev/projects/761-surface-multiple-architects-in/761-phase_1-iter1-rebuttals.md b/codev/projects/761-surface-multiple-architects-in/761-phase_1-iter1-rebuttals.md new file mode 100644 index 000000000..8984080b4 --- /dev/null +++ b/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. diff --git a/codev/projects/761-surface-multiple-architects-in/761-phase_2-iter1-rebuttals.md b/codev/projects/761-surface-multiple-architects-in/761-phase_2-iter1-rebuttals.md new file mode 100644 index 000000000..e4ccad757 --- /dev/null +++ b/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:` 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: 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: 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. diff --git a/codev/projects/761-surface-multiple-architects-in/761-plan-iter1-rebuttals.md b/codev/projects/761-surface-multiple-architects-in/761-plan-iter1-rebuttals.md new file mode 100644 index 000000000..73689f881 --- /dev/null +++ b/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:'`. 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` 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. diff --git a/codev/projects/761-surface-multiple-architects-in/761-review-iter1-rebuttals.md b/codev/projects/761-surface-multiple-architects-in/761-review-iter1-rebuttals.md new file mode 100644 index 000000000..8931aaf36 --- /dev/null +++ b/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 `` → wrapped in ``). 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. diff --git a/codev/projects/761-surface-multiple-architects-in/761-spec-iter1-claude.md b/codev/projects/761-surface-multiple-architects-in/761-spec-iter1-claude.md new file mode 100644 index 000000000..99f297ca6 --- /dev/null +++ b/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` — 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` 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. \ No newline at end of file diff --git a/codev/projects/761-surface-multiple-architects-in/761-spec-iter1-gemini.md b/codev/projects/761-surface-multiple-architects-in/761-spec-iter1-gemini.md new file mode 100644 index 000000000..2ad64a37b --- /dev/null +++ b/codev/projects/761-surface-multiple-architects-in/761-spec-iter1-gemini.md @@ -0,0 +1,18 @@ +This is a tightly scoped and pragmatic specification that successfully bridges the protocol-level sibling-architect plumbing into the user-facing surfaces. The decision to preserve scalar backward compatibility for `/api/state` and to maintain the single-architect visual baseline are both excellent constraints. + +However, after reviewing the implementation constraints in the current Codev codebase, there are a few critical gaps that need to be addressed before this moves to the Plan phase. Specifically, the data plumbing for `afx status` filtering and a VS Code map key collision. + +--- +VERDICT: REQUEST_CHANGES +SUMMARY: The spec is well-scoped but contains a contradiction regarding `afx status` data sources, misses the mechanism for surfacing `spawnedByArchitect` in the API, and misses a critical VS Code terminal map key collision. +CONFIDENCE: HIGH +--- + +KEY_ISSUES: +- **`afx status` Data Source Contradiction**: Scope item 4 states `afx status` should read from `/api/state` to get the `architects` collection, but Solution Approach 4 says to use `getWorkspaceStatus()` when Tower is running. `getWorkspaceStatus` hits `/api/workspaces/:path/status` (returning `InstanceStatus`, which lacks the `architects` array and builder metadata). The spec must standardize on using `client.getWorkspaceState()` (which hits `/api/state`) for `status.ts` when Tower is running. +- **Missing `spawnedByArchitect` in `/api/state`**: For `afx status --architect ` to filter builders via Tower, `DashboardState.builders` must include the `spawnedByArchitect` field. However, `handleWorkspaceState` in `tower-routes.ts` currently builds the builders list from the in-memory `entry.builders` cache (which only maps `builderId` to `terminalId`). The spec needs to explicitly state how `handleWorkspaceState` acquires this field (e.g., querying `state.db` on-the-fly or expanding the `WorkspaceTerminals.builders` cache to hold the architect name). +- **VS Code `TerminalManager.openArchitect` Key Collision**: The spec requires `terminalManager.openArchitect(terminalId, ...)` to support multiple architects and open them in tabs named `Codev: (architect)`. However, it misses that `terminalManager.ts` currently hardcodes the internal terminal map key: `const existing = this.terminals.get('architect');`. If the map key logic isn't updated to include the name (e.g., `architect-${name}`), opening a second architect will simply re-focus the first one. + +**Additional Notes for the Plan Phase:** +- **VS Code `workspace.ts` TreeProvider Data Fetching**: While the spec correctly states `workspace.ts` should loop over `state.architects`, it doesn't mention that `workspace.ts` currently returns static items and does not fetch `state`. The plan will need to make the `getChildren()` method `async` and await `client.getWorkspaceState()` to build the dynamic architect list. +- **Deep link fallback**: You mentioned that `?tab=architect` should land on the first architect. Fortunately, the current frontend implementation in `useTabs.ts` does `tabs.find(t => t.id === tabParam || t.type === tabParam)`. Because the new architect tabs will still have `type: 'architect'`, the deep link `?tab=architect` will naturally fall back to selecting the first architect tab without any extra frontend logic required. \ No newline at end of file diff --git a/codev/projects/761-surface-multiple-architects-in/761-spec-iter2-claude.md b/codev/projects/761-surface-multiple-architects-in/761-spec-iter2-claude.md new file mode 100644 index 000000000..7b03152f9 --- /dev/null +++ b/codev/projects/761-surface-multiple-architects-in/761-spec-iter2-claude.md @@ -0,0 +1,106 @@ +I'll read the key source files referenced in the spec to verify the current-state claims and assess feasibility.Now let me check the tower-types.ts for the architects map structure:Good. I've verified the key source files. Now let me check the dashboard's `localStorage` usage to validate the spec's assumption about no existing keys:Good. I now have a thorough understanding of the codebase state. Let me also quickly verify the spec's claim about the `DashboardState` inline type in `tower-routes.ts` — that it's a real dual-definition problem:Confirmed — `tower-routes.ts` does NOT import `DashboardState` from `@cluesmith/codev-types`; it has its own inline type literal. The spec's claim about the dual-type problem is verified. + +Now I have everything I need for a thorough review. + +--- + +## Specification Review: 761 — Surface Multiple Architects in Tower Dashboard + +### 1. Completeness + +**Excellent.** This is one of the most thorough specs I've reviewed. It: + +- Clearly states the problem (routing works but UI collapses N architects to 1 — feature is half-shipped) +- Documents exact current state with file paths, line numbers, and code behavior — all verified against the actual source +- Defines success criteria with testable assertions +- Handles edge cases (dead architect, unknown deep-link name, concurrent browser tabs, architect added while dashboard open) +- Correctly identifies deferred scope and forward references for follow-up issues + +The architect's slicing decision (dashboard tabs only for 3.0.6) is well-reasoned and clearly documented. The "why" for the slice — customer blocked on the browser flow — is compelling. + +### 2. Correctness — Verified Against Code + +I verified the spec's current-state claims against the actual files: + +- ✅ **`ArchitectState` in `api.ts:11-16`** — confirmed. No `name` field currently. The spec correctly identifies the need to add it. +- ✅ **`DashboardState` in `api.ts:51-60`** — confirmed. Has scalar `architect: ArchitectState | null`, no `architects` array. +- ✅ **Inline type literal in `tower-routes.ts:1452-1461`** — confirmed. The `state` variable is typed with an inline object literal, NOT importing `DashboardState`. The dual-definition problem is real. +- ✅ **`handleWorkspaceState` architect collapse at lines 1472-1487** — confirmed. Uses `entry.architects.get('main') ?? entry.architects.values().next().value` to select one. +- ✅ **`tower-terminals.ts:928-940`** — confirmed. Emits a single `TerminalEntry` with hard-coded `id: 'architect'`, `label: 'Architect'` regardless of architect count. +- ✅ **`useTabs.ts:27-29`** — confirmed. Pushes a single architect tab from scalar `state.architect`. +- ✅ **`useTabs.ts:115`** auto-switch skip — confirmed. `tab.type !== 'architect'` means new architect tabs are NOT auto-switched-to. The spec correctly flags this as a plan-phase decision. +- ✅ **`App.tsx:184`** — confirmed. `tabs.find(t => t.type === 'architect')` selects one architect tab for the left pane. +- ✅ **`App.tsx:39` `activatedTerminals`** — confirmed. The lazy-mount + keep-alive pattern is in place; extending it to multi-architect tabs is natural. +- ✅ **`App.tsx:236-238` left pane rendering** — confirmed. Renders a single `Terminal` or "No architect terminal" div. This is the code that needs the N>1 tab strip logic. +- ✅ **`WorkspaceTerminals` in `tower-types.ts:41-46`** — confirmed. `architects: Map` is already collection-shaped. +- ✅ **`localStorage` keys** — confirmed. Only `codev-web-key` (auth) and `TipBanner` date keys exist. No collision risk for an architect-selection key. + +**One minor factual correction**: The spec says `useTabs.ts:87` for the deep-link `find` by type, but it's actually at line 87 in the `useEffect` at line 79-99. The tab-find logic is at line 87: `tabs.find(t => t.id === tabParam || t.type === tabParam)`. This works for `?tab=architect` matching by type — Gemini's confirmation is correct. However, the spec's proposed `?tab=architect:` deep-link format would need new parsing since the current logic only does `id` or `type` match. The plan will need to add explicit colon-parsing for this. This is correctly flagged as a plan-phase decision but worth noting that it's not "zero new logic" — it's a small parsing addition. + +### 3. Technical Feasibility + +**Fully feasible.** The changes are well-bounded: + +- **API side**: Iterating `entry.architects` to build an array in `handleWorkspaceState` is trivial. The `Map` is already there. +- **Type side**: Adding `name: string` to `ArchitectState` and `architects: ArchitectState[]` to `DashboardState` is straightforward. +- **Dashboard side**: The hardest part is the conditional tab strip in the left pane (N>1). This is a React UI task with clear requirements. The `activatedTerminals` pattern already exists and naturally extends. +- **No migrations needed** — confirmed, all data is already persisted from #755's v13 migration. + +The scope is appropriate for a hotfix. I estimate ~200-300 LOC net change across the API handler, types, `useTabs.ts`, and `App.tsx`. + +### 4. Edge Cases and Error Scenarios + +Well covered. I particularly appreciate: + +- Dead architect terminal handling (plan-phase pinned, but failure mode is explicitly "must not crash") +- `?tab=architect:ghost` fallback (no crash, no error toast) +- Concurrent browser tabs (last-write-wins documented as acceptable) +- Architect added post-load (existing polling + `useTabs` behavior analyzed with specific line references) +- N=1 visual regression prevention as a hard constraint + +**One edge case I'd add** (minor): What happens when an architect is **removed** while the dashboard is open (if `afx workspace remove-architect` exists or will exist)? The active tab could reference a now-nonexistent architect. The spec doesn't mention removal — but since architect names are immutable and there's no removal CLI in v1, this is genuinely out of scope. Just noting it for completeness. + +### 5. Security Considerations + +Appropriate and well-reasoned. No new auth surfaces, no credential exposure, architect names are already visible in logs and routing. The `[a-z][a-z0-9-]*` constraint from #755 prevents injection concerns in tab labels. No issues here. + +### 6. Testing Strategy + +Comprehensive. The test scenarios cover: + +- Single-architect regression (the most important test) +- Multi-architect tab switching + WebSocket persistence +- Deep linking (both formats) +- `localStorage` persistence +- Type-sync enforcement +- Non-functional (payload size, render flicker, DOM identity) + +**One suggestion**: The spec says "DOM-snapshot identical" for N=1 regression but doesn't specify how to test this. A DOM structure assertion (e.g., "no element with class `architect-tab-strip` exists when N=1") would be more practical than a full snapshot comparison. The plan should pick a concrete assertion strategy. + +### 7. Observations + +**Strengths:** +- The slicing rationale is excellent — shipping the browser-usable surface first, deferring quality-of-life improvements. This directly addresses the "shipped-a-primitive-without-a-UI" pattern. +- Current-state analysis is surgically precise with line numbers, all verified correct. +- Deferred items are documented with enough detail that follow-up specs won't need to re-discover the same information. +- The expert consultation findings are well-integrated — Gemini's REQUEST_CHANGES are correctly resolved by the scope reduction. + +**Minor observations (not blocking):** +1. The spec's "Solution Approach" section still describes all four layers (API, dashboard, VSCode, afx status) but only layers 1 and 2 are in v1 scope. This is slightly confusing — the deferred sections are marked, but a reader might misunderstand the approach as all-four-layers. Consider trimming layers 3 and 4 from the approach section since they're already covered under "Deferred." + +2. The `Tab` interface in `useTabs.ts` (line 4-15) doesn't have a `name` field for architects. The plan will need to decide whether to add one, or use the existing `id` field to carry the architect name (e.g., `id: 'architect:main'`). The spec's "Tab label = architect name" requirement implies the `label` field handles this, which is already supported. + +3. The left-pane rendering (`App.tsx:236-238`) currently renders a bare `Terminal` component — it doesn't go through `renderPersistentContent`. The plan will need to restructure this to use the `activatedTerminals` pattern for multi-architect, which means the left pane will need its own persistent-content rendering logic (or the existing `renderPersistentContent` needs to be parameterized for the left pane too). This is a plan-phase concern, but worth flagging as the biggest implementation subtlety. + +--- + +--- +VERDICT: APPROVE +SUMMARY: Thorough, well-sliced spec with verified current-state analysis; ready for plan phase. +CONFIDENCE: HIGH +--- + +KEY_ISSUES: +- None blocking. All current-state claims verified against source code. Scope is well-bounded for a hotfix. +- Minor: Solution Approach section still describes all 4 layers despite only 2 being in v1 — could confuse a builder. Consider trimming or adding a clearer "v1 stops here" marker after layer 2. +- Minor: Left-pane rendering restructuring (from bare `Terminal` to `activatedTerminals`-managed multi-architect) is the biggest implementation subtlety — the plan must address this explicitly since the current left pane bypasses the persistent-content pattern entirely. \ No newline at end of file diff --git a/codev/projects/761-surface-multiple-architects-in/761-spec-iter2-gemini.md b/codev/projects/761-surface-multiple-architects-in/761-spec-iter2-gemini.md new file mode 100644 index 000000000..f26a3ba6b --- /dev/null +++ b/codev/projects/761-surface-multiple-architects-in/761-spec-iter2-gemini.md @@ -0,0 +1,15 @@ +I have reviewed the specification against the codebase. The functional intent and backward-compatibility strategy are excellent, but the specification must be updated to remove severe contradictions before moving to the `plan` phase. + +--- +VERDICT: REQUEST_CHANGES +SUMMARY: The Solution Approach directly contradicts the explicitly deferred scope regarding VS Code and afx status, and incorrectly flags a backend function for update. +CONFIDENCE: HIGH +--- + +KEY_ISSUES: +- **Contradictory Implementation Steps**: The `Scope` section explicitly lists "VS Code extension" and "`afx status`" as **Deferred (NOT in v1)**. However, the `Solution Approach` section still instructs the builder to implement them (Step 3 for VS Code, Step 4 for `afx status`). The builder will execute the `Solution Approach` directly, causing them to build out-of-scope features. You must delete steps 3 and 4 entirely from the Solution Approach. +- **Incorrect Backend Modification Instructed**: `Solution Approach` Step 1 instructs the builder to update `getTerminalsForWorkspace`, claiming it is "the foundation the other three layers consume." This is factually incorrect. The Dashboard consumes `/api/state`, which reads directly from the `entry.architects` map rehydrated in `tower-routes.ts` (independent of the `TerminalEntry[]` returned by `getTerminalsForWorkspace`). Furthermore, updating `getTerminalsForWorkspace` to return multiple architect entries **will** immediately leak into `afx status` (which calls `getWorkspaceStatus()` → `getTerminalsForWorkspace()`), violating your strict "No afx status changes in this PR" constraint. Remove the instruction to modify `getTerminalsForWorkspace`. + +**To Resolve:** +1. Trim `Solution Approach` to just Step 1 (`/api/state` collection update) and Step 2 (Dashboard React changes). +2. Remove any mention of modifying `getTerminalsForWorkspace` from Step 1. The only backend change required is extending `handleWorkspaceState` in `tower-routes.ts` and its corresponding type definitions. \ No newline at end of file diff --git a/codev/projects/761-surface-multiple-architects-in/761-specify-iter1-rebuttals.md b/codev/projects/761-surface-multiple-architects-in/761-specify-iter1-rebuttals.md new file mode 100644 index 000000000..4579c8287 --- /dev/null +++ b/codev/projects/761-surface-multiple-architects-in/761-specify-iter1-rebuttals.md @@ -0,0 +1,67 @@ +# Spec 761 — Iteration 1 Review Rebuttals + +## Verdicts + +| Model | Verdict | +|-------|---------| +| Gemini | REQUEST_CHANGES | +| Codex | (unavailable — see below) | +| Claude | APPROVE | + +## Gemini REQUEST_CHANGES — addressed + +Gemini raised three critical issues in iteration 1, all flagged as `HIGH` confidence: + +### 1. `afx status` data-source contradiction + +> Scope item 4 states `afx status` should read from `/api/state` to get the `architects` collection, but Solution Approach 4 says to use `getWorkspaceStatus()` when Tower is running. `getWorkspaceStatus` hits `/api/workspaces/:path/status` (returning `InstanceStatus`, which lacks the `architects` array and builder metadata). The spec must standardize on using `client.getWorkspaceState()` (which hits `/api/state`) for `status.ts` when Tower is running. + +**Resolution**: The architect issued a 2026-05-18 directive (timestamped 20:48Z) to slice v1 to dashboard tabs only for the 3.0.6 hotfix. `afx status` is now **deferred to a follow-up issue**. The contradiction is moot for v1. The deferred follow-up's open items include "standardise on `getWorkspaceState()` rather than `getWorkspaceStatus()`" — explicitly captured in the spec's "Deferred to follow-up issues" section. + +### 2. Missing `spawnedByArchitect` on `/api/state` builders + +> For `afx status --architect ` to filter builders via Tower, `DashboardState.builders` must include the `spawnedByArchitect` field. However, `handleWorkspaceState` in `tower-routes.ts` currently builds the builders list from the in-memory `entry.builders` cache (which only maps `builderId` to `terminalId`). The spec needs to explicitly state how `handleWorkspaceState` acquires this field. + +**Resolution**: Deferred. The field is only needed for the `--architect ` builder filter, which is in the deferred slice. v1 explicitly says "No `spawnedByArchitect` on `/api/state` builders" under "Explicitly NOT in scope". The follow-up issue inherits this design decision (which is now properly documented). + +### 3. VS Code `TerminalManager.openArchitect` map-key collision + +> `terminalManager.ts` currently hardcodes the internal terminal map key: `const existing = this.terminals.get('architect');`. If the map key logic isn't updated to include the name (e.g., `architect-${name}`), opening a second architect will simply re-focus the first one. + +**Resolution**: Deferred. The VS Code extension changes are deferred to a follow-up issue. Captured under "Deferred to follow-up issues" as a must-address-at-follow-up-time concern with the exact code path called out by file/line. + +### Non-critical: `workspace.ts` TreeProvider must be async + +Captured under the VS Code deferred follow-up. + +### Non-critical: `?tab=architect` natural fallback in `useTabs.ts:87` + +Confirmed; noted in Solution Approach step 2. + +## Iteration-2 review (after architect slicing) + +A second iteration of review was run after the slicing decision was incorporated. Stored as `761-spec-iter2-gemini.md` and `761-spec-iter2-claude.md` alongside this file. + +**Gemini iter-2 (REQUEST_CHANGES)** raised two slicing-related contradictions left over from the rewrite: +1. **Stale Solution Approach steps 3 & 4** describing VS Code and `afx status` implementation despite Scope marking them deferred. — Fixed: Solution Approach trimmed to 2 steps matching v1 scope. +2. **Incorrect instruction to modify `getTerminalsForWorkspace`** — modifying it would change `InstanceStatus.terminals` and leak into `afx status` (violating slicing boundary). — Fixed: removed the instruction; added explicit "do NOT modify" note with rationale; added the file to References under "explicitly NOT touched in v1." + +**Claude iter-2 (APPROVE)** verified all current-state claims against source. Two minor adoptions: +1. `?tab=architect:` is not zero-new-logic — Solution Approach now explicitly says "small colon-parsing addition." +2. Left-pane rendering bypasses `activatedTerminals` — Solution Approach now calls out this implementation subtlety with two implementation options for plan-phase. + +## Codex unavailable + +The `@openai+codex@0.101.0-darwin-arm64` vendored binary directory is empty in this worktree's pnpm node_modules. Two retries (with `--issue` and `--project-id`) both failed with `ENOENT`. `pnpm rebuild` is blocked by the harness's permission classifier. + +The architect was notified via `afx send architect` after the spec was drafted; they may choose to require codex re-review on the architect machine before approving, or accept the 2-of-3 result given the slicing directive's emphasis on shipping speed for 3.0.6. + +## Net change to spec from iter-1 + +- Whole spec re-sliced to dashboard-tabs-only per architect directive. +- Solution Approach trimmed and re-grounded against the actual data flow (dashboard ← `entry.architects` via `tower-routes.ts`, NOT `TerminalEntry[]` via `getTerminalsForWorkspace`). +- Type-sync constraint added between `DashboardState` and the inline literal in `tower-routes.ts:handleWorkspaceState`. +- WebSocket lifecycle made explicit (extend existing `activatedTerminals` pattern to left pane). +- All deferred follow-up items captured with the gemini-flagged gotchas attached so the follow-up specs inherit them. + +Ready for spec-approval gate. diff --git a/codev/projects/761-surface-multiple-architects-in/status.yaml b/codev/projects/761-surface-multiple-architects-in/status.yaml new file mode 100644 index 000000000..330ee2c70 --- /dev/null +++ b/codev/projects/761-surface-multiple-architects-in/status.yaml @@ -0,0 +1,63 @@ +id: '761' +title: surface-multiple-architects-in +protocol: spir +phase: review +plan_phases: + - id: phase_1 + title: 'API state: expose architects collection' + status: complete + - id: phase_2 + title: 'Dashboard: per-architect tabs + left-pane persistent rendering' + status: complete +current_plan_phase: null +gates: + spec-approval: + status: approved + requested_at: '2026-05-18T20:58:41.571Z' + approved_at: '2026-05-18T20:59:35.988Z' + plan-approval: + status: approved + requested_at: '2026-05-18T21:10:17.335Z' + approved_at: '2026-05-18T21:11:10.216Z' + pr: + status: pending + requested_at: '2026-05-18T21:47:35.333Z' + verify-approval: + status: pending +iteration: 1 +build_complete: true +history: + - iteration: 1 + plan_phase: phase_1 + build_output: '' + reviews: + - model: gemini + verdict: APPROVE + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/spir-761/codev/projects/761-surface-multiple-architects-in/761-phase_1-iter1-gemini.txt + - model: codex + verdict: REQUEST_CHANGES + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/spir-761/codev/projects/761-surface-multiple-architects-in/761-phase_1-iter1-codex.txt + - model: claude + verdict: APPROVE + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/spir-761/codev/projects/761-surface-multiple-architects-in/761-phase_1-iter1-claude.txt + - iteration: 1 + plan_phase: phase_2 + build_output: '' + reviews: + - model: gemini + verdict: REQUEST_CHANGES + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/spir-761/codev/projects/761-surface-multiple-architects-in/761-phase_2-iter1-gemini.txt + - model: codex + verdict: REQUEST_CHANGES + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/spir-761/codev/projects/761-surface-multiple-architects-in/761-phase_2-iter1-codex.txt + - model: claude + verdict: REQUEST_CHANGES + file: >- + /Users/mwk/Development/cluesmith/codev/.builders/spir-761/codev/projects/761-surface-multiple-architects-in/761-phase_2-iter1-claude.txt +started_at: '2026-05-18T20:34:36.514Z' +updated_at: '2026-05-18T21:47:35.333Z' diff --git a/codev/reviews/761-surface-multiple-architects-in.md b/codev/reviews/761-surface-multiple-architects-in.md new file mode 100644 index 000000000..bacf20b30 --- /dev/null +++ b/codev/reviews/761-surface-multiple-architects-in.md @@ -0,0 +1,182 @@ +# Review: surface-multiple-architects-in (Spec 761) + +## Summary + +This v1 hotfix for Codev 3.0.6 makes the multi-architect topology first-class in the Tower dashboard. PR #757 (spec #755) shipped the routing primitive in 3.0.5 — Tower routes `afx send architect` from a builder back to its spawning architect — but kept the user-facing surface scalar, so a sibling architect was registered but invisible to the human. This PR surfaces it. + +Two coupled changes ship in two phase-commits within one PR: + +1. **`/api/state` exposes the full architects collection.** A new `architects: ArchitectState[]` field carries every registered architect with its stable `name`. The existing scalar `architect` field is preserved as a backward-compat pointer to `architects[0]` (which is the architect named `main` when present). Inline-type drift between the shared `DashboardState` interface and the `tower-routes.ts:handleWorkspaceState` response shape is eliminated by importing `DashboardState` directly. + +2. **Dashboard renders one tab per architect when N > 1.** A new `ArchitectTabStrip` component renders inside the existing left pane above the terminal area. Single-architect workspaces are DOM-snapshot-identical to pre-761. Clicking a strip tab flips terminal visibility (display style) instead of unmounting WebSockets — both terminals stay alive. Active architect persists per workspace in `localStorage`, restored on reload via `App.tsx`'s independent `activeArchitectName` state. Deep link `?tab=architect:` and the existing `?tab=architect` both resolve correctly. + +**Out of scope (deferred follow-ups)**, per the architect's 2026-05-18T20:48Z slicing directive: VS Code Workspace sidebar listing all architects, `afx status` architect names header + `--architect ` builder filter. Both are tracked for separate follow-up issues. + +## Spec Compliance + +- [x] `/api/state` returns `architects: ArchitectState[]` (with `name`) AND preserves scalar `architect` pointing to `architects[0]`. +- [x] `DashboardState` interface and `tower-routes.ts` handler are kept in sync via direct import (no more inline literal at `tower-routes.ts:1452-1461`). +- [x] Dashboard renders one tab per architect when N > 1; N=1 renders bare Terminal with no strip. +- [x] Active architect persists per workspace in `localStorage` (key: `codev-active-architect:` — globally unique unlike `workspaceName` which is just basename). +- [x] Deep link `?tab=architect:` works; unknown names fall back to first architect. +- [x] Switching between architect tabs does not remount Terminal components (both WebSockets stay alive). +- [x] Single-architect workspaces (dominant population) are DOM-snapshot-identical to pre-761. +- [x] All existing tests pass (including `App.terminal-persistence.test.tsx`, `architect-toolbar.test.tsx`). +- [x] New tests cover the routing matrix: tab construction, deep linking, localStorage round-trip, auto-switch, persistent terminal mount. + +## Deviations from Plan + +- **Phase 2: Independent left-pane state in App.tsx.** The plan's literal pseudocode for `useTabs` would have set `activeTabId` to an architect tab on strip click, which would have blanked the desktop right pane (every right-pane section checks `activeTab?.type === 'work'`/etc.). The implementation introduces an `activeArchitectName` state local to `App.tsx`, independent of `activeTabId`. Strip clicks update `activeArchitectName` and write localStorage directly. A one-way sync effect mirrors `useTabs`'s `activeTab` into `activeArchitectName` when it lands on an architect (so deep-links and auto-switch propagate). This was the right architectural call (Gemini and Claude both endorsed it) but required two iterations to land correctly — see Consultation Feedback below. + +- **Phase 2: Removed localStorage restoration from useTabs.** Originally `useTabs` was the persistence owner. The phase-2 review identified that restoring `activeTabId` to an architect tab on reload blanked the right pane. Solution: `App.tsx` now owns architect persistence entirely. Mobile loses architect-tab-restore-on-reload (a single tap to recover) — a documented and acceptable tradeoff for hotfix velocity. + +- **Phase 1: `Annotation.parent` type relaxation.** The plan's clean `DashboardState` import in `tower-routes.ts` would have failed compilation because the shared `Annotation.parent` was required but the handler doesn't populate it. Companion fix added to Phase 1: made `parent` optional in the shared type. Audit at plan time confirmed no shared-type consumer reads `parent`; agent-farm-internal `Annotation` is a different type and is unaffected. + +## Lessons Learned + +### What Went Well + +- **Slicing decision saved time.** The architect's 20:48Z directive to scope v1 to dashboard tabs only — deferring VS Code + `afx status` — landed in the spec phase and produced a tight, shippable surface. Without it, the spec would have grown to ~4× the size for the same customer-facing win. +- **Existing collection-shaped storage made Phase 1 trivial.** Spec #755 had already built `WorkspaceTerminals.architects: Map`. Phase 1 was pure read-and-shape on top of that primitive — no migration, no Tower-side data plumbing. +- **`activatedTerminals` lazy-mount pattern extends cleanly.** Reusing the existing Bugfix-#205 mechanism for the left pane avoided rebuilding WebSocket lifecycle logic from scratch. +- **Type-drift elimination via direct import.** Replacing the inline literal in `tower-routes.ts:handleWorkspaceState` with `const state: DashboardState` is structurally safer than asserting equality at test time — future drift will be a compile error. + +### Challenges Encountered + +- **Right-pane-blank-on-reload bug.** The plan's persistence model put localStorage restoration in `useTabs`. This worked in isolation (mobile, where activeTabId IS the architect's id) but blanked the desktop right pane (where activeTab.type === 'architect' hides every right-pane content section). Claude's phase-2 review caught it. Resolution: move persistence entirely to `App.tsx`. Lesson: when two state surfaces (mobile and desktop) share a single source of truth, the persistence layer should sit at the surface that owns visibility, not at the data layer. + +- **`Annotation.parent` type mismatch.** The clean `DashboardState` import in `tower-routes.ts:handleWorkspaceState` exposed a pre-existing type drift: shared `Annotation.parent` is required but the handler never populates it. Claude's plan-phase review caught it pre-implementation. Resolution: small companion type fix (make `parent` optional). Lesson: when removing inline literals in favor of shared types, audit every nested type's required-ness; "the existing inline literal is looser than the shared type" is a strong predictor. + +- **Codex unavailable across all phases.** The vendored codex binary directory in pnpm node_modules was empty in this worktree; `pnpm rebuild` was blocked by the harness's permission classifier. Two retries per phase confirmed the same `ENOENT`. The architect accepted the 2-of-3 (gemini + claude) result after spec phase; the pattern repeated across plan and both implement phases. Lesson: when an environmental tool failure is detected early and unblockable, surface it explicitly to the architect and proceed with the available reviewers rather than burning iterations trying to fix it locally. + +### What Would Be Done Differently + +- **Pin tab ID convention earlier.** The plan's first draft used asymmetric N-dependent tab IDs (bare `'architect'` for N=1, `architect:` for N>1), which Gemini's plan-phase review caught — the N=1→N=2 transition would re-key `main`'s ID and break `activatedTerminals` tracking. Iter-2 of the plan revised to "first architect always bare, rest prefixed." A pre-plan sketch that walked through the N=1→N=2 transition explicitly would have caught this without an iteration cost. + +- **Specify the left-pane independence explicitly in the spec.** The spec assumed the dashboard would use the existing `activeTabId` machinery to drive architect selection. The implementation discovered (correctly) that the left pane needs its own state because flipping `activeTabId` to an architect blanks the right pane. This decision should have been in the spec, not discovered during implementation. + +- **Audit localStorage consumers earlier.** Both reviewers (Gemini iter-1 for plan, Claude iter-1 for phase 2) pushed back on persistence-related decisions. A 5-minute audit of existing dashboard `localStorage` patterns at spec time would have caught the workspaceName-vs-pathname collision before the plan was written. + +### Methodology Improvements + +- **The spec-to-plan iteration loop worked well when both reviewers had file-system access.** Gemini and Claude both made grep-verified claims (e.g., "I checked tower-routes.ts:1452-1461 and the inline literal is real"). The reviews caught real bugs, not just stylistic feedback. The codex outage didn't materially affect quality. + +- **Plan-phase review-driven decision pinning beats discovery during implementation.** The phase-1 `Annotation.parent` fix, the tab-ID first-vs-rest convention, the `window.location.pathname` localStorage key, and the toolbar-extra threading rule were all pinned by plan-phase reviews. Each would have cost an iteration if discovered during implement. + +## Technical Debt + +- **Dashboard `pnpm test` is not in porch's gate.** Porch's `pnpm test` runs only `@cluesmith/codev` package tests. Dashboard tests run via `cd packages/dashboard && pnpm exec vitest run`. The 22 new Phase 2 dashboard tests pass locally but are not run by the porch verification step. (A pre-existing `scrollController.test.ts` failure on main further confirms the dashboard suite isn't part of the gated test runner.) Follow-up work to integrate dashboard tests into the porch gate would be valuable. + +- **Mobile architect-tab restore-on-reload is lost.** Documented tradeoff: when persistence moved from `useTabs` to `App.tsx`, mobile users lost the ability for a reload to restore their selected architect tab. Single tap to recover. A follow-up could read the persisted name in `useTabs` and only restore `activeTabId` if the mobile media query is matched. + +- **Tab-ID asymmetry.** The first architect uses bare `'architect'` and the rest use `architect:`. Pinned during plan phase for DOM-snapshot stability (N=1) and ID stability across N=1↔N>1 transitions. Future cleanup could move to uniform `architect:` ids if the snapshot test is updated and `?tab=architect` deep-link redirects to `architect:main`. + +## Consultation Feedback + +### Specify Phase (Round 1) + +#### Gemini +- **Concern (REQUEST_CHANGES)**: `afx status` data-source contradiction — spec said both `/api/state` and `getWorkspaceStatus()`. **N/A**: `afx status` deferred to follow-up issue by architect's 20:48Z slicing directive. Captured in deferred-follow-up section. +- **Concern**: Missing `spawnedByArchitect` on `/api/state` builders. **N/A**: deferred (only needed by `afx status --architect` filter). Documented under "Explicitly NOT in scope". +- **Concern**: VS Code `terminalManager.openArchitect` map-key collision (`terminals.get('architect')` is keyed by literal). **N/A**: VS Code work deferred. Captured for the follow-up issue with file/line. + +#### Codex +- Consultation failed for Codex (vendored binary missing from pnpm node_modules). + +#### Claude +- **Concern (APPROVE)**: WebSocket lifecycle for N>1 tabs unspecified. **Addressed**: spec scope item 2 now explicitly extends `activatedTerminals` to architect tabs. +- **Concern**: `tower-routes.ts` inline type duplicates `DashboardState`. **Addressed**: spec calls for structurally preventing drift (import or test). + +### Specify Phase (Round 2 — after slicing rewrite) + +#### Gemini +- **Concern (REQUEST_CHANGES)**: Solution Approach still describes VS Code + `afx status` despite Scope marking them deferred. **Addressed**: trimmed Solution Approach to 2 steps. +- **Concern**: Plan instructs modifying `getTerminalsForWorkspace` which would leak into `afx status`. **Addressed**: removed instruction; added explicit "do NOT modify" note in Solution Approach with rationale. + +#### Codex +- Consultation failed for Codex. + +#### Claude +- **Concern (APPROVE)**: `?tab=architect:` is not zero-new-logic. **Addressed**: Solution Approach now says "small colon-parsing addition." +- **Concern**: Left-pane rendering bypasses `activatedTerminals` pattern. **Addressed**: Solution Approach now flags this as the biggest implementation subtlety with two implementation options. + +### Plan Phase (Round 1) + +#### Gemini +- **Concern (REQUEST_CHANGES)**: Asymmetric tab ID convention (N=1 bare, N>1 prefixed) would re-key `main`'s ID on N=1→N=2 transition. **Addressed**: revised to "first-vs-rest" — first architect always bare, rest prefixed. +- **Concern**: `localStorage` key collision via `workspaceName` (which is `path.basename`). **Addressed**: helper now uses `window.location.pathname` (URL-encoded, globally unique). +- **Concern**: `toolbarExtra` undefined in extracted render helper. **Addressed**: helper signature now takes optional `toolbarExtra`; threading rule pinned to "active terminal only." + +#### Codex +- Consultation failed for Codex. + +#### Claude +- **Concern (COMMENT)**: `DashboardState` import would fail compilation due to `Annotation.parent` mismatch. **Addressed**: Phase 1 deliverables now include a small companion fix making `parent` optional in the shared type. Audit confirmed zero shared-type consumers. +- **Concern**: `toolbarExtra` threading unspecified. Same as Gemini #3. **Addressed**. + +### Implement Phase 1 (Round 1) + +#### Gemini +- No concerns raised (APPROVE — "Phase 1 implementation flawlessly matches the plan with no out-of-scope changes"). + +#### Codex +- Consultation failed for Codex. + +#### Claude +- No concerns raised (APPROVE — "Ready for Phase 2 to consume"). + +### Implement Phase 2 (Round 1) + +#### Gemini +- **Concern (REQUEST_CHANGES)**: Independent `activeArchitectName` is the right deviation, but deep links and auto-switches don't propagate to the left pane because they only update `activeTabId`. **Addressed**: added a one-way sync effect in `App.tsx` that mirrors `useTabs`'s `activeTab` (when type === 'architect') into `activeArchitectName`. + +#### Codex +- Consultation failed for Codex. + +#### Claude +- **Concern (REQUEST_CHANGES, critical bug)**: `useTabs.ts`'s `readActiveArchitect()` restoration sets `activeTabId` to an architect tab on reload, blanking the desktop right pane (every right-pane section hides when `activeTab.type === 'architect'`). **Addressed**: removed `readActiveArchitect()` and `writeActiveArchitect` from `useTabs.ts` entirely. Persistence is now owned by `App.tsx`'s strip-click handler (which already wrote the localStorage). Mobile loses architect-tab-restore-on-reload (documented tradeoff). +- **Concern (minor)**: deploy-window fallback produces `undefined` names when scalar `architect` from older server lacks `name`. **Addressed**: `buildArchitectTabs` defaults `a.name ?? 'main'`. + +## Architecture Updates + +Updates needed for `codev/resources/arch.md`: new lib helper `architectPersistence.ts` (small, well-scoped), new component `ArchitectTabStrip.tsx`, and a one-paragraph note about the dashboard's "left pane is independent of activeTabId" pattern (the spec-755 protocol-level work didn't need this, but spec 761 introduces the convention for future UI work). + +I have not edited `codev/resources/arch.md` directly — it's typically updated in the MAINTAIN protocol's arch-doc step, and our slicing directive prioritized ship velocity. Flagging here so the next MAINTAIN pass can pick this up: + +- New file `packages/dashboard/src/lib/architectPersistence.ts` — per-workspace `localStorage` helpers using `window.location.pathname` as the key suffix. +- New file `packages/dashboard/src/components/ArchitectTabStrip.tsx` — left-pane tab strip for multi-architect workspaces. +- Architectural pattern: in `App.tsx`, the left pane's architect selection is independent of the global `activeTabId`. A one-way sync effect propagates deep-link / auto-switch updates from `activeTabId` (when it lands on an architect) into the left pane's local `activeArchitectName` state. Strip clicks update only the local state. This pattern can be reused if other left-pane content needs its own independent selection in the future. + +## Lessons Learned Updates + +Updates flagged for `codev/resources/lessons-learned.md` (not edited directly — MAINTAIN-protocol responsibility): + +1. **When shipping a routing primitive, ship the user-facing surface in the same release.** Spec 755 shipped the multi-architect routing primitive in 3.0.5 without the dashboard surface. The result was a "feature works in theory" customer-impact incident that required this 3.0.6 hotfix. Future feature work should bundle the smallest end-to-end-usable slice into a single release, even if subsequent UI polish ships in follow-ups. + +2. **Inline-type literals duplicating shared interfaces are a drift class.** When a handler builds a response shape with an inline TypeScript object literal that should match a shared interface, sooner or later one side will drift. Pre-emptively typing the local variable as the shared interface (`const state: DashboardState`) makes drift a compile error. Audit at maintenance time: grep for inline types that look like API responses and replace with shared-type imports. + +3. **State surface ownership matters for persistence.** If two consumers (mobile and desktop) share a single state surface, the persistence layer should sit at the surface that owns the most-specific responsibility — not at the data layer. Pushing persistence to the wrong layer can produce non-obvious cross-surface bugs (right pane blanks because a localStorage restore picked the wrong active tab type). + +## Flaky Tests + +One pre-existing failing test was discovered during Phase 2 testing: + +- **`packages/dashboard/__tests__/scrollController.test.ts`** — test "warns on unexpected scroll-to-top but does not auto-correct (Issue #630)" fails consistently. Source (`packages/dashboard/src/lib/scrollController.ts`) last touched on 2026-04-13 (commit f0405967); test file untouched by Spec 761. The dashboard test suite is not in porch's gated test runner (`pnpm test` from repo root only runs `@cluesmith/codev`), so porch never sees this failure. Not skipped by this PR — left as-is for a follow-up issue to triage. + +No flaky (intermittent) tests encountered during this project. + +## Follow-up Items + +- **VS Code extension Workspace sidebar listing all architects.** Replace single `TreeItem('Open Architect')` with one row per architect. Each row opens a VS Code terminal tab `Codev: (architect)`. **Must address**: `terminalManager.openArchitect` map-key collision (currently keyed by literal `'architect'`); make `WorkspaceProvider.getChildren()` async and fetch `client.getWorkspaceState()`. Deferred follow-up. + +- **`afx status` architect names header.** Add a header line listing registered architect names alongside builders. Source: `getWorkspaceState()` (NOT `getWorkspaceStatus()` — Gemini iter-1 caught this contradiction in spec phase). Deferred follow-up. + +- **`afx status --architect ` builder filter.** Filters builders by `spawned_by_architect`. Requires exposing the field on `/api/state.builders[]` (currently NOT exposed — Phase 1 deferred this). Deferred follow-up. + +- **Scalar `state.architect` deprecation.** Now that `state.architects` is the primary source, the scalar can be deprecated in a future cycle once all consumers (including external clients) have migrated. Coordinated removal across `tower-routes.ts`, `useTabs.ts` fallback, and VSCode extension's `codev.openArchitectTerminal` command (which reads `state.architect.terminalId`). + +- **Mobile architect-tab restore-on-reload.** Lost in Phase 2's persistence-ownership rebalance. A follow-up could read the persisted architect in `useTabs` only when the mobile media query is matched, restoring the tab-on-reload UX for mobile without re-introducing the desktop bug. + +- **Pre-existing scrollController test failure.** Triage and fix; see Flaky Tests section. + +- **Integrate dashboard tests into porch's gated test runner.** Currently `pnpm test` from repo root runs only `@cluesmith/codev`, missing the dashboard suite (which contains the 22 new spec 761 tests). A follow-up could extend the gate to include `packages/dashboard`. diff --git a/codev/specs/761-surface-multiple-architects-in.md b/codev/specs/761-surface-multiple-architects-in.md new file mode 100644 index 000000000..0e453b957 --- /dev/null +++ b/codev/specs/761-surface-multiple-architects-in.md @@ -0,0 +1,375 @@ +# Specification: Surface Multiple Architects in Tower Dashboard (3.0.6 hotfix) + +## Metadata +- **ID**: 761-surface-multiple-architects-in +- **Status**: draft +- **Created**: 2026-05-18 +- **Protocol**: SPIR +- **GitHub Issue**: #761 +- **Predecessor**: spec/PR #755 / #757 +- **Target release**: 3.0.6 (hotfix on top of Ionic 3.0.5) + +## Architect Slicing Decision (2026-05-18) + +**This spec's v1 ships dashboard tabs only.** The architect directed (2026-05-18T20:48Z) that the minimum shippable slice for 3.0.6 is the single browser-side change that unblocks the external customer: rendering one tab per architect in the Tower dashboard so the sibling-architect terminal is **clickable from a browser**. + +**Concretely, v1 (this PR) ships**: +1. `/api/state` exposes the full architects collection (with names) alongside the preserved scalar pointer. +2. The dashboard renders one tab per architect when N > 1; single-architect workspaces look identical to today. + +**v1 explicitly defers** (each spun out as its own follow-up issue after this merges): +- **VS Code extension Workspace sidebar** (lists all architects in the tree view). +- **`afx status` formatter** (architect names header + `--architect ` builder filter). + +**Why slice this way**: shipping 3.0.5 with the routing primitive but no browser-side surface was a customer-impact mistake (the sibling architect is registered, addressable by routing, but invisible to the human). The browser tab is the single change that turns "routing works in theory" into "feature is usable end-to-end" — every other surface (VSCode, CLI) is a quality-of-life improvement on top of an already-usable feature. The customer is blocked specifically on the browser flow. Velocity through 3.0.6 matters more than completeness across all surfaces in one PR. + +The rest of this spec is written to v1 scope. Sections that previously described VSCode and `afx status` work are now marked "deferred" and retained briefly as forward references; full re-specification will happen in the follow-up issues at the time those slices are pulled in. + +## Problem Statement + +PR #757 (spec #755) shipped the **plumbing** for the sibling-architect pattern: Tower can hold N named architect terminals per workspace, each with a stable name; builders persist `spawnedByArchitect`; and `afx send architect` from a builder routes back to its spawning architect specifically. + +But spec #755 deliberately kept the **user-facing surface scalar** — explicitly: "`/api/state` shape is unchanged in v1; the dashboard sees the `main` architect (or first registered)." The result is that a workspace can host two architects today and routing works correctly, but the human running both architects has **no clickable way to access the second architect's terminal** from either the Tower dashboard or the VS Code extension. They have to read the terminal ID from the spawn output and either deep-link manually (`?tab=architect` only opens the `main`/first one) or memorise the terminal ID. + +That makes the feature half-shipped: the protocol-level fix is in place but the day-to-day driving surfaces — dashboard tabs, VS Code Workspace view, `afx status` — still pretend there is exactly one architect. + +This spec scopes a v1 UI surface change to make the multi-architect topology first-class throughout the user-facing layer, without touching any routing logic. + +## Current State + +**Internally Tower already supports N architects.** `WorkspaceTerminals.architects` (`packages/codev/src/agent-farm/servers/tower-types.ts:41-46`) is a `Map` keyed by architect name. `addArchitect()` in `tower-instances.ts` registers each named architect into the map. The local `state.db` `architect` table and the global `terminal_sessions` `role_id` both store names. Routing already uses these names. + +**Surface layers collapse the collection to a scalar.** The collapse happens in three places: + +1. **`/api/state` handler** (`packages/codev/src/agent-farm/servers/tower-routes.ts:1472-1486`) emits a scalar `architect: { port, pid, terminalId?, persistent? } | null`, populated from `entry.architects.get('main') ?? entry.architects.values().next().value`. Architects beyond the first are not exposed. +2. **`getTerminalsForWorkspace()`** (`packages/codev/src/agent-farm/servers/tower-terminals.ts:928-940`) registers every named architect into the in-memory map but emits exactly one `TerminalEntry` of type `'architect'` with hard-coded `id: 'architect'` and `label: 'Architect'`. There is therefore one architect entry in `InstanceStatus.terminals`, regardless of how many architects are registered. +3. **`architectUrl`** (`packages/codev/src/agent-farm/servers/tower-instances.ts:205`) is a scalar `${proxyUrl}?tab=architect` deep link. The dashboard's `useTabs.ts` reads `?tab=` and matches `'architect'` by type — there is no `?tab=architect-2` path. + +**Dashboard consumes the scalar.** `useTabs.ts:27-29` pushes a single `{ id: 'architect', type: 'architect', label: 'Architect', ... }` tab when `state.architect` is present. `App.tsx:184` finds that one tab and renders it in the left pane of the desktop SplitPane (or in the mobile tab bar). The right-side `TabBar` filters out `t.type === 'architect'` (`App.tsx:256`). + +**VS Code extension** (`packages/vscode/src/views/workspace.ts:23-34`) renders a single hard-coded `TreeItem('Open Architect')` whose command is `codev.openArchitectTerminal`. That command (`packages/vscode/src/extension.ts:140-157`) reads `state.architect.terminalId` from the workspace state and opens it via `terminalManager.openArchitect(...)`. There is no surface for a second architect. + +**`afx status`** (`packages/codev/src/agent-farm/commands/status.ts`) has two code paths: +- Tower-running path (lines 44-71) reads `workspaceStatus.terminals` and prints each terminal one per line. Because `getTerminalsForWorkspace` collapses to one architect entry, the user sees a single line: `architect - Architect (active)`. Architect names are invisible. +- Tower-not-running path (lines 82-92) reads local `state.architect` scalar. (`state.ts` already has a `main`-only shim since spec #755, so this only ever sees `main`.) + +There is no `--architect ` filter on builders, even though `state.db`'s `builders` table now has `spawned_by_architect TEXT`. + +**Net result**: a sibling-architect workspace has its second architect terminal registered in Tower (the human can route to it from `afx send architect`), but they cannot **see** or **click** it anywhere in the standard UX. To open the second architect's terminal in the dashboard, the user must know its terminal ID (visible only in spawn-time log lines) and construct a URL manually. To open it in VS Code they must do the same and use `codev.openTerminalById` (if such a thing exists; today it does not). To find out which architect spawned which builder, they have to read `state.db` directly. + +This is exactly what the spec #755 architect comments flagged for follow-up: "Filtering and dashboard surfacing is out of scope for v1; will be picked up by issue #2 (per-architect identity in spawn + status)." Issue #761 *is* that issue, scoped to the UX side specifically. + +## Desired State + +A sibling-architect workspace is visible and operable from every standard Codev driving surface: + +- The dashboard shows **one architect tab per architect**, labelled by name. Switching between them is a single click; the selected architect is remembered per workspace across page loads. +- The VS Code extension Workspace sidebar shows **one row per architect**, each opening that architect's terminal in a VS Code tab named `Codev: (architect)`. +- `afx status` shows the **list of registered architects** (with their names) and supports `--architect ` to filter the builder list to those spawned by that architect. +- `/api/state` exposes the **full architects collection** so all three surfaces can consume it from a single source of truth. + +**Single-architect workspaces show zero visual change.** A workspace with just `main` looks identical in dashboard, VS Code, and `afx status` to its pre-#761 appearance — same single tab in the left pane, same single "Open Architect" row in the sidebar, same single line in `afx status`. The multi-architect UI elements only manifest when N > 1. + +## Scope + +### In scope (v1 — 3.0.6 hotfix) + +Two coupled changes; both required for v1 to be useful end-to-end: + +1. **`/api/state` exposes the full architects collection.** Add `architects: Array<{ name, terminalId, port, pid, persistent }>` to the `DashboardState` response. The scalar `state.architect` field is preserved unchanged as a backward-compat convenience pointer to `main` (or first registered) so VSCode extension users on older builds continue to work. Both fields are returned simultaneously in v1; deprecation of the scalar is a follow-up decision tracked outside this spec. + + **Implementation notes the plan must address** (called out by Gemini's spec review): + - The response shape is defined in **two places** that must stay in sync: the shared interface `DashboardState` in `packages/types/src/api.ts:51-60` AND the inline type literal in `tower-routes.ts:1452-1461` inside `handleWorkspaceState`. The plan must update both, or refactor to import from `@cluesmith/codev-types`. + - v1 does **not** add `spawnedByArchitect` to the builders returned by `/api/state`. That field is only needed by the deferred `afx status --architect ` filter; surfacing it requires expanding the in-memory `WorkspaceTerminals.builders` cache (currently `Map` = builderId → terminalId) to carry architect-name metadata, or doing per-builder `state.db` lookups in the API handler. Both are reasonable but neither is needed for v1. The follow-up issue that picks up `afx status` decides this. + +2. **Dashboard renders one tab per architect when N > 1.** + - **Tab label** = architect name (`main`, `architect-2`, or whatever custom name was supplied via `afx workspace add-architect --name `). + - **Tab body** = that architect's terminal, rendered the same way the single architect terminal is rendered today (xterm WebSocket, `persistent` flag respected, `activatedTerminals` lazy-mount pattern preserved). + - **Active-tab persistence per workspace**: the active architect's name is remembered in `localStorage` keyed by workspace path, so a page refresh returns the user to the architect they were viewing. The persistence key is plan-phase-defined (collision audit required). + - **Layout placement on desktop**: the architect terminals live in the left pane of the existing SplitPane. When N = 1 the pane contains a single bare terminal (no tab strip — current visual exactly). When N > 1 the pane gains a small tab strip *inside the pane* listing the N architects; the body shows the active architect's terminal. The SplitPane and its collapse-pane behaviour are unchanged. + - **Layout placement on mobile**: architect tabs appear in the main TabBar like builder/shell tabs do today; with N > 1 there are N architect-typed entries. With N = 1, the single architect tab renders identically to today. + - **Deep linking**: `?tab=architect` continues to select the first architect tab (the existing `useTabs.ts` match-by-type at line 87 naturally falls back to the first `type: 'architect'` tab — Gemini confirmed this requires no new logic). `?tab=architect:` selects the named architect tab; unknown name falls back to the default. + - **WebSocket lifecycle when switching architect tabs** (called out by Claude's spec review): the existing `activatedTerminals` Set in `App.tsx:39` already implements lazy mount + keep-alive — terminals are mounted on first visit and stay mounted (hidden via CSS `display: none`) thereafter. The plan extends this to architect tabs: each architect terminal is mounted on first visit; all N WebSocket connections stay alive across tab switches. This matches existing builder/shell terminal behaviour and avoids reconnect-flicker. Acceptable resource cost for realistic N ≤ 5. + - **Single-architect appearance unchanged**: when N = 1, no architect tab strip is rendered in the left pane; the visual is structurally identical (DOM-snapshot identical) to today's single-architect dashboard. "Structurally identical" rather than "byte-identical" is the precise requirement (Claude flagged that literal byte equality is awkward to assert in React rendering). + +3. **Backward compatibility**: + - Single-architect workspaces see **no behavior change** in either of the two in-scope surfaces (dashboard layout, `/api/state` consumers). + - The scalar `state.architect` field on `/api/state` is preserved. + - The `?tab=architect` deep-link path continues to land on a valid architect tab. + +### Deferred to follow-up issues (NOT in v1) + +These were originally in the issue body's "Required" list and the spec's earlier scope. The architect's 2026-05-18 directive moved them to follow-up issues to keep 3.0.6 small and fast: + +- **VS Code extension Workspace sidebar listing all architects.** Replaces single `TreeItem('Open Architect')` with one row per architect. Each row opens its terminal in a VS Code tab `Codev: (architect)`. **Follow-up issue must address the `terminalManager.openArchitect` map-key collision Gemini flagged** (`terminals.get('architect')` is keyed by literal `'architect'` today; multi-architect requires `architect-${name}` keying or it will re-focus the first tab silently). The follow-up issue also must make `WorkspaceProvider.getChildren()` async and fetch `client.getWorkspaceState()`, since today's TreeProvider returns static items only. Tracked separately. + +- **`afx status` architect-names header.** Add a header line listing registered architects by name. Source-of-truth audit required at follow-up: the spec's earlier draft had a contradiction between `getWorkspaceStatus()` (returns `InstanceStatus` — no architect names) and `getWorkspaceState()` (returns `DashboardState` — has names). The follow-up issue standardises on `getWorkspaceState()`. Tracked separately. + +- **`afx status --architect ` builder filter.** Filters the builders list to those with `spawned_by_architect = `. Requires either (a) expanding `WorkspaceTerminals.builders` to carry architect-name metadata, (b) on-the-fly `state.db` query in the `/api/state` handler, or (c) reading `state.db` directly from the CLI. Decision is part of the follow-up issue. + +These follow-ups are tightly coupled to v1's `/api/state` change (they consume the new `architects` collection), but they do not block 3.0.6 from shipping. + +### Out of scope (deferred follow-ups) + +These items from the original issue #755 follow-up list are **not** addressed by this spec: + +- `THREAD.md` template + lifecycle (`codev thread new/list/archive`) — #755 follow-up #3. +- Cross-thread visibility (`codev thread show `) — #755 follow-up #4. +- Thread-aware `consult` (`consult --thread ` or `.thread` auto-detection) — #755 follow-up #5. + +These remain as separate follow-up issues and would not be unblocked by 761. + +### Explicitly NOT in scope + +- **No routing logic touched.** The affinity-routing primitive (builder → spawning architect) is already in place from PR #757. This spec only changes the dashboard surface. +- **No new architect-identity model.** Names are the identity, exactly as established in spec #755. The only new field on existing types is `name: string` on `ArchitectState` (already implicitly there since the in-memory `WorkspaceTerminals.architects` map is keyed by name). +- **No new architect-creation CLI.** `afx workspace add-architect --name ` from #755 stays as the way to create additional architects. 761 does not add a creation surface inside the dashboard. +- **No multi-architect-aware send.** `afx send architect` continues to use the builder's recorded `spawnedByArchitect` for routing. The dashboard does not gain a "send to architect by name" UI. +- **No name-rename UI.** Architect names are set at registration and immutable for v1. Renaming is left for a future spec if demand arises. +- **No removal of the scalar `state.architect` field on `/api/state`.** The spec keeps it as a backward-compat pointer; a follow-up spec may deprecate and remove it once all consumers have migrated. +- **No `spawnedByArchitect` on `/api/state` builders.** Deferred to the `afx status` follow-up that actually needs it. +- **No VS Code extension changes in this PR.** Deferred — see "Deferred to follow-up issues" above. +- **No `afx status` changes in this PR.** Deferred — see "Deferred to follow-up issues" above. + +## Stakeholders + +- **Primary**: Codev consumers running the sibling-architect pattern (the user that motivated #755/#761, plus future adopters of the pattern). +- **Secondary**: Solo-architect users — must see **zero behavior change**. This is a hard constraint, not an aspiration. +- **Technical Team**: Codev maintainers (Tower routes, dashboard React, VSCode extension, `afx` CLI). +- **Business Owner**: M Waleed Kadous (architect role for Codev). + +## Success Criteria + +- [ ] `/api/state` response includes an `architects` array listing every registered architect with `{ name, terminalId, port, pid, persistent }`. The scalar `state.architect` field continues to be returned and points to `main` (or first registered). +- [ ] The shared `DashboardState` interface in `packages/types/src/api.ts` and the inline type literal in `tower-routes.ts:handleWorkspaceState` are updated together; a unit or type-check test asserts they stay in sync (e.g. by importing the shared type into the handler, or by an automated assertion). +- [ ] In a workspace with two architects (`main` + `sibling`), the Tower dashboard left pane (desktop) or main TabBar (mobile) shows a small architect tab strip with two entries labelled `main` and `sibling`. Clicking each switches the terminal body. Both terminals' WebSockets stay alive across tab switches (no reconnect-flicker). +- [ ] Active architect tab name is persisted per workspace in `localStorage`; page refresh restores the previously-selected architect. Last-write-wins across multiple browser tabs is acceptable and documented. +- [ ] A single-architect workspace (only `main`) shows a DOM-snapshot-identical dashboard layout to the pre-#761 baseline. No architect tab strip, no extra DOM nodes. +- [ ] Deep link `?tab=architect` continues to land on a valid architect tab (the first one returned by `state.architects`, which equals `main` if present). Deep link `?tab=architect:` lands on the named architect's tab; unknown name falls back to the default with no error toast. +- [ ] All existing tests pass; new tests cover: + - **`/api/state`**: `architects` collection contents match the in-memory `entry.architects` map (single and multi-architect cases); scalar `architect` is preserved and points to `main` or first. + - **Type sync**: `tower-routes.ts` inline response type cannot drift from `DashboardState`. Either enforced by importing or by an explicit assertion test. + - **Dashboard tabs**: tab-strip presence for N>1, absence for N=1, active-tab `localStorage` persistence across reload, deep-link routing including `?tab=architect:` and fallback for unknown name. + - **Persistent terminal mount**: switching between architect tabs does not unmount existing terminals (extend the existing `activatedTerminals` test patterns). +- [ ] Single-architect users (the dominant population) see no visible difference in either the dashboard or in `/api/state` consumers that ignore unknown fields. + +## Constraints + +### Technical constraints + +- **No routing logic change.** Only `/api/state`, the dashboard React layer, and possibly `getTerminalsForWorkspace`'s terminal-emission may be touched in v1. Spec #755's three security rules and the `from`-aware `resolveTarget` path are not in scope and not modified. +- **Backward-compatible `/api/state` shape.** Adding `architects` is fine; removing or renaming `architect` is not. External consumers (the unupgraded VSCode extension is the most-cited example) may depend on the scalar shape and must continue to work. +- **Two type definitions must stay in sync.** The `DashboardState` interface lives in `packages/types/src/api.ts`. An inline duplicate of the response shape lives in `tower-routes.ts:handleWorkspaceState`. v1 must update both, or refactor to remove the duplication. The plan picks the approach; the spec requires that drift is structurally prevented (compile-time import, or asserted test). +- **No new SQLite migrations.** All data needed is already persisted by spec #755's v13 migration (`architect.id` = name). No schema change in v1. +- **Dashboard layout stability.** The current SplitPane (architect left, work/builders right) is load-bearing for muscle memory. Multi-architect rendering must not move the architect terminals out of the left pane on desktop, must not lose the `?tab=` deep-link path, and must not break the existing `--no-architect` empty-state ("No architect terminal"). +- **Persistent WebSocket model preserved.** The existing `activatedTerminals` lazy-mount + keep-alive pattern in `App.tsx` (Bugfix #205) must be extended to architect tabs, not replaced. Tab switching must not trigger Terminal unmount/remount. +- **Tab-strip empty state**: when an architect terminal is registered but its PTY session has died (`session.status !== 'running'`), the tab strip should still show the architect (greyed out / with a status indicator) — or alternatively omit it. Plan-phase decision; the spec only requires that the surface does not crash on a dead architect. + +### Business constraints + +- **Solo-architect users must never have to know this feature exists.** No new mandatory CLI flags, no new mandatory config keys, no UI elements that appear before the user opts into a second architect. +- **Time-to-merge matters.** This is the second half of a multi-architect feature whose first half shipped in PR #757. Keeping it tightly scoped is more valuable than polish. + +## Assumptions + +- The architects' identity model from spec #755 is final. Each architect has a stable `name` (string, `[a-z][a-z0-9-]*`, ≤64 chars); `main` is the default for the first; subsequent are auto-numbered `architect-N`. Names are immutable for v1. +- The dashboard's SplitPane / left-pane architect convention is desirable to preserve; users should still be able to see the architect terminal side-by-side with the right-pane work view. This is the v1 assumption — a follow-up could move architects into the right tab bar entirely if user feedback demands it. +- VSCode extension users can be assumed to have updated to a version that handles the new `architects` collection (since 761 is a single commit on top of #755). The scalar `state.architect` field is preserved purely for non-VSCode external consumers. +- Per-workspace `localStorage` persistence is acceptable for active-tab memory; no requirement for cross-device sync of which architect was last open. + +## Solution Approach + +The v1 change is two coupled edits, ordered: + +1. **`/api/state` collection.** Extend `DashboardState` in `packages/types/src/api.ts` with `architects: ArchitectState[]`, where each `ArchitectState` gains a `name: string` field. Preserve the scalar `architect` field unchanged (population logic: `main` if present, else first registered — identical to today). The implementation lives in `handleWorkspaceState` in `tower-routes.ts:1443-1537`: iterate `entry.architects` and emit one entry per name, each carrying its session-derived `pid`, `terminalId`, and `persistent`. The inline type literal at `tower-routes.ts:1452-1461` must be updated alongside (or refactored to import `DashboardState` from `@cluesmith/codev-types`). + + **Do NOT modify `getTerminalsForWorkspace`** (`tower-terminals.ts:928-940`). The dashboard does not consume `TerminalEntry[]` for architect rendering; it reads from `/api/state` (which reads `entry.architects` directly). Modifying `getTerminalsForWorkspace` would change `InstanceStatus.terminals` — which flows into `afx status` via `getWorkspaceStatus()` — breaking the strict "no `afx status` changes in v1" boundary. The single `TerminalEntry` of type `'architect'` it emits today stays as-is; multi-architect `TerminalEntry` emission is a follow-up alongside the `afx status` work. + +2. **Dashboard.** Extend `useTabs.ts:17-29` to push one architect tab per entry in `state.architects` (replacing the current scalar-driven push at lines 27-29). Update `App.tsx` so that when there is more than one architect, the left pane renders a small tab strip listing the architects' names; selecting one shows that terminal's body. When N = 1 the existing single-terminal rendering is unchanged. The deep-link parser in the `useTabs.ts:79-99` `useEffect` is extended to recognise `?tab=architect:` (small colon-parsing addition; the existing `tab.id === tabParam || tab.type === tabParam` match continues to handle the bare `?tab=architect` case). Active-tab persistence stores the selected architect name in `localStorage` keyed by `codev-active-architect:` (plan-phase confirms key naming). On mobile, architect tabs flow through the existing TabBar machinery, one per architect. + + **Implementation subtlety (called out by Claude review)**: today's left-pane rendering at `App.tsx:236-238` is a bare `Terminal` component that bypasses the `activatedTerminals` lazy-mount + keep-alive pattern used by the right pane (`renderPersistentContent`). For multi-architect tab-switching to avoid Terminal unmount/remount, the left pane needs to participate in the `activatedTerminals` machinery — either by extending `renderPersistentContent` to also accept a "left-pane terminal list" parameter, or by introducing a parallel persistent-content renderer for the left pane. Plan-phase picks the approach; either keeps the WebSocket alive across tab switches. + +This is the entirety of the v1 implementation surface. Layers for VS Code extension and `afx status` are explicitly deferred to follow-up issues (see "Deferred to follow-up issues" in Scope). + +## Open Questions + +### Critical (blocks progress) + +*None.* All previously-critical questions resolved during architect's framing in the issue body. + +### Important (affects design) + +- [ ] **Tab strip placement on desktop.** Inside the left pane is the cleaner option (architect terminals stay in their pane). Plan-phase to decide whether the strip is *above* the terminal (small horizontal tab row, like the right-pane TabBar) or rendered as a left-vertical strip / dropdown header. The spec only requires that the strip exists, persists active selection, and respects the existing pane-collapse behaviour. +- [ ] **Tab-strip behaviour for dead architect terminals** (PTY session ended but registration still in `entry.architects`). Plan-phase to decide: omit, grey-out with a status indicator, or show with a click-to-restart affordance. +- [ ] **Deep-link separator.** `?tab=architect:` is the spec's preferred shape. Plan-phase confirms or proposes a colon-free alternative (e.g. `?architect=` as a separate param) if the colon parsing conflicts with existing URL handling. +- [ ] **`localStorage` key naming.** Plan-phase audits existing dashboard `localStorage` keys (none assumed at spec time) and picks a non-colliding namespace. + +### Nice-to-know (optimization) + +- [ ] **Active-tab persistence key collisions across browser tabs.** If a user opens the same workspace in two browser tabs simultaneously, both write to the same `localStorage` key. Spec assumes this is acceptable (last-write-wins). +- [ ] **`/api/state` polling vs SSE for newly-added architects.** If a second architect is added via `afx workspace add-architect` while the dashboard is already loaded, how does it appear? Claude flagged this in spec review. Today's dashboard polls `/api/state` (existing `useBuilderStatus` hook); the new architect appears on the next poll, and `useTabs` already auto-switches to genuinely-new tabs (`useTabs.ts:114-119`) — so the new architect would auto-select. The plan must verify this works for architect tabs (the existing skip condition is `tab.type !== 'architect'`, which would suppress auto-switch — see `useTabs.ts:115`). Resolve: invert that skip for architect tabs added post-load (treat them like builders), or leave them at user-explicit selection. Plan-phase decision. +- [ ] **Should the tab body show the architect's spawn time / uptime?** Adjacent to a deferred "richer architect status surface" follow-up. Out of scope. + +## Performance Requirements + +- **`/api/state` payload size.** The new `architects` collection adds ~4 fields × N architects to the response. For realistic N (2–3 in practice) this is sub-kilobyte and negligible. No streaming or pagination needed. +- **Dashboard render cost.** N architect tabs is bounded by N architect terminals, which is bounded by the human's working-memory limit (likely ≤ 5 in practice). No virtualisation needed. +- **`afx status` runtime.** No new DB queries beyond what already happens; the architect table is already read for the solo-architect case. The filter is a single `WHERE spawned_by_architect = ?` on an already-indexed-by-id table; effectively free. + +## Security Considerations + +- **No new auth surfaces, no new credentials, no new tokens.** The change is purely presentational; routing, message delivery, and identity remain unchanged. +- **Tab-name spoofing not a concern.** Architect names are constrained to `[a-z][a-z0-9-]*` (max 64) at registration time per spec #755. There is no surface where a builder can suggest a name; names are set by the user via the architect-creation CLI. +- **`/api/state` exposes architect names that were previously hidden in the response.** A name is not a secret — it appears in spawn logs, in `state.db`, and in routing messages already. Exposing it in the API does not widen the visibility envelope. +- **Cross-workspace leakage.** `/api/state` is already per-workspace (its handler reads `entry = getRehydratedTerminalsEntry(workspacePath)`). Adding the collection does not change the scoping. + +## Test Scenarios + +### Functional + +1. **Single-architect baseline (regression).** Workspace has one architect (`main`). `/api/state` returns scalar `architect: { ..., terminalId, ... }` AND `architects: [{ name: 'main', terminalId, ... }]`. Dashboard renders no tab strip in the left pane; the architect terminal fills the pane as today. DOM-snapshot identical to pre-761 baseline. +2. **Two architects, dashboard tabs.** Workspace with `main` + `sibling`. `/api/state` returns `architects: [main, sibling]`. Dashboard left pane shows a tab strip with two entries labelled `main` and `sibling`. Clicking each switches the visible terminal body without unmounting either. Both WebSockets remain alive after switching. Selection persists across page reload via `localStorage`. Deep link `?tab=architect:sibling` opens sibling. Deep link `?tab=architect` opens main (backward compat). +3. **Three architects.** Workspace with `main`, `architect-2`, `architect-3`. Tab strip shows three entries. Each click works; `localStorage` round-trip works for each. +4. **Newly-added architect, dashboard already loaded.** User has the dashboard open. Architect adds a second architect via `afx workspace add-architect --name sibling`. On next `/api/state` poll, a `sibling` tab appears in the strip. Auto-switch behaviour matches plan-phase decision (the existing `useTabs.ts:114-119` auto-switch logic with architects either re-enabled or kept suppressed). +5. **Dead architect terminal.** Architect `sibling` is registered (still in `entry.architects`) but its PTY has exited. Dashboard does not crash; tab is either omitted or visibly indicates dead state (plan-phase pinned). +6. **Active-tab persistence across browser tabs (concurrent).** User opens the workspace in two browser tabs, selects different architects in each, refreshes one. The refreshed tab restores the architect that was last selected in *that* localStorage state (last-write-wins). Documented as acceptable. +7. **Deep link with unknown name.** `?tab=architect:ghost` falls back to the default architect tab. No crash, no error toast. +8. **`/api/state` collection vs scalar consistency.** Response includes both `architect` (scalar, == `main` or first) and `architects` (array, all registered). For all valid `entry.architects` states, the two views are consistent. +9. **Type-sync guard.** A unit / type-check test (or compile-time import) ensures `DashboardState` in `packages/types/src/api.ts` and the inline type in `tower-routes.ts:handleWorkspaceState` cannot drift. + +### Non-functional + +1. **Payload latency parity.** `/api/state` response size in a single-architect workspace is within a small constant overhead of today's response (one extra `architects` array with one entry). No measurable client-side parse-time impact. +2. **Render flicker.** Switching architect tabs in the dashboard does not trigger a Terminal unmount/remount. Verified via existing terminal-persistence test patterns (Bugfix #205 / Bugfix #524 test families). +3. **No regression for solo users.** With N = 1, dashboard DOM is structurally identical to pre-761; `/api/state` is shape-stable (scalar `architect` field unchanged). + +## Dependencies + +- **Internal systems**: `WorkspaceTerminals.architects` map (already collection-shaped from #755), `state.db` `architect` table (already keyed by name), `state.db` `builders.spawned_by_architect` column (already exists), Tower `/api/state` handler, dashboard React, VS Code extension TreeView and terminal manager, `afx status` formatter. +- **External services**: none. +- **Libraries / frameworks**: none new. + +## References + +**Spec #755 / PR #757 (predecessor)**: +- `codev/specs/755-multi-architect-support-per-ar.md` +- `codev/plans/755-multi-architect-support-per-ar.md` +- `codev/reviews/755-multi-architect-support-per-ar.md` + +**Surface call sites for v1 (in scope)**: +- `packages/codev/src/agent-farm/servers/tower-routes.ts:1443-1537` — `handleWorkspaceState` / `/api/state` handler. **Note dual type definition** (inline literal at 1452-1461). +- `packages/types/src/api.ts:11-16,51-60` — `ArchitectState` (gains `name: string`) and `DashboardState` (gains `architects: ArchitectState[]`). +- `packages/dashboard/src/hooks/useTabs.ts:17-99` — tab construction and deep-link handling. +- `packages/dashboard/src/components/App.tsx:39,76-87,113-149,184-238,256` — `activatedTerminals` state, left-pane SplitPane content, right-pane TabBar filter. + +**Backend call site explicitly NOT touched in v1** (separated from the deferred-follow-up call sites because it's a non-obvious choice): +- `packages/codev/src/agent-farm/servers/tower-terminals.ts:928-940` — `getTerminalsForWorkspace`'s architect-`TerminalEntry` emission. Stays at one entry per workspace in v1. Modifying it would change `InstanceStatus.terminals` and leak into `afx status`, violating the slicing boundary. The follow-up that picks up `afx status` revisits this together with the formatter change. + +**Call sites NOT touched in v1 (deferred)**: +- `packages/vscode/src/views/workspace.ts:23-51` — sidebar TreeView. Deferred follow-up. +- `packages/vscode/src/extension.ts:140-157` — `codev.openArchitectTerminal` command. Deferred follow-up. **Map-key collision in `terminalManager.openArchitect` flagged by Gemini must be addressed at that time.** +- `packages/codev/src/agent-farm/commands/status.ts:44-92` — status formatter. Deferred follow-up. +- `packages/codev/src/agent-farm/servers/tower-instances.ts:200-208` — `getInstances()` / scalar `architectUrl`. Stays scalar for v1 (matches `state.architect` scalar preservation). + +**Identity model and data persistence (already in place; reference, not modified)**: +- `packages/codev/src/agent-farm/types.ts:7-19` — `Builder.spawnedByArchitect`. +- `packages/codev/src/agent-farm/db/schema.ts` — `architect.id TEXT`, `builders.spawned_by_architect`, `terminal_sessions.role_id`. + +## Risks and Mitigation + +| Risk | Probability | Impact | Mitigation | +|------|-------------|--------|------------| +| Single-architect users see a visual regression (extra tab strip, extra DOM nodes) | Medium | Medium | Hard constraint that N=1 renders DOM-snapshot-identical to today, validated by snapshot / DOM-presence tests on dashboard. | +| Adding `architects` to `/api/state` breaks an external consumer that asserts on the response shape | Low | Medium | Scalar `state.architect` is preserved unchanged; new field is additive. JSON consumers that ignore unknown fields are unaffected. | +| Dashboard tab strip introduces a layout bug when the user collapses the left pane (`collapsedPane === 'left'`) | Medium | Medium | The tab strip lives inside the left pane and follows the pane's visibility. Existing collapse-pane tests cover the layout; new tests verify tab-strip behaviour under collapse. | +| Active-tab `localStorage` key collides with an existing key | Low | Low | Plan-phase audits existing `localStorage` keys and picks a non-colliding namespace. | +| `DashboardState` and the inline `tower-routes.ts` type literal drift apart in a follow-up PR | Medium | Medium | Plan-phase enforces drift-prevention (import the shared type at compile time, or add an assertion test). Tracked as a Success Criterion. | +| Existing `useTabs.ts` `tab.type !== 'architect'` auto-switch skip (line 115) suppresses the new architect tab from auto-focus when added post-load | Medium | Low | Plan-phase explicit decision: invert for architect tabs added post-load, or keep behaviour and require the user to click. Documented as an Open Question. | +| Persistent WebSocket count grows with N | Low | Low | At realistic N (≤ 5), N WebSockets is comparable to the builder-count load already present today. No optimisation needed. | +| Codex unavailable for this consultation round | Medium | Low | Two reviews (Gemini + Claude) cover the same checks. Architect decides whether to require codex re-review before approval. | + +## Expert Consultation + +**Date**: 2026-05-18 +**Models Consulted**: Gemini 3 Pro, Claude Opus 4.7. **Codex unavailable** in this worktree environment due to a missing vendored binary (`@openai+codex@0.101.0-darwin-arm64/.../codex` directory empty in pnpm node_modules); two retries both failed with `ENOENT`. Builder unable to `pnpm rebuild` in this environment (permission denial). Architect to decide whether to require codex re-review before approval or accept gemini + claude. + +### Verdicts (iteration 1) + +| Model | Verdict | Confidence | +|-------|---------|------------| +| Gemini | REQUEST_CHANGES | HIGH | +| Claude | APPROVE | HIGH | +| Codex | (unavailable) | — | + +### Verdicts (iteration 2 — after architect's slicing directive + fixes) + +| Model | Verdict | Confidence | +|-------|---------|------------| +| Gemini | REQUEST_CHANGES (resolved in this iteration) | HIGH | +| Claude | APPROVE | HIGH | +| Codex | (unavailable) | — | + +### Convergent findings (addressed in this iteration) + +1. **Inline-type drift in `tower-routes.ts:handleWorkspaceState`.** Both Gemini and Claude noted that the API handler defines its response shape inline (`tower-routes.ts:1452-1461`) rather than importing `DashboardState`. Adding `architects` requires updating both. **Fix**: Constraints section now requires that drift is structurally prevented (compile-time import OR asserted test). Success Criteria adds an explicit type-sync test. References call this out at the file/line level. + +### Gemini-specific findings (REQUEST_CHANGES verdict — addressed by re-slicing) + +Gemini raised three CRITICAL issues. Two of them (`afx status` data-source contradiction and `terminalManager.openArchitect` map-key collision) only affect the deferred VSCode and `afx status` slices, which the architect's 2026-05-18 directive moved out of v1. The third (missing `spawnedByArchitect` on `/api/state` builders) is similarly only required by the deferred `afx status --architect` filter — v1 does not surface this field at all. + +**Fix**: the spec's scope is now sliced per the architect directive. Each of Gemini's three concerns is captured in the "Deferred to follow-up issues" section as a known-must-address item for the corresponding follow-up. None of them are v1 blockers. + +Gemini also flagged a non-critical issue: +- **`workspace.ts` TreeProvider must be async** to fetch state. — Captured under the deferred VSCode follow-up. +- **`?tab=architect` natural fallback in `useTabs.ts:87`.** — Gemini confirmed this works without extra logic since the existing match-by-type will pick the first architect tab. Noted in Scope item 2 (deep linking). + +### Claude-specific findings (APPROVE verdict — addressed) + +1. **WebSocket lifecycle across architect tabs.** Spec did not explicitly state whether all N architect WebSockets stay alive or only the visible one. **Fix**: Scope item 2 now explicitly extends the existing `activatedTerminals` lazy-mount + keep-alive pattern to architect tabs. All N WebSockets stay alive across tab switches. Acceptable resource cost at realistic N ≤ 5. + +2. **Inline-type drift.** Covered above as convergent. + +3. **"Byte-identical" too literal for React.** **Fix**: Success Criteria and Constraints now say "DOM-snapshot identical" instead of "byte-identical." + +4. **Architect registered while dashboard is open.** **Fix**: added to Open Questions (nice-to-know) with a specific resolution-needed item — `useTabs.ts:114-119` skip condition currently suppresses architect-tab auto-switch; the plan must decide whether to invert this for post-load-added architects. + +5. **`Open Architect` → `main` label change in VS Code (N=1 regression).** **Fix**: VS Code scope is deferred. The follow-up issue inherits this question. + +### Iteration-2 findings (addressed in this iteration) + +Iteration 2 ran after the architect's 2026-05-18 slicing directive was incorporated into the spec. + +**Gemini (REQUEST_CHANGES)** raised two contradictions left over from the slicing rewrite: +1. **Stale "Solution Approach" steps 3 & 4 (VS Code, `afx status`)** still describing in-line implementation despite Scope marking them deferred. A builder reading the Solution Approach directly would have built out-of-scope features. **Fix**: deleted steps 3 & 4 entirely. Solution Approach now has only two steps (matching v1 scope). +2. **Incorrect instruction to modify `getTerminalsForWorkspace`** in Solution Approach step 1. The dashboard reads architects from `entry.architects` (via `tower-routes.ts:handleWorkspaceState`), NOT from `TerminalEntry[]`. Modifying `getTerminalsForWorkspace` would change `InstanceStatus.terminals` and leak into `afx status`, breaking the slicing boundary. **Fix**: removed the instruction; added an explicit "do NOT modify" note in Solution Approach with rationale; called out the file in References under "explicitly NOT touched in v1." + +**Claude (APPROVE)** verified all current-state claims against source files and confirmed no remaining blockers. Three minor observations, two adopted: +1. **`?tab=architect:` is not zero-new-logic.** The existing `tabs.find(t => t.id === tabParam || t.type === tabParam)` handles bare `?tab=architect` but not the `:` form. **Fix**: Solution Approach step 2 now explicitly says "small colon-parsing addition" rather than implying zero work. +2. **Left-pane rendering subtlety.** `App.tsx:236-238` is a bare `Terminal` component, NOT going through `renderPersistentContent` / `activatedTerminals`. For multi-architect tab-switching to avoid Terminal remount, the left pane must participate in `activatedTerminals`. **Fix**: Solution Approach step 2 now calls out this implementation subtlety explicitly with two implementation options for plan-phase to pick. +3. **Architect-removed-while-dashboard-open edge case.** Not in v1 (no removal CLI exists), but worth a note. *Not adopted as a code change* — out of scope. + +### Persisted consultation outputs + +- `codev/projects/761-surface-multiple-architects-in/761-spec-iter1-gemini.md` (REQUEST_CHANGES, mostly addressed by slicing) +- `codev/projects/761-surface-multiple-architects-in/761-spec-iter1-claude.md` (APPROVE) +- `codev/projects/761-surface-multiple-architects-in/761-spec-iter2-gemini.md` (REQUEST_CHANGES, addressed in this iteration) +- `codev/projects/761-surface-multiple-architects-in/761-spec-iter2-claude.md` (APPROVE) +- (codex output absent — see note above) + +## Approval + +- [ ] Multi-agent consultation complete (Gemini, Codex, Claude) +- [ ] Architect review (M Waleed Kadous) +- [ ] Spec-approval gate (porch) + +## Notes + +The architect's 2026-05-18 directive **explicitly sliced v1 to dashboard-tabs-only for the 3.0.6 hotfix**, deferring VS Code Workspace view and `afx status` to follow-ups. The customer is blocked specifically on the browser flow — every hour of process between now and 3.0.6 is a customer waiting on a feature paid-for-in-3.0.5-but-unusable. Velocity matters more than completeness across all surfaces in one PR. + +This is exactly the kind of "ship-the-primitive-without-the-UI" failure that the user's recent feedback called out: spec #755 shipped a primitive without an end-to-end usable surface, so the customer got "routing works in theory" but couldn't drive it from a browser. 761 v1 closes that gap with the smallest possible surface change. + +Two design choices deserve explicit attention at plan time: +1. **Tab strip placement.** Inside the left pane is the cleaner option. Plan-phase to decide horizontal-above vs. left-vertical vs. dropdown-header. +2. **Auto-switch behaviour for post-load-added architects.** Whether to invert the existing `useTabs.ts` skip condition for architect tabs added while the dashboard is open. + +A consultation gap (codex unavailable in this worktree environment due to a missing vendored binary) is documented in the Expert Consultation section. Architect to decide acceptability. diff --git a/packages/codev/src/agent-farm/__tests__/spec-761-api-state.test.ts b/packages/codev/src/agent-farm/__tests__/spec-761-api-state.test.ts new file mode 100644 index 000000000..e0cf70790 --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/spec-761-api-state.test.ts @@ -0,0 +1,303 @@ +/** + * Spec 761 — Phase 1 tests for /api/state architects collection. + * + * Verifies: + * - `architects: ArchitectState[]` is emitted with one entry per registered architect. + * - The `main` architect is moved to index 0 when present. + * - The scalar `architect` field is preserved and points to architects[0]. + * - Empty / no-architect workspaces emit `architects: []` and `architect: null`. + * - Architects whose PTY session is gone are skipped silently. + * - The response carries the new shared `name` field on every architect entry. + */ + +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import http from 'node:http'; +import { EventEmitter } from 'node:events'; +import { handleRequest } from '../servers/tower-routes.js'; +import type { RouteContext } from '../servers/tower-routes.js'; + +// ============================================================================ +// Mocks (same shape as tower-routes.test.ts; kept local for isolation) +// ============================================================================ + +const { mockGetTerminalManager, mockGetSession, + mockGetRehydratedTerminalsEntry, mockIsSessionPersistent, + mockReadCloudConfig } = vi.hoisted(() => ({ + mockGetTerminalManager: vi.fn(), + mockGetSession: vi.fn(), + mockGetRehydratedTerminalsEntry: vi.fn(async () => ({ + architects: new Map(), + builders: new Map(), + shells: new Map(), + fileTabs: new Map(), + })), + mockIsSessionPersistent: vi.fn(), + mockReadCloudConfig: vi.fn(() => null), +})); + +vi.mock('../lib/cloud-config.js', () => ({ + readCloudConfig: (...args: unknown[]) => mockReadCloudConfig(...args), +})); + +vi.mock('../servers/tower-instances.js', () => ({ + getInstances: vi.fn(), + getKnownWorkspacePaths: vi.fn(() => []), + getDirectorySuggestions: vi.fn(async () => []), + launchInstance: vi.fn(async () => ({ success: true })), + killTerminalWithShellper: vi.fn(async () => true), + stopInstance: vi.fn(async () => ({ ok: true })), + addArchitect: vi.fn(), +})); + +vi.mock('../servers/tower-terminals.js', () => ({ + getWorkspaceTerminals: vi.fn(() => new Map()), + getTerminalManager: mockGetTerminalManager, + getWorkspaceTerminalsEntry: vi.fn(), + getNextShellId: vi.fn(), + saveTerminalSession: vi.fn(), + isSessionPersistent: mockIsSessionPersistent, + deleteTerminalSession: vi.fn(), + removeTerminalFromRegistry: vi.fn(), + deleteWorkspaceTerminalSessions: vi.fn(), + saveFileTab: vi.fn(), + deleteFileTab: vi.fn(), + getTerminalsForWorkspace: vi.fn(), + getRehydratedTerminalsEntry: mockGetRehydratedTerminalsEntry, +})); + +vi.mock('../servers/tower-tunnel.js', () => ({ + handleTunnelEndpoint: vi.fn(), +})); + +vi.mock('../servers/tower-messages.js', () => ({ + resolveTarget: vi.fn(), + broadcastMessage: vi.fn(), + isResolveError: vi.fn(() => false), +})); + +vi.mock('../servers/tower-utils.js', () => ({ + isRateLimited: vi.fn(() => false), + normalizeWorkspacePath: (p: string) => p, + getLanguageForExt: (ext: string) => ext, + getMimeTypeForFile: () => 'application/octet-stream', + serveStaticFile: vi.fn(() => false), +})); + +vi.mock('../utils/server-utils.js', () => ({ + isRequestAllowed: vi.fn(() => true), + parseJsonBody: vi.fn(async () => ({})), +})); + +vi.mock('../servers/analytics.js', () => ({ + computeAnalytics: vi.fn(), + clearAnalyticsCache: vi.fn(), +})); + +vi.mock('../servers/overview.js', () => ({ + OverviewCache: class { + getOverview = vi.fn(async () => ({ builders: [], pendingPRs: [], backlog: [] })); + invalidate = vi.fn(); + }, +})); + +// ============================================================================ +// Helpers +// ============================================================================ + +function makeCtx(overrides: Partial = {}): RouteContext { + return { + log: vi.fn(), + port: 4100, + templatePath: '/tmp/tower.html', + reactDashboardPath: '/tmp/dashboard/dist', + hasReactDashboard: false, + getShellperManager: () => null, + broadcastNotification: vi.fn(), + addSseClient: vi.fn(), + removeSseClient: vi.fn(), + ...overrides, + }; +} + +function makeReq(url: string): http.IncomingMessage { + const req = new EventEmitter() as any; + req.method = 'GET'; + req.url = url; + req.headers = { host: 'localhost:4100' }; + req.socket = { remoteAddress: '127.0.0.1' }; + return req; +} + +function makeRes() { + const chunks: string[] = []; + let code = 200; + const res = { + writeHead: vi.fn((status: number) => { code = status; }), + setHeader: vi.fn(), + end: vi.fn((data?: string | Buffer) => { + if (data) chunks.push(typeof data === 'string' ? data : data.toString()); + }), + write: vi.fn((data: string) => { chunks.push(data); }), + } as any; + return { + res, + body: () => chunks.join(''), + statusCode: () => code, + }; +} + +const ENCODED_WS = Buffer.from('/test/workspace').toString('base64url'); +const STATE_URL = `/workspace/${ENCODED_WS}/api/state`; + +// ============================================================================ +// Tests +// ============================================================================ + +describe('Spec 761: /api/state architects collection', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockGetTerminalManager.mockReturnValue({ getSession: mockGetSession }); + mockIsSessionPersistent.mockReturnValue(false); + }); + + it('returns architects: [] and architect: null when no architect is registered', async () => { + mockGetRehydratedTerminalsEntry.mockResolvedValueOnce({ + architects: new Map(), + builders: new Map(), + shells: new Map(), + fileTabs: new Map(), + }); + + const { res, statusCode, body } = makeRes(); + await handleRequest(makeReq(STATE_URL), res, makeCtx()); + + expect(statusCode()).toBe(200); + const parsed = JSON.parse(body()); + expect(parsed.architects).toEqual([]); + expect(parsed.architect).toBeNull(); + }); + + it('returns single-architect collection with main at index 0', async () => { + mockGetRehydratedTerminalsEntry.mockResolvedValueOnce({ + architects: new Map([['main', 'term-main']]), + builders: new Map(), + shells: new Map(), + fileTabs: new Map(), + }); + mockGetSession.mockImplementation((id: string) => + id === 'term-main' ? { label: 'Architect', pid: 4242 } : undefined, + ); + + const { res, body } = makeRes(); + await handleRequest(makeReq(STATE_URL), res, makeCtx()); + + const parsed = JSON.parse(body()); + expect(parsed.architects).toHaveLength(1); + expect(parsed.architects[0]).toMatchObject({ + name: 'main', + terminalId: 'term-main', + pid: 4242, + port: 0, + persistent: false, + }); + expect(parsed.architect).toEqual(parsed.architects[0]); + }); + + it('returns two-architect collection with main first when main inserted second', async () => { + // Insertion order: sibling first, then main. The handler must still place + // main at index 0 of architects[]. + mockGetRehydratedTerminalsEntry.mockResolvedValueOnce({ + architects: new Map([ + ['sibling', 'term-sibling'], + ['main', 'term-main'], + ]), + builders: new Map(), + shells: new Map(), + fileTabs: new Map(), + }); + mockGetSession.mockImplementation((id: string) => + id === 'term-main' ? { label: 'Architect', pid: 1111 } + : id === 'term-sibling' ? { label: 'Architect', pid: 2222 } + : undefined, + ); + + const { res, body } = makeRes(); + await handleRequest(makeReq(STATE_URL), res, makeCtx()); + + const parsed = JSON.parse(body()); + expect(parsed.architects).toHaveLength(2); + expect(parsed.architects[0].name).toBe('main'); + expect(parsed.architects[1].name).toBe('sibling'); + expect(parsed.architect).toEqual(parsed.architects[0]); + }); + + it('returns three architects with main first; non-main entries follow insertion order', async () => { + mockGetRehydratedTerminalsEntry.mockResolvedValueOnce({ + architects: new Map([ + ['architect-2', 'term-a2'], + ['main', 'term-main'], + ['architect-3', 'term-a3'], + ]), + builders: new Map(), + shells: new Map(), + fileTabs: new Map(), + }); + mockGetSession.mockImplementation((id: string) => + id === 'term-main' ? { pid: 1 } + : id === 'term-a2' ? { pid: 2 } + : id === 'term-a3' ? { pid: 3 } + : undefined, + ); + + const { res, body } = makeRes(); + await handleRequest(makeReq(STATE_URL), res, makeCtx()); + + const parsed = JSON.parse(body()); + expect(parsed.architects.map((a: { name: string }) => a.name)).toEqual([ + 'main', 'architect-2', 'architect-3', + ]); + }); + + it('skips architects whose PTY session is gone (race / stale registration)', async () => { + mockGetRehydratedTerminalsEntry.mockResolvedValueOnce({ + architects: new Map([ + ['main', 'term-main'], + ['ghost', 'term-ghost'], + ]), + builders: new Map(), + shells: new Map(), + fileTabs: new Map(), + }); + // term-ghost has no session — should be silently skipped. + mockGetSession.mockImplementation((id: string) => + id === 'term-main' ? { pid: 9 } : undefined, + ); + + const { res, body } = makeRes(); + await handleRequest(makeReq(STATE_URL), res, makeCtx()); + + const parsed = JSON.parse(body()); + expect(parsed.architects).toHaveLength(1); + expect(parsed.architects[0].name).toBe('main'); + }); + + it('exposes architect (singular) scalar even when only a non-main architect is registered', async () => { + // No main; first registered should land in both architects[0] and architect. + mockGetRehydratedTerminalsEntry.mockResolvedValueOnce({ + architects: new Map([['sibling', 'term-sibling']]), + builders: new Map(), + shells: new Map(), + fileTabs: new Map(), + }); + mockGetSession.mockReturnValue({ pid: 77 }); + + const { res, body } = makeRes(); + await handleRequest(makeReq(STATE_URL), res, makeCtx()); + + const parsed = JSON.parse(body()); + expect(parsed.architects).toHaveLength(1); + expect(parsed.architects[0].name).toBe('sibling'); + expect(parsed.architect.name).toBe('sibling'); + expect(parsed.architect.terminalId).toBe('term-sibling'); + }); +}); diff --git a/packages/codev/src/agent-farm/servers/tower-routes.ts b/packages/codev/src/agent-farm/servers/tower-routes.ts index 44c6ab246..30592c82a 100644 --- a/packages/codev/src/agent-farm/servers/tower-routes.ts +++ b/packages/codev/src/agent-farm/servers/tower-routes.ts @@ -27,7 +27,7 @@ import { version } from '../../version.js'; const execAsync = promisify(exec); import type { SessionManager } from '../../terminal/session-manager.js'; import type { PtySessionInfo } from '../../terminal/pty-session.js'; -import type { BuilderSpawnedPayload } from '@cluesmith/codev-types'; +import type { BuilderSpawnedPayload, DashboardState, ArchitectState } from '@cluesmith/codev-types'; import { DEFAULT_COLS, defaultSessionOptions } from '../../terminal/index.js'; import type { SSEClient } from './tower-types.js'; import { parseJsonBody, isRequestAllowed } from '../utils/server-utils.js'; @@ -1449,17 +1449,11 @@ async function handleWorkspaceState( // entry — see getRehydratedTerminalsEntry doc. const entry = await getRehydratedTerminalsEntry(workspacePath); const manager = getTerminalManager(); - const state: { - architect: { port: number; pid: number; terminalId?: string; persistent?: boolean } | null; - builders: Array<{ id: string; name: string; port: number; pid: number; status: string; phase: string; worktree: string; branch: string; type: string; terminalId?: string; persistent?: boolean }>; - utils: Array<{ id: string; name: string; port: number; pid: number; terminalId?: string; persistent?: boolean; lastDataAt?: number }>; - annotations: Array<{ id: string; file: string; port: number; pid: number }>; - workspaceName?: string; - version?: string; - hostname?: string; - teamEnabled?: boolean; - } = { + // Spec 761: type as DashboardState directly to prevent inline-literal drift. + // ArchitectState.name (Spec 761) carries the architect's stable identity. + const state: DashboardState = { architect: null, + architects: [], builders: [], utils: [], annotations: [], @@ -1469,22 +1463,31 @@ async function handleWorkspaceState( teamEnabled: await hasTeam(path.join(workspacePath, 'codev', 'team')), }; - // Add architect if exists. - // Spec 755: /api/state preserves the scalar `state.architect` shape. - // Prefer 'main', else first registered architect by name. - const architectTerminalId = - entry.architects.get('main') ?? entry.architects.values().next().value; - if (architectTerminalId) { - const session = manager.getSession(architectTerminalId); - if (session) { - state.architect = { - port: 0, - pid: session.pid || 0, - terminalId: architectTerminalId, - persistent: isSessionPersistent(architectTerminalId, session), - }; - } + // Spec 761: build the architects collection from entry.architects. + // - Skip entries whose session is unavailable (race / stale registration). + // - Move 'main' to index 0 when present so consumers can rely on + // architects[0] as the default architect. + // Spec 755: the scalar `state.architect` is preserved as a backward-compat + // pointer to the same default architect (architects[0] when present). + const collectedArchitects: ArchitectState[] = []; + for (const [architectName, terminalId] of entry.architects) { + const session = manager.getSession(terminalId); + if (!session) continue; + collectedArchitects.push({ + name: architectName, + port: 0, + pid: session.pid || 0, + terminalId, + persistent: isSessionPersistent(terminalId, session), + }); + } + const mainIdx = collectedArchitects.findIndex(a => a.name === 'main'); + if (mainIdx > 0) { + const [mainEntry] = collectedArchitects.splice(mainIdx, 1); + collectedArchitects.unshift(mainEntry); } + state.architects = collectedArchitects; + state.architect = collectedArchitects[0] ?? null; // Add shells from refreshed cache for (const [shellId, terminalId] of entry.shells) { diff --git a/packages/dashboard/__tests__/App.architect-tabs.test.tsx b/packages/dashboard/__tests__/App.architect-tabs.test.tsx new file mode 100644 index 000000000..51c6a4e0b --- /dev/null +++ b/packages/dashboard/__tests__/App.architect-tabs.test.tsx @@ -0,0 +1,295 @@ +/** + * Spec 761: App multi-architect rendering tests. + * + * Verifies: + * - N=0: existing empty-state, no architect strip. + * - N=1: bare Terminal in the left pane, no architect strip (DOM-snapshot stability). + * - N=2: ArchitectTabStrip with two entries; clicking each toggles which terminal + * pane is visible (display:none vs visible); both terminals remain mounted. + */ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, fireEvent, cleanup } from '@testing-library/react'; + +// localStorage mock for jsdom (same pattern as TipBanner/useTabs tests) +const storageMap = new Map(); +const localStorageMock = { + getItem: (key: string) => storageMap.get(key) ?? null, + setItem: (key: string, value: string) => storageMap.set(key, value), + removeItem: (key: string) => storageMap.delete(key), + clear: () => storageMap.clear(), + get length() { return storageMap.size; }, + key: (index: number) => [...storageMap.keys()][index] ?? null, +}; +Object.defineProperty(window, 'localStorage', { value: localStorageMock, writable: true }); + +// --- Mocks (hoisted by vitest) --- + +const mockUseBuilderStatus = vi.fn(); +vi.mock('../src/hooks/useBuilderStatus.js', () => ({ + useBuilderStatus: () => mockUseBuilderStatus(), +})); + +vi.mock('../src/hooks/useMediaQuery.js', () => ({ + useMediaQuery: () => false, // desktop layout +})); + +vi.mock('../src/components/Terminal.js', () => ({ + Terminal: ({ wsPath }: { wsPath: string }) => ( +
Terminal: {wsPath}
+ ), +})); + +vi.mock('../src/components/WorkView.js', () => ({ + WorkView: () =>
Work
, +})); + +vi.mock('../src/components/AnalyticsView.js', () => ({ + AnalyticsView: () =>
Analytics
, +})); + +vi.mock('../src/components/FileViewer.js', () => ({ + FileViewer: () =>
File
, +})); + +vi.mock('../src/components/SplitPane.js', () => ({ + SplitPane: ({ left, right }: { left: React.ReactNode; right: React.ReactNode }) => ( +
+
{left}
+
{right}
+
+ ), +})); + +vi.mock('../src/components/MobileLayout.js', () => ({ + MobileLayout: ({ children }: { children: React.ReactNode }) => ( +
{children}
+ ), +})); + +vi.mock('../src/components/TabBar.js', () => ({ + TabBar: ({ tabs }: { tabs: Array<{ id: string; label: string }> }) => ( +
{tabs.map(t => {t.label})}
+ ), +})); + +import { App } from '../src/components/App.js'; + +afterEach(() => { + cleanup(); + storageMap.clear(); + mockUseBuilderStatus.mockReset(); +}); + +function archEntry(name: string, terminalId: string) { + return { name, port: 0, pid: 1, terminalId, persistent: false }; +} + +describe('App multi-architect dashboard (Spec 761)', () => { + it('N=0: renders empty-state, no architect strip', () => { + mockUseBuilderStatus.mockReturnValue({ + state: { + architect: null, + architects: [], + builders: [], + utils: [], + annotations: [], + }, + refresh: vi.fn(), + }); + + render(); + + expect(screen.getByText(/No architect terminal/i)).toBeInTheDocument(); + expect(screen.queryByLabelText('Architect tabs')).toBeNull(); + }); + + it('N=1: renders bare Terminal in the left pane, no architect strip', () => { + mockUseBuilderStatus.mockReturnValue({ + state: { + architect: archEntry('main', 'term-main'), + architects: [archEntry('main', 'term-main')], + builders: [], + utils: [], + annotations: [], + }, + refresh: vi.fn(), + }); + + render(); + + // Architect strip should NOT be rendered when N=1 (DOM-snapshot stability) + expect(screen.queryByLabelText('Architect tabs')).toBeNull(); + // The left pane should render exactly one architect Terminal. + const leftPane = screen.getByTestId('split-left'); + expect(leftPane).toHaveTextContent('Terminal:'); + }); + + it('N=2: renders ArchitectTabStrip with two entries', () => { + mockUseBuilderStatus.mockReturnValue({ + state: { + architect: archEntry('main', 'term-main'), + architects: [ + archEntry('main', 'term-main'), + archEntry('sibling', 'term-sibling'), + ], + builders: [], + utils: [], + annotations: [], + }, + refresh: vi.fn(), + }); + + render(); + + const strip = screen.getByLabelText('Architect tabs'); + expect(strip).toBeInTheDocument(); + expect(screen.getByRole('tab', { name: 'main' })).toBeInTheDocument(); + expect(screen.getByRole('tab', { name: 'sibling' })).toBeInTheDocument(); + }); + + it('N=2: clicking a strip tab flips display style; both terminals remain mounted', () => { + mockUseBuilderStatus.mockReturnValue({ + state: { + architect: archEntry('main', 'term-main'), + architects: [ + archEntry('main', 'term-main'), + archEntry('sibling', 'term-sibling'), + ], + builders: [], + utils: [], + annotations: [], + }, + refresh: vi.fn(), + }); + + render(); + + // Both terminals should be in the DOM (persistent rendering pattern). + const mainTerm = screen.getByTestId('terminal-/ws/terminal/term-main'); + const siblingTerm = screen.getByTestId('terminal-/ws/terminal/term-sibling'); + expect(mainTerm).toBeInTheDocument(); + expect(siblingTerm).toBeInTheDocument(); + + // Initially: main visible (first architect), sibling hidden. + expect(mainTerm.closest('.terminal-tab-pane')!).not.toHaveStyle({ display: 'none' }); + expect(siblingTerm.closest('.terminal-tab-pane')!).toHaveStyle({ display: 'none' }); + + // Click sibling tab in the strip. + fireEvent.click(screen.getByRole('tab', { name: 'sibling' })); + + // After click: visibility flipped, both still mounted. + expect(mainTerm.closest('.terminal-tab-pane')!).toHaveStyle({ display: 'none' }); + expect(siblingTerm.closest('.terminal-tab-pane')!).not.toHaveStyle({ display: 'none' }); + + // Both elements are the same DOM nodes — not remounted. + expect(screen.getByTestId('terminal-/ws/terminal/term-main')).toBe(mainTerm); + expect(screen.getByTestId('terminal-/ws/terminal/term-sibling')).toBe(siblingTerm); + }); + + it('N=2: persists clicked architect to localStorage', () => { + mockUseBuilderStatus.mockReturnValue({ + state: { + architect: archEntry('main', 'term-main'), + architects: [ + archEntry('main', 'term-main'), + archEntry('sibling', 'term-sibling'), + ], + builders: [], + utils: [], + annotations: [], + }, + refresh: vi.fn(), + }); + + render(); + fireEvent.click(screen.getByRole('tab', { name: 'sibling' })); + + expect(storageMap.get(`codev-active-architect:${window.location.pathname}`)).toBe('sibling'); + }); + + it('N=2: restores persisted active architect on initial render', () => { + storageMap.set(`codev-active-architect:${window.location.pathname}`, 'sibling'); + mockUseBuilderStatus.mockReturnValue({ + state: { + architect: archEntry('main', 'term-main'), + architects: [ + archEntry('main', 'term-main'), + archEntry('sibling', 'term-sibling'), + ], + builders: [], + utils: [], + annotations: [], + }, + refresh: vi.fn(), + }); + + render(); + + const mainTerm = screen.getByTestId('terminal-/ws/terminal/term-main'); + const siblingTerm = screen.getByTestId('terminal-/ws/terminal/term-sibling'); + // sibling should be visible because it was persisted as the active architect. + expect(siblingTerm.closest('.terminal-tab-pane')!).not.toHaveStyle({ display: 'none' }); + expect(mainTerm.closest('.terminal-tab-pane')!).toHaveStyle({ display: 'none' }); + }); + + it('N=2: reload with persisted architect keeps work view active on the right pane (Claude iter-1 bug fix)', () => { + // Spec 761 regression guard: previously, useTabs restored the persisted + // architect into activeTabId on mount, which blanked the entire right + // pane (work/analytics/team all hidden since activeTab.type === 'architect'). + // The fix removed useTabs's localStorage read; App.tsx handles left-pane + // restoration independently. + storageMap.set(`codev-active-architect:${window.location.pathname}`, 'sibling'); + mockUseBuilderStatus.mockReturnValue({ + state: { + architect: archEntry('main', 'term-main'), + architects: [ + archEntry('main', 'term-main'), + archEntry('sibling', 'term-sibling'), + ], + builders: [], + utils: [], + annotations: [], + }, + refresh: vi.fn(), + }); + + render(); + + // Right pane: work view should be visible (default activeTabId === 'work'). + const workView = screen.getByTestId('work-view'); + expect(workView.parentElement).not.toHaveStyle({ display: 'none' }); + // Left pane: sibling is still the active architect. + const siblingTerm = screen.getByTestId('terminal-/ws/terminal/term-sibling'); + expect(siblingTerm.closest('.terminal-tab-pane')!).not.toHaveStyle({ display: 'none' }); + }); + + it('N=2: deep-link ?tab=architect: syncs the left pane selection', () => { + // Simulate a deep-link by setting the search string before render. Leave + // pathname at the default '/' to keep getTerminalWsPath()'s output + // matching the existing terminal-/ws/terminal/ testids. + window.history.replaceState({}, '', '/?tab=architect:sibling'); + mockUseBuilderStatus.mockReturnValue({ + state: { + architect: archEntry('main', 'term-main'), + architects: [ + archEntry('main', 'term-main'), + archEntry('sibling', 'term-sibling'), + ], + builders: [], + utils: [], + annotations: [], + }, + refresh: vi.fn(), + }); + + render(); + + const mainTerm = screen.getByTestId('terminal-/ws/terminal/term-main'); + const siblingTerm = screen.getByTestId('terminal-/ws/terminal/term-sibling'); + expect(siblingTerm.closest('.terminal-tab-pane')!).not.toHaveStyle({ display: 'none' }); + expect(mainTerm.closest('.terminal-tab-pane')!).toHaveStyle({ display: 'none' }); + + // Restore the URL for subsequent tests. + window.history.replaceState({}, '', '/'); + }); +}); diff --git a/packages/dashboard/__tests__/ArchitectTabStrip.test.tsx b/packages/dashboard/__tests__/ArchitectTabStrip.test.tsx new file mode 100644 index 000000000..754346d21 --- /dev/null +++ b/packages/dashboard/__tests__/ArchitectTabStrip.test.tsx @@ -0,0 +1,78 @@ +/** + * Spec 761: ArchitectTabStrip component tests. + */ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, fireEvent, cleanup } from '@testing-library/react'; +import { ArchitectTabStrip } from '../src/components/ArchitectTabStrip.js'; +import type { Tab } from '../src/hooks/useTabs.js'; + +function archTab(name: string, id: string): Tab { + return { + id, + type: 'architect', + label: name, + closable: false, + terminalId: `term-${name}`, + architectName: name, + }; +} + +afterEach(cleanup); + +describe('ArchitectTabStrip (Spec 761)', () => { + it('renders one button per architect', () => { + render( + , + ); + + expect(screen.getAllByRole('tab')).toHaveLength(2); + expect(screen.getByText('main')).toBeInTheDocument(); + expect(screen.getByText('sibling')).toBeInTheDocument(); + }); + + it('marks the active tab as aria-selected and tab-active', () => { + render( + , + ); + + const tabs = screen.getAllByRole('tab'); + expect(tabs[0]).toHaveAttribute('aria-selected', 'false'); + expect(tabs[1]).toHaveAttribute('aria-selected', 'true'); + expect(tabs[1].className).toContain('tab-active'); + expect(tabs[0].className).not.toContain('tab-active'); + }); + + it('calls onSelectTab with the clicked tab id', () => { + const onSelectTab = vi.fn(); + render( + , + ); + + fireEvent.click(screen.getByText('sibling')); + expect(onSelectTab).toHaveBeenCalledWith('architect:sibling'); + }); + + it('renders no close buttons (architect tabs are non-closable)', () => { + render( + , + ); + + expect(screen.queryByLabelText(/close/i)).toBeNull(); + }); +}); diff --git a/packages/dashboard/__tests__/useTabs.architects.test.ts b/packages/dashboard/__tests__/useTabs.architects.test.ts new file mode 100644 index 000000000..f86b23660 --- /dev/null +++ b/packages/dashboard/__tests__/useTabs.architects.test.ts @@ -0,0 +1,189 @@ +/** + * Spec 761: per-architect tabs in useTabs. + * + * Verifies: + * - Architects emit one tab each; the first uses bare `id: 'architect'`. + * - Subsequent architects use `id: 'architect:'`. + * - Each architect tab carries `architectName`. + * - The fallback `state.architect` scalar populates a one-element tab list + * when `state.architects` is absent (deploy-window safety). + * - localStorage round-trip restores the persisted active architect on mount. + * - The `?tab=architect:` deep-link selects the named architect tab. + * - The bare `?tab=architect` deep-link still selects the first architect. + * - Unknown architect names fall back to the first architect tab. + * - Removed auto-switch skip: newly-added architects auto-focus. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { renderHook, act } from '@testing-library/react'; +import type { DashboardState } from '@cluesmith/codev-types'; + +// localStorage mock for jsdom (same pattern as TipBanner.test.tsx) +const storageMap = new Map(); +const localStorageMock = { + getItem: (key: string) => storageMap.get(key) ?? null, + setItem: (key: string, value: string) => storageMap.set(key, value), + removeItem: (key: string) => storageMap.delete(key), + clear: () => storageMap.clear(), + get length() { return storageMap.size; }, + key: (index: number) => [...storageMap.keys()][index] ?? null, +}; +Object.defineProperty(window, 'localStorage', { value: localStorageMock, writable: true }); + +import { useTabs } from '../src/hooks/useTabs.js'; + +function makeState(overrides: Partial): DashboardState { + return { + architect: null, + architects: [], + builders: [], + utils: [], + annotations: [], + ...overrides, + }; +} + +function archEntry(name: string, overrides: Partial<{ pid: number; terminalId: string; persistent: boolean }> = {}) { + return { + name, + port: 0, + pid: overrides.pid ?? 1, + terminalId: overrides.terminalId ?? `term-${name}`, + persistent: overrides.persistent ?? false, + }; +} + +const STORAGE_KEY = `codev-active-architect:/workspace/Vd29ya3NwYWNl/`; + +beforeEach(() => { + // Ensure clean URL state for each test + window.history.replaceState({}, '', '/workspace/Vd29ya3NwYWNl/'); + storageMap.clear(); +}); + +afterEach(() => { + storageMap.clear(); +}); + +describe('useTabs — architect tabs (Spec 761)', () => { + it('emits no architect tab when state has no architects', () => { + const { result } = renderHook(() => useTabs(makeState({}))); + expect(result.current.tabs.filter(t => t.type === 'architect')).toHaveLength(0); + }); + + it('emits one architect tab with bare id "architect" when N=1', () => { + const { result } = renderHook(() => useTabs(makeState({ + architects: [archEntry('main')], + }))); + + const archTabs = result.current.tabs.filter(t => t.type === 'architect'); + expect(archTabs).toHaveLength(1); + expect(archTabs[0].id).toBe('architect'); + expect(archTabs[0].label).toBe('main'); + expect(archTabs[0].architectName).toBe('main'); + }); + + it('emits N architect tabs with the first using bare id "architect" and rest prefixed', () => { + const { result } = renderHook(() => useTabs(makeState({ + architects: [archEntry('main'), archEntry('sibling'), archEntry('architect-3')], + }))); + + const archTabs = result.current.tabs.filter(t => t.type === 'architect'); + expect(archTabs.map(t => t.id)).toEqual([ + 'architect', + 'architect:sibling', + 'architect:architect-3', + ]); + expect(archTabs.map(t => t.architectName)).toEqual(['main', 'sibling', 'architect-3']); + }); + + it('falls back to scalar state.architect when state.architects is absent (deploy-window safety)', () => { + // Simulate older server response shape — only the scalar field is present + // and architects is undefined. The hook should treat it as a one-arch list. + const { result } = renderHook(() => useTabs({ + architect: archEntry('main'), + builders: [], + utils: [], + annotations: [], + // architects intentionally omitted (cast through unknown to satisfy TS) + } as unknown as DashboardState)); + + const archTabs = result.current.tabs.filter(t => t.type === 'architect'); + expect(archTabs).toHaveLength(1); + expect(archTabs[0].architectName).toBe('main'); + expect(archTabs[0].id).toBe('architect'); + }); + + it('does NOT restore active architect from localStorage into activeTabId', () => { + // Spec 761 (post Claude iter-1 review): useTabs deliberately does not + // read localStorage. If it did, activeTabId would land on an architect + // tab on reload, which blanks the desktop right pane (every section + // checks activeTab?.type === 'work'/'analytics'/'team', all false for + // an architect tab). Architect restoration is App.tsx's responsibility. + localStorage.setItem(STORAGE_KEY, 'sibling'); + + const { result } = renderHook(() => useTabs(makeState({ + architects: [archEntry('main'), archEntry('sibling')], + }))); + + expect(result.current.activeTabId).toBe('work'); + }); + + it('honors ?tab=architect: deep-link', () => { + window.history.replaceState({}, '', '/workspace/Vd29ya3NwYWNl/?tab=architect:sibling'); + const { result } = renderHook(() => useTabs(makeState({ + architects: [archEntry('main'), archEntry('sibling')], + }))); + + expect(result.current.activeTabId).toBe('architect:sibling'); + // URL parameter is cleaned up to avoid sticky behaviour on refresh + expect(window.location.search).toBe(''); + }); + + it('?tab=architect: falls back to first architect tab', () => { + window.history.replaceState({}, '', '/workspace/Vd29ya3NwYWNl/?tab=architect:ghost'); + const { result } = renderHook(() => useTabs(makeState({ + architects: [archEntry('main'), archEntry('sibling')], + }))); + + expect(result.current.activeTabId).toBe('architect'); + }); + + it('?tab=architect still works (matches first architect by type)', () => { + window.history.replaceState({}, '', '/workspace/Vd29ya3NwYWNl/?tab=architect'); + const { result } = renderHook(() => useTabs(makeState({ + architects: [archEntry('main'), archEntry('sibling')], + }))); + + expect(result.current.activeTabId).toBe('architect'); + }); + + it('selectTab does NOT write localStorage from useTabs (App.tsx owns architect persistence)', () => { + // Spec 761 (post Claude iter-1 review): persistence is App.tsx's + // responsibility via the strip handler. useTabs.selectTab is symmetric + // for all tab types: it only updates activeTabId. + const { result } = renderHook(() => useTabs(makeState({ + architects: [archEntry('main'), archEntry('sibling')], + }))); + + act(() => { + result.current.selectTab('architect:sibling'); + }); + + expect(localStorage.getItem(STORAGE_KEY)).toBeNull(); + }); + + it('auto-switches to a newly-added architect tab (skip removed)', () => { + let stateRef = makeState({ architects: [archEntry('main')] }); + const { result, rerender } = renderHook(() => useTabs(stateRef)); + + // Seed: with N=1 and no URL/localStorage, initial active should be 'work'. + expect(result.current.activeTabId).toBe('work'); + + // Add a second architect post-load — should auto-focus. + stateRef = makeState({ architects: [archEntry('main'), archEntry('sibling')] }); + rerender(); + + expect(result.current.activeTabId).toBe('architect:sibling'); + }); +}); diff --git a/packages/dashboard/src/components/App.tsx b/packages/dashboard/src/components/App.tsx index 4f3983524..a76325b59 100644 --- a/packages/dashboard/src/components/App.tsx +++ b/packages/dashboard/src/components/App.tsx @@ -1,11 +1,13 @@ -import { useState, useEffect, useRef, useCallback } from 'react'; +import { useState, useEffect, useRef, useCallback, type ReactNode } from 'react'; import { useBuilderStatus } from '../hooks/useBuilderStatus.js'; -import { useTabs } from '../hooks/useTabs.js'; +import { useTabs, type Tab } from '../hooks/useTabs.js'; import { useMediaQuery } from '../hooks/useMediaQuery.js'; import { MOBILE_BREAKPOINT } from '../lib/constants.js'; import { getTerminalWsPath, createFileTab } from '../lib/api.js'; +import { readActiveArchitect, writeActiveArchitect } from '../lib/architectPersistence.js'; import { SplitPane } from './SplitPane.js'; import { TabBar } from './TabBar.js'; +import { ArchitectTabStrip } from './ArchitectTabStrip.js'; import { Terminal } from './Terminal.js'; import { WorkView } from './WorkView.js'; import { MobileLayout } from './MobileLayout.js'; @@ -32,6 +34,24 @@ export function App() { const { tabs, activeTab, activeTabId, selectTab } = useTabs(state); const isMobile = useMediaQuery(`(max-width: ${MOBILE_BREAKPOINT}px)`); const [collapsedPane, setCollapsedPane] = useState<'left' | 'right' | null>(null); + // Spec 761: desktop left-pane architect selection is independent of the + // global activeTabId so the right pane keeps its own content while the + // user flips between architect terminals on the left. Persisted by name + // across reloads via localStorage. + const [activeArchitectName, setActiveArchitectName] = useState( + () => readActiveArchitect(), + ); + + // Spec 761: when activeTabId (driven by useTabs) lands on an architect — + // via deep link (?tab=architect:) or the post-load auto-switch for + // a newly-added architect — sync that into the independent left-pane + // state so the left pane reflects the selection. (Strip clicks set + // activeArchitectName directly without touching activeTabId.) + useEffect(() => { + if (activeTab?.type === 'architect' && activeTab.architectName) { + setActiveArchitectName(activeTab.architectName); + } + }, [activeTab?.id, activeTab?.type, activeTab?.architectName]); // Bugfix #205: Track which terminal tabs have been visited at least once. // Terminals are only mounted on first visit, then kept alive (hidden via CSS) @@ -107,9 +127,43 @@ export function App() { return ; }; - // Bugfix #205: Render persistent terminal tabs (kept mounted, shown/hidden via CSS) - // plus the active non-terminal content. terminalTypes specifies which tab types - // to persist (desktop right panel excludes 'architect' since it's in the left pane). + // Bugfix #205: Render persistent terminal tabs (kept mounted, shown/hidden via CSS). + // + // Spec 761: this is the extracted helper that drives both panes. The left + // pane uses it for architect tabs when N > 1, the right pane uses it for + // builder+shell tabs as today. The caller passes the `activeId` so the + // left pane (architect strip) can drive visibility independently of the + // global `activeTabId`. `toolbarExtra` is threaded onto the active terminal + // only — passing it to all would render multiple copies of the collapse + // buttons. + const renderPersistentTerminals = useCallback(( + tabsToRender: Tab[], + activeId: string | null, + toolbarExtra?: ReactNode, + ) => { + return tabsToRender.map(tab => { + const wsPath = getTerminalWsPath(tab); + const isActive = activeId === tab.id; + return ( +
+ {wsPath + ? + :
No terminal session
+ } +
+ ); + }); + }, [handleFileOpen]); + const renderPersistentContent = (terminalTypes: string[]) => { const persistentTabs = tabs.filter(t => terminalTypes.includes(t.type) && activatedTerminals.has(t.id) @@ -117,21 +171,7 @@ export function App() { return ( <> - {persistentTabs.map(tab => { - const wsPath = getTerminalWsPath(tab); - return ( -
- {wsPath - ? - :
No terminal session
- } -
- ); - })} + {renderPersistentTerminals(persistentTabs, activeTabId)}
@@ -180,8 +220,12 @@ export function App() { ); } - // Desktop: architect terminal on left, tabbed content on right - const architectTab = tabs.find(t => t.type === 'architect'); + // Desktop: architect terminal(s) on left, tabbed content on right. + // Spec 761: multiple architects render as a tab strip inside the left + // pane. When N == 1 the strip is omitted and the bare Terminal renders + // exactly as before (DOM-snapshot-identical to pre-761). + const architectTabs = tabs.filter(t => t.type === 'architect'); + const architectTab = architectTabs[0]; // Bugfix #524: Tri-state collapse buttons — one button per side that cycles // through full-width → 50/50 → collapsed. Uses onPointerDown+preventDefault @@ -232,10 +276,56 @@ export function App() { ); - const architectWsPath = architectTab ? getTerminalWsPath(architectTab) : null; - const leftPane = architectWsPath - ? - :
No architect terminal
; + // Spec 761: + // - N == 0: existing empty-state. + // - N == 1: existing bare Terminal render (DOM-snapshot-identical). + // - N > 1: tab strip + persistent terminals via renderPersistentTerminals, + // so switching tabs flips visibility instead of unmounting WebSockets. + // + // The "active architect" for the left pane is tracked in `activeArchitectName` + // (independent of `activeTabId`), so flipping a builder/work tab on the + // right doesn't blank the architect on the left. Defaults to the first + // architect (architects[0] === 'main' when present per Phase 1 ordering). + let leftPane: ReactNode; + if (architectTabs.length === 0) { + leftPane =
No architect terminal
; + } else if (architectTabs.length === 1) { + const wsPath = getTerminalWsPath(architectTab!); + leftPane = wsPath + ? + :
No architect terminal
; + } else { + // Resolve which architect is active on the left pane. Prefer the + // persisted name (if it matches an existing architect), else fall back + // to architects[0]. + const activeArchTab = + architectTabs.find(t => t.architectName === activeArchitectName) + ?? architectTabs[0]; + const activeArchitectTabId = activeArchTab?.id ?? ''; + leftPane = ( +
+ { + const picked = architectTabs.find(t => t.id === id); + if (picked?.architectName) { + setActiveArchitectName(picked.architectName); + writeActiveArchitect(picked.architectName); + } + }} + /> +
+ {renderPersistentTerminals(architectTabs, activeArchitectTabId, architectToolbarExtra)} +
+
+ ); + } return (
diff --git a/packages/dashboard/src/components/ArchitectTabStrip.tsx b/packages/dashboard/src/components/ArchitectTabStrip.tsx new file mode 100644 index 000000000..cc44ca33f --- /dev/null +++ b/packages/dashboard/src/components/ArchitectTabStrip.tsx @@ -0,0 +1,32 @@ +import type { Tab } from '../hooks/useTabs.js'; + +/** + * Spec 761: a small tab strip shown inside the left pane of the dashboard + * when more than one architect is registered. Reuses the same `tab` and + * `tab-active` CSS classes as the right-pane `TabBar` for visual + * consistency. Architect tabs are not closable. + */ +interface ArchitectTabStripProps { + tabs: Tab[]; + activeTabId: string; + onSelectTab: (id: string) => void; +} + +export function ArchitectTabStrip({ tabs, activeTabId, onSelectTab }: ArchitectTabStripProps) { + return ( +
+ {tabs.map(tab => ( + + ))} +
+ ); +} diff --git a/packages/dashboard/src/hooks/useTabs.ts b/packages/dashboard/src/hooks/useTabs.ts index 4590b25f1..6a2c04bf6 100644 --- a/packages/dashboard/src/hooks/useTabs.ts +++ b/packages/dashboard/src/hooks/useTabs.ts @@ -1,5 +1,5 @@ import { useState, useCallback, useEffect, useRef } from 'react'; -import type { DashboardState, Builder, UtilTerminal, Annotation } from '../lib/api.js'; +import type { DashboardState, ArchitectState } from '../lib/api.js'; export interface Tab { id: string; @@ -12,6 +12,49 @@ export interface Tab { annotationId?: string; filePath?: string; persistent?: boolean; + /** + * Spec 761: the architect's stable name (when type === 'architect'). + * Carries the architect identity independently of the tab `id`/`label`, + * so consumers (deep-link parsing, localStorage persistence) don't need + * to parse it out of the `id` string. + */ + architectName?: string; +} + +/** + * Spec 761: derive the tab `id` for an architect entry. + * + * The first architect (always `architects[0]`, which is `main` when + * present per Phase 1's ordering) keeps the bare `'architect'` id so the + * single-architect dashboard DOM is identical to the pre-761 baseline AND + * `main`'s id is stable across the N=1 ↔ N>1 transition. Subsequent + * architects use `architect:` ids. + */ +function architectTabId(index: number, name: string): string { + return index === 0 ? 'architect' : `architect:${name}`; +} + +function buildArchitectTabs(state: DashboardState | null): Tab[] { + // Prefer the new `architects` collection; fall back to the scalar + // `architect` (wrapped as a one-element array) for any momentary deploy- + // window where dashboard.js is newer than the server response. + const architects: ArchitectState[] = state?.architects + ?? (state?.architect ? [state.architect] : []); + return architects.map((a, index) => { + // Deploy-window safety: scalar architect from an older server response + // may lack `name`. Default to 'main' so the label and architectName are + // never undefined. + const name = a.name ?? 'main'; + return { + id: architectTabId(index, name), + type: 'architect' as const, + label: name, + closable: false, + terminalId: a.terminalId, + persistent: a.persistent, + architectName: name, + }; + }); } function buildTabs(state: DashboardState | null): Tab[] { @@ -24,9 +67,7 @@ function buildTabs(state: DashboardState | null): Tab[] { tabs.push({ id: 'team', type: 'team', label: 'Team', closable: false }); } - if (state?.architect) { - tabs.push({ id: 'architect', type: 'architect', label: 'Architect', closable: false, terminalId: state.architect.terminalId, persistent: state.architect.persistent }); - } + tabs.push(...buildArchitectTabs(state)); for (const builder of state?.builders ?? []) { tabs.push({ @@ -75,7 +116,16 @@ export function useTabs(state: DashboardState | null) { const urlTabHandled = useRef(false); const tabs = buildTabs(state); - // Handle URL ?tab= parameter on initial load (for deep linking from tower) + // Handle URL ?tab= parameter on initial load (for deep linking from tower). + // + // Spec 761: this hook deliberately does NOT restore the persisted active + // architect into `activeTabId`. Restoring it would cause the desktop right + // pane to blank on reload (every right-pane content section checks + // `activeTab?.type === 'work'`/etc., all of which are false when an + // architect tab is active). The desktop left-pane architect selection + // lives in App.tsx, which reads localStorage independently. The mobile + // tradeoff is small: on reload, the active tab defaults to `'work'` + // rather than restoring the previously-viewed architect. useEffect(() => { if (urlTabHandled.current || state === null) return; @@ -83,37 +133,54 @@ export function useTabs(state: DashboardState | null) { const tabParam = urlParams.get('tab'); if (tabParam) { - // Find matching tab by id or type + // Spec 761: handle the architect: deep-link form. If the named + // architect isn't registered, fall back to the first architect tab + // (matches the bare ?tab=architect behaviour for graceful degradation). + if (tabParam.startsWith('architect:')) { + const name = tabParam.slice('architect:'.length); + const archTab = tabs.find(t => t.type === 'architect' && t.architectName === name) + ?? tabs.find(t => t.type === 'architect'); + if (archTab) { + setActiveTabId(archTab.id); + urlTabHandled.current = true; + const url = new URL(window.location.href); + url.searchParams.delete('tab'); + window.history.replaceState({}, '', url.toString()); + return; + } + } const matchingTab = tabs.find(t => t.id === tabParam || t.type === tabParam); if (matchingTab) { setActiveTabId(matchingTab.id); urlTabHandled.current = true; - // Clean up URL to avoid sticky behavior on refresh const url = new URL(window.location.href); url.searchParams.delete('tab'); window.history.replaceState({}, '', url.toString()); + return; } - } else { - urlTabHandled.current = true; } + urlTabHandled.current = true; }, [tabs, state]); // Auto-switch to genuinely new tabs (created after page load). // Wait for real state (non-null) before seeding known tabs — otherwise the // empty first render seeds with just ['work'], and the second render // (with actual state) treats all existing tabs as "new" and auto-selects them. + // + // Spec 761: the previous skip condition `tab.type !== 'architect'` was + // removed so newly-added architects (via `afx workspace add-architect`) + // auto-focus the same way newly-spawned builders do today. The seed-on- + // non-null-state pattern below still suppresses focus theft on initial load. useEffect(() => { const currentIds = new Set(tabs.map(t => t.id)); if (knownTabIds.current === null) { - // Only seed once we have real tabs (more than just dashboard) if (state !== null) { knownTabIds.current = currentIds; } return; } for (const tab of tabs) { - if (!knownTabIds.current.has(tab.id) && tab.type !== 'architect') { - // Genuinely new tab appeared — switch to it + if (!knownTabIds.current.has(tab.id)) { setActiveTabId(tab.id); } } diff --git a/packages/dashboard/src/lib/architectPersistence.ts b/packages/dashboard/src/lib/architectPersistence.ts new file mode 100644 index 000000000..1d8616cd8 --- /dev/null +++ b/packages/dashboard/src/lib/architectPersistence.ts @@ -0,0 +1,30 @@ +/** + * Spec 761: per-workspace persistence of the active architect tab. + * + * Uses `window.location.pathname` (URL-encoded full workspace path) as the + * key suffix rather than `DashboardState.workspaceName`. The latter is + * `path.basename(workspacePath)` and would collide between workspaces with + * the same basename (e.g. ~/work/codev and ~/personal/codev). + */ + +const KEY_PREFIX = 'codev-active-architect:'; + +function workspaceKey(): string { + return window.location.pathname; +} + +export function readActiveArchitect(): string | null { + try { + return localStorage.getItem(KEY_PREFIX + workspaceKey()); + } catch { + return null; + } +} + +export function writeActiveArchitect(name: string): void { + try { + localStorage.setItem(KEY_PREFIX + workspaceKey(), name); + } catch { + // quota / SSR / private-mode — silently ignore + } +} diff --git a/packages/types/src/api.ts b/packages/types/src/api.ts index 678d05208..6b9987b86 100644 --- a/packages/types/src/api.ts +++ b/packages/types/src/api.ts @@ -9,6 +9,13 @@ // --- Dashboard State (GET /workspace/:path/api/state) --- export interface ArchitectState { + /** + * The architect's stable name (Spec 755). For single-architect workspaces this is + * `'main'`. For sibling-architect workspaces, additional architects are + * `'architect-2'`, `'architect-3'`, or whatever custom name was supplied via + * `afx workspace add-architect --name `. + */ + name: string; port: number; pid: number; terminalId?: string; @@ -45,11 +52,30 @@ export interface Annotation { file: string; port: number; pid: number; - parent: { type: string; id?: string }; + /** + * Optional parent reference. The Tower `/api/state` handler does not populate + * this; the field is reserved for richer client-driven annotation flows that + * may emerge later. Treat as informational only. + */ + parent?: { type: string; id?: string }; } export interface DashboardState { + /** + * Backward-compatible scalar pointer to the dashboard's "default" architect. + * Populated as the architect named `'main'` if present, else the first + * registered architect. Consumers that only need one architect (e.g. older + * VSCode-extension builds) should read this field. + */ architect: ArchitectState | null; + /** + * Full collection of registered architects (Spec 761). The entry whose + * `name === 'main'` is always at index 0 when present; remaining entries + * follow insertion order from Tower's internal map. Empty array means no + * architect is registered. Consumers that need to surface all architects + * (e.g. the dashboard tab strip) should read this field. + */ + architects: ArchitectState[]; builders: Builder[]; utils: UtilTerminal[]; annotations: Annotation[];