From 1f16ffe962cc34ace1450644b030e7a029fc373d Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Sun, 25 Oct 2026 09:42:53 +0100 Subject: [PATCH 01/15] chore(checks): the tool guard checks prompts, bodies, claims, labels, draft PRs, git rewrites and the slow lane --- .agents/scripts/body-rules.mjs | 153 ++++++++++ .agents/scripts/shell-words.mjs | 243 ++++++++++++++++ .agents/scripts/tool-guard.mjs | 489 +++++++++++++++++++++++++++++++- .claude/settings.json | 2 + src/tool-guard.test.ts | 412 +++++++++++++++++++++++---- 5 files changed, 1241 insertions(+), 58 deletions(-) create mode 100644 .agents/scripts/body-rules.mjs create mode 100644 .agents/scripts/shell-words.mjs diff --git a/.agents/scripts/body-rules.mjs b/.agents/scripts/body-rules.mjs new file mode 100644 index 000000000..9cf9f3838 --- /dev/null +++ b/.agents/scripts/body-rules.mjs @@ -0,0 +1,153 @@ +// `bodyDefects(text)`: what delivery rules 2, 8 and 12 say about a body or comment an agent +// posts, for the tool guard to refuse before it is posted. One sentence per defect, naming the +// rule; an empty list when the body passes. Words are counted outside code blocks, without +// headings and the footer line. +// +// every body the last line is `*Written by an agent.*` (rule 12) +// `## Handoff` Done, Not done, Findings, Blocked on, once each and in that order (rule +// 2); at most 150 words +// `## Reproduction` (a triage report, maybe after `Supersedes the report above.`) the +// headings Reproduction, Root cause, Simplest fix, Alternatives rejected, +// Risk and an optional Side findings, in order (rule 8); a `path:line` in +// Root cause; at most three Alternatives lines and three Risk bullets; at +// most 300 words +// `Spec updated:` one line and the footer +// `## Review notes` every note one bullet on one line +const FOOTER = /^[*_]Written by an agent\.[*_]$/; +const HANDOFF = ["Done", "Not done", "Findings", "Blocked on"]; +const TRIAGE = ["Reproduction", "Root cause", "Simplest fix", "Alternatives rejected", "Risk"]; + +export function bodyDefects(raw) { + const text = String(raw ?? "").replace(/\r/g, ""); + const defects = []; + const prose = withoutCode(text); + const lines = prose.split("\n"); + const filled = text + .split("\n") + .map((l) => l.trim()) + .filter((l) => l !== ""); + if (!FOOTER.test(filled.at(-1) ?? "")) { + defects.push( + "Every body and comment an agent writes ends with the line `*Written by an agent.*` (delivery rule 12): add it as the last line.", + ); + } + const first = filled[0] ?? ""; + const second = filled[1] ?? ""; + if (/^## Handoff\s*$/.test(first)) defects.push(...handoff(lines)); + else if ( + /^## Reproduction\s*$/.test(first) || + (/^Supersedes the report above\.?$/.test(first) && /^## Reproduction\s*$/.test(second)) + ) { + defects.push(...triage(lines)); + } else if (first.startsWith("Spec updated:")) { + if (filled.filter((l) => !FOOTER.test(l)).length > 1) { + defects.push( + 'A "Spec updated" comment is one line and the footer (delivery rule 12): say in that line which sections moved.', + ); + } + } else if (/^## Review notes\s*$/.test(first)) { + defects.push(...reviewNotes(lines)); + } + return defects; +} + +function handoff(lines) { + const found = []; + for (const l of lines) { + // why: a label is a heading, emphasised or followed by a colon; prose may start "Done when". + const label = + /^(?:- )?(#{2,6}\s+)?(\*\*|__|\*|_)?(Done|Not done|Findings|Blocked on)(?:\*\*|__|\*|_)?(:)?(?:\*\*|__)?(?=\s|$)/.exec( + l.trim(), + ); + if (label !== null && (label[1] || label[2] || label[4])) found.push(label[3]); + } + const out = []; + if (found.join("|") !== HANDOFF.join("|")) { + out.push( + `A \`## Handoff\` has four parts, once each and in this order: Done, Not done, Findings, Blocked on (delivery rule 2); this one has ${found.length === 0 ? "none" : found.join(", ")}.`, + ); + } + const words = countWords(lines); + if (words > 150) { + out.push( + `This handoff is ${words} words outside code; the budget is 150 (delivery rule 12): cut it.`, + ); + } + return out; +} + +function triage(lines) { + const out = []; + const headings = lines.flatMap((l) => { + const h = /^##\s+(.+?)\s*$/.exec(l); + return h === null ? [] : [h[1]]; + }); + const expected = headings.at(-1) === "Side findings" ? [...TRIAGE, "Side findings"] : TRIAGE; + if (headings.join("|") !== expected.join("|")) { + out.push( + "A triage report has the sections Reproduction, Root cause, Simplest fix, Alternatives rejected, Risk and, when there are any, Side findings, in that order (delivery rule 8).", + ); + } + const root = section(lines, "Root cause"); + if (root !== null && !/\S+\.\w+:\d+/.test(root.join("\n"))) { + out.push("The Root cause names the file and line, as `path:line` (delivery rule 8)."); + } + const alternatives = section(lines, "Alternatives rejected"); + if (alternatives !== null && alternatives.filter((l) => l.trim() !== "").length > 3) { + out.push( + "Alternatives rejected holds at most three lines, one per alternative (delivery rule 8).", + ); + } + const risk = section(lines, "Risk"); + if (risk !== null && risk.filter((l) => /^\s*[-*] /.test(l)).length > 3) { + out.push("Risk holds at most three bullets (delivery rule 8)."); + } + const words = countWords(lines); + if (words > 300) { + out.push( + `This report is ${words} words outside code; the budget is 300 (delivery rule 12): cut it.`, + ); + } + return out; +} + +function reviewNotes(lines) { + let inBullet = false; + for (const l of lines) { + if (l.trim() === "" || FOOTER.test(l.trim())) inBullet = false; + else if (/^\s*[-*] /.test(l)) inBullet = true; + else if (inBullet) { + return [ + "In `## Review notes` each note is one bullet on one line (delivery rule 12): join or cut the line that continues a bullet.", + ]; + } + } + return []; +} + +function section(lines, name) { + const at = lines.findIndex((l) => new RegExp(`^##\\s+${name}\\s*$`).test(l)); + if (at === -1) return null; + const end = lines.findIndex((l, i) => i > at && /^##\s/.test(l)); + return lines.slice(at + 1, end === -1 ? lines.length : end); +} + +function countWords(lines) { + return lines + .filter((l) => !/^#{1,6}\s/.test(l) && !FOOTER.test(l.trim())) + .reduce((sum, l) => sum + l.split(/\s+/).filter((w) => /\w/.test(w)).length, 0); +} + +function withoutCode(text) { + let inside = false; + return text + .split("\n") + .map((l) => { + if (/^\s*(```|~~~)/.test(l)) { + inside = !inside; + return ""; + } + return inside ? "" : l; + }) + .join("\n"); +} diff --git a/.agents/scripts/shell-words.mjs b/.agents/scripts/shell-words.mjs new file mode 100644 index 000000000..c31b2ba0a --- /dev/null +++ b/.agents/scripts/shell-words.mjs @@ -0,0 +1,243 @@ +// `parseShell(command)`: the simple commands a Bash tool call runs, for the tool guard's rules +// that need a command's real words (a quoted `--body`, a `--head` value) rather than the +// blanked text the older rules match. Each command is `{ words, dynamic, stdin, redirects }`: +// `words` are the unquoted words, `dynamic[i]` says word i holds a `$(…)`, a backtick or a +// `$VAR` the guard cannot read, `stdin` is a heredoc or here-string body (null when none), and +// `redirects` are `{ op, target }` pairs. The words of a `$(…)` or backtick substitution become +// commands of their own. A `$(cat <<'EOF' … EOF)` inside a word is read as its heredoc body. It +// never throws: input it cannot follow ends the command it is in. `commandWords(cmd)` is a +// command's words from the program name on, without leading `VAR=value` assignments, wrappers +// (`env`, `time`, ...) or a `timeout `. +export function parseShell(command) { + const commands = []; + parseInto(String(command ?? ""), commands); + return commands; +} + +const WRAPPERS = new Set(["time", "nice", "env", "command", "exec", "nohup"]); + +export function commandWords(cmd) { + let at = 0; + const { words } = cmd; + while (at < words.length && (/^\w+=/.test(words[at]) || WRAPPERS.has(words[at]))) at++; + if (words[at] === "timeout") at += 2; + return { words: words.slice(at), dynamic: cmd.dynamic.slice(at) }; +} + +function parseInto(src, commands) { + let i = 0; + let cmd = newCommand(); + let word = null; + let redirectOp = null; + const pending = []; + + const current = () => (word ??= { text: "", dynamic: false }); + const endWord = () => { + if (word === null) return; + if (redirectOp === "<<<") cmd.stdin = word.text; + else if (redirectOp !== null) cmd.redirects.push({ op: redirectOp, target: word.text }); + else { + cmd.words.push(word.text); + cmd.dynamic.push(word.dynamic); + } + redirectOp = null; + word = null; + }; + const endCommand = () => { + endWord(); + redirectOp = null; + if (cmd.words.length > 0 || cmd.redirects.length > 0 || cmd.stdin !== null) commands.push(cmd); + cmd = newCommand(); + }; + + while (i < src.length) { + const c = src[i]; + if (c === "\n") { + endCommand(); + i++; + // why: heredoc bodies start on the line after the operator, in the order they were opened. + for (const doc of pending.splice(0)) { + const lines = []; + while (i < src.length) { + const eol = src.indexOf("\n", i); + const line = src.slice(i, eol === -1 ? src.length : eol); + i = eol === -1 ? src.length : eol + 1; + if ((doc.strip ? line.replace(/^\t+/, "") : line).trim() === doc.delimiter) break; + lines.push(doc.strip ? line.replace(/^\t+/, "") : line); + } + doc.cmd.stdin = lines.join("\n"); + doc.cmd.stdinDynamic = !doc.quoted && /[$`]/.test(doc.cmd.stdin); + } + continue; + } + if (c === " " || c === "\t") { + endWord(); + i++; + continue; + } + if (c === "#" && word === null) { + while (i < src.length && src[i] !== "\n") i++; + continue; + } + if (c === "\\") { + if (src[i + 1] === "\n") i += 2; + else { + current().text += src[i + 1] ?? ""; + i += 2; + } + continue; + } + if (c === "'") { + const end = src.indexOf("'", i + 1); + current().text += src.slice(i + 1, end === -1 ? src.length : end); + i = end === -1 ? src.length : end + 1; + continue; + } + if (c === '"') { + i = readDoubleQuoted(src, i + 1, current(), commands); + continue; + } + if (c === "$") { + i = readDollar(src, i, current(), commands); + continue; + } + if (c === "`") { + i = readBacktick(src, i, current(), commands); + continue; + } + if (c === "<" && src.startsWith("<<<", i)) { + endWord(); + redirectOp = "<<<"; + i += 3; + continue; + } + if (c === "<" && src[i + 1] === "<") { + endWord(); + i += 2; + const strip = src[i] === "-"; + if (strip) i++; + while (src[i] === " " || src[i] === "\t") i++; + const quote = src[i] === "'" || src[i] === '"' ? src[i] : ""; + if (quote !== "") i++; + let delimiter = ""; + while (i < src.length && !/[\s;&|()<>'"]/.test(src[i])) delimiter += src[i++]; + if (quote !== "" && src[i] === quote) i++; + pending.push({ cmd, delimiter, strip, quoted: quote !== "" }); + continue; + } + if (c === ">" || c === "<" || (c === "&" && src[i + 1] === ">")) { + if (word !== null && /^\d+$/.test(word.text) && !word.dynamic) word = null; + else endWord(); + let op = c; + i++; + while (i < src.length && /[>&|]/.test(src[i]) && op.length < 3) op += src[i++]; + if (op.endsWith("&") && /[\d-]/.test(src[i] ?? "")) { + while (/[\d-]/.test(src[i] ?? "")) i++; + continue; + } + redirectOp = op; + continue; + } + if (c === ";" || c === "|" || c === "&" || c === "(" || c === ")") { + endCommand(); + i++; + continue; + } + current().text += c; + i++; + } + endCommand(); +} + +function newCommand() { + return { words: [], dynamic: [], stdin: null, stdinDynamic: false, redirects: [] }; +} + +function readDoubleQuoted(src, i, word, commands) { + while (i < src.length && src[i] !== '"') { + const c = src[i]; + if (c === "\\" && /[$`"\\\n]/.test(src[i + 1] ?? "")) { + if (src[i + 1] !== "\n") word.text += src[i + 1]; + i += 2; + } else if (c === "$") { + i = readDollar(src, i, word, commands); + } else if (c === "`") { + i = readBacktick(src, i, word, commands); + } else { + word.text += c; + i++; + } + } + return i + 1; +} + +// why: `"$(cat <<'EOF' … EOF)"` is how a model writes a long body inline; its text is plain. +const CAT_HEREDOC = /^\$\(\s*cat\s*<<-?\s*(['"]?)(\w+)\1[ \t]*\n([\s\S]*?)\n[ \t]*\2[ \t]*\n\s*\)/; + +function readDollar(src, i, word, commands) { + const next = src[i + 1] ?? ""; + if (next === "(") { + const doc = CAT_HEREDOC.exec(src.slice(i)); + if (doc !== null) { + word.text += doc[3]; + if (doc[1] === "" && /[$`]/.test(doc[3])) word.dynamic = true; + return i + doc[0].length; + } + const end = matchingParen(src, i + 2); + parseInto(src.slice(i + 2, end), commands); + word.dynamic = true; + return end + 1; + } + if (next === "'") { + let j = i + 2; + while (j < src.length && src[j] !== "'") { + if (src[j] === "\\" && j + 1 < src.length) { + word.text += { n: "\n", t: "\t", "\\": "\\", "'": "'" }[src[j + 1]] ?? src[j + 1]; + j += 2; + } else word.text += src[j++]; + } + return j + 1; + } + if (next === "{") { + const end = src.indexOf("}", i); + word.dynamic = true; + return end === -1 ? src.length : end + 1; + } + if (/[\w@*#?$!-]/.test(next)) { + let j = i + 1; + if (/\w/.test(next)) while (/\w/.test(src[j] ?? "")) j++; + else j++; + word.dynamic = true; + return j; + } + word.text += "$"; + return i + 1; +} + +function readBacktick(src, i, word, commands) { + const end = src.indexOf("`", i + 1); + parseInto(src.slice(i + 1, end === -1 ? src.length : end), commands); + word.dynamic = true; + return end === -1 ? src.length : end + 1; +} + +function matchingParen(src, from) { + let depth = 1; + let i = from; + while (i < src.length) { + const c = src[i]; + if (c === "'") { + const end = src.indexOf("'", i + 1); + i = end === -1 ? src.length : end + 1; + continue; + } + if (c === "\\") { + i += 2; + continue; + } + if (c === "(") depth++; + if (c === ")" && --depth === 0) return i; + i++; + } + return src.length; +} diff --git a/.agents/scripts/tool-guard.mjs b/.agents/scripts/tool-guard.mjs index d60464923..1f991b355 100644 --- a/.agents/scripts/tool-guard.mjs +++ b/.agents/scripts/tool-guard.mjs @@ -17,9 +17,37 @@ // job ends or hits its 30-minute cap. The slow lane is exempt: `slow-e2e.sh` runs // detached by design and its agents wait on `.exit` (delivery rule 16). // +// prompt a stage or lens agent (`implementer`, `reviewer`, `hardware-verifier`, +// `spec-reviewer`, `code-reviewer`, `claims-reviewer`) is started without the ledger +// tag as its prompt's first line (toolchain.md, "Review measurement"); an +// `implementer` without `brief:` and its mode's brief text, a `reviewer` without +// `protocol:` and REVIEW.md's text; a stage agent with a `model`; a `code-reviewer` +// off Opus on a diff that touches a strict area (REVIEW.md, "Routing"), outside a +// replay; or a sub-agent starts a helper that is not `run_in_background: false`. +// git a sub-agent runs `git stash`, `git rebase` (or `pull --rebase`), or a push that +// rewrites or deletes a remote branch (`+`, `:`, `--force`, `--delete`). +// gh a sub-agent opens a PR without `--draft` (or a REST create without +// `draft=true`), opens one from a `-repro` branch, runs `gh pr ready`, or adds +// `bug:ready`, `task:ready` or `feature:planned`; any session assigns an issue by +// hand instead of through `claim.mjs`. +// body any session posts a body or comment (`gh issue|pr comment|create|edit|close`, +// `gh api ... body=`) that `body-rules.mjs` rejects, or one the guard cannot read +// (built by `$(...)`, a backtick or a variable: use `--body-file`). +// slow lane any session runs vitest on an `e2e/slow-*` file, or the e2e project without a +// `!slow` tags filter; a sub-agent runs `pnpm test:e2e:slow` (or a physical lane); +// an agent other than `hardware-verifier` and `implementer` runs `slow-e2e.sh` +// for anything but `status` (delivery rule 16). +// // A refusal exits 2 with the reason on stderr, which the agent reads. +import { execFileSync } from "node:child_process"; import fs from "node:fs"; +import os from "node:os"; import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { bodyDefects } from "./body-rules.mjs"; +import { commandWords, parseShell } from "./shell-words.mjs"; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..", ".."); const SCRIPTS = new Set([ "check", @@ -41,6 +69,43 @@ const NODE_SCRIPTS = new Set(["comment-lint.mjs", "test-lint.mjs", "mutate.mjs"] const WRAPPERS = new Set(["time", "nice", "env", "command", "exec"]); const PNPM_VALUE_FLAGS = new Set(["-C", "--dir", "-F", "--filter", "--reporter"]); const STAGES = new Set(["implementer", "reviewer", "hardware-verifier"]); +const LENSES = new Set(["spec-reviewer", "code-reviewer", "claims-reviewer"]); +const LEDGER = + /^ledger: (?=.*\bpr=\S+)(?=.*\bstage=(?:build|audit|fix|hardware-fix|review|hardware|replay)\b)(?=.*\brole=\S+)/; +// why: toolchain.md "Project checks" lists these; src/contract/ holds the wire validators. +const STRICT_AREAS = [ + /^src\/leasing\//, + /^src\/contract\//, + /^src\/core\/cleanup\//, + /^src\/core\/(?:registry|serialized-decision|device-operation-claims|reclaim-coordinator|quarantine-coordinator|reaper|cleanup-executor|nuke|nuke-service|managed-device-lifecycle|device-root|physical-enrollment|startup-converger)\.ts$/, +]; +const WORK_LABELS = new Set(["bug:ready", "task:ready", "feature:planned"]); +const SLOW_LANE_AGENTS = new Set(["hardware-verifier", "implementer"]); +const SLOW_LANE_SCRIPTS = new Set([ + "test:e2e:slow", + "test:e2e:physical", + "test:e2e:physical-manual", +]); +const GIT_VALUE_FLAGS = new Set(["-C", "-c", "--git-dir", "--work-tree", "--namespace"]); +const GH_API_VALUE_FLAGS = new Set([ + "-X", + "--method", + "-H", + "--header", + "-f", + "-F", + "--field", + "--raw-field", + "--input", + "-q", + "--jq", + "-t", + "--template", + "--hostname", + "--cache", + "-p", + "--preview", +]); // why: the runners agents are allowed to run; `slow-e2e.sh` is excluded above, not here. const TEST_RUNNER = /\bvitest\s+(?:run|--)|\bplaywright\s+test\b|\bpnpm\s+(?:run\s+)?test(?::[\w-]+)?(?:\s|$)/; @@ -59,16 +124,12 @@ const HOOK_BYPASS = [ const input = JSON.parse(fs.readFileSync(0, "utf8") || "{}"); const tool = input.tool_name; const args = input.tool_input ?? {}; +const subAgent = typeof input.agent_id === "string"; +const cwd = typeof input.cwd === "string" ? input.cwd : process.cwd(); -if ((tool === "Agent" || tool === "Task") && args.isolation && STAGES.has(args.subagent_type)) { - refuse( - `Refused: pass no \`isolation\` to the ${args.subagent_type}. Every stage of a task works in ` + - "the task's one worktree (`.agents/scripts/worktree.sh `), one stage after " + - "another. Start the agent again without `isolation`.", - ); -} +if (tool === "Agent" || tool === "Task") agentRules(); -if (tool === "Bash" && typeof input.agent_id === "string") { +if (tool === "Bash" && subAgent) { const shell = code(args.command ?? ""); if (HOOK_BYPASS.some((re) => re.test(shell))) { refuse( @@ -117,8 +178,420 @@ if (tool === "Bash" && typeof input.agent_id === "string") { } } } +if (tool === "Bash") commandRules(args.command ?? ""); process.exit(0); +function agentRules() { + const type = args.subagent_type; + const prompt = String(args.prompt ?? ""); + if (args.isolation && STAGES.has(type)) { + refuse( + `Refused: pass no \`isolation\` to the ${type}. Every stage of a task works in ` + + "the task's one worktree (`.agents/scripts/worktree.sh `), one stage after " + + "another. Start the agent again without `isolation`.", + ); + } + if (STAGES.has(type) && typeof args.model === "string" && args.model !== "") { + refuse( + `Refused: pass no \`model\` to the ${type}: it runs on the model its frontmatter pins. ` + + "Start the agent again without `model`.", + ); + } + if (subAgent && args.run_in_background !== false) { + refuse( + "Refused: a helper agent runs in the foreground: pass `run_in_background: false`. A " + + "background helper reports to the orchestrator, not to you, and nothing wakes you when " + + "it ends.", + ); + } + if (!STAGES.has(type) && !LENSES.has(type)) return; + if (!LEDGER.test(prompt.replace(/^\s+/, "").split("\n")[0])) { + refuse( + "Refused: the prompt's first line is the ledger tag (toolchain.md, \"Review " + + 'measurement"): `ledger: pr= issue= stage= round= role= ' + + "lens=`, `-` for a key that does not apply yet. Start the agent again with it.", + ); + } + if (type === "implementer") briefCheck(prompt); + if (type === "reviewer") protocolCheck(prompt); + if (type === "code-reviewer") strictAreaCheck(prompt); +} + +function briefCheck(prompt) { + const mode = /^mode:\s*`?([\w-]+)/m.exec(prompt)?.[1]; + const file = path.join(ROOT, ".agents", "briefs", `implement-${mode}.md`); + if (mode === undefined || !fs.existsSync(file)) { + refuse( + "Refused: an implementer's prompt names its mode on a `mode:` line: build, audit, fix or " + + "hardware-fix. Start the agent again with it.", + ); + } + const at = prompt.search(/^brief:/m); + const want = squash(fs.readFileSync(file, "utf8")).slice(0, 200); + if (at === -1 || !squash(prompt.slice(at)).includes(want)) { + refuse( + `Refused: an implementer's prompt carries \`brief:\` and then the full text of ` + + `\`.agents/briefs/implement-${mode}.md\` from this checkout, so the agent opens no file ` + + "to learn its mode. Paste it and start the agent again.", + ); + } +} + +function protocolCheck(prompt) { + const heading = + fs + .readFileSync(path.join(ROOT, "docs", "internal", "REVIEW.md"), "utf8") + .split("\n") + .find((l) => l.startsWith("# ")) ?? ""; + const at = prompt.search(/^protocol:/m); + if ( + at === -1 || + !prompt + .slice(at) + .split("\n") + .some((l) => l.trim() === heading.trim()) + ) { + refuse( + "Refused: a reviewer's prompt carries `protocol:` and then the full text of " + + "`docs/internal/REVIEW.md` from this checkout. Paste it and start the agent again.", + ); + } +} + +function strictAreaCheck(prompt) { + const model = typeof args.model === "string" ? args.model : ""; + if (model === "" || /opus/i.test(model) || /\bstage=replay\b/.test(prompt)) return; + const dir = /^inputs:\s*[`'"]?([^`'"\s]+)/m.exec(prompt)?.[1]; + if (dir === undefined) return; + let patch; + try { + patch = fs.readFileSync(path.resolve(cwd, dir, "diff.patch"), "utf8"); + } catch { + return; + } + const hit = [...patch.matchAll(/^diff --git a\/(\S+) b\/(\S+)/gm)] + .flatMap((m) => [m[1], m[2]]) + .find((p) => STRICT_AREAS.some((re) => re.test(p))); + if (hit !== undefined) { + refuse( + `Refused: the diff touches a strict area (\`${hit}\`, toolchain.md "Project checks"), and ` + + 'its code review runs on Opus (REVIEW.md, "Routing"). Start the code-reviewer again ' + + "with no `model`, or `model: opus`.", + ); + } +} + +function commandRules(raw) { + let dir = cwd; + const written = new Map(); + for (const cmd of parseShell(raw)) { + const { words, dynamic } = commandWords(cmd); + if (words.length === 0) continue; + const name = path.basename(words[0]); + if (name === "cd" && words[1] !== undefined && !dynamic[1]) { + dir = path.resolve(dir, words[1].replace(/^~(?=\/|$)/, os.homedir())); + continue; + } + if (name === "cat" && cmd.stdin !== null) { + for (const r of cmd.redirects) { + if (r.op === ">" || r.op === ">>") written.set(path.resolve(dir, r.target), cmd.stdin); + } + } + if (name === "git" && subAgent) gitRules(words); + if (name === "gh") ghRules(words, dynamic, cmd, dir, written); + slowLaneRules(words); + } +} + +function gitRules(words) { + let at = 1; + while (at < words.length && words[at].startsWith("-")) + at += GIT_VALUE_FLAGS.has(words[at]) ? 2 : 1; + const sub = words[at]; + const rest = words.slice(at + 1); + if (sub === "stash") { + refuse( + "Refused: never `git stash` in a delivery stage: every worktree shares one stash list, so " + + "a pop can take another agent's changes. Commit the work instead.", + ); + } + if ( + sub === "rebase" || + (sub === "pull" && rest.some((w) => /^(?:-r|--rebase(?!=false))/.test(w))) + ) { + refuse( + "Refused: never rebase in a delivery stage: a stack moves by merging forward " + + "(`.agents/scripts/stack-sync.sh`), and a rebased branch needs a force push nobody may " + + "make (delivery rule 15). Merge the base branch instead: `git merge origin/`.", + ); + } + if ( + sub === "push" && + rest.some( + (w) => + w.startsWith("+") || + /^:./.test(w) || + /^--(?:force|delete|mirror)/.test(w) || + /^-[a-zA-Z]*[fd]/.test(w), + ) + ) { + refuse( + "Refused: this push rewrites or deletes a remote branch (`+`, `:`, `--force` or " + + "`--delete`). Push fast-forwards only; when the push is rejected, merge the remote " + + "branch in and push again.", + ); + } +} + +function ghRules(words, dynamic, cmd, dir, written) { + const [, group, verb] = words; + if (group === "issue" && verb === "edit" && flagValues(words, ["--add-assignee"]).length > 0) { + refuse( + "Refused: claim an issue with `node .agents/scripts/claim.mjs ` (`--triage` for a " + + "`bug:new` triage), not by hand. It assigns you only when the issue is open, unassigned " + + "and labelled as work an agent may take (delivery rules 1 and 2).", + ); + } + if (subAgent) { + const label = addedLabels(words, group, verb).find((l) => WORK_LABELS.has(l)); + if (label !== undefined) { + refuse( + `Refused: agents never label their way into work (delivery rule 1): \`${label}\` is the ` + + "maintainer's or the spec session's to set. Leave the label as it is and say in your " + + "report what should change.", + ); + } + if (group === "pr" && verb === "ready") { + refuse( + "Refused: only the orchestrator marks a PR ready, after its review (delivery rule 14). " + + "Leave it in draft and report.", + ); + } + if (group === "pr" && verb === "create") { + if (!words.includes("--draft") && !words.includes("-d")) { + refuse( + "Refused: every PR starts in draft (delivery rule 14). Run `gh pr create` again with " + + "`--draft`; the orchestrator marks it ready after review.", + ); + } + const head = flagValues(words, ["--head", "-H"]).at(-1) ?? currentBranch(dir); + if (head.endsWith("-repro")) { + refuse( + "Refused: a `bug/-repro` branch opens no pull request (delivery rule 8). Push the " + + "test to the branch and name it in the triage report.", + ); + } + } + if (group === "api" && /(?:^|\/)repos\/[^/]+\/[^/]+\/pulls$/.test(apiPath(words))) { + const fields = apiFields(words); + const post = + flagValues(words, ["-X", "--method"]).at(-1)?.toUpperCase() ?? + (fields.length > 0 ? "POST" : "GET"); + if (post === "POST" && !fields.some((f) => f.key === "draft" && f.value === "true")) { + refuse( + "Refused: every PR starts in draft (delivery rule 14). Create it again with " + + "`-F draft=true`; the orchestrator marks it ready after review.", + ); + } + } + } + const body = postedBody(words, dynamic, cmd, dir, written); + if (body === null) return; + const defects = bodyDefects(body); + if (defects.length > 0) { + const reason = defects.join(" "); + refuse(`Refused: ${reason[0].toLowerCase()}${reason.slice(1)} Fix the body and post again.`); + } +} + +// why: a body is null when the command posts none; an unreadable one refuses here. +function postedBody(words, dynamic, cmd, dir, written) { + const [, group, verb] = words; + const unreadable = () => + refuse( + "Refused: the guard cannot read a body built by `$(...)`, a backtick or a variable, so it " + + "cannot check its shape (delivery rule 12). Write the body to a file and pass " + + "`--body-file ` (`-F body=@` for `gh api`).", + ); + const fromFile = (file) => { + if (file === "-") { + if (cmd.stdin === null || cmd.stdinDynamic) unreadable(); + return cmd.stdin; + } + const at = path.resolve(dir, file.replace(/^~(?=\/|$)/, os.homedir())); + if (written.has(at)) return written.get(at); + try { + return fs.readFileSync(at, "utf8"); + } catch { + return refuse( + `Refused: \`${file}\` does not exist yet, so the guard cannot check the body. Write the ` + + "file first, then post it in its own command.", + ); + } + }; + if ( + (group === "issue" || group === "pr") && + ["comment", "create", "edit", "close"].includes(verb) + ) { + const inline = verb === "close" ? ["--comment", "-c"] : ["--body", "-b"]; + for (let i = 3; i < words.length; i++) { + const [flag, value, valueAt] = splitFlag(words, i); + if (inline.includes(flag)) { + if (dynamic[valueAt]) unreadable(); + return value ?? ""; + } + if (verb !== "close" && (flag === "--body-file" || flag === "-F")) { + if (dynamic[valueAt]) unreadable(); + return fromFile(value ?? ""); + } + } + return null; + } + if (group === "api") { + for (let i = 2; i < words.length; i++) { + const [flag, value, valueAt] = splitFlag(words, i); + if (flag === "--input" && value !== undefined) { + if (dynamic[valueAt]) unreadable(); + try { + const json = JSON.parse(fromFile(value)); + if (typeof json?.body === "string") return json.body; + } catch { + return null; + } + } + if ( + !["-f", "-F", "--field", "--raw-field"].includes(flag) || + !(value ?? "").startsWith("body=") + ) + continue; + if (dynamic[valueAt]) unreadable(); + const text = value.slice("body=".length); + return (flag === "-F" || flag === "--field") && text.startsWith("@") + ? fromFile(text.slice(1)) + : text; + } + } + return null; +} + +function slowLaneRules(words) { + const name = path.basename(words[0]); + const runner = words.findIndex((w) => path.basename(w) === "vitest"); + if ( + runner !== -1 && + (runner === 0 || + (["pnpm", "npx", "bunx", "yarn", "npm"].includes(name) && + words + .slice(1, runner) + .every((w) => ["exec", "dlx", "x", "run"].includes(w) || w.startsWith("-")))) + ) { + const after = words.slice(runner + 1); + const slowFile = after.some((w) => /(?:^|\/)e2e\/slow-[^/]*$/.test(w)); + const e2e = after.some( + (w, i) => w === "--project=e2e" || (w === "--project" && after[i + 1] === "e2e"), + ); + const filtered = flagValues(after, ["--tags-filter"], 0).some((v) => v.includes("!slow")); + if (slowFile || (e2e && !filtered)) { + refuse( + "Refused: the slow lane runs only through `scripts/slow-e2e.sh `, which holds " + + "the machine-wide lock (delivery rule 16). Run the fast e2e lane with `pnpm test:e2e " + + "`, and leave the slow lane to the hardware verifier.", + ); + } + } + if ( + subAgent && + name === "pnpm" && + SLOW_LANE_SCRIPTS.has(pnpmOperands(words.slice(1)).find((w) => w !== "run")) + ) { + refuse( + "Refused: the slow lane runs only through `scripts/slow-e2e.sh `, which holds " + + "the machine-wide lock (delivery rule 16). Run it through the script, or report the " + + "lines that need it under `hardware`.", + ); + } + const script = words.findIndex((w) => path.basename(w) === "slow-e2e.sh"); + if ( + script !== -1 && + (script === 0 || ["sh", "bash", "zsh"].includes(name)) && + words[script + 1] !== "status" && + typeof input.agent_type === "string" && + !SLOW_LANE_AGENTS.has(input.agent_type) + ) { + refuse( + "Refused: only the hardware verifier and the implementer run the slow lane (delivery rule " + + "16); `scripts/slow-e2e.sh status` is open to every agent. Report what needs the lane " + + "instead.", + ); + } +} + +function addedLabels(words, group, verb) { + let values = []; + if ((group === "issue" || group === "pr") && verb === "edit") + values = flagValues(words, ["--add-label"]); + if ((group === "issue" || group === "pr") && verb === "create") + values = flagValues(words, ["--label", "-l"]); + if (group === "api") { + values = apiFields(words) + .filter((f) => f.key === "labels" || f.key === "labels[]") + .map((f) => f.value); + } + return values.flatMap((v) => v.split(",")).map((v) => v.trim()); +} + +function apiPath(words) { + for (let i = 2; i < words.length; i++) { + if (GH_API_VALUE_FLAGS.has(words[i])) i++; + else if (!words[i].startsWith("-")) return words[i]; + } + return ""; +} + +function apiFields(words) { + const fields = []; + for (let i = 2; i < words.length; i++) { + const [flag, value] = splitFlag(words, i); + if (["-f", "-F", "--field", "--raw-field"].includes(flag) && value !== undefined) { + const eq = value.indexOf("="); + if (eq !== -1) fields.push({ key: value.slice(0, eq), value: value.slice(eq + 1) }); + } + } + return fields; +} + +// why: `--flag value` and `--flag=value` are the same flag; returns [flag, value, value index]. +function splitFlag(words, i) { + const w = words[i]; + const eq = w.indexOf("="); + if (w.startsWith("--") && eq !== -1) return [w.slice(0, eq), w.slice(eq + 1), i]; + return [w, words[i + 1], i + 1]; +} + +function flagValues(words, names, from = 1) { + const values = []; + for (let i = from; i < words.length; i++) { + const [flag, value] = splitFlag(words, i); + if (names.includes(flag) && value !== undefined) values.push(value); + } + return values; +} + +function currentBranch(dir) { + try { + return execFileSync("git", ["-C", dir, "branch", "--show-current"], { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + }).trim(); + } catch { + return ""; + } +} + +function squash(text) { + return text.replace(/\s+/g, " ").trim(); +} + function refuse(reason) { process.stderr.write(`${reason}\n`); process.exit(2); diff --git a/.claude/settings.json b/.claude/settings.json index 764ba573a..fd140c1ba 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -52,6 +52,8 @@ "Bash(git push -f*)", "Bash(git push * --force*)", "Bash(git push * -f*)", + "Bash(git push +*)", + "Bash(git push * +*)", "Bash(git reset --hard*)", "Bash(git branch -D *)", "Bash(gh pr merge*)", diff --git a/src/tool-guard.test.ts b/src/tool-guard.test.ts index 3ca0181fe..b2181f638 100644 --- a/src/tool-guard.test.ts +++ b/src/tool-guard.test.ts @@ -1,4 +1,6 @@ -import { spawnSync } from "node:child_process"; +import { execFileSync, spawnSync } from "node:child_process"; +import { mkdirSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; @@ -17,22 +19,36 @@ function call(input: object): { status: number | null; stderr: string } { return { status: result.status, stderr: result.stderr }; } +const ROOT = join(dirname(fileURLToPath(import.meta.url)), ".."); +const FOOTER = "*Written by an agent.*"; +const LEDGER = "ledger: pr=975 issue=966 stage=build round=- role=implementer lens=-"; + +// why: the timeout is set so each case meets the one rule it names, not the test-run timeout. function bash( command: string, agentId: string | null = "a1", agentType = "reviewer", - input: Record = {}, + extra: { cwd?: string; timeout?: number } = {}, ) { return call({ tool_name: "Bash", - tool_input: { command, ...input }, + tool_input: { command, timeout: extra.timeout ?? 600000 }, + ...(extra.cwd !== undefined && { cwd: extra.cwd }), ...(agentId && { agent_id: agentId, agent_type: agentType }), }); } -// why: a test run is allowed only with the Bash tool's timeout set (the default 120 s moves it to -// the background), so the "lets run" cases pass it the way an agent must. -const TIMEOUT = { timeout: 600_000 }; +function agent(toolInput: object, agentId: string | null = null) { + return call({ tool_name: "Agent", tool_input: toolInput, ...(agentId && { agent_id: agentId }) }); +} + +function bodyFile(body: string): string { + const file = join(mkdtempSync(join(tmpdir(), "tool-guard-")), "body.md"); + writeFileSync(file, body); + return file; +} + +const OK = { status: 0, stderr: "" }; describe("tool-guard.mjs", () => { it.each([ @@ -44,8 +60,6 @@ describe("tool-guard.mjs", () => { "npx -y tsc --noEmit", "node_modules/.bin/oxlint src", "node .agents/scripts/comment-lint.mjs --base origin/main", - "node .agents/scripts/test-lint.mjs --base origin/main", - "pnpm lint:tests", "pnpm test:e2e", "pnpm test:console", "sed -i '' 's/a/b/' f.ts && pnpm check 2>&1 | tail -5", @@ -71,51 +85,19 @@ describe("tool-guard.mjs", () => { "pnpm test:changed", "pnpm exec vitest run --project unit src/a.test.ts", "pnpm test:e2e e2e/http-api.test.ts", - "pnpm test:console --project=chromium e2e/console/shell.spec.ts", + "pnpm test:console --project=chromium e2e/console/leases.spec.ts", 'git commit -qm "run pnpm check in CI"', 'git commit -m "chore(fallow): remove four stale suppressions"', - 'gh pr create --title "chore(fallow): x" --body-file b.md', + 'gh pr edit 5 --title "chore(fallow): x"', "echo 'a; pnpm check'", "sed -i '' 's/oxfmt/oxlint/' docs/a.md", "grep -n tsc package.json", "python3 - <<'E'\nprint(\"pnpm check\")\nE\ngit status --short", "cat > x.sh < { - expect(bash(command, "a1", "reviewer", TIMEOUT)).toEqual({ status: 0, stderr: "" }); - }); - - it.each([ - ["pnpm exec vitest run --project unit src/a.test.ts", {}, "timeout: 600000"], - ["pnpm test:changed", { timeout: 120_000 }, "timeout: 600000"], - ["pnpm test:console --project=chromium", TIMEOUT, "Checks agents never run"], - ["ls", { run_in_background: true }, "background"], - ["(git push origin x > log 2>&1; echo done) &", {}, "background"], - ["until ! pgrep -f lefthook >/dev/null; do sleep 5; done; tail x", {}, "loop that sleeps"], - ["for _ in $(seq 18); do [ -f out ] && break; sleep 30; done", {}, "loop that sleeps"], - ["cat > /tmp/x 2>/dev/null; mkdir -p .agents/scratch", {}, "no input"], - ["cat > ../pr-911.md", {}, "no input"], - ])("refuses a sub-agent's wait or background job: %s", (command, input, reason) => { - const result = bash(command, "a1", "implementer", input); - - expect(result.status).toBe(2); - expect(result.stderr).toContain(reason); + expect(bash(command)).toEqual({ status: 0, stderr: "" }); }); - it.each([ - ["git add -A && git commit -m x", {}], - ["ls vitest* ui/vitest.config.ts", {}], - ["cat vitest.config.ts | head", {}], - ['for _ in $(seq 18); do [ -f "$log.exit" ] && break; sleep 30; done; cat "$log.exit"', {}], - ["scripts/slow-e2e.sh e2e/slow-ios.test.ts", {}], - ["cat > ../pr-911.md <<'EOF'\nCloses #1\nEOF\ngit status", {}], - ["python3 - <<'EOF'\nfor i in range(3):\n time.sleep(1)\nEOF", {}], - ])( - "lets a sub-agent run a foreground command, the slow lane wait included: %s", - (command, input) => { - expect(bash(command, "a1", "implementer", input)).toEqual({ status: 0, stderr: "" }); - }, - ); - it.each([ "git push --no-verify -u origin task/1", "git commit -q --no-verify -m x", @@ -154,14 +136,344 @@ describe("tool-guard.mjs", () => { ); it("starts a stage agent without isolation, and another agent with it", () => { + expect(agent({ subagent_type: "hardware-verifier", prompt: `${LEDGER}\npr: 975` })).toEqual(OK); + expect(agent({ subagent_type: "general-purpose", isolation: "worktree", prompt: "x" })).toEqual( + OK, + ); + }); + + it("refuses a sub-agent's test run without the long timeout", () => { + const result = bash("pnpm exec vitest run --project unit src/a.test.ts", "a1", "reviewer", { + timeout: 120000, + }); + + expect(result.status).toBe(2); + expect(result.stderr).toContain("timeout: 600000"); + }); + + it.each([ + ["git stash push -u", "never `git stash`"], + ["git stash", "never `git stash`"], + ["git -C ../x stash pop", "never `git stash`"], + ["git rebase origin/main", "never rebase"], + ["git pull --rebase origin task/1", "never rebase"], + ["git push origin +task/1", "rewrites or deletes a remote branch"], + ["git push origin +HEAD:task/1", "rewrites or deletes a remote branch"], + ["git push origin :task/1", "rewrites or deletes a remote branch"], + ["git push --delete origin task/1", "rewrites or deletes a remote branch"], + ["git -C ../x push --force-with-lease origin task/1", "rewrites or deletes a remote branch"], + ])("refuses a sub-agent's %s", (command, reason) => { + const result = bash(command, "a1", "implementer"); + + expect(result.status).toBe(2); + expect(result.stderr).toContain(reason); + }); + + it.each([ + "git push -u origin task/1", + "git push origin HEAD:task/1", + "git merge origin/main", + 'git commit -m "test: never git stash or git rebase (#1)"', + ])("lets a sub-agent run %s", (command) => { + expect(bash(command, "a1", "implementer")).toEqual(OK); + }); + + it("lets the main session stash and rebase", () => { + expect(bash("git stash push -u -m tag && git rebase origin/main", null)).toEqual(OK); + }); + + it("refuses a sub-agent's PR that is not a draft, or comes from a -repro branch, and its gh pr ready", () => { + const body = bodyFile(`Closes #1\n\n${FOOTER}\n`); + const repro = mkdtempSync(join(tmpdir(), "tool-guard-repro-")); + execFileSync("git", ["init", "-q", "-b", "bug/5-repro", repro]); + expect( - call({ tool_name: "Agent", tool_input: { subagent_type: "implementer", prompt: "x" } }), - ).toEqual({ status: 0, stderr: "" }); + bash(`gh pr create --title x --body-file ${body}`, "a1", "implementer").stderr, + ).toContain("every PR starts in draft"); expect( - call({ - tool_name: "Agent", - tool_input: { subagent_type: "general-purpose", isolation: "worktree", prompt: "x" }, - }), - ).toEqual({ status: 0, stderr: "" }); + bash("gh api -X POST repos/o/r/pulls -f title=x -f head=task/1 -f base=main", "a1").stderr, + ).toContain("every PR starts in draft"); + expect(bash("gh pr ready 975", "a1", "implementer").stderr).toContain( + "only the orchestrator marks a PR ready", + ); + expect( + bash(`gh pr create --draft --head bug/5-repro --body-file ${body}`, "a1", "bug-triager") + .stderr, + ).toContain("opens no pull request"); + expect( + bash(`gh pr create --draft --body-file ${body}`, "a1", "bug-triager", { cwd: repro }).stderr, + ).toContain("opens no pull request"); + + expect(bash(`gh pr create --draft --title x --body-file ${body}`, "a1", "implementer")).toEqual( + OK, + ); + expect( + bash("gh api -X POST repos/o/r/pulls -f title=x -f head=task/1 -F draft=true", "a1"), + ).toEqual(OK); + expect(bash("gh pr ready 975", null)).toEqual(OK); + }); + + it.each([ + "gh issue edit 9 --add-label bug:ready --remove-label bug:new", + "gh issue edit 9 --add-label=task:ready", + "gh issue edit 9 --add-label x,feature:planned", + "gh issue create --title x --label task:ready --body-file b.md", + "gh api -X POST repos/o/r/issues/9/labels -f 'labels[]=bug:ready'", + ])("refuses a sub-agent's label into work: %s", (command) => { + const result = bash(command, "a1", "bug-triager"); + + expect(result.status).toBe(2); + expect(result.stderr).toContain("never label their way into work"); + }); + + it("lets a sub-agent move a bug to needs-info, and the main session set a ready label", () => { + expect( + bash( + "gh issue edit 9 --add-label bug:needs-info --remove-label bug:new", + "a1", + "bug-triager", + ), + ).toEqual(OK); + expect(bash("gh issue edit 9 --add-label task:ready --remove-label task:draft", null)).toEqual( + OK, + ); + }); + + it.each([null, "a1"])("refuses a claim by hand from any session (agent %s)", (agentId) => { + const result = bash("gh issue edit 9 --add-assignee @me", agentId); + + expect(result.status).toBe(2); + expect(result.stderr).toContain("node .agents/scripts/claim.mjs "); + expect(bash("gh issue edit 9 --remove-assignee @me", agentId)).toEqual(OK); + }); + + it.each([ + ["pnpm exec vitest run --project e2e e2e/slow-ios.test.ts", null], + ["pnpm exec vitest run --project e2e --tags-filter='!slow' e2e/slow-ios.test.ts", "a1"], + ["npx vitest run --project=e2e e2e/http-api.test.ts", "a1"], + ["node_modules/.bin/vitest run --project e2e", null], + ])("refuses a slow-lane test outside its script: %s", (command, agentId) => { + const result = bash(command, agentId); + + expect(result.status).toBe(2); + expect(result.stderr).toContain("runs only through `scripts/slow-e2e.sh"); + }); + + it("refuses the slow lane's pnpm scripts in a sub-agent, and slow-e2e.sh outside its two agents", () => { + expect(bash("pnpm test:e2e:slow e2e/slow-ios.test.ts", "a1", "implementer").stderr).toContain( + "runs only through `scripts/slow-e2e.sh", + ); + expect(bash("scripts/slow-e2e.sh e2e/slow-ios.test.ts", "a1", "bug-triager").stderr).toContain( + "only the hardware verifier and the implementer run the slow lane", + ); + expect(bash("sh scripts/slow-e2e.sh clean", "a1", "code-reviewer").status).toBe(2); + }); + + it.each([ + [ + "pnpm exec vitest run --project e2e --tags-filter='!slow' e2e/http-api.test.ts", + "a1", + "reviewer", + ], + ["scripts/slow-e2e.sh status", "a1", "bug-triager"], + ["scripts/slow-e2e.sh e2e/slow-ios.test.ts", "a1", "hardware-verifier"], + ["scripts/slow-e2e.sh --physical e2e/slow-ios-physical.test.ts", "a1", "implementer"], + ["scripts/slow-e2e.sh e2e/slow-ios.test.ts", null, "reviewer"], + ["ls e2e/slow-ios.test.ts vitest.config.ts", "a1", "reviewer"], + ])("lets %s run (agent %s, %s)", (command, agentId, agentType) => { + expect(bash(command, agentId, agentType)).toEqual(OK); + }); + + it("refuses a sub-agent's helper agent unless it runs in the foreground", () => { + for (const background of [undefined, true]) { + const result = agent( + { subagent_type: "Explore", prompt: "x", run_in_background: background }, + "a1", + ); + expect(result.status).toBe(2); + expect(result.stderr).toContain("pass `run_in_background: false`"); + } + expect( + agent({ subagent_type: "Explore", prompt: "x", run_in_background: false }, "a1"), + ).toEqual(OK); + expect(agent({ subagent_type: "Explore", prompt: "x" })).toEqual(OK); + }); + + it.each(["implementer", "reviewer", "hardware-verifier"])( + "refuses a model on the %s from any session", + (type) => { + const result = agent({ subagent_type: type, model: "opus", prompt: LEDGER }); + + expect(result.status).toBe(2); + expect(result.stderr).toContain(`pass no \`model\` to the ${type}`); + }, + ); + + it.each([ + "implementer", + "reviewer", + "hardware-verifier", + "spec-reviewer", + "code-reviewer", + "claims-reviewer", + ])("refuses to start a %s whose prompt does not begin with the ledger tag", (type) => { + const result = agent( + { subagent_type: type, prompt: "issue: 966\nmode: build", run_in_background: false }, + type.endsWith("-reviewer") ? "a1" : null, + ); + + expect(result.status).toBe(2); + expect(result.stderr).toContain("first line is the ledger tag"); + }); + + it("requires an implementer's brief and a reviewer's protocol in the prompt, from this checkout", () => { + const build = readFileSync(join(ROOT, ".agents", "briefs", "implement-build.md"), "utf8"); + const fix = readFileSync(join(ROOT, ".agents", "briefs", "implement-fix.md"), "utf8"); + const protocol = readFileSync(join(ROOT, "docs", "internal", "REVIEW.md"), "utf8"); + const implementer = (prompt: string) => agent({ subagent_type: "implementer", prompt }); + const reviewer = (prompt: string) => agent({ subagent_type: "reviewer", prompt }); + + expect(implementer(`${LEDGER}\nissue: 966\nmode: build\nbrief:\n${build}`)).toEqual(OK); + expect(implementer(`${LEDGER}\nissue: 966\nmode: build`).stderr).toContain( + "`.agents/briefs/implement-build.md`", + ); + expect(implementer(`${LEDGER}\nissue: 966\nmode: build\nbrief:\n${fix}`).status).toBe(2); + expect(implementer(`${LEDGER}\nissue: 966\nbrief:\n${build}`).stderr).toContain("`mode:` line"); + expect(reviewer(`${LEDGER}\npr: 975\nprotocol:\n${protocol}`)).toEqual(OK); + expect(reviewer(`${LEDGER}\npr: 975`).stderr).toContain("carries `protocol:`"); + }); + + it("refuses a code review off Opus on a diff that touches a strict area, outside a replay", () => { + const inputs = mkdtempSync(join(tmpdir(), "tool-guard-inputs-")); + const plain = join(inputs, "plain"); + const strict = join(inputs, "strict"); + for (const [dir, path] of [ + [plain, "src/cli/main.ts"], + [strict, "src/leasing/wait-queue.ts"], + ] as const) { + mkdirSync(dir); + writeFileSync(join(dir, "diff.patch"), `diff --git a/${path} b/${path}\n+x\n`); + } + const lens = (dir: string, model?: string, stage = "review") => + agent( + { + subagent_type: "code-reviewer", + ...(model !== undefined && { model }), + run_in_background: false, + prompt: `ledger: pr=975 issue=966 stage=${stage} round=1 role=code lens=both\ninputs: ${dir}`, + }, + "a1", + ); + + expect(lens(strict, "sonnet").stderr).toContain("`src/leasing/wait-queue.ts`"); + expect(lens(strict, "opus")).toEqual(OK); + expect(lens(strict)).toEqual(OK); + expect(lens(strict, "sonnet", "replay")).toEqual(OK); + expect(lens(plain, "sonnet")).toEqual(OK); + }); + + const handoff = (blocked = "nothing") => + `## Handoff\n\n**Done:** two cycles, green.\n**Not done:** the rest.\n**Findings:** none.\n**Blocked on:** ${blocked}\n\n${FOOTER}\n`; + const triage = [ + "## Reproduction", + "`src/a.test.ts` fails.", + "## Root cause", + "Defect: `src/a.ts:12` drops the lease.", + "## Simplest fix", + "Keep it.", + "## Alternatives rejected", + "- a — no", + "## Risk", + "- b", + "", + FOOTER, + ].join("\n"); + + it.each([ + ["no footer", "Done.", "ends with the line `*Written by an agent.*`"], + ["a handoff part missing", handoff().replace("**Findings:** none.\n", ""), "has four parts"], + [ + "handoff parts out of order", + handoff().replace( + "**Done:** two cycles, green.\n**Not done:** the rest.", + "**Not done:** the rest.\n**Done:** x", + ), + "has four parts", + ], + [ + "a handoff over 150 words", + handoff().replace("the rest.", "word ".repeat(150)), + "the budget is 150", + ], + [ + "a triage report without Risk", + triage.replace("## Risk\n- b\n", ""), + "a triage report has the sections", + ], + [ + "a root cause without path:line", + triage.replace("`src/a.ts:12`", "the queue"), + "names the file and line", + ], + ["four alternatives", triage.replace("- a — no", "- a\n- b\n- c\n- d"), "at most three lines"], + [ + "a triage report over 300 words", + triage.replace("Keep it.", "word ".repeat(300)), + "the budget is 300", + ], + [ + "a two-line Spec updated", + `Spec updated: Scope.\nAnd Tests.\n\n${FOOTER}`, + "one line and the footer", + ], + [ + "a review note on two lines", + `## Review notes\n\nNot blocking.\n\n- spec: src/a.ts:1 x\n and more\n\n${FOOTER}`, + "one bullet on one line", + ], + ])("refuses a body with %s", (_, body, reason) => { + const result = bash(`gh issue comment 9 --body-file ${bodyFile(body)}`, null); + + expect(result.status).toBe(2); + expect(result.stderr).toContain(reason); + }); + + it.each([ + ["a handoff", handoff("the maintainer's answer on the timeout")], + ["a triage report", triage], + ["a superseding triage report", `Supersedes the report above.\n\n${triage}`], + ["a Spec updated line", `Spec updated: Scope and Tests.\n\n${FOOTER}`], + [ + "review notes", + `## Review notes\n\nNot blocking.\n\n- spec: src/a.ts:1 x\n- code: y\n\n_Written by an agent._`, + ], + ])("posts %s in its shape", (_, body) => { + expect(bash(`gh pr comment 9 --body-file ${bodyFile(body)}`, null)).toEqual(OK); + }); + + it("reads the body from every place gh takes it, and refuses one it cannot read", () => { + const good = `Done.\n\n${FOOTER}`; + const file = bodyFile(good); + const scratch = join(mkdtempSync(join(tmpdir(), "tool-guard-")), "new.md"); + + expect(bash(`gh issue comment 9 --body "${good}"`, null)).toEqual(OK); + expect(bash(`gh issue comment 9 --body "$(cat <<'EOF'\n${good}\nEOF\n)"`, null)).toEqual(OK); + expect(bash(`gh issue comment 9 --body-file - <<'EOF'\n${good}\nEOF`, null)).toEqual(OK); + expect( + bash(`cat > ${scratch} <<'EOF'\n${good}\nEOF\ngh pr edit 9 --body-file ${scratch}`, null), + ).toEqual(OK); + expect(bash(`gh api -X PATCH repos/o/r/pulls/9 -F body=@${file}`, null)).toEqual(OK); + expect(bash(`gh issue close 9 --comment "${good}"`, null)).toEqual(OK); + expect(bash("gh api -X PATCH repos/o/r/pulls/9 -f body=Done.", null).status).toBe(2); + expect(bash("gh issue edit 9 --body-file /no/such/file.md", null).stderr).toContain( + "does not exist yet", + ); + for (const command of [ + 'gh issue comment 9 --body "$(git log -1)"', + 'gh issue comment 9 --body "$BODY"', + "gh issue comment 9 --body `cat x`", + ]) { + expect(bash(command, null).stderr).toContain("pass `--body-file `"); + } }); }); From abe0de154d83919c74ac9094e48a71546dd455f8 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Sun, 25 Oct 2026 09:44:01 +0100 Subject: [PATCH 02/15] chore(checks): the report guard checks remaining, open, Rerun, lens counts and the findings file --- .agents/scripts/report-guard.mjs | 90 +++++++++++++++++++++++++++++++- src/report-guard.test.ts | 80 +++++++++++++++++++++++++--- 2 files changed, 162 insertions(+), 8 deletions(-) diff --git a/.agents/scripts/report-guard.mjs b/.agents/scripts/report-guard.mjs index e1186f437..e5cc29843 100644 --- a/.agents/scripts/report-guard.mjs +++ b/.agents/scripts/report-guard.mjs @@ -9,9 +9,15 @@ // An implementer's `mode: audit` report must also hold its `audit:` line: an audit given fixes // as well once did the fixes and skipped the audit. That line's `probed

