diff --git a/codev-skeleton/roles/builder.md b/codev-skeleton/roles/builder.md index 293e01cfc..55bfe9b1d 100644 --- a/codev-skeleton/roles/builder.md +++ b/codev-skeleton/roles/builder.md @@ -15,6 +15,27 @@ hand-run consultations it would run, advance plan phases yourself, or skip the 3 Never hand-edit `status.yaml` — only porch commands modify project state. +## A phase handoff is not a stopping point + +When porch hands you a phase, **begin it in the same turn**. Receiving work is not a milestone, +and neither is finishing the previous phase. Do not end your turn to announce that you got the +phase, to summarize what you just did, or to ask whether to proceed with the thing you were +just told to do. + +Porch's `DO NOT start until you run porch again` is narrow: it forbids skipping *ahead* +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. + +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. +2. A **blocker you cannot resolve** — say what it is and what you tried, in the same message. +3. A **question whose answer changes the work** — ask it; don't ask permission to continue. + +"I finished a phase and thought I should check in" is none of these. An idle builder is +invisible: nobody is watching your pane, so a turn you end for courtesy can sit untouched for +hours. Reporting is what `afx send` is for, and it does not require ending your turn. + ## Gates Porch stops at human approval gates (`spec-approval`, `plan-approval`, `pr`). When it does: diff --git a/codev/roles/builder.md b/codev/roles/builder.md index 293e01cfc..55bfe9b1d 100644 --- a/codev/roles/builder.md +++ b/codev/roles/builder.md @@ -15,6 +15,27 @@ hand-run consultations it would run, advance plan phases yourself, or skip the 3 Never hand-edit `status.yaml` — only porch commands modify project state. +## A phase handoff is not a stopping point + +When porch hands you a phase, **begin it in the same turn**. Receiving work is not a milestone, +and neither is finishing the previous phase. Do not end your turn to announce that you got the +phase, to summarize what you just did, or to ask whether to proceed with the thing you were +just told to do. + +Porch's `DO NOT start until you run porch again` is narrow: it forbids skipping *ahead* +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. + +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. +2. A **blocker you cannot resolve** — say what it is and what you tried, in the same message. +3. A **question whose answer changes the work** — ask it; don't ask permission to continue. + +"I finished a phase and thought I should check in" is none of these. An idle builder is +invisible: nobody is watching your pane, so a turn you end for courtesy can sit untouched for +hours. Reporting is what `afx send` is for, and it does not require ending your turn. + ## Gates Porch stops at human approval gates (`spec-approval`, `plan-approval`, `pr`). When it does: diff --git a/packages/codev/src/commands/porch/__tests__/issue-40-phase-handoff-rules.test.ts b/packages/codev/src/commands/porch/__tests__/issue-40-phase-handoff-rules.test.ts new file mode 100644 index 000000000..378b5bdd1 --- /dev/null +++ b/packages/codev/src/commands/porch/__tests__/issue-40-phase-handoff-rules.test.ts @@ -0,0 +1,231 @@ +/** + * Issue #40 — the CRITICAL RULES box must tell a builder to start. + * + * ## The failure this pins + * + * A builder ended its turn at a plan-phase boundary with nothing blocking it + * and idled for two hours. Asked why, it said it read porch's + * `DO NOT start until you run porch again` as a general stop-and-wait, + * and treated the handoff as a reporting checkpoint. + * + * Both readings were available. The box porch marked CRITICAL contained one + * prohibition, one conditional ("when complete, run porch done"), and one + * imperative the builder could not perform — `/compact`, a slash command a + * human types into a composer. Nothing in it said "begin the phase you were + * just handed." + * + * These tests assert the box's *content*, not its formatting, because content + * is what the failure was about. The affirmative rule must come first: a + * builder that reads far enough to find rule 1 and stops there must have been + * told to work. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { tmpdir } from 'node:os'; +import * as yaml from 'js-yaml'; +import { status } from '../index.js'; +import type { ProjectState } from '../types.js'; + +const PROJECT = '9040-handoff'; + +describe('issue #40: phase-handoff CRITICAL RULES', () => { + let root: string; + let logged: string[]; + + /** + * A phased project sitting on `phase_1_a` with `phase_2_b` still pending — + * the exact shape that produces the handoff box. + */ + function writeProject(planPhases: ProjectState['plan_phases']): void { + const dir = path.join(root, 'codev/projects', PROJECT); + fs.mkdirSync(dir, { recursive: true }); + const state: ProjectState = { + id: '9040', + title: 'handoff', + protocol: 'fixture-spir', + phase: 'implement', + plan_phases: planPhases, + current_plan_phase: planPhases.find(p => p.status === 'in_progress')?.id ?? null, + gates: {}, + iteration: 1, + build_complete: false, + history: [], + started_at: 'T0', + updated_at: 'T0', + }; + fs.writeFileSync(path.join(dir, 'status.yaml'), yaml.dump(state)); + + const proto = path.join(root, 'codev/protocols/fixture-spir'); + fs.mkdirSync(proto, { recursive: true }); + fs.writeFileSync( + path.join(proto, 'protocol.json'), + JSON.stringify({ + name: 'fixture-spir', + version: '1.0.0', + description: 'f', + phases: [ + { + id: 'implement', + name: 'Implement', + type: 'per_plan_phase', + build: { prompt: 'i.md', artifact: 'src/**/*.ts' }, + }, + ], + }), + ); + } + + beforeEach(() => { + root = fs.mkdtempSync(path.join(tmpdir(), 'porch-handoff-40-')); + logged = []; + vi.spyOn(console, 'log').mockImplementation((...args: unknown[]) => { + logged.push(args.join(' ')); + }); + vi.spyOn(process.stdout, 'write').mockImplementation((chunk: unknown) => { + logged.push(String(chunk)); + return true; + }); + }); + + afterEach(() => { + vi.restoreAllMocks(); + fs.rmSync(root, { recursive: true, force: true }); + }); + + const out = (): string => logged.join('\n'); + + /** + * The box wraps, so a rule's text can straddle a line break. Assertions + * about what a rule SAYS have to see the rule, not the frame: strip ANSI, + * drop the borders, and collapse whitespace. + */ + const boxText = (): string => + // eslint-disable-next-line no-control-regex + out().replace(/\[[0-9;]*m/g, '') + .split('\n') + .filter(l => l.startsWith('║')) + .map(l => l.slice(1).replace(/║$/, '')) + .join(' ') + .replace(/\s+/g, ' '); + + const MID_BUILD: ProjectState['plan_phases'] = [ + { id: 'phase_1_a', title: 'A', status: 'in_progress' }, + { id: 'phase_2_b', title: 'B', status: 'pending' }, + ]; + + it('tells the builder to START the phase it was handed', async () => { + writeProject(MID_BUILD); + + await status(root, '9040'); + + expect(out()).toMatch(/START phase_1_a NOW/); + }); + + it('puts the affirmative rule FIRST, ahead of the prohibition', async () => { + // Order is the whole point. A box that opens with "DO NOT" has told a + // builder what not to do before it has told it to do anything, and the + // conservative reading of that is to stop. + writeProject(MID_BUILD); + + await status(root, '9040'); + + // Scoped to the box, not the whole status output: asserting on `out()` + // happens to work only because "DO NOT start" appears nowhere else, and + // that is a fact about today's output, not about this rule's order. + const text = boxText(); + expect(text.indexOf('START phase_1_a NOW')).toBeGreaterThan(-1); + expect(text.indexOf('START phase_1_a NOW')).toBeLessThan(text.indexOf('DO NOT start')); + }); + + it('says in the box that a handoff is not a stopping point', async () => { + writeProject(MID_BUILD); + + await status(root, '9040'); + + expect(boxText()).toMatch(/not a stopping point/i); + }); + + it('names the three things that DO justify stopping', async () => { + // "Should I stop here?" must have a written answer, or it gets answered + // by whichever reading looks safest. + writeProject(MID_BUILD); + + await status(root, '9040'); + + expect(boxText()).toMatch(/human gate/i); + expect(boxText()).toMatch(/blocker you cannot resolve/i); + expect(boxText()).toMatch(/question whose answer changes the work/i); + }); + + it('scopes the prohibition to the NAMED next phase', async () => { + writeProject(MID_BUILD); + + await status(root, '9040'); + + expect(boxText()).toMatch(/DO NOT start phase_2_b until you run porch again/); + }); + + it('still names the current phase on the LAST plan phase, where no next phase exists', async () => { + // The unqualified "DO NOT start the next phase" is the line the builder + // reported reading as stop-and-wait. It is unavoidable here — there is no + // next phase to name — so the affirmative rule has to carry the weight. + writeProject([ + { id: 'phase_1_a', title: 'A', status: 'complete' }, + { id: 'phase_2_b', title: 'B', status: 'in_progress' }, + ]); + + await status(root, '9040'); + + expect(boxText()).toMatch(/START phase_2_b NOW/); + expect(boxText()).toMatch(/DO NOT start the next phase until you run porch again/); + }); + + it('does not tell the builder to run /compact, which it cannot do', async () => { + // `/compact` is typed by a human into a composer. Nothing in the codebase + // consumes it, and a builder that treats it as a required step before + // starting has an unsatisfiable precondition and stops. + writeProject(MID_BUILD); + + await status(root, '9040'); + + expect(out()).not.toMatch(/\/compact/); + }); + + it('keeps the border intact when a phase id is wider than the box', async () => { + // Phase ids come from plan headings, so a long slug arrives as one + // unbreakable word. Without a hard break it runs through the frame. + const longId = 'phase_1_' + 'x'.repeat(120); + writeProject([ + { id: longId, title: 'A', status: 'in_progress' }, + { id: 'phase_2_b', title: 'B', status: 'pending' }, + ]); + + await status(root, '9040'); + + // eslint-disable-next-line no-control-regex + const plain = out().replace(/\[[0-9;]*m/g, ''); + const boxLines = plain.split('\n').filter(l => l.startsWith('║')); + expect(boxLines.length).toBeGreaterThan(4); + for (const line of boxLines) { + expect(line.endsWith('║')).toBe(true); + } + }); + + it('keeps the box legible: every line closes its border', async () => { + // The old box hand-padded each line, so any rule long enough to matter + // broke the frame. Wrapping is what makes a full sentence affordable. + writeProject(MID_BUILD); + + await status(root, '9040'); + + // eslint-disable-next-line no-control-regex + const plain = out().replace(/\[[0-9;]*m/g, ''); + const boxLines = plain.split('\n').filter(l => l.startsWith('║')); + expect(boxLines.length).toBeGreaterThan(4); + for (const line of boxLines) { + expect(line.endsWith('║')).toBe(true); + } + }); +}); diff --git a/packages/codev/src/commands/porch/index.ts b/packages/codev/src/commands/porch/index.ts index 56ef71f16..ded2600a0 100644 --- a/packages/codev/src/commands/porch/index.ts +++ b/packages/codev/src/commands/porch/index.ts @@ -66,6 +66,85 @@ function section(title: string, content: string): string { return `\n${chalk.bold(title)}:\n${content}`; } +/** Interior width of the CRITICAL RULES box, excluding the two `║` edges. */ +const RULES_BOX_WIDTH = 62; + +/** + * Render the CRITICAL RULES box. + * + * The rules read as a numbered list and wrap at the box width, so a rule can be + * a sentence instead of whatever fits in one hand-padded line. The old call + * sites padded each line themselves, which capped every rule at what fit and is + * why the box held only prohibitions. + * + * The first rule MUST be the affirmative one. A builder that reads a box whose + * every line is a "do not" has been told what not to do and nothing to do, and + * the safest-looking reading of that is to stop and ask. That misreading costs + * hours per occurrence, so the box now opens by naming the work to start. + */ +function criticalRulesBox(rules: string[]): string { + const edge = '═'.repeat(RULES_BOX_WIDTH); + const lines: string[] = [`╔${edge}╗`, `║ 🛑 CRITICAL RULES`.padEnd(RULES_BOX_WIDTH + 1) + '║']; + + rules.forEach((rule, i) => { + // 2 leading spaces + "N. " marker; continuation lines align under the text. + const marker = `${i + 1}. `; + const indent = ' '.repeat(2 + marker.length); + const avail = RULES_BOX_WIDTH - indent.length; + // Hard-break anything wider than the box before wrapping. Phase ids come + // from plan headings via `extractPlanPhases`, so a long slug is a single + // unbreakable word — and a word wider than `avail` would otherwise run + // straight through the border and break the frame it is rendered in. + const words = rule.split(/\s+/).flatMap(w => { + if (w.length <= avail) return [w]; + const parts: string[] = []; + for (let k = 0; k < w.length; k += avail) parts.push(w.slice(k, k + avail)); + return parts; + }); + const wrapped: string[] = []; + let cur = ''; + for (const w of words) { + if (cur && (cur + ' ' + w).length > avail) { + wrapped.push(cur); + cur = w; + } else { + cur = cur ? `${cur} ${w}` : w; + } + } + if (cur) wrapped.push(cur); + wrapped.forEach((text, j) => { + const prefix = j === 0 ? ` ${marker}` : indent; + lines.push(`║${(prefix + text).padEnd(RULES_BOX_WIDTH)}║`); + }); + }); + + lines.push(`╚${edge}╝`); + return lines.map(l => chalk.red.bold(l)).join('\n'); +} + +/** + * The rules shown when porch hands a builder a plan phase. + * + * `currentPhaseId` is the phase to begin NOW; `nextPhaseId` is the one to stay + * off until porch is run again. Keeping both named in the same box is the point + * — the prohibition used to appear alone, and "DO NOT start the next phase" + * with no next phase named reads as a general stop-and-wait. + */ +function phaseHandoffRules( + projectId: string, + currentPhaseId: string, + nextPhaseId: string | undefined, +): string { + return criticalRulesBox([ + `START ${currentPhaseId} NOW — a phase handoff is not a stopping point. Do not end your turn to report that you received it.`, + nextPhaseId + ? `DO NOT start ${nextPhaseId} until you run porch again!` + : 'DO NOT start the next phase until you run porch again!', + `When ${currentPhaseId} is complete, run: porch done ${projectId}`, + 'Stop only for a human gate, a blocker you cannot resolve, or a question whose answer changes the work.', + ]); +} + /** * Return a resolver scoped to `artifactRoot` when it differs from the caller's * cwd-rooted resolver. The incoming `resolver` is typically built from @@ -287,16 +366,7 @@ export async function status( const nextPlanPhase = state.plan_phases[currentIdx + 1]; console.log(''); - console.log(chalk.red.bold('╔══════════════════════════════════════════════════════════════╗')); - console.log(chalk.red.bold('║ 🛑 CRITICAL RULES ║')); - if (nextPlanPhase) { - console.log(chalk.red.bold(`║ 1. DO NOT start ${nextPlanPhase.id} until you run porch again!`.padEnd(63) + '║')); - } else { - console.log(chalk.red.bold('║ 1. DO NOT start the next phase until you run porch again! ║')); - } - console.log(chalk.red.bold('║ 2. Run /compact before starting each new phase ║')); - console.log(chalk.red.bold('║ 3. After completing this phase, run: porch done ' + state.id.padEnd(12) + '║')); - console.log(chalk.red.bold('╚══════════════════════════════════════════════════════════════╝')); + console.log(phaseHandoffRules(state.id, currentPlanPhase.id, nextPlanPhase?.id)); } } @@ -631,16 +701,7 @@ async function advanceProtocolPhase(workspaceRoot: string, state: ProjectState, } console.log(''); - console.log(chalk.red.bold('╔══════════════════════════════════════════════════════════════╗')); - console.log(chalk.red.bold('║ 🛑 CRITICAL RULES ║')); - if (nextPlanPhase) { - console.log(chalk.red.bold(`║ 1. DO NOT start ${nextPlanPhase.id} until you run porch again!`.padEnd(63) + '║')); - } else { - console.log(chalk.red.bold('║ 1. DO NOT start the next phase until you run porch again! ║')); - } - console.log(chalk.red.bold('║ 2. Run /compact before starting each new phase ║')); - console.log(chalk.red.bold('║ 3. When phase complete, run: porch done ' + state.id.padEnd(20) + '║')); - console.log(chalk.red.bold('╚══════════════════════════════════════════════════════════════╝')); + console.log(phaseHandoffRules(state.id, firstPhase.id, nextPlanPhase?.id)); } console.log(`\n Run: porch status ${state.id}`);