diff --git a/codev/projects/bugfix-1094-afx-send-detectcurrentbuilderi/status.yaml b/codev/projects/bugfix-1094-afx-send-detectcurrentbuilderi/status.yaml new file mode 100644 index 000000000..f27c60baa --- /dev/null +++ b/codev/projects/bugfix-1094-afx-send-detectcurrentbuilderi/status.yaml @@ -0,0 +1,16 @@ +id: bugfix-1094 +title: afx-send-detectcurrentbuilderi +protocol: bugfix +phase: pr +plan_phases: [] +current_plan_phase: null +gates: + pr: + status: pending + requested_at: '2026-06-23T22:08:09.451Z' +iteration: 1 +build_complete: false +history: [] +started_at: '2026-06-23T21:45:58.428Z' +updated_at: '2026-06-23T22:08:09.451Z' +pr_ready_for_human: true diff --git a/codev/state/bugfix-1094_thread.md b/codev/state/bugfix-1094_thread.md new file mode 100644 index 000000000..ffa13a5ff --- /dev/null +++ b/codev/state/bugfix-1094_thread.md @@ -0,0 +1,94 @@ +# bugfix-1094 — afx send detectCurrentBuilderId silent misroute + +Issue #1094. Protocol: BUGFIX (strict). Branch: `builder/bugfix-1094`. + +## Investigate + +**Bug**: `detectCurrentBuilderId()` (`packages/codev/src/agent-farm/commands/send.ts`) +silently returns the bare worktree dir name (e.g. `bugfix-2461`) on three +unverifiable paths inside a *confirmed* `.builders//` context: + +1. `!existsSync(dbPath)` → `return worktreeDirName` (line 73) +2. `catch { return worktreeDirName }` — DB open failure (line 78-80) ← the real incident (Node ABI mismatch) +3. no matching builder row → `return worktreeDirName` (line 97) + +The bare name is non-canonical (`builder-bugfix-2461` is the real id), so +Tower's `lookupBuilderSpawningArchitect(sender)` returns `undefined`, and +`resolveAgentInWorkspace` drops to the "non-builder sender → main first" +branch (`tower-messages.ts:344-347`). Net: builder's `afx send architect` +silently lands on **main** instead of its spawning architect. + +Violates global principle #1 "fail fast, NEVER implement fallbacks": a fatal +environmental fault (DB unreadable) is laundered into a plausible-looking +misroute that is very hard to diagnose. Related memory: +reference_hermes_node_shadow_misroute. + +**Call graph**: `detectCurrentBuilderId()` is used in exactly one place — +`send()` line 222: `const from = detectCurrentBuilderId() ?? 'architect'`. +Plus the two test files. Blast radius is just `afx send`'s `from` identity. + +## Fix plan + +1. `detectCurrentBuilderId`: in a confirmed builder context, **throw** a typed, + actionable `BuilderIdResolutionError` on all three unverifiable paths instead + of returning a bare name. DB-open failure gets a message that names the + likely ABI-mismatch cause. Contract becomes: returns canonical id | null + (not a builder); throws when it's a builder but id can't be verified. +2. `send()`: wrap the `from` computation; on throw, `fatal(message)` so afx send + aborts loudly rather than misrouting. +3. Tower guard (defense-in-depth): in `resolveAgentInWorkspace`, when sender + looks like a builder id (`parseAgentName` / `…-` shape) but + `lookupBuilderSpawningArchitect` returns `undefined`, log a warning before + the main-first fallback. +4. Tests: update bugfix-774 tests that encoded the bare-name fallback; add a + DB-open-failure regression test asserting it does NOT return a bare name. + +Note: worktree had no node_modules — running `pnpm install` to build/test. + +## Fix (done) + +Implemented: +- `send.ts`: new `BuilderIdResolutionError` + `describeStateDbOpenFailure` (ABI-mismatch + hint). `detectCurrentBuilderId` now **throws** on all 3 unverifiable paths in a + confirmed builder context (missing db / open failure / no row) instead of returning + a bare name. Call site in `send()` wraps it → `fatal()` on throw. +- `tower-messages.ts`: `looksLikeBuilderId` + guard in `resolveAgentInWorkspace` — warns + before the main-first fallback when a builder-shaped sender has no row (and isn't a + registered architect). Defense-in-depth. + +Reproduced bug (unopenable db → old code returned bare `bugfix-2461`). Verified fix +against REAL main state.db: worktree resolves to canonical `builder-bugfix-1094` +(happy path unchanged). + +Tests: +- bugfix-774 test: the 2 fallback tests now assert throws; added DB-open-failure + regression + `describeStateDbOpenFailure` message tests. +- new bugfix-1094-tower-guard test: `looksLikeBuilderId` + warn-then-main-fallback. +- send.test.ts: was built on the old silent fallback (ran from worktree CWD, mocked + existsSync→false). Now chdir to tmpdir in beforeEach so `from` resolves to + 'architect' deterministically; simplified `getExpectedFrom()`. + +Full suite green: 167 files / 3347 tests pass, 0 fail. Net ~217 LOC tracked + +~110 new test file. Within BUGFIX scope. + +## PR (gate: pr) + +PR #1095 opened. CMAP (`--protocol bugfix --type pr`): +- codex = APPROVE (HIGH, no key issues) +- claude = APPROVE (HIGH, no key issues) +- gemini = NO VERDICT (broken agy review lane — burned session hunting for source + under sandbox restrictions; matches reference_agy_consult_review_broken #1032/1033) + +No REQUEST_CHANGES, no defects surfaced. One non-blocking note (looksLikeBuilderId +could match a hypothetical architect name like `custom-42`) — already mitigated by the +`!entry.architects.has(sender)` guard + warning-only nature. No changes made. + +Two pre-existing consult tooling gotchas hit (NOT my bug, candidates for follow-up issues): +1. `consult --type pr` needs explicit `--protocol bugfix` or it looks at top-level + `codev/consult-types/pr-review.md` (only `integration-review.md` lives there; + pr-review.md is per-protocol under `protocols/

