From d4e9d5f399526c81a4ad6df5c1ad687285004eee Mon Sep 17 00:00:00 2001 From: pseudo Date: Sat, 22 Aug 2026 13:57:22 -0600 Subject: [PATCH 1/3] Fix #41: a Stop hook, because reporting and stopping are the same act MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A builder ends its turn mid-phase with nothing blocking it and nobody notices for hours. Two incidents a week apart. The second one's last message named the next phase it was about to start — and then did nothing: - Phase 1 committed as e62d1c0; build and tests green - Moving to phase 2, the seam measurement harness - No action needed from you It was not confused. It intended to continue. Its turn ended BECAUSE it wrote that paragraph: the loop runs while the model emits tool calls and terminates at its first response that is only prose. "Report, then keep working" cannot happen in that order. #40 rewrote porch's handoff box and the role doc to say this out loud, which makes the mistake harder. It cannot make it impossible — documentation loads into context and can be read past. This is enforcement: the harness runs the hook whether or not the model remembers. The guard rides the seam that already exists. buildWorktreeGuardFiles emits worktree-write-guard.cjs plus a PreToolUse entry into every Claude builder worktree at spawn; it now also emits phase-stop-guard.cjs and a Stop entry into the SAME settings file, because two writers of one file is how one of them silently loses. Three departures from a hand-written version of this that has been sitting unwired in a home directory since 2026-08-15: It reads status.yaml rather than grepping `porch status` output. That version matched on four gate phrases porch does not print, so it would have nudged a builder parked at dev-approval to "do the next unit of work now" — pushing it past a decision only a human may make. Display strings are a contract nobody enforces; the state file's shape is fixed by porch's writer. The project id is baked in at spawn from porch's OWN detectProjectIdFromCwd, not a second copy of the rule. A guard that disagrees with porch about which project it watches nudges about the wrong one. Node, not bash: the old one needed /usr/bin/jq and a porch on PATH, and adopter machines are not guaranteed either. detectProjectIdFromCwd moves to a leaf module. Importing porch/state.js reaches its module-load `promisify(execFile)`, and that new import edge failed 24 doctor tests whose partial node:child_process mock has no execFile — a failure in a file this change never touched. 24 new tests, ten allow-paths to one block-path. That ratio is the design: a guard that fails to block costs one idle builder, and a guard that blocks wrongly can trap a session or shove a builder through a human gate. Co-Authored-By: Claude Opus 5 (1M context) --- codev-skeleton/roles/builder.md | 24 ++ codev/roles/builder.md | 24 ++ .../src/__tests__/phase-stop-guard.test.ts | 254 ++++++++++++++++++ .../src/agent-farm/utils/phase-stop-guard.ts | 252 +++++++++++++++++ .../agent-farm/utils/worktree-write-guard.ts | 50 +++- .../codev/src/commands/porch/project-id.ts | 41 +++ packages/codev/src/commands/porch/state.ts | 27 +- 7 files changed, 649 insertions(+), 23 deletions(-) create mode 100644 packages/codev/src/__tests__/phase-stop-guard.test.ts create mode 100644 packages/codev/src/agent-farm/utils/phase-stop-guard.ts create mode 100644 packages/codev/src/commands/porch/project-id.ts diff --git a/codev-skeleton/roles/builder.md b/codev-skeleton/roles/builder.md index 55bfe9b1d..dc5261050 100644 --- a/codev-skeleton/roles/builder.md +++ b/codev-skeleton/roles/builder.md @@ -26,6 +26,30 @@ Porch's `DO NOT start until you run porch again` is narrow: it forbids s to a later phase. It has never meant stop and wait. Read it as a fence on the far side of your current phase, not a gate in front of it. +### Writing a summary *is* ending the turn + +This is the part that catches builders who were not trying to stop at all. + +Your turn runs while you emit tool calls. It ends at your first response that is only prose. So +a closing summary is not something you write *before* continuing — writing it **is** the act of +stopping. "Report, then keep working" cannot happen in that order. + +A builder ended its turn with `Moving to phase 2, the seam measurement harness` and then did +nothing for hours. It was not confused about what to do next; it named the next phase. That +sentence became false in the act of writing it, because nothing of yours runs between user +messages. + +The distinction is mechanical, and it is the whole trick: + +| | Ends your turn? | +|---|---| +| `afx send architect "..."` — a tool call | **No.** Keep working in the same turn. | +| A summary or status paragraph in your pane | **Yes.** Immediately, whatever it says. | + +So report with `afx send` and keep going. Write a closing summary only when you are actually +done or actually blocked — never as a milestone marker, and never containing a promise about +what you will do next, because you will not be there to do it. + There are exactly three reasons to end a turn mid-project: 1. A **human gate** — porch says `WAITING FOR HUMAN APPROVAL`, or your phase prompt says stop. diff --git a/codev/roles/builder.md b/codev/roles/builder.md index 55bfe9b1d..dc5261050 100644 --- a/codev/roles/builder.md +++ b/codev/roles/builder.md @@ -26,6 +26,30 @@ Porch's `DO NOT start until you run porch again` is narrow: it forbids s to a later phase. It has never meant stop and wait. Read it as a fence on the far side of your current phase, not a gate in front of it. +### Writing a summary *is* ending the turn + +This is the part that catches builders who were not trying to stop at all. + +Your turn runs while you emit tool calls. It ends at your first response that is only prose. So +a closing summary is not something you write *before* continuing — writing it **is** the act of +stopping. "Report, then keep working" cannot happen in that order. + +A builder ended its turn with `Moving to phase 2, the seam measurement harness` and then did +nothing for hours. It was not confused about what to do next; it named the next phase. That +sentence became false in the act of writing it, because nothing of yours runs between user +messages. + +The distinction is mechanical, and it is the whole trick: + +| | Ends your turn? | +|---|---| +| `afx send architect "..."` — a tool call | **No.** Keep working in the same turn. | +| A summary or status paragraph in your pane | **Yes.** Immediately, whatever it says. | + +So report with `afx send` and keep going. Write a closing summary only when you are actually +done or actually blocked — never as a milestone marker, and never containing a promise about +what you will do next, because you will not be there to do it. + There are exactly three reasons to end a turn mid-project: 1. A **human gate** — porch says `WAITING FOR HUMAN APPROVAL`, or your phase prompt says stop. diff --git a/packages/codev/src/__tests__/phase-stop-guard.test.ts b/packages/codev/src/__tests__/phase-stop-guard.test.ts new file mode 100644 index 000000000..44edf0fb2 --- /dev/null +++ b/packages/codev/src/__tests__/phase-stop-guard.test.ts @@ -0,0 +1,254 @@ +/** + * Tests for the builder phase stop-guard (Issue #41). + * + * Like the write-guard tests, these exercise the EXACT emitted artifact: the + * script constant is written to a temp .cjs and spawned with fixture stdin, so + * the tested behavior is the behavior builders get. + * + * The ordering below is deliberate. The allow-paths come first and outnumber + * the block-path, because that is the risk profile: a guard that fails to block + * costs one idle builder, and a guard that blocks when it should not can trap a + * session or push a builder past a human approval gate. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { + PHASE_STOP_GUARD_SCRIPT, + STOP_GUARD_SCRIPT_RELPATH, + TERMINAL_PHASES, + buildPhaseStopGuardCommand, +} from '../agent-farm/utils/phase-stop-guard.js'; +import { + buildWorktreeGuardFiles, + GUARD_SCRIPT_RELPATH, + GUARD_SETTINGS_RELPATH, +} from '../agent-farm/utils/worktree-write-guard.js'; + +const FIXTURE_HOME = path.join(path.resolve(__dirname, '..', '..'), 'node_modules', '.sguard-fixtures'); + +let base: string; +let worktree: string; +let scriptPath: string; + +beforeAll(() => { + fs.mkdirSync(FIXTURE_HOME, { recursive: true }); + base = fs.mkdtempSync(path.join(FIXTURE_HOME, 'sguard-')); + worktree = path.join(base, 'main', '.builders', 'pir-77'); + fs.mkdirSync(worktree, { recursive: true }); + scriptPath = path.join(base, 'stop-guard.cjs'); + fs.writeFileSync(scriptPath, PHASE_STOP_GUARD_SCRIPT); +}); + +afterAll(() => { + fs.rmSync(FIXTURE_HOME, { recursive: true, force: true }); +}); + +/** Write a status.yaml for project `id` inside the fixture worktree. */ +function writeStatus(id: string, slug: string, body: string): void { + const dir = path.join(worktree, 'codev', 'projects', `${id}-${slug}`); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, 'status.yaml'), body); +} + +function clearProjects(): void { + fs.rmSync(path.join(worktree, 'codev', 'projects'), { recursive: true, force: true }); +} + +interface GuardResult { + status: number | null; + blocked: boolean; + reason: string; +} + +function runGuard( + stdin: string, + env: Record = {}, +): GuardResult { + const res = spawnSync(process.execPath, [scriptPath], { + input: stdin, + encoding: 'utf-8', + env: { + ...process.env, + CODEV_WORKTREE_ROOT: worktree, + CODEV_PROJECT_ID: '77', + ...env, + }, + }); + const out = (res.stdout || '').trim(); + if (!out) return { status: res.status, blocked: false, reason: '' }; + try { + const parsed = JSON.parse(out); + return { status: res.status, blocked: parsed.decision === 'block', reason: parsed.reason ?? '' }; + } catch { + return { status: res.status, blocked: false, reason: out }; + } +} + +const MID_PHASE = `id: '77' +title: test +protocol: pir +phase: implement +plan_phases: [] +gates: + plan-approval: + status: approved +build_complete: false +`; + +const STOP = JSON.stringify({ stop_hook_active: false }); + +describe('phase stop-guard: paths that must ALLOW the stop', () => { + it('allows when the hook has already nudged this cycle', () => { + // Without this the guard fights the model forever over a stop it cannot + // talk its way out of. One nudge, then the model's judgment wins. + writeStatus('77', 'test', MID_PHASE); + const r = runGuard(JSON.stringify({ stop_hook_active: true })); + expect(r.blocked).toBe(false); + expect(r.status).toBe(0); + }); + + it.each(TERMINAL_PHASES)('allows at the terminal phase %s', (phase) => { + writeStatus('77', 'test', MID_PHASE.replace('phase: implement', `phase: ${phase}`)); + expect(runGuard(STOP).blocked).toBe(false); + }); + + it('allows when ANY gate is pending', () => { + // The one way this guard can do real harm: nudging a builder parked at a + // human approval gate is pushing it past a decision only a human may make. + writeStatus( + '77', + 'test', + MID_PHASE.replace(' status: approved\n', ' status: approved\n dev-approval:\n status: pending\n'), + ); + const r = runGuard(STOP); + expect(r.blocked).toBe(false); + }); + + it('allows when the status file cannot be read at all', () => { + // "I could not tell" must not be spelled the same way as "no gate here". + clearProjects(); + expect(runGuard(STOP).blocked).toBe(false); + }); + + it('allows when the project id matches no project directory', () => { + writeStatus('77', 'test', MID_PHASE); + expect(runGuard(STOP, { CODEV_PROJECT_ID: '999' }).blocked).toBe(false); + }); + + it('allows when the baked env vars are missing', () => { + writeStatus('77', 'test', MID_PHASE); + expect(runGuard(STOP, { CODEV_WORKTREE_ROOT: undefined }).blocked).toBe(false); + expect(runGuard(STOP, { CODEV_PROJECT_ID: undefined }).blocked).toBe(false); + }); + + it('allows on malformed stdin', () => { + writeStatus('77', 'test', MID_PHASE); + expect(runGuard('not json at all').blocked).toBe(false); + }); + + it('allows on empty stdin', () => { + writeStatus('77', 'test', MID_PHASE); + expect(runGuard('').blocked).toBe(false); + }); + + it('allows when status.yaml has no phase key', () => { + writeStatus('77', 'test', "id: '77'\ntitle: test\n"); + expect(runGuard(STOP).blocked).toBe(false); + }); + + it('exits 0 on every allow path, so a hook failure can never wedge a session', () => { + clearProjects(); + for (const input of ['', 'garbage', STOP]) { + expect(runGuard(input).status).toBe(0); + } + }); +}); + +describe('phase stop-guard: the path that must BLOCK', () => { + it('blocks a mid-phase stop with no gate pending', () => { + writeStatus('77', 'test', MID_PHASE); + const r = runGuard(STOP); + expect(r.blocked).toBe(true); + expect(r.status).toBe(0); + }); + + it('names the phase, so the nudge is about this project and not a generic scold', () => { + writeStatus('77', 'test', MID_PHASE); + expect(runGuard(STOP).reason).toMatch(/phase "implement"/); + }); + + it('explains the mechanism, not just the rule', () => { + // The builder in the second incident knew what to do next and said so. It + // did not know that saying so was the act of stopping. A nudge that only + // repeats "do not stop" leaves that misunderstanding intact. + writeStatus('77', 'test', MID_PHASE); + const reason = runGuard(STOP).reason; + expect(reason).toMatch(/IS the act of ending the turn/); + expect(reason).toMatch(/afx send architect/); + }); + + it('tells the builder how to stop anyway, and that it will not be blocked twice', () => { + writeStatus('77', 'test', MID_PHASE); + expect(runGuard(STOP).reason).toMatch(/will not block you twice/); + }); +}); + +describe('phase stop-guard: id matching', () => { + it('does not let project 77 match project 7713', () => { + // The zero-strip collision that bit artifact lookup in #9. A guard watching + // the wrong project nudges about a phase that is not open. + clearProjects(); + writeStatus('7713', 'other', MID_PHASE); + expect(runGuard(STOP).blocked).toBe(false); + }); + + it('matches an exact directory name with no slug', () => { + clearProjects(); + const dir = path.join(worktree, 'codev', 'projects', '77'); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, 'status.yaml'), MID_PHASE); + expect(runGuard(STOP).blocked).toBe(true); + }); +}); + +describe('worktree wiring', () => { + it('emits the stop-guard script and a Stop hook for a builder worktree', () => { + const files = buildWorktreeGuardFiles('/x/main/.builders/pir-42'); + const paths = files.map(f => f.relativePath); + expect(paths).toContain(GUARD_SCRIPT_RELPATH); + expect(paths).toContain(STOP_GUARD_SCRIPT_RELPATH); + + const settings = JSON.parse(files.find(f => f.relativePath === GUARD_SETTINGS_RELPATH)!.content); + expect(settings.hooks.Stop).toHaveLength(1); + expect(settings.hooks.Stop[0].hooks[0].command).toContain("CODEV_PROJECT_ID='42'"); + // Both guards must survive in one settings file — this function is the + // single owner of it, and dropping either is silent. + expect(settings.hooks.PreToolUse).toHaveLength(1); + }); + + it('derives bugfix project ids the way porch does', () => { + const files = buildWorktreeGuardFiles('/x/main/.builders/bugfix-9-some-slug'); + const settings = JSON.parse(files.find(f => f.relativePath === GUARD_SETTINGS_RELPATH)!.content); + expect(settings.hooks.Stop[0].hooks[0].command).toContain("CODEV_PROJECT_ID='bugfix-9'"); + }); + + it('installs NO Stop hook when the path is not a recognized builder worktree', () => { + // Fail open: an unrecognized path means the guard cannot know which project + // it would be watching, and a guard that guesses nudges about the wrong one. + const files = buildWorktreeGuardFiles('/some/random/dir'); + const settings = JSON.parse(files.find(f => f.relativePath === GUARD_SETTINGS_RELPATH)!.content); + expect(settings.hooks.Stop).toBeUndefined(); + expect(files.map(f => f.relativePath)).not.toContain(STOP_GUARD_SCRIPT_RELPATH); + // The write-guard must still be installed — the two are independent. + expect(settings.hooks.PreToolUse).toHaveLength(1); + }); + + it('bakes an absolute, shell-safe command', () => { + const cmd = buildPhaseStopGuardCommand('/x/main/.builders/pir-42', '42'); + expect(cmd).toContain("CODEV_WORKTREE_ROOT='/x/main/.builders/pir-42'"); + expect(cmd).toContain('/x/main/.builders/pir-42/.claude/hooks/phase-stop-guard.cjs'); + }); +}); diff --git a/packages/codev/src/agent-farm/utils/phase-stop-guard.ts b/packages/codev/src/agent-farm/utils/phase-stop-guard.ts new file mode 100644 index 000000000..46c2a5fcc --- /dev/null +++ b/packages/codev/src/agent-farm/utils/phase-stop-guard.ts @@ -0,0 +1,252 @@ +/** + * Builder phase stop-guard (Issue #41). + * + * ## The failure + * + * A builder ends its turn mid-phase with nothing blocking it, and nobody + * notices until a human opens the pane — routinely hours. Two incidents, one + * week apart, with the same shape. The second one's last message read: + * + * - Phase 1 committed as e62d1c0; build and tests green + * - Moving to phase 2, the seam measurement harness + * - No action needed from you + * + * ...and then it did nothing. It was not confused about what to do next; it + * named the next phase. It intended to continue. + * + * ## Why prompt wording cannot fix it + * + * **Writing a summary to the pane IS ending the turn.** The agentic loop runs + * while the model emits tool calls and terminates at its first response that is + * only prose. So "report, then keep working" cannot happen in that order — + * reporting is the stop. The builder does not experience those as one act, and + * no amount of instruction changes the mechanic, because the instruction is + * itself read at the moment the model has already decided to write prose. + * + * Issue #40 rewrote porch's phase-handoff box and the builder role doc to say + * this out loud, which makes the mistake harder. It cannot make it impossible: + * documentation loads into context and can be read past. A Stop hook is + * enforcement — the harness runs it whether or not the model remembers. + * + * ## Design, in order of how badly each would hurt if got wrong + * + * 1. FAILS OPEN, ALWAYS. Every error path allows the stop. A Stop hook that + * blocks on its own bug traps a builder in a loop it cannot talk its way + * out of. No failure mode here is worse than "the guard did nothing". + * 2. NEVER BLOCKS A HUMAN GATE. Telling a builder parked at `dev-approval` + * to "do the next unit of work now" pushes it past a decision only a human + * may make. Any gate in `pending` allows the stop. When the guard cannot + * tell, it allows: a false "there is a gate" costs one un-nudged turn, a + * false "there is no gate" costs a bypassed gate. + * 3. NUDGES ONCE, NEVER LOOPS. `stop_hook_active` is true when this hook has + * already blocked and the model is continuing because of it. Seeing that, + * allow. The shape is: stop -> one nudge -> work -> stop -> allowed. A + * genuine need to stop costs one extra turn, not an infinite fight. + * + * ## Why it reads status.yaml instead of shelling out to porch + * + * A prior hand-written version of this guard called `porch status` and grepped + * its output for gate wording. It grepped for four phrases porch does not + * print, so it matched nothing and would have nudged at every gate. Display + * strings are a fragile contract between two programs. `status.yaml` is the + * structured source, needs no subprocess, and does not change wording between + * releases. + * + * @see codev/roles/builder.md — "A phase handoff is not a stopping point" + */ + +import { isAbsolute, join, resolve } from 'node:path'; + +/** Worktree-relative path of the emitted guard script. */ +export const STOP_GUARD_SCRIPT_RELPATH = '.claude/hooks/phase-stop-guard.cjs'; + +/** + * Phases at which there is nothing left to drive. + * + * `verified` is porch's own terminal phase — `advanceProtocolPhase` sets it + * when a protocol runs out of phases, and `state.ts` migrates the legacy + * `complete` onto it. Omitting it earns every finished project one pointless + * nudge. + */ +export const TERMINAL_PHASES = ['verified', 'complete', 'completed', 'done', 'cleanup', 'archived']; + +/** + * The self-contained Node guard script, emitted verbatim into each worktree. + * + * Kept as a string constant (not a separate asset) so it ships with the + * compiled package automatically — `tsc` does not copy non-TS files to `dist`. + * The unit test writes this constant to a temp `.cjs` and spawns it with + * fixture stdin, so the constant is the tested artifact. + * + * Self-contained: Node core only. Worktrees have no access to the package's + * node_modules, and neither do adopter repos. It also must not depend on `jq` + * or a `porch` on PATH, both of which are absent on some adopter machines. + */ +export const PHASE_STOP_GUARD_SCRIPT = `#!/usr/bin/env node +// AUTO-GENERATED by @cluesmith/codev (Issue #41). Do not edit in the worktree; +// edit packages/codev/src/agent-farm/utils/phase-stop-guard.ts instead. +'use strict'; + +const fs = require('node:fs'); +const path = require('node:path'); + +const TERMINAL_PHASES = new Set(${JSON.stringify(TERMINAL_PHASES)}); + +/** Allow the stop. Every failure path lands here. */ +function allow() { + process.exit(0); +} + +function block(phase) { + process.stdout.write(JSON.stringify({ + decision: 'block', + reason: [ + 'STOP BLOCKED — porch is still at phase "' + phase + '".', + '', + 'You were about to end the turn while a protocol phase is open. If your last message ' + + 'stated a next action ("next I will...", "continuing with...", "starting X now"), that ' + + 'sentence became false the moment the turn ended: nothing of yours runs between user ' + + 'messages, so ending the turn IS going idle.', + '', + 'Writing a summary to the pane is not a step you take before continuing — it IS the act ' + + 'of ending the turn. A turn runs while you emit tool calls and ends at your first ' + + 'message that is only prose. So "report, then keep working" cannot happen in that order.', + '', + 'Do the next unit of work now, in THIS turn. Reporting is a side channel, never a ' + + 'handoff: \`afx send architect\` is a tool call and does not end your turn, and the ' + + 'architect cannot see porch state.', + '', + 'Legitimate stops: a gate awaiting a human, a blocker you cannot act around, or protocol ' + + 'completion. If this is one of those, say which and stop again — this guard nudges once ' + + 'per cycle and will not block you twice.', + ].join('\\n'), + })); + process.exit(0); +} + +/** + * Locate /codev/projects/-/status.yaml. + * + * The id is baked in at spawn time, but the slug is not, so this matches on the + * id segment. Exact-match first: an id of "13" must not resolve to "1313-..." + * (the zero-strip collision that bit artifact lookup in issue #9). + */ +function findStatusPath(root, projectId) { + const projectsDir = path.join(root, 'codev', 'projects'); + let entries; + try { + entries = fs.readdirSync(projectsDir, { withFileTypes: true }); + } catch { + return null; + } + const dirs = entries.filter((e) => e.isDirectory()).map((e) => e.name); + const prefix = projectId + '-'; + const match = dirs.find((d) => d === projectId || d.startsWith(prefix)); + if (!match) return null; + const p = path.join(projectsDir, match, 'status.yaml'); + return fs.existsSync(p) ? p : null; +} + +/** + * Read \`phase\` and whether any gate is still \`pending\`. + * + * A deliberately small line scanner rather than a YAML parser: this file has no + * dependencies available to it, and status.yaml's shape is fixed by porch's own + * writer. Anything it cannot understand returns nulls, which fail open. + */ +function readState(statusPath) { + let text; + try { + text = fs.readFileSync(statusPath, 'utf8'); + } catch { + return { phase: null, gatePending: null }; + } + + let phase = null; + let gatePending = false; + let inGates = false; + + for (const line of text.split('\\n')) { + const phaseMatch = /^phase:\\s*['"]?([A-Za-z0-9_-]+)['"]?\\s*$/.exec(line); + if (phaseMatch) phase = phaseMatch[1]; + + if (/^gates:\\s*$/.test(line)) { inGates = true; continue; } + // Any column-0 key ends the gates block. + if (inGates && /^[^\\s]/.test(line)) inGates = false; + if (inGates && /^\\s+status:\\s*['"]?pending['"]?\\s*$/.test(line)) gatePending = true; + } + + return { phase, gatePending }; +} + +let raw = ''; +process.stdin.on('data', (c) => { raw += c; }); +process.stdin.on('end', () => { + try { + let input; + try { + input = JSON.parse(raw); + } catch { + return allow(); + } + + // Already nudged this cycle — let the model stop. + if (input && input.stop_hook_active === true) return allow(); + + const root = process.env.CODEV_WORKTREE_ROOT; + const projectId = process.env.CODEV_PROJECT_ID; + if (!root || !projectId) return allow(); + + const statusPath = findStatusPath(root, projectId); + if (!statusPath) return allow(); + + const { phase, gatePending } = readState(statusPath); + if (!phase) return allow(); + if (TERMINAL_PHASES.has(phase)) return allow(); + + // A gate awaiting a human is a legitimate stop, and so is "I could not + // tell" — gatePending is null only when the file could not be read. + if (gatePending !== false) return allow(); + + return block(phase); + } catch { + allow(); + } +}); +`; + +/** Single-quote-escape a value for safe embedding in a POSIX shell command. */ +function shellSingleQuote(value: string): string { + return value.replace(/'/g, `'\\''`); +} + +/** + * Build the shell command that runs the stop-guard for a builder worktree. + * + * The worktree root and project id are baked in rather than detected at run + * time: the hook runs in whatever cwd the session happens to be in, and a guard + * that guesses its own project is a guard that can nudge the wrong one. + */ +export function buildPhaseStopGuardCommand(worktreePath: string, projectId: string): string { + const root = isAbsolute(worktreePath) ? worktreePath : resolve(worktreePath); + const scriptAbs = join(root, STOP_GUARD_SCRIPT_RELPATH); + return ( + `CODEV_WORKTREE_ROOT='${shellSingleQuote(root)}' ` + + `CODEV_PROJECT_ID='${shellSingleQuote(projectId)}' ` + + `node '${shellSingleQuote(scriptAbs)}'` + ); +} + +/** + * Build the worktree files that install the phase stop-guard. + * + * Returns only the script. The `Stop` wiring is merged into the same + * `.claude/settings.local.json` the write-guard emits, because two writers of + * one file is how one of them silently loses. + */ +export function buildPhaseStopGuardFiles( + worktreePath: string, +): Array<{ relativePath: string; content: string }> { + void worktreePath; + return [{ relativePath: STOP_GUARD_SCRIPT_RELPATH, content: PHASE_STOP_GUARD_SCRIPT }]; +} diff --git a/packages/codev/src/agent-farm/utils/worktree-write-guard.ts b/packages/codev/src/agent-farm/utils/worktree-write-guard.ts index 526715082..d75c0b4b0 100644 --- a/packages/codev/src/agent-farm/utils/worktree-write-guard.ts +++ b/packages/codev/src/agent-farm/utils/worktree-write-guard.ts @@ -25,6 +25,15 @@ */ import { isAbsolute, join, resolve } from 'node:path'; +// Imported from the leaf module, NOT from porch/state.js: state.ts promisifies +// `execFile` at module load, and reaching it from here broke 24 doctor tests +// (its partial `node:child_process` mock has no `execFile`). See project-id.ts. +import { detectProjectIdFromCwd } from '../../commands/porch/project-id.js'; +import { + STOP_GUARD_SCRIPT_RELPATH, + PHASE_STOP_GUARD_SCRIPT, + buildPhaseStopGuardCommand, +} from './phase-stop-guard.js'; /** Worktree-relative path of the emitted guard script. */ export const GUARD_SCRIPT_RELPATH = '.claude/hooks/worktree-write-guard.cjs'; @@ -215,7 +224,27 @@ export function buildWorktreeGuardFiles( `CODEV_WORKTREE_ROOT='${shellSingleQuote(root)}' ` + `node '${shellSingleQuote(scriptAbs)}'`; - const settings = { + // Issue #41: the phase stop-guard rides in the same settings file. Two + // writers of one file is how one of them silently loses, so this function + // stays the single owner of `.claude/settings.local.json`. + // + // The project id comes from the worktree path via porch's OWN detector, not a + // second copy of the rule. A guard that disagrees with porch about which + // project it is watching would nudge about the wrong one. When the path is + // not a recognized builder worktree the id is null and the Stop hook is + // simply not installed — fail open, like every other path in these guards. + const projectId = detectProjectIdFromCwd(root); + + const files: Array<{ relativePath: string; content: string }> = [ + { relativePath: GUARD_SCRIPT_RELPATH, content: WORKTREE_WRITE_GUARD_SCRIPT }, + ]; + + const settings: { + hooks: { + PreToolUse: Array<{ matcher: string; hooks: Array<{ type: string; command: string }> }>; + Stop?: Array<{ hooks: Array<{ type: string; command: string }> }>; + }; + } = { hooks: { PreToolUse: [ { @@ -226,8 +255,19 @@ export function buildWorktreeGuardFiles( }, }; - return [ - { relativePath: GUARD_SCRIPT_RELPATH, content: WORKTREE_WRITE_GUARD_SCRIPT }, - { relativePath: GUARD_SETTINGS_RELPATH, content: JSON.stringify(settings, null, 2) + '\n' }, - ]; + if (projectId) { + settings.hooks.Stop = [ + { + hooks: [{ type: 'command', command: buildPhaseStopGuardCommand(root, projectId) }], + }, + ]; + files.push({ relativePath: STOP_GUARD_SCRIPT_RELPATH, content: PHASE_STOP_GUARD_SCRIPT }); + } + + files.push({ + relativePath: GUARD_SETTINGS_RELPATH, + content: JSON.stringify(settings, null, 2) + '\n', + }); + + return files; } diff --git a/packages/codev/src/commands/porch/project-id.ts b/packages/codev/src/commands/porch/project-id.ts new file mode 100644 index 000000000..343f83b19 --- /dev/null +++ b/packages/codev/src/commands/porch/project-id.ts @@ -0,0 +1,41 @@ +/** + * Deriving a porch project id from a builder worktree path. + * + * Split out of `state.ts` (issue #41) so that callers who need only this rule + * do not drag in the rest of that module. `state.ts` promisifies `execFile` at + * module load for `writeStateAndCommit`, which makes importing it from a leaf + * utility surprisingly expensive — and, in one case, breaking: `doctor.test.ts` + * partially mocks `node:child_process` without `execFile`, so any new import + * edge that reaches `state.ts` fails 24 of its tests with + * `No "execFile" export is defined on the "node:child_process" mock`. + * + * This module has no imports beyond `node:path` and must stay that way. + * `state.ts` re-exports from here, so existing importers are unaffected. + */ + +import * as path from 'node:path'; + +/** + * Derive the porch project id from a path inside a builder worktree. + * + * Returns null when the path is not a recognized builder worktree — callers + * treat that as "cannot tell", never as a default. + */ +export function detectProjectIdFromCwd(cwd: string): string | null { + const normalized = path.resolve(cwd).split(path.sep).join('/'); + // bugfix worktrees: .builders/bugfix-{N}-{slug} (slug optional) + // porch project ID is "bugfix-{N}" — historical convention, kept untouched. + // PIR / SPIR / ASPIR / AIR worktrees: .builders/{prefix}-{N}-{slug} (slug optional) + // porch project ID is the bare numeric ID. + // Spec worktrees (legacy): .builders/{NNNN} (bare 4-digit ID, no slug) + const match = normalized.match( + /\/\.builders\/(bugfix-(\d+)(?:-[^/]*)?|(?:aspir|spir|air|pir)-(\d+)(?:-[^/]*)?|(\d{4}))(\/|$)/, + ); + if (!match) return null; + // bugfix uses "bugfix-N" as the porch project ID + if (match[2]) return `bugfix-${match[2]}`; + // Protocol worktrees (aspir, spir, air, pir) use the bare numeric ID + if (match[3]) return match[3]; + // Spec worktrees use zero-padded numeric IDs + return match[4]; +} diff --git a/packages/codev/src/commands/porch/state.ts b/packages/codev/src/commands/porch/state.ts index 4408eef78..261d2d6f2 100644 --- a/packages/codev/src/commands/porch/state.ts +++ b/packages/codev/src/commands/porch/state.ts @@ -12,6 +12,7 @@ import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; import type { ProjectState, Protocol, PlanPhase } from './types.js'; import type { ArtifactResolver } from './artifacts.js'; +import { detectProjectIdFromCwd } from './project-id.js'; const execFileAsync = promisify(execFile); @@ -357,24 +358,14 @@ export function listAllProjects( * Works from any subdirectory within the worktree. * Returns the porch project ID (e.g. "bugfix-237", "1298", or "0073"), or null if not in a recognized worktree. */ -export function detectProjectIdFromCwd(cwd: string): string | null { - const normalized = path.resolve(cwd).split(path.sep).join('/'); - // bugfix worktrees: .builders/bugfix-{N}-{slug} (slug optional) - // porch project ID is "bugfix-{N}" — historical convention, kept untouched. - // PIR / SPIR / ASPIR / AIR worktrees: .builders/{prefix}-{N}-{slug} (slug optional) - // porch project ID is the bare numeric ID. - // Spec worktrees (legacy): .builders/{NNNN} (bare 4-digit ID, no slug) - const match = normalized.match( - /\/\.builders\/(bugfix-(\d+)(?:-[^/]*)?|(?:aspir|spir|air|pir)-(\d+)(?:-[^/]*)?|(\d{4}))(\/|$)/, - ); - if (!match) return null; - // bugfix uses "bugfix-N" as the porch project ID - if (match[2]) return `bugfix-${match[2]}`; - // Protocol worktrees (aspir, spir, air, pir) use the bare numeric ID - if (match[3]) return match[3]; - // Spec worktrees use zero-padded numeric IDs - return match[4]; -} +// Moved to ./project-id.ts (issue #41) and re-exported here so existing +// importers are unaffected. It lives in a leaf module now because importing +// THIS file pulls in `execFile` via `writeStateAndCommit`, which is more than a +// pure path-to-id rule should cost a caller. +// +// Imported as well as re-exported: `export { x } from` does NOT bind the name +// in this module's scope, and `resolveProjectId` below calls it directly. +export { detectProjectIdFromCwd }; export type ResolvedProjectId = { id: string; source: 'explicit' | 'cwd' | 'filesystem' }; From ea639e151b02eceab77bcb9e546af3c021a1821d Mon Sep 17 00:00:00 2001 From: pseudo Date: Sat, 22 Aug 2026 16:58:13 -0600 Subject: [PATCH 2/3] [Fix #41] The guard never fired. Fixture was a guess at porch's output. The review found this by running the emitted script against a real committed status.yaml instead of reading the diff, and it is total: the hook allowed every stop in production. createInitialState (state.ts:228-231) pre-seeds EVERY gate in the protocol as { status: 'pending' } at project creation, before any has been reached. The guard treated any `pending` gate as "a human is waiting", so from the moment a project exists there was always a pending gate and it always allowed. BUGFIX has exactly one gate, making this a no-op for that entire protocol. For SPIR, `pr` and `verify-approval` sit pending through the whole of `implement` -- which is where both reported incidents happened. The second incident, the one that motivated the issue, would not have been caught. A gate now counts as waiting only with BOTH `status: pending` AND `requested_at`. That is porch's own predicate, used verbatim at index.ts:383 and :1080 to decide "WAITING FOR HUMAN APPROVAL", and requested_at is set only by requestGate. Matching it rather than inventing a second definition of "waiting" is the point. The scanner closes each gate block so a `pending` on one gate cannot pair with a `requested_at` on another. Why 24 green tests missed it: MID_PHASE was hand-typed and carried a single approved gate, a shape porch never writes. The fixture is now produced by createInitialState run through porch's own YAML writer, and two tests copy REAL committed status.yaml files out of this repo -- the check the reviewer performed, now in the suite. Both would have failed before this commit. Removed buildPhaseStopGuardFiles, exported and called by nothing; the `void worktreePath;` on an unused parameter was the tell. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/__tests__/phase-stop-guard.test.ts | 120 +++++++++++++++--- .../src/agent-farm/utils/phase-stop-guard.ts | 61 ++++++--- 2 files changed, 143 insertions(+), 38 deletions(-) diff --git a/packages/codev/src/__tests__/phase-stop-guard.test.ts b/packages/codev/src/__tests__/phase-stop-guard.test.ts index 44edf0fb2..1de8a61ed 100644 --- a/packages/codev/src/__tests__/phase-stop-guard.test.ts +++ b/packages/codev/src/__tests__/phase-stop-guard.test.ts @@ -15,12 +15,14 @@ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as path from 'node:path'; +import * as yaml from 'js-yaml'; import { PHASE_STOP_GUARD_SCRIPT, STOP_GUARD_SCRIPT_RELPATH, TERMINAL_PHASES, buildPhaseStopGuardCommand, } from '../agent-farm/utils/phase-stop-guard.js'; +import { createInitialState } from '../commands/porch/state.js'; import { buildWorktreeGuardFiles, GUARD_SCRIPT_RELPATH, @@ -87,16 +89,33 @@ function runGuard( } } -const MID_PHASE = `id: '77' -title: test -protocol: pir -phase: implement -plan_phases: [] -gates: - plan-approval: - status: approved -build_complete: false -`; +/** + * A SPIR-shaped project mid-implement, produced by porch's OWN state factory + * and YAML writer. + * + * The previous fixture was hand-typed and carried a single approved gate -- + * a shape porch never writes. That is why 24 green tests covered a guard that + * was a total no-op in production: `createInitialState` pre-seeds EVERY gate + * as `{ status: 'pending' }` at creation, so the "is a gate pending?" check + * always said yes and the guard always allowed. + */ +const FIXTURE_PROTOCOL = { + name: 'fixture-spir', + version: '1.0.0', + description: 'f', + phases: [ + { id: 'implement', name: 'Implement', type: 'per_plan_phase' }, + { id: 'pr', name: 'PR', gate: 'pr' }, + { id: 'verify', name: 'Verify', gate: 'verify-approval' }, + ], +} as never; + +function porchState(overrides: Record = {}): string { + const state = createInitialState(FIXTURE_PROTOCOL, '77', 'test'); + return yaml.dump({ ...state, phase: 'implement', ...overrides }); +} + +const MID_PHASE = porchState(); const STOP = JSON.stringify({ stop_hook_active: false }); @@ -115,16 +134,43 @@ describe('phase stop-guard: paths that must ALLOW the stop', () => { expect(runGuard(STOP).blocked).toBe(false); }); - it('allows when ANY gate is pending', () => { + it('allows when a gate is genuinely awaiting a human', () => { // The one way this guard can do real harm: nudging a builder parked at a // human approval gate is pushing it past a decision only a human may make. - writeStatus( - '77', - 'test', - MID_PHASE.replace(' status: approved\n', ' status: approved\n dev-approval:\n status: pending\n'), - ); - const r = runGuard(STOP); - expect(r.blocked).toBe(false); + // "Genuinely" means BOTH keys — porch sets requested_at only in requestGate. + writeStatus('77', 'test', porchState({ + gates: { + pr: { status: 'pending', requested_at: '2026-08-22T00:00:00Z' }, + 'verify-approval': { status: 'pending' }, + }, + })); + + expect(runGuard(STOP).blocked).toBe(false); + }); + + it('BLOCKS on a gate that is seeded pending but never requested', () => { + // This is the case that made the guard a no-op. createInitialState seeds + // every gate as { status: 'pending' } at project creation, so treating bare + // `pending` as "a human is waiting" means a human is always waiting and the + // guard never fires. Verified against a real committed status.yaml before + // this test was written. + writeStatus('77', 'test', porchState()); + + expect(runGuard(STOP).blocked).toBe(true); + }); + + it('is not fooled by a requested_at on a DIFFERENT gate', () => { + // The two keys arrive on separate lines, so a scanner that does not close + // each gate block would pair `pending` from one gate with `requested_at` + // from another and allow every stop again. + writeStatus('77', 'test', porchState({ + gates: { + pr: { status: 'pending' }, + 'verify-approval': { status: 'approved', requested_at: '2026-08-22T00:00:00Z' }, + }, + })); + + expect(runGuard(STOP).blocked).toBe(true); }); it('allows when the status file cannot be read at all', () => { @@ -252,3 +298,41 @@ describe('worktree wiring', () => { expect(cmd).toContain('/x/main/.builders/pir-42/.claude/hooks/phase-stop-guard.cjs'); }); }); + +describe('against REAL committed status.yaml files', () => { + // The reviewer found the no-op by running the guard against a real file + // rather than a fixture. That check belongs in the suite, because a + // hand-typed fixture is a guess at porch's output and this is a sample of it. + const repoRoot = path.resolve(__dirname, '..', '..', '..', '..'); + + function runAgainstRealProject(projectDirName: string, projectId: string): GuardResult { + const src = path.join(repoRoot, 'codev', 'projects', projectDirName, 'status.yaml'); + if (!fs.existsSync(src)) return { status: 0, blocked: false, reason: 'FIXTURE-MISSING' }; + + const dest = path.join(worktree, 'codev', 'projects', projectDirName); + fs.mkdirSync(dest, { recursive: true }); + fs.copyFileSync(src, path.join(dest, 'status.yaml')); + + return runGuard(STOP, { CODEV_PROJECT_ID: projectId }); + } + + it('BLOCKS a real mid-phase project whose gates are only seeded', () => { + clearProjects(); + const r = runAgainstRealProject('bugfix-1137-gitea-forge-preset-is-broken-a', 'bugfix-1137'); + if (r.reason === 'FIXTURE-MISSING') return; + + // phase: fix, gates: { merge-approval: { status: pending } } and no + // requested_at. Under the original `pending`-only scan this allowed, which + // made the guard a no-op for the whole BUGFIX protocol. + expect(r.blocked).toBe(true); + }); + + it('ALLOWS a real completed project', () => { + clearProjects(); + const r = runAgainstRealProject('13-add-ci-concepts-to-the-forge-l', '13'); + if (r.reason === 'FIXTURE-MISSING') return; + + // phase: verified — terminal, nothing left to drive. + expect(r.blocked).toBe(false); + }); +}); diff --git a/packages/codev/src/agent-farm/utils/phase-stop-guard.ts b/packages/codev/src/agent-farm/utils/phase-stop-guard.ts index 46c2a5fcc..2a0b09230 100644 --- a/packages/codev/src/agent-farm/utils/phase-stop-guard.ts +++ b/packages/codev/src/agent-farm/utils/phase-stop-guard.ts @@ -148,11 +148,30 @@ function findStatusPath(root, projectId) { } /** - * Read \`phase\` and whether any gate is still \`pending\`. +/** + * Read \`phase\` and whether any gate is genuinely awaiting a human. + * + * A deliberately small line scanner rather than a YAML parser: this file has + * no dependencies available to it, and status.yaml's shape is fixed by porch's + * own writer. Anything it cannot understand returns nulls, which fail open. + * + * A GATE COUNTS AS WAITING ONLY WITH BOTH \`status: pending\` AND + * \`requested_at\`. Scanning for \`pending\` alone made this guard a total no-op + * in production: \`createInitialState\` (state.ts:228-231) pre-seeds EVERY gate + * in the protocol as \`{ status: 'pending' }\` at project creation, before any + * has been reached. So from the moment a project exists there is always a + * pending gate, and the guard always allowed. BUGFIX has exactly one gate, + * making it a no-op for that entire protocol; for SPIR, \`pr\` and + * \`verify-approval\` sit pending through the whole of \`implement\` -- which is + * where both reported incidents happened. * - * A deliberately small line scanner rather than a YAML parser: this file has no - * dependencies available to it, and status.yaml's shape is fixed by porch's own - * writer. Anything it cannot understand returns nulls, which fail open. + * 24 green tests did not notice, because the fixture was hand-typed rather + * than produced by porch. Verified against a real committed status.yaml. + * + * \`requested_at\` is set only by \`requestGate\` (index.ts:608-609), and porch + * itself uses exactly this pair to decide "WAITING FOR HUMAN APPROVAL" + * (index.ts:383 and :1080). Matching porch's own predicate is the point: a + * second definition of "waiting" is how the two drift apart. */ function readState(statusPath) { let text; @@ -166,15 +185,31 @@ function readState(statusPath) { let gatePending = false; let inGates = false; + // Per-gate accumulators: the two keys arrive on separate lines, so a gate is + // only judged once its block ends. + let curPending = false; + let curRequested = false; + const flush = () => { + if (curPending && curRequested) gatePending = true; + curPending = false; + curRequested = false; + }; + for (const line of text.split('\\n')) { const phaseMatch = /^phase:\\s*['"]?([A-Za-z0-9_-]+)['"]?\\s*$/.exec(line); if (phaseMatch) phase = phaseMatch[1]; if (/^gates:\\s*$/.test(line)) { inGates = true; continue; } // Any column-0 key ends the gates block. - if (inGates && /^[^\\s]/.test(line)) inGates = false; - if (inGates && /^\\s+status:\\s*['"]?pending['"]?\\s*$/.test(line)) gatePending = true; + if (inGates && /^\\S/.test(line)) { flush(); inGates = false; } + if (!inGates) continue; + + // A new gate name (two-space indent, bare key) closes the previous one. + if (/^ {2}[^\\s#][^:]*:\\s*$/.test(line)) { flush(); continue; } + if (/^\\s+status:\\s*['"]?pending['"]?\\s*$/.test(line)) curPending = true; + if (/^\\s+requested_at:\\s*\\S/.test(line)) curRequested = true; } + flush(); return { phase, gatePending }; } @@ -236,17 +271,3 @@ export function buildPhaseStopGuardCommand(worktreePath: string, projectId: stri `node '${shellSingleQuote(scriptAbs)}'` ); } - -/** - * Build the worktree files that install the phase stop-guard. - * - * Returns only the script. The `Stop` wiring is merged into the same - * `.claude/settings.local.json` the write-guard emits, because two writers of - * one file is how one of them silently loses. - */ -export function buildPhaseStopGuardFiles( - worktreePath: string, -): Array<{ relativePath: string; content: string }> { - void worktreePath; - return [{ relativePath: STOP_GUARD_SCRIPT_RELPATH, content: PHASE_STOP_GUARD_SCRIPT }]; -} From 1ec88d2f434072fcb465d3bd461c565f6d0c62f6 Mon Sep 17 00:00:00 2001 From: pseudo Date: Sat, 22 Aug 2026 22:48:31 -0600 Subject: [PATCH 3/3] [Fix #41] Name the plan phase in the nudge; review cleanups Round-2 review returned COMMENT (non-blocking) after confirming the guard now fires: it ran the emitted script against real committed status.yaml files and against porch's own gate predicate. Four of its six notes are worth taking. The nudge said phase "implement" while the builder was on plan phase 2 of 5 -- which is the exact incident this exists for. It now reads "implement / phase_2_seam_harness". The scanner was already reading the file. Two BLOCK tests were sitting under describe('paths that must ALLOW the stop') after an earlier commit inverted their semantics without moving them. Moved. The emitted artifact carried a doubled comment opener, landing in every builder worktree. The watchdog is split out to #56 rather than left to vanish. #41's entire "Proposed behaviour" section is a Tower idle watchdog, and `Closes #41` would auto-close it on merge. #56 keeps the three cases a Stop hook structurally cannot see: a dead process (no turn ends), a turn that never ends, and harnesses with no Stop-hook support -- the hook is emitted only for CLAUDE_HARNESS, so opencode and codex builders get nothing. Not taken, deliberately: the flow-style YAML note (unreachable from porch, which never sets flowLevel) and the "every human turn gets nudged" note -- that is the one-nudge design working as intended, now recorded in #56 so the next reader does not file it as a bug. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/__tests__/phase-stop-guard.test.ts | 70 ++++++++++++------- .../src/agent-farm/utils/phase-stop-guard.ts | 15 ++-- 2 files changed, 56 insertions(+), 29 deletions(-) diff --git a/packages/codev/src/__tests__/phase-stop-guard.test.ts b/packages/codev/src/__tests__/phase-stop-guard.test.ts index 1de8a61ed..264dbc5ae 100644 --- a/packages/codev/src/__tests__/phase-stop-guard.test.ts +++ b/packages/codev/src/__tests__/phase-stop-guard.test.ts @@ -148,31 +148,6 @@ describe('phase stop-guard: paths that must ALLOW the stop', () => { expect(runGuard(STOP).blocked).toBe(false); }); - it('BLOCKS on a gate that is seeded pending but never requested', () => { - // This is the case that made the guard a no-op. createInitialState seeds - // every gate as { status: 'pending' } at project creation, so treating bare - // `pending` as "a human is waiting" means a human is always waiting and the - // guard never fires. Verified against a real committed status.yaml before - // this test was written. - writeStatus('77', 'test', porchState()); - - expect(runGuard(STOP).blocked).toBe(true); - }); - - it('is not fooled by a requested_at on a DIFFERENT gate', () => { - // The two keys arrive on separate lines, so a scanner that does not close - // each gate block would pair `pending` from one gate with `requested_at` - // from another and allow every stop again. - writeStatus('77', 'test', porchState({ - gates: { - pr: { status: 'pending' }, - 'verify-approval': { status: 'approved', requested_at: '2026-08-22T00:00:00Z' }, - }, - })); - - expect(runGuard(STOP).blocked).toBe(true); - }); - it('allows when the status file cannot be read at all', () => { // "I could not tell" must not be spelled the same way as "no gate here". clearProjects(); @@ -214,6 +189,31 @@ describe('phase stop-guard: paths that must ALLOW the stop', () => { }); describe('phase stop-guard: the path that must BLOCK', () => { + it('BLOCKS on a gate that is seeded pending but never requested', () => { + // This is the case that made the guard a no-op. createInitialState seeds + // every gate as { status: 'pending' } at project creation, so treating bare + // `pending` as "a human is waiting" means a human is always waiting and the + // guard never fires. Verified against a real committed status.yaml before + // this test was written. + writeStatus('77', 'test', porchState()); + + expect(runGuard(STOP).blocked).toBe(true); + }); + + it('is not fooled by a requested_at on a DIFFERENT gate', () => { + // The two keys arrive on separate lines, so a scanner that does not close + // each gate block would pair `pending` from one gate with `requested_at` + // from another and allow every stop again. + writeStatus('77', 'test', porchState({ + gates: { + pr: { status: 'pending' }, + 'verify-approval': { status: 'approved', requested_at: '2026-08-22T00:00:00Z' }, + }, + })); + + expect(runGuard(STOP).blocked).toBe(true); + }); + it('blocks a mid-phase stop with no gate pending', () => { writeStatus('77', 'test', MID_PHASE); const r = runGuard(STOP); @@ -336,3 +336,23 @@ describe('against REAL committed status.yaml files', () => { expect(r.blocked).toBe(false); }); }); + +describe('the nudge names where the builder actually stopped', () => { + it('names the PLAN phase, not just the protocol phase', () => { + // Mid-implement in SPIR the protocol phase is "implement" for the whole + // build, while the builder is on plan phase 2 of 5 — which is the exact + // incident this guard exists for. "You stopped during implement" is not + // actionable; "implement / phase_2_seam_harness" is. + writeStatus('77', 'test', porchState({ current_plan_phase: 'phase_2_seam_harness' })); + + expect(runGuard(STOP).reason).toMatch(/implement \/ phase_2_seam_harness/); + }); + + it('falls back to the protocol phase when there is no plan phase', () => { + writeStatus('77', 'test', porchState({ current_plan_phase: null })); + + const reason = runGuard(STOP).reason; + expect(reason).toMatch(/phase "implement"/); + expect(reason).not.toMatch(/null/); + }); +}); diff --git a/packages/codev/src/agent-farm/utils/phase-stop-guard.ts b/packages/codev/src/agent-farm/utils/phase-stop-guard.ts index 2a0b09230..7268c9770 100644 --- a/packages/codev/src/agent-farm/utils/phase-stop-guard.ts +++ b/packages/codev/src/agent-farm/utils/phase-stop-guard.ts @@ -147,7 +147,6 @@ function findStatusPath(root, projectId) { return fs.existsSync(p) ? p : null; } -/** /** * Read \`phase\` and whether any gate is genuinely awaiting a human. * @@ -182,6 +181,7 @@ function readState(statusPath) { } let phase = null; + let planPhase = null; let gatePending = false; let inGates = false; @@ -198,6 +198,13 @@ function readState(statusPath) { for (const line of text.split('\\n')) { const phaseMatch = /^phase:\\s*['"]?([A-Za-z0-9_-]+)['"]?\\s*$/.exec(line); if (phaseMatch) phase = phaseMatch[1]; + // Mid-implement in SPIR the protocol phase is just "implement" while the + // builder is on plan phase 2 of 5 — which is the exact incident. Naming the + // plan phase makes the nudge about where it actually stopped. + const planMatch = /^current_plan_phase:\\s*['"]?([A-Za-z0-9_-]+)['"]?\\s*$/.exec(line); + // A null current_plan_phase is the common case, and the character class + // matches the literal word 'null', so exclude it explicitly. + if (planMatch && planMatch[1] !== 'null') planPhase = planMatch[1]; if (/^gates:\\s*$/.test(line)) { inGates = true; continue; } // Any column-0 key ends the gates block. @@ -211,7 +218,7 @@ function readState(statusPath) { } flush(); - return { phase, gatePending }; + return { phase, planPhase, gatePending }; } let raw = ''; @@ -235,7 +242,7 @@ process.stdin.on('end', () => { const statusPath = findStatusPath(root, projectId); if (!statusPath) return allow(); - const { phase, gatePending } = readState(statusPath); + const { phase, planPhase, gatePending } = readState(statusPath); if (!phase) return allow(); if (TERMINAL_PHASES.has(phase)) return allow(); @@ -243,7 +250,7 @@ process.stdin.on('end', () => { // tell" — gatePending is null only when the file could not be read. if (gatePending !== false) return allow(); - return block(phase); + return block(planPhase ? phase + ' / ' + planPhase : phase); } catch { allow(); }