diff --git a/codev/projects/271-safety-adjacent-an-architect-c/status.yaml b/codev/projects/271-safety-adjacent-an-architect-c/status.yaml new file mode 100644 index 000000000..30baab20f --- /dev/null +++ b/codev/projects/271-safety-adjacent-an-architect-c/status.yaml @@ -0,0 +1,19 @@ +id: '271' +title: safety-adjacent-an-architect-c +protocol: air +phase: pr +plan_phases: [] +current_plan_phase: null +gates: + pr: + status: pending +iteration: 1 +build_complete: false +history: [] +started_at: '2026-08-31T15:42:55.843Z' +updated_at: '2026-08-31T16:51:31.821Z' +pr_history: + - phase: implement + pr_number: 274 + branch: builder/air-271 + created_at: '2026-08-31T16:24:29.278Z' diff --git a/codev/state/air-271_thread.md b/codev/state/air-271_thread.md new file mode 100644 index 000000000..dcd0ef71d --- /dev/null +++ b/codev/state/air-271_thread.md @@ -0,0 +1,82 @@ +# air-271 — architect created by afx lands with codev_role empty + +Issue #271. AIR, strict mode. + +## What the investigation actually found + +The role is **not** lost anywhere in the merged tree. Every layer the architect named +forwards it, and I verified the whole chain end to end against a live fork server. + +The thing running on hardware was a **stale global install**: + +``` +/Users/chris/.nvm/versions/node/v20.19.2/lib/node_modules/@cluesmith/codev + dist/agent-farm/porch-thread-engine.js forwards roleContent/roleFilePath only, never input.role +/Users/chris/.nvm/.../@cluesmith/porch-driver + dist/thread.js no role on the thread.create payload at all +``` + +Both are pre-spec-250 builds carrying version 3.3.1, the same version as the source, so +nothing about the version string says they are behind. `pnpm -w run local-install` is the +step that was missed after #266 merged. + +The decisive evidence is the event, not the projection: + +``` +sqlite3 ~/.t3/dev/state.sqlite "select payload_json from orchestration_events where event_type='thread.created'" +{"threadId":"2e2bd2c7-...","title":"architect-lan",...,"role":null,"parentThreadId":null,...} +``` + +`role: null` in the event means the decider was handed a command with no role — the client +never sent one. The server, the event payload and the projection writer are all innocent; +all three were read and all three carry it. + +## Two corrections to the report + +**The architect WAS registered.** `global.db` has it, written 24 ms after the thread: + +``` +/Users/chris/dev/codev-1455|lan|0|0||2026-08-31T15:37:09.219Z|||2e2bd2c7-...|claude|claude-haiku-4-5 +``` + +So `createArchitectThread` returned and `setArchitectByName` ran. What could not see it was +`afx status`, which built its Architects section entirely from Tower's terminal list — +and a thread-backed architect has no terminal, by definition. Registered and invisible, +which reads from outside exactly like a command that did nothing. + +**The 2-minute hang is a third fault, not the same one.** The thread branch of +`workspace-add-architect` returned without `closeThreadBackend`, so the open WebSocket kept +the event loop alive and the process never exited. `afx interrupt` already carries this fix +with a comment describing the identical symptom. + +## What shipped + +1. `issue-271-architect-role-live.e2e.test.ts` — drives `createArchitectThread` against a + `start-fork` server and reads `codev_role` out of the server's own `projection_threads`. + Nothing in it writes `role` on a payload. Verified failable: removing the forwarding in + `porch-thread-engine.ts` reproduces `role: null` exactly, the hardware symptom. +2. `closeThreadBackend` in a `finally` in `workspace-add-architect`, so the command exits. + Ordering asserted (register, then close) — closing first would exit a process whose + registration had not landed. +3. `afx status` lists thread-backed architects from state, de-duplicated against the ones + Tower already listed, with `threadId` added to the `--json` payload as + nullable-not-optional. + +All six new assertions were confirmed to fail with their fix reverted. + +## Notes for whoever runs this next + +- The fork checkout `/Users/chris/dev/t3code-codev` had an uncommitted `tools/lan-serve.mjs`, + and `t3-server.mjs start-fork` refuses a dirty checkout. I parked it, ran, and restored it + byte-identical. It is untracked work someone will want; do not delete it. A run of the live + e2e test will skip with a reason while it sits there. +- The live test needs `T3_NODE` pointing at a Node 22 binary and `T3_HARNESS_PORT`. It skips + with a stated reason otherwise; it never passes for want of a server. +- `~/.t3/dev` is the fork's `vp dev` server, not the harness's. The harness uses + `tools/t3-server/.runtime/data/userdata/state.sqlite` and starts on empty data. + +## Left out, deliberately + +Nothing in a new build can stop an OLD build from dropping a field silently — the old code +is what runs. Making a stale install detectable (a build-provenance check on `afx`) is real +and is a different issue; raised with the architect rather than grown into this one. diff --git a/packages/codev/src/agent-farm/__tests__/issue-271-architect-role-live.e2e.test.ts b/packages/codev/src/agent-farm/__tests__/issue-271-architect-role-live.e2e.test.ts new file mode 100644 index 000000000..699232d3e --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/issue-271-architect-role-live.e2e.test.ts @@ -0,0 +1,180 @@ +/** + * Issue #271 — the role an architect is created with, read back out of the + * server's own projection. + * + * ## Why this test exists at all + * + * Every hierarchy assertion spec 250 shipped used SEEDED threads: a fixture + * dispatched `thread.create` with `role` on it, or wrote `codev_role` into + * `projection_threads` directly. Both prove the projector stores what it is + * handed. Neither proves that the command a human actually runs — + * `afx workspace add-architect` — hands it anything. + * + * It does not. On real hardware the thread was created and `codev_role` came back + * empty, so t3code's sidebar drew an ordinary thread and nothing could ever nest + * under it. + * + * So this test drives the PRODUCTION entry point, `createArchitectThread`, against + * a live fork server, and then reads the column out of the server's own SQLite + * file the way the issue's reproducer does. Nothing here writes `role` on a + * payload; if a layer between `createArchitectThread` and `projection_threads` + * drops it, this fails. + * + * ## Why it reads SQLite rather than an RPC + * + * The projection row is what the sidebar renders from, and it is the thing that + * was observed empty. A query that went back through the server's read path could + * be satisfied by a value the server still had in memory. The file is the end of + * the line. + * + * ## Unavailable is a SKIP, never a pass + * + * The fork server needs `T3_NODE` and a clean fork checkout. When it cannot + * start, this run has learned nothing about the fix — and "I could not tell" must + * not be spelled like "no". Each bail-out names its own reason. + */ + +import { describe, it, expect, afterAll } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { + mintPairingCredential, + startForkServer, + stopForkStack, + type ForkStackReady, +} from '../../__tests__/e2e/spec-250-fork-stack.js'; +import { closeThreadBackend, ensureThreadBackendReady } from '../thread-backend.js'; +import { createArchitectThread } from '../thread-runtime.js'; + +const here = dirname(fileURLToPath(import.meta.url)); +const repoRoot = resolve(here, '..', '..', '..', '..', '..'); + +/** + * The server's projection database. + * + * `t3-server.mjs` starts the fork with `--base-dir /data`, and the + * server puts its state under `userdata/`. Named here rather than discovered so a + * moved file fails loudly instead of matching some other database. + */ +function projectionDbPath(): string { + const runtimeDir = process.env.T3_HARNESS_DIR ?? join(repoRoot, 'tools/t3-server/.runtime'); + return resolve(runtimeDir, 'data/userdata/state.sqlite'); +} + +/** + * One column of one row, read out of the running server's file. + * + * Read-only and through `sqlite3`, so nothing here can write to a database a live + * server owns, and the WAL the server is holding open is read the same way the + * issue's reproducer read it. + */ +function projectionRole(threadId: string): { present: boolean; role: string | null } { + // Refused rather than escaped. `sqlite3` takes one SQL string and no bound + // parameters, so the id is interpolated — and an interpolation that quietly + // accepts anything is the shape someone copies into a query where the value is + // not a server-minted UUID. A thread id that is not one is a bug in the caller, + // and it stops here. + if (!/^[0-9a-fA-F-]{36}$/.test(threadId)) { + throw new Error(`refusing to query for a thread id that is not a UUID: ${JSON.stringify(threadId)}`); + } + const out = execFileSync( + 'sqlite3', + [ + `file:${projectionDbPath()}?mode=ro`, + '-cmd', + '.timeout 5000', + // Two markers, because "no row" and "a row whose role is NULL" are + // different failures and a single string could not tell them apart. + `select 'ROW', coalesce(codev_role, 'NULL-ROLE') from projection_threads ` + + `where thread_id = '${threadId}';`, + ], + { encoding: 'utf8' }, + ).trim(); + if (out === '') return { present: false, role: null }; + const [, role] = out.split('|'); + return { present: true, role: role === 'NULL-ROLE' ? null : (role ?? null) }; +} + +/** A workspace that is a real git repository, because the backend registers one. */ +function scratchWorkspace(): string { + const ws = mkdtempSync(join(tmpdir(), 'issue-271-')); + execFileSync('git', ['init', '-q'], { cwd: ws }); + mkdirSync(join(ws, '.codev'), { recursive: true }); + writeFileSync(join(ws, '.codev', 'config.json'), '{}\n'); + return ws; +} + +let workspace: string | undefined; + +/** + * The env this test sets, captured so it can be put back. + * + * `CODEV_T3_URL` and its siblings are read by `readThreadBackendConfig` for EVERY + * workspace, so leaving them set points the rest of the run at a server this test + * has already stopped. The suite is sequential and this file happened to run + * last; that is a property of the schedule, not a guarantee. + */ +const CODEV_T3_KEYS = ['CODEV_T3_URL', 'CODEV_T3_TOKEN', 'CODEV_T3_MODEL'] as const; +const savedEnv = new Map( + CODEV_T3_KEYS.map((key) => [key, process.env[key]]), +); + +afterAll(() => { + if (workspace !== undefined) { + closeThreadBackend(workspace); + rmSync(workspace, { recursive: true, force: true }); + } + for (const [key, value] of savedEnv) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + stopForkStack(); +}); + +describe('issue 271: an architect created by the production path carries its role', () => { + it( + 'writes codev_role="architect" into projection_threads', + { timeout: 300_000 }, + async () => { + const server = await startForkServer(); + if (!server.available) { + // A skip that says why. See the header: this is not a pass. + console.warn(`[issue-271] skipped: ${server.reason}`); + return; + } + const stack: ForkStackReady = { ...server, webUrl: '' }; + + // A FRESH single-use pairing credential, which is what the backend's + // `bootstrapToken` is in production against this server — the harness's own + // start token was already spent on `stack.accessToken`. + const bootstrapToken = await mintPairingCredential(stack); + + workspace = scratchWorkspace(); + process.env.CODEV_T3_URL = stack.serverBase; + process.env.CODEV_T3_TOKEN = bootstrapToken; + // `thread.create` requires `modelSelection`, so a workspace with no + // `threads.model` cannot create a thread at all. Production reads this from + // config; the env override is the same field by another layer. + process.env.CODEV_T3_MODEL = 'gpt-5.6-luna'; + + const installed = await ensureThreadBackendReady(workspace); + expect(installed, 'the thread backend did not install, so nothing below was exercised') + .toBe('installed'); + + const threadId = await createArchitectThread({ name: 'lan', workspaceRoot: workspace }); + expect(typeof threadId, 'createArchitectThread returned no thread id').toBe('string'); + + const row = projectionRole(threadId); + expect(row.present, `no projection row for thread ${threadId}`).toBe(true); + expect( + row.role, + 'the architect reached projection_threads with no role — issue #271. The sidebar ' + + 'renders this as an ordinary thread and nothing can nest under it.', + ).toBe('architect'); + }, + ); +}); diff --git a/packages/codev/src/agent-farm/__tests__/issue-271-architect-role-seam.test.ts b/packages/codev/src/agent-farm/__tests__/issue-271-architect-role-seam.test.ts new file mode 100644 index 000000000..064ce3ef6 --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/issue-271-architect-role-seam.test.ts @@ -0,0 +1,88 @@ +/** + * Issue #271 — `createArchitectThread` names the role at the seam. + * + * The live test in `issue-271-architect-role-live.e2e.test.ts` proves the whole + * chain, and it skips without `T3_NODE` and a clean fork checkout — so CI never + * runs it. A guard that cannot run reads exactly like a guard that passed, which + * is the shape of the failure this issue is about, so the same regression is + * pinned here as well by a test that runs everywhere. + * + * This is the ONE seam neither test can substitute for the other on. The live + * test cannot say WHERE a lost role was lost; this one says nothing about + * whether the server stores it. Both, or neither is enough. + * + * The engine is a fake registered through `setThreadEngine` — the same + * registration production uses — rather than a hand-built call to `engine.create`. + * A test that called the engine itself would be asserting its own argument. + */ + +import { describe, it, expect, afterEach } from 'vitest'; + +import { + clearThreadEngines, + createArchitectThread, + setThreadEngine, + type ThreadEngine, +} from '../thread-runtime.js'; + +type CreateInput = Parameters[0]; + +function recordingEngine(): { engine: ThreadEngine; creates: CreateInput[] } { + const creates: CreateInput[] = []; + const engine = { + defaults: { harness: 'claude', model: 'claude-opus-5' }, + async create(input: CreateInput) { + creates.push(input); + return 'thr-architect-1'; + }, + async attach() { throw new Error('attach is not part of this seam'); }, + async startTurn() { throw new Error('startTurn is not part of this seam'); }, + async recoverTurn() { throw new Error('recoverTurn is not part of this seam'); }, + async interrupt() { throw new Error('interrupt is not part of this seam'); }, + } as unknown as ThreadEngine; + return { engine, creates }; +} + +afterEach(() => { + clearThreadEngines(); +}); + +describe('issue 271: createArchitectThread hands the engine a role', () => { + it("passes role: 'architect' through to engine.create", async () => { + const { engine, creates } = recordingEngine(); + setThreadEngine(engine, '/ws'); + + const threadId = await createArchitectThread({ name: 'lan', workspaceRoot: '/ws' }); + + expect(threadId).toBe('thr-architect-1'); + expect(creates).toHaveLength(1); + // The assertion the issue is about. Without it the thread is created, the row + // is written, the command succeeds — and t3code's sidebar draws an ordinary + // thread that nothing can nest under. + expect(creates[0]?.role).toBe('architect'); + }); + + /** + * `parentThreadId` stays ABSENT, not null. An architect has no parent, and the + * server refuses a parent on a non-builder — so a null here would be a value + * the create path cannot carry, not a tidier way to say "none". + */ + it('names no parent for an architect', async () => { + const { engine, creates } = recordingEngine(); + setThreadEngine(engine, '/ws'); + + await createArchitectThread({ name: 'lan', workspaceRoot: '/ws' }); + + expect(creates[0]).not.toHaveProperty('parentThreadId'); + }); + + it('creates the thread at the workspace root, which is what makes it an architect', async () => { + const { engine, creates } = recordingEngine(); + setThreadEngine(engine, '/ws'); + + await createArchitectThread({ name: 'lan', workspaceRoot: '/ws' }); + + expect(creates[0]?.worktreePath).toBe('/ws'); + expect(creates[0]?.builderId).toBe('architect-lan'); + }); +}); diff --git a/packages/codev/src/agent-farm/__tests__/issue-271-status-thread-architect.test.ts b/packages/codev/src/agent-farm/__tests__/issue-271-status-thread-architect.test.ts new file mode 100644 index 000000000..5a9e58f5f --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/issue-271-status-thread-architect.test.ts @@ -0,0 +1,238 @@ +/** + * Issue #271 — `afx status` can see a thread-backed architect. + * + * `afx workspace add-architect --name lan` created the thread, wrote the row, and + * printed its success line. `afx status` then listed only the terminal-backed + * `main`, which read as "the command did nothing" — the report on the issue says + * exactly that, and it was wrong: the architect was registered the whole time. + * + * The cause is that the Architects section was built entirely from Tower's + * terminal list, and a thread-backed architect has no terminal. Nothing was + * missing from state; nothing looked there. + * + * The assertions below are on the ARCHITECTS section, not on the fact that some + * line somewhere mentions the name. A run whose builder table happened to name + * `lan` would satisfy a looser check while the section stayed empty. + */ + +import { describe, it, expect, beforeEach, vi } from 'vitest'; + +const mockLoadState = vi.fn(); +const mockIsRunning = vi.fn(); +const mockGetHealth = vi.fn(); +const mockGetWorkspaceStatus = vi.fn(); +const mockLoggerInfo = vi.fn(); +const mockLog = vi.fn(); + +vi.mock('../utils/config.js', () => ({ + getConfig: vi.fn(() => ({ workspaceRoot: '/fake/workspace' })), +})); + +vi.mock('../state.js', () => ({ + loadState: (...args: any[]) => mockLoadState(...args), +})); + +vi.mock('../lib/tower-client.js', () => ({ + getTowerClient: () => ({ + isRunning: (...a: any[]) => mockIsRunning(...a), + getHealth: (...a: any[]) => mockGetHealth(...a), + getWorkspaceStatus: (...a: any[]) => mockGetWorkspaceStatus(...a), + getOverview: async () => null, + }), +})); + +vi.mock('../../lib/config.js', () => ({ + loadConfig: vi.fn(() => ({})), +})); + +vi.mock('../utils/logger.js', () => ({ + logger: { + header: vi.fn(), + success: vi.fn(), + error: vi.fn(), + warn: vi.fn(), + debug: vi.fn(), + info: (...args: any[]) => mockLoggerInfo(...args), + kv: vi.fn(), + blank: vi.fn(), + row: vi.fn(), + }, + fatal: vi.fn((msg: string) => { throw new Error(msg); }), +})); + +import { status } from '../commands/status.js'; + +// eslint-disable-next-line no-control-regex +const stripAnsi = (s: string) => s.replace(/\[[0-9;]*m/g, ''); + +/** + * The lines under the `Architects:` heading, up to the next heading. + * + * Scoped deliberately. Reading the whole log would let a name printed anywhere + * else stand in for a section that never rendered. + */ +function architectSection(): string[] { + const lines = mockLoggerInfo.mock.calls.map((c: any[]) => stripAnsi(String(c[0]))); + const start = lines.indexOf('Architects:'); + if (start === -1) return []; + const rest = lines.slice(start + 1); + const end = rest.findIndex((line) => !line.startsWith(' ')); + return end === -1 ? rest : rest.slice(0, end); +} + +function architectTerminal(name: string) { + return { + id: `architect:${name}`, + terminalId: `term-${name}`, + type: 'architect', + label: name, + architectName: name, + pid: 4242, + active: true, + }; +} + +describe('issue 271: afx status shows thread-backed architects', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockIsRunning.mockResolvedValue(true); + mockGetHealth.mockResolvedValue({ ok: true }); + }); + + it('lists an architect that has a thread and no terminal', async () => { + mockGetWorkspaceStatus.mockResolvedValue({ + name: 'codev-1455', + active: true, + terminals: [architectTerminal('main')], + }); + mockLoadState.mockReturnValue({ + architect: null, + architects: [ + { name: 'main', cmd: 'claude', startedAt: 'now', terminalId: 'term-main' }, + { + name: 'lan', + cmd: '', + startedAt: 'now', + threadId: '2e2bd2c7-3ae7-4582-9d60-673da525a93f', + harness: 'claude', + model: 'claude-haiku-4-5', + }, + ], + builders: [], + utils: [], + annotations: [], + }); + + await status(); + + const section = architectSection(); + expect(section.some((line) => line.includes('main'))).toBe(true); + const lan = section.find((line) => line.includes('lan')); + expect(lan, 'the thread-backed architect is absent from the Architects section').toBeDefined(); + expect(lan).toContain('thread=2e2bd2c7-3ae7-4582-9d60-673da525a93f'); + expect(lan).toContain('model=claude-haiku-4-5'); + // A thread has no pid and no port. Printing `pid=?` would report a value this + // command failed to read, when there was never one to read. + expect(lan).not.toContain('pid='); + }); + + /** + * The workspace whose ONLY architect is thread-backed. The section used to be + * nested inside a check on the terminal list, so this case printed nothing at + * all — the emptier the workspace, the more complete the silence. + */ + it('renders the section when there are no architect terminals at all', async () => { + mockGetWorkspaceStatus.mockResolvedValue({ name: 'codev-1455', active: true, terminals: [] }); + mockLoadState.mockReturnValue({ + architect: null, + architects: [{ name: 'lan', cmd: '', startedAt: 'now', threadId: 'thr-1' }], + builders: [], + utils: [], + annotations: [], + }); + + await status(); + + expect(architectSection()).toEqual([expect.stringContaining('lan')]); + }); + + /** + * An architect Tower already listed must not appear twice. Tower is the source + * for a terminal-backed one; state carries a row for it too. + */ + it('does not print an architect twice when Tower already listed it', async () => { + mockGetWorkspaceStatus.mockResolvedValue({ + name: 'codev-1455', + active: true, + terminals: [architectTerminal('main')], + }); + mockLoadState.mockReturnValue({ + architect: null, + // A row carrying BOTH is the dual-identity state the codebase forbids; it is + // used here because a duplicate can only be produced by one, and this test + // is about the de-duplication rather than about how the row got that way. + architects: [{ name: 'main', cmd: 'claude', startedAt: 'now', terminalId: 'term-main', threadId: 'thr-1' }], + builders: [], + utils: [], + annotations: [], + }); + + await status(); + + expect(architectSection().filter((line) => line.includes('main'))).toHaveLength(1); + }); + + /** + * The Tower-down fallback renders the same rows through different code. It + * printed `cmd= started=…` for a thread-backed architect — `cmd` is empty + * because there is no process — so the two paths disagreed about the same row: + * one named the thread, the other named nothing. + */ + it('names the thread in the Tower-down fallback too, instead of an empty cmd', async () => { + mockIsRunning.mockResolvedValue(false); + mockLoadState.mockReturnValue({ + architect: null, + architects: [ + { name: 'main', cmd: 'claude', startedAt: 'T0', terminalId: 'term-main' }, + { name: 'lan', cmd: '', startedAt: 'T0', threadId: 'thr-1' }, + ], + builders: [], + utils: [], + annotations: [], + }); + + await status(); + + const lines = mockLoggerInfo.mock.calls.map((c: any[]) => stripAnsi(String(c[0]))); + const lan = lines.find((line) => line.includes('lan:')); + expect(lan, 'the thread-backed architect is absent from the Tower-down listing').toBeDefined(); + expect(lan).toContain('thread=thr-1'); + expect(lan).not.toContain('cmd='); + // The PTY-backed row is unchanged: it has a command, and that is what it says. + expect(lines.find((line) => line.includes('main:'))).toContain('cmd=claude'); + }); + + it('carries threadId into the --json payload, null for a PTY-backed architect', async () => { + mockGetWorkspaceStatus.mockResolvedValue({ name: 'codev-1455', active: true, terminals: [] }); + mockLoadState.mockReturnValue({ + architect: null, + architects: [ + { name: 'main', cmd: 'claude', startedAt: 'now', terminalId: 'term-main' }, + { name: 'lan', cmd: '', startedAt: 'now', threadId: 'thr-1' }, + ], + builders: [], + utils: [], + annotations: [], + }); + const spy = vi.spyOn(console, 'log').mockImplementation((...args: any[]) => { mockLog(...args); }); + + await status({ json: true }); + + spy.mockRestore(); + const payload = JSON.parse(String(mockLog.mock.calls[0]?.[0])); + expect(payload.architects).toEqual([ + { name: 'main', threadId: null }, + { name: 'lan', threadId: 'thr-1' }, + ]); + }); +}); diff --git a/packages/codev/src/agent-farm/__tests__/spec-146-phase-9-add-architect-thread-path.test.ts b/packages/codev/src/agent-farm/__tests__/spec-146-phase-9-add-architect-thread-path.test.ts index b201340a7..7444002b2 100644 --- a/packages/codev/src/agent-farm/__tests__/spec-146-phase-9-add-architect-thread-path.test.ts +++ b/packages/codev/src/agent-farm/__tests__/spec-146-phase-9-add-architect-thread-path.test.ts @@ -16,6 +16,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; const ensureThreadBackendReady = vi.fn(); +const closeThreadBackend = vi.fn(); const createArchitectThread = vi.fn(); const tryGetThreadEngine = vi.fn(); const architectThreadDefaults = vi.fn(); @@ -24,6 +25,7 @@ const addArchitect = vi.fn(); vi.mock('../thread-backend.js', () => ({ ensureThreadBackendReady: (...args: unknown[]) => ensureThreadBackendReady(...args), + closeThreadBackend: (...args: unknown[]) => closeThreadBackend(...args), })); vi.mock('../thread-runtime.js', () => ({ @@ -158,6 +160,40 @@ describe('workspace add-architect — the thread path is reachable in a fresh pr expect(addArchitect).toHaveBeenCalledWith('/ws', 'uiv2'); }); + /** + * Issue #271. The command hung past two minutes on real hardware, having + * already created the thread AND written the row: an open WebSocket keeps the + * event loop alive, and nothing closed it. + * + * Asserted AFTER `setArchitectByName`, not merely "was called". Closing the + * socket before the row is written would exit a process whose registration had + * not landed, which is a worse bug than the hang it replaces. + */ + it('closes the thread backend after registering, so the process can exit', async () => { + threadEngineInstalled(); + const order: string[] = []; + setArchitectByName.mockImplementation(() => { order.push('register'); }); + closeThreadBackend.mockImplementation(() => { order.push('close'); }); + + await workspaceAddArchitect({ name: 'uiv2' }); + + expect(closeThreadBackend).toHaveBeenCalledWith('/ws'); + expect(order).toEqual(['register', 'close']); + }); + + /** + * A create that throws must still close. Otherwise the failure path is the + * hang: an error printed, and a process that never returns to print it from. + */ + it('closes the thread backend even when the create fails', async () => { + threadEngineInstalled(); + createArchitectThread.mockRejectedValue(new Error('server refused')); + + await expect(workspaceAddArchitect({ name: 'uiv2' })).rejects.toThrow('server refused'); + + expect(closeThreadBackend).toHaveBeenCalledWith('/ws'); + }); + /** * A server that was named and could not be reached must not fall through to * Tower. `ensureThreadBackendReady` throws for exactly this reason, and diff --git a/packages/codev/src/agent-farm/commands/status.ts b/packages/codev/src/agent-farm/commands/status.ts index 8222970ff..65abc2d9a 100644 --- a/packages/codev/src/agent-farm/commands/status.ts +++ b/packages/codev/src/agent-farm/commands/status.ts @@ -221,7 +221,7 @@ function emitStatusJson(params: { // must still emit `"name": null` so the machine-readable contract is stable // for tooling — `JSON.stringify` would otherwise drop an `undefined` key. workspace: { path: string; name: string | null; active: boolean }; - architects: Array<{ name: string }>; + architects: Array<{ name: string; threadId?: string | null }>; builders: Builder[]; ownerFilter: string | undefined; // Issue #1227: null (not omitted) when Tower is down or the running Tower @@ -245,7 +245,11 @@ function emitStatusJson(params: { orphans, ptyDrain, ownerFilter: ownerFilter ?? null, - architects: architects.map((a) => ({ name: a.name ?? 'main' })), + // Issue #271: `threadId` is nullable-not-optional, the same contract as + // `workspace.name` above — a PTY-backed architect emits `"threadId": null` + // rather than dropping the key, so tooling can tell the two backings apart + // instead of inferring one from a missing field. + architects: architects.map((a) => ({ name: a.name ?? 'main', threadId: a.threadId ?? null })), builders: visible.map((b) => ({ id: b.id, name: b.name, @@ -374,42 +378,64 @@ export async function status(options: StatusOptions = {}): Promise { logger.kv('PTY drain', ptyDrain === 0 ? chalk.gray('0') : String(ptyDrain)); renderOrphans(orphans, sized); - if (workspaceStatus.terminals.length > 0) { - // Spec 786 Phase 5: enumerate architects explicitly first, so users see - // ALL registered architects (not just one collapsed "Architect" row). - // Each architect entry's `architectName`, `pid`, and optional `port` - // come from the Tower API (per Spec 786 Phase 5's TowerWorkspaceStatus - // extension). Spec 1057: builders move to their own owner-aware section - // below; shells/dev remain in the general Terminals list. - const architectTerminals = workspaceStatus.terminals.filter(t => t.type === 'architect'); - const otherTerminals = workspaceStatus.terminals.filter( - t => t.type !== 'architect' && t.type !== 'builder', - ); - - if (architectTerminals.length > 0) { - logger.blank(); - logger.info('Architects:'); - for (const term of architectTerminals) { - const name = term.architectName || term.label; - const pid = term.pid ? `pid=${term.pid}` : 'pid=?'; - const port = term.port ? ` port=${term.port}` : ''; - // Spec 786 Phase 5: prefer `terminalId` (the actual PtySession id) - // over `id` (the Spec 761 tab identifier, e.g. `architect` or - // `architect:`). Falls back to `id` for older Tower versions - // that haven't shipped the Phase 5 extension yet. - const termIdValue = term.terminalId ?? term.id; - const termId = ` terminal=${termIdValue}`; - logger.info(` ${chalk.cyan(name)} (${pid}${port}${termId})`); - } + // Spec 786 Phase 5: enumerate architects explicitly first, so users see + // ALL registered architects (not just one collapsed "Architect" row). + // Each architect entry's `architectName`, `pid`, and optional `port` + // come from the Tower API (per Spec 786 Phase 5's TowerWorkspaceStatus + // extension). Spec 1057: builders move to their own owner-aware section + // below; shells/dev remain in the general Terminals list. + const architectTerminals = workspaceStatus.terminals.filter(t => t.type === 'architect'); + const otherTerminals = workspaceStatus.terminals.filter( + t => t.type !== 'architect' && t.type !== 'builder', + ); + + // Issue #271. A thread-backed architect has NO Tower terminal — that is + // what being thread-backed means — so a section built only from the + // terminal list could never show one. `afx workspace add-architect` wrote + // its row, `add-architect` printed its success line, and `afx status` then + // reported only the terminal-backed `main`: registered and invisible, which + // reads exactly like a command that did nothing. + // + // Sourced from state, the same place the row was written, and matched on + // NAME rather than on the absence of a terminalId: a name Tower already + // listed above must not print twice. + const shownNames = new Set( + architectTerminals.map(t => (t.architectName || t.label || '').toLowerCase()), + ); + const threadArchitects = architects.filter( + (a) => a.threadId !== undefined && !shownNames.has((a.name ?? 'main').toLowerCase()), + ); + + if (architectTerminals.length > 0 || threadArchitects.length > 0) { + logger.blank(); + logger.info('Architects:'); + for (const term of architectTerminals) { + const name = term.architectName || term.label; + const pid = term.pid ? `pid=${term.pid}` : 'pid=?'; + const port = term.port ? ` port=${term.port}` : ''; + // Spec 786 Phase 5: prefer `terminalId` (the actual PtySession id) + // over `id` (the Spec 761 tab identifier, e.g. `architect` or + // `architect:`). Falls back to `id` for older Tower versions + // that haven't shipped the Phase 5 extension yet. + const termIdValue = term.terminalId ?? term.id; + const termId = ` terminal=${termIdValue}`; + logger.info(` ${chalk.cyan(name)} (${pid}${port}${termId})`); } + for (const arch of threadArchitects) { + // No pid and no port, and those are not printed as unknowns: a thread + // has neither, so `pid=?` would report a value that could not exist as + // one this command failed to read. + const model = arch.model ? ` model=${arch.model}` : ''; + logger.info(` ${chalk.cyan(arch.name ?? 'main')} (thread=${arch.threadId}${model})`); + } + } - if (otherTerminals.length > 0) { - logger.blank(); - logger.info('Terminals:'); - for (const term of otherTerminals) { - const typeColor = term.type === 'dev' ? chalk.green : chalk.gray; - logger.info(` ${typeColor(term.type)} - ${term.label} (${term.active ? 'active' : 'stopped'})`); - } + if (otherTerminals.length > 0) { + logger.blank(); + logger.info('Terminals:'); + for (const term of otherTerminals) { + const typeColor = term.type === 'dev' ? chalk.green : chalk.gray; + logger.info(` ${typeColor(term.type)} - ${term.label} (${term.active ? 'active' : 'stopped'})`); } } @@ -456,7 +482,15 @@ export async function status(options: StatusOptions = {}): Promise { logger.kv('Architects', chalk.green(`${architects.length} registered`)); logger.info(` (Tower not running — PID/port not available)`); for (const a of architects) { - logger.info(` ${chalk.cyan(a.name ?? 'main')}: cmd=${a.cmd} started=${a.startedAt}`); + // Issue #271. A thread-backed architect has an empty `cmd` — there is no + // process, which is the point — so this line rendered as `cmd= started=…` + // and named nothing that identified the thread. The two render paths then + // disagreed about the same row: Tower-up showed a thread id, Tower-down + // showed a blank. Same fact, one spelling. + const backing = a.threadId !== undefined + ? `thread=${a.threadId}` + : `cmd=${a.cmd}`; + logger.info(` ${chalk.cyan(a.name ?? 'main')}: ${backing} started=${a.startedAt}`); } } else { logger.kv('Architects', chalk.gray('none registered')); diff --git a/packages/codev/src/agent-farm/commands/workspace-add-architect.ts b/packages/codev/src/agent-farm/commands/workspace-add-architect.ts index d05aa6b4c..30ed3fdfd 100644 --- a/packages/codev/src/agent-farm/commands/workspace-add-architect.ts +++ b/packages/codev/src/agent-farm/commands/workspace-add-architect.ts @@ -22,7 +22,7 @@ import { } from '../utils/architect-name.js'; import { getArchitects, setArchitectByName } from '../state.js'; import { architectThreadDefaults, createArchitectThread, tryGetThreadEngine } from '../thread-runtime.js'; -import { ensureThreadBackendReady } from '../thread-backend.js'; +import { closeThreadBackend, ensureThreadBackendReady } from '../thread-backend.js'; export interface WorkspaceAddArchitectOptions { name?: string; @@ -95,21 +95,33 @@ export async function workspaceAddArchitect( // Read BEFORE the create, so the pair recorded is the one this create resolves — // not a re-read of configuration that a concurrent edit could have moved. const defaults = architectThreadDefaults(workspacePath); - const threadId = await createArchitectThread({ name, workspaceRoot: workspacePath }); - setArchitectByName(workspacePath, name, { - name, - cmd: '', - startedAt: new Date().toISOString(), - threadId, - // Issue #227 item 3: the pair this thread was created with, pinned on the row the - // way a builder's is. Without it a later `attach` — which is where Tower resumes - // this thread — carries no harness or model and falls back to whatever - // `.codev/config.json` says at THAT moment, so editing `threads.model` between a - // spawn and a delivery silently moved a live architect onto a different model. - harness: defaults?.harness, - model: defaults?.model, - }); - logger.success(`Started architect '${name}' (thread ${threadId}).`); + try { + const threadId = await createArchitectThread({ name, workspaceRoot: workspacePath }); + setArchitectByName(workspacePath, name, { + name, + cmd: '', + startedAt: new Date().toISOString(), + threadId, + // Issue #227 item 3: the pair this thread was created with, pinned on the row the + // way a builder's is. Without it a later `attach` — which is where Tower resumes + // this thread — carries no harness or model and falls back to whatever + // `.codev/config.json` says at THAT moment, so editing `threads.model` between a + // spawn and a delivery silently moved a live architect onto a different model. + harness: defaults?.harness, + model: defaults?.model, + }); + logger.success(`Started architect '${name}' (thread ${threadId}).`); + } finally { + // Issue #271. An open WebSocket keeps the event loop alive, and this command is + // expected to exit. Without it the live run printed nothing and hung past two + // minutes until the caller killed it — the architect having already been created + // AND registered. Working, and hung, which reads from outside exactly like a + // command that failed. + // + // The same fix `afx interrupt` carries, for the same reason. Every one-shot + // command that reaches a thread owes it. + closeThreadBackend(workspacePath); + } return; }