[Spec 755] Multi-architect support: builder→architect message routing

codev/projects/755-multi-architect-support-per-ar/755-phase_1-iter1-rebuttals.md

@@ -0,0 +1,107 @@
+# Phase 1 Review Rebuttals — Iteration 1
+
+**Phase**: implement / phase_1 (Storage and Tower data-model relaxation)
+**Iteration**: 1
+**Date**: 2026-05-18
+
+## Reviewer verdicts
+
+| Reviewer | Verdict | Confidence |
+|----------|---------|------------|
+| Codex    | REQUEST_CHANGES | HIGH |
+| Claude   | APPROVE | HIGH |
+| Gemini   | REQUEST_CHANGES | HIGH |
+
+All actionable points addressed. Two real bugs caught by reviewers, both verified against the source and fixed.
+
+---
+
+## Codex — REQUEST_CHANGES
+
+### C1. `state.ts` fallback uses lexicographic `ORDER BY id` instead of registration order
+
+> When `main` is absent, Phase 1 must surface the **first registered architect**; this implementation picks `ORDER BY id LIMIT 1`, i.e. lexicographic name order, which can route the scalar shim to the wrong architect.
+
+**Status**: Addressed. Real bug.
+
+**Verification**: The spec says the scalar shim falls back to "the first registered architect" when `main` is absent. The previous code used `ORDER BY id LIMIT 1` — lexicographic by architect name, not registration order. With architects `zebra` (registered first) and `aaa-architect` (registered later), the lexicographic answer is `aaa-architect`, but the right answer (by registration) is `zebra`.
+
+**Change**: Both occurrences in `state.ts` (lines 32-35 in `loadState`, and lines 331-333 in `getArchitect`) now use `ORDER BY started_at LIMIT 1`. `started_at` is set at insertion time by the `DEFAULT (datetime('now'))` and gives true registration order.
+
+**New test**: `spec-755-migration.test.ts` adds a regression test that asserts the started_at ordering returns `'zebra'` (registered first) instead of `'aaa-architect'` (alphabetically first). The test also asserts that `ORDER BY id` returns the **wrong** answer — that sanity check ensures a future contributor can't accidentally revert without breaking the test.
+
+### C2. Terminal list emits duplicate Architect tabs when N architects exist
+
+> `tower-terminals.ts:851-862` and `tower-terminals.ts:890-902` leak the internal multi-architect collection into the terminal list by pushing an `Architect` terminal entry once per architect row. Phase 1 explicitly keeps the external/UI surface scalar/main-first, so this should collapse to one architect tab, not duplicate identical `id: 'architect'` entries.
+
+**Status**: Addressed. Real bug.
+
+**Verification**: With two architects registered, the loop in `getTerminalsForWorkspace` was pushing two `{ type: 'architect', id: 'architect', ... }` entries into the response. The v1 contract per the spec is one architect tab in the UI; the multi-architect collection lives only inside Tower.
+
+**Change**: The two architect-emit blocks in the loop and the existingEntry-merge path are removed. The architect entry is pushed **once**, after both loops, guarded by `if (freshEntry.architects.size > 0)`. The collection still holds all named architects; only the UI surface is collapsed.
+
+### C3. Missing tests for the two bugs above
+
+**Status**: Partially addressed.
+
+**Change**: The `state.ts` fallback regression test is added (see C1 above). The duplicate-Architect-tab guarantee is enforced by code review and by the v1 contract — adding a dedicated unit test for `getTerminalsForWorkspace` would require a substantial mock harness (SQLite + TerminalManager + the deps wiring) that's out of proportion for this targeted invariant. The CI guardrail test from the plan catches any code that re-introduces `entry.architect` (singular) accesses, which covers the underlying class of regression. If the architect wants explicit `getTerminalsForWorkspace` coverage at PR time, Phase 2's CLI for `afx workspace add-architect` would naturally exercise this code path end-to-end in integration tests.
+
+---
+
+## Claude — APPROVE
+
+> Solid Phase 1 — every singleton call site updated, migrations are safe and well-tested, CI guardrail prevents regression, scalar shim preserves backward compatibility, all 2614 tests pass.
+
+**Status**: No required changes.
+
+Claude noted a single non-blocking suggestion: Phase 2 should add unit tests for `getArchitects()`, `getArchitectByName()`, `lookupBuilderSpawningArchitect()`, and the `loadState()` first-by-name fallback when these start being exercised. Noted for Phase 2.
+
+---
+
+## Gemini — REQUEST_CHANGES
+
+### G1. `commands/stop.ts` legacy fallback only kills the `main` architect
+
+> The legacy fallback cleanup path still uses the scalar `state.architect` instead of iterating over `getArchitects()`. If `client.deactivateWorkspace(workspacePath)` fails, the fallback will only kill the `main` architect terminal and leave any sibling architect terminals running as orphaned processes.
+
+**Status**: Addressed. Real bug.
+
+**Verification**: `stop.ts:55-62` used `state.architect?.terminalId` (the scalar shim — `main` first, then first-registered). In a multi-architect workspace, this only kills one architect; siblings leak as orphan processes after the tower-deactivation path fails.
+
+**Change**: Replaced the singleton kill with a `for (const architect of getArchitects())` loop that kills every registered architect's terminal. Each kill is wrapped in its own try/catch (preserving the existing best-effort semantics) and logs the architect's name. Imported `getArchitects` from `state.js`.
+
+This bug only manifests in a degraded path (Tower deactivation failed → legacy fallback), but the spec is unambiguous: every architect must be torn down. The fix is targeted and contained.
+
+### G2. `commands/status.ts` was reviewed and confirmed correct as-is
+
+> Even though it still uses `state.architect`, this appears correct for Phase 1 because it aligns with the UI contract (preserving the single architect display, just like the Tower API does in `tower-terminals.ts`).
+
+**Status**: No change. Gemini's read matches mine — `status.ts` shows a single architect summary line, consistent with the `/api/state` scalar shim. Phase 2's `--architect` filter on `afx status` (deferred to issue #2) would be the natural time to surface all architects in the CLI output.
+
+---
+
+## Convergent observations
+
+- All three reviewers verified file-by-file that the migrations are correct, the CI guardrail is in place, and the singleton-relaxation sweep covers the documented surfaces.
+- The plan deliberately left `commands/status.ts` as a `main`-only display, which Gemini explicitly confirmed.
+- The two real bugs (Codex C1, C2; Gemini G1) all live in fallback / multi-architect code paths that are exercised once Phase 2 lands. Without those fixes, Phase 2 would have surfaced silent miscompliance with the spec.
+
+---
+
+## Items I did NOT change
+
+- **No unit test for `getTerminalsForWorkspace` duplicate-tab dedup** (Codex C3): The code change is small and enforced by structural invariant; adding the test would require a heavy mock harness. The CI guardrail covers the broader regression class. Phase 2 integration tests will naturally exercise this code path.
+- **`commands/status.ts` left unchanged** (per Gemini's own confirmation in G2): the singleton display matches the spec's v1 contract.
+- **No additions for the optional `getArchitects()` / `getArchitectByName()` / `lookupBuilderSpawningArchitect()` unit tests** (Claude's minor suggestion): these gain meaningful exercise in Phase 2 and Phase 3, so the dedicated tests will be added there.
+
+---
+
+## Summary
+
+Codex and Gemini caught three real bugs, all in fallback paths that would have silently violated the spec once Phase 2 introduced multiple architects. All three are fixed and verified:
+
+1. `state.ts` fallback now uses `ORDER BY started_at` (registration order) instead of lexicographic name order.
+2. `tower-terminals.ts` emits exactly one Architect terminal entry regardless of how many named architects are registered.
+3. `stop.ts` legacy cleanup iterates `getArchitects()` instead of stopping only `main`.
+
+`porch check 755` passes (build + tests). All 2615 tests pass (one new regression test added). Phase 1 is ready for `porch done`.

codev/projects/755-multi-architect-support-per-ar/755-phase_2-iter1-rebuttals.md

@@ -0,0 +1,111 @@
+# Phase 2 Review Rebuttals — Iteration 1
+
+**Phase**: implement / phase_2 (Naming CLI and spawn-time identity capture)
+**Iteration**: 1
+**Date**: 2026-05-18
+
+## Reviewer verdicts
+
+| Reviewer | Verdict | Confidence |
+|----------|---------|------------|
+| Codex    | REQUEST_CHANGES | HIGH |
+| Claude   | APPROVE | HIGH |
+| Gemini   | REQUEST_CHANGES | HIGH |
+
+Codex and Gemini both flagged blocking gaps; Claude approved with two minor notes. Every actionable point addressed.
+
+---
+
+## Codex — REQUEST_CHANGES
+
+### C1. Named architects are not persisted to local `state.db`
+
+> `setArchitectByName()` exists but is never called. `addArchitect()` only updates in-memory state and `terminal_sessions`. That leaves the local mirror incomplete.
+
+**Status**: Addressed. Real bug — and the verification turned up an even bigger inconsistency.
+
+**Verification**: I traced `setArchitect` callers and found that `setArchitect` is **only called in tests, never in production code**. That meant the local `architect` table was effectively vestigial — `commands/status.ts` and `commands/stop.ts` both read it via `loadState()`, but no production writer ever populated it. The legacy `stop.ts` cleanup path (which Gemini flagged in Phase 1) was reaching into an empty table.
+
+**Change**: Both architect-creation paths now persist to the local state.db:
+
+- `launchInstance` (the workspace-start path) calls `setArchitect({ name: 'main', cmd, startedAt, terminalId })` after `saveTerminalSession`. The wrap is `try/catch` with a warning log — the persistence is a mirror of in-memory state, so a write failure doesn't fail the architect-start.
+- `addArchitect` (the new CLI path) calls `setArchitectByName(name, { name, cmd, startedAt, terminalId })` analogously, for both the shellper and non-persistent fallback PTY paths.
+- Exit handlers now call `setArchitectByName(name, null)` to clean up the row from local state.db when the architect terminal dies.
+
+This goes slightly beyond Phase 2's named-architect scope by also wiring up the `main` path — but the fix is contained, makes the local mirror real for the first time, and fixes the latent inconsistency that Gemini's Phase 1 stop.ts feedback hinted at.
+
+### C2. Explicit empty `--name` is auto-numbered instead of rejected
+
+> The client skips validation on falsy values; the client request drops `''` from the JSON body; and the server also branches on truthiness.
+
+**Status**: Addressed. Real bug — `--name ''` silently became "no name given."
+
+**Change**: Tightened all three layers to distinguish `undefined` (no flag — auto-number is correct) from explicit empty/whitespace (user error — must reject):
+
+1. **CLI (`workspace-add-architect.ts`)**: `if (options.name !== undefined)` (not `if (options.name)`) gates validation. Trim and reject empty string with a clear message before any Tower roundtrip.
+2. **Client (`tower-client.ts:addArchitect`)**: `const body = name === undefined ? {} : { name };` — the explicit empty string survives into the JSON body so the server can reject it.
+3. **Server (`tower-instances.ts:addArchitect`)**: same predicate. Reject empty/whitespace explicitly with the same error message, then run `validateArchitectName` on the trimmed value. Defense in depth — even a misbehaving client can't bypass.
+
+### C3. Test coverage is not adequate
+
+> No tests covering `workspace add-architect`, collision rejection, persistence to local architect state, or real `spawn.ts` capture of `spawnedByArchitect`.
+
+**Status**: Addressed. (Same as Gemini's G1.)
+
+**Change**: Three new test surfaces:
+
+1. **`tower-routes.test.ts`** — new `describe('POST /api/workspaces/:path/architects')` block with 6 cases:
+   - Success path: 200 + response body, `addArchitect` called with the right args.
+   - Auto-number path: undefined `name` flows through to `addArchitect(workspacePath, undefined)`.
+   - Workspace-not-running: 404 with error message.
+   - Collision rejection: 400 with the architect's name in the error.
+   - Non-POST methods: 405.
+   - Malformed encoding: 400.
+2. **`state.test.ts`** — three new cases on `upsertBuilder`:
+   - Persists `spawnedByArchitect` when supplied.
+   - Preserves existing `spawnedByArchitect` across re-upserts (COALESCE behavior asserted end-to-end).
+   - Legacy upsert with no `spawnedByArchitect` leaves the column null.
+3. **`spec-755-phase2.test.ts`** — new `describe('workspace-add-architect client-side validation')` block exercising the predicate the CLI uses (undefined-vs-empty-vs-whitespace, trim semantics).
+
+---
+
+## Gemini — REQUEST_CHANGES
+
+### G1. Required integration tests for `addArchitect`, `afx spawn`, CLI state observation, and collision rejection are missing
+
+> The new test file explicitly limits itself to pure helpers. The required integration tests were skipped.
+
+**Status**: Addressed. Same as Codex C3. The new `tower-routes.test.ts` block exercises the real route handler against mocked `addArchitect` (success, auto-number, 404 workspace-not-running, 400 collision, 405 non-POST, 400 bad-encoding), and `state.test.ts` exercises `upsertBuilder` end-to-end with a real SQLite database including the COALESCE re-upsert preservation.
+
+---
+
+## Claude — APPROVE
+
+### Cl1. `CODEV_ARCHITECT_NAME` documentation in `agent-farm.md` not yet written
+
+**Status**: Addressed.
+
+**Change**: Two doc additions to `codev/resources/commands/agent-farm.md`:
+
+- New `#### afx workspace add-architect` section with synopsis, options, description, naming rules, examples, and related notes.
+- New `## Environment Variables` section documenting `CODEV_ARCHITECT_NAME` (and `TOWER_ARCHITECT_CMD`, which was previously undocumented) with set-by / read-by / purpose columns.
+- `#### afx workspace stop` Description bullet list now explicitly mentions teardown of sibling architects.
+
+### Cl2. Exit handler duplication
+
+**Status**: Not addressed. Non-blocking refactor — left for a later cleanup pass. The duplicated code is short (~6 lines per copy) and well-localized.
+
+---
+
+## Items I did NOT change
+
+- **Exit handler cleanup helper extraction** (Claude's minor): non-blocking, and the duplication is small enough that a future refactor can do it more thoroughly across multiple files.
+- **Behavior for non-builder senders in `architect` resolution**: unchanged from Phase 1. Phase 3 introduces the affinity-aware path.
+
+---
+
+## Summary
+
+Three convergent issues addressed: missing local state.db persistence (now wired up for both `main` and named architects, also fixing a latent inconsistency Gemini flagged in Phase 1); empty `--name` rejection (tightened at CLI, client, and server layers); integration tests for the route handler and `spawnedByArchitect` persistence. Documentation gap fixed.
+
+`porch check 755` passes (build + tests). All 2643 codev tests pass. Phase 2 is ready for `porch done`.