afx workspace recover: preserve spawnedByArchitect on respawned builders

codev/plans/1140-afx-workspace-recover-respawne.md

@@ -0,0 +1,93 @@
+# PIR Plan: Preserve spawnedByArchitect across `afx workspace recover`
+
+## Understanding
+
+`afx workspace recover --apply` respawns dead builders by shelling out to `afx spawn <issue> --resume --protocol <p>`. The child process inherits the recovery shell's environment, so `spawn.ts`'s module-scope constant (`packages/codev/src/agent-farm/commands/spawn.ts:37-38`) resolves `CODEV_ARCHITECT_NAME` from whatever terminal the operator happened to run recovery in, and writes that into the respawned builder's `spawned_by_architect` column. Every recovered builder is therefore reattributed to the recovery shell's architect (typically `main`), breaking:
+
+- `afx send architect` affinity routing from the builder
+- the anti-spoofing check for `architect:<name>` addressing (`tower-messages.ts:213-218`)
+- dashboard / VS Code Agents-view attribution
+- sibling architects' per-team builder lists
+
+Root cause confirmed in the code:
+
+- `BuilderInfo` (`packages/codev/src/agent-farm/commands/workspace-recover.ts:130-134`) carries only `{ builderId, issueArg, cliProtocol }`.
+- `deriveBuilderInfo` (`workspace-recover.ts:144-161`) builds it purely from porch `ProjectState`; the builder's `global.db` row is never consulted.
+- `respawnBuilder` (`workspace-recover.ts:247-263`) spawns the child with `{ stdio: 'inherit' }` and no `env` override, so `CODEV_ARCHITECT_NAME` leaks through from the operator's shell.
+
+Key discovery: the exact DB read we need already exists and is already unit-tested. `lookupBuilderSpawningArchitect(builderId, workspacePath)` in `packages/codev/src/agent-farm/state.ts:537-554` returns the recorded `spawned_by_architect` (`string`), `null` for legacy rows, or `undefined` when no row exists. It is the same helper the Phase 3 affinity resolver uses, so recover and message-routing stay on one source of truth. No new SQL is needed.
+
+## Proposed Change
+
+Three surgical edits to `workspace-recover.ts`, all consistent with the issue's fix sketch:
+
+1. **Extend `BuilderInfo`** with a required `spawnedByArchitect: string | null` field. `deriveBuilderInfo` stays pure and always sets it to `null` (it has no DB access by design; `resolveWorktreePath` also calls it and does not care about attribution).
+
+2. **Add a small injectable wrapper** that enriches the derived info with the DB value:
+
+   ```ts
+   export function deriveBuilderInfoWithArchitect(
+     state: ProjectState,
+     lookupArchitect: (builderId: string) => string | null | undefined,
+   ): BuilderInfo | null {
+     const base = deriveBuilderInfo(state);
+     if (base === null) return null;
+     return { ...base, spawnedByArchitect: lookupArchitect(base.builderId) ?? null };
+   }
+   ```
+
+   `workspaceRecover` calls it with `(id) => lookupBuilderSpawningArchitect(id, config.workspaceRoot)`. The `?? null` collapses `undefined` (no DB row) into the same legacy fallback as a NULL column value. The lookup-as-parameter shape keeps the function unit-testable without a real global.db, matching the existing dependency-injection style of `evaluateEligibility` (`isProcessAlive` / `socketExists` are injected the same way).
+
+   DB lifecycle note: `workspaceRecover` currently closes the global DB in a `finally` immediately after reading terminal sessions (`workspace-recover.ts:286-291`). The row-building loop that calls `deriveBuilderInfo` runs after that close; `getDb()` would lazily reopen the connection and leave it open across the child `afx spawn` processes. To keep the existing close-before-respawn discipline, the `try` block will be widened so both the sessions read and the `allRows` construction (which now performs the per-builder lookups) happen before `closeGlobalDb()` runs in `finally`.
+
+3. **`respawnBuilder` sets the child env explicitly.** A new exported pure helper keeps it testable:
+
+   ```ts
+   export function respawnEnv(
+     spawnedByArchitect: string | null,
+     baseEnv: NodeJS.ProcessEnv,
+   ): NodeJS.ProcessEnv {
+     if (spawnedByArchitect === null) {
+       return baseEnv;
+     }
+     return { ...baseEnv, CODEV_ARCHITECT_NAME: spawnedByArchitect };
+   }
+   ```
+
+   `respawnBuilder` passes `env: respawnEnv(info.spawnedByArchitect, process.env)` to `child_process.spawn`. When the DB has a recorded architect, the child sees exactly that name. When the value is `null` (legacy / pre-Spec-755 rows), the caller's env passes through untouched, which reproduces today's behavior for those rows (they get the recovery shell's architect, and `spawn.ts` still falls back to `main` when the variable is absent or blank). This matches the issue's fallback chain without hardcoding a second copy of the `main` default in recover.
+
+No changes to `afx spawn`, the DB schema, or recovery eligibility semantics. This is package source code, not framework docs, so there is no `codev-skeleton/` mirror to update.
+
+## Files to Change
+
+- `packages/codev/src/agent-farm/commands/workspace-recover.ts`
+  - `:130-134` — add `spawnedByArchitect: string | null` to `BuilderInfo`
+  - `:144-161` — `deriveBuilderInfo` returns `spawnedByArchitect: null` in both branches; new exported `deriveBuilderInfoWithArchitect(state, lookupArchitect)` wrapper directly below it
+  - `:247-263` — new exported `respawnEnv` helper; `respawnBuilder` passes `env: respawnEnv(info.spawnedByArchitect, process.env)`
+  - `:286-316` — widen the `try`/`finally` so the `allRows` construction (now using `deriveBuilderInfoWithArchitect` with a `lookupBuilderSpawningArchitect` closure) happens before `closeGlobalDb()`
+  - import `lookupBuilderSpawningArchitect` from `../state.js`
+- `packages/codev/src/agent-farm/__tests__/workspace-recover.test.ts`
+  - update `makeBuilderInfo` and the `deriveBuilderInfo` expectations for the new field (`spawnedByArchitect: null`)
+  - new `describe('deriveBuilderInfoWithArchitect')`: recorded name is carried through; `null` row stays `null`; `undefined` (no row) normalizes to `null`; unsupported protocol still returns `null` without invoking the lookup
+  - new `describe('respawnEnv')`: recorded name overrides an inherited `CODEV_ARCHITECT_NAME`; recorded name is set even when the base env lacks the variable; `null` returns the base env unchanged (inherited value preserved, and no key invented when absent)
+
+## Risks & Alternatives Considered
+
+- **Risk**: reading `builders` rows for every project adds DB queries to recover. Mitigation: one indexed point-read per revivable project; `workspace recover` is a cold, operator-invoked path. The issue explicitly blesses this cost.
+- **Risk**: widening the `try`/`finally` around `allRows` changes when `closeGlobalDb()` runs. Mitigation: the block still closes before any confirmation prompt or respawn, preserving the original intent (no open handle across child processes); all reads simply move inside it.
+- **Alternative**: have `deriveBuilderInfo` itself query the DB. Rejected: it would break the function's purity, force DB setup into the existing unit tests, and couple `resolveWorktreePath` (which reuses it) to DB state it does not need.
+- **Alternative**: pass `--architect <name>` as a new `afx spawn` flag instead of env. Rejected: out of scope per the issue ("no change to afx spawn"), and `CODEV_ARCHITECT_NAME` is already the established contract between Tower, spawn, and architect terminals.
+- **Alternative**: hardcode `'main'` as the final fallback in `respawnEnv`. Rejected: `spawn.ts` already owns that default via `DEFAULT_ARCHITECT_NAME`; duplicating it in recover creates a second place to keep in sync. Passing the base env through gives identical behavior.
+
+## Test Plan
+
+- **Unit** (`pnpm --filter @cluesmith/codev test -- workspace-recover`, run from the worktree):
+  - `deriveBuilderInfo` existing cases updated for the new field
+  - `deriveBuilderInfoWithArchitect` covers string / null / undefined lookups and the multi-architect scenario (two builders with different recorded architects each keep their own)
+  - `respawnEnv` covers override, set-when-absent, and null-passthrough cases
+- **Manual** (reviewer, at the dev-approval gate):
+  1. In a workspace with a sibling architect (e.g. `main` plus `vscode`), confirm `global.db` has a builder row with `spawned_by_architect = 'vscode'`: `sqlite3 ~/.agent-farm/global.db "SELECT id, spawned_by_architect FROM builders WHERE workspace_path = '<ws>'"`.
+  2. Kill that builder's shellper (or reboot), then run `afx workspace recover --apply` from main's terminal.
+  3. Re-run the same query: the respawned builder must still show `spawned_by_architect = 'vscode'`, not `main`.
+  4. From the respawned builder pane, `afx send architect "ping"` should land on the vscode architect, and the VS Code Agents view should attribute the builder to vscode.
+- **Regression**: full `pnpm --filter @cluesmith/codev test` plus `pnpm build` from the worktree.

codev/projects/1140-afx-workspace-recover-respawne/status.yaml

@@ -0,0 +1,30 @@
+id: '1140'
+title: afx-workspace-recover-respawne
+protocol: pir
+phase: verified
+plan_phases: []
+current_plan_phase: null
+gates:
+  plan-approval:
+    status: approved
+    requested_at: '2026-07-06T04:18:30.795Z'
+    approved_at: '2026-07-07T02:55:40.944Z'
+  dev-approval:
+    status: approved
+    requested_at: '2026-07-07T03:01:57.654Z'
+    approved_at: '2026-07-08T05:07:25.136Z'
+  pr:
+    status: approved
+    requested_at: '2026-07-08T05:19:54.862Z'
+    approved_at: '2026-07-08T05:26:05.719Z'
+iteration: 1
+build_complete: false
+history: []
+started_at: '2026-07-06T04:15:05.820Z'
+updated_at: '2026-07-08T05:26:15.876Z'
+pr_history:
+  - phase: review
+    pr_number: 1156
+    branch: builder/pir-1140
+    created_at: '2026-07-08T05:17:31.553Z'
+pr_ready_for_human: false

codev/resources/lessons-learned.md

@@ -67,6 +67,7 @@ Generalizable wisdom extracted from review documents, ordered by impact. Updated
 ## Architecture
 
 - [From 1144] When activation becomes ambient (`onStartupFinished` runs in every window), any detection heuristic feeding it becomes an **inertness gate** and must be downgraded to an exact, one-sentence-explainable rule. The extension's workspace detector walked ancestors to the filesystem root looking for `codev/`/`.codev/`; harmless while activation implied a codev workspace, but under ambient activation every folder beneath the user's codev-enabled home directory silently inherited that workspace, connected to Tower, and rendered live workspace actions in unrelated projects. The fix was "the opened folder itself is the workspace root, or nothing" (a settings override covers exotic layouts), which also matches the CLI's semantics: afx runs from the workspace root and never marker-walks. Audit convenience heuristics whenever the trigger that constrained them broadens.
+- [From #1140] When a process derives identity from an inherited environment variable, every *programmatic* respawn/recovery path must set that variable explicitly from recorded state, never inherit it. `afx spawn` derives the new builder's `spawned_by_architect` from `CODEV_ARCHITECT_NAME` (correct interactively, because Tower injects it per architect terminal), but `afx workspace recover` re-invoked spawn via `child_process.spawn` with no `env` override, so after any reboot every recovered builder was silently reattributed to whichever architect's shell ran the recovery: affinity routing, the `architect:<name>` anti-spoofing check, and Agents-view ownership all broke. The fix reads the recorded value from `global.db` via the *same* helper the message router uses (`lookupBuilderSpawningArchitect`) and forces it into the child env; rows with no recorded value pass the base env through so the interactive default stays owned by `spawn.ts` alone. Env-derived identity + programmatic re-invocation is a recurring bug shape: the env var is a per-terminal fact, and any automation that fans out from one terminal turns it into a wrong global.
 - [From #1118] When a subsystem's *scope* outgrows its storage *location*, relocate the storage to match — don't patch the scope with a column and leave the location lying. `state.db` was born per-workspace; when Tower became a system-wide singleton its scope became user-global, but the file stayed at a cwd-dependent `<workspace>/.agent-farm/state.db`. Bugfix #826 patched the *symptom* (a `workspace_path` column so cross-workspace rows could coexist in one file) while leaving the *location* wrong, so which file Tower opened still depended on its start-cwd — architect state "disappeared" after any restart from a different directory. The real fix was to move the tables into the already-user-global `global.db` and make `getDb()` return it. Corollary caught mid-implementation: moving a table into a *shared* DB forces you to re-examine its primary key — `builders` was keyed by `id` alone (safe when each workspace had its own file), but ids are `<protocol>-<issueNumber>`, unique per repo yet **reused across repos**, so the shared table needs a composite `(workspace_path, id)` PK (mirroring what #826 did for `architect`) or same-id builders from two repos collide — and that collision is security-relevant (it feeds the `afx send` spoofing check). The issue's "tables move as-is" framing was true for 3 of 4 tables but not the one whose key assumed per-file isolation. Migration technique that generalized: an **upsert-if-newer** engine (`ON CONFLICT DO UPDATE … WHERE excluded.started_at > existing`) is the same code for the empty-target one-off and the non-empty satellite import, and reading legacy source files **defensively** (`PRAGMA table_info` + synthesizing the new key column) handles heterogeneous historical schema versions without running the old migration ladder.
 - [From #832] A recovery mechanism that keys off a process holding a resource *open* (a file descriptor, a lock) is only as sound as that assumption — verify it empirically *before* building on it. #832's first design disambiguated sibling architects (which share one cwd) by correlating each one's process subtree to the session jsonl it "holds open" via `lsof`. But Claude appends to its session jsonl and **closes** it between writes: `lsof -p <pid>` against a live architect showed **zero** jsonl fds, so the correlation never matched — every apparent success was the *unrelated* sole-architect newest-by-mtime fallback. One captured `lsof` (raw data, not reasoning) falsified the whole subsystem in minutes; it had passed unit tests because the tests only exercised the fallback. Corollary: don't delete a *working* self-recovery path to rebuild it as a bridge that can't cover the hard case. The branch had removed #830's sole-architect jsonl resume for `main` and replaced it with a backfill that — because pre-existing siblings are fundamentally unrecoverable (no `--session-id` arg, no open fd, only a jsonl filename with no reliable pid bridge) — could only ever rescue `main`, which the original fallback already did losslessly. Restoring the existing fallback (gated to the unambiguous sole-architect case) and deleting the entire bridge was strictly simpler and lost nothing.
 - [From #929] When abstracting per-CLI (or per-provider) behavior behind a provider interface, audit **every** call site that builds a CLI invocation — including resume/restart paths, not just the obvious fresh-launch path. Spec 591's harness abstraction routed fresh-launch role injection through the provider but left the `--resume` seam reading Claude's session store unconditionally; a codex/gemini architect (or resumed builder) with any stale Claude `.jsonl` then built `<cmd> --resume <claude-uuid>` and shellper restart-looped to death. The bug only fires on the resume branch (fresh launches were already correct), which is exactly why "builders already prove the path" didn't cover it — resume is a *different* call site. Fix: an optional capability (`buildResume`) that returns the resume invocation in both Node-argv and shell-escaped-fragment forms (mirroring `buildRoleInjection`/`buildScriptRoleInjection`), so no CLI-specific string survives at the call site and only the harness that has a cwd-keyed session store opts in (others return null → fresh launch).