diff --git a/.claude/skills/afx/SKILL.md b/.claude/skills/afx/SKILL.md index cae2e9a0a..007fa4723 100644 --- a/.claude/skills/afx/SKILL.md +++ b/.claude/skills/afx/SKILL.md @@ -69,10 +69,16 @@ afx send [builder] [message] | `--interrupt` | Ready the prompt first — end the turn and clear the composer — with the keystrokes recorded as safe for the target (Ctrl+C on claude/codex and shells; ESC then Ctrl+U on opencode) | | `--raw` | Skip structured formatting | | `--no-enter` | Don't press Enter after message | +| `--exact` | Resolve the address exactly — no builder tail match. A miss names the address and the workspace and sends nothing. | +| `--worktree ` | Resolve the recipient and the workspace from that worktree, not from your session. With no builder given, addresses the builder that owns it. | ```bash afx send 0042 "PR approved, please merge" afx send 0585 "check the test output" --file /tmp/test-results.txt + +# Machine-generated, authority-adjacent messages address a worktree, never a project id. +# `250` tail-matches builder-spir-250 in whatever workspace your session resolves to (#264). +afx send --worktree /repo/.builders/spir-250 --exact "gate notification" --raw ``` **Addressing forms** — the recipient isn't only a builder ID: diff --git a/.codex/skills/afx/SKILL.md b/.codex/skills/afx/SKILL.md index cae2e9a0a..007fa4723 100644 --- a/.codex/skills/afx/SKILL.md +++ b/.codex/skills/afx/SKILL.md @@ -69,10 +69,16 @@ afx send [builder] [message] | `--interrupt` | Ready the prompt first — end the turn and clear the composer — with the keystrokes recorded as safe for the target (Ctrl+C on claude/codex and shells; ESC then Ctrl+U on opencode) | | `--raw` | Skip structured formatting | | `--no-enter` | Don't press Enter after message | +| `--exact` | Resolve the address exactly — no builder tail match. A miss names the address and the workspace and sends nothing. | +| `--worktree ` | Resolve the recipient and the workspace from that worktree, not from your session. With no builder given, addresses the builder that owns it. | ```bash afx send 0042 "PR approved, please merge" afx send 0585 "check the test output" --file /tmp/test-results.txt + +# Machine-generated, authority-adjacent messages address a worktree, never a project id. +# `250` tail-matches builder-spir-250 in whatever workspace your session resolves to (#264). +afx send --worktree /repo/.builders/spir-250 --exact "gate notification" --raw ``` **Addressing forms** — the recipient isn't only a builder ID: diff --git a/codev-skeleton/.claude/skills/afx/SKILL.md b/codev-skeleton/.claude/skills/afx/SKILL.md index 24128da23..ed89440a4 100644 --- a/codev-skeleton/.claude/skills/afx/SKILL.md +++ b/codev-skeleton/.claude/skills/afx/SKILL.md @@ -67,10 +67,16 @@ afx send [builder] [message] | `--interrupt` | Ready the prompt first — end the turn and clear the composer — with the keystrokes recorded as safe for the target (Ctrl+C on claude/codex and shells; ESC then Ctrl+U on opencode) | | `--raw` | Skip structured formatting | | `--no-enter` | Don't press Enter after message | +| `--exact` | Resolve the address exactly — no builder tail match. A miss names the address and the workspace and sends nothing. | +| `--worktree ` | Resolve the recipient and the workspace from that worktree, not from your session. With no builder given, addresses the builder that owns it. | ```bash afx send 0042 "PR approved, please merge" afx send 0585 "check the test output" --file /tmp/test-results.txt + +# Machine-generated, authority-adjacent messages address a worktree, never a project id. +# `250` tail-matches builder-spir-250 in whatever workspace your session resolves to (#264). +afx send --worktree /repo/.builders/spir-250 --exact "gate notification" --raw ``` ## afx interrupt diff --git a/codev-skeleton/.codex/skills/afx/SKILL.md b/codev-skeleton/.codex/skills/afx/SKILL.md index 24128da23..ed89440a4 100644 --- a/codev-skeleton/.codex/skills/afx/SKILL.md +++ b/codev-skeleton/.codex/skills/afx/SKILL.md @@ -67,10 +67,16 @@ afx send [builder] [message] | `--interrupt` | Ready the prompt first — end the turn and clear the composer — with the keystrokes recorded as safe for the target (Ctrl+C on claude/codex and shells; ESC then Ctrl+U on opencode) | | `--raw` | Skip structured formatting | | `--no-enter` | Don't press Enter after message | +| `--exact` | Resolve the address exactly — no builder tail match. A miss names the address and the workspace and sends nothing. | +| `--worktree ` | Resolve the recipient and the workspace from that worktree, not from your session. With no builder given, addresses the builder that owns it. | ```bash afx send 0042 "PR approved, please merge" afx send 0585 "check the test output" --file /tmp/test-results.txt + +# Machine-generated, authority-adjacent messages address a worktree, never a project id. +# `250` tail-matches builder-spir-250 in whatever workspace your session resolves to (#264). +afx send --worktree /repo/.builders/spir-250 --exact "gate notification" --raw ``` ## afx interrupt diff --git a/codev/projects/bugfix-264-safety-a-gate-approval-notific/status.yaml b/codev/projects/bugfix-264-safety-a-gate-approval-notific/status.yaml new file mode 100644 index 000000000..39a77f300 --- /dev/null +++ b/codev/projects/bugfix-264-safety-a-gate-approval-notific/status.yaml @@ -0,0 +1,27 @@ +id: bugfix-264 +title: safety-a-gate-approval-notific +protocol: bugfix +phase: pr +plan_phases: [] +current_plan_phase: null +gates: + pr: + status: approved + requested_at: '2026-08-31T14:36:52.282Z' + approved_at: '2026-08-31T14:37:14.217Z' + approval: + authorization: flag-only + approved_at: '2026-08-31T14:37:14.217Z' + machine: chriss-MacBook-Pro.local + caller: CODEV_ARCHITECT_NAME=main (an architect session or a process it spawned) +iteration: 1 +build_complete: false +history: [] +started_at: '2026-08-31T13:58:05.489Z' +updated_at: '2026-08-31T14:37:14.220Z' +pr_history: + - phase: pr + pr_number: 270 + branch: builder/bugfix-264 + created_at: '2026-08-31T14:36:44.677Z' +pr_ready_for_human: false diff --git a/codev/state/bugfix-264_thread.md b/codev/state/bugfix-264_thread.md new file mode 100644 index 000000000..446794d4f --- /dev/null +++ b/codev/state/bugfix-264_thread.md @@ -0,0 +1,129 @@ +# bugfix-264 — gate-approval notification crosses workspaces + +## Investigate (2026-08-31) + +### Reproduced, twice, deliberately + +Built a throwaway porch project in a temp workspace: +`/repro-ws/.builders/fake-264/codev/projects/264-repro/status.yaml`, +protocol `air`, one pending gate `pr`. Ran `porch approve 264 pr +--a-human-explicitly-approved-this` from `/repro-ws`. + +**Run 1** (my normal builder env): the message +`Gate pr approved — please run porch next to advance.` landed in +`builder-bugfix-264`'s pane in `/Users/chris/dev/codev-1455`. My real +`bugfix-264` project's `pr` gate stayed `pending`; `porch next` returned the +same phase. The notification was false. + +**Run 2** (identical, but `env -u CODEV_BUILDER_ID -u CODEV_WORKTREE_ROOT -u +CODEV_THREAD_ID`): no delivery. `afx send` failed with +`Workspace '/repro-ws' has no registered terminals. (NOT_FOUND)`. + +The delta between the two runs is the whole mechanism. + +### Root cause — two hops, neither carrying the project's identity + +**Hop 1 — the recipient is a bare project id.** +`packages/codev/src/commands/porch/index.ts:1267` sends `target: state.id`. +`notify.ts:66-71` turns that into `afx send "" --raw` with +`cwd = workspaceRoot`. Nothing names the builder that owns the project, and +nothing names the workspace the approved `status.yaml` lives in. + +**Hop 2 — the workspace comes from the SENDER's session, and the agent is +tail-matched.** `agent-farm/commands/send.ts:60-110` resolves the workspace +from `CODEV_THREAD_ID`, else `CODEV_BUILDER_ID`+`CODEV_WORKTREE_ROOT`, else a +cwd walk-up. The issue attributes this to cwd; run 2 shows the launch-identity +env wins over cwd, which is why a Playwright suite running in `spir-250`'s pane +made a temp-workspace approval resolve to `/Users/chris/dev/codev-1455`. Then +`servers/tower-messages.ts:387-392` (live) and `:504-515` (registry) match +`builderId.endsWith('-' + stripLeadingZeros(agent))`, so `250` reaches +`builder-spir-250` and `bugfix-264` reaches `builder-bugfix-264`. + +The existing #1094 guard checks that the sender's identity is *verifiable*, not +that the recipient is *correct*. Both runs had a perfectly verifiable identity. + +### Scope + +Fits BUGFIX. Planned change, well under 300 LOC: + +1. `porch` addresses the builder that owns the project — canonical id resolved + from the project's own worktree path — and pins the workspace, instead of + sending a bare project id from wherever the process happens to be. +2. `afx send --exact` disables tail matching; a miss is an error naming the + target and the workspace, never a plausible neighbour. +3. The message text carries project id + workspace and says it is a hint to + verify against porch, not an approval. + +No architectural change; no new abstraction. + +## Fix (2026-08-31) + +### The change + +Both hops now carry the project's identity, and the last one refuses to guess. + +**Porch addresses the project's worktree, not its id.** `notifyGateApproved` +(new, `commands/porch/notify.ts`) sends +`afx send --worktree --exact "" --raw`. `artifactRoot` is +the directory whose `status.yaml` was just written, so it names one builder in +one workspace or it names nothing. `notifyProtocolComplete` is pinned the same +way — it prompts `afx cleanup`, which is destructive. + +**`afx send --worktree `** resolves the recipient AND the resolution +workspace from that path, scoped to the workspace that owns it +(`resolveRecipientWorktree`, `agent-farm/commands/send.ts`). `fromWorkspace` +still comes from the sender's session; those were one value and are now two. +A worktree no builder owns throws, naming the worktree and listing who is +registered. + +**`afx send --exact`** turns off the builder tail match in both Tower resolvers +(live and the offline-hold registry). The miss names the address, the workspace, +and the builders actually there. + +**The message text is checkable.** It names the project and the workspace, says +outright that it is not an approval and carries no authority, and tells the +recipient to confirm with `porch next ` and what a mismatch means. + +### Verified + +- Original reproducer, re-run against this build: nothing delivered. porch logs + `No builder in workspace '' owns worktree '/.builders/fake-264'`. +- Live positive: `afx send --worktree /Users/chris/dev/codev-1455/.builders/bugfix-264 + --exact` delivered to `builder-bugfix-264`. +- Both regression files confirmed to fail with the fix backed out (3 failures + each), then restored. + +### Not fixed here + +Issue #185's "On it" false positive in the spawn render gate, per the +architect — separate issue. + +## PR + CMAP (2026-08-31) + +PR #270. Three verdicts, all APPROVE: + +- **claude** — HIGH. Verified the `exact` short-circuit sits after the exact + builder loop and the architect block, so `--exact builder-spir-250` and + `--exact architect` still resolve. Two non-blocking items, both applied: + `exact` also skipped the SHELL lookup (wider than the flag's name), and a + JSDoc block was orphaned above `workspaceForWorktree`. +- **codex** — HIGH, no issues. +- **opencode** — HIGH, no issues. Run in place of gemini. + +The **gemini lane did not review**: `agy` exited 1 on quota. Recorded as a +non-blocking skip, which is not an approval — so opencode was run as the third +lane rather than counting the skip as a verdict. + +Post-review changes: `exact` now removes the tail match and nothing else (shells +and architects still resolve), pinned by a new test; JSDoc reattached. + +## Gate (2026-08-31) + +`pr` gate approved by the architect from the workspace root. Verified against +`status.yaml` before acting on it — `status: approved`, `approved_at: +2026-08-31T14:37:14.217Z`, `authorization: flag-only`, caller +`CODEV_ARCHITECT_NAME=main`. That verification is the behaviour this whole +project exists to make unnecessary to reason about, so it is recorded here. + +Merging on green with `--merge` (never squash). PR head `7627777a0` is the +commit CI is running and the one that merges. diff --git a/packages/codev/src/agent-farm/__tests__/bugfix-264-gate-notification-addressing.test.ts b/packages/codev/src/agent-farm/__tests__/bugfix-264-gate-notification-addressing.test.ts new file mode 100644 index 000000000..b3b564a66 --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/bugfix-264-gate-notification-addressing.test.ts @@ -0,0 +1,199 @@ +/** + * Issue #264 — a gate-approval notification must not reach a project it is not about. + * + * The delivery that prompted this had two hops, and each one is pinned here: + * + * 1. `resolveRecipientWorktree` — the recipient and the resolution workspace + * come from the PROJECT's worktree, so a project in workspace A cannot + * address a builder in workspace B. Before the fix there was no such + * resolution at all: porch handed `afx send` a bare project id and let the + * sending process's session decide the workspace. + * + * 2. `resolveTarget` / `resolveAgentInRegistry` with `exact` — a bare id must + * not tail-match a builder that merely ends with it, and the miss must name + * what it could not resolve. Before the fix `250` resolved to + * `builder-spir-250`, which is how a temp-workspace approval woke a live + * builder. + * + * The two-workspace fixture is the point: a single-workspace test cannot tell a + * correct resolution from a lucky one. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { mkdtempSync, rmSync, mkdirSync, realpathSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import Database from 'better-sqlite3'; +import type { WorkspaceTerminals } from '../servers/tower-types.js'; + +const { mockGetWorkspaceTerminals, mockGetGlobalDbPath, mockGetBuilders } = vi.hoisted(() => ({ + mockGetWorkspaceTerminals: vi.fn<() => Map>(), + mockGetGlobalDbPath: vi.fn<() => string>(), + mockGetBuilders: vi.fn(), +})); + +vi.mock('../servers/tower-terminals.js', () => ({ + getWorkspaceTerminals: () => mockGetWorkspaceTerminals(), +})); + +vi.mock('../db/index.js', async () => { + const actual = await vi.importActual('../db/index.js'); + return { ...actual, getGlobalDbPath: () => mockGetGlobalDbPath() }; +}); + +vi.mock('../state.js', async () => { + const actual = await vi.importActual('../state.js'); + return { ...actual, getBuilders: (ws: string) => mockGetBuilders(ws) }; +}); + +import { resolveTarget, resolveAgentInRegistry, isResolveError } from '../servers/tower-messages.js'; +import { + resolveRecipientWorktree, + workspaceForWorktree, + RecipientResolutionError, +} from '../commands/send.js'; + +// --------------------------------------------------------------------------- +// Fixture: two live workspaces, each with a builder whose id ends in `-250`. +// --------------------------------------------------------------------------- + +const LIVE_WS = '/Users/dev/codev-1455'; +const OTHER_WS = '/Users/dev/other-repo'; + +function terminalsFor(builderIds: string[], shellIds: string[] = []): WorkspaceTerminals { + return { + architects: new Map([['main', 'term-arch']]), + builders: new Map(builderIds.map((id) => [id, `term-${id}`])), + shells: new Map(shellIds.map((id) => [id, `term-${id}`])), + fileTabs: new Map(), + }; +} + +describe('issue #264 — exact resolution refuses the builder tail match', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockGetWorkspaceTerminals.mockReturnValue( + new Map([ + [LIVE_WS, terminalsFor(['builder-spir-250'], ['shell-1'])], + [OTHER_WS, terminalsFor(['builder-bugfix-250'])], + ]), + ); + }); + + it('tail-matches a bare project id by default (the convenience being constrained)', () => { + const result = resolveTarget('250', LIVE_WS); + expect(isResolveError(result)).toBe(false); + expect((result as { agent: string }).agent).toBe('builder-spir-250'); + }); + + it('refuses to deliver a bare project id when the caller demands an exact match', () => { + const result = resolveTarget('250', LIVE_WS, undefined, { exact: true }); + expect(isResolveError(result)).toBe(true); + expect((result as { code: string }).code).toBe('NOT_FOUND'); + }); + + it('names the address, the workspace, and who is actually there', () => { + const result = resolveTarget('250', LIVE_WS, undefined, { exact: true }); + const message = (result as { message: string }).message; + expect(message).toContain('250'); + expect(message).toContain(LIVE_WS); + expect(message).toContain('builder-spir-250'); + expect(message).toMatch(/nothing was delivered/i); + }); + + it('still resolves the canonical id it was actually given', () => { + const result = resolveTarget('builder-spir-250', LIVE_WS, undefined, { exact: true }); + expect(isResolveError(result)).toBe(false); + expect((result as { agent: string }).agent).toBe('builder-spir-250'); + }); + + it('removes the tail match and nothing else — architects and shells still resolve', () => { + // `exact` must be as wide as its name. Short-circuiting the shell lookup + // too would refuse an address that was never fuzzy in the first place. + const shell = resolveTarget('shell-1', LIVE_WS, undefined, { exact: true }); + expect(isResolveError(shell)).toBe(false); + expect((shell as { agent: string }).agent).toBe('shell-1'); + + const architect = resolveTarget('architect', LIVE_WS, undefined, { exact: true }); + expect(isResolveError(architect)).toBe(false); + expect((architect as { agent: string }).agent).toBe('architect'); + }); + + it('refuses the tail match on the offline-hold path too', () => { + mockGetBuilders.mockReturnValue([{ id: 'builder-spir-250' }]); + const held = resolveAgentInRegistry('250', LIVE_WS); + expect(isResolveError(held)).toBe(false); + + const exact = resolveAgentInRegistry('250', LIVE_WS, undefined, { exact: true }); + expect(isResolveError(exact)).toBe(true); + expect((exact as { message: string }).message).toContain('builder-spir-250'); + }); +}); + +describe('issue #264 — the recipient comes from the project worktree, not the sender', () => { + let farmDir: string; + let dbPath: string; + + beforeEach(() => { + // realpath, because `workspace_path` is stored canonicalized and macOS + // resolves /var to /private/var — an unnormalized fixture would test the + // symlink, not the lookup. + farmDir = realpathSync(mkdtempSync(join(tmpdir(), 'bugfix-264-'))); + dbPath = join(farmDir, 'global.db'); + const db = new Database(dbPath); + db.exec(` + CREATE TABLE builders ( + id TEXT PRIMARY KEY, + workspace_path TEXT, + worktree TEXT + ); + `); + // The shape that produced the misdelivery: two workspaces, two builders, + // ids that tail-match the same project number. + const insert = db.prepare('INSERT INTO builders (id, workspace_path, worktree) VALUES (?, ?, ?)'); + mkdirSync(join(farmDir, 'live', '.builders', 'spir-250'), { recursive: true }); + mkdirSync(join(farmDir, 'temp', '.builders', 'fake-250'), { recursive: true }); + insert.run('builder-spir-250', join(farmDir, 'live'), join(farmDir, 'live', '.builders', 'spir-250')); + insert.run('builder-air-250', join(farmDir, 'temp'), join(farmDir, 'temp', '.builders', 'fake-250')); + db.close(); + mockGetGlobalDbPath.mockReturnValue(dbPath); + }); + + afterEach(() => { + rmSync(farmDir, { recursive: true, force: true }); + }); + + it('resolves the builder that owns the worktree, in that worktree’s workspace', () => { + const recipient = resolveRecipientWorktree(join(farmDir, 'temp', '.builders', 'fake-250')); + expect(recipient.builderId).toBe('builder-air-250'); + expect(recipient.workspacePath).toBe(join(farmDir, 'temp')); + }); + + it('does not reach across workspaces to a builder whose id merely tail-matches', () => { + const recipient = resolveRecipientWorktree(join(farmDir, 'live', '.builders', 'spir-250')); + expect(recipient.builderId).toBe('builder-spir-250'); + expect(recipient.builderId).not.toBe('builder-air-250'); + }); + + it('reports no recipient for a workspace root, rather than inventing one', () => { + const recipient = resolveRecipientWorktree(join(farmDir, 'live')); + expect(recipient.builderId).toBeNull(); + expect(recipient.workspacePath).toBe(join(farmDir, 'live')); + }); + + it('pins the workspace from the path alone when the caller names its own recipient', () => { + // `notifyProtocolComplete` addresses `architect` and needs only the scope. + // Reading global.db to learn what the path already says would let an + // orphaned worktree suppress the cleanup trigger. + expect(workspaceForWorktree(join(farmDir, 'live', '.builders', 'ghost-9'))).toBe(join(farmDir, 'live')); + expect(workspaceForWorktree(join(farmDir, 'live'))).toBe(join(farmDir, 'live')); + }); + + it('throws, naming the worktree, when no builder owns it', () => { + mkdirSync(join(farmDir, 'live', '.builders', 'ghost-9'), { recursive: true }); + expect(() => resolveRecipientWorktree(join(farmDir, 'live', '.builders', 'ghost-9'))) + .toThrow(RecipientResolutionError); + expect(() => resolveRecipientWorktree(join(farmDir, 'live', '.builders', 'ghost-9'))) + .toThrow(/ghost-9/); + }); +}); diff --git a/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts b/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts index df1d77ba3..b8f902a3a 100644 --- a/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts +++ b/packages/codev/src/agent-farm/__tests__/tower-routes.test.ts @@ -1466,7 +1466,7 @@ describe('tower-routes', () => { const { res } = makeRes(); await handleRequest(req, res, makeCtx()); - expect(mockResolveTarget).toHaveBeenCalledWith('architect', '/tmp/ws', 'spir-100'); + expect(mockResolveTarget).toHaveBeenCalledWith('architect', '/tmp/ws', 'spir-100', { exact: false }); }); it('forwards undefined `from` when sender is not supplied (non-builder send)', async () => { @@ -1484,7 +1484,7 @@ describe('tower-routes', () => { const { res } = makeRes(); await handleRequest(req, res, makeCtx()); - expect(mockResolveTarget).toHaveBeenCalledWith('architect', '/tmp/ws', undefined); + expect(mockResolveTarget).toHaveBeenCalledWith('architect', '/tmp/ws', undefined, { exact: false }); }); it('returns 200 delivered:true on a successful send to a clean prompt (Spec 1313)', async () => { diff --git a/packages/codev/src/agent-farm/cli.ts b/packages/codev/src/agent-farm/cli.ts index 113a66ced..329445a09 100644 --- a/packages/codev/src/agent-farm/cli.ts +++ b/packages/codev/src/agent-farm/cli.ts @@ -459,6 +459,8 @@ export async function runAgentFarm(args: string[]): Promise { .option('--raw', 'Skip structured message formatting') .option('--no-enter', 'Do not send Enter after message') .option('--delay ', 'Deliver after N seconds (persisted; survives a Tower restart, except a delayed --interrupt keystroke nudge)') + .option('--exact', 'Resolve the address exactly — no builder tail match; a miss is an error and nothing is sent') + .option('--worktree ', "Resolve the recipient and the workspace from this worktree instead of the sender's session; with no builder given, addresses the builder that owns it") .action(async (builder, message, options) => { const { send } = await import('./commands/send.js'); try { @@ -492,6 +494,8 @@ export async function runAgentFarm(args: string[]): Promise { raw: options.raw, noEnter: !options.enter, delay, + exact: options.exact, + worktree: options.worktree, }); } catch (error) { logger.error(error instanceof Error ? error.message : String(error)); diff --git a/packages/codev/src/agent-farm/commands/send.ts b/packages/codev/src/agent-farm/commands/send.ts index 570043b9f..49f062676 100644 --- a/packages/codev/src/agent-farm/commands/send.ts +++ b/packages/codev/src/agent-farm/commands/send.ts @@ -260,6 +260,107 @@ export function detectCurrentBuilderId(): string | null { } } +/** + * Thrown when `--worktree ` cannot be resolved to the builder that owns it. + * + * A miss must END the send. Issue #264's failure was a notification about a + * project in one workspace arriving at a builder in another, and the reason it + * arrived at all is that every hop had something plausible to fall back on. This + * error exists so the last hop has nothing. + */ +export class RecipientResolutionError extends Error { + constructor(message: string) { + super(message); + this.name = 'RecipientResolutionError'; + } +} + +/** The workspace that owns a worktree, and the builder working in it. */ +export interface RecipientWorktree { + /** Canonical builder id, or null when no builder worktree is involved. */ + builderId: string | null; + /** Normalized workspace path — the resolution scope for the address. */ + workspacePath: string; +} + +/** + * The workspace that owns a worktree, from the path alone. + * + * Used when the caller already names its recipient and needs only the + * resolution SCOPE pinned to the project — `notifyProtocolComplete` addresses + * `architect`, and reading global.db to learn something the path already says + * would let an orphaned worktree suppress the cleanup trigger. + */ +export function workspaceForWorktree(worktreePath: string): string { + const trimmed = worktreePath.replace(/\/+$/, ''); + const match = trimmed.match(/^(.+)\/\.builders\/([^/]+)$/); + return normalizeWorkspacePath(match ? match[1] : trimmed); +} + +/** + * Resolve a worktree path to the workspace that owns it and the builder in it. + * + * This is `--worktree`'s whole point: the recipient and the workspace both come + * from the PROJECT's location on disk, not from the sending process's session + * environment or cwd. In issue #264 those two disagreed — a `porch approve` for a + * project in a throwaway temp workspace ran in a process whose launch identity + * belonged to a live builder elsewhere, and the notification followed the + * process, not the project. + * + * A path with no `.builders/` segment is a workspace root: a project owned + * by the main checkout rather than a builder. That resolves the workspace and + * reports `builderId: null` — there is no builder to wake, and inventing one is + * the bug. + */ +export function resolveRecipientWorktree(worktreePath: string): RecipientWorktree { + const trimmed = worktreePath.replace(/\/+$/, ''); + const match = trimmed.match(/^(.+)\/\.builders\/([^/]+)$/); + if (!match) { + return { builderId: null, workspacePath: normalizeWorkspacePath(trimmed) }; + } + + const workspacePath = normalizeWorkspacePath(match[1]); + const worktreeDirName = match[2]; + + const dbPath = getGlobalDbPath(); + if (!existsSync(dbPath)) { + throw new RecipientResolutionError( + `Cannot resolve the builder owning worktree '${trimmed}': global.db not found at ${dbPath}.`, + ); + } + + let gdb: Database.Database; + try { + gdb = new Database(dbPath, { readonly: true }); + } catch (err) { + throw new RecipientResolutionError(describeStateDbOpenFailure(dbPath, worktreeDirName, err)); + } + + try { + const rows = gdb + .prepare('SELECT id, worktree FROM builders WHERE workspace_path = ? AND worktree IS NOT NULL') + .all(workspacePath) as Array<{ id: string; worktree: string }>; + // Scoped to this workspace, then matched on the full path or the worktree + // directory name — which is unique within a workspace. Same shape as + // detectCurrentBuilderId's lookup, and deliberately not a tail match on the + // builder id. + const owner = rows.find( + r => normalizeWorkspacePath(r.worktree) === normalizeWorkspacePath(trimmed) + || r.worktree.replace(/\/+$/, '').split('/').pop() === worktreeDirName, + ); + if (owner) return { builderId: owner.id, workspacePath }; + + throw new RecipientResolutionError( + `No builder in workspace '${workspacePath}' owns worktree '${trimmed}' ` + + `(registered: ${rows.length ? rows.map(r => r.id).join(', ') : ''}). ` + + `Refusing to guess a recipient — addressing a plausible neighbour is how a ` + + `notification for one project reaches another (issue #264).`, + ); + } finally { + gdb.close(); + } +} + /** * Read file content for --file flag, with size validation. */ @@ -372,8 +473,10 @@ export async function send(options: SendOptions): Promise { let message = options.message; let target = options.builder; - // When using --all, the first positional arg (builder) is actually the message - if (options.all && target && !message) { + // When using --all, the first positional arg (builder) is actually the message. + // `--worktree` names the recipient the same way, so it shifts the positionals + // identically. + if ((options.all || options.worktree) && target && !message) { message = target; target = undefined; } @@ -388,10 +491,14 @@ export async function send(options: SendOptions): Promise { fatal('No message provided. Usage: afx send "message" or afx send --all "message"'); } - if (!options.all && !target) { + if (!options.all && !options.worktree && !target) { fatal('Must specify a builder ID or use --all flag. Usage: afx send "message"'); } + if (options.all && options.worktree) { + fatal('Cannot use --all with --worktree: one addresses every builder, the other addresses exactly one.'); + } + if (options.all && target) { fatal('Cannot use --all with a specific builder ID.'); } @@ -404,8 +511,36 @@ export async function send(options: SendOptions): Promise { logger.header('Sending Instruction'); - // Detect workspace for target resolution and sender provenance - const workspace = detectWorkspaceRoot() ?? undefined; + // Detect workspace for target resolution and sender provenance. + // + // Issue #264: these are two different questions and used to share one answer. + // `fromWorkspace` is provenance — where the SENDER is — and belongs to the + // session. The resolution scope belongs to the RECIPIENT, and when the caller + // names the project's worktree it comes from there instead, so a notification + // cannot follow the sending process into a workspace the project has nothing + // to do with. + const senderWorkspace = detectWorkspaceRoot() ?? undefined; + let workspace = senderWorkspace; + + if (options.worktree && target) { + // The recipient is already named; the worktree pins only the scope. + workspace = workspaceForWorktree(options.worktree); + } else if (options.worktree) { + let recipient: RecipientWorktree; + try { + recipient = resolveRecipientWorktree(options.worktree); + } catch (err) { + fatal(err instanceof Error ? err.message : String(err)); + } + workspace = recipient!.workspacePath; + if (!recipient!.builderId) { + fatal( + `--worktree '${options.worktree}' is not a builder worktree, so it names no recipient. ` + + `Address the agent explicitly, or pass a '/.builders/' path.`, + ); + } + target = recipient!.builderId!; + } // Detect sender identity (builder ID if in a worktree, otherwise 'architect'). // In a confirmed builder worktree, detectCurrentBuilderId throws when the @@ -439,7 +574,7 @@ export async function send(options: SendOptions): Promise { if (options.all) { // Broadcast to all builders - const results = await sendToAll(client, message, workspace, from, options); + const results = await sendToAll(client, message, senderWorkspace, from, options); if (results.delivered.length > 0) { logger.success(`Delivered to ${results.delivered.length} builder(s): ${results.delivered.join(', ')}`); @@ -467,11 +602,12 @@ export async function send(options: SendOptions): Promise { from, fromName: fromName ?? undefined, workspace, - fromWorkspace: workspace, + fromWorkspace: senderWorkspace, raw: options.raw, noEnter: options.noEnter, interrupt: options.interrupt, deliverAfter: options.delay, + exact: options.exact, }); if (!result.ok) { diff --git a/packages/codev/src/agent-farm/servers/tower-messages.ts b/packages/codev/src/agent-farm/servers/tower-messages.ts index 03edfc5ed..781df62c3 100644 --- a/packages/codev/src/agent-farm/servers/tower-messages.ts +++ b/packages/codev/src/agent-farm/servers/tower-messages.ts @@ -38,6 +38,27 @@ export function addressSpoofingErrorMessage(builderId: string): string { return `builder ${builderId} may only address its own spawning architect`; } +/** + * The NOT_FOUND for an exact-only address (issue #264). + * + * It names the address, the workspace it was resolved in, and who is actually + * there — because the failure mode this replaces was silent success against the + * wrong agent. A caller reading this must be able to see that it asked the + * wrong workspace, without having to reproduce the send. + */ +export function exactMissErrorMessage( + agent: string, + workspacePath: string, + registeredBuilders: string[], +): string { + const names = registeredBuilders.length ? registeredBuilders.join(', ') : ''; + return ( + `Agent '${agent}' is not a builder in workspace '${workspacePath}' ` + + `(builders there: ${names}). This send required an exact match, so no ` + + `tail match was attempted — nothing was delivered.` + ); +} + /** * Heuristic: does `sender` look like a builder identity (canonical * `builder--` or the bare worktree form `-[-slug]`) @@ -126,6 +147,24 @@ export interface ResolveError { message: string; } +/** + * Resolution strictness. + * + * The builder tail match (`250` -> `builder-spir-250`) is a convenience for a + * human typing an address. On a machine-generated, authority-adjacent message + * it is a way to reach the wrong agent silently: issue #264 had a gate-approval + * notification for a project in a throwaway temp workspace land on a live + * builder in another workspace, because the sending process's session decided + * the workspace and `250` tail-matched the builder there. + * + * `exact: true` refuses that convenience. The address must match a builder id + * outright; anything else is NOT_FOUND naming what could not be resolved, + * never a plausible neighbour. + */ +export interface ResolveOptions { + exact?: boolean; +} + /** * Resolve a `[project:]agent` address to a terminal ID. * @@ -147,12 +186,14 @@ export interface ResolveError { * @param fallbackWorkspace - Workspace path when no project: prefix is given * @param sender - Optional sender identity (a builder ID or 'architect'). * Enables affinity-aware architect routing per Spec 755. + * @param options - `exact: true` disables the builder tail match (issue #264). * @returns ResolveResult on success, ResolveError on failure */ export function resolveTarget( target: string, fallbackWorkspace?: string, sender?: string, + options?: ResolveOptions, ): ResolveResult | ResolveError { const { project, agent } = parseAddress(target); @@ -196,7 +237,7 @@ export function resolveTarget( } // Resolve agent within the workspace - return resolveAgentInWorkspace(agent, workspacePath, sender); + return resolveAgentInWorkspace(agent, workspacePath, sender, options); } /** @@ -298,6 +339,7 @@ function resolveAgentInWorkspace( agent: string, workspacePath: string, sender?: string, + options?: ResolveOptions, ): ResolveResult | ResolveError { const allWorkspaces = getWorkspaceTerminals(); const entry = allWorkspaces.get(workspacePath); @@ -384,30 +426,35 @@ function resolveAgentInWorkspace( } } - // Check builders — tail match with leading-zero stripping - const strippedAgent = stripLeadingZeros(agent).toLowerCase(); - const tailMatches: Array<{ builderId: string; terminalId: string }> = []; - - for (const [builderId, terminalId] of entry.builders) { - if (builderId.toLowerCase().endsWith(`-${strippedAgent}`)) { - tailMatches.push({ builderId, terminalId }); + // Check builders — tail match with leading-zero stripping. + // Issue #264: `exact` removes THIS step and only this step. Skipping the + // shell lookup below would make the flag wider than its name, and a shell is + // matched by its full id anyway — the fuzziness being refused lives here. + if (!options?.exact) { + const strippedAgent = stripLeadingZeros(agent).toLowerCase(); + const tailMatches: Array<{ builderId: string; terminalId: string }> = []; + + for (const [builderId, terminalId] of entry.builders) { + if (builderId.toLowerCase().endsWith(`-${strippedAgent}`)) { + tailMatches.push({ builderId, terminalId }); + } } - } - if (tailMatches.length === 1) { - return { - terminalId: tailMatches[0].terminalId, - workspacePath, - agent: tailMatches[0].builderId, - }; - } + if (tailMatches.length === 1) { + return { + terminalId: tailMatches[0].terminalId, + workspacePath, + agent: tailMatches[0].builderId, + }; + } - if (tailMatches.length > 1) { - const candidates = tailMatches.map(m => m.builderId).join(', '); - return { - code: 'AMBIGUOUS', - message: `Agent '${agent}' is ambiguous — matches ${tailMatches.length} builders: ${candidates}. Use the full name.`, - }; + if (tailMatches.length > 1) { + const candidates = tailMatches.map(m => m.builderId).join(', '); + return { + code: 'AMBIGUOUS', + message: `Agent '${agent}' is ambiguous — matches ${tailMatches.length} builders: ${candidates}. Use the full name.`, + }; + } } // Check shells — exact match @@ -417,6 +464,13 @@ function resolveAgentInWorkspace( } } + if (options?.exact) { + return { + code: 'NOT_FOUND', + message: exactMissErrorMessage(agent, workspacePath, [...entry.builders.keys()]), + }; + } + return { code: 'NOT_FOUND', message: `Agent '${agent}' not found in workspace '${path.basename(workspacePath)}'.`, @@ -463,6 +517,7 @@ export function resolveAgentInRegistry( target: string, fallbackWorkspace?: string, sender?: string, + options?: ResolveOptions, ): RegistryResolveResult | ResolveError { const { project, agent } = parseAddress(target); @@ -506,6 +561,16 @@ export function resolveAgentInRegistry( for (const b of builders) { if (b.id.toLowerCase() === lower) return { workspacePath: ws, agent: b.id, kind: 'builder' }; } + // Issue #264: the exact-only contract holds on the offline-hold path too. + // Holding mail for a tail-matched neighbour is the same misdelivery, merely + // slower. + if (options?.exact) { + return { + code: 'NOT_FOUND', + message: exactMissErrorMessage(agent, ws, builders.map((b) => b.id)), + }; + } + const stripped = stripLeadingZeros(agent).toLowerCase(); const tail = builders.filter((b) => b.id.toLowerCase().endsWith(`-${stripped}`)); if (tail.length === 1) return { workspacePath: ws, agent: tail[0].id, kind: 'builder' }; diff --git a/packages/codev/src/agent-farm/servers/tower-routes.ts b/packages/codev/src/agent-farm/servers/tower-routes.ts index 986d074f8..fb0133205 100644 --- a/packages/codev/src/agent-farm/servers/tower-routes.ts +++ b/packages/codev/src/agent-farm/servers/tower-routes.ts @@ -1693,6 +1693,8 @@ interface DelayedSendParams { raw: boolean; noEnter: boolean; interrupt: boolean; + /** Issue #264: exact-only resolution; no builder tail match. */ + exact: boolean; deliverAfter: number; senderWorkspace: string; } @@ -1731,7 +1733,7 @@ function handleDelayedSend( db: ReturnType, params: DelayedSendParams, ): void { - const { to, workspace, from, fromName, message, raw, noEnter, interrupt, deliverAfter, senderWorkspace } = params; + const { to, workspace, from, fromName, message, raw, noEnter, interrupt, exact, deliverAfter, senderWorkspace } = params; const now = Date.now(); const notBefore = now + deliverAfter * 1000; @@ -1741,7 +1743,7 @@ function handleDelayedSend( let isArchitectTarget: boolean; let terminalId: string | null; - const live = resolveTarget(to, workspace, from); + const live = resolveTarget(to, workspace, from, { exact }); if (!isResolveError(live)) { const identity = liveTargetIdentity(live); workspacePath = live.workspacePath; @@ -1749,7 +1751,7 @@ function handleDelayedSend( isArchitectTarget = identity.isArchitectTarget; terminalId = live.terminalId; } else if (live.code === 'NOT_FOUND') { - const reg = resolveAgentInRegistry(to, workspace, from); + const reg = resolveAgentInRegistry(to, workspace, from, { exact }); if (isResolveError(reg)) { const statusCode = reg.code === 'AMBIGUOUS' ? 409 : reg.code === 'NO_CONTEXT' ? 400 : 404; const errorCode = reg.code === 'NO_CONTEXT' ? 'INVALID_PARAMS' : reg.code; @@ -1888,6 +1890,10 @@ async function handleSend( const noEnter = options.noEnter === true; const interrupt = options.interrupt === true; const escape = options.escape === true; + // Issue #264: exact-only resolution. Machine-generated, authority-adjacent + // sends (porch's gate notification) set this so a miss is an error naming the + // address, never a tail match onto a plausible neighbour. + const exact = options.exact === true; // Spec 1307 `--delay` (re-homed onto the mailbox): optional deferred delivery. // Validated here as well as at the CLI boundary — /api/send is a public route, so an @@ -1925,7 +1931,7 @@ async function handleSend( // makes the delay durable across a Tower restart. if (deliverAfter !== undefined) { handleDelayedSend(res, ctx, db, { - to, workspace, from, fromName, message, raw, noEnter, interrupt, deliverAfter, senderWorkspace, + to, workspace, from, fromName, message, raw, noEnter, interrupt, exact, deliverAfter, senderWorkspace, }); return; } @@ -1933,7 +1939,7 @@ async function handleSend( // Resolve the target address against LIVE terminals. // Spec 755: pass `from` so architect resolution is sender-affinity-aware // when the sender is a builder. Non-builder senders see unchanged behavior. - const result = resolveTarget(to, workspace, from); + const result = resolveTarget(to, workspace, from, { exact }); // --- Resolution failed against live terminals --- if (isResolveError(result)) { @@ -1942,7 +1948,7 @@ async function handleSend( // its mail instead of 404ing. escape/interrupt act on a live session only, so // an unresolved target keeps the original error for them. if (result.code === 'NOT_FOUND' && !escape && !interrupt) { - const reg = resolveAgentInRegistry(to, workspace, from); + const reg = resolveAgentInRegistry(to, workspace, from, { exact }); if (!isResolveError(reg)) { const formattedMessage = formatMessageForTarget(reg.kind === 'architect', from, message, raw); const row = enqueueMailbox(db, { diff --git a/packages/codev/src/agent-farm/types.ts b/packages/codev/src/agent-farm/types.ts index 390756daa..6b3f7ef6b 100644 --- a/packages/codev/src/agent-farm/types.ts +++ b/packages/codev/src/agent-farm/types.ts @@ -188,6 +188,18 @@ export interface SendOptions { interrupt?: boolean; raw?: boolean; // Skip structured formatting noEnter?: boolean; // Don't send Enter after message + /** + * Issue #264: resolve the address exactly — no builder tail match, no + * plausible neighbour. A miss names the address and the workspace and sends + * nothing. + */ + exact?: boolean; + /** + * Issue #264: resolve the recipient and the resolution workspace from THIS + * worktree path rather than from the sending process's session or cwd. With no + * explicit target, the recipient is the builder that owns the worktree. + */ + worktree?: string; /** * Spec 1307: hold in Tower and deliver after this many seconds. Resolution * and authorization still happen at request time; only delivery is deferred. diff --git a/packages/codev/src/commands/porch/__tests__/bugfix-264-gate-notify-addressing.test.ts b/packages/codev/src/commands/porch/__tests__/bugfix-264-gate-notify-addressing.test.ts new file mode 100644 index 000000000..9f75db634 --- /dev/null +++ b/packages/codev/src/commands/porch/__tests__/bugfix-264-gate-notify-addressing.test.ts @@ -0,0 +1,71 @@ +/** + * Issue #264 — what `porch approve` puts on the wire after a gate is approved. + * + * The defect was an address: `afx send ""`, run from + * wherever porch happened to be, with the workspace inherited from the sending + * process's session. A project id is not an identity — `250` reaches + * `builder-spir-250` in any workspace that has one — so an approval for a + * throwaway project in a temp workspace woke a live builder elsewhere, and the + * message it delivered was indistinguishable from a real one. + * + * These assert the two halves of the fix at the seam where the address is + * decided: the argv, and the message text. + */ + +import { describe, it, expect } from 'vitest'; +import { buildSendArgs, gateApprovedMessage } from '../notify.js'; + +const WORKTREE = '/Users/dev/codev-1455/.builders/spir-250'; + +function gateArgs(): string[] { + return buildSendArgs({ + message: gateApprovedMessage('pr', '250', WORKTREE), + worktreeDir: WORKTREE, + projectWorktree: WORKTREE, + exact: true, + }); +} + +describe('issue #264 — the gate wake-up addresses the project’s own worktree', () => { + it('pins the recipient to the worktree whose status.yaml was written', () => { + const args = gateArgs(); + expect(args[0]).toBe('send'); + expect(args).toContain('--worktree'); + expect(args[args.indexOf('--worktree') + 1]).toBe(WORKTREE); + }); + + it('never puts the bare project id on the wire as an address', () => { + // The whole defect in one assertion: `250` as a target is what the tail + // match turned into a delivery to a project that approved nothing. + const args = gateArgs(); + expect(args).not.toContain('250'); + }); + + it('demands an exact match, so a miss cannot become a delivery', () => { + expect(gateArgs()).toContain('--exact'); + }); + + it('keeps the message a raw wake-up', () => { + expect(gateArgs()).toContain('--raw'); + }); +}); + +describe('issue #264 — the message is checkable, not an instruction', () => { + const message = gateApprovedMessage('pr', '250', WORKTREE); + + it('names the project and the workspace it is about', () => { + expect(message).toContain('250'); + expect(message).toContain(WORKTREE); + }); + + it('says outright that it carries no authority', () => { + expect(message).toMatch(/not an approval/i); + expect(message).toMatch(/no authority/i); + }); + + it('tells the recipient what to check it against, and what a mismatch means', () => { + expect(message).toContain('porch next 250'); + expect(message).toMatch(/still pending/i); + expect(message).toMatch(/ignore it/i); + }); +}); diff --git a/packages/codev/src/commands/porch/__tests__/notify.test.ts b/packages/codev/src/commands/porch/__tests__/notify.test.ts index 4df86b3fb..78588fb98 100644 --- a/packages/codev/src/commands/porch/__tests__/notify.test.ts +++ b/packages/codev/src/commands/porch/__tests__/notify.test.ts @@ -23,7 +23,13 @@ vi.mock('../../../lib/test-env.js', () => ({ })); import { execFile } from 'node:child_process'; -import { notifyTerminal, gateApprovedMessage, protocolCompleteMessage, notifyProtocolComplete } from '../notify.js'; +import { + notifyTerminal, + gateApprovedMessage, + protocolCompleteMessage, + notifyProtocolComplete, + notifyGateApproved, +} from '../notify.js'; const mockExecFile = vi.mocked(execFile); @@ -126,6 +132,7 @@ describe('notifyTerminal', () => { mockIsUnderTestRunner.mockReturnValue(true); notifyTerminal({ target: 'architect', message: 'x', worktreeDir: '/p' }); notifyProtocolComplete('/p', 'bugfix-147'); + notifyGateApproved('/p/.builders/pir-108', '108', 'pr'); expect(mockExecFile).not.toHaveBeenCalled(); }); @@ -133,14 +140,14 @@ describe('notifyTerminal', () => { notifyProtocolComplete('/projects/test', 'bugfix-147'); const args = mockExecFile.mock.calls[0][1]!; expect(args).toContain('send'); - expect(args[args.indexOf('send') + 1]).toBe('architect'); - expect(args).toContain(protocolCompleteMessage('bugfix-147')); + expect(args).toContain('architect'); + expect(args).toContain(protocolCompleteMessage('bugfix-147', '/projects/test')); }); }); describe('gateApprovedMessage', () => { it('references the gate and porch next', () => { - const msg = gateApprovedMessage('dev-approval'); + const msg = gateApprovedMessage('dev-approval', '108', '/ws/.builders/pir-108'); expect(msg).toContain('dev-approval'); expect(msg).toContain('porch next'); }); @@ -148,7 +155,7 @@ describe('gateApprovedMessage', () => { describe('protocolCompleteMessage (issue #109)', () => { it('names the project and cleanup', () => { - const msg = protocolCompleteMessage('bugfix-147'); + const msg = protocolCompleteMessage('bugfix-147', '/ws/.builders/bugfix-147'); expect(msg).toContain('bugfix-147'); expect(msg).toMatch(/protocol complete/i); expect(msg).toMatch(/cleanup/i); diff --git a/packages/codev/src/commands/porch/index.ts b/packages/codev/src/commands/porch/index.ts index 04f561758..03d6005a1 100644 --- a/packages/codev/src/commands/porch/index.ts +++ b/packages/codev/src/commands/porch/index.ts @@ -59,7 +59,7 @@ import { loadCheckOverrides, resolveConsultationModels } from './config.js'; import { findUnlandedCommits, completionReport } from './unlanded.js'; import { resolveDefaultBranch } from '../../lib/default-branch.js'; -import { notifyTerminal, gateApprovedMessage, notifyProtocolComplete } from './notify.js'; +import { notifyGateApproved, notifyProtocolComplete } from './notify.js'; import type { ApprovalRecord } from './approval-record.js'; import { loadConfig } from '../../lib/config.js'; import { version } from '../../version.js'; @@ -1264,11 +1264,12 @@ export async function approve( // inside the worktree it resolves to the same path as artifactRoot. const calledFromBuilderWorktree = path.resolve(workspaceRoot) === path.resolve(artifactRoot); if (!calledFromBuilderWorktree) { - notifyTerminal({ - target: state.id, - message: gateApprovedMessage(gateName), - worktreeDir: workspaceRoot, - }); + // Issue #264: addressed by the PROJECT's worktree, never by its bare id. + // `state.id` plus the invoking process's workspace was an address that could + // resolve anywhere on the machine; `artifactRoot` is the directory whose + // status.yaml was just written, so it names one builder in one workspace or + // it names nothing. + notifyGateApproved(artifactRoot, state.id, gateName); } say(''); diff --git a/packages/codev/src/commands/porch/notify.ts b/packages/codev/src/commands/porch/notify.ts index 471c33607..1c315bee7 100644 --- a/packages/codev/src/commands/porch/notify.ts +++ b/packages/codev/src/commands/porch/notify.ts @@ -1,6 +1,6 @@ /** - * Porch terminal notifications — sends `afx send ` to deliver - * messages into a target terminal as PTY input. + * Porch terminal notifications — sends `afx send` to deliver messages into a + * target terminal as PTY input. * * Two callers: * - gate approval: wake the builder so an idle session runs `porch next` @@ -10,6 +10,20 @@ * human-decision points, and the architect cannot approve them. Protocol * complete is different — it is the cleanup trigger, and without it * finished builders sit until a human happens to look. + * + * ADDRESSING IS THE SAFETY PROPERTY HERE (issue #264). + * + * A gate is where a human's authority enters the system. A message saying a + * gate was approved, delivered to a project that approved nothing, is a channel + * around that authority — and it happened: `porch approve` addressed the + * recipient by bare project id and let the SENDING PROCESS's session decide the + * workspace, so an approval for a throwaway project in a temp workspace woke a + * live builder in another workspace whose id merely ended in the same digits. + * + * Every send from this module therefore names the project's OWN worktree + * (`--worktree`), which fixes both the workspace and the recipient to the + * project's location on disk, and demands an exact match (`--exact`), so a miss + * is a logged error rather than a delivery to a plausible neighbour. */ import { execFile } from 'node:child_process'; @@ -23,35 +37,78 @@ function resolveAfxBinary(): string { } export interface NotifyTerminalOptions { - /** Target terminal — currently always a builder ID (e.g., 'pir-1298'). */ - target: string; + /** Target agent — a builder ID, or 'architect'. Omit to address the worktree's own builder. */ + target?: string; /** Message text to deliver. */ message: string; /** Working directory — used by afx to resolve the workspace. */ worktreeDir: string; + /** + * The worktree that owns the project this notification is about. Pins BOTH + * the workspace and (when `target` is omitted) the recipient, so neither is + * inherited from whatever process happens to be running porch. + */ + projectWorktree?: string; + /** Refuse the builder tail match — a miss must not become a delivery. */ + exact?: boolean; } -/** Builder-bound wake-up after a gate is approved. */ -export function gateApprovedMessage(gateName: string): string { - return `Gate ${gateName} approved — please run \`porch next\` to advance.`; +/** + * Builder-bound wake-up after a gate is approved. + * + * The text is written to be CHECKABLE. The old wording — "Gate pr approved — + * please run `porch next` to advance" — was an instruction carrying no project + * id, no workspace, and nothing a recipient could test it against, so a + * misdelivered copy read exactly like a real one. This names what was approved + * and where, and says plainly that the message is not the authority. + */ +export function gateApprovedMessage( + gateName: string, + projectId: string, + workspacePath: string, +): string { + return ( + `Notification, not an approval: porch recorded gate ${gateName} approved for ` + + `project ${projectId} in ${workspacePath}. This message carries no authority. ` + + `Confirm with \`porch next ${projectId}\` before advancing — if that shows the ` + + `gate still pending, or names a project that is not yours, this was not meant ` + + `for you: ignore it and report it.` + ); } /** Architect-bound cleanup trigger when a protocol reaches verified. */ -export function protocolCompleteMessage(projectId: string): string { - return `Project ${projectId} protocol complete. Ready for cleanup.`; +export function protocolCompleteMessage(projectId: string, workspacePath: string): string { + return `Project ${projectId} in ${workspacePath} reports protocol complete. Ready for cleanup.`; } -export function notifyProtocolComplete(workspaceRoot: string, projectId: string): void { +export function notifyProtocolComplete(artifactRoot: string, projectId: string): void { notifyTerminal({ target: 'architect', - message: protocolCompleteMessage(projectId), - worktreeDir: workspaceRoot, + message: protocolCompleteMessage(projectId, artifactRoot), + worktreeDir: artifactRoot, + // Pinned to the project's worktree for the same reason the gate wake-up is: + // `afx cleanup` is destructive, and this message is what prompts it. + projectWorktree: artifactRoot, + }); +} + +/** Builder-bound wake-up, addressed to the builder that owns the project. */ +export function notifyGateApproved( + artifactRoot: string, + projectId: string, + gateName: string, +): void { + notifyTerminal({ + message: gateApprovedMessage(gateName, projectId, artifactRoot), + worktreeDir: artifactRoot, + projectWorktree: artifactRoot, + exact: true, }); } /** * Fire-and-forget notification to a terminal. - * Uses `afx send ` via execFile (no shell, no injection risk). + * Uses `afx send` via execFile (no shell, no injection risk). * Errors are logged but never thrown — notification is best-effort. */ export function notifyTerminal(opts: NotifyTerminalOptions): void { @@ -65,12 +122,30 @@ export function notifyTerminal(opts: NotifyTerminalOptions): void { execFile( process.execPath, - [afBinary, 'send', opts.target, opts.message, '--raw'], + [afBinary, ...buildSendArgs(opts)], { cwd: opts.worktreeDir, timeout: 10_000 }, (error) => { if (error) { - console.error(`[porch] notifyTerminal(${opts.target}) failed: ${error.message}`); + console.error( + `[porch] notifyTerminal(${opts.target ?? opts.projectWorktree ?? '?'}) failed: ${error.message}`, + ); } } ); } + +/** + * The `afx send` argv for a notification. + * + * Exported so a test can assert the addressing WITHOUT spawning a process: the + * regression this pins is which agent gets addressed and in which workspace, and + * that is decided entirely here. + */ +export function buildSendArgs(opts: NotifyTerminalOptions): string[] { + const args = ['send']; + if (opts.projectWorktree) args.push('--worktree', opts.projectWorktree); + if (opts.exact) args.push('--exact'); + if (opts.target) args.push(opts.target); + args.push(opts.message, '--raw'); + return args; +} diff --git a/packages/sdk/src/tower-client.ts b/packages/sdk/src/tower-client.ts index e4c6f18b4..c14cdbfce 100644 --- a/packages/sdk/src/tower-client.ts +++ b/packages/sdk/src/tower-client.ts @@ -824,6 +824,17 @@ export class TowerClient { * the pending send is listable/cancellable via `afx inbox`. */ deliverAfter?: number; + /** + * Issue #264: resolve the address EXACTLY — no builder tail match. + * + * The tail match (`250` -> `builder-spir-250`) is a convenience for a human + * typing an address. A machine-generated, authority-adjacent message must + * not have it: a gate-approval notification addressed by bare project id + * reached a live builder in a different workspace whose id merely ended + * with the same digits. With this set, a miss is an error naming the + * address and the workspace, and nothing is delivered. + */ + exact?: boolean; }, ): Promise<{ ok: boolean; @@ -901,6 +912,7 @@ export class TowerClient { interrupt: options?.interrupt, escape: options?.escape, deliverAfter: options?.deliverAfter, + exact: options?.exact, }, }), },