/consult-types/`). +2. consult project auto-detect regex `/\.builders\/[^/]*?-?(\d+)-([^/]+)/` requires a + trailing slug after the issue number; worktree `bugfix-1094` (no slug) fails → + "Multiple projects found". Workaround: `--project-id 1094`. + +Requesting pr gate via `porch done`. Waiting for human approval. diff --git a/packages/codev/src/agent-farm/__tests__/bugfix-1094-tower-guard.test.ts b/packages/codev/src/agent-farm/__tests__/bugfix-1094-tower-guard.test.ts new file mode 100644 index 000000000..defb7e286 --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/bugfix-1094-tower-guard.test.ts @@ -0,0 +1,111 @@ +/** + * Issue #1094 — Tower-side defense-in-depth guard. + * + * When `resolveAgentInWorkspace` is asked to route 'architect' for a `sender` + * that LOOKS like a builder id (canonical `builder--` or the bare + * worktree form `-`) but `lookupBuilderSpawningArchitect` returns + * `undefined` (no matching state.db row), affinity routing has been silently + * bypassed. The send still falls through to 'main' (unchanged behavior), but a + * warning is now emitted so the misroute is visible instead of silent. + * + * Also unit-tests the `looksLikeBuilderId` heuristic directly. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import type { WorkspaceTerminals } from '../servers/tower-types.js'; + +const { mockGetWorkspaceTerminals, mockLookupBuilderSpawningArchitect } = vi.hoisted(() => ({ + mockGetWorkspaceTerminals: vi.fn<() => Map>(), + mockLookupBuilderSpawningArchitect: vi.fn<(id: string) => string | null | undefined>(), +})); + +vi.mock('../servers/tower-terminals.js', () => ({ + getWorkspaceTerminals: () => mockGetWorkspaceTerminals(), +})); + +vi.mock('../state.js', () => ({ + lookupBuilderSpawningArchitect: (id: string) => mockLookupBuilderSpawningArchitect(id), +})); + +import { resolveTarget, isResolveError, looksLikeBuilderId } from '../servers/tower-messages.js'; + +const WS = '/home/user/project'; + +function mkEntry(architects: Record, builders: Record = {}): WorkspaceTerminals { + return { + architects: new Map(Object.entries(architects)), + builders: new Map(Object.entries(builders)), + shells: new Map(), + fileTabs: new Map(), + }; +} + +describe('looksLikeBuilderId — issue #1094', () => { + it('matches canonical and bare builder ids', () => { + expect(looksLikeBuilderId('builder-bugfix-2461')).toBe(true); + expect(looksLikeBuilderId('bugfix-2461')).toBe(true); + expect(looksLikeBuilderId('bugfix-2461-some-slug')).toBe(true); + expect(looksLikeBuilderId('spir-100')).toBe(true); + }); + + it('does not match architect senders', () => { + expect(looksLikeBuilderId('architect')).toBe(false); + expect(looksLikeBuilderId('arch')).toBe(false); + expect(looksLikeBuilderId('main')).toBe(false); + expect(looksLikeBuilderId('sibling')).toBe(false); + }); +}); + +describe('Tower guard — non-canonical builder sender warns before main fallback', () => { + let warnSpy: ReturnType; + + beforeEach(() => { + vi.clearAllMocks(); + warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + + afterEach(() => { + warnSpy.mockRestore(); + }); + + it('warns when a builder-shaped sender has no state.db row, then routes to main', () => { + // Two architects → fast path skipped → lookup consulted. The bare, + // non-canonical sender id is exactly what the old send.ts fallback shipped. + mockGetWorkspaceTerminals.mockReturnValue( + new Map([[WS, mkEntry({ main: 'term-main', triage: 'term-triage' })]]), + ); + mockLookupBuilderSpawningArchitect.mockReturnValue(undefined); // no matching row + + const result = resolveTarget('architect', WS, 'bugfix-2461'); + if (isResolveError(result)) throw new Error(`unexpected: ${result.message}`); + // Behavior unchanged: still falls through to main. + expect(result.terminalId).toBe('term-main'); + // But the bypass is now visible. + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(warnSpy.mock.calls[0][0]).toContain('bugfix-2461'); + expect(warnSpy.mock.calls[0][0]).toMatch(/issue #1094/); + }); + + it('does NOT warn for a legitimate non-builder (architect) sender', () => { + mockGetWorkspaceTerminals.mockReturnValue( + new Map([[WS, mkEntry({ main: 'term-main', sibling: 'term-sibling' })]]), + ); + mockLookupBuilderSpawningArchitect.mockReturnValue(undefined); // not a builder + + const result = resolveTarget('architect', WS, 'architect'); + if (isResolveError(result)) throw new Error(`unexpected: ${result.message}`); + expect(result.terminalId).toBe('term-main'); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it('does NOT warn when there is no sender at all', () => { + mockGetWorkspaceTerminals.mockReturnValue( + new Map([[WS, mkEntry({ main: 'term-main', sibling: 'term-sibling' })]]), + ); + + const result = resolveTarget('architect', WS); + if (isResolveError(result)) throw new Error(`unexpected: ${result.message}`); + expect(result.terminalId).toBe('term-main'); + expect(warnSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/codev/src/agent-farm/__tests__/bugfix-774-detect-builder-id.test.ts b/packages/codev/src/agent-farm/__tests__/bugfix-774-detect-builder-id.test.ts index a4c7aeb61..bdb556e4e 100644 --- a/packages/codev/src/agent-farm/__tests__/bugfix-774-detect-builder-id.test.ts +++ b/packages/codev/src/agent-farm/__tests__/bugfix-774-detect-builder-id.test.ts @@ -19,7 +19,11 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import Database from 'better-sqlite3'; -import { detectCurrentBuilderId } from '../commands/send.js'; +import { + detectCurrentBuilderId, + BuilderIdResolutionError, + describeStateDbOpenFailure, +} from '../commands/send.js'; import { LOCAL_SCHEMA } from '../db/schema.js'; function writeBuilderRow(dbPath: string, id: string, worktree: string): void { @@ -77,20 +81,53 @@ describe('detectCurrentBuilderId — issue #774', () => { expect(detectCurrentBuilderId()).toBe('builder-bugfix-1599'); }); - it('falls back to worktree dir name when workspace state.db is missing', () => { + // Issue #1094: in a confirmed builder worktree, an inability to verify the + // canonical id is an ERROR — never a silent bare-name fallback (which would + // misroute `afx send architect` to 'main'). The three unverifiable paths + // below must all throw rather than return the bare worktree dir name. + + it('throws (does not return bare name) when workspace state.db is missing', () => { rmSync(join(workspacePath, '.agent-farm'), { recursive: true }); process.chdir(worktreePath); - expect(detectCurrentBuilderId()).toBe('bugfix-1599'); + expect(() => detectCurrentBuilderId()).toThrow(BuilderIdResolutionError); + expect(() => detectCurrentBuilderId()).toThrow(/bugfix-1599/); }); - it('falls back to worktree dir name when no row matches', () => { + it('throws (does not return bare name) when no builder row matches', () => { // Workspace DB exists but has no row for this worktree. const wsDb = new Database(join(workspacePath, '.agent-farm', 'state.db')); wsDb.prepare('DELETE FROM builders').run(); wsDb.close(); process.chdir(worktreePath); - expect(detectCurrentBuilderId()).toBe('bugfix-1599'); + expect(() => detectCurrentBuilderId()).toThrow(BuilderIdResolutionError); + expect(() => detectCurrentBuilderId()).toThrow(/no matching builder row/); + }); + + it('throws (does not return bare name) when state.db cannot be opened — issue #1094', () => { + // The real incident: a Node ABI mismatch made `new Database()` throw, and + // the old `catch { return worktreeDirName }` swallowed it, shipping the + // bare name `bugfix-1599`. Simulate an unopenable DB by replacing the file + // with a directory at the same path (existsSync passes; open throws). + rmSync(join(workspacePath, '.agent-farm'), { recursive: true }); + mkdirSync(join(workspacePath, '.agent-farm', 'state.db'), { recursive: true }); + + process.chdir(worktreePath); + + // Must NOT silently return the bare worktree directory name. + let returned: string | null | undefined; + try { + returned = detectCurrentBuilderId(); + } catch (err) { + expect(err).toBeInstanceOf(BuilderIdResolutionError); + expect((err as Error).message).toContain('bugfix-1599'); + expect((err as Error).message).not.toBe('bugfix-1599'); + expect((err as Error).message).toMatch(/issue #1094/); + return; + } + // Reaching here means it returned instead of throwing — fail loudly. + expect(returned).toBeUndefined(); // never reached if it threw; asserts no silent bare-name + throw new Error(`Expected a throw, got a silent return of '${returned}'`); }); it('returns null when not in a builder worktree', () => { @@ -98,3 +135,24 @@ describe('detectCurrentBuilderId — issue #774', () => { expect(detectCurrentBuilderId()).toBeNull(); }); }); + +describe('describeStateDbOpenFailure — issue #1094 actionable messages', () => { + it('names the better-sqlite3 ABI mismatch and points at reinstalling codev', () => { + const abiErr = new Error( + "The module was compiled against a different Node.js version using NODE_MODULE_VERSION 147. " + + "This version of Node.js requires NODE_MODULE_VERSION 127.", + ); + const msg = describeStateDbOpenFailure('/ws/.agent-farm/state.db', 'bugfix-2461', abiErr); + expect(msg).toMatch(/ABI mismatch/i); + expect(msg).toMatch(/reinstall codev/i); + expect(msg).toContain('bugfix-2461'); + expect(msg).toMatch(/issue #1094/); + }); + + it('gives a generic hint for non-ABI open failures', () => { + const msg = describeStateDbOpenFailure('/ws/.agent-farm/state.db', 'bugfix-2461', new Error('disk I/O error')); + expect(msg).toMatch(/corruption|permissions|stale lock/i); + expect(msg).not.toMatch(/ABI mismatch/i); + expect(msg).toContain('bugfix-2461'); + }); +}); diff --git a/packages/codev/src/agent-farm/__tests__/send.test.ts b/packages/codev/src/agent-farm/__tests__/send.test.ts index 736694e7d..fd5072919 100644 --- a/packages/codev/src/agent-farm/__tests__/send.test.ts +++ b/packages/codev/src/agent-farm/__tests__/send.test.ts @@ -3,7 +3,7 @@ * Spec 0110: Messaging Infrastructure — Phase 4 */ -import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; // ============================================================================ // Mocks @@ -66,6 +66,7 @@ vi.mock('node:fs', async () => { }; }); +import { tmpdir } from 'node:os'; import { send, detectWorkspaceRoot } from '../commands/send.js'; import { fatal } from '../utils/logger.js'; @@ -74,18 +75,17 @@ import { fatal } from '../utils/logger.js'; // ============================================================================ /** - * Detect the 'from' value the send command will use based on CWD. - * If CWD is inside .builders/

/, `from` will be the builder's canonical ID - * (from state.db) or the dir name. Otherwise 'architect'. + * The 'from' sender identity these tests expect. The suite runs from a CWD + * outside any `.builders//` worktree (see beforeEach), so + * detectCurrentBuilderId() returns null and send() uses 'architect'. + * + * Builder-id detection (and its #1094 fail-loud behavior when state.db is + * unreadable inside a worktree) is covered by bugfix-774 / bugfix-1094 tests; + * these tests deliberately isolate from it so they exercise send()'s other + * behavior without depending on the physical CWD of the test runner. */ function getExpectedFrom(): string { - const cwd = process.cwd(); - const match = cwd.match(/\.builders\/([^/]+)/); - if (!match) return 'architect'; - // In tests, loadState returns our mock with builders that have worktree paths. - // The worktree dir match will look for match[1] in state.db. - // Our defaultState doesn't match the actual CWD dir, so it falls back to dir name. - return match[1]; // e.g., '0110' + return 'architect'; } function defaultState() { @@ -105,13 +105,23 @@ function defaultState() { // ============================================================================ describe('send command', () => { + const origCwd = process.cwd(); + beforeEach(() => { + // Run from outside any `.builders//` worktree so the sender identity + // resolves deterministically to 'architect' regardless of where the test + // runner physically lives (it may itself run inside a builder worktree). + process.chdir(tmpdir()); vi.clearAllMocks(); mockIsRunning.mockResolvedValue(true); mockSendMessage.mockResolvedValue({ ok: true, resolvedTo: 'builder-spir-109' }); mockLoadState.mockReturnValue(defaultState()); }); + afterEach(() => { + process.chdir(origCwd); + }); + describe('single target send', () => { it('sends to a builder by full name', async () => { await send({ builder: 'builder-spir-109', message: 'Hello builder' }); diff --git a/packages/codev/src/agent-farm/commands/send.ts b/packages/codev/src/agent-farm/commands/send.ts index 55b4a730c..1b044f51f 100644 --- a/packages/codev/src/agent-farm/commands/send.ts +++ b/packages/codev/src/agent-farm/commands/send.ts @@ -40,6 +40,45 @@ export function detectWorkspaceRoot(): string | null { return null; } +/** + * Thrown when CWD is confirmed to be inside `.builders//` but the canonical + * builder identity cannot be verified against the workspace `state.db`. + * + * We refuse to fall back to the bare worktree directory name (e.g. `bugfix-774`) + * here: that non-canonical id does not match any `builders.id` (`builder-bugfix-774`), + * so Tower's affinity resolver (`lookupBuilderSpawningArchitect` → undefined) + * silently drops to the "non-builder sender → main first" branch — the builder's + * `afx send architect` lands on `main` instead of its spawning architect. + * + * Per "fail fast, never implement fallbacks": a fatal environmental fault must + * surface loudly, not be laundered into a subtle misroute (issue #1094). + */ +export class BuilderIdResolutionError extends Error { + constructor(message: string) { + super(message); + this.name = 'BuilderIdResolutionError'; + } +} + +/** + * Build an actionable message for a `state.db` open failure, naming the likely + * cause. A better-sqlite3 ABI mismatch (a `node` on PATH built for a different + * NODE_MODULE_VERSION than codev's native module) is the real-world trigger + * from issue #1094 and gets a specific reinstall hint. + */ +export function describeStateDbOpenFailure(dbPath: string, worktreeDirName: string, err: unknown): string { + const detail = err instanceof Error ? err.message : String(err); + const abiMismatch = /NODE_MODULE_VERSION|different Node\.js version|was compiled against/i.test(detail); + const hint = abiMismatch + ? "This is a better-sqlite3 native-module ABI mismatch: the 'node' on your PATH differs from the one codev was built for. Reinstall codev under your current node (e.g. `npm install -g @cluesmith/codev`)." + : 'Check the file for corruption, a permissions problem, or a stale lock.'; + return ( + `Cannot resolve builder identity for worktree '${worktreeDirName}': ` + + `failed to open workspace state.db at ${dbPath} (${detail}). ${hint} ` + + `Refusing to send with an unverified identity — it would silently misroute to 'main' (issue #1094).` + ); +} + /** * Detect the current builder ID from worktree path. * @@ -47,15 +86,21 @@ export function detectWorkspaceRoot(): string | null { * directly (not the singleton). When CWD is `.builders//`, the singleton * `getDb()` resolves to the worktree's own state.db — which is empty because * the worktree is itself a full git checkout with its own `codev/`. Reading - * that empty DB causes the lookup to miss and the function to fall back to - * the worktree directory name (e.g. `bugfix-774`), breaking multi-architect - * routing downstream because the canonical ID is `builder-bugfix-774`. + * that empty DB causes the lookup to miss; that miss must NOT fall back to the + * worktree directory name (e.g. `bugfix-774`), because the canonical ID is + * `builder-bugfix-774` and a non-canonical id misroutes affinity routing + * downstream (issue #774, then issue #1094 for the silent-fallback class). * * Mirrors the per-workspace-handle pattern used by - * `lookupBuilderSpawningArchitect` in state.ts. Issue #774. + * `lookupBuilderSpawningArchitect` in state.ts. * - * Falls back to the worktree directory name only as a safety net. - * Returns null if not in a builder worktree. + * Contract: + * - Returns `null` when CWD is not inside a builder worktree (not a builder). + * - Returns the canonical builder ID when it can be verified against state.db. + * - **Throws `BuilderIdResolutionError`** when CWD *is* a builder worktree but + * the canonical ID cannot be verified (state.db missing, unopenable, or no + * matching row). Failing loud here is deliberate: returning a bare, + * unverified id silently misroutes `afx send architect` to `main` (#1094). */ export function detectCurrentBuilderId(): string | null { const cwd = process.cwd(); @@ -68,15 +113,23 @@ export function detectCurrentBuilderId(): string | null { // Open the WORKSPACE's state.db readonly — not the singleton getDb(), // which resolves to the worktree-local state.db when CWD is inside - // .builders//. + // .builders//. From here on we are unambiguously in a builder worktree, + // so any inability to resolve the canonical id is an ERROR condition, not a + // "this isn't a builder" condition. const dbPath = join(workspacePath, '.agent-farm', 'state.db'); - if (!existsSync(dbPath)) return worktreeDirName; + if (!existsSync(dbPath)) { + throw new BuilderIdResolutionError( + `Cannot resolve builder identity for worktree '${worktreeDirName}': ` + + `workspace state.db not found at ${dbPath} (is Tower running for this workspace?). ` + + `Refusing to send with an unverified identity — it would silently misroute to 'main' (issue #1094).`, + ); + } let wsDb: Database.Database; try { wsDb = new Database(dbPath, { readonly: true }); - } catch { - return worktreeDirName; + } catch (err) { + throw new BuilderIdResolutionError(describeStateDbOpenFailure(dbPath, worktreeDirName, err)); } try { @@ -94,7 +147,11 @@ export function detectCurrentBuilderId(): string | null { const tail = rows.find(r => r.worktree.split('/').pop() === worktreeDirName); if (tail) return tail.id; - return worktreeDirName; + throw new BuilderIdResolutionError( + `Cannot resolve canonical builder id for worktree '${worktreeDirName}': ` + + `no matching builder row in ${dbPath} (the worktree may be stale or unregistered). ` + + `Refusing to send with an unverified identity — it would silently misroute to 'main' (issue #1094).`, + ); } finally { wsDb.close(); } @@ -218,8 +275,16 @@ export async function send(options: SendOptions): Promise { // Detect workspace for target resolution and sender provenance const workspace = detectWorkspaceRoot() ?? undefined; - // Detect sender identity (builder ID if in a worktree, otherwise 'architect') - const from = detectCurrentBuilderId() ?? 'architect'; + // Detect sender identity (builder ID if in a worktree, otherwise 'architect'). + // In a confirmed builder worktree, detectCurrentBuilderId throws when the + // canonical id can't be verified — abort loudly here rather than send an + // unverified `from` that Tower would silently route to 'main' (issue #1094). + let from: string; + try { + from = detectCurrentBuilderId() ?? 'architect'; + } catch (err) { + fatal(err instanceof Error ? err.message : String(err)); + } // Ensure Tower is running const client = new TowerClient(); diff --git a/packages/codev/src/agent-farm/servers/tower-messages.ts b/packages/codev/src/agent-farm/servers/tower-messages.ts index 1bb103114..f708ef7d1 100644 --- a/packages/codev/src/agent-farm/servers/tower-messages.ts +++ b/packages/codev/src/agent-farm/servers/tower-messages.ts @@ -38,6 +38,20 @@ export function addressSpoofingErrorMessage(builderId: string): string { return `builder ${builderId} may only address its own spawning architect`; } +/** + * Heuristic: does `sender` look like a builder identity (canonical + * `builder--` or the bare worktree form `-[-slug]`) + * rather than an architect? Used only for the issue #1094 defense-in-depth + * warning. Any `architect`/`arch`-prefixed name (e.g. the numbered architect + * names `architect-3`) is excluded; the call site additionally suppresses the + * warning for any currently-registered architect name. + */ +export function looksLikeBuilderId(sender: string): boolean { + const s = sender.toLowerCase(); + if (s === 'arch' || s.startsWith('architect')) return false; + return /^builder-[a-z0-9]+-/.test(s) || /-\d+(?:-[a-z0-9-]+)?$/.test(s); +} + // ============================================================================ // Message Frame // ============================================================================ @@ -342,6 +356,18 @@ function resolveAgentInWorkspace( } // Non-builder sender (or no sender): 'main' first, then first registered. + // Issue #1094 defense-in-depth: a sender that LOOKS like a builder id but + // had no state.db row (lookupBuilderSpawningArchitect → undefined) means + // affinity routing was silently bypassed — most likely a non-canonical, + // unverified builder id (e.g. a bare worktree name). Make it visible + // instead of quietly delivering to 'main'. + if (sender && !entry.architects.has(sender) && looksLikeBuilderId(sender)) { + console.warn( + `[tower] affinity routing bypassed: sender '${sender}' looks like a builder id but has ` + + `no matching row in workspace '${path.basename(workspacePath)}' — routing 'architect' to ` + + `'main'. The sender may be a non-canonical builder id (issue #1094).`, + ); + } const terminalId = entry.architects.get(DEFAULT_ARCHITECT_NAME) ?? entry.architects.values().next().value!; return { terminalId, workspacePath, agent: 'architect' };