/` must show every // test it added or changed probed: audits that skipped the probe left half of all review -// findings to the reviewers. Other agents stop freely. Claude Code ends the turn anyway after -// eight blocks in a row. +// findings to the reviewers. Unless its Assessment is incomplete, an implementer's `cycles +// /` with b < t lists the t - b Tests lines not built under `remaining`, or an `open` line +// `spec needs: ...`; an audit's `donewhen /` or `covered /` short of its total has an +// `open` line per missing one. A reviewer's `Rerun:` is `round 2`, `fix-check` or `none`; its +// Spec, Code and Claims review counts are each ` findings ( blocking)` or `skipped`; and +// its `Findings file:` exists (relative paths from the agent's cwd). Other agents stop freely. +// Claude Code ends the turn anyway after eight blocks in a row. import fs from "node:fs"; +import path from "node:path"; const REPORT = { implementer: /^issue: \d+/m, @@ -23,6 +29,7 @@ const input = JSON.parse(fs.readFileSync(0, "utf8") || "{}"); const shape = REPORT[input.agent_type]; const message = input.last_assistant_message ?? ""; if (shape === undefined) process.exit(0); +let blocked = false; if (!shape.test(message)) { block( @@ -37,6 +44,8 @@ if (!shape.test(message)) { block( "Report Assessment: complete or incomplete with missing scope; budget exhaustion is not a pass.", ); +} else if (input.agent_type === "reviewer" && reviewerDefect() !== null) { + block(reviewerDefect()); } else if (/\bmode: audit\b/.test(message) && !/^Assessment: incomplete\b/m.test(message)) { const audit = /^audit: .*$/m.exec(message); const probed = audit === null ? null : /\bprobed (\d+)\/(\d+)/.exec(audit[0]); @@ -58,6 +67,83 @@ if (!shape.test(message)) { } } +if (!blocked && input.agent_type === "implementer" && !/^Assessment: incomplete\b/m.test(message)) { + const defect = implementerDefect(); + if (defect !== null) block(defect); +} + +function implementerDefect() { + const open = items("open"); + const cycles = /\bcycles: (\d+)\/(\d+)/.exec(message); + if (cycles !== null && Number(cycles[1]) < Number(cycles[2])) { + const missing = Number(cycles[2]) - Number(cycles[1]); + if (items("remaining").length < missing && !open.some((l) => l.startsWith("spec needs:"))) { + return ( + `Your report says \`cycles ${cycles[1]}/${cycles[2]}\`: list the ${missing} Tests lines not ` + + "built under `remaining`, in build order, or name what stops them under `open` as " + + "`spec needs: `." + ); + } + } + const audit = /^audit: .*$/m.exec(message)?.[0] ?? ""; + for (const [name, what] of [ + ["donewhen", "Done when line"], + ["covered", "changed function no test covers"], + ]) { + const count = new RegExp(`\\b${name} (\\d+)/(\\d+)`).exec(audit); + if (count !== null && open.length < Number(count[2]) - Number(count[1])) { + return ( + `Your \`audit:\` line says \`${name} ${count[1]}/${count[2]}\`: give each ${what} left ` + + "under `open`, with why, or finish it and count it." + ); + } + } + return null; +} + +function reviewerDefect() { + if (!/^Rerun: (?:round 2|fix-check|none)\s*$/m.test(message)) { + return "Your report's `Rerun:` line is `round 2`, `fix-check` or `none`. Report again with it."; + } + for (const lens of ["Spec", "Code", "Claims"]) { + const value = new RegExp( + `\\b${lens} review: (.+?)(?=\\s+(?:Spec|Code|Claims) review:|\\s*$)`, + "m", + ).exec(message)?.[1]; + if (value === undefined || !/^(?:\d+ findings? \(\d+ blocking\)|skipped)$/.test(value)) { + return ( + `Your report's \`${lens} review:\` is \` findings ( blocking)\` or \`skipped\`. ` + + "Report again with the counts." + ); + } + } + const file = /^Findings file: *(.*?)\s*$/m.exec(message)?.[1] ?? ""; + const complete = /^Assessment: complete\b/m.test(message); + const exists = file !== "" && fs.existsSync(path.resolve(input.cwd ?? ".", file)); + if ((complete || file !== "") && !exists) { + return ( + "Your report's `Findings file:` names no file on disk. Write the findings file REVIEW.md " + + '"Measurement" describes and report its path.' + ); + } + return null; +} + +// why: a list key holds an inline value, or items indented under it; `none` is empty. +function items(key) { + const lines = message.split("\n"); + const at = lines.findIndex((l) => l.startsWith(`${key}:`)); + if (at === -1) return []; + const inline = lines[at].slice(key.length + 1).trim(); + const out = inline === "" || inline === "none" ? [] : [inline]; + for (const l of lines.slice(at + 1)) { + if (!/^\s{2,}\S/.test(l)) break; + if (l.trim() !== "none") out.push(l.trim()); + } + return out; +} + function block(reason) { + blocked = true; console.log(JSON.stringify({ decision: "block", reason })); } diff --git a/src/report-guard.test.ts b/src/report-guard.test.ts index 893165ae2..f7aab7968 100644 --- a/src/report-guard.test.ts +++ b/src/report-guard.test.ts @@ -1,4 +1,6 @@ import { execFileSync } from "node:child_process"; +import { mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; @@ -12,27 +14,38 @@ const SCRIPT = join( "report-guard.mjs", ); +const REVIEWS = + "Spec review: 1 findings (0 blocking) Code review: 2 findings (1 blocking) Claims review: skipped"; +const CWD = mkdtempSync(join(tmpdir(), "report-guard-")); +writeFileSync(join(CWD, "findings.json"), '{"findings": []}'); +const REVIEWER = `PR: #456 Round: 2 Kind: full Commit: abc\nAssessment: complete\nRisk: routine — x\n${REVIEWS}\nFix: none\nRerun: none\nFindings file: findings.json`; + function stop(agentType: string, lastMessage: string): string { return execFileSync("node", [SCRIPT], { - input: JSON.stringify({ agent_type: agentType, last_assistant_message: lastMessage }), + input: JSON.stringify({ agent_type: agentType, last_assistant_message: lastMessage, cwd: CWD }), encoding: "utf8", }); } describe("report-guard.mjs", () => { it("requires explicit review coverage but lets incomplete work stop honestly", () => { - const report = "PR: #456 Round: 2 Kind: full Commit: abc"; + const report = REVIEWER.replace("Assessment: complete\n", ""); expect(JSON.parse(stop("reviewer", report)).reason).toContain("Assessment"); - expect(stop("reviewer", `${report}\nAssessment: incomplete — budget exhausted`)).toBe(""); + expect( + stop( + "reviewer", + report + .replace("Risk:", "Assessment: incomplete — budget exhausted\nRisk:") + .replace("Findings file: findings.json", "Findings file:"), + ), + ).toBe(""); }); it("lets each stage agent stop once its last message holds its report block", () => { expect( stop("implementer", "```\nissue: 438 branch: task/438 pr: 456 mode: green\n```"), ).toBe(""); - expect( - stop("reviewer", "PR: #456 Round: 2 Kind: full Commit: abc\nAssessment: complete"), - ).toBe(""); + expect(stop("reviewer", REVIEWER)).toBe(""); expect(stop("hardware-verifier", "PR: #456 Hardware: pass\nRan: e2e/x.test.ts")).toBe(""); }); @@ -81,4 +94,59 @@ describe("report-guard.mjs", () => { }); expect(stop("Explore", "src/a.ts:12")).toBe(""); }); + + it("blocks an implementer whose cycles fall short without remaining lines or a spec needs", () => { + const head = "issue: 966 branch: task/966 pr: 975 mode: build head: 9d78eb95\n"; + + expect( + JSON.parse(stop("implementer", `${head}tests: 4/9 cycles: 4/9\nremaining: none\n`)).reason, + ).toContain("list the 5 Tests lines not built"); + expect( + JSON.parse(stop("implementer", `${head}cycles: 7/9\nremaining:\n a\n`)).reason, + ).toContain("list the 2 Tests lines"); + expect(stop("implementer", `${head}cycles: 7/9\nremaining:\n a\n b\n`)).toBe(""); + expect( + stop("implementer", `${head}cycles: 4/9\nremaining: none\nopen:\n spec needs: the bound\n`), + ).toBe(""); + expect( + stop("implementer", `${head}Assessment: incomplete — out of context\ncycles: 4/9\n`), + ).toBe(""); + }); + + it("blocks an audit whose donewhen or covered count falls short without an open line each", () => { + const head = "issue: 966 branch: task/966 pr: 975 mode: audit head: 9d78eb95\ntests: 9/9\n"; + const audit = (counts: string) => `${head}audit: stale 0 replaced 0 ${counts} probed 2/2\n`; + + expect(JSON.parse(stop("implementer", audit("donewhen 4/6"))).reason).toContain( + "`donewhen 4/6`", + ); + expect( + JSON.parse(stop("implementer", `${audit("donewhen 6/6 covered 3/5")}open:\n x\n`)).reason, + ).toContain("`covered 3/5`"); + expect(stop("implementer", `${audit("donewhen 5/6")}open:\n line 3: needs the lane\n`)).toBe( + "", + ); + }); + + it.each([ + [ + "a Rerun outside its values", + REVIEWER.replace("Rerun: none", "Rerun: maybe"), + "`Rerun:` line", + ], + ["no Rerun line", REVIEWER.replace("Rerun: none\n", ""), "`Rerun:` line"], + [ + "a lens count in another shape", + REVIEWER.replace("1 findings (0 blocking)", "1 note"), + "`Spec review:`", + ], + ["a missing lens", REVIEWER.replace(" Claims review: skipped", ""), "`Claims review:`"], + [ + "a findings file not on disk", + REVIEWER.replace("findings.json", "gone.json"), + "`Findings file:`", + ], + ])("blocks a reviewer report with %s", (_, report, reason) => { + expect(JSON.parse(stop("reviewer", report)).reason).toContain(reason); + }); }); From 9af975a9b8734f8dc05ae648fc192c4b77232eac Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Sun, 25 Oct 2026 09:49:16 +0100 Subject: [PATCH 03/15] chore(checks): the PR body check reads the ledger, Done when, assumptions and the diff's kind; branch names are exact --- .agents/scripts/pr-body-check.mjs | 281 ++++++++++++++++++++++++++---- .github/workflows/issue-state.yml | 34 +++- src/pr-body-check.test.ts | 278 +++++++++++++++++++++++++---- 3 files changed, 523 insertions(+), 70 deletions(-) diff --git a/.agents/scripts/pr-body-check.mjs b/.agents/scripts/pr-body-check.mjs index ffc17cade..70581ca2a 100755 --- a/.agents/scripts/pr-body-check.mjs +++ b/.agents/scripts/pr-body-check.mjs @@ -12,25 +12,61 @@ // - on a / branch, `### Status` or `## Assumptions` is missing (coding rule 2); // - on a / branch, the Review counts line names a review that ran and the PR has no // comment headed `## Review ledger` (REVIEW.md, "Measurement"); checked only with a PR number. +// The ledger's json block has no finding whose `outcome` is `open`, and the `## Review` +// section has the line `Assessment: complete` (rule 15). The ledger's `Round : ` +// lines are 40-hex SHAs with at most one round 1 and one round 2, round 1 first, each an +// ancestor of the head (a warning when this clone lacks a SHA); at most one comment is +// headed `## Review notes` (REVIEW.md, the cap); +// - `## Assumptions` holds lines other than `none` or `- Assumption: — ` (coding +// rule 2); +// - the prose runs over 200 words: everything outside code blocks, checklist lines and the +// Status, Assumptions and Review sections (rule 12); +// - with a PR number, on a / branch: a `## Done when` bullet of issue is not a +// `- [ ]` or `- [x]` line of the body (its first 60 characters, case, backticks and spaces +// folded, start the line; a `— ` may follow), or is not ticked; and when every +// other open sub-issue of the issue's parent is closed by a PR below this one in its stack +// (or there is none), the same for the parent's `## Completion conditions` (coding rule 5); +// - the diff's kind (`diff-kind.mjs` from the merge-base to the head) and the counts line +// disagree: Code review skipped on a `full`, `large` or `tests-only` diff, or Spec review +// skipped on a / branch (REVIEW.md, "Routing"); a Claims review that ran on an +// `adr-only` diff is a warning; +// - a routine's branch changed what its routine may not: `chore/garden-comments-*` is +// `comments-only`, `chore/audit-tests-*` and `chore/harden-tests-*` are `tests-only`, and +// `chore/calibrate-reviews-*` changes `.agents/briefs/calibration.md` alone. // -// It warns (exit 0) when the Review section has no `Rounds: . Tokens: M.` line, or the -// prose runs over 200 words: everything outside code blocks, checklist lines and the Status, -// Assumptions and Review sections (rule 12). `--draft-ok` turns every failure into a warning, -// for the check that runs while the PR is still a draft. `--body-file [--branch ]` reads -// a body from a file instead of GitHub. The repo is GH_REPO, else the origin remote's owner/name. +// The diff is read from this clone: a PR's base and head SHAs, or `--base --head `; +// `--kind ` gives the kind instead. A PR whose SHAs this clone lacks gets a warning in +// place of the diff checks. It warns (exit 0) when the Review section has no `Rounds: . +// Tokens: M.` line. `--draft-ok` turns every failure into a warning, for the check that runs +// while the PR is still a draft. `--body-file [--branch ]` reads a body from a file +// instead of GitHub. The repo is GH_REPO, else the origin remote's owner/name. import { execFileSync } from "node:child_process"; import { readFileSync } from "node:fs"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { option } from "./cli-args.mjs"; const BUDGET = 200; +const HERE = dirname(fileURLToPath(import.meta.url)); const REVIEW_COUNTS = /^Spec review: (?:\d+ blocking, \d+ fixed, \d+ notes?|skipped)\. Code review: (?:\d+ blocking, \d+ fixed, \d+ notes?|skipped)\. Claims review: (?:\d+ blocking, \d+ fixed, \d+ notes?|skipped)\.(?: Comment review: (?:\d+ blocking, \d+ fixed, \d+ notes?|skipped)\.)?\s*$/; const ROUNDS = /^Rounds: \d+\. Tokens: (?:[\d.]+M|n\/a)\.\s*$/m; +const DELIVERY_BRANCH = /^(bug|feature|task)\/(\d+)$/; +const ROUTINES = [ + [/^chore\/garden-comments-/, "comments-only"], + [/^chore\/(?:audit|harden)-tests-/, "tests-only"], +]; +const CALIBRATION = ".agents/briefs/calibration.md"; + const args = process.argv.slice(2); const draftOk = args.includes("--draft-ok"); let body; let branch = ""; let prNumber = null; +let pull = null; +let diffBase = option("--base", args); +let diffHead = option("--head", args); const fileAt = args.indexOf("--body-file"); if (fileAt !== -1) { body = readFileSync(args[fileAt + 1], "utf8"); @@ -44,15 +80,12 @@ if (fileAt !== -1) { } // why: a cloud session's GitHub proxy refuses GraphQL, which `gh pr view` uses; repo-scoped // REST works. - const pull = JSON.parse( - execFileSync("gh", ["api", "-X", "GET", `repos/${repository()}/pulls/${pr}`], { - encoding: "utf8", - maxBuffer: 64 * 1024 * 1024, - }), - ); + pull = api(`pulls/${pr}`); body = pull.body ?? ""; branch = pull.head.ref; prNumber = pr; + diffHead = pull.head.sha ?? null; + diffBase = pull.base?.sha ?? null; } body = body.replace(/\r/g, ""); @@ -87,6 +120,8 @@ if (review === null) { } if (/^Assessment: incomplete\b/im.test(review.join("\n"))) { failures.push("review assessment is incomplete; resolve missing coverage before ready"); + } else if (DELIVERY_BRANCH.test(branch) && !/^Assessment: complete\b/m.test(review.join("\n"))) { + failures.push("the `## Review` section has no `Assessment: complete` line (delivery rule 15)"); } const at = review.findIndex((l) => /^Rejected:\s*$/.test(l)); if (at !== -1) { @@ -107,18 +142,24 @@ if (/spec needs/i.test(prose)) { ); } -if (/^(bug|feature|task)\/\d+$/.test(branch)) { +if (DELIVERY_BRANCH.test(branch)) { if (section(lines, /^### Status\s*$/) === null) failures.push("no `### Status` section"); if (section(lines, /^## Assumptions\s*$/) === null) { failures.push("no `## Assumptions` section (coding rule 2)"); } } +for (const l of section(lines, /^## Assumptions\s*$/) ?? []) { + if (l.trim() === "" || /^none\.?$/i.test(l.trim()) || /^- Assumption: .+ — .+/.test(l)) continue; + failures.push( + `an Assumptions line is not "none" or "- Assumption: — ": "${l.slice(0, 60)}" (coding rule 2)`, + ); +} // why: only a delivery PR (/) has a ledger; a routine's or a person's chore PR gets // the reviewer's findings comment instead. if ( prNumber !== null && - /^(bug|feature|task)\/\d+$/.test(branch) && + DELIVERY_BRANCH.test(branch) && lines.some( (l) => REVIEW_COUNTS.test(l) && @@ -127,32 +168,27 @@ if ( ), ) ) { - const comments = JSON.parse( - execFileSync( - "gh", - [ - "api", - "-X", - "GET", - `repos/${repository()}/issues/${prNumber}/comments`, - "-f", - "per_page=100", - ], - { - encoding: "utf8", - maxBuffer: 64 * 1024 * 1024, - }, - ), - ); - if (!comments.some((c) => /^## Review ledger\s*$/m.test(c.body ?? ""))) { + const comments = api(`issues/${prNumber}/comments`, "-f", "per_page=100"); + const ledger = comments.find((c) => /^## Review ledger\s*$/m.test(c.body ?? "")); + if (ledger === undefined) { failures.push("no comment headed `## Review ledger` (REVIEW.md, Measurement)"); + } else { + checkLedger(ledger.body.replace(/\r/g, "")); + } + if (comments.filter((c) => /^## Review notes\s*$/m.test(c.body ?? "")).length > 1) { + failures.push( + "more than one comment is headed `## Review notes`; notes go out once (REVIEW.md)", + ); } } +if (prNumber !== null && DELIVERY_BRANCH.test(branch)) checkDoneWhen(); +checkDiff(); + const words = budgetedWords(lines); if (words > BUDGET) { - warnings.push( - `${words} words outside the checklist, Status, Assumptions and Review; the budget is ${BUDGET} (delivery rule 12)`, + failures.push( + `${words} words outside the checklist, Status, Assumptions and Review; the budget is ${BUDGET}: cut it (delivery rule 12)`, ); } @@ -163,6 +199,185 @@ for (const f of failures) { } process.exit(failures.length > 0 && !draftOk ? 1 : 0); +function checkLedger(text) { + const json = /```json\n([\s\S]*?)\n```/.exec(text); + let findings = []; + try { + findings = JSON.parse(json?.[1] ?? "").findings ?? []; + } catch { + failures.push( + "the `## Review ledger` comment has no json block that parses (REVIEW.md, Measurement)", + ); + } + for (const f of findings.filter((f) => f.outcome === "open")) { + failures.push(`finding ${f.id} is open in the Review ledger (delivery rule 15)`); + } + const rounds = []; + for (const l of text.split("\n").filter((l) => l.startsWith("Round "))) { + const round = /^Round (1|2|fix-check): ([0-9a-f]{40})\s*$/.exec(l); + if (round === null) + failures.push( + `a ledger line is not "Round <1|2|fix-check>: ": "${l.slice(0, 60)}"`, + ); + else rounds.push({ round: round[1], sha: round[2] }); + } + const order = rounds.map((r) => r.round).filter((r) => r !== "fix-check"); + if (order.filter((r) => r === "1").length > 1 || order.filter((r) => r === "2").length > 1) { + failures.push("the ledger has more than one round 1 or round 2 (REVIEW.md, the cap)"); + } else if (order.length > 0 && order[0] !== "1") { + failures.push("the ledger's first round is not round 1 (REVIEW.md, the cap)"); + } + if (diffHead === null) return; + for (const { round, sha } of rounds) { + if (!hasCommit(sha) || !hasCommit(diffHead)) { + warnings.push( + `round ${round}'s ${sha.slice(0, 9)} or the head is not in this clone; its ancestry is unchecked`, + ); + } else if (!git(["merge-base", "--is-ancestor", sha, diffHead], true)) { + failures.push(`round ${round}'s ${sha.slice(0, 9)} is not an ancestor of the head`); + } + } +} + +function checkDoneWhen() { + const number = Number(DELIVERY_BRANCH.exec(branch)[2]); + const issue = api(`issues/${number}`); + const checklist = lines.flatMap((l) => { + const item = /^\s*- \[([ xX])\] (.*)$/.exec(l); + return item === null ? [] : [{ ticked: item[1] !== " ", text: fold(item[2]) }]; + }); + const walk = (text, heading, from) => { + for (const bullet of bullets(text, heading)) { + const key = fold(bullet) + .replace(/[.;:,]+$/, "") + .slice(0, 60); + const line = checklist.find((c) => c.text.startsWith(key)); + const quoted = `"${bullet.slice(0, 50)}"`; + if (line === undefined) { + failures.push( + `${from}'s ${heading} line ${quoted} is not a checklist line of the body (coding rule 5)`, + ); + } else if (!line.ticked) { + failures.push(`${from}'s ${heading} line ${quoted} is not ticked (coding rule 5)`); + } + } + }; + walk(issue.body ?? "", "Done when", `#${number}`); + const parent = apiOrNull(`issues/${number}/parent`); + if (parent === null) return; + const open = api(`issues/${parent.number}/sub_issues`, "-f", "per_page=100") + .filter((i) => i.state === "open" && i.number !== number) + .map((i) => i.number); + if (open.length > 0) { + const stack = apiOrNull(`stacks?pull_request=${prNumber}`) ?? []; + const members = (stack.find((x) => x.open)?.pull_requests ?? []).map((p) => p.number); + const below = members.slice(0, Math.max(0, members.indexOf(Number(prNumber)))); + const closed = new Set(below.flatMap((m) => closes(api(`pulls/${m}`).body ?? ""))); + if (open.some((n) => !closed.has(n))) return; + } + walk(parent.body ?? "", "Completion conditions", `#${parent.number}`); +} + +function checkDiff() { + const routine = ROUTINES.find(([re]) => re.test(branch)); + const calibration = branch.startsWith("chore/calibrate-reviews-"); + const counts = lines.find((l) => REVIEW_COUNTS.test(l)) ?? ""; + if (DELIVERY_BRANCH.test(branch) && counts.startsWith("Spec review: skipped.")) { + failures.push( + "Spec review is skipped on a branch whose issue is its spec (REVIEW.md, Routing)", + ); + } + let kind = option("--kind", args); + if (kind === null && diffBase !== null && diffHead !== null) { + if (!hasCommit(diffBase) || !hasCommit(diffHead)) { + warnings.push( + "the PR's base or head is not in this clone (git fetch origin); its diff is unchecked", + ); + return; + } + const mergeBase = git(["merge-base", diffBase, diffHead]).trim(); + kind = execFileSync(process.execPath, [join(HERE, "diff-kind.mjs"), mergeBase, diffHead], { + encoding: "utf8", + }).trim(); + if (calibration) { + const names = git(["diff", "--name-only", mergeBase, diffHead]).split("\n").filter(Boolean); + if (names.some((n) => n !== CALIBRATION)) { + failures.push( + `a calibrate-reviews PR changes ${CALIBRATION} alone; it also changes ${names.filter((n) => n !== CALIBRATION)[0]}`, + ); + } + } + } + if (kind === null) return; + if (routine !== undefined && kind !== routine[1]) { + failures.push(`this routine's PR must be ${routine[1]}, and its diff is ${kind}`); + } + if (/\bCode review: skipped\./.test(counts) && ["full", "large", "tests-only"].includes(kind)) { + failures.push(`Code review is skipped on a ${kind} diff (REVIEW.md, Routing)`); + } + if (kind === "adr-only" && counts !== "" && !/\bClaims review: skipped\./.test(counts)) { + warnings.push("a Claims review ran on an adr-only diff, which needs the spec review alone"); + } +} + +function bullets(text, heading) { + const all = withoutCodeBlocks(text.replace(/\r/g, "")).split("\n"); + const out = []; + for (const l of section(all, new RegExp(`^## ${heading}\\s*$`)) ?? []) { + if (/^\s*[-*] /.test(l)) out.push(l.replace(/^\s*[-*] /, "")); + else if (/^\s+\S/.test(l) && out.length > 0) out[out.length - 1] += ` ${l.trim()}`; + } + return out; +} + +function fold(text) { + return text.toLowerCase().replace(/[`*_]/g, "").replace(/\s+/g, " ").trim(); +} + +function closes(text) { + return [ + ...text.matchAll( + /\b(?:close|closes|closed|fix|fixes|fixed|resolve|resolves|resolved)\s+#(\d+)/gi, + ), + ].map((m) => Number(m[1])); +} + +function api(at, ...extra) { + return JSON.parse( + execFileSync("gh", ["api", "-X", "GET", `repos/${repository()}/${at}`, ...extra], { + encoding: "utf8", + maxBuffer: 64 * 1024 * 1024, + stdio: ["ignore", "pipe", "pipe"], + }), + ); +} + +// why: GitHub answers 404 for an issue with no parent and a PR in no stack. +function apiOrNull(at) { + try { + return api(at); + } catch { + return null; + } +} + +function git(argv, status = false) { + try { + const out = execFileSync("git", argv, { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + }); + return status ? true : out; + } catch (error) { + if (status) return false; + throw error; + } +} + +function hasCommit(sha) { + return git(["cat-file", "-e", `${sha}^{commit}`], true); +} + function repository() { if (process.env.GH_REPO) return process.env.GH_REPO; const url = execFileSync("git", ["remote", "get-url", "origin"], { encoding: "utf8" }).trim(); diff --git a/.github/workflows/issue-state.yml b/.github/workflows/issue-state.yml index ea1304504..03725f666 100644 --- a/.github/workflows/issue-state.yml +++ b/.github/workflows/issue-state.yml @@ -14,7 +14,9 @@ name: Issue state # - a PR closed without merge releases the claim on the issue it would close; # - a claim with no activity for three days is released; # - bug:needs-info with no activity for two weeks closes as not planned; -# - a PR from a / branch must close issue and no other; +# - a PR from a / branch must close issue and no other; a branch starting with a +# kind prefix is exactly / (or an ADR's feature/-adr...), and a PR closing a task +# or a bug comes from that issue's /; # - a PR body passes .agents/scripts/pr-body-check.mjs: warnings while it is a draft, a failed # check once it is ready. # Actions taken with the workflow token do not trigger this workflow again, @@ -291,20 +293,39 @@ jobs: name: Branch name closes its issue if: github.event_name == 'pull_request' && github.event.action != 'closed' runs-on: ubuntu-24.04 + permissions: + issues: read timeout-minutes: 5 steps: - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: script: | const pr = context.payload.pull_request; - const m = pr.head.ref.match(/^(bug|feature|task)\/(\d+)$/); + const head = pr.head.ref; + const closes = [...(pr.body ?? "").matchAll(/\b(?:close|closes|closed|fix|fixes|fixed|resolve|resolves|resolved)\s+#(\d+)/gi)] + .map((x) => Number(x[1])); + // why: a slugged / branch would skip every check below and in pr-body-check.mjs. + if (/^(bug|feature|task)\//.test(head) && !/^(bug|feature|task)\/\d+$/.test(head) && !/^feature\/\d+-adr[\w-]*$/.test(head)) { + const repro = /^bug\/\d+-repro$/.test(head) ? " A bug/-repro branch opens no pull request (delivery rule 8)." : ""; + core.setFailed(`Branch ${head} is not named from its issue: it is /, no slug (coding rule 3).${repro}`); + return; + } + for (const n of closes) { + const { data: issue } = await github.rest.issues.get({ ...context.repo, issue_number: n }); + const kind = issue.labels + .map((l) => /^(bug|task):/.exec(typeof l === "string" ? l : l.name)?.[1]) + .find(Boolean); + if (kind !== undefined && head !== `${kind}/${n}`) { + core.setFailed(`#${n} is a ${kind}, so the PR that closes it comes from ${kind}/${n}, not ${head} (coding rule 3).`); + return; + } + } + const m = head.match(/^(bug|feature|task)\/(\d+)$/); if (!m) { - core.info(`${pr.head.ref} is not a / branch; nothing to check`); + core.info(`${head} is not a / branch; nothing more to check`); return; } const expected = Number(m[2]); - const closes = [...(pr.body ?? "").matchAll(/\b(?:close|closes|closed|fix|fixes|fixed|resolve|resolves|resolved)\s+#(\d+)/gi)] - .map((x) => Number(x[1])); if (!closes.includes(expected)) { core.setFailed(`Branch ${pr.head.ref} must close #${expected}: add "Closes #${expected}" to the PR body.`); return; @@ -320,12 +341,15 @@ jobs: runs-on: ubuntu-24.04 permissions: contents: read + issues: read pull-requests: read timeout-minutes: 5 steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false + # pr-body-check.mjs reads the diff's kind from the PR's base and head commits. + fetch-depth: 0 - uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 with: node-version: 22 diff --git a/src/pr-body-check.test.ts b/src/pr-body-check.test.ts index 9abd32734..cb34a9956 100644 --- a/src/pr-body-check.test.ts +++ b/src/pr-body-check.test.ts @@ -1,5 +1,5 @@ import { execFileSync } from "node:child_process"; -import { chmodSync, mkdtempSync, writeFileSync } from "node:fs"; +import { chmodSync, mkdirSync, mkdtempSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; @@ -33,28 +33,37 @@ function check(body: string, ...args: string[]) { } } -function fromGitHub( - pull: object, - options: { origin?: boolean; comments?: readonly string[] } = {}, -) { +interface GitHub { + readonly origin?: boolean; + readonly comments?: readonly string[]; + // why: what each other `repos/o/r/` answers; null is GitHub's 404. + readonly routes?: Readonly>; +} + +function fromGitHub(pull: object, options: GitHub = {}) { const dir = mkdtempSync(join(tmpdir(), "pr-body-gh-")); - writeFileSync(join(dir, "pull.json"), JSON.stringify(pull)); + const routes: Record = { "pulls/7": pull, ...options.routes }; // why: the comments endpoint answers only when the test says so, so a case with no // `comments` proves the script never asked for them (a chore branch has no ledger). if (options.comments !== undefined) { - writeFileSync( - join(dir, "comments.json"), - JSON.stringify(options.comments.map((body) => ({ body }))), - ); + routes["issues/7/comments -f per_page=100"] = options.comments.map((body) => ({ body })); } + writeFileSync(join(dir, "routes.json"), JSON.stringify(routes)); writeFileSync( join(dir, "gh"), - `#!/bin/sh -case "$*" in - "api -X GET repos/o/r/pulls/7") cat ${JSON.stringify(join(dir, "pull.json"))} ;; - "api -X GET repos/o/r/issues/7/comments -f per_page=100") cat ${JSON.stringify(join(dir, "comments.json"))} 2>/dev/null || { echo "fake gh: comments not expected" >&2; exit 9; } ;; - *) echo "fake gh: unexpected: $*" >&2; exit 9 ;; -esac + `#!/usr/bin/env node +const routes = JSON.parse(require("node:fs").readFileSync(${JSON.stringify(join(dir, "routes.json"))}, "utf8")); +const args = process.argv.slice(2); +const key = args.join(" ").replace(/^api -X GET repos\\/o\\/r\\//, ""); +if (args[0] !== "api" || !(key in routes)) { + process.stderr.write("fake gh: unexpected: " + args.join(" ") + "\\n"); + process.exit(9); +} +if (routes[key] === null) { + process.stderr.write("gh: Not Found (HTTP 404)\\n"); + process.exit(1); +} +process.stdout.write(JSON.stringify(routes[key])); `, ); chmodSync(join(dir, "gh"), 0o755); @@ -80,6 +89,9 @@ esac } } +// why: an issue with no Done when, no parent and no stack, for cases about other rules. +const LONE_ISSUE = { "issues/7": { body: "" }, "issues/7/parent": null }; + const valid = [ "Closes #12", "", @@ -90,12 +102,13 @@ const valid = [ "", "## Assumptions", "", - "- Assumption: none", + "none", "", "## Review", "", COUNTS, "Rounds: 1. Tokens: 3.2M.", + "Assessment: complete", "", "Rejected:", "", @@ -105,12 +118,15 @@ const valid = [ "", ].join("\n"); -describe("pr-body-check.mjs", () => { +describe("pr-body-check.mjs", { timeout: 20_000 }, () => { it("refuses incomplete assessment even with zero findings and allows a completed assessment", () => { - const body = valid.replace("Rounds:", "Assessment: incomplete — budget exhausted\nRounds:"); + const body = valid.replace("Assessment: complete", "Assessment: incomplete — budget exhausted"); expect(check(body).status).toBe(1); expect(check(body, "--draft-ok").status).toBe(0); - expect(check(body.replace("incomplete — budget exhausted", "complete")).status).toBe(0); + expect(check(valid).status).toBe(0); + expect(check(valid.replace("Assessment: complete\n", ""), "--branch", "task/12").lines).toEqual( + [expect.stringContaining("no `Assessment: complete` line")], + ); }); it("accepts a body with the counts line, a tagged Rejected line and the footer, without a word", () => { @@ -148,17 +164,20 @@ describe("pr-body-check.mjs", () => { expect(check(bare, "--branch", "chore/x").status).toBe(0); expect(check(bare, "--branch", "bug/7").lines).toEqual([ + expect.stringContaining("no `Assessment: complete` line"), "pr-body: no `### Status` section", expect.stringContaining("no `## Assumptions` section"), ]); }); - it("warns on prose over 200 words, not counting code blocks, checklist lines or the Status, Assumptions and Review sections", () => { + it("fails prose over 200 words, not counting code blocks, checklist lines or the Status, Assumptions and Review sections", () => { const words = (n: number) => Array.from({ length: n }, (_, i) => `word${i}`).join(" "); const counted = valid.replace("Closes #12", `Closes #12\n\n${words(201)}`); - expect(check(counted).lines).toEqual([ - expect.stringContaining("203 words outside the checklist"), - ]); + expect(check(counted)).toEqual({ + status: 1, + lines: [expect.stringContaining("203 words outside the checklist")], + }); + expect(check(counted, "--draft-ok").status).toBe(0); const uncounted = valid .replace("Implement: done", `Implement: done ${words(300)}`) @@ -172,7 +191,11 @@ describe("pr-body-check.mjs", () => { const bare = `## Review\n\n${COUNTS}\nRounds: 1. Tokens: 1.0M.\n\n${FOOTER}\n`; expect(fromGitHub({ body: bare, head: { ref: "chore/x" } })).toEqual({ status: 0, lines: [] }); - expect(fromGitHub({ body: bare, head: { ref: "bug/7" } }, { comments: [] }).lines).toEqual([ + expect( + fromGitHub({ body: bare, head: { ref: "bug/7" } }, { comments: [], routes: LONE_ISSUE }) + .lines, + ).toEqual([ + expect.stringContaining("no `Assessment: complete` line"), "pr-body: no `### Status` section", expect.stringContaining("no `## Assumptions` section"), expect.stringContaining("no comment headed `## Review ledger`"), @@ -185,22 +208,213 @@ describe("pr-body-check.mjs", () => { it("requires the Review ledger comment on a delivery branch whose reviews ran, and only there", () => { const body = valid; - const ledger = '## Review ledger\n\nRound 1: abc\n```json\n{"findings": []}\n```'; - expect(fromGitHub({ body, head: { ref: "task/7" } }, { comments: ["hi", ledger] })).toEqual({ - status: 0, - lines: [], - }); - expect(fromGitHub({ body, head: { ref: "task/7" } }, { comments: ["hi"] }).lines).toEqual([ + const ledger = `## Review ledger\n\nRound 1: ${"a".repeat(40)}\n\`\`\`json\n{"findings": []}\n\`\`\``; + const task = (b: string, comments?: string[]) => + fromGitHub( + { body: b, head: { ref: "task/7" } }, + { ...(comments !== undefined && { comments }), routes: LONE_ISSUE }, + ); + expect(task(body, ["hi", ledger])).toEqual({ status: 0, lines: [] }); + expect(task(body, ["hi"]).lines).toEqual([ "pr-body: no comment headed `## Review ledger` (REVIEW.md, Measurement)", ]); const skipped = body.replace( COUNTS, "Spec review: skipped. Code review: skipped. Claims review: skipped.", ); - expect(fromGitHub({ body: skipped, head: { ref: "task/7" } }).status).toBe(0); + expect(task(skipped).lines).toEqual([expect.stringContaining("Spec review is skipped")]); expect(fromGitHub({ body, head: { ref: "chore/x" } }).status).toBe(0); }); + it("fails an open finding, a malformed or repeated round, a round off the head, and two notes comments", () => { + const head = execFileSync("git", ["rev-parse", "HEAD"], { encoding: "utf8" }).trim(); + const older = execFileSync("git", ["rev-parse", "HEAD~1"], { encoding: "utf8" }).trim(); + const ledger = (rounds: string, findings = "[]") => + `## Review ledger\n\n${rounds}\n\`\`\`json\n{"findings": ${findings}}\n\`\`\``; + const task = (...comments: string[]) => + fromGitHub( + { body: valid, head: { ref: "task/7", sha: older } }, + { comments, routes: LONE_ISSUE }, + ); + + expect(task(ledger(`Round 1: ${older}\nRound fix-check: ${older}`))).toEqual({ + status: 0, + lines: [], + }); + expect(task(ledger(`Round 1: ${older}`, '[{"id":"R2","outcome":"open"}]')).lines).toEqual([ + expect.stringContaining("finding R2 is open"), + ]); + expect(task(ledger("Round 1: abc")).lines).toEqual([ + expect.stringContaining('is not "Round <1|2|fix-check>: "'), + ]); + expect(task(ledger(`Round 2: ${older}\nRound 1: ${older}`)).lines).toEqual([ + expect.stringContaining("first round is not round 1"), + ]); + expect(task(ledger(`Round 1: ${older}\nRound 1: ${older}`)).lines).toEqual([ + expect.stringContaining("more than one round 1 or round 2"), + ]); + expect(task(ledger(`Round 1: ${head}`)).lines).toEqual([ + expect.stringContaining("is not an ancestor of the head"), + ]); + expect(task(ledger(`Round 1: ${"b".repeat(40)}`)).lines).toEqual([ + expect.stringContaining("its ancestry is unchecked"), + ]); + const notes = "## Review notes\n\n- spec: x\n\n*Written by an agent.*"; + expect(task(ledger(`Round 1: ${older}`), notes, notes).lines).toEqual([ + expect.stringContaining("more than one comment is headed `## Review notes`"), + ]); + }); + + it("fails an Assumptions line that is not none or one Assumption line", () => { + const body = (line: string) => valid.replace("\nnone\n", `\n${line}\n`); + + expect( + check(body("- Assumption: the wording — today's text, the smallest change.")).status, + ).toBe(0); + expect(check(body("- I picked the shorter message")).lines).toEqual([ + expect.stringContaining('an Assumptions line is not "none"'), + ]); + expect(check(body("- Assumption: a bound with no choice")).status).toBe(1); + }); + + it("walks the issue's Done when lines as checklist lines, ticked once the PR is ready", () => { + const issue = { + "issues/7": { + body: "## Done when\n\n- `pnpm test:changed` passes with the Tests lines above.\n- The website build passes.\n\n## Out of scope\n\nx\n", + }, + "issues/7/parent": null, + }; + const body = (list: string) => valid.replace("- [x] a Done when line", list); + const ledger1 = `## Review ledger\n\nRound 1: ${"a".repeat(40)}\n\`\`\`json\n{"findings": []}\n\`\`\``; + const task = (list: string) => + fromGitHub( + { body: body(list), head: { ref: "task/7" } }, + { comments: [ledger1], routes: issue }, + ); + + expect( + task( + "- [x] pnpm test:changed passes with the Tests lines above — 9/9 green\n- [x] The website build passes.", + ), + ).toEqual({ status: 0, lines: [] }); + expect( + task( + "- [x] `pnpm test:changed` passes with the Tests lines above.\n- [ ] The website build passes.", + ).lines, + ).toEqual([expect.stringContaining('"The website build passes." is not ticked')]); + expect(task("- [x] The website build passes.").lines).toEqual([ + expect.stringContaining( + '#7\'s Done when line "`pnpm test:changed` passes with the Tests lines ab" is not a checklist line', + ), + ]); + }); + + it("walks the parent's Completion conditions on the PR that closes its last open sub-issue, counting the stack below", () => { + const ledger1 = `## Review ledger\n\nRound 1: ${"a".repeat(40)}\n\`\`\`json\n{"findings": []}\n\`\`\``; + const parent = { + number: 9, + body: "## Completion conditions\n\n- One requester holds an iOS and an Android lease.\n", + }; + const routes = (subIssues: object[], stack: object[] | null) => ({ + "issues/7": { body: "" }, + "issues/7/parent": parent, + "issues/9/sub_issues -f per_page=100": subIssues, + "stacks?pull_request=7": stack, + "pulls/5": { body: "Closes #6" }, + }); + const task = (r: object) => + fromGitHub( + { body: valid, head: { ref: "task/7" } }, + { comments: [ledger1], routes: r as Record }, + ); + const missing = [expect.stringContaining("#9's Completion conditions line")]; + const open6 = [ + { number: 6, state: "open" }, + { number: 7, state: "open" }, + ]; + const stack = [{ open: true, pull_requests: [{ number: 5 }, { number: 7 }] }]; + + expect(task(routes([{ number: 7, state: "open" }], null)).lines).toEqual(missing); + expect(task(routes(open6, stack)).lines).toEqual(missing); + expect(task(routes(open6, null))).toEqual({ status: 0, lines: [] }); + expect( + task(routes(open6, [{ open: true, pull_requests: [{ number: 7 }, { number: 5 }] }])), + ).toEqual({ status: 0, lines: [] }); + }); + + it("fails a skipped lens the diff's kind needs, and warns on a claims lens for an ADR-only diff", () => { + const counts = (spec: string, code: string, claims: string) => + valid.replace( + COUNTS, + `Spec review: ${spec}. Code review: ${code}. Claims review: ${claims}.`, + ); + const ran = "0 blocking, 0 fixed, 0 notes"; + + expect(check(counts(ran, "skipped", ran), "--kind", "docs-only").status).toBe(0); + for (const kind of ["full", "large", "tests-only"]) { + expect(check(counts(ran, "skipped", ran), "--kind", kind).lines).toEqual([ + expect.stringContaining(`Code review is skipped on a ${kind} diff`), + ]); + } + expect( + check(counts("skipped", ran, ran), "--branch", "task/12", "--kind", "full").lines, + ).toEqual([expect.stringContaining("Spec review is skipped")]); + expect(check(counts(ran, "skipped", ran), "--kind", "adr-only")).toEqual({ + status: 0, + lines: [expect.stringContaining("warning: a Claims review ran on an adr-only diff")], + }); + }); + + it("holds a routine's PR to its kind of change, reading the diff from git", () => { + const repo = mkdtempSync(join(tmpdir(), "pr-body-git-")); + const git = (...a: string[]) => + execFileSync("git", ["-c", "user.name=t", "-c", "user.email=t@t", ...a], { + cwd: repo, + encoding: "utf8", + }).trim(); + git("init", "-q"); + mkdirSync(join(repo, "src")); + mkdirSync(join(repo, ".agents", "briefs"), { recursive: true }); + writeFileSync(join(repo, "src", "a.ts"), "export const a = 1;\n"); + writeFileSync(join(repo, ".agents", "briefs", "calibration.md"), "x\n"); + git("add", "."); + git("commit", "-qm", "base"); + const base = git("rev-parse", "HEAD"); + writeFileSync(join(repo, "src", "a.ts"), "export const a = 2;\n"); + writeFileSync(join(repo, ".agents", "briefs", "calibration.md"), "y\n"); + git("commit", "-qam", "change"); + const head = git("rev-parse", "HEAD"); + const routine = (branch: string) => { + const file = join(repo, "body.md"); + writeFileSync(file, valid); + try { + execFileSync( + process.execPath, + [SCRIPT, "--body-file", file, "--branch", branch, "--base", base, "--head", head], + { + cwd: repo, + encoding: "utf8", + env: { ...process.env, GITHUB_ACTIONS: "" }, + }, + ); + return []; + } catch (error) { + return (error as { stdout: string }).stdout.split("\n").filter((l) => l !== ""); + } + }; + + expect(routine("chore/garden-comments-2026-10-25")).toEqual([ + expect.stringContaining("this routine's PR must be comments-only, and its diff is full"), + expect.stringContaining("Code review is skipped on a full diff"), + ]); + expect(routine("chore/audit-tests-2026-10-25")).toContainEqual( + expect.stringContaining("must be tests-only"), + ); + expect(routine("chore/calibrate-reviews-2026-10-25")).toContainEqual( + expect.stringContaining("it also changes src/a.ts"), + ); + }); + it("takes the repo from the origin remote when GH_REPO is unset", () => { const bare = `## Review\n\n${COUNTS}\nRounds: 1. Tokens: 1.0M.\n\n${FOOTER}\n`; const result = fromGitHub({ body: bare, head: { ref: "chore/x" } }, { origin: true }); From b49bdf4bcc26d34e3af6addeebdaf8988d9d4513 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Sun, 25 Oct 2026 09:52:19 +0100 Subject: [PATCH 04/15] chore(checks): commit subjects on delivery branches, the ADR flip check and the claim script --- .agents/scripts/adr-flip-check.mjs | 78 +++++++++++++ .agents/scripts/claim.mjs | 96 ++++++++++++++++ .agents/scripts/commit-msg-check.mjs | 59 ++++++++++ lefthook.yml | 6 + src/adr-flip-check.test.ts | 82 ++++++++++++++ src/claim.test.ts | 162 +++++++++++++++++++++++++++ src/commit-msg-check.test.ts | 60 ++++++++++ 7 files changed, 543 insertions(+) create mode 100644 .agents/scripts/adr-flip-check.mjs create mode 100644 .agents/scripts/claim.mjs create mode 100644 .agents/scripts/commit-msg-check.mjs create mode 100644 src/adr-flip-check.test.ts create mode 100644 src/claim.test.ts create mode 100644 src/commit-msg-check.test.ts diff --git a/.agents/scripts/adr-flip-check.mjs b/.agents/scripts/adr-flip-check.mjs new file mode 100644 index 000000000..c201e453f --- /dev/null +++ b/.agents/scripts/adr-flip-check.mjs @@ -0,0 +1,78 @@ +#!/usr/bin/env node +// `node .agents/scripts/adr-flip-check.mjs `: the coordinator's mechanical fix-check of +// an `adr: to Accepted` fix (the deliver skill, "Merging the stack"; delivery rule 10). +// It passes (exit 0, `adr-flip: ok ...`) when changes only ADR files and, in +// each, only the `- **Status:**` item (its line and the indented lines that continue it), now +// reading `Accepted` and not "not yet implemented"; and in `docs/internal/adr/README.md` only +// index rows, at most one per ADR the commit flips, each the row of one of them. Anything else +// exits 1 with one `adr-flip: : ` line per defect, and the coordinator sends the +// fix back instead of recording `verified by: coordinator`. +import { execFileSync } from "node:child_process"; + +const commit = process.argv[2]; +if (commit === undefined) { + console.error("usage: adr-flip-check.mjs "); + process.exit(64); +} +const git = (...args) => + execFileSync("git", args, { encoding: "utf8", maxBuffer: 1 << 26 }).replace(/\r/g, ""); +const ADR = /^docs\/internal\/adr\/(\d{4})-[^/]+\.md$/; +const INDEX = "docs/internal/adr/README.md"; + +const changed = git("diff-tree", "--no-commit-id", "--name-status", "-r", "--no-renames", commit) + .split("\n") + .filter(Boolean) + .map((row) => row.split("\t")); +const defects = []; +const flipped = []; +for (const [status, path] of changed) { + if (!ADR.test(path) && path !== INDEX) { + defects.push(`${path}: not an ADR file`); + } else if (status !== "M") { + defects.push(`${path}: ${status === "A" ? "added" : "deleted"}, not edited`); + } else if (path !== INDEX) { + const before = git("show", `${commit}^:${path}`); + const after = git("show", `${commit}:${path}`); + const line = statusItem(after)[0] ?? ""; + if (withoutStatus(before) !== withoutStatus(after)) { + defects.push(`${path}: changes more than its Status line`); + } else if ( + !/^- \*\*Status:\*\* Accepted\b/.test(line) || + /not yet implemented/.test(statusItem(after).join(" ")) + ) { + defects.push(`${path}: its Status does not read Accepted: "${line.slice(0, 60)}"`); + } else { + flipped.push(ADR.exec(path)[1]); + } + } +} +if (changed.some(([, path]) => path === INDEX)) { + const before = git("show", `${commit}^:${INDEX}`).split("\n"); + const after = git("show", `${commit}:${INDEX}`).split("\n"); + const rows = after.filter((l, i) => l !== before[i]); + if (before.length !== after.length) { + defects.push(`${INDEX}: adds or removes lines`); + } else if ( + rows.length > flipped.length || + rows.some((l) => !flipped.some((n) => l.startsWith(`| [${n}]`))) + ) { + defects.push(`${INDEX}: changes a line that is not the index row of an ADR this commit flips`); + } +} +if (flipped.length === 0 && defects.length === 0) defects.push(`${commit}: flips no ADR`); +for (const d of defects) console.log(`adr-flip: ${d}`); +if (defects.length > 0) process.exit(1); +console.log(`adr-flip: ok ${flipped.join(" ")}`); + +function statusItem(text) { + const lines = text.split("\n"); + const at = lines.findIndex((l) => l.startsWith("- **Status:**")); + if (at === -1) return []; + const end = lines.findIndex((l, i) => i > at && !/^\s+\S/.test(l)); + return lines.slice(at, end === -1 ? lines.length : end); +} + +function withoutStatus(text) { + const item = statusItem(text); + return item.length === 0 ? text : text.replace(item.join("\n"), ""); +} diff --git a/.agents/scripts/claim.mjs b/.agents/scripts/claim.mjs new file mode 100644 index 000000000..56227784a --- /dev/null +++ b/.agents/scripts/claim.mjs @@ -0,0 +1,96 @@ +#!/usr/bin/env node +// `node .agents/scripts/claim.mjs [--triage]`: claims issue for this session's GitHub +// user, the one way an agent assigns an issue (delivery rule 2; the tool guard refuses `gh +// issue edit --add-assignee`). It refuses, exit 1 with one `claim: # ...` line saying why, an +// issue that is closed, has an assignee, or carries no label an agent may act on (delivery rule +// 1): `bug:new` with `--triage` and only then, `bug:ready`, `task:ready`. A `task:ready` task +// also needs every issue its `## Depends on` names closed, or a sub-issue of the same feature +// whose PR (from `/`) is open and not a draft. Then it assigns the user, and prints +// `claim: # assigned to `; when someone else got the issue at the same moment it +// unassigns the user again and refuses. The repo is GH_REPO, else the origin remote's owner/name. +import { execFileSync } from "node:child_process"; + +const [number] = process.argv.slice(2); +const triage = process.argv.includes("--triage"); +if (!/^\d+$/.test(number ?? "")) { + console.error("usage: claim.mjs [--triage]"); + process.exit(64); +} +const nwo = repository(); +const issue = api("GET", `issues/${number}`); +const labels = issue.labels.map((l) => (typeof l === "string" ? l : l.name)); +const assignees = issue.assignees.map((a) => a.login); +if (issue.state !== "open") refuse("is closed"); +if (assignees.length > 0) refuse(`is assigned to ${assignees.join(", ")}; it is theirs`); +const work = triage ? ["bug:new"] : ["bug:ready", "task:ready"]; +const label = labels.find((l) => work.includes(l)); +if (label === undefined) { + refuse( + `carries ${labels.filter((l) => /^\w+:/.test(l)).join(", ") || "no state label"}; ` + + (triage + ? "triage takes a bug:new issue" + : "an agent acts on bug:ready and task:ready (with --triage, bug:new)"), + ); +} +if (label === "task:ready") { + const depends = + /^## Depends on\s*$([\s\S]*?)(?=^## |(?![\s\S]))/m.exec(issue.body ?? "")?.[1] ?? ""; + const parent = parentOf(number); + for (const dep of new Set([...depends.matchAll(/#(\d+)/g)].map((m) => m[1]))) { + const other = api("GET", `issues/${dep}`); + if (other.state === "closed") continue; + const kind = other.labels + .map((l) => (typeof l === "string" ? l : l.name)) + .find((l) => /^(task|bug):/.test(l)) + ?.split(":")[0]; + const sameFeature = parent !== null && parentOf(dep) === parent; + const reviewed = + kind !== undefined && + api("GET", `pulls?head=${nwo.split("/")[0]}:${kind}/${dep}&state=open`).some((p) => !p.draft); + if (!sameFeature || !reviewed) { + refuse( + `depends on #${dep}, which is open${sameFeature ? " with no reviewed PR" : " and not a task of the same feature"} (delivery rule 1)`, + ); + } + } +} +const login = execFileSync("gh", ["api", "user", "-q", ".login"], { encoding: "utf8" }).trim(); +const after = api("POST", `issues/${number}/assignees`, "-f", `assignees[]=${login}`); +const others = after.assignees.map((a) => a.login).filter((a) => a !== login); +if (others.length > 0) { + api("DELETE", `issues/${number}/assignees`, "-f", `assignees[]=${login}`); + refuse(`was claimed by ${others.join(", ")} at the same moment; it is theirs`); +} +if (!after.assignees.some((a) => a.login === login)) refuse(`could not be assigned to ${login}`); +console.log(`claim: #${number} assigned to ${login}`); + +function parentOf(n) { + try { + return api("GET", `issues/${n}/parent`).number; + } catch { + return null; + } +} + +function api(method, at, ...extra) { + return JSON.parse( + execFileSync("gh", ["api", "-X", method, `repos/${nwo}/${at}`, ...extra], { + encoding: "utf8", + maxBuffer: 64 * 1024 * 1024, + stdio: ["ignore", "pipe", "pipe"], + }), + ); +} + +function refuse(why) { + console.error(`claim: #${number} ${why}. Not claimed.`); + process.exit(1); +} + +function repository() { + if (process.env.GH_REPO) return process.env.GH_REPO; + const url = execFileSync("git", ["remote", "get-url", "origin"], { encoding: "utf8" }).trim(); + const match = /([^/:]+)\/([^/]+?)(?:\.git)?$/.exec(url); + if (match === null) throw new Error(`Cannot read owner/name from origin ${url}; set GH_REPO`); + return `${match[1]}/${match[2]}`; +} diff --git a/.agents/scripts/commit-msg-check.mjs b/.agents/scripts/commit-msg-check.mjs new file mode 100644 index 000000000..010bf217d --- /dev/null +++ b/.agents/scripts/commit-msg-check.mjs @@ -0,0 +1,59 @@ +#!/usr/bin/env node +// `node .agents/scripts/commit-msg-check.mjs `: the commit-msg hook's check of a +// subject on a delivery branch, in the shapes the build brief and the triager write. On +// `/` the subject is `(): (#)`, one of feat, fix, test, +// docs, chore, refactor, perf, build, ci, the scope optional; another issue's number (a +// feature's, on an ADR commit in its stack) is a warning. On `bug/-repro` it is `test: +// reproduce # — `. Merge commits and every other branch pass. A failure exits 1 with +// the reason and the shape on stderr. +import { execFileSync } from "node:child_process"; +import { readFileSync } from "node:fs"; + +const file = process.argv[2]; +if (file === undefined) { + console.error("usage: commit-msg-check.mjs "); + process.exit(64); +} +const subject = + readFileSync(file, "utf8") + .split("\n") + .find((l) => l.trim() !== "" && !l.startsWith("#")) ?? ""; +let branch = ""; +try { + branch = execFileSync("git", ["branch", "--show-current"], { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + }).trim(); +} catch { + process.exit(0); +} +if (subject.startsWith("Merge ")) process.exit(0); + +const repro = /^bug\/(\d+)-repro$/.exec(branch); +if (repro !== null && !new RegExp(`^test: reproduce #${repro[1]} — .+`).test(subject)) { + fail(`a commit on ${branch} is \`test: reproduce #${repro[1]} — \``); +} +const delivery = /^(?:bug|task|feature)\/(\d+)$/.exec(branch); +if (delivery !== null) { + const shape = + /^(?:feat|fix|test|docs|chore|refactor|perf|build|ci)(?:\([\w./-]+\))?: .+ \(#(\d+)\)$/.exec( + subject, + ); + if (shape === null) { + fail( + `a commit on ${branch} is \`(): (#${delivery[1]})\`, or ` + + `\`test: (#${delivery[1]})\`; is feat, fix, test, docs, ` + + "chore, refactor, perf, build or ci", + ); + } + if (shape[1] !== delivery[1]) { + console.error( + `commit-msg: warning: the subject names #${shape[1]}, and the branch is ${branch}`, + ); + } +} + +function fail(reason) { + console.error(`commit-msg: "${subject}" is refused: ${reason}.`); + process.exit(1); +} diff --git a/lefthook.yml b/lefthook.yml index 1b7afa45f..70c378357 100644 --- a/lefthook.yml +++ b/lefthook.yml @@ -37,6 +37,12 @@ pre-commit: glob: "*.{ts,tsx,mts,cts}" run: pnpm run typecheck +commit-msg: + commands: + # A commit on a delivery branch names its issue, in the build brief's shapes. + subject: + run: node .agents/scripts/commit-msg-check.mjs {1} + # These audit the whole branch against main, so they run once per push rather than on every # checkpoint commit. pre-push: diff --git a/src/adr-flip-check.test.ts b/src/adr-flip-check.test.ts new file mode 100644 index 000000000..bed83e13f --- /dev/null +++ b/src/adr-flip-check.test.ts @@ -0,0 +1,82 @@ +import { execFileSync, spawnSync } from "node:child_process"; +import { mkdirSync, mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; + +import { describe, expect, it } from "vitest"; + +const SCRIPT = join( + dirname(fileURLToPath(import.meta.url)), + "..", + ".agents", + "scripts", + "adr-flip-check.mjs", +); +const ADR = "docs/internal/adr/0048-keep-simulators.md"; +const INDEX = "docs/internal/adr/README.md"; +const adr = (status: string, context = "Simulators live in the set.") => + `# 0048. Keep simulators\n\n- **Status:** ${status}\n for a worker\n- **Date:** 2026-10-24\n\n## Context\n\n${context}\n`; +const index = (status: string) => + `# ADRs\n\n| ADR | Title | Status |\n|---|---|---|\n| [0001](0001-a.md) | A | Accepted |\n| [0048](0048-keep-simulators.md) | Keep simulators | ${status} |\n`; + +// why: each case is one commit on top of a base that holds the ADR not yet implemented. +function flip(files: Readonly>) { + const repo = mkdtempSync(join(tmpdir(), "adr-flip-")); + const git = (...args: string[]) => + execFileSync("git", ["-c", "user.name=t", "-c", "user.email=t@t", ...args], { + cwd: repo, + encoding: "utf8", + }); + const write = (all: Readonly>) => { + for (const [path, text] of Object.entries(all)) { + mkdirSync(join(repo, dirname(path)), { recursive: true }); + writeFileSync(join(repo, path), text); + } + }; + git("init", "-q"); + write({ + [ADR]: adr("Accepted — not yet implemented"), + [INDEX]: index("Accepted — not yet implemented"), + }); + git("add", "."); + git("commit", "-qm", "base"); + write(files); + git("add", "."); + git("commit", "-qm", "adr"); + const result = spawnSync(process.execPath, [SCRIPT, "HEAD"], { cwd: repo, encoding: "utf8" }); + return { status: result.status, stdout: result.stdout }; +} + +describe("adr-flip-check.mjs", () => { + it("passes a commit that flips the Status line and the index row, and nothing else", () => { + expect(flip({ [ADR]: adr("Accepted"), [INDEX]: index("Accepted") })).toEqual({ + status: 0, + stdout: "adr-flip: ok 0048\n", + }); + }); + + it.each([ + [ + "a paragraph", + { [ADR]: adr("Accepted", "Simulators move.") }, + "changes more than its Status line", + ], + ["a status that is not Accepted", { [ADR]: adr("Proposed") }, "does not read Accepted"], + ["another file", { [ADR]: adr("Accepted"), "src/a.ts": "x\n" }, "src/a.ts: not an ADR file"], + [ + "another index row", + { + [ADR]: adr("Accepted"), + [INDEX]: index("Accepted").replace("| A | Accepted", "| A | Rejected"), + }, + "not the index row of an ADR this commit flips", + ], + ["no ADR", { [INDEX]: index("Accepted") }, "not the index row"], + ])("fails a commit that changes %s", (_, files, reason) => { + const result = flip(files); + + expect(result.status).toBe(1); + expect(result.stdout).toContain(reason); + }); +}); diff --git a/src/claim.test.ts b/src/claim.test.ts new file mode 100644 index 000000000..cec45eff0 --- /dev/null +++ b/src/claim.test.ts @@ -0,0 +1,162 @@ +import { spawnSync } from "node:child_process"; +import { chmodSync, existsSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; + +import { describe, expect, it } from "vitest"; + +const SCRIPT = join( + dirname(fileURLToPath(import.meta.url)), + "..", + ".agents", + "scripts", + "claim.mjs", +); + +interface Issue { + readonly state?: string; + readonly labels?: readonly string[]; + readonly assignees?: readonly string[]; + readonly body?: string; + readonly parent?: number; +} + +// why: the fake answers the REST calls the script makes and records the assignment it asks for. +const FAKE_GH = String.raw`#!/usr/bin/env node +const fs = require("node:fs"); +const d = process.env.FAKE_GH_DIR; +const world = JSON.parse(fs.readFileSync(d + "/world.json", "utf8")); +const args = process.argv.slice(2); +const out = (v) => { process.stdout.write(typeof v === "string" ? v + "\n" : JSON.stringify(v)); process.exit(0); }; +if (args.join(" ") === "api user -q .login") out("me"); +const [, , method, path] = args; +const m = /^repos\/o\/r\/(.*)$/.exec(path ?? ""); +const at = m === null ? "" : m[1]; +const issue = (n) => world.issues[n]; +const shape = (n) => ({ ...issue(n), labels: (issue(n).labels ?? []).map((name) => ({ name })), assignees: (issue(n).assignees ?? []).map((login) => ({ login })) }); +let r; +if (method === "GET" && (r = /^issues\/(\d+)$/.exec(at))) out(shape(r[1])); +if (method === "GET" && (r = /^issues\/(\d+)\/parent$/.exec(at))) { + if (issue(r[1]).parent === undefined) { process.stderr.write("HTTP 404"); process.exit(1); } + out({ number: issue(r[1]).parent }); +} +if (method === "GET" && (r = /^pulls\?head=o:(\w+\/\d+)&state=open$/.exec(at))) out(world.pulls[r[1]] ?? []); +if ((method === "POST" || method === "DELETE") && (r = /^issues\/(\d+)\/assignees$/.exec(at))) { + fs.appendFileSync(d + "/calls", method + " " + r[1] + " " + args.slice(4).join(" ") + "\n"); + out({ assignees: [...(world.raceWith ?? []), "me"].map((login) => ({ login })) }); +} +process.stderr.write("fake gh: unexpected: " + args.join(" ") + "\n"); +process.exit(9); +`; + +function claim( + world: { + issues: Record; + pulls?: Record; + raceWith?: string[]; + }, + ...args: string[] +) { + const dir = mkdtempSync(join(tmpdir(), "claim-")); + writeFileSync(join(dir, "world.json"), JSON.stringify(world)); + writeFileSync(join(dir, "gh"), FAKE_GH); + chmodSync(join(dir, "gh"), 0o755); + const result = spawnSync(process.execPath, [SCRIPT, ...args], { + encoding: "utf8", + env: { ...process.env, PATH: `${dir}:${process.env.PATH}`, FAKE_GH_DIR: dir, GH_REPO: "o/r" }, + }); + const calls = existsSync(join(dir, "calls")) ? readFileSync(join(dir, "calls"), "utf8") : ""; + return { status: result.status, out: result.stdout + result.stderr, calls }; +} + +const task = (body: string, extra: Issue = {}): Issue => ({ + state: "open", + labels: ["task:ready"], + body: `Part of #9.\n\n## Depends on\n\n${body}\n\n*Written by an agent.*`, + parent: 9, + ...extra, +}); + +describe("claim.mjs", () => { + it.each([ + ["a ready bug", { 5: { state: "open", labels: ["bug:ready"] } }, []], + ["a bug:new issue to triage", { 5: { state: "open", labels: ["bug:new"] } }, ["--triage"]], + ["a task with no dependency", { 5: task("none") }, []], + [ + "a task whose dependency closed", + { 5: task("- #4"), 4: { state: "closed", labels: ["task:ready"] } }, + [], + ], + ])("assigns %s to the user", (_, issues, flags) => { + expect(claim({ issues }, "5", ...flags)).toEqual({ + status: 0, + out: "claim: #5 assigned to me\n", + calls: "POST 5 -f assignees[]=me\n", + }); + }); + + it("takes a task whose dependency is a reviewed task of the same feature", () => { + const issues = { 5: task("- #4"), 4: { state: "open", labels: ["task:ready"], parent: 9 } }; + + expect(claim({ issues, pulls: { "task/4": [{ draft: false }] } }, "5").status).toBe(0); + expect(claim({ issues, pulls: { "task/4": [{ draft: true }] } }, "5").out).toContain( + "depends on #4, which is open with no reviewed PR", + ); + expect( + claim( + { + issues: { ...issues, 4: { ...issues[4], parent: 8 } }, + pulls: { "task/4": [{ draft: false }] }, + }, + "5", + ).out, + ).toContain("not a task of the same feature"); + }); + + it.each([ + ["a closed issue", { state: "closed", labels: ["bug:ready"] }, [], "is closed"], + [ + "an assigned issue", + { state: "open", labels: ["bug:ready"], assignees: ["ann"] }, + [], + "is assigned to ann", + ], + [ + "a bug:new issue without --triage", + { state: "open", labels: ["bug:new"] }, + [], + "carries bug:new", + ], + [ + "a ready bug with --triage", + { state: "open", labels: ["bug:ready"] }, + ["--triage"], + "triage takes a bug:new", + ], + ["a draft task", { state: "open", labels: ["task:draft"] }, [], "carries task:draft"], + [ + "a planned feature", + { state: "open", labels: ["feature:planned"] }, + [], + "carries feature:planned", + ], + ])("refuses %s and assigns nobody", (_, issue, flags, reason) => { + const result = claim({ issues: { 5: issue } }, "5", ...flags); + + expect(result.status).toBe(1); + expect(result.out).toContain(reason); + expect(result.calls).toBe(""); + }); + + it("gives the issue back when someone else claimed it at the same moment", () => { + const result = claim( + { issues: { 5: { state: "open", labels: ["bug:ready"] } }, raceWith: ["ann"] }, + "5", + ); + + expect(result.status).toBe(1); + expect(result.out).toContain("was claimed by ann at the same moment"); + expect(result.calls).toBe("POST 5 -f assignees[]=me\nDELETE 5 -f assignees[]=me\n"); + }); +}); diff --git a/src/commit-msg-check.test.ts b/src/commit-msg-check.test.ts new file mode 100644 index 000000000..ac18f9d0c --- /dev/null +++ b/src/commit-msg-check.test.ts @@ -0,0 +1,60 @@ +import { spawnSync, execFileSync } from "node:child_process"; +import { mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; + +import { describe, expect, it } from "vitest"; + +const SCRIPT = join( + dirname(fileURLToPath(import.meta.url)), + "..", + ".agents", + "scripts", + "commit-msg-check.mjs", +); + +function commit(branch: string, message: string) { + const repo = mkdtempSync(join(tmpdir(), "commit-msg-")); + execFileSync("git", ["init", "-q", "-b", branch, repo]); + const file = join(repo, "COMMIT_EDITMSG"); + writeFileSync(file, message); + const result = spawnSync(process.execPath, [SCRIPT, file], { cwd: repo, encoding: "utf8" }); + return { status: result.status, stderr: result.stderr }; +} + +describe("commit-msg-check.mjs", () => { + it.each([ + ["task/968", "test: a new simulator's marks name this instance (#968)"], + ["task/968", "feat(ios): keep simulators in Xcode's device set (#968)\n\nBody.\n"], + ["bug/79", "# Please enter the commit message\nfix(leasing): release the hold (#79)"], + ["bug/79-repro", "test: reproduce #79 — a released lease keeps its hold"], + ["task/968", "Merge remote-tracking branch 'origin/feature/962-adr' into task/968"], + ["chore/lints-b", "wip"], + ])("accepts a commit on %s: %s", (branch, message) => { + expect(commit(branch, message)).toEqual({ status: 0, stderr: "" }); + }); + + it.each([ + ["task/968", "wip", "(#968)"], + ["task/968", "feat: keep simulators in the device set", "(#968)"], + ["task/968", "feature(ios): x (#968)", " is feat"], + [ + "bug/79-repro", + "test: a released lease keeps its hold (#79)", + "test: reproduce #79 — ", + ], + ])("refuses a commit on %s: %s", (branch, message, shape) => { + const result = commit(branch, message); + + expect(result.status).toBe(1); + expect(result.stderr).toContain(shape); + }); + + it("warns when the subject names another issue, as an ADR commit in a feature's stack does", () => { + expect(commit("task/968", "docs(adr): 0048 to Accepted (#962)")).toEqual({ + status: 0, + stderr: "commit-msg: warning: the subject names #962, and the branch is task/968\n", + }); + }); +}); From 8a34d21fa865bb5ca1fd4584ff5df146a070994a Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Sun, 25 Oct 2026 09:54:56 +0100 Subject: [PATCH 05/15] chore(checks): the merge gate refuses a feature's last stack while an ADR it changes is not Accepted --- .agents/scripts/merge-pr.sh | 32 +++++++++++++++++ src/merge-pr.test.ts | 68 ++++++++++++++++++++++++++++++++++++- 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/.agents/scripts/merge-pr.sh b/.agents/scripts/merge-pr.sh index 6f5d1c39b..f4799e259 100755 --- a/.agents/scripts/merge-pr.sh +++ b/.agents/scripts/merge-pr.sh @@ -13,6 +13,10 @@ # names>), min waited`, and `merge-pr: # re-running the failed jobs of run : ` # for each run it re-runs. # +# When the PRs it merges close the last open sub-issues of the top PR's feature, every +# `docs/internal/adr/NNNN-*.md` they add or change must read `**Status:** Accepted` (not +# Proposed, not "not yet implemented") at the top PR's head (delivery rule 10), checked first. +# # Red CI gets one re-run of its failed jobs when no failure is this PR's to fix: every red # check is a job GitHub cancelled (no runner came), or every test that failed is named by an # open `flaky-test` issue (testing rule 5). MERGE_PR_POLL_SECONDS sets the poll interval. @@ -151,10 +155,38 @@ gates() { esac } +# why: a feature's ADRs flip to Accepted when it closes (delivery rule 10), and the stack that +# closes its last open sub-issue is the last chance to see that they did. +adr_status() { + closing='(close|closes|closed|fix|fixes|fixed|resolve|resolves|resolved)[[:space:]]+#[0-9]+' + closed=$(for p in $members; do api "pulls/$p" -q '.body // ""' | grep -oiE "$closing" || true; done | + grep -oE '[0-9]+$' | sort -u) + issue=$(api "pulls/$target" -q '.body // ""' | grep -oiE "$closing" | grep -oE '[0-9]+$' | head -n 1 || true) + [ -n "$issue" ] || return 0 + feature=$(api "issues/$issue/parent" -q .number 2>/dev/null) || return 0 + for n in $(paged "issues/$feature/sub_issues" '.[] | select(.state == "open") | .number'); do + echo "$closed" | grep -qx "$n" || return 0 + done + head=$(api "pulls/$target" -q .head.sha) + adrs=$(for p in $members; do paged "pulls/$p/files" '.[] | select(.status != "removed") | .filename'; done | + grep -E '^docs/internal/adr/[0-9]{4}-[^/]*\.md$' | sort -u || true) + for adr in $adrs; do + status=$(api "contents/$adr" -f ref="$head" -H 'Accept: application/vnd.github.raw' | + awk '/^- \*\*Status:\*\*/ { on = 1; printf "%s", $0; next } + on && /^[[:space:]]+[^[:space:]]/ { printf " %s", $0; next } { on = 0 }') + case $status in + *"not yet implemented"*) ;; + "- **Status:** Accepted"*) continue ;; + esac + fail "#$target closes the last open sub-issue of #$feature, and $adr reads \"$(echo "${status:-no Status line}" | cut -c1-80)\"; flip it to Accepted first (delivery rule 10)" + done +} + stack=$(api "stacks?pull_request=$target" \ -q '[.[] | select(.open)][0].pull_requests // [] | .[] | select(.state == "open") | .number') members=$(echo "$stack" | sed "/^$target\$/q") [ -n "$members" ] || members=$target +adr_status for pr in $members; do gates; done if [ "$members" = "$target" ]; then diff --git a/src/merge-pr.test.ts b/src/merge-pr.test.ts index d262490a7..27f12b66c 100644 --- a/src/merge-pr.test.ts +++ b/src/merge-pr.test.ts @@ -48,6 +48,17 @@ interface Fleet { }[]; readonly runsByPr?: Readonly>; readonly target?: number; + readonly feature?: Feature; +} + +// why: the feature behind the PRs' `Closes #`: its parent link, sub-issues, the files each +// PR changes, and each ADR file's text at the top PR's head. +interface Feature { + readonly issue: number; + readonly parent: number; + readonly subIssues: readonly { number: number; state: string }[]; + readonly files: Readonly>; + readonly adrs: Readonly>; } const VALID_BODY = @@ -101,7 +112,7 @@ let filter; for (let i = 4; i < args.length; i += 2) { if (args[i] === "-f") fields[args[i + 1].split("=")[0]] = args[i + 1].split("=").slice(1).join("="); else if (args[i] === "-q") filter = args[i + 1]; - else fail(); + else if (args[i] !== "-H") fail(); } const page = (items) => { const p = Number(fields.page); @@ -144,6 +155,19 @@ if (method === "PUT" && path === "repos/o/r/pulls/" + fleet.target + "/merge" && value = { state: "pending", statuses: page(shown) }; } else if (path === "repos/o/r/issues" && fields.labels === "flaky-test" && fields.state === "open") { value = fleet.flakyIssues; +} else if (fleet.feature !== null && path === "repos/o/r/issues/" + fleet.feature.issue + "/parent") { + value = { number: fleet.feature.parent }; +} else if (/^repos\\/o\\/r\\/issues\\/\\d+\\/parent$/.test(path)) { + process.stderr.write("gh: Not Found (HTTP 404)\\n"); + process.exit(1); +} else if (fleet.feature !== null && path === "repos/o/r/issues/" + fleet.feature.parent + "/sub_issues") { + value = page(fleet.feature.subIssues); +} else if (/^repos\\/o\\/r\\/pulls\\/\\d+\\/files$/.test(path)) { + const files = fleet.feature === null ? [] : (fleet.feature.files[pr] ?? []); + value = page(files.map((filename) => ({ filename, status: "modified" }))); +} else if (fleet.feature !== null && path.startsWith("repos/o/r/contents/") && fields.ref === "abc" + fleet.target) { + process.stdout.write(fleet.feature.adrs[path.slice("repos/o/r/contents/".length)] ?? ""); + process.exit(0); } else { fail(); } @@ -187,6 +211,7 @@ function fleetFile(fleet: Fleet) { stacks: f.stacks, runsByPr: f.runsByPr, target: f.target, + feature: f.feature ?? null, }; } @@ -497,4 +522,45 @@ describe("merge-pr.sh on a stacked PR", { timeout: 20_000 }, () => { expect(result.merged).toBe(false); expect(result.output).toContain("CI not green on PR #11"); }); + + const ADR = "docs/internal/adr/0048-keep-simulators.md"; + const lastTask = (status: string, subIssues = [{ number: 21, state: "open" }]): Fleet => ({ + target: 12, + stacks: [{ open: true, pull_requests: [11, 12].map((number) => ({ number, state: "open" })) }], + body: `Closes #21\n\n${VALID_BODY}`, + feature: { + issue: 21, + parent: 20, + subIssues, + files: { 11: [ADR, "docs/internal/adr/README.md"], 12: ["src/a.ts"] }, + adrs: { + [ADR]: `# 0048. Keep simulators\n\n- **Status:** ${status}\n for a worker\n- **Date:** x\n`, + }, + }, + }); + + it("merges nothing when the stack closes its feature's last open sub-issue and an ADR it changes is not Accepted", () => { + for (const status of ["Accepted — not yet implemented", "Proposed"]) { + const result = gate(lastTask(status)); + + expect(result.status).toBe(1); + expect(result.merged).toBe(false); + expect(result.gated).toEqual([]); + expect(result.output).toContain( + `#12 closes the last open sub-issue of #20, and ${ADR} reads "- **Status:** ${status}`, + ); + } + }); + + it("merges the stack whose ADRs read Accepted, or that leaves a sub-issue of its feature open", () => { + expect(gate(lastTask("Accepted")).merged).toBe(true); + expect( + gate( + lastTask("Accepted — not yet implemented", [ + { number: 21, state: "open" }, + { number: 22, state: "open" }, + ]), + ).merged, + ).toBe(true); + }); }); From 1f5f570381d3a282a32a5957a7bb010742d5a21b Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Sun, 25 Oct 2026 09:56:03 +0100 Subject: [PATCH 06/15] chore(checks): claims go through claim.mjs, the ADR flip through adr-flip-check.mjs, and stage prompts carry their brief or protocol --- .claude/agents/bug-triager.md | 8 ++++---- .claude/agents/reviewer.md | 3 ++- .claude/skills/deliver/SKILL.md | 7 +++++-- .claude/skills/implement/SKILL.md | 2 +- .claude/skills/review/SKILL.md | 2 +- docs/internal/agent-rules/delivery.md | 13 +++++++++---- docs/internal/agent-rules/toolchain.md | 8 ++++++-- 7 files changed, 28 insertions(+), 15 deletions(-) diff --git a/.claude/agents/bug-triager.md b/.claude/agents/bug-triager.md index b1e6c2852..f35eb1865 100644 --- a/.claude/agents/bug-triager.md +++ b/.claude/agents/bug-triager.md @@ -23,12 +23,12 @@ gh issue list --label bug:new --search 'no:assignee' --json number,title,comment ## 1. Claim The issue must be open, carry exactly `bug:new` and have no assignee. -Otherwise (`bug:ready`, `bug:needs-info`, `bug:blocked`, any other label, -closed, or assigned) stop and say why. Triage is the only work on -`bug:new`; `bug:ready` is the maintainer's call. Then: +Triage is the only work on `bug:new`; `bug:ready` is the maintainer's +call. The claim script checks this and assigns you; when it refuses, stop +and say why: ```bash -gh issue edit --add-assignee @me +node .agents/scripts/claim.mjs --triage ``` Continue from a previous attempt, if any, rather than repeating it: diff --git a/.claude/agents/reviewer.md b/.claude/agents/reviewer.md index 12f02a40d..4567d740a 100644 --- a/.claude/agents/reviewer.md +++ b/.claude/agents/reviewer.md @@ -37,7 +37,8 @@ SHA after round 1, prior resolution ledger, and optional `pilot: sonnet`. lens gets its own detached worktree at `commit`, prepared with `.agents/scripts/worktree.sh --prepare `; spec and claims get read-only worktrees. Spawn the selected `spec-reviewer`, - `code-reviewer` and `claims-reviewer` together in the foreground, each + `code-reviewer` and `claims-reviewer` together in the foreground + (`run_in_background: false`), each prompt the ledger tag line (`toolchain.md`, "Review measurement"), then inputs, worktree, round and lens, then the full text of `/briefs/calibration.md`, of `review-later-round.md` for a diff --git a/.claude/skills/deliver/SKILL.md b/.claude/skills/deliver/SKILL.md index 21cd06000..57081ee6c 100644 --- a/.claude/skills/deliver/SKILL.md +++ b/.claude/skills/deliver/SKILL.md @@ -142,7 +142,8 @@ gh issue view --json number,title,labels,assignees,body ## 2. Deliver one issue -Claim it: `gh issue edit --add-assignee @me`. Its branch is +Claim it: `node .agents/scripts/claim.mjs `; a refusal says why the +issue is not yours to take (rules 1 and 2): stop there. Its branch is `/`. A task of a feature, or an issue whose feature has an open ADR PR, is stacked (step 3): its build stage gets `base:` and its PR waits in the stack instead of the gate. If it already has a PR, read the PR body's `### Status` @@ -350,7 +351,9 @@ in flight: first the feature's ADRs. Each ADR under its Decisions is _Accepted — not yet implemented_; send the top PR's branch one `implementer` fix run with `fixes: adr: to Accepted` per ADR (a status line and the index row, nothing else; rule 10), then the -coordinator's mechanical fix-check. Then run +coordinator's mechanical fix-check: `node .agents/scripts/adr-flip-check.mjs +` on each fix commit, `verified by: coordinator` only on exit 0, +another fix run otherwise. Then run `.agents/scripts/stack-sync.sh ` once more (another feature may have merged meanwhile), resolve a conflict as above, then the gate on the top PR: `merge-pr.sh ` gates every PR in the stack and merges them all, diff --git a/.claude/skills/implement/SKILL.md b/.claude/skills/implement/SKILL.md index 87645fb9b..60a2690a5 100644 --- a/.claude/skills/implement/SKILL.md +++ b/.claude/skills/implement/SKILL.md @@ -6,5 +6,5 @@ description: Implement one claimed task or bug one test at a time (build, audit) Arguments: $ARGUMENTS 1. **Title.** Rename this session with `mcp__ccd_session_mgmt__set_session_title` (`session_id: "self"`; load it with ToolSearch first) to `[Implement #N] `, read with `gh issue view` or `gh pr view`. Skip this step when the arguments name no issue or PR, or the tool is missing or declined: the title never stops the run. -2. **Run.** Start the `implementer` agent with the Agent tool: `subagent_type: "implementer"`, no model, its prompt the ledger tag line (`toolchain.md`, "Review measurement") and then the arguments above as `key: value` lines, one fresh agent per mode in the **Order** `implementer.md` gives, each waited for before the next. Do not do its work yourself. +2. **Run.** Start the `implementer` agent with the Agent tool: `subagent_type: "implementer"`, no model, its prompt the ledger tag line (`toolchain.md`, "Review measurement"), then the arguments above as `key: value` lines, then `brief:` and the full text of `.agents/briefs/implement-.md` from this checkout, one fresh agent per mode in the **Order** `implementer.md` gives, each waited for before the next. Do not do its work yourself. 3. **Report.** Print the last report block exactly as returned, with nothing added. diff --git a/.claude/skills/review/SKILL.md b/.claude/skills/review/SKILL.md index 3eccd8887..17599ed78 100644 --- a/.claude/skills/review/SKILL.md +++ b/.claude/skills/review/SKILL.md @@ -6,6 +6,6 @@ description: Run the blind pre-merge spec, code and claims reviews on a PR or br Arguments: $ARGUMENTS 1. **Title.** Rename this session with `mcp__ccd_session_mgmt__set_session_title` (`session_id: "self"`; load it with ToolSearch first) to `[Review PR #M] `, read with `gh issue view` or `gh pr view`. Skip this step when the arguments name no issue or PR, or the tool is missing or declined: the title never stops the run. -2. **Run.** Start the `reviewer` agent with the Agent tool: `subagent_type: "reviewer"`, no model, its prompt the ledger tag line (`toolchain.md`, "Review measurement") and then the arguments above. If the user explicitly prohibits sub-agents, perform the +2. **Run.** Start the `reviewer` agent with the Agent tool: `subagent_type: "reviewer"`, no model, its prompt the ledger tag line (`toolchain.md`, "Review measurement"), then the arguments above, then `protocol:` and the full text of `docs/internal/REVIEW.md` from this checkout. If the user explicitly prohibits sub-agents, perform the protocol directly and label it author self-check, not independent review. 3. **Report.** When it finishes, print its report block exactly as returned, with nothing added. diff --git a/docs/internal/agent-rules/delivery.md b/docs/internal/agent-rules/delivery.md index d2c880ee5..ab541548b 100644 --- a/docs/internal/agent-rules/delivery.md +++ b/docs/internal/agent-rules/delivery.md @@ -193,7 +193,10 @@ is open, done means closed as completed. use it to tell agent text from a person's, since agents post under a maintainer's account. `.agents/scripts/pr-body-check.mjs` checks a PR body's shape and budget: the issue-state workflow runs it on every PR, - and the gate (rule 15) refuses a body it fails. + and the gate (rule 15) refuses a body it fails. The tool guard refuses, + before it is posted, a body or comment without the footer, and a + handoff, triage report, "Spec updated" or review notes comment out of + its shape or budget. 13. **Assume every agent is in a worktree:** [coding.md](coding.md) rule 4. @@ -252,8 +255,7 @@ closes the issue: ```bash gh issue list --search 'label:bug:ready,task:ready no:assignee' -gh issue view --json body -q .body # a task: every Depends on allows it (rule 1) -gh issue edit --add-assignee @me +node .agents/scripts/claim.mjs # refuses a closed, claimed or not-ready issue, or a task its Depends on hold back .agents/scripts/worktree.sh task/ # or bug/, feature/; resumes origin/ ``` @@ -305,7 +307,10 @@ rules mechanically, so nobody has to remember them: - A claim on a `*:ready` issue with no comment, label change, or commit on `/` for three days is released, with a comment. - A pull request from a `/` branch fails its check unless its body - closes `#` and closes nothing else. + closes `#` and closes nothing else. A branch that starts with a kind + prefix and is not exactly `/` (or an ADR's `feature/-adr...`) + fails it, and so does a PR that closes a task or a bug from any branch + but that issue's `/`. The judgment call stays manual by design: `bug:new` → `bug:ready`, once the maintainer has read the triage report. A task needs none: the spec diff --git a/docs/internal/agent-rules/toolchain.md b/docs/internal/agent-rules/toolchain.md index a4317f912..1ec3a77e7 100644 --- a/docs/internal/agent-rules/toolchain.md +++ b/docs/internal/agent-rules/toolchain.md @@ -41,7 +41,10 @@ list and this one the same. - **Commit hook:** formats the staged files and stages the result, then lint, the comment lint (architecture rule 17), the test lint (testing rules 5, 6 and 10 and always-in-scope 8, on the lines the commit adds) and - typecheck. Errors show in its output. + typecheck. Errors show in its output. Its commit-msg step + (`.agents/scripts/commit-msg-check.mjs`) refuses a subject on + `/` that is not `(): (#)`, and on + `bug/-repro` one that is not `test: reproduce # — `. - **Push hook:** Fallow, then the e2e typecheck. - **CI:** what `pnpm check` runs, and Fallow, on every push. The `pnpm check` steps run as parallel jobs (static checks, unit tests, the timing @@ -80,7 +83,8 @@ gate: the ending below. Every other routine ends the same way; its skill adds only what it changes: -1. `reviewer` with the PR number, round 1. Fix each Fix line yourself, +1. `reviewer` with the PR number, round 1, started as the `review` skill + starts it. Fix each Fix line yourself, within the routine's kind of edit, then the check its `Rerun` names: `reviewer` with that round, `previous ` and the complete resolution ledger. `Rerun: none` with findings open: leave the PR in From 171fa13719f8b9447eff79eef347ff5fbec68386 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Sun, 25 Oct 2026 09:58:58 +0100 Subject: [PATCH 07/15] chore(checks): a PR walks its Done when lines in the issue's words, so the body check can match them --- .agents/briefs/implement-build.md | 2 +- .claude/skills/deliver/SKILL.md | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/.agents/briefs/implement-build.md b/.agents/briefs/implement-build.md index dc1492ade..bc297367e 100644 --- a/.agents/briefs/implement-build.md +++ b/.agents/briefs/implement-build.md @@ -79,7 +79,7 @@ cycle wires in are dead code until then. The last cycle's push runs every hook. `base` argument (a task stacked on its feature's last reviewed PR), the PR targets that branch. The PR body, in this order: `Closes #`; a `### Status` section the orchestrator keeps current; the Done when lines -as an unchecked list; `## Assumptions` ("none", or one `- Assumption:` line +as an unchecked list, in the issue's words; `## Assumptions` ("none", or one `- Assumption:` line per gap); the line `*Written by an agent.*`. Keep it current as you go. - Every test command runs in the foreground with `timeout: 600000` set on diff --git a/.claude/skills/deliver/SKILL.md b/.claude/skills/deliver/SKILL.md index 57081ee6c..f17e721fc 100644 --- a/.claude/skills/deliver/SKILL.md +++ b/.claude/skills/deliver/SKILL.md @@ -212,9 +212,10 @@ Review`, `### Status` and `Notes so far:`, and a decision folded into and 5, run `gh pr ready `, add the `needs-hardware` label to the PR, and park instead of running the gate. - Notes never start a round. Keep every Notes and Rejected line. -4. **Finish the PR body.** Walk every Done when line and say how each was - checked; for the last open sub-issue of a feature, the parent's - Completion conditions too (rule 9). Keep `## Assumptions` as it is. Add: +4. **Finish the PR body.** Walk every Done when line: a ticked checklist + line that keeps the issue's words and adds `— ` + (`pr-body-check.mjs` matches the start); for the last open sub-issue of + a feature, the parent's Completion conditions too (rule 9). Keep `## Assumptions` as it is. Add: ```markdown ## Review From dd7132c35b27eebb5056a1573ba848d37ff93a72 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Sun, 25 Oct 2026 10:05:40 +0100 Subject: [PATCH 08/15] chore(checks): the PR body check matches a Done when line by its content words, not its first characters --- .agents/scripts/pr-body-check.mjs | 48 +++++++++++++++++++++++-------- src/pr-body-check.test.ts | 10 ++++++- 2 files changed, 45 insertions(+), 13 deletions(-) diff --git a/.agents/scripts/pr-body-check.mjs b/.agents/scripts/pr-body-check.mjs index 70581ca2a..18dee5c99 100755 --- a/.agents/scripts/pr-body-check.mjs +++ b/.agents/scripts/pr-body-check.mjs @@ -21,9 +21,11 @@ // rule 2); // - the prose runs over 200 words: everything outside code blocks, checklist lines and the // Status, Assumptions and Review sections (rule 12); -// - with a PR number, on a / branch: a `## Done when` bullet of issue is not a -// `- [ ]` or `- [x]` line of the body (its first 60 characters, case, backticks and spaces -// folded, start the line; a `— ` may follow), or is not ticked; and when every +// - with a PR number, on a / branch: a `## Done when` bullet of issue matches no +// `- [ ]` or `- [x]` line of the body, or matches one that is not ticked. A line matches when +// it holds at least 70% of the bullet's content words (lowercased, camelCase split, backticks +// and punctuation dropped, a trailing `s` dropped, stop words left out), so it may reword the +// bullet or add `— `; and when every // other open sub-issue of the issue's parent is closed by a PR below this one in its stack // (or there is none), the same for the parent's `## Completion conditions` (coding rule 5); // - the diff's kind (`diff-kind.mjs` from the merge-base to the head) and the counts line @@ -48,6 +50,13 @@ import { option } from "./cli-args.mjs"; const BUDGET = 200; const HERE = dirname(fileURLToPath(import.meta.url)); +// why: a checklist line may reword its Done when bullet; most of the bullet's words must stay. +const MATCH = 0.7; +const STOP_WORDS = new Set( + "a an the and or but nor of to in on at by for with from into onto as is are was were be been it its this that these those than then there their they them which who whom what when where how all any each every both either no not only so such too very can could may might must shall should will would do doe did has have had here our we you your one".split( + " ", + ), +); const REVIEW_COUNTS = /^Spec review: (?:\d+ blocking, \d+ fixed, \d+ notes?|skipped)\. Code review: (?:\d+ blocking, \d+ fixed, \d+ notes?|skipped)\. Claims review: (?:\d+ blocking, \d+ fixed, \d+ notes?|skipped)\.(?: Comment review: (?:\d+ blocking, \d+ fixed, \d+ notes?|skipped)\.)?\s*$/; const ROUNDS = /^Rounds: \d+\. Tokens: (?:[\d.]+M|n\/a)\.\s*$/m; @@ -242,16 +251,25 @@ function checkLedger(text) { function checkDoneWhen() { const number = Number(DELIVERY_BRANCH.exec(branch)[2]); const issue = api(`issues/${number}`); - const checklist = lines.flatMap((l) => { + const checklist = []; + for (const l of lines) { const item = /^\s*- \[([ xX])\] (.*)$/.exec(l); - return item === null ? [] : [{ ticked: item[1] !== " ", text: fold(item[2]) }]; - }); + if (item !== null) checklist.push({ ticked: item[1] !== " ", text: item[2] }); + else if (/^\s+\S/.test(l) && checklist.length > 0) checklist.at(-1).text += ` ${l.trim()}`; + else if (l.trim() === "" || !/^\s/.test(l)) checklist.push(null); + } + const items = checklist + .filter((c) => c !== null) + .map((c) => ({ ...c, words: new Set(contentWords(c.text)) })); const walk = (text, heading, from) => { for (const bullet of bullets(text, heading)) { - const key = fold(bullet) - .replace(/[.;:,]+$/, "") - .slice(0, 60); - const line = checklist.find((c) => c.text.startsWith(key)); + const wanted = [...new Set(contentWords(bullet))]; + const share = (c) => wanted.filter((w) => c.words.has(w)).length / Math.max(1, wanted.length); + const best = items.reduce( + (a, c) => (a === undefined || share(c) > share(a) ? c : a), + undefined, + ); + const line = best !== undefined && share(best) >= MATCH ? best : undefined; const quoted = `"${bullet.slice(0, 50)}"`; if (line === undefined) { failures.push( @@ -330,8 +348,14 @@ function bullets(text, heading) { return out; } -function fold(text) { - return text.toLowerCase().replace(/[`*_]/g, "").replace(/\s+/g, " ").trim(); +function contentWords(text) { + return text + .replace(/`/g, "") + .replace(/([a-z\d])([A-Z])/g, "$1 $2") + .toLowerCase() + .split(/[^a-z\d]+/) + .map((w) => (w.length > 3 && w.endsWith("s") ? w.slice(0, -1) : w)) + .filter((w) => w !== "" && !STOP_WORDS.has(w)); } function closes(text) { diff --git a/src/pr-body-check.test.ts b/src/pr-body-check.test.ts index cb34a9956..d0b6a46fe 100644 --- a/src/pr-body-check.test.ts +++ b/src/pr-body-check.test.ts @@ -277,7 +277,7 @@ describe("pr-body-check.mjs", { timeout: 20_000 }, () => { expect(check(body("- Assumption: a bound with no choice")).status).toBe(1); }); - it("walks the issue's Done when lines as checklist lines, ticked once the PR is ready", () => { + it("walks the issue's Done when lines as checklist lines, ticked once the PR is ready, matching a reworded line by its words", () => { const issue = { "issues/7": { body: "## Done when\n\n- `pnpm test:changed` passes with the Tests lines above.\n- The website build passes.\n\n## Out of scope\n\nx\n", @@ -307,6 +307,14 @@ describe("pr-body-check.mjs", { timeout: 20_000 }, () => { '#7\'s Done when line "`pnpm test:changed` passes with the Tests lines ab" is not a checklist line', ), ]); + expect( + task( + "- [x] Website builds and passes — ran it\n- [x] With the `Tests` line above `pnpmTest:changed` passing:\n 9/9 green", + ), + ).toEqual({ status: 0, lines: [] }); + expect(task("- [x] The website build passes.\n- [x] pnpm passes above").lines).toEqual([ + expect.stringContaining("is not a checklist line"), + ]); }); it("walks the parent's Completion conditions on the PR that closes its last open sub-issue, counting the stack below", () => { From f8ef6a80ebbb4011f66fef15ec82e0c0dd67108e Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Sun, 25 Oct 2026 10:05:52 +0100 Subject: [PATCH 09/15] chore(checks): the deliver skill says how the body check matches a Done when line --- .claude/skills/deliver/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude/skills/deliver/SKILL.md b/.claude/skills/deliver/SKILL.md index f17e721fc..a54d8ed70 100644 --- a/.claude/skills/deliver/SKILL.md +++ b/.claude/skills/deliver/SKILL.md @@ -214,7 +214,7 @@ Review`, `### Status` and `Notes so far:`, and a decision folded into - Notes never start a round. Keep every Notes and Rejected line. 4. **Finish the PR body.** Walk every Done when line: a ticked checklist line that keeps the issue's words and adds `— ` - (`pr-body-check.mjs` matches the start); for the last open sub-issue of + (`pr-body-check.mjs` matches it by its words); for the last open sub-issue of a feature, the parent's Completion conditions too (rule 9). Keep `## Assumptions` as it is. Add: ```markdown From cb116ba9c3679105f99104794da21d58b684f26c Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Sun, 25 Oct 2026 10:08:00 +0100 Subject: [PATCH 10/15] chore(checks): a Done when line matches on half its prose words, leaving code spans and asides out --- .agents/scripts/pr-body-check.mjs | 14 ++++++++------ src/pr-body-check.test.ts | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/.agents/scripts/pr-body-check.mjs b/.agents/scripts/pr-body-check.mjs index 18dee5c99..a8ae83606 100755 --- a/.agents/scripts/pr-body-check.mjs +++ b/.agents/scripts/pr-body-check.mjs @@ -23,9 +23,10 @@ // Status, Assumptions and Review sections (rule 12); // - with a PR number, on a / branch: a `## Done when` bullet of issue matches no // `- [ ]` or `- [x]` line of the body, or matches one that is not ticked. A line matches when -// it holds at least 70% of the bullet's content words (lowercased, camelCase split, backticks -// and punctuation dropped, a trailing `s` dropped, stop words left out), so it may reword the -// bullet or add `— `; and when every +// it holds at least half the bullet's content words (lowercased, camelCase split, punctuation +// dropped, a trailing `s` dropped, stop words left out; words in code spans and parentheses +// count only when nothing else is left), so it may reword the bullet or add `— `; and when every // other open sub-issue of the issue's parent is closed by a PR below this one in its stack // (or there is none), the same for the parent's `## Completion conditions` (coding rule 5); // - the diff's kind (`diff-kind.mjs` from the merge-base to the head) and the counts line @@ -50,8 +51,8 @@ import { option } from "./cli-args.mjs"; const BUDGET = 200; const HERE = dirname(fileURLToPath(import.meta.url)); -// why: a checklist line may reword its Done when bullet; most of the bullet's words must stay. -const MATCH = 0.7; +// why: the check catches a line the PR never walked, not paraphrase; half the words is enough. +const MATCH = 0.5; const STOP_WORDS = new Set( "a an the and or but nor of to in on at by for with from into onto as is are was were be been it its this that these those than then there their they them which who whom what when where how all any each every both either no not only so such too very can could may might must shall should will would do doe did has have had here our we you your one".split( " ", @@ -263,7 +264,8 @@ function checkDoneWhen() { .map((c) => ({ ...c, words: new Set(contentWords(c.text)) })); const walk = (text, heading, from) => { for (const bullet of bullets(text, heading)) { - const wanted = [...new Set(contentWords(bullet))]; + const prose = bullet.replace(/`[^`]*`/g, " ").replace(/\([^)]*\)/g, " "); + const wanted = [...new Set(contentWords(contentWords(prose).length > 0 ? prose : bullet))]; const share = (c) => wanted.filter((w) => c.words.has(w)).length / Math.max(1, wanted.length); const best = items.reduce( (a, c) => (a === undefined || share(c) > share(a) ? c : a), diff --git a/src/pr-body-check.test.ts b/src/pr-body-check.test.ts index d0b6a46fe..775f99f6f 100644 --- a/src/pr-body-check.test.ts +++ b/src/pr-body-check.test.ts @@ -312,7 +312,7 @@ describe("pr-body-check.mjs", { timeout: 20_000 }, () => { "- [x] Website builds and passes — ran it\n- [x] With the `Tests` line above `pnpmTest:changed` passing:\n 9/9 green", ), ).toEqual({ status: 0, lines: [] }); - expect(task("- [x] The website build passes.\n- [x] pnpm passes above").lines).toEqual([ + expect(task("- [x] The website build passes.\n- [x] pnpm ran, 9/9 green").lines).toEqual([ expect.stringContaining("is not a checklist line"), ]); }); From f69a4ca4f12e580a5db69904cc87d392f6576cfc Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Sun, 25 Oct 2026 10:15:22 +0100 Subject: [PATCH 11/15] chore(checks): split the new guards and checks into rule tables and small helpers, under Fallow's complexity limit --- .agents/scripts/body-rules.mjs | 119 ++--- .agents/scripts/pr-body-check.mjs | 176 ++++---- .agents/scripts/report-guard.mjs | 121 ++++-- .agents/scripts/shell-words.mjs | 354 ++++++++------- .agents/scripts/tool-guard.mjs | 697 ++++++++++++++++-------------- 5 files changed, 821 insertions(+), 646 deletions(-) diff --git a/.agents/scripts/body-rules.mjs b/.agents/scripts/body-rules.mjs index 9cf9f3838..cdaf61b5a 100644 --- a/.agents/scripts/body-rules.mjs +++ b/.agents/scripts/body-rules.mjs @@ -17,38 +17,42 @@ const FOOTER = /^[*_]Written by an agent\.[*_]$/; const HANDOFF = ["Done", "Not done", "Findings", "Blocked on"]; const TRIAGE = ["Reproduction", "Root cause", "Simplest fix", "Alternatives rejected", "Risk"]; +// why: the first lines name what a body is; the first kind whose `at` holds checks it. +const KINDS = [ + { at: (filled) => /^## Handoff\s*$/.test(filled[0] ?? ""), check: handoff }, + { at: isTriageReport, check: triage }, + { at: (filled) => (filled[0] ?? "").startsWith("Spec updated:"), check: specUpdated }, + { at: (filled) => /^## Review notes\s*$/.test(filled[0] ?? ""), check: reviewNotes }, +]; + export function bodyDefects(raw) { const text = String(raw ?? "").replace(/\r/g, ""); - const defects = []; - const prose = withoutCode(text); - const lines = prose.split("\n"); + const lines = withoutCode(text).split("\n"); const filled = text .split("\n") .map((l) => l.trim()) .filter((l) => l !== ""); + const defects = []; if (!FOOTER.test(filled.at(-1) ?? "")) { defects.push( "Every body and comment an agent writes ends with the line `*Written by an agent.*` (delivery rule 12): add it as the last line.", ); } - const first = filled[0] ?? ""; - const second = filled[1] ?? ""; - if (/^## Handoff\s*$/.test(first)) defects.push(...handoff(lines)); - else if ( - /^## Reproduction\s*$/.test(first) || - (/^Supersedes the report above\.?$/.test(first) && /^## Reproduction\s*$/.test(second)) - ) { - defects.push(...triage(lines)); - } else if (first.startsWith("Spec updated:")) { - if (filled.filter((l) => !FOOTER.test(l)).length > 1) { - defects.push( - 'A "Spec updated" comment is one line and the footer (delivery rule 12): say in that line which sections moved.', - ); - } - } else if (/^## Review notes\s*$/.test(first)) { - defects.push(...reviewNotes(lines)); - } - return defects; + const kind = KINDS.find((k) => k.at(filled)); + return kind === undefined ? defects : [...defects, ...kind.check(lines, filled)]; +} + +function isTriageReport(filled) { + const [first = "", second = ""] = filled; + const report = (l) => /^## Reproduction\s*$/.test(l); + return report(first) || (/^Supersedes the report above\.?$/.test(first) && report(second)); +} + +function specUpdated(_, filled) { + if (filled.filter((l) => !FOOTER.test(l)).length <= 1) return []; + return [ + 'A "Spec updated" comment is one line and the footer (delivery rule 12): say in that line which sections moved.', + ]; } function handoff(lines) { @@ -76,39 +80,50 @@ function handoff(lines) { return out; } -function triage(lines) { - const out = []; - const headings = lines.flatMap((l) => { - const h = /^##\s+(.+?)\s*$/.exec(l); - return h === null ? [] : [h[1]]; - }); - const expected = headings.at(-1) === "Side findings" ? [...TRIAGE, "Side findings"] : TRIAGE; - if (headings.join("|") !== expected.join("|")) { - out.push( +const TRIAGE_RULES = [ + { + when: (lines) => { + const headings = lines.flatMap((l) => /^##\s+(.+?)\s*$/.exec(l)?.slice(1) ?? []); + const expected = headings.at(-1) === "Side findings" ? [...TRIAGE, "Side findings"] : TRIAGE; + return headings.join("|") !== expected.join("|"); + }, + reason: () => "A triage report has the sections Reproduction, Root cause, Simplest fix, Alternatives rejected, Risk and, when there are any, Side findings, in that order (delivery rule 8).", - ); - } - const root = section(lines, "Root cause"); - if (root !== null && !/\S+\.\w+:\d+/.test(root.join("\n"))) { - out.push("The Root cause names the file and line, as `path:line` (delivery rule 8)."); - } - const alternatives = section(lines, "Alternatives rejected"); - if (alternatives !== null && alternatives.filter((l) => l.trim() !== "").length > 3) { - out.push( + }, + { + when: (lines) => + sectionHas(lines, "Root cause", (body) => !/\S+\.\w+:\d+/.test(body.join("\n"))), + reason: () => "The Root cause names the file and line, as `path:line` (delivery rule 8).", + }, + { + when: (lines) => + sectionHas( + lines, + "Alternatives rejected", + (body) => body.filter((l) => l.trim() !== "").length > 3, + ), + reason: () => "Alternatives rejected holds at most three lines, one per alternative (delivery rule 8).", - ); - } - const risk = section(lines, "Risk"); - if (risk !== null && risk.filter((l) => /^\s*[-*] /.test(l)).length > 3) { - out.push("Risk holds at most three bullets (delivery rule 8)."); - } - const words = countWords(lines); - if (words > 300) { - out.push( - `This report is ${words} words outside code; the budget is 300 (delivery rule 12): cut it.`, - ); - } - return out; + }, + { + when: (lines) => + sectionHas(lines, "Risk", (body) => body.filter((l) => /^\s*[-*] /.test(l)).length > 3), + reason: () => "Risk holds at most three bullets (delivery rule 8).", + }, + { + when: (lines) => countWords(lines) > 300, + reason: (lines) => + `This report is ${countWords(lines)} words outside code; the budget is 300 (delivery rule 12): cut it.`, + }, +]; + +function triage(lines) { + return TRIAGE_RULES.filter((r) => r.when(lines)).map((r) => r.reason(lines)); +} + +function sectionHas(lines, name, test) { + const body = section(lines, name); + return body !== null && test(body); } function reviewNotes(lines) { diff --git a/.agents/scripts/pr-body-check.mjs b/.agents/scripts/pr-body-check.mjs index a8ae83606..15d42f4fb 100755 --- a/.agents/scripts/pr-body-check.mjs +++ b/.agents/scripts/pr-body-check.mjs @@ -210,48 +210,70 @@ for (const f of failures) { process.exit(failures.length > 0 && !draftOk ? 1 : 0); function checkLedger(text) { + for (const f of ledgerFindings(text).filter((f) => f.outcome === "open")) { + failures.push(`finding ${f.id} is open in the Review ledger (delivery rule 15)`); + } + const rounds = ledgerRounds(text); + checkRoundOrder(rounds.map((r) => r.round).filter((r) => r !== "fix-check")); + if (diffHead !== null) for (const r of rounds) checkAncestry(r); +} + +function ledgerFindings(text) { const json = /```json\n([\s\S]*?)\n```/.exec(text); - let findings = []; try { - findings = JSON.parse(json?.[1] ?? "").findings ?? []; + return JSON.parse(json?.[1] ?? "").findings ?? []; } catch { failures.push( "the `## Review ledger` comment has no json block that parses (REVIEW.md, Measurement)", ); + return []; } - for (const f of findings.filter((f) => f.outcome === "open")) { - failures.push(`finding ${f.id} is open in the Review ledger (delivery rule 15)`); - } +} + +function ledgerRounds(text) { const rounds = []; for (const l of text.split("\n").filter((l) => l.startsWith("Round "))) { const round = /^Round (1|2|fix-check): ([0-9a-f]{40})\s*$/.exec(l); - if (round === null) + if (round === null) { failures.push( `a ledger line is not "Round <1|2|fix-check>: ": "${l.slice(0, 60)}"`, ); - else rounds.push({ round: round[1], sha: round[2] }); + } else rounds.push({ round: round[1], sha: round[2] }); } - const order = rounds.map((r) => r.round).filter((r) => r !== "fix-check"); - if (order.filter((r) => r === "1").length > 1 || order.filter((r) => r === "2").length > 1) { + return rounds; +} + +function checkRoundOrder(order) { + const twice = (r) => order.filter((x) => x === r).length > 1; + if (twice("1") || twice("2")) { failures.push("the ledger has more than one round 1 or round 2 (REVIEW.md, the cap)"); } else if (order.length > 0 && order[0] !== "1") { failures.push("the ledger's first round is not round 1 (REVIEW.md, the cap)"); } - if (diffHead === null) return; - for (const { round, sha } of rounds) { - if (!hasCommit(sha) || !hasCommit(diffHead)) { - warnings.push( - `round ${round}'s ${sha.slice(0, 9)} or the head is not in this clone; its ancestry is unchecked`, - ); - } else if (!git(["merge-base", "--is-ancestor", sha, diffHead], true)) { - failures.push(`round ${round}'s ${sha.slice(0, 9)} is not an ancestor of the head`); - } +} + +function checkAncestry({ round, sha }) { + if (!hasCommit(sha) || !hasCommit(diffHead)) { + warnings.push( + `round ${round}'s ${sha.slice(0, 9)} or the head is not in this clone; its ancestry is unchecked`, + ); + } else if (!git(["merge-base", "--is-ancestor", sha, diffHead], true)) { + failures.push(`round ${round}'s ${sha.slice(0, 9)} is not an ancestor of the head`); } } function checkDoneWhen() { const number = Number(DELIVERY_BRANCH.exec(branch)[2]); - const issue = api(`issues/${number}`); + const items = checklistItems(); + walkLines(items, api(`issues/${number}`).body ?? "", "Done when", `#${number}`); + const parent = apiOrNull(`issues/${number}/parent`); + if (parent !== null && closesLastSubIssue(parent.number, number)) { + walkLines(items, parent.body ?? "", "Completion conditions", `#${parent.number}`); + } +} + +// why: a checklist line may wrap onto indented lines; a blank or unindented line ends it. +function checklistItems() { const checklist = []; for (const l of lines) { const item = /^\s*- \[([ xX])\] (.*)$/.exec(l); @@ -259,76 +281,84 @@ function checkDoneWhen() { else if (/^\s+\S/.test(l) && checklist.length > 0) checklist.at(-1).text += ` ${l.trim()}`; else if (l.trim() === "" || !/^\s/.test(l)) checklist.push(null); } - const items = checklist + return checklist .filter((c) => c !== null) .map((c) => ({ ...c, words: new Set(contentWords(c.text)) })); - const walk = (text, heading, from) => { - for (const bullet of bullets(text, heading)) { - const prose = bullet.replace(/`[^`]*`/g, " ").replace(/\([^)]*\)/g, " "); - const wanted = [...new Set(contentWords(contentWords(prose).length > 0 ? prose : bullet))]; - const share = (c) => wanted.filter((w) => c.words.has(w)).length / Math.max(1, wanted.length); - const best = items.reduce( - (a, c) => (a === undefined || share(c) > share(a) ? c : a), - undefined, +} + +function walkLines(items, text, heading, from) { + for (const bullet of bullets(text, heading)) { + const line = matchingLine(items, bullet); + const quoted = `"${bullet.slice(0, 50)}"`; + if (line === undefined) { + failures.push( + `${from}'s ${heading} line ${quoted} is not a checklist line of the body (coding rule 5)`, ); - const line = best !== undefined && share(best) >= MATCH ? best : undefined; - const quoted = `"${bullet.slice(0, 50)}"`; - if (line === undefined) { - failures.push( - `${from}'s ${heading} line ${quoted} is not a checklist line of the body (coding rule 5)`, - ); - } else if (!line.ticked) { - failures.push(`${from}'s ${heading} line ${quoted} is not ticked (coding rule 5)`); - } + } else if (!line.ticked) { + failures.push(`${from}'s ${heading} line ${quoted} is not ticked (coding rule 5)`); } - }; - walk(issue.body ?? "", "Done when", `#${number}`); - const parent = apiOrNull(`issues/${number}/parent`); - if (parent === null) return; - const open = api(`issues/${parent.number}/sub_issues`, "-f", "per_page=100") + } +} + +function matchingLine(items, bullet) { + const prose = bullet.replace(/`[^`]*`/g, " ").replace(/\([^)]*\)/g, " "); + const wanted = [...new Set(contentWords(contentWords(prose).length > 0 ? prose : bullet))]; + const share = (c) => wanted.filter((w) => c.words.has(w)).length / Math.max(1, wanted.length); + const best = items.reduce((a, c) => (a === undefined || share(c) > share(a) ? c : a), undefined); + return best !== undefined && share(best) >= MATCH ? best : undefined; +} + +// why: in a stack every other open sub-issue must be closed by a PR below this one. +function closesLastSubIssue(parent, number) { + const open = api(`issues/${parent}/sub_issues`, "-f", "per_page=100") .filter((i) => i.state === "open" && i.number !== number) .map((i) => i.number); - if (open.length > 0) { - const stack = apiOrNull(`stacks?pull_request=${prNumber}`) ?? []; - const members = (stack.find((x) => x.open)?.pull_requests ?? []).map((p) => p.number); - const below = members.slice(0, Math.max(0, members.indexOf(Number(prNumber)))); - const closed = new Set(below.flatMap((m) => closes(api(`pulls/${m}`).body ?? ""))); - if (open.some((n) => !closed.has(n))) return; - } - walk(parent.body ?? "", "Completion conditions", `#${parent.number}`); + if (open.length === 0) return true; + const stack = apiOrNull(`stacks?pull_request=${prNumber}`) ?? []; + const members = (stack.find((x) => x.open)?.pull_requests ?? []).map((p) => p.number); + const below = members.slice(0, Math.max(0, members.indexOf(Number(prNumber)))); + const closed = new Set(below.flatMap((m) => closes(api(`pulls/${m}`).body ?? ""))); + return open.every((n) => closed.has(n)); } function checkDiff() { - const routine = ROUTINES.find(([re]) => re.test(branch)); - const calibration = branch.startsWith("chore/calibrate-reviews-"); const counts = lines.find((l) => REVIEW_COUNTS.test(l)) ?? ""; if (DELIVERY_BRANCH.test(branch) && counts.startsWith("Spec review: skipped.")) { failures.push( "Spec review is skipped on a branch whose issue is its spec (REVIEW.md, Routing)", ); } - let kind = option("--kind", args); - if (kind === null && diffBase !== null && diffHead !== null) { - if (!hasCommit(diffBase) || !hasCommit(diffHead)) { - warnings.push( - "the PR's base or head is not in this clone (git fetch origin); its diff is unchecked", - ); - return; - } - const mergeBase = git(["merge-base", diffBase, diffHead]).trim(); - kind = execFileSync(process.execPath, [join(HERE, "diff-kind.mjs"), mergeBase, diffHead], { - encoding: "utf8", - }).trim(); - if (calibration) { - const names = git(["diff", "--name-only", mergeBase, diffHead]).split("\n").filter(Boolean); - if (names.some((n) => n !== CALIBRATION)) { - failures.push( - `a calibrate-reviews PR changes ${CALIBRATION} alone; it also changes ${names.filter((n) => n !== CALIBRATION)[0]}`, - ); - } - } + const kind = option("--kind", args) ?? kindFromGit(); + if (kind !== null) checkKind(kind, counts); +} + +// why: null when the diff cannot be read: no SHAs given, or this clone lacks them. +function kindFromGit() { + if (diffBase === null || diffHead === null) return null; + if (!hasCommit(diffBase) || !hasCommit(diffHead)) { + warnings.push( + "the PR's base or head is not in this clone (git fetch origin); its diff is unchecked", + ); + return null; + } + const mergeBase = git(["merge-base", diffBase, diffHead]).trim(); + if (branch.startsWith("chore/calibrate-reviews-")) checkCalibration(mergeBase); + return execFileSync(process.execPath, [join(HERE, "diff-kind.mjs"), mergeBase, diffHead], { + encoding: "utf8", + }).trim(); +} + +function checkCalibration(mergeBase) { + const other = git(["diff", "--name-only", mergeBase, diffHead]) + .split("\n") + .find((n) => n !== "" && n !== CALIBRATION); + if (other !== undefined) { + failures.push(`a calibrate-reviews PR changes ${CALIBRATION} alone; it also changes ${other}`); } - if (kind === null) return; +} + +function checkKind(kind, counts) { + const routine = ROUTINES.find(([re]) => re.test(branch)); if (routine !== undefined && kind !== routine[1]) { failures.push(`this routine's PR must be ${routine[1]}, and its diff is ${kind}`); } diff --git a/.agents/scripts/report-guard.mjs b/.agents/scripts/report-guard.mjs index e5cc29843..361cea3ab 100644 --- a/.agents/scripts/report-guard.mjs +++ b/.agents/scripts/report-guard.mjs @@ -25,6 +25,14 @@ const REPORT = { "hardware-verifier": /^PR: #\d+\s+Hardware:/m, }; +// why: each check returns its block reason or null; the first reason found is the one given. +const IMPLEMENTER_CHECKS = [ + cyclesDefect, + auditCount("donewhen", "Done when line"), + auditCount("covered", "changed function no test covers"), +]; +const REVIEWER_CHECKS = [rerunDefect, lensCountsDefect, findingsFileDefect]; + const input = JSON.parse(fs.readFileSync(0, "utf8") || "{}"); const shape = REPORT[input.agent_type]; const message = input.last_assistant_message ?? ""; @@ -73,60 +81,77 @@ if (!blocked && input.agent_type === "implementer" && !/^Assessment: incomplete\ } function implementerDefect() { - const open = items("open"); - const cycles = /\bcycles: (\d+)\/(\d+)/.exec(message); - if (cycles !== null && Number(cycles[1]) < Number(cycles[2])) { - const missing = Number(cycles[2]) - Number(cycles[1]); - if (items("remaining").length < missing && !open.some((l) => l.startsWith("spec needs:"))) { - return ( - `Your report says \`cycles ${cycles[1]}/${cycles[2]}\`: list the ${missing} Tests lines not ` + - "built under `remaining`, in build order, or name what stops them under `open` as " + - "`spec needs: `." - ); - } - } - const audit = /^audit: .*$/m.exec(message)?.[0] ?? ""; - for (const [name, what] of [ - ["donewhen", "Done when line"], - ["covered", "changed function no test covers"], - ]) { - const count = new RegExp(`\\b${name} (\\d+)/(\\d+)`).exec(audit); - if (count !== null && open.length < Number(count[2]) - Number(count[1])) { - return ( - `Your \`audit:\` line says \`${name} ${count[1]}/${count[2]}\`: give each ${what} left ` + - "under `open`, with why, or finish it and count it." - ); - } - } - return null; + return firstDefect(IMPLEMENTER_CHECKS); } function reviewerDefect() { - if (!/^Rerun: (?:round 2|fix-check|none)\s*$/m.test(message)) { - return "Your report's `Rerun:` line is `round 2`, `fix-check` or `none`. Report again with it."; - } - for (const lens of ["Spec", "Code", "Claims"]) { - const value = new RegExp( - `\\b${lens} review: (.+?)(?=\\s+(?:Spec|Code|Claims) review:|\\s*$)`, - "m", - ).exec(message)?.[1]; - if (value === undefined || !/^(?:\d+ findings? \(\d+ blocking\)|skipped)$/.test(value)) { - return ( - `Your report's \`${lens} review:\` is \` findings ( blocking)\` or \`skipped\`. ` + - "Report again with the counts." - ); - } + return firstDefect(REVIEWER_CHECKS); +} + +function firstDefect(checks) { + for (const check of checks) { + const defect = check(); + if (defect !== null) return defect; } - const file = /^Findings file: *(.*?)\s*$/m.exec(message)?.[1] ?? ""; - const complete = /^Assessment: complete\b/m.test(message); - const exists = file !== "" && fs.existsSync(path.resolve(input.cwd ?? ".", file)); - if ((complete || file !== "") && !exists) { + return null; +} + +function cyclesDefect() { + const cycles = /\bcycles: (\d+)\/(\d+)/.exec(message); + const missing = cycles === null ? 0 : Number(cycles[2]) - Number(cycles[1]); + if (missing <= 0 || items("remaining").length >= missing) return null; + if (items("open").some((l) => l.startsWith("spec needs:"))) return null; + return ( + `Your report says \`cycles ${cycles[1]}/${cycles[2]}\`: list the ${missing} Tests lines not ` + + "built under `remaining`, in build order, or name what stops them under `open` as " + + "`spec needs: `." + ); +} + +function auditCount(name, what) { + return () => { + const audit = /^audit: .*$/m.exec(message)?.[0] ?? ""; + const count = new RegExp(`\\b${name} (\\d+)/(\\d+)`).exec(audit); + if (count === null || items("open").length >= Number(count[2]) - Number(count[1])) return null; return ( - "Your report's `Findings file:` names no file on disk. Write the findings file REVIEW.md " + - '"Measurement" describes and report its path.' + `Your \`audit:\` line says \`${name} ${count[1]}/${count[2]}\`: give each ${what} left ` + + "under `open`, with why, or finish it and count it." ); - } - return null; + }; +} + +function rerunDefect() { + if (/^Rerun: (?:round 2|fix-check|none)\s*$/m.test(message)) return null; + return "Your report's `Rerun:` line is `round 2`, `fix-check` or `none`. Report again with it."; +} + +function lensCountsDefect() { + const lens = ["Spec", "Code", "Claims"].find( + (l) => !/^(?:\d+ findings? \(\d+ blocking\)|skipped)$/.test(lensCount(l)), + ); + if (lens === undefined) return null; + return ( + `Your report's \`${lens} review:\` is \` findings ( blocking)\` or \`skipped\`. ` + + "Report again with the counts." + ); +} + +function lensCount(lens) { + const value = new RegExp( + `\\b${lens} review: (.+?)(?=\\s+(?:Spec|Code|Claims) review:|\\s*$)`, + "m", + ).exec(message)?.[1]; + return value ?? ""; +} + +function findingsFileDefect() { + const file = /^Findings file: *(.*?)\s*$/m.exec(message)?.[1] ?? ""; + const needed = file !== "" || /^Assessment: complete\b/m.test(message); + if (!needed || (file !== "" && fs.existsSync(path.resolve(input.cwd ?? ".", file)))) return null; + return ( + "Your report's `Findings file:` names no file on disk. Write the findings file REVIEW.md " + + '"Measurement" describes and report its path.' + ); } // why: a list key holds an inline value, or items indented under it; `none` is empty. diff --git a/.agents/scripts/shell-words.mjs b/.agents/scripts/shell-words.mjs index c31b2ba0a..670c1075b 100644 --- a/.agents/scripts/shell-words.mjs +++ b/.agents/scripts/shell-words.mjs @@ -24,129 +24,160 @@ export function commandWords(cmd) { return { words: words.slice(at), dynamic: cmd.dynamic.slice(at) }; } +// why: one entry per character class the scanner meets; the first whose `at` holds scans it. +const SCANNERS = [ + { at: (p) => p.src[p.i] === "\n", scan: newline }, + { at: (p) => p.src[p.i] === " " || p.src[p.i] === "\t", scan: blank }, + { at: (p) => p.src[p.i] === "#" && p.word === null, scan: comment }, + { at: (p) => p.src[p.i] === "\\", scan: escaped }, + { at: (p) => p.src[p.i] === "'", scan: singleQuoted }, + { + at: (p) => p.src[p.i] === '"', + scan: (p) => (p.i = readDoubleQuoted(p.src, p.i + 1, current(p), p.commands)), + }, + { + at: (p) => p.src[p.i] === "$", + scan: (p) => (p.i = readDollar(p.src, p.i, current(p), p.commands)), + }, + { + at: (p) => p.src[p.i] === "`", + scan: (p) => (p.i = readBacktick(p.src, p.i, current(p), p.commands)), + }, + { at: (p) => p.src.startsWith("<<<", p.i), scan: hereString }, + { at: (p) => p.src.startsWith("<<", p.i), scan: heredoc }, + { at: (p) => /^(?:[<>]|&>)/.test(p.src.slice(p.i, p.i + 2)), scan: redirect }, + { at: (p) => ";|&()".includes(p.src[p.i]), scan: separator }, +]; + function parseInto(src, commands) { - let i = 0; - let cmd = newCommand(); - let word = null; - let redirectOp = null; - const pending = []; - - const current = () => (word ??= { text: "", dynamic: false }); - const endWord = () => { - if (word === null) return; - if (redirectOp === "<<<") cmd.stdin = word.text; - else if (redirectOp !== null) cmd.redirects.push({ op: redirectOp, target: word.text }); - else { - cmd.words.push(word.text); - cmd.dynamic.push(word.dynamic); - } - redirectOp = null; - word = null; - }; - const endCommand = () => { - endWord(); - redirectOp = null; - if (cmd.words.length > 0 || cmd.redirects.length > 0 || cmd.stdin !== null) commands.push(cmd); - cmd = newCommand(); - }; + const p = { src, commands, i: 0, cmd: newCommand(), word: null, redirectOp: null, pending: [] }; + while (p.i < src.length) { + const scanner = SCANNERS.find((s) => s.at(p)); + if (scanner === undefined) current(p).text += src[p.i++]; + else scanner.scan(p); + } + endCommand(p); +} - while (i < src.length) { - const c = src[i]; - if (c === "\n") { - endCommand(); - i++; - // why: heredoc bodies start on the line after the operator, in the order they were opened. - for (const doc of pending.splice(0)) { - const lines = []; - while (i < src.length) { - const eol = src.indexOf("\n", i); - const line = src.slice(i, eol === -1 ? src.length : eol); - i = eol === -1 ? src.length : eol + 1; - if ((doc.strip ? line.replace(/^\t+/, "") : line).trim() === doc.delimiter) break; - lines.push(doc.strip ? line.replace(/^\t+/, "") : line); - } - doc.cmd.stdin = lines.join("\n"); - doc.cmd.stdinDynamic = !doc.quoted && /[$`]/.test(doc.cmd.stdin); - } - continue; - } - if (c === " " || c === "\t") { - endWord(); - i++; - continue; - } - if (c === "#" && word === null) { - while (i < src.length && src[i] !== "\n") i++; - continue; - } - if (c === "\\") { - if (src[i + 1] === "\n") i += 2; - else { - current().text += src[i + 1] ?? ""; - i += 2; - } - continue; - } - if (c === "'") { - const end = src.indexOf("'", i + 1); - current().text += src.slice(i + 1, end === -1 ? src.length : end); - i = end === -1 ? src.length : end + 1; - continue; - } - if (c === '"') { - i = readDoubleQuoted(src, i + 1, current(), commands); - continue; - } - if (c === "$") { - i = readDollar(src, i, current(), commands); - continue; - } - if (c === "`") { - i = readBacktick(src, i, current(), commands); - continue; - } - if (c === "<" && src.startsWith("<<<", i)) { - endWord(); - redirectOp = "<<<"; - i += 3; - continue; - } - if (c === "<" && src[i + 1] === "<") { - endWord(); - i += 2; - const strip = src[i] === "-"; - if (strip) i++; - while (src[i] === " " || src[i] === "\t") i++; - const quote = src[i] === "'" || src[i] === '"' ? src[i] : ""; - if (quote !== "") i++; - let delimiter = ""; - while (i < src.length && !/[\s;&|()<>'"]/.test(src[i])) delimiter += src[i++]; - if (quote !== "" && src[i] === quote) i++; - pending.push({ cmd, delimiter, strip, quoted: quote !== "" }); - continue; - } - if (c === ">" || c === "<" || (c === "&" && src[i + 1] === ">")) { - if (word !== null && /^\d+$/.test(word.text) && !word.dynamic) word = null; - else endWord(); - let op = c; - i++; - while (i < src.length && /[>&|]/.test(src[i]) && op.length < 3) op += src[i++]; - if (op.endsWith("&") && /[\d-]/.test(src[i] ?? "")) { - while (/[\d-]/.test(src[i] ?? "")) i++; - continue; - } - redirectOp = op; - continue; - } - if (c === ";" || c === "|" || c === "&" || c === "(" || c === ")") { - endCommand(); - i++; - continue; - } - current().text += c; - i++; +function current(p) { + p.word ??= { text: "", dynamic: false }; + return p.word; +} + +function endWord(p) { + if (p.word === null) return; + if (p.redirectOp === "<<<") p.cmd.stdin = p.word.text; + else if (p.redirectOp !== null) p.cmd.redirects.push({ op: p.redirectOp, target: p.word.text }); + else { + p.cmd.words.push(p.word.text); + p.cmd.dynamic.push(p.word.dynamic); + } + p.redirectOp = null; + p.word = null; +} + +function endCommand(p) { + endWord(p); + p.redirectOp = null; + const { cmd } = p; + if (cmd.words.length > 0 || cmd.redirects.length > 0 || cmd.stdin !== null) p.commands.push(cmd); + p.cmd = newCommand(); +} + +function newline(p) { + endCommand(p); + p.i++; + // why: heredoc bodies start on the line after the operator, in the order they were opened. + for (const doc of p.pending.splice(0)) readHeredocBody(p, doc); +} + +function readHeredocBody(p, doc) { + const lines = []; + while (p.i < p.src.length) { + const line = nextLine(p); + const text = doc.strip ? line.replace(/^\t+/, "") : line; + if (text.trim() === doc.delimiter) break; + lines.push(text); + } + doc.cmd.stdin = lines.join("\n"); + doc.cmd.stdinDynamic = !doc.quoted && /[$`]/.test(doc.cmd.stdin); +} + +function nextLine(p) { + const eol = p.src.indexOf("\n", p.i); + const end = eol === -1 ? p.src.length : eol; + const line = p.src.slice(p.i, end); + p.i = end + 1; + return line; +} + +function blank(p) { + endWord(p); + p.i++; +} + +function comment(p) { + while (p.i < p.src.length && p.src[p.i] !== "\n") p.i++; +} + +function escaped(p) { + if (p.src[p.i + 1] !== "\n") current(p).text += p.src[p.i + 1] ?? ""; + p.i += 2; +} + +function singleQuoted(p) { + const end = p.src.indexOf("'", p.i + 1); + const close = end === -1 ? p.src.length : end; + current(p).text += p.src.slice(p.i + 1, close); + p.i = close + 1; +} + +function hereString(p) { + endWord(p); + p.redirectOp = "<<<"; + p.i += 3; +} + +function heredoc(p) { + endWord(p); + p.i += 2; + const strip = p.src[p.i] === "-"; + if (strip) p.i++; + while (p.src[p.i] === " " || p.src[p.i] === "\t") p.i++; + p.pending.push({ cmd: p.cmd, strip, ...readDelimiter(p) }); +} + +function readDelimiter(p) { + const { src } = p; + const quote = src[p.i] === "'" || src[p.i] === '"' ? src[p.i] : ""; + if (quote !== "") p.i++; + let delimiter = ""; + while (p.i < src.length && !/[\s;&|()<>'"]/.test(src[p.i])) delimiter += src[p.i++]; + if (quote !== "" && src[p.i] === quote) p.i++; + return { delimiter, quoted: quote !== "" }; +} + +function redirect(p) { + // why: a number right before the operator is the file descriptor, not a word. + if (p.word !== null && /^\d+$/.test(p.word.text) && !p.word.dynamic) p.word = null; + else endWord(p); + const op = readOperator(p); + if (op.endsWith("&") && /[\d-]/.test(p.src[p.i] ?? "")) { + while (/[\d-]/.test(p.src[p.i] ?? "")) p.i++; + return; } - endCommand(); + p.redirectOp = op; +} + +function readOperator(p) { + let op = p.src[p.i++]; + while (p.i < p.src.length && /[>&|]/.test(p.src[p.i]) && op.length < 3) op += p.src[p.i++]; + return op; +} + +function separator(p) { + endCommand(p); + p.i++; } function newCommand() { @@ -174,46 +205,61 @@ function readDoubleQuoted(src, i, word, commands) { // why: `"$(cat <<'EOF' … EOF)"` is how a model writes a long body inline; its text is plain. const CAT_HEREDOC = /^\$\(\s*cat\s*<<-?\s*(['"]?)(\w+)\1[ \t]*\n([\s\S]*?)\n[ \t]*\2[ \t]*\n\s*\)/; +// why: what follows a `$` decides what it starts; the first entry whose `at` holds reads it. +const DOLLARS = [ + { at: (next) => next === "(", read: dollarParen }, + { at: (next) => next === "'", read: ansiQuoted }, + { at: (next) => next === "{", read: dollarBrace }, + { at: (next) => /[\w@*#?$!-]/.test(next), read: dollarName }, +]; + function readDollar(src, i, word, commands) { const next = src[i + 1] ?? ""; - if (next === "(") { - const doc = CAT_HEREDOC.exec(src.slice(i)); - if (doc !== null) { - word.text += doc[3]; - if (doc[1] === "" && /[$`]/.test(doc[3])) word.dynamic = true; - return i + doc[0].length; - } - const end = matchingParen(src, i + 2); - parseInto(src.slice(i + 2, end), commands); - word.dynamic = true; - return end + 1; - } - if (next === "'") { - let j = i + 2; - while (j < src.length && src[j] !== "'") { - if (src[j] === "\\" && j + 1 < src.length) { - word.text += { n: "\n", t: "\t", "\\": "\\", "'": "'" }[src[j + 1]] ?? src[j + 1]; - j += 2; - } else word.text += src[j++]; - } - return j + 1; - } - if (next === "{") { - const end = src.indexOf("}", i); - word.dynamic = true; - return end === -1 ? src.length : end + 1; - } - if (/[\w@*#?$!-]/.test(next)) { - let j = i + 1; - if (/\w/.test(next)) while (/\w/.test(src[j] ?? "")) j++; - else j++; - word.dynamic = true; - return j; - } + const reader = DOLLARS.find((d) => d.at(next)); + if (reader !== undefined) return reader.read(src, i, word, commands); word.text += "$"; return i + 1; } +function dollarParen(src, i, word, commands) { + const doc = CAT_HEREDOC.exec(src.slice(i)); + if (doc !== null) { + word.text += doc[3]; + if (doc[1] === "" && /[$`]/.test(doc[3])) word.dynamic = true; + return i + doc[0].length; + } + const end = matchingParen(src, i + 2); + parseInto(src.slice(i + 2, end), commands); + word.dynamic = true; + return end + 1; +} + +const ANSI_ESCAPES = { n: "\n", t: "\t", "\\": "\\", "'": "'" }; + +function ansiQuoted(src, i, word) { + let j = i + 2; + while (j < src.length && src[j] !== "'") { + if (src[j] === "\\" && j + 1 < src.length) { + word.text += ANSI_ESCAPES[src[j + 1]] ?? src[j + 1]; + j += 2; + } else word.text += src[j++]; + } + return j + 1; +} + +function dollarBrace(src, i, word) { + const end = src.indexOf("}", i); + word.dynamic = true; + return end === -1 ? src.length : end + 1; +} + +function dollarName(src, i, word) { + let j = i + 2; + if (/\w/.test(src[i + 1])) while (/\w/.test(src[j] ?? "")) j++; + word.dynamic = true; + return j; +} + function readBacktick(src, i, word, commands) { const end = src.indexOf("`", i + 1); parseInto(src.slice(i + 1, end === -1 ? src.length : end), commands); diff --git a/.agents/scripts/tool-guard.mjs b/.agents/scripts/tool-guard.mjs index 1f991b355..d2ce91442 100644 --- a/.agents/scripts/tool-guard.mjs +++ b/.agents/scripts/tool-guard.mjs @@ -121,100 +121,56 @@ const HOOK_BYPASS = [ /\bLEFTHOOK_EXCLUDE=(?!fallow(?:\s|$))/, ]; -const input = JSON.parse(fs.readFileSync(0, "utf8") || "{}"); -const tool = input.tool_name; -const args = input.tool_input ?? {}; -const subAgent = typeof input.agent_id === "string"; -const cwd = typeof input.cwd === "string" ? input.cwd : process.cwd(); - -if (tool === "Agent" || tool === "Task") agentRules(); - -if (tool === "Bash" && subAgent) { - const shell = code(args.command ?? ""); - if (HOOK_BYPASS.some((re) => re.test(shell))) { - refuse( - "Refused: this skips the git hooks. They run the checks agents never run by hand. Only the " + - "build stage may skip Fallow, on a push before its last cycle: " + - "`LEFTHOOK_EXCLUDE=fallow git push ...`.", - ); - } - const found = forbiddenCheck(shell); - if (found !== null) { - refuse( - `Refused: \`${found}\` is under "Checks agents never run" in toolchain.md. The commit ` + - "hook runs lint, the comment lint and typecheck; the push hook runs Fallow and the e2e " + - "typecheck; CI runs the rest. Their errors show in the commit or push output. Run only " + - 'the commands under "Tests agents run".', - ); - } - const raw = args.command ?? ""; - const slowLane = /slow-e2e\.sh|\.exit\b/.test(raw); - if (!slowLane) { - if (TEST_RUNNER.test(shell) && !(Number(args.timeout) >= 300000)) { - refuse( - "Refused: a test run needs `timeout: 600000` on this Bash call. The default 120 s " + - "moves a two-minute run to the background, and a background job holds your report " + - "until it ends. Run it again in the foreground with the timeout set.", - ); - } - if (args.run_in_background === true || BACKGROUND_JOB.test(shell)) { - refuse( - "Refused: nothing runs in the background in a delivery stage. A background job holds " + - "your report until it ends or hits its 30-minute cap, and polling it costs the turn. " + - "Run the command in the foreground with `timeout: 600000` on the Bash call.", - ); - } - if (SLEEP_LOOP.test(shell)) { - refuse( - "Refused: no loop that sleeps. Nothing you started runs in the background, so there is " + - "nothing to wait for: run the command itself in the foreground with `timeout: 600000`.", - ); - } - if (BARE_CAT.test(raw)) { - refuse( - "Refused: `cat > file` with no input reads stdin until the tool times out. Write the " + - "file with the Write tool, or give cat a heredoc (`cat > file <<'EOF'`).", - ); - } +// why: each rule is a refusal and the test that triggers it; `refuseFirst` walks a table. +function refuseFirst(rules, ctx) { + for (const rule of rules) { + if (rule.when(ctx)) refuse(typeof rule.reason === "function" ? rule.reason(ctx) : rule.reason); } } -if (tool === "Bash") commandRules(args.command ?? ""); -process.exit(0); -function agentRules() { - const type = args.subagent_type; - const prompt = String(args.prompt ?? ""); - if (args.isolation && STAGES.has(type)) { - refuse( - `Refused: pass no \`isolation\` to the ${type}. Every stage of a task works in ` + - "the task's one worktree (`.agents/scripts/worktree.sh `), one stage after " + - "another. Start the agent again without `isolation`.", - ); - } - if (STAGES.has(type) && typeof args.model === "string" && args.model !== "") { - refuse( - `Refused: pass no \`model\` to the ${type}: it runs on the model its frontmatter pins. ` + - "Start the agent again without `model`.", - ); - } - if (subAgent && args.run_in_background !== false) { - refuse( +const AGENT_RULES = [ + { + when: (c) => Boolean(args.isolation) && STAGES.has(c.type), + reason: (c) => + `Refused: pass no \`isolation\` to the ${c.type}. Every stage of a task works in ` + + "the task's one worktree (`.agents/scripts/worktree.sh `), one stage after " + + "another. Start the agent again without `isolation`.", + }, + { + when: (c) => STAGES.has(c.type) && typeof args.model === "string" && args.model !== "", + reason: (c) => + `Refused: pass no \`model\` to the ${c.type}: it runs on the model its frontmatter pins. ` + + "Start the agent again without `model`.", + }, + { + when: () => subAgent && args.run_in_background !== false, + reason: "Refused: a helper agent runs in the foreground: pass `run_in_background: false`. A " + - "background helper reports to the orchestrator, not to you, and nothing wakes you when " + - "it ends.", - ); - } - if (!STAGES.has(type) && !LENSES.has(type)) return; - if (!LEDGER.test(prompt.replace(/^\s+/, "").split("\n")[0])) { - refuse( + "background helper reports to the orchestrator, not to you, and nothing wakes you when " + + "it ends.", + }, + { + when: (c) => (STAGES.has(c.type) || LENSES.has(c.type)) && !LEDGER.test(firstLine(c.prompt)), + reason: "Refused: the prompt's first line is the ledger tag (toolchain.md, \"Review " + - 'measurement"): `ledger: pr= issue= stage= round= role= ' + - "lens=`, `-` for a key that does not apply yet. Start the agent again with it.", - ); - } - if (type === "implementer") briefCheck(prompt); - if (type === "reviewer") protocolCheck(prompt); - if (type === "code-reviewer") strictAreaCheck(prompt); + 'measurement"): `ledger: pr= issue= stage= round= role= ' + + "lens=`, `-` for a key that does not apply yet. Start the agent again with it.", + }, +]; +const PROMPT_CHECKS = { + implementer: briefCheck, + reviewer: protocolCheck, + "code-reviewer": strictAreaCheck, +}; + +function agentRules() { + const ctx = { type: args.subagent_type, prompt: String(args.prompt ?? "") }; + refuseFirst(AGENT_RULES, ctx); + PROMPT_CHECKS[ctx.type]?.(ctx.prompt); +} + +function firstLine(prompt) { + return prompt.replace(/^\s+/, "").split("\n")[0]; } function briefCheck(prompt) { @@ -244,13 +200,8 @@ function protocolCheck(prompt) { .split("\n") .find((l) => l.startsWith("# ")) ?? ""; const at = prompt.search(/^protocol:/m); - if ( - at === -1 || - !prompt - .slice(at) - .split("\n") - .some((l) => l.trim() === heading.trim()) - ) { + const lines = at === -1 ? [] : prompt.slice(at).split("\n"); + if (!lines.some((l) => l.trim() === heading.trim())) { refuse( "Refused: a reviewer's prompt carries `protocol:` and then the full text of " + "`docs/internal/REVIEW.md` from this checkout. Paste it and start the agent again.", @@ -261,15 +212,7 @@ function protocolCheck(prompt) { function strictAreaCheck(prompt) { const model = typeof args.model === "string" ? args.model : ""; if (model === "" || /opus/i.test(model) || /\bstage=replay\b/.test(prompt)) return; - const dir = /^inputs:\s*[`'"]?([^`'"\s]+)/m.exec(prompt)?.[1]; - if (dir === undefined) return; - let patch; - try { - patch = fs.readFileSync(path.resolve(cwd, dir, "diff.patch"), "utf8"); - } catch { - return; - } - const hit = [...patch.matchAll(/^diff --git a\/(\S+) b\/(\S+)/gm)] + const hit = [...inputsPatch(prompt).matchAll(/^diff --git a\/(\S+) b\/(\S+)/gm)] .flatMap((m) => [m[1], m[2]]) .find((p) => STRICT_AREAS.some((re) => re.test(p))); if (hit !== undefined) { @@ -281,251 +224,306 @@ function strictAreaCheck(prompt) { } } +// why: an `inputs:` directory the guard cannot read leaves the call alone. +function inputsPatch(prompt) { + const dir = /^inputs:\s*[`'"]?([^`'"\s]+)/m.exec(prompt)?.[1]; + if (dir === undefined) return ""; + try { + return fs.readFileSync(path.resolve(cwd, dir, "diff.patch"), "utf8"); + } catch { + return ""; + } +} + function commandRules(raw) { - let dir = cwd; - const written = new Map(); + const state = { dir: cwd, written: new Map() }; for (const cmd of parseShell(raw)) { const { words, dynamic } = commandWords(cmd); - if (words.length === 0) continue; - const name = path.basename(words[0]); - if (name === "cd" && words[1] !== undefined && !dynamic[1]) { - dir = path.resolve(dir, words[1].replace(/^~(?=\/|$)/, os.homedir())); - continue; - } - if (name === "cat" && cmd.stdin !== null) { - for (const r of cmd.redirects) { - if (r.op === ">" || r.op === ">>") written.set(path.resolve(dir, r.target), cmd.stdin); - } - } - if (name === "git" && subAgent) gitRules(words); - if (name === "gh") ghRules(words, dynamic, cmd, dir, written); - slowLaneRules(words); + if (words.length > 0 && !changesDirectory(state, words, dynamic)) + commandStep(state, cmd, words, dynamic); + } +} + +function changesDirectory(state, words, dynamic) { + if (path.basename(words[0]) !== "cd" || words[1] === undefined || dynamic[1]) return false; + state.dir = path.resolve(state.dir, words[1].replace(/^~(?=\/|$)/, os.homedir())); + return true; +} + +function commandStep(state, cmd, words, dynamic) { + const name = path.basename(words[0]); + if (name === "cat" && cmd.stdin !== null) trackWrites(state, cmd); + if (name === "git" && subAgent) gitRules(words); + if (name === "gh") ghRules({ words, dynamic, cmd, ...state, group: words[1], verb: words[2] }); + slowLaneRules(words); +} + +// why: a body file written by a heredoc earlier in the same command is not on disk yet. +function trackWrites(state, cmd) { + for (const r of cmd.redirects) { + if (r.op === ">" || r.op === ">>") + state.written.set(path.resolve(state.dir, r.target), cmd.stdin); } } +const GIT_RULES = [ + { + when: (g) => g.sub === "stash", + reason: + "Refused: never `git stash` in a delivery stage: every worktree shares one stash list, so " + + "a pop can take another agent's changes. Commit the work instead.", + }, + { + when: (g) => + g.sub === "rebase" || + (g.sub === "pull" && g.rest.some((w) => /^(?:-r|--rebase(?!=false))/.test(w))), + reason: + "Refused: never rebase in a delivery stage: a stack moves by merging forward " + + "(`.agents/scripts/stack-sync.sh`), and a rebased branch needs a force push nobody may " + + "make (delivery rule 15). Merge the base branch instead: `git merge origin/`.", + }, + { + when: (g) => g.sub === "push" && g.rest.some(rewritesRemote), + reason: + "Refused: this push rewrites or deletes a remote branch (`+`, `:`, `--force` or " + + "`--delete`). Push fast-forwards only; when the push is rejected, merge the remote " + + "branch in and push again.", + }, +]; + function gitRules(words) { let at = 1; while (at < words.length && words[at].startsWith("-")) at += GIT_VALUE_FLAGS.has(words[at]) ? 2 : 1; - const sub = words[at]; - const rest = words.slice(at + 1); - if (sub === "stash") { - refuse( - "Refused: never `git stash` in a delivery stage: every worktree shares one stash list, so " + - "a pop can take another agent's changes. Commit the work instead.", - ); - } - if ( - sub === "rebase" || - (sub === "pull" && rest.some((w) => /^(?:-r|--rebase(?!=false))/.test(w))) - ) { - refuse( - "Refused: never rebase in a delivery stage: a stack moves by merging forward " + - "(`.agents/scripts/stack-sync.sh`), and a rebased branch needs a force push nobody may " + - "make (delivery rule 15). Merge the base branch instead: `git merge origin/`.", - ); - } - if ( - sub === "push" && - rest.some( - (w) => - w.startsWith("+") || - /^:./.test(w) || - /^--(?:force|delete|mirror)/.test(w) || - /^-[a-zA-Z]*[fd]/.test(w), - ) - ) { - refuse( - "Refused: this push rewrites or deletes a remote branch (`+`, `:`, `--force` or " + - "`--delete`). Push fast-forwards only; when the push is rejected, merge the remote " + - "branch in and push again.", - ); - } + refuseFirst(GIT_RULES, { sub: words[at], rest: words.slice(at + 1) }); } -function ghRules(words, dynamic, cmd, dir, written) { - const [, group, verb] = words; - if (group === "issue" && verb === "edit" && flagValues(words, ["--add-assignee"]).length > 0) { - refuse( +function rewritesRemote(word) { + return ( + word.startsWith("+") || + /^:./.test(word) || + /^--(?:force|delete|mirror)/.test(word) || + /^-[a-zA-Z]*[fd]/.test(word) + ); +} + +const GH_RULES = [ + { + when: (c) => + c.group === "issue" && + c.verb === "edit" && + flagValues(c.words, ["--add-assignee"]).length > 0, + reason: "Refused: claim an issue with `node .agents/scripts/claim.mjs ` (`--triage` for a " + - "`bug:new` triage), not by hand. It assigns you only when the issue is open, unassigned " + - "and labelled as work an agent may take (delivery rules 1 and 2).", - ); - } - if (subAgent) { - const label = addedLabels(words, group, verb).find((l) => WORK_LABELS.has(l)); - if (label !== undefined) { - refuse( - `Refused: agents never label their way into work (delivery rule 1): \`${label}\` is the ` + - "maintainer's or the spec session's to set. Leave the label as it is and say in your " + - "report what should change.", - ); - } - if (group === "pr" && verb === "ready") { - refuse( - "Refused: only the orchestrator marks a PR ready, after its review (delivery rule 14). " + - "Leave it in draft and report.", - ); - } - if (group === "pr" && verb === "create") { - if (!words.includes("--draft") && !words.includes("-d")) { - refuse( - "Refused: every PR starts in draft (delivery rule 14). Run `gh pr create` again with " + - "`--draft`; the orchestrator marks it ready after review.", - ); - } - const head = flagValues(words, ["--head", "-H"]).at(-1) ?? currentBranch(dir); - if (head.endsWith("-repro")) { - refuse( - "Refused: a `bug/-repro` branch opens no pull request (delivery rule 8). Push the " + - "test to the branch and name it in the triage report.", - ); - } - } - if (group === "api" && /(?:^|\/)repos\/[^/]+\/[^/]+\/pulls$/.test(apiPath(words))) { - const fields = apiFields(words); - const post = - flagValues(words, ["-X", "--method"]).at(-1)?.toUpperCase() ?? - (fields.length > 0 ? "POST" : "GET"); - if (post === "POST" && !fields.some((f) => f.key === "draft" && f.value === "true")) { - refuse( - "Refused: every PR starts in draft (delivery rule 14). Create it again with " + - "`-F draft=true`; the orchestrator marks it ready after review.", - ); - } - } - } - const body = postedBody(words, dynamic, cmd, dir, written); - if (body === null) return; - const defects = bodyDefects(body); + "`bug:new` triage), not by hand. It assigns you only when the issue is open, unassigned " + + "and labelled as work an agent may take (delivery rules 1 and 2).", + }, + { + when: (c) => subAgent && workLabel(c) !== undefined, + reason: (c) => + `Refused: agents never label their way into work (delivery rule 1): \`${workLabel(c)}\` is the ` + + "maintainer's or the spec session's to set. Leave the label as it is and say in your " + + "report what should change.", + }, + { + when: (c) => subAgent && c.group === "pr" && c.verb === "ready", + reason: + "Refused: only the orchestrator marks a PR ready, after its review (delivery rule 14). " + + "Leave it in draft and report.", + }, + { + when: (c) => subAgent && prCreate(c) && !c.words.includes("--draft") && !c.words.includes("-d"), + reason: + "Refused: every PR starts in draft (delivery rule 14). Run `gh pr create` again with " + + "`--draft`; the orchestrator marks it ready after review.", + }, + { + when: (c) => subAgent && prCreate(c) && prHead(c).endsWith("-repro"), + reason: + "Refused: a `bug/-repro` branch opens no pull request (delivery rule 8). Push the " + + "test to the branch and name it in the triage report.", + }, + { + when: (c) => subAgent && restCreatesReadyPr(c.words), + reason: + "Refused: every PR starts in draft (delivery rule 14). Create it again with " + + "`-F draft=true`; the orchestrator marks it ready after review.", + }, +]; + +function ghRules(c) { + refuseFirst(GH_RULES, c); + const body = postedBody(c); + const defects = body === null ? [] : bodyDefects(body); if (defects.length > 0) { const reason = defects.join(" "); refuse(`Refused: ${reason[0].toLowerCase()}${reason.slice(1)} Fix the body and post again.`); } } +function workLabel(c) { + return addedLabels(c.words, c.group, c.verb).find((l) => WORK_LABELS.has(l)); +} + +function prCreate(c) { + return c.group === "pr" && c.verb === "create"; +} + +function prHead(c) { + return flagValues(c.words, ["--head", "-H"]).at(-1) ?? currentBranch(c.dir); +} + +function restCreatesReadyPr(words) { + if (words[1] !== "api" || !/(?:^|\/)repos\/[^/]+\/[^/]+\/pulls$/.test(apiPath(words))) + return false; + const fields = apiFields(words); + const method = flagValues(words, ["-X", "--method"]).at(-1)?.toUpperCase(); + const post = (method ?? (fields.length > 0 ? "POST" : "GET")) === "POST"; + return post && !fields.some((f) => f.key === "draft" && f.value === "true"); +} + +const BODY_VERBS = ["comment", "create", "edit", "close"]; +const FIELD_FLAGS = ["-f", "-F", "--field", "--raw-field"]; + // why: a body is null when the command posts none; an unreadable one refuses here. -function postedBody(words, dynamic, cmd, dir, written) { - const [, group, verb] = words; - const unreadable = () => - refuse( - "Refused: the guard cannot read a body built by `$(...)`, a backtick or a variable, so it " + - "cannot check its shape (delivery rule 12). Write the body to a file and pass " + - "`--body-file ` (`-F body=@` for `gh api`).", - ); - const fromFile = (file) => { - if (file === "-") { - if (cmd.stdin === null || cmd.stdinDynamic) unreadable(); - return cmd.stdin; - } - const at = path.resolve(dir, file.replace(/^~(?=\/|$)/, os.homedir())); - if (written.has(at)) return written.get(at); - try { - return fs.readFileSync(at, "utf8"); - } catch { - return refuse( - `Refused: \`${file}\` does not exist yet, so the guard cannot check the body. Write the ` + - "file first, then post it in its own command.", - ); - } - }; - if ( - (group === "issue" || group === "pr") && - ["comment", "create", "edit", "close"].includes(verb) - ) { - const inline = verb === "close" ? ["--comment", "-c"] : ["--body", "-b"]; - for (let i = 3; i < words.length; i++) { - const [flag, value, valueAt] = splitFlag(words, i); - if (inline.includes(flag)) { - if (dynamic[valueAt]) unreadable(); - return value ?? ""; - } - if (verb !== "close" && (flag === "--body-file" || flag === "-F")) { - if (dynamic[valueAt]) unreadable(); - return fromFile(value ?? ""); - } - } - return null; +function postedBody(c) { + if ((c.group === "issue" || c.group === "pr") && BODY_VERBS.includes(c.verb)) return issueBody(c); + if (c.group === "api") return apiBody(c); + return null; +} + +function issueBody(c) { + const inline = c.verb === "close" ? ["--comment", "-c"] : ["--body", "-b"]; + const files = c.verb === "close" ? [] : ["--body-file", "-F"]; + for (let i = 3; i < c.words.length; i++) { + const [flag, value, valueAt] = splitFlag(c.words, i); + if (inline.includes(flag)) return readable(c, valueAt, value ?? ""); + if (files.includes(flag)) return bodyFile(c, readable(c, valueAt, value ?? "")); } - if (group === "api") { - for (let i = 2; i < words.length; i++) { - const [flag, value, valueAt] = splitFlag(words, i); - if (flag === "--input" && value !== undefined) { - if (dynamic[valueAt]) unreadable(); - try { - const json = JSON.parse(fromFile(value)); - if (typeof json?.body === "string") return json.body; - } catch { - return null; - } - } - if ( - !["-f", "-F", "--field", "--raw-field"].includes(flag) || - !(value ?? "").startsWith("body=") - ) - continue; - if (dynamic[valueAt]) unreadable(); - const text = value.slice("body=".length); - return (flag === "-F" || flag === "--field") && text.startsWith("@") - ? fromFile(text.slice(1)) - : text; - } + return null; +} + +function apiBody(c) { + for (let i = 2; i < c.words.length; i++) { + const [flag, value, valueAt] = splitFlag(c.words, i); + const input = + flag === "--input" && value !== undefined ? inputBody(c, value, valueAt) : undefined; + if (input !== undefined) return input; + if (FIELD_FLAGS.includes(flag) && (value ?? "").startsWith("body=")) + return fieldBody(c, flag, value, valueAt); } return null; } -function slowLaneRules(words) { - const name = path.basename(words[0]); - const runner = words.findIndex((w) => path.basename(w) === "vitest"); - if ( - runner !== -1 && - (runner === 0 || - (["pnpm", "npx", "bunx", "yarn", "npm"].includes(name) && - words - .slice(1, runner) - .every((w) => ["exec", "dlx", "x", "run"].includes(w) || w.startsWith("-")))) - ) { - const after = words.slice(runner + 1); - const slowFile = after.some((w) => /(?:^|\/)e2e\/slow-[^/]*$/.test(w)); - const e2e = after.some( - (w, i) => w === "--project=e2e" || (w === "--project" && after[i + 1] === "e2e"), - ); - const filtered = flagValues(after, ["--tags-filter"], 0).some((v) => v.includes("!slow")); - if (slowFile || (e2e && !filtered)) { - refuse( - "Refused: the slow lane runs only through `scripts/slow-e2e.sh `, which holds " + - "the machine-wide lock (delivery rule 16). Run the fast e2e lane with `pnpm test:e2e " + - "`, and leave the slow lane to the hardware verifier.", - ); - } +// why: undefined goes on to the next word, null stops at a file that holds no JSON. +function inputBody(c, value, valueAt) { + try { + const json = JSON.parse(bodyFile(c, readable(c, valueAt, value))); + return typeof json?.body === "string" ? json.body : undefined; + } catch { + return null; } - if ( - subAgent && - name === "pnpm" && - SLOW_LANE_SCRIPTS.has(pnpmOperands(words.slice(1)).find((w) => w !== "run")) - ) { - refuse( - "Refused: the slow lane runs only through `scripts/slow-e2e.sh `, which holds " + - "the machine-wide lock (delivery rule 16). Run it through the script, or report the " + - "lines that need it under `hardware`.", - ); +} + +function fieldBody(c, flag, value, valueAt) { + const text = readable(c, valueAt, value).slice("body=".length); + const file = (flag === "-F" || flag === "--field") && text.startsWith("@"); + return file ? bodyFile(c, text.slice(1)) : text; +} + +function readable(c, valueAt, value) { + if (c.dynamic[valueAt]) unreadable(); + return value; +} + +function unreadable() { + refuse( + "Refused: the guard cannot read a body built by `$(...)`, a backtick or a variable, so it " + + "cannot check its shape (delivery rule 12). Write the body to a file and pass " + + "`--body-file ` (`-F body=@` for `gh api`).", + ); +} + +function bodyFile(c, file) { + if (file === "-") { + if (c.cmd.stdin === null || c.cmd.stdinDynamic) unreadable(); + return c.cmd.stdin; } - const script = words.findIndex((w) => path.basename(w) === "slow-e2e.sh"); - if ( - script !== -1 && - (script === 0 || ["sh", "bash", "zsh"].includes(name)) && - words[script + 1] !== "status" && - typeof input.agent_type === "string" && - !SLOW_LANE_AGENTS.has(input.agent_type) - ) { - refuse( - "Refused: only the hardware verifier and the implementer run the slow lane (delivery rule " + - "16); `scripts/slow-e2e.sh status` is open to every agent. Report what needs the lane " + - "instead.", + const at = path.resolve(c.dir, file.replace(/^~(?=\/|$)/, os.homedir())); + if (c.written.has(at)) return c.written.get(at); + try { + return fs.readFileSync(at, "utf8"); + } catch { + return refuse( + `Refused: \`${file}\` does not exist yet, so the guard cannot check the body. Write the ` + + "file first, then post it in its own command.", ); } } +const SLOW_LANE_RULES = [ + { + when: (words) => vitestRunsSlowLane(words), + reason: + "Refused: the slow lane runs only through `scripts/slow-e2e.sh `, which holds " + + "the machine-wide lock (delivery rule 16). Run the fast e2e lane with `pnpm test:e2e " + + "`, and leave the slow lane to the hardware verifier.", + }, + { + when: (words) => + subAgent && + path.basename(words[0]) === "pnpm" && + SLOW_LANE_SCRIPTS.has(pnpmOperands(words.slice(1)).find((w) => w !== "run")), + reason: + "Refused: the slow lane runs only through `scripts/slow-e2e.sh `, which holds " + + "the machine-wide lock (delivery rule 16). Run it through the script, or report the " + + "lines that need it under `hardware`.", + }, + { + when: (words) => startsSlowLane(words) && !SLOW_LANE_AGENTS.has(input.agent_type ?? ""), + reason: + "Refused: only the hardware verifier and the implementer run the slow lane (delivery rule " + + "16); `scripts/slow-e2e.sh status` is open to every agent. Report what needs the lane " + + "instead.", + }, +]; + +function slowLaneRules(words) { + refuseFirst(SLOW_LANE_RULES, words); +} + +function vitestRunsSlowLane(words) { + const runner = words.findIndex((w) => path.basename(w) === "vitest"); + if (runner === -1 || !runsAsCommand(words, runner)) return false; + const after = words.slice(runner + 1); + const slowFile = after.some((w) => /(?:^|\/)e2e\/slow-[^/]*$/.test(w)); + const e2e = after.some( + (w, i) => w === "--project=e2e" || (w === "--project" && after[i + 1] === "e2e"), + ); + const filtered = flagValues(after, ["--tags-filter"], 0).some((v) => v.includes("!slow")); + return slowFile || (e2e && !filtered); +} + +// why: `vitest` as the program, or after `pnpm exec`, `npx` and the like; not as an operand. +function runsAsCommand(words, runner) { + if (runner === 0) return true; + const launcher = ["pnpm", "npx", "bunx", "yarn", "npm"].includes(path.basename(words[0])); + return ( + launcher && + words + .slice(1, runner) + .every((w) => ["exec", "dlx", "x", "run"].includes(w) || w.startsWith("-")) + ); +} + +// why: the main session has no `agent_type`, and it may start the lane. +function startsSlowLane(words) { + const script = words.findIndex((w) => path.basename(w) === "slow-e2e.sh"); + if (script === -1 || typeof input.agent_type !== "string") return false; + const shell = script === 0 || ["sh", "bash", "zsh"].includes(path.basename(words[0])); + return shell && words[script + 1] !== "status"; +} + function addedLabels(words, group, verb) { let values = []; if ((group === "issue" || group === "pr") && verb === "edit") @@ -658,3 +656,64 @@ function firstOperand(words) { function unquote(word) { return word.replace(/^['"]|['"]$/g, ""); } + +// why: the rule tables above are consts, so the hook runs once they all exist. +const input = JSON.parse(fs.readFileSync(0, "utf8") || "{}"); +const tool = input.tool_name; +const args = input.tool_input ?? {}; +const subAgent = typeof input.agent_id === "string"; +const cwd = typeof input.cwd === "string" ? input.cwd : process.cwd(); + +if (tool === "Agent" || tool === "Task") agentRules(); + +if (tool === "Bash" && subAgent) { + const shell = code(args.command ?? ""); + if (HOOK_BYPASS.some((re) => re.test(shell))) { + refuse( + "Refused: this skips the git hooks. They run the checks agents never run by hand. Only the " + + "build stage may skip Fallow, on a push before its last cycle: " + + "`LEFTHOOK_EXCLUDE=fallow git push ...`.", + ); + } + const raw = args.command ?? ""; + const slowLane = /slow-e2e\.sh|\.exit\b/.test(raw); + if (!slowLane) { + if (TEST_RUNNER.test(shell) && !(Number(args.timeout) >= 300000)) { + refuse( + "Refused: a test run needs `timeout: 600000` on this Bash call. The default 120 s " + + "moves a two-minute run to the background, and a background job holds your report " + + "until it ends. Run it again in the foreground with the timeout set.", + ); + } + if (args.run_in_background === true || BACKGROUND_JOB.test(shell)) { + refuse( + "Refused: nothing runs in the background in a delivery stage. A background job holds " + + "your report until it ends or hits its 30-minute cap, and polling it costs the turn. " + + "Run the command in the foreground with `timeout: 600000` on the Bash call.", + ); + } + if (SLEEP_LOOP.test(shell)) { + refuse( + "Refused: no loop that sleeps. Nothing you started runs in the background, so there is " + + "nothing to wait for: run the command itself in the foreground with `timeout: 600000`.", + ); + } + if (BARE_CAT.test(raw)) { + refuse( + "Refused: `cat > file` with no input reads stdin until the tool times out. Write the " + + "file with the Write tool, or give cat a heredoc (`cat > file <<'EOF'`).", + ); + } + } + const found = forbiddenCheck(shell); + if (found !== null) { + refuse( + `Refused: \`${found}\` is under "Checks agents never run" in toolchain.md. The commit ` + + "hook runs lint, the comment lint and typecheck; the push hook runs Fallow and the e2e " + + "typecheck; CI runs the rest. Their errors show in the commit or push output. Run only " + + 'the commands under "Tests agents run".', + ); + } +} +if (tool === "Bash") commandRules(args.command ?? ""); +process.exit(0); From ec051148a25420ceed86aea417fa3cdeaec5fe6d Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Sun, 25 Oct 2026 10:18:23 +0100 Subject: [PATCH 12/15] test(checks): the guard tests keep the waiting rules' cases after the rebase onto the test lint --- src/tool-guard.test.ts | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/tool-guard.test.ts b/src/tool-guard.test.ts index b2181f638..36f78b079 100644 --- a/src/tool-guard.test.ts +++ b/src/tool-guard.test.ts @@ -98,6 +98,37 @@ describe("tool-guard.mjs", () => { expect(bash(command)).toEqual({ status: 0, stderr: "" }); }); + it.each([ + ["pnpm exec vitest run --project unit src/a.test.ts", {}, "timeout: 600000"], + ["pnpm test:changed", { timeout: 120_000 }, "timeout: 600000"], + ["ls", { run_in_background: true, timeout: 600_000 }, "background"], + ["(git push origin x > log 2>&1; echo done) &", { timeout: 600_000 }, "background"], + ["until ! pgrep -f lefthook >/dev/null; do sleep 5; done; tail x", {}, "loop that sleeps"], + ["for _ in $(seq 18); do [ -f out ] && break; sleep 30; done", {}, "loop that sleeps"], + ["cat > /tmp/x 2>/dev/null; mkdir -p .agents/scratch", {}, "no input"], + ["cat > ../pr-911.md", {}, "no input"], + ])("refuses a sub-agent's wait or background job: %s", (command, input, reason) => { + const result = call({ + tool_name: "Bash", + tool_input: { command, ...input }, + agent_id: "a1", + agent_type: "implementer", + }); + + expect(result.status).toBe(2); + expect(result.stderr).toContain(reason); + }); + + it.each([ + "ls vitest* ui/vitest.config.ts", + "cat vitest.config.ts | head", + 'for _ in $(seq 18); do [ -f "$log.exit" ] && break; sleep 30; done; cat "$log.exit"', + "cat > ../pr-911.md <<'EOF'\nCloses #1\nEOF\ngit status", + "python3 - <<'EOF'\nfor i in range(3):\n time.sleep(1)\nEOF", + ])("lets a sub-agent run a foreground command, the slow lane wait included: %s", (command) => { + expect(bash(command, "a1", "implementer")).toEqual(OK); + }); + it.each([ "git push --no-verify -u origin task/1", "git commit -q --no-verify -m x", From 0620c1c3752c421941aabdc1942ac8969d79045b Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Sun, 25 Oct 2026 10:21:52 +0100 Subject: [PATCH 13/15] test(checks): the claim script's cases get half a minute, since each spawns node and a fake gh --- src/claim.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/claim.test.ts b/src/claim.test.ts index cec45eff0..d70bc1cc6 100644 --- a/src/claim.test.ts +++ b/src/claim.test.ts @@ -78,7 +78,9 @@ const task = (body: string, extra: Issue = {}): Issue => ({ ...extra, }); -describe("claim.mjs", () => { +// why: each case spawns node and a fake gh several times; under a loaded suite run the default +// five seconds is not enough, and a hang still fails within the half minute. +describe("claim.mjs", { timeout: 30_000 }, () => { it.each([ ["a ready bug", { 5: { state: "open", labels: ["bug:ready"] } }, []], ["a bug:new issue to triage", { 5: { state: "open", labels: ["bug:new"] } }, ["--triage"]], From 082943adc9b826d7e56ed932caf69c667dae99ea Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Sun, 25 Oct 2026 10:56:43 +0100 Subject: [PATCH 14/15] chore(checks): the lens-routing failure of the body check applies to delivery and routine PRs, not a person's chore PR --- .agents/scripts/pr-body-check.mjs | 10 +++++++++- src/pr-body-check.test.ts | 10 +++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/.agents/scripts/pr-body-check.mjs b/.agents/scripts/pr-body-check.mjs index 15d42f4fb..873a4cf02 100755 --- a/.agents/scripts/pr-body-check.mjs +++ b/.agents/scripts/pr-body-check.mjs @@ -362,7 +362,7 @@ function checkKind(kind, counts) { if (routine !== undefined && kind !== routine[1]) { failures.push(`this routine's PR must be ${routine[1]}, and its diff is ${kind}`); } - if (/\bCode review: skipped\./.test(counts) && ["full", "large", "tests-only"].includes(kind)) { + if (skipsNeededCodeReview(kind, counts, routine)) { failures.push(`Code review is skipped on a ${kind} diff (REVIEW.md, Routing)`); } if (kind === "adr-only" && counts !== "" && !/\bClaims review: skipped\./.test(counts)) { @@ -370,6 +370,14 @@ function checkKind(kind, counts) { } } +// why: the review protocol routes a delivery PR's and a routine's lenses; a person's chore PR +// has no review round to route, and merges on the maintainer's say. +function skipsNeededCodeReview(kind, counts, routine) { + const reviewed = DELIVERY_BRANCH.test(branch) || routine !== undefined; + const needsCode = ["full", "large", "tests-only"].includes(kind); + return reviewed && needsCode && /\bCode review: skipped\./.test(counts); +} + function bullets(text, heading) { const all = withoutCodeBlocks(text.replace(/\r/g, "")).split("\n"); const out = []; diff --git a/src/pr-body-check.test.ts b/src/pr-body-check.test.ts index 775f99f6f..5ad1c7765 100644 --- a/src/pr-body-check.test.ts +++ b/src/pr-body-check.test.ts @@ -359,10 +359,14 @@ describe("pr-body-check.mjs", { timeout: 20_000 }, () => { const ran = "0 blocking, 0 fixed, 0 notes"; expect(check(counts(ran, "skipped", ran), "--kind", "docs-only").status).toBe(0); + expect( + check(counts("skipped", "skipped", "skipped"), "--branch", "chore/lints", "--kind", "full") + .status, + ).toBe(0); for (const kind of ["full", "large", "tests-only"]) { - expect(check(counts(ran, "skipped", ran), "--kind", kind).lines).toEqual([ - expect.stringContaining(`Code review is skipped on a ${kind} diff`), - ]); + expect( + check(counts(ran, "skipped", ran), "--branch", "task/12", "--kind", kind).lines, + ).toEqual([expect.stringContaining(`Code review is skipped on a ${kind} diff`)]); } expect( check(counts("skipped", ran, ran), "--branch", "task/12", "--kind", "full").lines, From c6fa1c71dd4e48cccd8f7e5a9e5d10cd43c04740 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Sun, 25 Oct 2026 11:16:06 +0100 Subject: [PATCH 15/15] test(checks): the body check's ancestry case reads two commits of its own, since CI's unit job checks out one commit deep --- src/pr-body-check.test.ts | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/src/pr-body-check.test.ts b/src/pr-body-check.test.ts index 5ad1c7765..2996ea0c5 100644 --- a/src/pr-body-check.test.ts +++ b/src/pr-body-check.test.ts @@ -35,6 +35,9 @@ function check(body: string, ...args: string[]) { interface GitHub { readonly origin?: boolean; + // why: the directory the script's git commands run in; a case about commit ancestry gives a + // scratch repository, since CI's unit job checks the repo out one commit deep. + readonly cwd?: string; readonly comments?: readonly string[]; // why: what each other `repos/o/r/` answers; null is GitHub's 404. readonly routes?: Readonly>; @@ -73,7 +76,7 @@ process.stdout.write(JSON.stringify(routes[key])); GH_REPO: "o/r", GITHUB_ACTIONS: "", }; - let cwd: string | undefined; + let cwd = options.cwd; if (options.origin === true) { delete (env as { GH_REPO?: string }).GH_REPO; cwd = join(dir, "clone"); @@ -92,6 +95,25 @@ process.stdout.write(JSON.stringify(routes[key])); // why: an issue with no Done when, no parent and no stack, for cases about other rules. const LONE_ISSUE = { "issues/7": { body: "" }, "issues/7/parent": null }; +// why: two commits of our own, so a case about ancestry does not read this clone's history, +// which CI's unit job fetches one commit deep. +function scratchRepo(): { dir: string; head: string; older: string } { + const dir = mkdtempSync(join(tmpdir(), "pr-body-repo-")); + const git = (...args: string[]) => + execFileSync("git", ["-c", "user.name=t", "-c", "user.email=t@t", ...args], { + cwd: dir, + encoding: "utf8", + }).trim(); + git("init", "-q"); + writeFileSync(join(dir, "a"), "1\n"); + git("add", "a"); + git("commit", "-qm", "one"); + const older = git("rev-parse", "HEAD"); + writeFileSync(join(dir, "a"), "2\n"); + git("commit", "-qam", "two"); + return { dir, head: git("rev-parse", "HEAD"), older }; +} + const valid = [ "Closes #12", "", @@ -227,14 +249,13 @@ describe("pr-body-check.mjs", { timeout: 20_000 }, () => { }); it("fails an open finding, a malformed or repeated round, a round off the head, and two notes comments", () => { - const head = execFileSync("git", ["rev-parse", "HEAD"], { encoding: "utf8" }).trim(); - const older = execFileSync("git", ["rev-parse", "HEAD~1"], { encoding: "utf8" }).trim(); + const { dir, head, older } = scratchRepo(); const ledger = (rounds: string, findings = "[]") => `## Review ledger\n\n${rounds}\n\`\`\`json\n{"findings": ${findings}}\n\`\`\``; const task = (...comments: string[]) => fromGitHub( { body: valid, head: { ref: "task/7", sha: older } }, - { comments, routes: LONE_ISSUE }, + { comments, routes: LONE_ISSUE, cwd: dir }, ); expect(task(ledger(`Round 1: ${older}\nRound fix-check: ${older}`))).toEqual({