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..264dbc5ae --- /dev/null +++ b/packages/codev/src/__tests__/phase-stop-guard.test.ts @@ -0,0 +1,358 @@ +/** + * 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 * 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, + 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 }; + } +} + +/** + * 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 }); + +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 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. + // "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('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 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); + 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'); + }); +}); + +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); + }); +}); + +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 new file mode 100644 index 000000000..7268c9770 --- /dev/null +++ b/packages/codev/src/agent-farm/utils/phase-stop-guard.ts @@ -0,0 +1,280 @@ +/** + * 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 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. + * + * 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; + try { + text = fs.readFileSync(statusPath, 'utf8'); + } catch { + return { phase: null, gatePending: null }; + } + + let phase = null; + let planPhase = null; + 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]; + // 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. + 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, planPhase, 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, planPhase, 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(planPhase ? phase + ' / ' + planPhase : 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)}'` + ); +} 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' };