diff --git a/codev/plans/1140-afx-workspace-recover-respawne.md b/codev/plans/1140-afx-workspace-recover-respawne.md new file mode 100644 index 000000000..56642aaab --- /dev/null +++ b/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 --resume --protocol

`. 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:` 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 ` 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 = ''"`. + 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. diff --git a/codev/projects/1140-afx-workspace-recover-respawne/status.yaml b/codev/projects/1140-afx-workspace-recover-respawne/status.yaml new file mode 100644 index 000000000..858baa4c2 --- /dev/null +++ b/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 diff --git a/codev/resources/lessons-learned.md b/codev/resources/lessons-learned.md index b5397cc74..be3a68e61 100644 --- a/codev/resources/lessons-learned.md +++ b/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:` 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 `/.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 `-`, 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 ` 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 ` --resume ` 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). diff --git a/codev/reviews/1140-afx-workspace-recover-respawne.md b/codev/reviews/1140-afx-workspace-recover-respawne.md new file mode 100644 index 000000000..87938f967 --- /dev/null +++ b/codev/reviews/1140-afx-workspace-recover-respawne.md @@ -0,0 +1,54 @@ +# PIR Review: Preserve spawnedByArchitect across `afx workspace recover` + +Fixes #1140 + +## Summary + +`afx workspace recover --apply` respawned builders by shelling out to `afx spawn` with an inherited environment, so every recovered builder's `spawned_by_architect` was regenerated from the recovery shell's `CODEV_ARCHITECT_NAME` (typically `main`) instead of being read from its `global.db` row. This PR makes recovery read each builder's recorded architect via the existing `lookupBuilderSpawningArchitect` helper (the same one the anti-spoofing check uses) and force it into the child process env, so sibling-architect workspaces keep per-builder attribution across recovery. Legacy rows with no recorded architect fall back to the caller's env, reproducing the pre-fix behavior for those rows only. + +## Files Changed + +- `packages/codev/src/agent-farm/commands/workspace-recover.ts` (+113 / −30) +- `packages/codev/src/agent-farm/__tests__/workspace-recover.test.ts` (+108 / −1) +- `codev/plans/1140-afx-workspace-recover-respawne.md` (+93 / −0) +- `codev/reviews/1140-afx-workspace-recover-respawne.md` (this file) +- `codev/resources/lessons-learned.md` (+1 line, cold tier) +- `codev/state/pir-1140_thread.md` (builder thread log) +- `codev/projects/1140-afx-workspace-recover-respawne/status.yaml` (porch-managed) + +## Commits + +- `031bca87` [PIR #1140] Plan draft +- `8077b7b0` [PIR #1140] Preserve spawnedByArchitect when workspace recover respawns builders +- `75ee3383` [PIR #1140] Thread: implement phase notes +- `chore(porch)` commits are porch-managed state transitions. + +## Test Results + +- `pnpm --filter @cluesmith/codev build`: ✓ pass +- `pnpm test` (full package suite): ✓ 3443 passed, 48 pre-existing skips, 0 failures +- `workspace-recover.test.ts`: 57/57 pass (11 new: 6 for `deriveBuilderInfoWithArchitect`, 5 for `respawnEnv`) +- Manual verification: human reviewed the worktree at the `dev-approval` gate (approved 2026-07-08); the multi-architect respawn path is additionally pinned by the "preserves distinct attribution across builders in the same workspace" unit test. + +## Architecture Updates + +No arch changes needed. The fix does not move any module boundary or introduce new state; it aligns `workspace-recover.ts` with the already-documented design (attribution lives solely in `global.db`, per the Issue #1118 hot-tier fact) by reusing the existing `lookupBuilderSpawningArchitect` read path instead of adding a second source of truth. + +## Lessons Learned Updates + +One COLD-tier lesson added to `codev/resources/lessons-learned.md` (Architecture section): when a process derives identity from an inherited environment variable, every programmatic respawn/recovery path must explicitly set that variable from recorded state rather than inheriting it, otherwise the respawned process's identity silently becomes a function of who ran the recovery. Not HOT-tier: it is a recipe for a specific bug class, not a behavior-changing cross-cutting rule, and the hot file is at cap. + +## Things to Look At During PR Review + +- **The null-fallback policy in `respawnEnv`**: legacy rows (no recorded architect) return the base env untouched rather than hardcoding `'main'`. This keeps `spawn.ts`'s `DEFAULT_ARCHITECT_NAME` as the single owner of the default. Confirm you agree that legacy-row recoveries attaching to the recovery shell's architect (today's behavior) is the right degradation. +- **The `try`/`finally` restructure in `workspaceRecover`**: the `allRows` construction moved inside the existing DB block so the per-builder architect lookups complete before `closeGlobalDb()`. Behavior is otherwise unchanged, but it is the largest visual diff hunk; verify the connection still closes before any child `afx spawn` launches (it does: the confirm prompt and respawn loop remain outside the block). +- **The three-valued lookup collapse**: `lookupBuilderSpawningArchitect` distinguishes `null` (legacy row) from `undefined` (no row); recovery collapses both to `null` since they demand the same fallback. The distinction remains load-bearing in the message-routing spoofing check and is untouched. + +## How to Test Locally + +- **View diff**: VSCode sidebar → right-click builder pir-1140 → **View Diff** +- **What to verify** (mapped to the plan's Test Plan): + 1. In a workspace with a sibling architect, confirm a builder row with a non-`main` architect: `sqlite3 ~/.agent-farm/global.db "SELECT id, spawned_by_architect FROM builders WHERE workspace_path = ''"` + 2. Kill that builder's shellper (or reboot), run `afx workspace recover --apply` from main's terminal + 3. Re-run the query: the respawned builder retains its original `spawned_by_architect` + 4. From the respawned builder, `afx send architect "ping"` routes to the original architect; the VS Code Agents view shows the correct owner diff --git a/codev/state/pir-1140_thread.md b/codev/state/pir-1140_thread.md new file mode 100644 index 000000000..5dae8bed9 --- /dev/null +++ b/codev/state/pir-1140_thread.md @@ -0,0 +1,25 @@ +# Builder thread: pir-1140 + +PIR strict mode. Issue #1140: `afx workspace recover` respawns builders with the recovery shell's architect instead of their original `spawned_by_architect`. + +## Plan phase + +- Confirmed the root cause exactly as the issue describes: `BuilderInfo` has no architect field, `deriveBuilderInfo` reads only porch state, and `respawnBuilder` spawns `afx spawn` with inherited env so `CODEV_ARCHITECT_NAME` leaks from the operator's shell into the new builder row. +- Key find: `lookupBuilderSpawningArchitect(builderId, workspacePath)` in `state.ts:537` already implements the needed DB read (string / null / undefined three-valued return) and is already unit-tested. The plan reuses it instead of writing new SQL, keeping recover and message-routing on one source of truth. +- Design choices: keep `deriveBuilderInfo` pure; add an injectable `deriveBuilderInfoWithArchitect(state, lookup)` wrapper (matches the existing DI style of `evaluateEligibility`); add a pure exported `respawnEnv` helper so the env construction is unit-testable; widen the existing try/finally so DB reads finish before `closeGlobalDb()` (which today runs before the row-building loop that will now do lookups). +- Null fallback: legacy rows (null/missing `spawned_by_architect`) pass the caller's env through unchanged, reproducing today's behavior for those rows only; no second copy of the `main` default in recover. +- Plan written to `codev/plans/1140-afx-workspace-recover-respawne.md`, committed, pushed. Sitting at `plan-approval` gate. +- Gate approved 2026-07-07 with no changes requested. + +## Implement phase + +- Implemented per plan: `BuilderInfo.spawnedByArchitect` (required, nullable), pure `deriveBuilderInfo` sets null, new injectable `deriveBuilderInfoWithArchitect(state, lookup)` wrapper, new pure `respawnEnv(name, baseEnv)` helper, `respawnBuilder` passes `env: respawnEnv(...)`, and the try/finally widened so all global.db reads (sessions + architect lookups via `lookupBuilderSpawningArchitect`) finish before `closeGlobalDb()`. +- Worktree was greenfield (no node_modules): ran `pnpm install --frozen-lockfile` + built codev-core before the package build would pass. Note for future recover-adjacent builders: build core first. +- 11 new tests (6 wrapper, 5 respawnEnv); updated existing `deriveBuilderInfo` expectations and `makeBuilderInfo` for the new field. Targeted file: 57/57 pass. Build clean. +- Sitting at `dev-approval` gate. +- Gate approved 2026-07-08. During the gate the human probed whether the spawning architect should also be recorded in status.yaml; assessment was no (porch is architect-agnostic, status.yaml is git-committed while architect names are workspace-local runtime identity, and duplicating the fact reintroduces the two-sources bug class this issue fixed). No scope change. + +## Review phase + +- Retrospective written to `codev/reviews/1140-afx-workspace-recover-respawne.md`. No arch-doc changes (fix aligns recover with the documented single-source-of-truth design). One COLD-tier lesson added to `codev/resources/lessons-learned.md` (Architecture): env-derived identity + programmatic re-invocation requires explicitly setting the env from recorded state. +- PR opened; porch ran the single-pass 3-way consultation; waiting at `pr` gate. diff --git a/packages/codev/src/agent-farm/__tests__/workspace-recover.test.ts b/packages/codev/src/agent-farm/__tests__/workspace-recover.test.ts index d392db73c..c7fb5d3bc 100644 --- a/packages/codev/src/agent-farm/__tests__/workspace-recover.test.ts +++ b/packages/codev/src/agent-farm/__tests__/workspace-recover.test.ts @@ -2,7 +2,8 @@ * Tests for `afx workspace recover` — eligibility predicate, builder-info * derivation, worktree resolution, and listAllProjects precedence. * - * Issue #829. + * Issue #829. Architect-attribution preservation (deriveBuilderInfoWithArchitect, + * respawnEnv) is Issue #1140. */ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; @@ -13,6 +14,8 @@ import { join } from 'node:path'; import { evaluateEligibility, deriveBuilderInfo, + deriveBuilderInfoWithArchitect, + respawnEnv, resolveWorktreePath, formatRelativeAge, type EligibilityInputs, @@ -58,7 +61,13 @@ function makeSession(overrides: Partial = {}): DbTerminalSess } function makeBuilderInfo(overrides: Partial = {}): BuilderInfo { - return { builderId: 'builder-spir-87', issueArg: '87', cliProtocol: 'spir', ...overrides }; + return { + builderId: 'builder-spir-87', + issueArg: '87', + cliProtocol: 'spir', + spawnedByArchitect: null, + ...overrides, + }; } function defaults(): Omit { @@ -315,6 +324,7 @@ describe('deriveBuilderInfo', () => { builderId: 'builder-spir-87', issueArg: '87', cliProtocol: 'spir', + spawnedByArchitect: null, }); }); @@ -323,6 +333,7 @@ describe('deriveBuilderInfo', () => { builderId: 'builder-bugfix-693', issueArg: '693', cliProtocol: 'bugfix', + spawnedByArchitect: null, }); }); @@ -331,6 +342,7 @@ describe('deriveBuilderInfo', () => { builderId: 'builder-pir-829', issueArg: '829', cliProtocol: 'pir', + spawnedByArchitect: null, }); }); @@ -339,6 +351,7 @@ describe('deriveBuilderInfo', () => { builderId: 'builder-aspir-438', issueArg: '438', cliProtocol: 'aspir', + spawnedByArchitect: null, }); }); @@ -347,6 +360,7 @@ describe('deriveBuilderInfo', () => { builderId: 'builder-air-501', issueArg: '501', cliProtocol: 'air', + spawnedByArchitect: null, }); }); @@ -360,6 +374,96 @@ describe('deriveBuilderInfo', () => { }); }); +describe('deriveBuilderInfoWithArchitect (Issue #1140)', () => { + it('carries the recorded architect name through to BuilderInfo', () => { + const info = deriveBuilderInfoWithArchitect( + makeState({ id: '0087', protocol: 'spir' }), + () => 'vscode', + ); + expect(info).toEqual({ + builderId: 'builder-spir-87', + issueArg: '87', + cliProtocol: 'spir', + spawnedByArchitect: 'vscode', + }); + }); + + it('passes the derived builderId to the lookup', () => { + const seen: string[] = []; + deriveBuilderInfoWithArchitect(makeState({ id: 'bugfix-693', protocol: 'bugfix' }), (id) => { + seen.push(id); + return 'main'; + }); + expect(seen).toEqual(['builder-bugfix-693']); + }); + + it('keeps null for legacy rows with a NULL spawned_by_architect column', () => { + const info = deriveBuilderInfoWithArchitect(makeState(), () => null); + expect(info?.spawnedByArchitect).toBeNull(); + }); + + it('normalizes undefined (no builders row) to null', () => { + const info = deriveBuilderInfoWithArchitect(makeState(), () => undefined); + expect(info?.spawnedByArchitect).toBeNull(); + }); + + it('preserves distinct attribution across builders in the same workspace', () => { + const byBuilder: Record = { + 'builder-spir-87': 'vscode', + 'builder-pir-829': 'main', + }; + const a = deriveBuilderInfoWithArchitect( + makeState({ id: '0087', protocol: 'spir' }), + (id) => byBuilder[id], + ); + const b = deriveBuilderInfoWithArchitect( + makeState({ id: '0829', protocol: 'pir' }), + (id) => byBuilder[id], + ); + expect(a?.spawnedByArchitect).toBe('vscode'); + expect(b?.spawnedByArchitect).toBe('main'); + }); + + it('returns null for unsupported protocols without invoking the lookup', () => { + let called = false; + const info = deriveBuilderInfoWithArchitect(makeState({ protocol: 'experiment' }), () => { + called = true; + return 'main'; + }); + expect(info).toBeNull(); + expect(called).toBe(false); + }); +}); + +describe('respawnEnv (Issue #1140)', () => { + it('overrides an inherited CODEV_ARCHITECT_NAME with the recorded architect', () => { + const env = respawnEnv('vscode', { CODEV_ARCHITECT_NAME: 'main', PATH: '/usr/bin' }); + expect(env.CODEV_ARCHITECT_NAME).toBe('vscode'); + expect(env.PATH).toBe('/usr/bin'); + }); + + it('sets CODEV_ARCHITECT_NAME even when the base env lacks it', () => { + const env = respawnEnv('vscode', { PATH: '/usr/bin' }); + expect(env.CODEV_ARCHITECT_NAME).toBe('vscode'); + }); + + it('does not mutate the base env when overriding', () => { + const base = { CODEV_ARCHITECT_NAME: 'main' }; + respawnEnv('vscode', base); + expect(base.CODEV_ARCHITECT_NAME).toBe('main'); + }); + + it('passes the base env through unchanged when no architect was recorded', () => { + const base = { CODEV_ARCHITECT_NAME: 'main', PATH: '/usr/bin' }; + expect(respawnEnv(null, base)).toBe(base); + }); + + it('does not invent CODEV_ARCHITECT_NAME for legacy rows when the base env lacks it', () => { + const env = respawnEnv(null, { PATH: '/usr/bin' }); + expect('CODEV_ARCHITECT_NAME' in env).toBe(false); + }); +}); + describe('resolveWorktreePath', () => { let tmp: string; let buildersDir: string; diff --git a/packages/codev/src/agent-farm/commands/workspace-recover.ts b/packages/codev/src/agent-farm/commands/workspace-recover.ts index fc50221ef..555037ae1 100644 --- a/packages/codev/src/agent-farm/commands/workspace-recover.ts +++ b/packages/codev/src/agent-farm/commands/workspace-recover.ts @@ -11,6 +11,7 @@ import { logger } from '../utils/logger.js'; import { buildAgentName, stripLeadingZeros } from '../utils/agent-names.js'; import { processExists, getTerminalSessionsForWorkspace } from '../servers/tower-terminals.js'; import { closeGlobalDb } from '../db/index.js'; +import { lookupBuilderSpawningArchitect } from '../state.js'; import { listAllProjects } from '../../commands/porch/state.js'; import type { ProjectState } from '../../commands/porch/types.js'; import type { DbTerminalSession } from '../servers/tower-types.js'; @@ -131,6 +132,13 @@ export interface BuilderInfo { builderId: string; issueArg: string; cliProtocol: string; + /** + * The architect recorded in global.db as having spawned this builder + * (Issue #1140). Null for legacy / pre-Spec-755 rows with no recorded + * value, or when no builders row exists. `deriveBuilderInfo` is pure and + * always sets null; `deriveBuilderInfoWithArchitect` enriches from the DB. + */ + spawnedByArchitect: string | null; } /** @@ -151,15 +159,38 @@ export function deriveBuilderInfo(state: ProjectState): BuilderInfo | null { builderId: buildAgentName('bugfix', numericId), issueArg: numericId, cliProtocol: 'bugfix', + spawnedByArchitect: null, }; } return { builderId: buildAgentName('spec', state.id, state.protocol), issueArg: stripLeadingZeros(state.id), cliProtocol: state.protocol, + spawnedByArchitect: null, }; } +/** + * Enrich the derived builder info with the spawning-architect name recorded in + * global.db (Issue #1140). Without this, respawned builders inherit the + * recovery shell's CODEV_ARCHITECT_NAME and every builder in a sibling-architect + * workspace gets reattributed to whichever architect ran `workspace recover`. + * + * The lookup is injected so the function stays unit-testable without a real + * database, matching `evaluateEligibility`'s isProcessAlive/socketExists style. + * A three-valued lookup result (string / null legacy row / undefined no row) + * collapses to string-or-null: both null and undefined mean "no recorded + * architect" and fall back to the caller's env at respawn time. + */ +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 }; +} + /** * Resolve the builder's worktree path on disk, handling both the Spec-653 * ID-only layout and the legacy title-suffixed form. @@ -244,6 +275,26 @@ function printPreview(rows: RecoverRow[]): void { } } +/** + * Build the child environment for the respawn invocation (Issue #1140). + * + * spawn.ts derives the new builder row's `spawned_by_architect` from + * CODEV_ARCHITECT_NAME, so the recorded architect must be forced into the + * child env or the respawned builder gets reattributed to the recovery + * shell's architect. When no architect was recorded (legacy rows), the base + * env passes through untouched, which reproduces the pre-fix behavior for + * those rows only (spawn.ts still defaults absent/blank values to 'main'). + */ +export function respawnEnv( + spawnedByArchitect: string | null, + baseEnv: NodeJS.ProcessEnv, +): NodeJS.ProcessEnv { + if (spawnedByArchitect === null) { + return baseEnv; + } + return { ...baseEnv, CODEV_ARCHITECT_NAME: spawnedByArchitect }; +} + async function respawnBuilder(info: BuilderInfo): Promise { // Use the current node binary and CLI entry point so the respawn invocation // matches the install method this command was started under (npm global, @@ -252,7 +303,7 @@ async function respawnBuilder(info: BuilderInfo): Promise { const child = spawn( process.execPath, [process.argv[1], 'spawn', info.issueArg, '--resume', '--protocol', info.cliProtocol], - { stdio: 'inherit' }, + { stdio: 'inherit', env: respawnEnv(info.spawnedByArchitect, process.env) }, ); child.on('error', rejectPromise); child.on('exit', (code) => { @@ -283,37 +334,43 @@ export async function workspaceRecover(options: WorkspaceRecoverOptions = {}): P return; } - let sessions: DbTerminalSession[]; + // All global.db reads (terminal sessions + per-builder architect lookups) + // happen inside this try so the connection is closed before any child + // `afx spawn` process is launched. + let allRows: RecoverRow[]; try { - sessions = getTerminalSessionsForWorkspace(config.workspaceRoot); + const sessions = getTerminalSessionsForWorkspace(config.workspaceRoot); + // role_id has no UNIQUE constraint in the schema, so collect every matching + // row per builder id rather than collapsing to last-write-wins. + const sessionsByRoleId = new Map(); + for (const s of sessions) { + if (s.type !== 'builder' || !s.role_id) continue; + const bucket = sessionsByRoleId.get(s.role_id); + if (bucket) bucket.push(s); + else sessionsByRoleId.set(s.role_id, [s]); + } + + allRows = projects.map(({ state }) => { + const builderInfo = deriveBuilderInfoWithArchitect( + state, + (builderId) => lookupBuilderSpawningArchitect(builderId, config.workspaceRoot), + ); + const matchingSessions = builderInfo ? sessionsByRoleId.get(builderInfo.builderId) ?? [] : []; + const worktreePath = resolveWorktreePath(config.buildersDir, state); + const ageDays = (Date.now() - Date.parse(state.updated_at)) / 86_400_000; + const eligibility = evaluateEligibility({ + state, builderInfo, + sessions: matchingSessions, + worktreeExists: worktreePath !== null, + ageDays, maxAgeDays, includeStale, + isProcessAlive: processExists, + socketExists: existsSync, + }); + return { state, builderInfo, worktreePath, eligibility, ageDays }; + }); } finally { closeGlobalDb(); } - // role_id has no UNIQUE constraint in the schema, so collect every matching - // row per builder id rather than collapsing to last-write-wins. - const sessionsByRoleId = new Map(); - for (const s of sessions) { - if (s.type !== 'builder' || !s.role_id) continue; - const bucket = sessionsByRoleId.get(s.role_id); - if (bucket) bucket.push(s); - else sessionsByRoleId.set(s.role_id, [s]); - } - - const allRows: RecoverRow[] = projects.map(({ state }) => { - const builderInfo = deriveBuilderInfo(state); - const matchingSessions = builderInfo ? sessionsByRoleId.get(builderInfo.builderId) ?? [] : []; - const worktreePath = resolveWorktreePath(config.buildersDir, state); - const ageDays = (Date.now() - Date.parse(state.updated_at)) / 86_400_000; - const eligibility = evaluateEligibility({ - state, builderInfo, - sessions: matchingSessions, - worktreeExists: worktreePath !== null, - ageDays, maxAgeDays, includeStale, - isProcessAlive: processExists, - socketExists: existsSync, - }); - return { state, builderInfo, worktreePath, eligibility, ageDays }; - }); // By default the preview hides projects beyond the recency window — for a // large workspace the stale tail dominates the table and obscures the few