From ff055df2f8d42138f44cdfec4a0645b4ba7a1ed6 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 19 Oct 2026 19:05:05 +0200 Subject: [PATCH 01/15] =?UTF-8?q?chore(agents):=20measured=20review=20and?= =?UTF-8?q?=20test=20strategy=20=E2=80=94=20mutation=20by=20tool,=20a=20se?= =?UTF-8?q?lf-filling=20ledger,=20fewer=20notes=20and=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - pnpm mutate runs a near pass (tests that import a changed file, through their module's barrel, or that the branch changed), then a wide related pass only for survivors: 122-273 s down to 16-21 s on #682, #669, #674. - The implementer's audit runs pnpm mutate instead of probing every test by hand; survivors and reasons go under ## Mutants, which the code review checks. The tool guard lets only the implementer run it. - review-metrics.mjs reads usage and cost from Claude Code transcripts, found by a ledger tag line every stage and lens prompt starts with; reproduces #683's recorded costs to the cent. - Lenses write notes only for an unproven secret leak or data loss, and at most 400 words; the spec review leaves test strength and unnamed behaviour to the code review. - Testing rules: a spec Tests line is one test, a claim is proved at one seam, a review fix that adds a test names the one it could not extend. - New weekly routine recurring-findings: classes fixed in 3+ PRs become draft tasks for a check or rule line. --- .agents/briefs/implement-audit.md | 20 +- .agents/briefs/implement-fix.md | 3 +- .agents/briefs/implement-red.md | 4 +- .agents/briefs/review-claims.md | 9 +- .agents/briefs/review-code.md | 48 ++-- .agents/briefs/review-spec.md | 21 +- .agents/scripts/recurring-findings.mjs | 104 ++++++++ .agents/scripts/report-guard.mjs | 24 +- .agents/scripts/review-inputs.sh | 11 + .agents/scripts/review-metrics.mjs | 275 +++++++++++++++----- .agents/scripts/tool-guard.mjs | 14 +- .claude/agents/implementer.md | 2 +- .claude/agents/reviewer.md | 14 +- .claude/skills/calibrate-reviews/SKILL.md | 2 +- .claude/skills/deliver/SKILL.md | 12 +- .claude/skills/implement/SKILL.md | 2 +- .claude/skills/recurring-findings/SKILL.md | 54 ++++ .claude/skills/review/SKILL.md | 2 +- .claude/skills/verify-hardware/SKILL.md | 2 +- docs/internal/REVIEW.md | 39 ++- docs/internal/agent-rules/testing.md | 16 +- docs/internal/agent-rules/toolchain.md | 49 +++- docs/internal/templates/review-metrics.json | 20 -- scripts/mutate.mjs | 164 +++++++++--- src/mutate.test.ts | 105 ++++++-- src/recurring-findings.test.ts | 103 ++++++++ src/report-guard.test.ts | 23 +- src/review-inputs.test.ts | 21 +- src/review-metrics.test.ts | 141 ++++++++++ src/tool-guard.test.ts | 16 +- 30 files changed, 1048 insertions(+), 272 deletions(-) create mode 100644 .agents/scripts/recurring-findings.mjs create mode 100644 .claude/skills/recurring-findings/SKILL.md delete mode 100644 docs/internal/templates/review-metrics.json create mode 100644 src/recurring-findings.test.ts create mode 100644 src/review-metrics.test.ts diff --git a/.agents/briefs/implement-audit.md b/.agents/briefs/implement-audit.md index d62b3d708..87bae7852 100644 --- a/.agents/briefs/implement-audit.md +++ b/.agents/briefs/implement-audit.md @@ -39,14 +39,22 @@ edited after the red commit: - It proves a claim no other test proves, through the module's surface (testing rules 6 and 7). One that repeats another test's claim is folded into that test. -- Break the code it covers once (flip the condition, drop the filter, return - early) and run its file: it fails on the assertion its title names. - Restore the code. A test that stays green is vacuous: fix it. Every - test, no skipping: a probe the audit skips, a reviewer does on a far - costlier model. Report `probed

/` on the `audit:` line: `t` tests - added or changed, `p` probed. - A test comparing two outputs also asserts one of them against a literal. +**Mutate.** Run `pnpm mutate > /mutants.txt 2>&1` +(after fixes: the same command; it reuses every unchanged result). Exit 2: +`Assessment: incomplete`. For each mutant it lists alive, either kill it +(extend the test that proves the behaviour next to it, testing rule 7) or +write one line on why it changes nothing a caller, user, stored record or +event can see; in a strict area (`toolchain.md`) only a log line may stay +alive. Then put the result in the PR body under `## Mutants`: the summary +line, and per mutant still alive its `path:line`, mutator and reason. +Report `mutants alive explained ` on the `audit:` line. +When the script finds no changed source line (a tests-only diff), report +`mutants none` and instead break the code each added or changed test covers +once, run its file, see it fail on the assertion its title names, and +restore the code. + Run the "Project checks" in `toolchain.md` that the diff triggers. Then the tests from mode `green` pass, and you push. diff --git a/.agents/briefs/implement-fix.md b/.agents/briefs/implement-fix.md index 88b978b4e..27e12101a 100644 --- a/.agents/briefs/implement-fix.md +++ b/.agents/briefs/implement-fix.md @@ -12,7 +12,8 @@ mechanical. Inputs include immutable `base`, `merge-base`, `previous` and mode and order, including both at once) and check each. 2. **Fix every instance:** - behaviour: make a test fail first, then fix. Extend the test that - proves the behaviour next to it; write a new one only when none does + proves the behaviour next to it; write a new one only when none does, + and list it under `fixes` with the test you could not extend and why (testing rule 9); - a broken rule (`[architecture N]`, `[safety N]`, ...): change the code so it keeps the rule, never the doc that states the rule; diff --git a/.agents/briefs/implement-red.md b/.agents/briefs/implement-red.md index 7cd483af2..1ac413676 100644 --- a/.agents/briefs/implement-red.md +++ b/.agents/briefs/implement-red.md @@ -1,7 +1,9 @@ # Mode `red`: the spec's tests first Write one test per line under Tests (a bug's triage test already exists), -or extend the existing test the line names (testing rule 7). Each title is +or extend the existing test the line names (testing rule 7). A line that +lists cases is still one test, its cases a table in its body; a line names +one seam, and its claim is tested there only. Each title is the spec's claim, word for word where it fits. Before you run them, read each against testing rules 6, 8 and 10: it asserts on the module's output, not its internals; no log line the user docs do not describe; none of the diff --git a/.agents/briefs/review-claims.md b/.agents/briefs/review-claims.md index bb73cb9ee..4427729c2 100644 --- a/.agents/briefs/review-claims.md +++ b/.agents/briefs/review-claims.md @@ -31,7 +31,8 @@ Whether a comment may exist at all is the commit hook's (rule 17 in A claim is false when the code at this commit contradicts it: a name or path that no longer exists, a behaviour, order, owner, default or limit that -changed. Blocking: a false claim. Note: a claim that is true but misleading. +changed. Blocking: a false claim. A claim that is true but misleading is +no finding. ## Assessment @@ -44,8 +45,10 @@ already supported remain valid; identify unresolved coverage separately. `F0` is a good result: do not look for findings to have some. Output only your list, then your findings: no preamble, no headings, no summary, no -narration. Report every blocking finding; there is no limit. At most five -notes; write no other note. One line per defect, in this form and no other: +narration, at most 400 words besides the list. Report every blocking +finding; there is no limit. Write a note only for a suspected leak of a +secret or a credential that you could not prove: at most two. One line per +defect, in this form and no other: ``` F B|N [architecture 13] ev: diff --git a/.agents/briefs/review-code.md b/.agents/briefs/review-code.md index 30a7740c2..5efa07d64 100644 --- a/.agents/briefs/review-code.md +++ b/.agents/briefs/review-code.md @@ -7,9 +7,11 @@ the code. In the inputs directory: the specification (`issue.md`, `feature.md`, `triage.md` and `spec/`, those that exist), so you know what the code is meant to do; `rules/`, every rule file but `delivery.md`, and the ADR index; -`diff.patch`; `briefs/calibration.md`, when it exists, findings like yours that -were rejected lately: read it first. Repository: the worktree you were -given, checked out at the reviewed commit. +`diff.patch`; `mutants.md`, when it exists, what `pnpm mutate` left alive on +the changed lines and the implementer's reason for each; +`briefs/calibration.md`, when it exists, findings like yours that were +rejected lately: read it first. Repository: the worktree you were given, +checked out at the reviewed commit. A conflict between the specification and a rule is a finding of class `spec vs rule`; never resolve it silently. A defect on a line the diff does @@ -23,8 +25,9 @@ under "Checks agents never run", and never the slow lane. You may break code to see what a test catches: one edit, one run of its test file, then `git checkout .`. You may also write one new test file in the worktree to prove an interleaving or an input the suite does not cover, run it, and -delete it. Probe budget: `lens: behaviour` four probes, `lens: tests` six, -`lens: both` nine. Restore the tree before you report. +delete it. Probe budget: four, for behaviour; the tests lens gets four +more only when there is no `mutants.md` (a tests-only diff). Restore the +tree before you report. ## Lens: behaviour (`lens: behaviour` or `both`) @@ -43,26 +46,28 @@ Then answer, for each of them: Blocking: the change breaks behaviour, leaves wrong state, or breaches a rule or an accepted ADR; a user or a later change pays for it. When unsure, -ask "what breaks, and for whom?"; no answer means note. +ask "what breaks, and for whom?"; no answer means drop it. ## Lens: tests and rules (`lens: tests` or `both`) First list every test the diff adds or changes by line, never by title, -one line per file: `file: 12 read ok, 40 probed ok, 88 probed F3`. A file -whose tests only gained a fixture field: `file: fixture only`. +one line per file: `file: 12 read ok, 40 F3`. A file whose tests only +gained a fixture field: `file: fixture only`. Then answer: -1. For each test: can it fail for the reason its title gives? Read it - against testing rule 10's list first; those shapes need no probe to - prove. Spend your probes on the tests most likely to be wired to - nothing: break the code the title names and see whether the test goes - red on an assertion. A test that stays green is a finding. -2. Does the diff break any rule in `rules/`? Cite the file and the rule +1. For each mutant `mutants.md` lists alive: does its reason hold? Read the + code: a mutant whose change a caller, user, stored record or event can + see is untested behaviour (testing rule 3). No `mutants.md` on a diff + that changes source: `Assessment: incomplete`. +2. For each test: can it fail for the reason its title gives? Read it + against testing rule 10's list. With no `mutants.md`, spend your probes + on the tests most likely to be wired to nothing. +3. Does the diff break any rule in `rules/`? Cite the file and the rule number. -Blocking: a test that cannot fail for the reason its title gives, or a -breach of a rule or an accepted ADR. +Blocking: a survivor's reason that does not hold, a test that cannot fail +for the reason its title gives, or a breach of a rule or an accepted ADR. ## Assessment @@ -77,14 +82,15 @@ A finding is blocking only when it shows wrong behaviour or state, a false statement, a rule or ADR breach, or a line of the specification left undelivered or unproven. Testing rules and always-in-scope coverage apply even when the spec omits a case. Extra coverage beyond those requirements, -a stronger assertion than its claim needs, or defensive code is a note. +a stronger assertion than its claim needs, or defensive code is no +finding. `F0` is a good result: do not look for findings to have some. Output only your lists, then your findings: no preamble, no headings, no -summary, no narration. Report every blocking finding; there is no limit. -At most five notes, each a possible wrong behaviour or state you could not -prove within your probes; write no other note. One line per defect, in -this form and no other: +summary, no narration, at most 400 words besides the lists. Report every +blocking finding; there is no limit. Write a note only for a suspected +leak of a secret or a credential, or loss of user data, that you could not +prove: at most two. One line per defect, in this form and no other: ``` F B|N [] ev: | => diff --git a/.agents/briefs/review-spec.md b/.agents/briefs/review-spec.md index a25f1cf2a..47b6628a8 100644 --- a/.agents/briefs/review-spec.md +++ b/.agents/briefs/review-spec.md @@ -23,16 +23,21 @@ delivered`, or `Done when 3: differently: `. 2. Does the diff do anything the specification did not ask for? Anything `always-in-scope.md` lists is asked for: never a finding. 3. For every Tests line of the specification: the test in the diff that - proves it, by `file:line`, or `missing`. For every change in Tests after + proves it, by `file:line`, or `missing`. A line proven by more than one + new test, or a new test no Tests line asks for and no failure-mode or + always-in-scope item needs, is a finding [testing 7]. For every change in Tests after red that deletes a test or removes or loosens an assertion: which line of the specification is no longer proven? Whether a test's body proves its - title is the code review's question, not yours. + title, or can fail at all, is the code review's question, not yours; so + is a defect in behaviour the specification does not name (a missing + bound, a race). Do not report them. 4. For each assumption: is it the conservative choice, and consistent with the specification? One that contradicts a line of it, or decides behaviour a user would see that nobody decided, is blocking. Blocking: a line not delivered or delivered differently, a Tests line with -no test, a line of the specification left unproven, an +no test or with several, a new test nothing asks for, a line of the +specification left unproven, an assumption that fails question 4, behaviour a user sees that the specification did not ask for, or a breach of a file in `spec/`. @@ -55,13 +60,15 @@ A finding is blocking only when it shows wrong behaviour or state, a false statement, a rule or ADR breach, or a line of the specification left undelivered or unproven. Testing rules and always-in-scope coverage apply even when the spec omits a case. Extra coverage beyond those requirements, -a stronger assertion than its claim needs, or defensive code is a note. +a stronger assertion than its claim needs, or defensive code is no +finding. `F0` is a good result: do not look for findings to have some. Output only your list, then your findings: no preamble, no headings, no -summary, no narration. Report every blocking finding; there is no limit. -At most five notes, each a possible wrong behaviour or state; write no -other note. One line per defect, in this form and no other: +summary, no narration, at most 400 words besides the list. Report every +blocking finding; there is no limit. Write a note only for a suspected +leak of a secret or a credential, or loss of user data, that you could not +prove: at most two. One line per defect, in this form and no other: ``` F B|N [] ev: diff --git a/.agents/scripts/recurring-findings.mjs b/.agents/scripts/recurring-findings.mjs new file mode 100644 index 000000000..f50cf3b50 --- /dev/null +++ b/.agents/scripts/recurring-findings.mjs @@ -0,0 +1,104 @@ +#!/usr/bin/env node +// `node .agents/scripts/recurring-findings.mjs [--days ] [--min-prs ]`: which classes of review +// finding were fixed in at least --min-prs (default 3) pull requests merged in the last --days +// (default 28), read from each PR's `## Review ledger` comment (docs/internal/REVIEW.md, +// "Measurement"): the findings file in its `json` code block. +// +// Prints one line for the window, then one per class that recurs, most PRs first: +// +// Window: 2026-09-21 to 2026-10-19. Merged PRs: 40, with a review ledger: 31. +// testing 2: 4 PRs (#701, #698, #690, #688) src/a.ts:12, src/b.ts:40, ... +// +// The repo is GH_REPO, else the origin remote's owner/name. It calls repo-scoped REST only: a +// cloud session's GitHub proxy refuses GraphQL. +import { execFileSync } from "node:child_process"; + +const args = process.argv.slice(2); +const option = (name, fallback) => { + const at = args.indexOf(name); + return at === -1 ? fallback : Number(args[at + 1]); +}; +const days = option("--days", 28); +const minPrs = option("--min-prs", 3); +if (!(Number.isInteger(days) && days > 0 && Number.isInteger(minPrs) && minPrs > 0)) { + console.error("usage: recurring-findings.mjs [--days ] [--min-prs ]"); + process.exit(64); +} + +const nwo = + process.env.GH_REPO ?? + execFileSync("git", ["remote", "get-url", "origin"], { encoding: "utf8" }) + .trim() + .replace(/\.git$/, "") + .replace(/.*[/:]([^/:]+\/[^/]+)$/, "$1"); +const now = Date.now(); +const since = now - days * 24 * 60 * 60 * 1000; + +const merged = []; +for (let page = 1; ; page++) { + const pulls = get("pulls", { state: "closed", sort: "updated", direction: "desc", page }); + for (const pull of pulls) { + if (pull.merged_at !== null && Date.parse(pull.merged_at) >= since) merged.push(pull.number); + } + // why: sorted by update, so once a page ends before the window no later page reaches into it. + if (pulls.length < 100 || Date.parse(pulls.at(-1).updated_at) < since) break; +} + +const classes = new Map(); +let withLedger = 0; +for (const pr of merged) { + const findings = ledgerFindings(pr); + if (findings === null) continue; + withLedger += 1; + for (const f of findings.filter((x) => x.outcome === "fixed")) { + const key = String(f.class).trim().toLowerCase(); + const entry = classes.get(key) ?? { prs: new Set(), paths: [] }; + entry.prs.add(pr); + entry.paths.push(f.path); + classes.set(key, entry); + } +} + +const day = (ms) => new Date(ms).toISOString().slice(0, 10); +console.log( + `Window: ${day(since)} to ${day(now)}. Merged PRs: ${merged.length}, with a review ledger: ${withLedger}.`, +); +const recurring = [...classes.entries()] + .filter(([, e]) => e.prs.size >= minPrs) + .sort((a, b) => b[1].prs.size - a[1].prs.size || a[0].localeCompare(b[0])); +for (const [name, e] of recurring) { + const prs = [...e.prs].map((n) => `#${n}`).join(", "); + console.log(`${name}: ${e.prs.size} PRs (${prs}) ${e.paths.slice(0, 5).join(", ")}`); +} +if (recurring.length === 0) console.log(`No class was fixed in ${minPrs} or more PRs.`); + +function ledgerFindings(pr) { + for (let page = 1; ; page++) { + const comments = get(`issues/${pr}/comments`, { page }); + for (const { body } of comments) { + const text = (body ?? "").replace(/\r/g, ""); + if (!/^## Review ledger\s*$/m.test(text)) continue; + const block = /```json\n([\s\S]*?)\n```/.exec(text); + if (block === null) return null; + try { + const findings = JSON.parse(block[1]).findings; + return Array.isArray(findings) ? findings : null; + } catch { + return null; + } + } + if (comments.length < 100) return null; + } +} + +function get(path, fields) { + const flags = Object.entries({ per_page: 100, ...fields }).flatMap(([k, v]) => [ + "-f", + `${k}=${v}`, + ]); + const out = execFileSync("gh", ["api", "-X", "GET", `repos/${nwo}/${path}`, ...flags], { + encoding: "utf8", + maxBuffer: 64 * 1024 * 1024, + }); + return JSON.parse(out); +} diff --git a/.agents/scripts/report-guard.mjs b/.agents/scripts/report-guard.mjs index e1186f437..1d9091186 100644 --- a/.agents/scripts/report-guard.mjs +++ b/.agents/scripts/report-guard.mjs @@ -7,10 +7,10 @@ // hardware-verifier PR: # Hardware: ... // // 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. +// as well once did the fixes and skipped the audit. That line's `mutants alive +// explained ` (or `mutants none` when no source line changed) must explain every mutant +// `pnpm mutate` left alive: a test that cannot fail is the most repeated review finding. Other +// agents stop freely. Claude Code ends the turn anyway after eight blocks in a row. import fs from "node:fs"; const REPORT = { @@ -39,21 +39,23 @@ if (!shape.test(message)) { ); } 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]); + const mutants = + audit === null ? null : /\bmutants (?:none|\d+ alive (\d+) explained (\d+))\b/.exec(audit[0]); if (audit === null) { block( "Your report is an audit's but has no `audit:` line, so the audit did not run. Do the " + "audit in implement-audit.md on the whole diff, then report again with its `audit:` line.", ); - } else if (probed === null) { + } else if (mutants === null) { block( - "Your `audit:` line has no `probed` count. Probe every test you added or changed as " + - "implement-audit.md says, then report again with `probed /`.", + "Your `audit:` line has no `mutants` count. Run `pnpm mutate` as implement-audit.md says, " + + "then report again with `mutants alive explained `, or " + + "`mutants none` when it found no source line to mutate.", ); - } else if (Number(probed[1]) < Number(probed[2])) { + } else if (mutants[1] !== undefined && Number(mutants[2]) < Number(mutants[1])) { block( - `You probed ${probed[1]} of ${probed[2]} tests. Probe the rest as implement-audit.md ` + - "says: break the code each covers, run its file, see it fail on its assertion, restore.", + `You explained ${mutants[2]} of ${mutants[1]} mutants left alive. Kill each of the rest ` + + "with a test, or write why it changes nothing a caller can see, as implement-audit.md says.", ); } } diff --git a/.agents/scripts/review-inputs.sh b/.agents/scripts/review-inputs.sh index 2fa615e1f..291cb6a01 100755 --- a/.agents/scripts/review-inputs.sh +++ b/.agents/scripts/review-inputs.sh @@ -32,6 +32,8 @@ # tests-after-red.patch what changed in the files of the branch's first `test:` commit (the # red tests, or a bug's reproduction) between that commit and the head # assumptions.md the PR body's `Assumption:` lines (coding rule 2) +# mutants.md the PR body's `## Mutants` section: what `pnpm mutate` left alive at the +# audit, and how each was killed or why it changes nothing a caller sees # # Nothing else: never the rest of the PR body, commit messages, or any other comment. Rules, ADRs # and briefs come from the base branch, so a PR cannot rewrite what it is judged by. Whatever it looks @@ -195,6 +197,15 @@ else say "PR #$pr closes no issue (branch $head_ref, no 'Closes #n' in the body)" fi +printf '%s\n' "$pr_body" | awk ' + /^## / { inside = ($0 ~ /^## Mutants[ \t]*$/); next } + inside { print } +' >"$out/mutants.md" +grep -q . "$out/mutants.md" || { + rm -f "$out/mutants.md" + say "no ## Mutants section in the PR body" +} + # ADRs by number, wherever the spec names them, or wherever the record an ADR diff adds or edits # names them: "ADR 0014", "ADR-0014", "ADRs 0004 and 0005", or a link to adr/0014-....md. Each # comes as the base has it; one this diff adds is in diff.patch alone. diff --git a/.agents/scripts/review-metrics.mjs b/.agents/scripts/review-metrics.mjs index d3f937eaa..8ef2482a5 100644 --- a/.agents/scripts/review-metrics.mjs +++ b/.agents/scripts/review-metrics.mjs @@ -1,84 +1,219 @@ #!/usr/bin/env node -// Summarize a per-PR invocation ledger: node .agents/scripts/review-metrics.mjs ledger.json -import { readFileSync } from "node:fs"; +// `node .agents/scripts/review-metrics.mjs --pr [--issue ] [--findings ] [--json]`: +// what each agent run of a PR's delivery used, read from Claude Code's own transcripts, so no +// agent writes a number by hand. +// +// A run belongs to the PR when its prompt's first line is the ledger tag the deliver and review +// skills write, `ledger: pr= issue= stage= round= role= lens=` +// (keys in any order, any of them `-`), naming pr= or, before the PR exists, issue=. It +// reads every transcript under $CLAUDE_CONFIG_DIR/projects (default ~/.claude/projects), +// subagents' too, and counts each API message once (one message is written as several lines +// with the same id). Cost comes from the price table below; a model it does not list makes the +// cost unknown, never zero. +// +// --findings validates the coordinator's findings file (`{"findings": [...]}`, each with id, +// role, class, outcome, path) and adds the outcome and class counts. --json prints the ledger +// (runs, totals, findings) instead of the summary lines. +import { existsSync, openSync, readFileSync, readSync, closeSync, readdirSync } from "node:fs"; +import { homedir } from "node:os"; +import { join } from "node:path"; -const ledger = JSON.parse(readFileSync(process.argv[2], "utf8")); -const invocations = ledger.invocations; -if (!Array.isArray(invocations) || invocations.length === 0) { - throw new Error("invocations must be a nonempty array"); -} -const ids = new Set(); -const knownNumber = (value) => value === null || (Number.isFinite(value) && value >= 0); -for (const item of invocations) { - if (!item.id || ids.has(item.id)) throw new Error("invocation IDs must be unique"); - ids.add(item.id); - for (const key of ["role", "provider", "model", "commit"]) { - if (typeof item[key] !== "string" || !item[key]) throw new Error(`missing ${key}`); - } - if (!["1", "2", "fix-check"].includes(item.round)) throw new Error("invalid round"); - for (const key of ["costUsd", "fixSeconds"]) { - if (!knownNumber(item[key])) throw new Error(`invalid ${key}; use null for unknown`); - } - for (const key of ["input", "output", "cacheRead", "cacheWrite"]) { - if (!knownNumber(item.tokens?.[key])) throw new Error(`invalid tokens.${key}`); - } - for (const key of ["startedAt", "finishedAt"]) { - if (item[key] !== null && !Number.isFinite(Date.parse(item[key]))) { - throw new Error(`invalid ${key}; use null for unknown`); - } - } - if ( - item.startedAt !== null && - item.finishedAt !== null && - Date.parse(item.finishedAt) < Date.parse(item.startedAt) - ) { - throw new Error("finish precedes start"); - } +// $ per million tokens: input, output, cache read, 5-minute cache write, 1-hour cache write. +// https://platform.claude.com/docs/en/about-claude/pricing, read 2026-10-19. +const PRICES = { + "claude-opus-5-5": [4, 20, 0.2, 5, 8], + "claude-sonnet-5-5": [2, 10, 0.1, 2.5, 4], + "claude-haiku-5-5": [0.1, 0.5, 0.01, 0.125, 0.2], +}; +const OUTCOMES = ["open", "fixed", "duplicate", "incorrect", "intentional", "nonblocking"]; + +const args = process.argv.slice(2); +const option = (name) => { + const at = args.indexOf(name); + return at === -1 ? null : args[at + 1]; +}; +const pr = option("--pr"); +const issue = option("--issue"); +const findingsFile = option("--findings"); +if (pr === null || !/^\d+$/.test(pr) || (issue !== null && !/^\d+$/.test(issue))) { + console.error( + "usage: review-metrics.mjs --pr [--issue ] [--findings ] [--json]", + ); + process.exit(64); } + +const root = join(process.env.CLAUDE_CONFIG_DIR ?? join(homedir(), ".claude"), "projects"); +const runs = transcripts(root) + .map((file) => ({ file, tag: tagOf(file) })) + .filter(({ tag }) => tag !== null && (tag.pr === pr || (issue !== null && tag.issue === issue))) + .map(({ file, tag }) => run(file, tag)) + .sort((a, b) => Date.parse(a.startedAt ?? "") - Date.parse(b.startedAt ?? "")); +const findings = + findingsFile === null ? null : checked(JSON.parse(readFileSync(findingsFile, "utf8"))); + const total = (values) => - values.some((v) => v === null) ? null : values.reduce((a, b) => a + b, 0); + values.length === 0 || values.some((v) => v === null) + ? null + : Math.round(values.reduce((a, b) => a + b, 0) * 1e4) / 1e4; +const totals = { + runs: runs.length, + costUsd: total(runs.map((r) => r.costUsd)), + seconds: total(runs.map((r) => r.seconds)), + tokens: Object.fromEntries( + ["input", "output", "cacheRead", "cacheWrite"].map((k) => [ + k, + total(runs.map((r) => r.tokens[k])), + ]), + ), +}; + +if (args.includes("--json")) { + console.log(JSON.stringify({ pr: Number(pr), runs, totals, findings }, null, 2)); + process.exit(0); +} const show = (value) => (value === null ? "unknown" : String(value)); -const checks = new Set( - invocations - .filter((v) => ["spec", "code", "claims"].includes(v.role)) - .map((v) => `${v.round}:${v.commit}`), -).size; -const cost = total(invocations.map((v) => v.costUsd)); -const elapsed = invocations.some((v) => v.startedAt === null || v.finishedAt === null) - ? null - : (Math.max(...invocations.map((v) => Date.parse(v.finishedAt))) - - Math.min(...invocations.map((v) => Date.parse(v.startedAt)))) / - 1000; console.log( - `Review checks: ${checks}. Invocations: ${invocations.length}. Cost USD: ${show(cost)}.`, + `Runs: ${totals.runs}. Cost USD: ${show(totals.costUsd)}. Agent seconds: ${show(totals.seconds)}.`, ); console.log( - `Review elapsed seconds: ${show(elapsed)}. Fix effort seconds: ${show(total(invocations.map((v) => v.fixSeconds)))}.`, + `Tokens input ${show(totals.tokens.input)}, output ${show(totals.tokens.output)}, cache read ${show(totals.tokens.cacheRead)}, cache write ${show(totals.tokens.cacheWrite)}.`, ); -for (const key of ["input", "output", "cacheRead", "cacheWrite"]) { - console.log(`Review tokens ${key}: ${show(total(invocations.map((v) => v.tokens[key])))}.`); +for (const r of runs) { + const where = [r.stage, r.round, r.role, r.lens].filter((v) => v !== "-").join(" "); + console.log(`- ${where}: ${r.models.join("+")} ${show(r.seconds)} s $${show(r.costUsd)}`); } -const findings = ledger.findings ?? []; -const findingIds = new Set(); -for (const finding of findings) { - if (!finding.id || findingIds.has(finding.id)) throw new Error("finding IDs must be unique"); - findingIds.add(finding.id); - if ( - !["open", "fixed", "duplicate", "incorrect", "intentional", "nonblocking"].includes( - finding.outcome, - ) - ) { - throw new Error("invalid finding outcome"); +if (findings !== null) { + const count = (key) => + JSON.stringify( + Object.fromEntries( + [...new Set(findings.map((f) => f[key]))] + .sort() + .map((v) => [v, findings.filter((f) => f[key] === v).length]), + ), + ); + console.log(`Finding outcomes: ${count("outcome")}. Classes: ${count("class")}.`); +} + +function transcripts(dir) { + if (!existsSync(dir)) return []; + const files = []; + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const path = join(dir, entry.name); + if (entry.isDirectory()) files.push(...transcripts(path)); + else if (entry.name.endsWith(".jsonl")) files.push(path); } + return files; } -for (const finding of findings.filter((f) => f.outcome === "duplicate")) { - if (!findings.some((f) => f.id === finding.canonicalId && f.outcome !== "duplicate")) { - throw new Error("duplicate must reference a canonical finding"); + +// why: transcripts run to hundreds of megabytes; the tag is in the first user record, near the top. +function tagOf(file) { + const fd = openSync(file, "r"); + const head = Buffer.alloc(256 * 1024); + const read = readSync(fd, head, 0, head.length, 0); + closeSync(fd); + for (const line of head.subarray(0, read).toString("utf8").split("\n")) { + const rec = parsed(line); + if (rec?.type !== "user") continue; + const first = promptOf(rec).split("\n")[0].trim(); + if (!first.startsWith("ledger:")) return null; + const tag = { pr: "-", issue: "-", stage: "-", round: "-", role: "-", lens: "-" }; + for (const [, key, value] of first.matchAll(/(\w+)=(\S+)/g)) + if (key in tag) tag[key] = value.replace(/^#/, ""); + return tag; + } + return null; +} + +function run(file, tag) { + const messages = new Map(); + let startedAt = null; + let finishedAt = null; + for (const line of readFileSync(file, "utf8").split("\n")) { + const rec = parsed(line); + if (rec === null) continue; + if (typeof rec.timestamp === "string") { + startedAt ??= rec.timestamp; + finishedAt = rec.timestamp; + } + const usage = rec.message?.usage; + if (rec.type === "assistant" && usage && rec.message.id) { + messages.set(rec.message.id, { model: rec.message.model, usage }); + } + } + const tokens = { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }; + const models = new Set(); + let cost = 0; + for (const { model, usage } of messages.values()) { + models.add(model); + const split = usage.cache_creation; + const write5m = split + ? (split.ephemeral_5m_input_tokens ?? 0) + : (usage.cache_creation_input_tokens ?? 0); + const write1h = split ? (split.ephemeral_1h_input_tokens ?? 0) : 0; + const counts = [ + usage.input_tokens ?? 0, + usage.output_tokens ?? 0, + usage.cache_read_input_tokens ?? 0, + write5m, + write1h, + ]; + tokens.input += counts[0]; + tokens.output += counts[1]; + tokens.cacheRead += counts[2]; + tokens.cacheWrite += write5m + write1h; + const price = PRICES[model]; + cost = + price === undefined || cost === null + ? null + : cost + counts.reduce((sum, n, i) => sum + n * price[i], 0) / 1e6; + } + return { + file, + ...tag, + models: [...models], + startedAt, + finishedAt, + seconds: + startedAt === null + ? null + : Math.round((Date.parse(finishedAt) - Date.parse(startedAt)) / 1000), + tokens, + costUsd: cost === null ? null : Math.round(cost * 1e4) / 1e4, + }; +} + +function checked(file) { + const list = file.findings; + if (!Array.isArray(list)) throw new Error("findings must be an array"); + const ids = new Set(); + for (const f of list) { + if (typeof f.id !== "string" || ids.has(f.id)) throw new Error("finding IDs must be unique"); + ids.add(f.id); + for (const key of ["role", "class", "path"]) { + if (typeof f[key] !== "string" || f[key] === "") + throw new Error(`finding ${f.id} has no ${key}`); + } + if (!OUTCOMES.includes(f.outcome)) throw new Error(`finding ${f.id} has an invalid outcome`); + } + for (const f of list.filter((x) => x.outcome === "duplicate")) { + if (!list.some((x) => x.id === f.canonicalId && x.outcome !== "duplicate")) { + throw new Error(`duplicate ${f.id} must name a canonical finding`); + } + } + return list; +} + +function promptOf(rec) { + const content = rec.message?.content; + if (typeof content === "string") return content; + return Array.isArray(content) ? (content.find((b) => b.type === "text")?.text ?? "") : ""; +} + +function parsed(line) { + if (line === "") return null; + try { + return JSON.parse(line); + } catch { + return null; } } -console.log( - `Finding outcomes: ${JSON.stringify(Object.fromEntries(["open", "fixed", "duplicate", "incorrect", "intentional", "nonblocking"].map((outcome) => [outcome, findings.filter((f) => f.outcome === outcome).length])))}.`, -); -console.log( - `Escaped defects: ${ledger.escapedDefects === null || ledger.escapedDefects === undefined ? "unknown" : JSON.stringify(ledger.escapedDefects)}.`, -); diff --git a/.agents/scripts/tool-guard.mjs b/.agents/scripts/tool-guard.mjs index 7fde2f723..f5fed7e6f 100644 --- a/.agents/scripts/tool-guard.mjs +++ b/.agents/scripts/tool-guard.mjs @@ -3,7 +3,8 @@ // // check a sub-agent runs a check from toolchain.md's "Checks agents never run" (`pnpm // check`, `pnpm fallow`, ...) or a tool behind one, from any path. The commit hook, -// the push hook and CI run them. The main session (a person, a routine) may. +// the push hook and CI run them. The main session (a person, a routine) may, and +// an implementer may run `pnpm mutate` (its audit and fix modes do). // hooks a sub-agent skips the git hooks: `--no-verify`, `LEFTHOOK=0`, `core.hooksPath`, or // `LEFTHOOK_EXCLUDE` naming anything but `fallow` (the red stage's one push, whose // placeholders Fallow calls dead code). @@ -25,10 +26,9 @@ const SCRIPTS = new Set([ "format", "format:check", "fallow", - "mutate", ]); const TOOLS = new Set(["tsc", "oxlint", "depcruise", "oxfmt", "fallow", "stryker"]); -const NODE_SCRIPTS = new Set(["comment-lint.mjs", "mutate.mjs"]); +const NODE_SCRIPTS = new Set(["comment-lint.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"]); @@ -41,6 +41,7 @@ const HOOK_BYPASS = [ const input = JSON.parse(fs.readFileSync(0, "utf8") || "{}"); const tool = input.tool_name; +const mayMutate = input.agent_type === "implementer"; const args = input.tool_input ?? {}; if ((tool === "Agent" || tool === "Task") && args.isolation && STAGES.has(args.subagent_type)) { @@ -98,7 +99,11 @@ function isForbidden(words) { const [first, ...rest] = words; const name = path.basename(first); if (TOOLS.has(name) || NODE_SCRIPTS.has(name)) return true; - if (name === "node") return rest.some((w) => NODE_SCRIPTS.has(path.basename(w))); + if (name === "mutate.mjs") return !mayMutate; + if (name === "node") { + if (rest.some((w) => path.basename(w) === "mutate.mjs")) return !mayMutate; + return rest.some((w) => NODE_SCRIPTS.has(path.basename(w))); + } if (name === "npx" || name === "bunx") return isTool(firstOperand(rest)); return name === "pnpm" && isForbiddenPnpm(rest); } @@ -112,6 +117,7 @@ function isForbiddenPnpm(args) { // why: these scripts are allowed only in their narrow forms: named e2e files, Chromium only. if (sub === "test:e2e") return after.length === 0; if (sub === "test:console") return !args.includes("--project=chromium"); + if (sub === "mutate") return !mayMutate; return SCRIPTS.has(sub); } diff --git a/.claude/agents/implementer.md b/.claude/agents/implementer.md index f9e4fd32b..c3e01e2f6 100644 --- a/.claude/agents/implementer.md +++ b/.claude/agents/implementer.md @@ -122,7 +122,7 @@ failing: map: -> -audit: stale replaced donewhen / probed

/ +audit: stale replaced donewhen / mutants alive explained | mutants none fixes: -> found fixed run: pass | fail ci: pass | fail | running | unchecked diff --git a/.claude/agents/reviewer.md b/.claude/agents/reviewer.md index 8618d0264..6d1a95308 100644 --- a/.claude/agents/reviewer.md +++ b/.claude/agents/reviewer.md @@ -25,18 +25,20 @@ 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 with - inputs, worktree, round and lens. A user prohibiting delegation: do the + `code-reviewer` and `claims-reviewer` together in the foreground, each + prompt the ledger tag line (`toolchain.md`, "Review measurement") and + then inputs, worktree, round and lens. A user prohibiting delegation: do the checks yourself and label the report author self-check. 3. **Synthesis.** Wait for every lens, then follow REVIEW.md "Synthesis and - convergence". Remove probe worktrees after collecting evidence. Record - every invocation in the metrics ledger. + convergence". Remove probe worktrees after collecting evidence. Write + every canonical finding, with its class, to the findings file REVIEW.md + "Measurement" describes. ## Report End with this block; counts are unique canonical findings by owning role. A repeated finding keeps its ID and is not counted again. Preserve the -resolution ledger and per-invocation metrics with the PR. +resolution ledger and the findings file with the PR. ``` PR: #M Round: <1|2|fix-check> Kind: Commit: @@ -50,7 +52,7 @@ Notes: Rejected: Spec needs: Rerun: round 2 | fix-check | none -Metrics: +Findings file: ``` `Rerun: none` never means unresolved findings passed. A non-delivery PR diff --git a/.claude/skills/calibrate-reviews/SKILL.md b/.claude/skills/calibrate-reviews/SKILL.md index 5757fb9b6..83c6e1070 100644 --- a/.claude/skills/calibrate-reviews/SKILL.md +++ b/.claude/skills/calibrate-reviews/SKILL.md @@ -48,5 +48,5 @@ PR: #M merged | #M open () | none Calibration uses canonical incorrect findings, excluding duplicate, intentional and nonblocking outcomes. Preserve unknown legacy outcomes; do not infer false positives from raised-minus-fixed. For model promotion, -use the replay/shadow protocol and per-invocation ledger in +use the replay/shadow protocol and the run measurement in `docs/internal/REVIEW.md`, not this rejection sample alone. diff --git a/.claude/skills/deliver/SKILL.md b/.claude/skills/deliver/SKILL.md index d9f6a5157..d2571ed1c 100644 --- a/.claude/skills/deliver/SKILL.md +++ b/.claude/skills/deliver/SKILL.md @@ -15,7 +15,8 @@ rules 1, 2, 3, 9, 14, 15 and 16. **Delegating.** Start each stage with the Agent tool, in the background: `subagent_type` `implementer`, `reviewer` or `hardware-verifier`; a short `description` naming the issue and stage (`#362 implement`); and a prompt -of `key: value` lines and nothing else: `issue`, `branch`, `pr` once there +whose first line is the ledger tag (`toolchain.md`, "Review measurement"), +then `key: value` lines and nothing else: `issue`, `branch`, `pr` once there is one, `mode` or `round`, and what the stage works on, pasted from the last report (`map`, `fixes`, the reviewer's arguments). Include immutable `base-sha`, `merge-base`, `head` and, after review, `previous` plus the resolution ledger. No prose, and no @@ -165,8 +166,9 @@ they name; never build again or restart review at round 1. test fixes, need the applicable targeted fix-check before the gate. A clean base merge alone needs no repeat review; record its new head and base and let CI check it. Never silently reset the round cap. - - After each check persist the resolution ledger, all invocation metrics - (fix-check included), counts and dispositions in `## Review`. + - After each check persist the resolution ledger, the findings file + (`docs/internal/REVIEW.md`, "Measurement"), counts and dispositions in + `## Review`. - Out of scope lines may become separate issues only when verified as pre-existing and unrelated. New PR regressions block even if found late. - Spec needs: decide, then tell (above), before the fix run. @@ -202,8 +204,8 @@ they name; never build again or restart review at round 1. `` those confirmed and fixed, `` its notes; `skipped` in place of the counts for a review the diff's kind left out (for an ADR-only diff: `Code review: skipped. Claims review: skipped.`). `` is the number of - legacy full/targeted rounds, 1 or 2; record fix-check invocations separately - with the metrics ledger and summary from `review-metrics.mjs`. Add + legacy full/targeted rounds, 1 or 2. Post the `## Review ledger` comment + as `docs/internal/REVIEW.md` "Measurement" says, fix-checks included. Add `Assessment: complete` only when coverage is complete and no finding is open; otherwise `Assessment: incomplete` and park. `` is `tokens=` from `node .agents/scripts/usage.mjs`, this session's usage so diff --git a/.claude/skills/implement/SKILL.md b/.claude/skills/implement/SKILL.md index 444077427..2d2283bb7 100644 --- a/.claude/skills/implement/SKILL.md +++ b/.claude/skills/implement/SKILL.md @@ -6,5 +6,5 @@ description: Implement one claimed task or bug tests-first (red, green, 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, 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") 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. 3. **Report.** Print the last report block exactly as returned, with nothing added. diff --git a/.claude/skills/recurring-findings/SKILL.md b/.claude/skills/recurring-findings/SKILL.md new file mode 100644 index 000000000..d75526d63 --- /dev/null +++ b/.claude/skills/recurring-findings/SKILL.md @@ -0,0 +1,54 @@ +--- +name: recurring-findings +description: Weekly routine — find the review-finding classes fixed in three or more PRs merged in the last four weeks, and open one draft task per class proposing the check or rule that would catch it before review. Use when a routine or the user says "recurring findings". +model: sonnet +effort: low +--- + +# Recurring findings + +A defect that reviews keep finding is cheaper to catch with a script than +with a reviewer. You open draft tasks; you change no code and open no PR. + +## 1. Count + +```bash +node .agents/scripts/recurring-findings.mjs > /recurring.txt; echo "exit $?" +``` + +Each line after the first is a class fixed in three or more merged PRs. +None: report `Issues: none` and stop. + +## 2. One draft task per class + +For each class: + +1. Skip it when an open issue's title already names it: + `gh issue list --state open --search "Recurring finding: in:title"`. +2. Read each listed PR's `## Review ledger` comment and the fix commit of + every finding of the class (`git show `): what the code was before + the fix, and after. +3. Decide what would have caught it before review: + - **a check**: a lint rule, a comment-lint case, a `stale-refs.sh` + pattern or a new script, when the pre-fix code shows the defect + without judgement. Name where it goes. + - **a rule line**: one sentence for the review brief or rule file + whose check missed it, when only judgement can see it. +4. Open the task: + + ```bash + gh issue create --label task:draft --title "Recurring finding: " --body-file /.md + ``` + + The body: the class and the PRs; for each finding its `path:line` and + its pre-fix and fixed commits; the proposed check or rule line; and, + for a check, one Done when line: it flags every pre-fix example above + and passes every fixed one. End with `*Written by an agent.*`. + +## Report + +``` +Routine: recurring-findings Window: to Classes: +Issues: #a, #b | none +Skipped: — | none +``` diff --git a/.claude/skills/review/SKILL.md b/.claude/skills/review/SKILL.md index 901e1aa6d..3eccd8887 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"`, the arguments above as its whole prompt, no model. 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") and then the arguments above. 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/.claude/skills/verify-hardware/SKILL.md b/.claude/skills/verify-hardware/SKILL.md index 753401df0..170b3503a 100644 --- a/.claude/skills/verify-hardware/SKILL.md +++ b/.claude/skills/verify-hardware/SKILL.md @@ -6,5 +6,5 @@ description: Run a PR's slow-lane Done when lines on real devices on this machin Arguments: $ARGUMENTS 1. **Title.** Rename this session with `mcp__ccd_session_mgmt__set_session_title` (`session_id: "self"`; load it with ToolSearch first) to `[Hardware 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 `hardware-verifier` agent with the Agent tool: `subagent_type: "hardware-verifier"`, the arguments above as its whole prompt, no model. Do not do its work yourself. +2. **Run.** Start the `hardware-verifier` agent with the Agent tool: `subagent_type: "hardware-verifier"`, no model, its prompt the ledger tag line (`toolchain.md`, "Review measurement") and then the arguments above. Do not do its work yourself. 3. **Report.** When it finishes, print its report block exactly as returned, with nothing added. diff --git a/docs/internal/REVIEW.md b/docs/internal/REVIEW.md index 2696dcebc..b2107af47 100644 --- a/docs/internal/REVIEW.md +++ b/docs/internal/REVIEW.md @@ -49,11 +49,12 @@ without that assessment leave the finding unresolved, never rejected. Select independent lenses by work, not size alone. ADR-only: spec. Docs or comments: spec and claims. Tests: spec, code/tests and affected claims. Other changes: spec and code/both, plus affected claims. No issue: skip spec. -Add a separate code/tests lens for complex fixtures, mutation evidence or -high-risk coverage; split behavior and tests responsibilities explicitly. +Add a separate code/tests lens for complex fixtures, many survivors in +`mutants.md` or high-risk coverage; split behavior and tests +responsibilities explicitly. A large mechanical diff alone does not warrant two code reviewers. Record why a lens was added or skipped. Run selected lenses in parallel, with -separate worktrees for mutation probes; do not share findings until synthesis. +separate worktrees for probes; do not share findings until synthesis. ## Synthesis and convergence @@ -96,24 +97,20 @@ no incomplete assessment may be marked ready or merged. ## Measurement and cheaper-model evaluation -For every invoked role, including coordination, verification and fix work, -append a record to a per-PR JSON ledger. See -[the ledger example](templates/review-metrics.json). IDs are unique per -invocation, and review round is `1`, `2` or `fix-check` (fix work uses the -round it responds to). Store exact provider/model, reviewed commit, start -and finish timestamps, input/output/cache-read/cache-write tokens, cost USD -and fix effort seconds. Unknown values are null, never zero. Use provider -usage deltas for this invocation; whole-session cumulative tokens are only -legacy data. Cost comes from provider billing or a documented price/version -and token categories; never infer cost from model names alone. - -Record canonical finding outcomes, severity and escaped defects after -merge. Link the ledger as a durable PR attachment or committed evaluation -artifact; paste the output of `node .agents/scripts/review-metrics.mjs -` into `## Review`. Preserve the legacy counts and Rounds line -for older tooling, with `Tokens: n/a.` if no comparable session total exists. -`delivery-stats.mjs` displays new invocation/check/cost totals separately. -Do not call raised-minus-fixed a false-positive rate. +Nobody writes usage by hand. Every agent prompt a delivery writes starts +with the ledger tag line (`toolchain.md`, "Review measurement"), and +`node .agents/scripts/review-metrics.mjs --pr --issue ` reads each +tagged run's model, start, finish, tokens and cost from Claude Code's +transcripts. Unknown values print as unknown, never zero. + +The coordinator keeps one findings file per PR, `{"findings": [...]}`, one +entry per canonical finding: `id`, `role`, `class` (the `[class]` the lens +gave, a rule like `testing 2` or at most 8 words), `outcome`, `path`, +`fix` (the fix commit, once fixed) and `canonicalId` for a duplicate. Before the gate, post one PR comment headed +`## Review ledger`: the output of `review-metrics.mjs --pr --issue +--findings `, then the findings file in a `json` code block. The +weekly `recurring-findings` routine reads these comments. Record escaped +defects after merge. Do not call raised-minus-fixed a false-positive rate. Before promoting Sonnet code review, freeze at least 20 representative historical PR snapshots: at least 10 known-bug and 10 clean, covering ordinary diff --git a/docs/internal/agent-rules/testing.md b/docs/internal/agent-rules/testing.md index 5ac9ea05b..9afe979ea 100644 --- a/docs/internal/agent-rules/testing.md +++ b/docs/internal/agent-rules/testing.md @@ -49,7 +49,8 @@ prove the behaviour, each at the cheapest level that can prove it of the part that matters — usually because the test exercises an internal callback rather than the observable output the path exists to produce. Write the test that goes red, then restore the code. Mutation - testing does this mechanically, weekly, for the newest changes on `main` + testing does this mechanically for every pull request's changed lines, + and weekly for the newest changes on `main` ([toolchain.md](toolchain.md) says where it runs). 4. **A test that enforces a rule must see everything the rule covers.** A @@ -88,9 +89,13 @@ prove the behaviour, each at the cheapest level that can prove it 7. **Prove each claim once, at the cheapest level that can prove it.** Before you add a test, search for one that already proves the claim or the behaviour next to it, and extend that one (a case, an assertion); - write a new test only when none does. A contract several - implementations share is one suite run over each of them. A claim proved - at one level is not proved again at another: + write a new test only when none does. A spec's Tests line is one test: + the cases it lists go in that test's body, as one table when they differ + only in data, never one test per case. A contract several + implementations share (the worker's and the gateway's handlers of one + operation) is one suite run over each of them. A claim proved at one + level is not proved again at another, nor through a second seam of the + same level: | Claim | Level | | ---------------------------------------------------------- | --------------------------------------------------- | @@ -109,7 +114,8 @@ prove the behaviour, each at the cheapest level that can prove it `main` keeps its reproduction as its regression test. A defect a review finds in a pull request is fixed and proven as rule 7 says: by extending the test that proves the behaviour next to it, and by a new test only - when none does. + when none does; the fix report then names the test it could not extend, + and why. 10. **The usual ways an assertion cannot fail.** Each of these passes against a broken implementation; check every test you write or change diff --git a/docs/internal/agent-rules/toolchain.md b/docs/internal/agent-rules/toolchain.md index 3f707380e..27edc69d8 100644 --- a/docs/internal/agent-rules/toolchain.md +++ b/docs/internal/agent-rules/toolchain.md @@ -22,7 +22,8 @@ Hooks and CI run these. Never run them by hand, and never the tools behind them (from `node_modules/.bin` or any other path): `pnpm check`, `pnpm test`, `pnpm test:timing`, `pnpm typecheck`, `pnpm typecheck:e2e`, `pnpm lint`, `pnpm lint:comments`, `pnpm format`, `pnpm format:check`, -`pnpm fallow`, `pnpm mutate` (outside the `harden-tests` routine), the whole +`pnpm fallow`, `pnpm mutate` (outside the implementer and the `harden-tests` +routine), the whole e2e suite, the console lane in every browser (`pnpm test:console`), `tsc`, `oxlint`, `depcruise`, `oxfmt`, `fallow`, `stryker`, `.agents/scripts/comment-lint.mjs` (outside the `garden-comments` routine). A PreToolUse hook @@ -43,16 +44,26 @@ list and this one the same. ## Mutation testing -Not part of delivery. The weekly `harden-tests` routine runs -`pnpm mutate --max-lines 1500` on `main` for the source lines -changed since ``, newest files first up to that many lines, kills -the survivors that change behaviour a caller or user can see, and opens -one tests-only PR. +`pnpm mutate ` mutates the source lines changed since `` and +lists each mutant the unit suite let live. It first runs only the test +files that import a changed file, or reach it through their own module's +`index.ts`, or that the branch changed; a mutant those leave alive gets a +second run with every related test file. It reuses each unchanged result, +so a fix run pays only for what changed. + +- **Delivery:** the implementer's audit runs it on the PR's diff (testing + rule 2) and writes what stays alive, with a reason each, under + `## Mutants` in the PR body; the code review checks those reasons. +- **Weekly:** the `harden-tests` routine runs `pnpm mutate + --max-lines 1500` on `main`, newest files first up to that many lines, + kills the survivors that change behaviour a caller or user can see, and + opens one tests-only PR. ## Routines -Claude Code routines run these skills on a schedule, each opening one PR -that goes through the reviews its diff's kind gets and the merge gate: +Claude Code routines run these skills on a schedule, each but one opening +one PR that goes through the reviews its diff's kind gets and the merge +gate: | Routine | Skill | When | | ------------------ | ------------------- | ------ | @@ -60,8 +71,11 @@ that goes through the reviews its diff's kind gets and the merge gate: | Test audit | `audit-tests` | weekly | | Comment gardening | `garden-comments` | weekly | | Review calibration | `calibrate-reviews` | weekly | +| Recurring findings | `recurring-findings` | weekly | -Every routine ends the same way; its skill adds only what it changes: +`recurring-findings` opens draft task issues instead of a PR, and skips +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, within the routine's kind of edit, then the check its `Rerun` names: @@ -178,7 +192,16 @@ the tool's docs alone. ## Review measurement -`node .agents/scripts/review-metrics.mjs ` validates and -summarizes per-PR invocation records. Use the template and evaluation -procedure in [REVIEW.md](../REVIEW.md). This is reporting, not a broad -project check. Preserve unknown usage/cost as null. +Every prompt a delivery stage or review lens is started with begins with +this line, `-` for a key that does not apply yet: + +``` +ledger: pr= issue= stage= round=<1|2|fix-check> role= lens= +``` + +`node .agents/scripts/review-metrics.mjs --pr --issue [--findings +]` reads the runs so tagged from Claude Code's transcripts on this +machine and prints their models, seconds, tokens and cost, and with +`--findings` the outcome and class counts; [REVIEW.md](../REVIEW.md) says +what goes in the findings file and where the result is posted. It reports, +it checks nothing. diff --git a/docs/internal/templates/review-metrics.json b/docs/internal/templates/review-metrics.json deleted file mode 100644 index 336ce8d96..000000000 --- a/docs/internal/templates/review-metrics.json +++ /dev/null @@ -1,20 +0,0 @@ -{ - "pr": 123, - "invocations": [ - { - "id": "code-r1", - "role": "code", - "provider": "anthropic", - "model": "record-exact-model-id", - "commit": "record-full-reviewed-sha", - "round": "1", - "startedAt": null, - "finishedAt": null, - "tokens": { "input": null, "output": null, "cacheRead": null, "cacheWrite": null }, - "costUsd": null, - "fixSeconds": null - } - ], - "findings": [], - "escapedDefects": null -} diff --git a/scripts/mutate.mjs b/scripts/mutate.mjs index 72f84e3ea..3c429fbda 100644 --- a/scripts/mutate.mjs +++ b/scripts/mutate.mjs @@ -1,6 +1,9 @@ #!/usr/bin/env node // `pnpm mutate [base] [--max-lines ]`: mutation-test only the source lines this branch changed // against base (default origin/main), with the unit suite, and print each mutant the suite let live. +// A near pass runs only the unit test files that import a mutated file directly and those the +// branch changed; a wide pass gives each mutant the near pass left alive every unit test file +// Vitest finds related to its file, through any depth of imports. // With --max-lines, it takes the changed files newest first (by the last commit on HEAD's // first-parent history that touched each; uncommitted changes are newest of all) until the next // file would pass n changed lines, cuts a first file larger than n at n, and prints what it skipped. @@ -22,7 +25,7 @@ import { statSync, writeFileSync, } from "node:fs"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; const REPORT = ".stryker-tmp/report/mutation.json"; const KEEP_MS = 14 * 24 * 60 * 60 * 1000; @@ -112,47 +115,138 @@ if (clean && last?.sources === sources && last.mutate === mutate) { print(last.total, ranges.length, last.alive); } -// A killed run leaves its sandboxes behind; start clean. -rmSync(".stryker-tmp", { force: true, recursive: true }); -const run = spawnSync( - "pnpm", - [ - "exec", - "stryker", - "run", - "--mutate", - mutate, - "--incremental", - "--incrementalFile", - `${branch}.json`, - ], - { stdio: ["ignore", "inherit", "pipe"], encoding: "utf8", maxBuffer: 64 * 1024 * 1024 }, -); -process.stderr.write(run.stderr ?? ""); -if (!existsSync(REPORT)) { - // Last, so a caller that keeps only the tail of the output still sees why. - const tail = (run.stderr ?? "").trimEnd().split("\n").slice(-15).join("\n"); - console.error(`\nStryker exited ${run.status} without a report: no mutant was tested.\n${tail}`); - process.exit(2); -} - -const report = JSON.parse(readFileSync(REPORT, "utf8")); -const alive = []; +// why: Stryker's first run executes every test it may use, to learn which test covers which +// line, and Vitest's related mode follows imports through any depth: one changed file can +// pull in thousands of tests. So the near pass runs only the test files that import a mutated +// file directly, plus the test files the branch changed; the wide pass gives the mutants that +// near pass left alive the whole related set. A mutant is reported alive only when the related +// set lets it live, as a single related run would. +const near = nearTests([...new Set(ranges.map((r) => r.slice(0, r.indexOf(":"))))]); let total = 0; -for (const [path, { mutants }] of Object.entries(report.files)) { - for (const m of mutants) { - total += 1; - if (m.status === "Survived" || m.status === "NoCoverage") { - const replacement = (m.replacement ?? "").replace(/\s+/g, " ").slice(0, 80); - alive.push(`${path}:${m.location.start.line} ${m.status} ${m.mutatorName}: ${replacement}`); - } - } +let alive = []; +let wideMutate = mutate; +let nearAlive = null; +if (near.length > 0) { + console.log(`Near pass: ${near.length} test files that import a mutated file directly.`); + const report = stryker(mutate, near, `${branch}.json`); + total = report.total; + wideMutate = report.alive.map((m) => m.range).join(","); + nearAlive = new Set(report.alive.map((m) => m.key)); +} +if (wideMutate !== "") { + console.log(`Wide pass: ${wideMutate.split(",").length} ranges with every related test file.`); + const report = stryker( + wideMutate, + [], + near.length > 0 ? `${branch}.wide.json` : `${branch}.json`, + ); + if (near.length === 0) total = report.total; + // why: a range can hold a mutant the near pass killed, and a branch's changed test file need + // not be related to the file, so only a mutant both passes left alive is alive. + alive = report.alive.filter((m) => nearAlive?.has(m.key) ?? true).map((m) => m.text); } if (clean) { writeFileSync(`${branch}.last.json`, JSON.stringify({ sources, mutate, total, alive })); } print(total, ranges.length, alive); +function stryker(mutate, testFiles, incrementalFile) { + // why: a killed run leaves its sandboxes behind, and a stale report would be read as this one. + rmSync(".stryker-tmp", { force: true, recursive: true }); + const argv = ["exec", "stryker", "run", "--mutate", mutate]; + if (testFiles.length > 0) argv.push("--testFiles", testFiles.join(",")); + argv.push("--incremental", "--incrementalFile", incrementalFile); + const run = spawnSync("pnpm", argv, { + stdio: ["ignore", "inherit", "pipe"], + encoding: "utf8", + maxBuffer: 64 * 1024 * 1024, + }); + process.stderr.write(run.stderr ?? ""); + if (!existsSync(REPORT)) { + // why: last, so a caller that keeps only the tail of the output still sees the cause. + const tail = (run.stderr ?? "").trimEnd().split("\n").slice(-15).join("\n"); + console.error( + `\nStryker exited ${run.status} without a report: no mutant was tested.\n${tail}`, + ); + process.exit(2); + } + const report = JSON.parse(readFileSync(REPORT, "utf8")); + const alive = []; + let total = 0; + for (const [path, { mutants }] of Object.entries(report.files)) { + for (const m of mutants) { + total += 1; + if (m.status === "Survived" || m.status === "NoCoverage") { + const { start, end } = m.location; + const replacement = (m.replacement ?? "").replace(/\s+/g, " ").slice(0, 80); + alive.push({ + // why: the report counts columns from 1 and a --mutate range from 0; keeping the end + // as reported widens the range by one column, which can only take this mutant in. + range: `${path}:${start.line}:${start.column - 1}-${end.line}:${end.column}`, + key: `${path}:${start.line}:${start.column}-${end.line}:${end.column} ${m.mutatorName} ${m.replacement}`, + text: `${path}:${start.line} ${m.status} ${m.mutatorName}: ${replacement}`, + }); + } + } + } + return { total, alive }; +} + +// why: a module's own tests reach it through its index.ts (architecture rule 14), so a test +// that imports a barrel re-exporting a mutated file counts when it sits in that barrel's +// directory; tests elsewhere import the barrel as callers and are left to the wide pass. A +// mutated file that only re-exports has no mutant, so its importers are not taken for it. The +// test files are vitest.mutate.config.ts's unit set. +function nearTests(files) { + const reExport = /^\s*export\s+(?:type\s+)?(?:\*|\{[^}]*\})\s+from\s+["'](\.[^"']+)["'];?/gm; + const importLine = /^\s*import\s[^;]*?from\s+["'][^"']+["'];?/gm; + const comment = /\/\*[\s\S]*?\*\/|^\s*\/\/.*$/gm; + const specifier = /(?:from\s+|import\s*\(\s*|import\s+)["'](\.[^"']+)["']/g; + const resolve = (from, s) => { + const path = join(dirname(from), s); + return [path, path.replace(/\.jsx?$/, ".ts"), path.replace(/\.jsx?$/, ".tsx")].find( + (p) => statSync(p, { throwIfNoEntry: false })?.isFile() === true, + ); + }; + const source = (f) => readFileSync(f, "utf8").replace(comment, ""); + const targets = new Set( + files.filter((f) => source(f).replace(reExport, "").replace(importLine, "").trim() !== ""), + ); + // why: a barrel may re-export another barrel, so a chain is followed to its files. + const exposed = new Map(); + const exposes = (file) => { + if (!exposed.has(file)) { + exposed.set(file, new Set()); + for (const [, s] of source(file).matchAll(reExport)) { + const target = resolve(file, s); + if (target === undefined) continue; + exposed.get(file).add(target); + for (const t of exposes(target)) exposed.get(file).add(t); + } + } + return exposed.get(file); + }; + const reaches = (test, imported) => + targets.has(imported) || + (test.startsWith(`${dirname(imported)}/`) && + [...exposes(imported)].some((f) => targets.has(f))); + const unit = (f) => f !== "" && !f.endsWith(".timing.test.ts"); + const globs = ["src/*.test.ts", "ui/*.test.ts", "ui/*.test.tsx"]; + const tests = git("ls-files", ...globs) + .split("\n") + .filter(unit); + const changedTests = git("diff", "--name-only", "--diff-filter=d", mergeBase, "--", ...globs) + .split("\n") + .filter(unit); + const picked = tests.filter((test) => + [...readFileSync(test, "utf8").matchAll(specifier)].some(([, s]) => { + const imported = resolve(test, s); + return imported !== undefined && reaches(test, imported); + }), + ); + return [...new Set([...picked, ...changedTests])]; +} + function print(total, rangeCount, alive) { console.log(`\n${total} mutants on ${rangeCount} changed ranges; ${alive.length} alive.`); for (const line of alive) console.log(line); diff --git a/src/mutate.test.ts b/src/mutate.test.ts index 5b1defb22..3990589f6 100644 --- a/src/mutate.test.ts +++ b/src/mutate.test.ts @@ -15,26 +15,15 @@ import { describe, expect, it } from "vitest"; const SCRIPT = join(dirname(fileURLToPath(import.meta.url)), "..", "scripts", "mutate.mjs"); -const REPORT = JSON.stringify({ - files: { - "src/a.ts": { - mutants: [ - { - status: "Killed", - mutatorName: "BooleanLiteral", - replacement: "true", - location: { start: { line: 1 } }, - }, - { - status: "Survived", - mutatorName: "BooleanLiteral", - replacement: "false", - location: { start: { line: 2 } }, - }, - ], - }, - }, +const mutant = (status: string, line: number, replacement: string) => ({ + status, + mutatorName: "BooleanLiteral", + replacement, + location: { start: { line, column: 18 }, end: { line, column: 22 } }, }); +const report = (...mutants: ReturnType[]) => + JSON.stringify({ files: { "src/a.ts": { mutants } } }); +const REPORT = report(mutant("Killed", 1, "true"), mutant("Survived", 2, "false")); interface Repo { readonly dir: string; @@ -53,7 +42,7 @@ function repo(): Repo { mkdirSync(bin); writeFileSync( join(bin, "pnpm"), - `#!/bin/sh\necho "$@" >> "${log}"\nif [ -n "$CRASH" ]; then echo "$CRASH" >&2; exit 1; fi\nmkdir -p .stryker-tmp/report\necho '${REPORT}' > .stryker-tmp/report/mutation.json\n`, + `#!/bin/sh\necho "$@" >> "${log}"\nif [ -n "$CRASH" ]; then echo "$CRASH" >&2; exit 1; fi\nmkdir -p .stryker-tmp/report\ncase "$*" in *--testFiles*) [ -n "$NEAR_REPORT" ] && { echo "$NEAR_REPORT" > .stryker-tmp/report/mutation.json; exit 0; } ;; esac\n[ -n "$WIDE_REPORT" ] && { echo "$WIDE_REPORT" > .stryker-tmp/report/mutation.json; exit 0; }\necho '${REPORT}' > .stryker-tmp/report/mutation.json\n`, ); chmodSync(join(bin, "pnpm"), 0o755); const git = (...args: string[]) => @@ -76,11 +65,24 @@ function repo(): Repo { return { dir, log, bin, git, commit }; } -function mutate(r: Repo, cwd = r.dir, crash = "", args: string[] = []) { +function mutate( + r: Repo, + cwd = r.dir, + crash = "", + args: string[] = [], + nearReport = "", + wideReport = "", +) { const run = spawnSync(process.execPath, [SCRIPT, "base", ...args], { cwd, encoding: "utf8", - env: { ...process.env, PATH: `${r.bin}:/usr/bin:/bin`, CRASH: crash }, + env: { + ...process.env, + PATH: `${r.bin}:/usr/bin:/bin`, + CRASH: crash, + NEAR_REPORT: nearReport, + WIDE_REPORT: wideReport, + }, }); return { code: run.status, output: run.stdout + run.stderr }; } @@ -105,6 +107,65 @@ describe("mutate.mjs", () => { expect(code).toBe(1); }); + it("runs first only the test files that import a mutated file, reach it through their own module's barrel, or that the branch changed", () => { + const r = repo(); + r.git("switch", "-q", "base"); + r.commit({ + "src/m/x.ts": "export const x = 1;\n", + "src/m/index.ts": 'export { x } from "./x.js";\n', + "src/m/x.test.ts": 'import { x } from "./index.js";\n', + "src/other/y.test.ts": 'import { x } from "../m/index.js";\n', + "src/a.test.ts": 'import { a } from "./a.js";\n', + "src/lone.test.ts": "export {};\n", + "src/lone2.test.ts": "export {};\n", + }); + r.git("switch", "-q", "work"); + r.git("rebase", "-q", "base"); + r.commit({ + "src/m/x.ts": "export const x = 2;\n", + "src/lone2.test.ts": "export const t = 1;\n", + }); + + const { output } = mutate(r, r.dir, "", [], report(mutant("Killed", 2, "false"))); + + expect(strykerRuns(r)).toHaveLength(1); + expect(strykerRuns(r)[0]?.split(" ")[6]?.split(",").sort()).toEqual([ + "src/a.test.ts", + "src/lone2.test.ts", + "src/m/x.test.ts", + ]); + expect(output).toContain("Near pass: 3 test files"); + }); + + it("gives only the mutants the first run left alive a run with every related test, and reports alive only those both runs left alive", () => { + const r = repo(); + r.commit({ "src/a.test.ts": 'import { b } from "./a.js";\n' }); + const near = report( + mutant("Survived", 1, "true"), + mutant("NoCoverage", 2, "false"), + mutant("Killed", 3, "true"), + ); + // why: the second run's range can take in a mutant the first run killed; it stays killed. + const wide = report( + mutant("Killed", 1, "true"), + mutant("Survived", 2, "false"), + mutant("Survived", 3, "true"), + ); + + const { code, output } = mutate(r, r.dir, "", [], near, wide); + + const runs = strykerRuns(r); + expect(runs).toHaveLength(2); + expect(runs[1]).toMatch( + /^exec stryker run --mutate src\/a\.ts:1:17-1:22,src\/a\.ts:2:17-2:22 --incremental /, + ); + expect(output).toContain("3 mutants on 1 changed ranges; 1 alive."); + expect(output).toContain("src/a.ts:2 Survived BooleanLiteral: false"); + expect(output).not.toContain("src/a.ts:1 "); + expect(output).not.toContain("src/a.ts:3 "); + expect(code).toBe(1); + }); + it("ends with Stryker's last error lines and exits 2, not 1, when Stryker fails without a report", () => { const r = repo(); diff --git a/src/recurring-findings.test.ts b/src/recurring-findings.test.ts new file mode 100644 index 000000000..7aaff4b19 --- /dev/null +++ b/src/recurring-findings.test.ts @@ -0,0 +1,103 @@ +import { spawnSync } from "node:child_process"; +import { chmodSync, 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", + "recurring-findings.mjs", +); + +// why: the fake answers repo-scoped REST GETs only, as a cloud session's proxy does. +const FAKE_GH = `#!/usr/bin/env node +const fs = require("node:fs"); +const github = JSON.parse(fs.readFileSync(process.env.FAKE_GH_FILE, "utf8")); +const args = process.argv.slice(2); +if (args[0] !== "api" || args[1] !== "-X" || args[2] !== "GET") process.exit(9); +const fields = {}; +for (let i = 4; i < args.length; i += 2) fields[args[i + 1].split("=")[0]] = args[i + 1].split("=")[1]; +const value = github[args[3]]; +if (value === undefined) process.exit(9); +const page = Number(fields.page); +process.stdout.write(JSON.stringify(value.slice((page - 1) * 100, page * 100))); +`; + +const DAY = 24 * 60 * 60 * 1000; +const ago = (days: number) => new Date(Date.now() - days * DAY).toISOString(); +const pull = (number: number, mergedDaysAgo: number | null) => ({ + number, + merged_at: mergedDaysAgo === null ? null : ago(mergedDaysAgo), + updated_at: ago(mergedDaysAgo ?? 1), +}); +const ledger = (...findings: object[]) => ({ + body: `## Review ledger\r\n\r\nRuns: 3.\r\n\r\n\`\`\`json\r\n${JSON.stringify({ findings })}\r\n\`\`\`\r\n`, +}); +const fixed = (cls: string, path: string) => ({ + id: "R1", + role: "code", + class: cls, + path, + outcome: "fixed", +}); + +function recurring(github: Record, ...args: string[]) { + const dir = mkdtempSync(join(tmpdir(), "recurring-")); + writeFileSync(join(dir, "github.json"), JSON.stringify(github)); + writeFileSync(join(dir, "gh"), FAKE_GH); + chmodSync(join(dir, "gh"), 0o755); + const run = spawnSync(process.execPath, [SCRIPT, ...args], { + encoding: "utf8", + env: { + ...process.env, + PATH: `${dir}:${process.env.PATH}`, + FAKE_GH_FILE: join(dir, "github.json"), + GH_REPO: "o/r", + }, + }); + return run.stdout; +} + +describe("recurring-findings.mjs", () => { + it("lists each class fixed in at least --min-prs PRs merged in the window, most PRs first, from their review ledger comments", () => { + const out = recurring( + { + "repos/o/r/pulls": [pull(5, 2), pull(4, 3), pull(3, 4), pull(2, null), pull(1, 40)], + "repos/o/r/issues/5/comments": [ + { body: "## Review notes\n- code: x" }, + ledger(fixed("Testing 2", "src/a.ts:1"), fixed("architecture 13", "docs/a.md:3")), + ], + "repos/o/r/issues/4/comments": [ + ledger(fixed("testing 2", "src/b.ts:2"), { + ...fixed("architecture 13", "docs/b.md:1"), + outcome: "incorrect", + }), + ], + "repos/o/r/issues/3/comments": [ledger(fixed("testing 2", "src/c.ts:3"))], + "repos/o/r/issues/1/comments": [ledger(fixed("architecture 13", "docs/c.md:9"))], + }, + "--min-prs", + "2", + ); + + expect(out).toMatch(/^Window: \S+ to \S+\. Merged PRs: 3, with a review ledger: 3\.$/m); + expect(out.trim().split("\n").slice(1)).toEqual([ + "testing 2: 3 PRs (#5, #4, #3) src/a.ts:1, src/b.ts:2, src/c.ts:3", + ]); + }); + + it("says so when no class recurs, and counts a PR with no ledger comment as merged but not read", () => { + const out = recurring({ + "repos/o/r/pulls": [pull(9, 1)], + "repos/o/r/issues/9/comments": [{ body: "LGTM" }], + }); + + expect(out).toContain("Merged PRs: 1, with a review ledger: 0."); + expect(out).toContain("No class was fixed in 3 or more PRs."); + }); +}); diff --git a/src/report-guard.test.ts b/src/report-guard.test.ts index 893165ae2..193eef28f 100644 --- a/src/report-guard.test.ts +++ b/src/report-guard.test.ts @@ -54,25 +54,22 @@ describe("report-guard.mjs", () => { JSON.parse(stop("implementer", `${head}fixes:\n a -> found 1 fixed 1\n`)).reason, ).toContain("no `audit:` line"); expect( - stop("implementer", `${head}audit: stale 0 replaced 0 donewhen 5/5 probed 4/4\n`), + stop("implementer", `${head}audit: stale 0 replaced 0 donewhen 5/5 mutants none\n`), ).toBe(""); }); - it("blocks an audit that probed fewer tests than it added or changed, or gives no count", () => { + it("blocks an audit that explained fewer mutants than it left alive, or gives no count", () => { const head = "issue: 472 branch: task/472 pr: 497 mode: audit head: 9d78eb95\ntests: 49/49\n"; + const audit = (mutants: string) => + stop("implementer", `${head}audit: stale 0 replaced 0 donewhen 5/5 ${mutants}\n`); - expect( - JSON.parse( - stop("implementer", `${head}audit: stale 0 replaced 0 donewhen 5/5 probed 1/4\n`), - ).reason, - ).toContain("probed 1 of 4"); - expect( - JSON.parse(stop("implementer", `${head}audit: stale 0 replaced 0 donewhen 5/5\n`)).reason, - ).toContain("no `probed` count"); - expect( - stop("implementer", `${head}audit: stale 0 replaced 0 donewhen 5/5 probed 0/0\n`), - ).toBe(""); + expect(JSON.parse(audit("mutants 40 alive 3 explained 1")).reason).toContain( + "explained 1 of 3", + ); + expect(JSON.parse(audit("probed 4/4")).reason).toContain("no `mutants` count"); + expect(audit("mutants 40 alive 3 explained 3")).toBe(""); + expect(audit("mutants 40 alive 0 explained 0")).toBe(""); }); it("blocks a reviewer whose block belongs to another agent, and lets any other agent stop", () => { diff --git a/src/review-inputs.test.ts b/src/review-inputs.test.ts index 52c547e82..e70166900 100644 --- a/src/review-inputs.test.ts +++ b/src/review-inputs.test.ts @@ -91,12 +91,13 @@ function inputs( comments: readonly { body: string }[], previous?: string, base = "main", + prBody: string | null = null, ) { const file = join(mkdtempSync(join(root, "gh-")), "github.json"); writeFileSync( file, JSON.stringify({ - "repos/o/r/pulls/7": { head: { ref: "bug/12" }, base: { ref: base }, body: null }, + "repos/o/r/pulls/7": { head: { ref: "bug/12" }, base: { ref: base }, body: prBody }, "repos/o/r/issues/12": issue, "repos/o/r/issues/12/comments": comments, "repos/o/r/issues/5": { body: "Feature\r\nfive." }, @@ -126,6 +127,24 @@ function inputs( const BUG = { body: "The bug.", labels: [{ name: "bug:ready" }], parent_issue_url: null }; describe("review-inputs.sh over repo-scoped REST", { timeout: 30_000 }, () => { + it("writes the PR body's Mutants section to mutants.md and no other part of the body", () => { + const body = [ + "Closes #12", + "## Mutants", + "40 mutants, 1 alive:", + "- src/a.ts:3 ConditionalExpression: equivalent, the branch only logs", + "## Review", + "Spec review: skipped.", + ].join("\r\n"); + + const { read } = inputs(BUG, [], undefined, "main", body); + + expect(read("mutants.md")).toBe( + "40 mutants, 1 alive:\n- src/a.ts:3 ConditionalExpression: equivalent, the branch only logs\n", + ); + expect(inputs(BUG, []).read("mutants.md")).toBeNull(); + }); + it("writes the issue the branch names, and its parent from parent_issue_url", () => { const parented = { ...BUG, parent_issue_url: "https://api.github.com/repos/o/r/issues/5" }; const result = inputs(parented, []); diff --git a/src/review-metrics.test.ts b/src/review-metrics.test.ts new file mode 100644 index 000000000..a66b98477 --- /dev/null +++ b/src/review-metrics.test.ts @@ -0,0 +1,141 @@ +import { 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", + "review-metrics.mjs", +); + +const prompt = (text: string, at: string) => + JSON.stringify({ type: "user", timestamp: at, message: { content: text } }); +const reply = (id: string, model: string, at: string, usage: Record) => + JSON.stringify({ type: "assistant", timestamp: at, message: { id, model, usage, content: [] } }); + +function home(transcripts: Record): string { + const config = mkdtempSync(join(tmpdir(), "review-metrics-")); + for (const [path, lines] of Object.entries(transcripts)) { + const file = join(config, "projects", path); + mkdirSync(dirname(file), { recursive: true }); + writeFileSync(file, `${lines.join("\n")}\n`); + } + return config; +} + +function metrics(config: string, ...args: string[]) { + const run = spawnSync(process.execPath, [SCRIPT, ...args], { + encoding: "utf8", + env: { ...process.env, CLAUDE_CONFIG_DIR: config }, + }); + return { code: run.status, out: run.stdout, err: run.stderr }; +} + +const T0 = "2026-10-19T10:00:00.000Z"; +const T1 = "2026-10-19T10:00:30.000Z"; + +describe("review-metrics.mjs", () => { + it("adds up the runs whose prompt's first line tags the PR, or its issue with --issue, a subagent's included, each API message once, priced per model and cache tier", () => { + const config = home({ + "a/s1.jsonl": [ + prompt("ledger: pr=7 issue=3 stage=review round=1 role=code lens=tests\nreview", T0), + reply("m1", "claude-opus-5-5", T0, { + input_tokens: 1_000_000, + output_tokens: 100_000, + cache_read_input_tokens: 1_000_000, + cache_creation: { ephemeral_5m_input_tokens: 0, ephemeral_1h_input_tokens: 1_000_000 }, + }), + reply("m1", "claude-opus-5-5", T1, { + input_tokens: 1_000_000, + output_tokens: 100_000, + cache_read_input_tokens: 1_000_000, + cache_creation: { ephemeral_5m_input_tokens: 0, ephemeral_1h_input_tokens: 1_000_000 }, + }), + ], + "a/s0/subagents/agent-x.jsonl": [ + prompt("ledger: pr=- issue=3 stage=red round=- role=implementer lens=-\nbuild", T0), + reply("m2", "claude-sonnet-5-5", T1, { + input_tokens: 0, + output_tokens: 0, + cache_read_input_tokens: 1_000_000, + cache_creation_input_tokens: 1_000_000, + }), + ], + "b/other-pr.jsonl": [ + prompt("ledger: pr=8 issue=4 stage=review round=1 role=spec lens=-\nreview", T0), + reply("m3", "claude-opus-5-5", T1, { input_tokens: 5_000_000 }), + ], + "b/untagged.jsonl": [ + prompt("review it, pr=7 issue=3 role=code", T0), + reply("m4", "claude-opus-5-5", T1, { input_tokens: 5_000_000 }), + ], + }); + + const prOnly = metrics(config, "--pr", "7"); + const withIssue = metrics(config, "--pr", "7", "--issue", "3"); + + // why: Opus 4 + 2 (100k out at 20) + 0.2 + 8 (1h write) = 14.2; Sonnet 0.1 + 2.5 (5m write) = 2.6. + expect(prOnly.out).toContain("Runs: 1. Cost USD: 14.2. Agent seconds: 30."); + expect(withIssue.out).toContain("Runs: 2. Cost USD: 16.8. Agent seconds: 60."); + expect(withIssue.out).toContain( + "Tokens input 1000000, output 100000, cache read 2000000, cache write 2000000.", + ); + expect(withIssue.out).toContain("- review 1 code tests: claude-opus-5-5 30 s $14.2"); + expect(withIssue.out).toContain("- red implementer: claude-sonnet-5-5 30 s $2.6"); + }); + + it("reports cost as unknown, never zero, for a model it has no price for and for a PR with no tagged run", () => { + const config = home({ + "a/s1.jsonl": [ + prompt("ledger: pr=7 stage=review role=code\nreview", T0), + reply("m1", "some-new-model", T1, { input_tokens: 10 }), + ], + }); + + expect(metrics(config, "--pr", "7").out).toContain("Runs: 1. Cost USD: unknown."); + expect(metrics(config, "--pr", "9").out).toContain( + "Runs: 0. Cost USD: unknown. Agent seconds: unknown.", + ); + }); + + it("with --findings counts outcomes and classes, and refuses a duplicate that names no canonical finding", () => { + const config = home({}); + const dir = mkdtempSync(join(tmpdir(), "findings-")); + const good = join(dir, "good.json"); + const bad = join(dir, "bad.json"); + const finding = (id: string, outcome: string, more: Record = {}) => ({ + id, + role: "code", + class: "testing 2", + path: "src/a.ts:1", + outcome, + ...more, + }); + writeFileSync( + good, + JSON.stringify({ + findings: [ + finding("R1", "fixed"), + finding("R2", "duplicate", { canonicalId: "R1" }), + finding("R3", "incorrect", { class: "architecture 13" }), + ], + }), + ); + writeFileSync(bad, JSON.stringify({ findings: [finding("R1", "duplicate")] })); + + const ok = metrics(config, "--pr", "7", "--findings", good); + const refused = metrics(config, "--pr", "7", "--findings", bad); + + expect(ok.out).toContain( + 'Finding outcomes: {"duplicate":1,"fixed":1,"incorrect":1}. Classes: {"architecture 13":1,"testing 2":2}.', + ); + expect(refused.code).not.toBe(0); + expect(refused.err).toContain("duplicate R1 must name a canonical finding"); + }); +}); diff --git a/src/tool-guard.test.ts b/src/tool-guard.test.ts index 916182368..3d00d0ad8 100644 --- a/src/tool-guard.test.ts +++ b/src/tool-guard.test.ts @@ -17,11 +17,11 @@ function call(input: object): { status: number | null; stderr: string } { return { status: result.status, stderr: result.stderr }; } -function bash(command: string, agentId: string | null = "a1") { +function bash(command: string, agentId: string | null = "a1", agentType = "reviewer") { return call({ tool_name: "Bash", tool_input: { command }, - ...(agentId && { agent_id: agentId }), + ...(agentId && { agent_id: agentId, agent_type: agentType }), }); } @@ -46,6 +46,9 @@ describe("tool-guard.mjs", () => { "pnpm --workspace check", "pnpm -C . run lint", "(FORCE_COLOR=0 pnpm lint | tail -3)", + "pnpm mutate origin/main", + "node scripts/mutate.mjs origin/main", + "pnpm exec stryker run", ])("refuses a sub-agent's check: %s", (command) => { const result = bash(command); @@ -94,6 +97,15 @@ describe("tool-guard.mjs", () => { expect(bash("pnpm mutate origin/main", null)).toEqual({ status: 0, stderr: "" }); }); + it("lets an implementer run the mutation script, and no tool behind it", () => { + const implementer = (command: string) => bash(command, "a1", "implementer"); + + expect(implementer("pnpm mutate abc123 > m.txt 2>&1")).toEqual({ status: 0, stderr: "" }); + expect(implementer("node scripts/mutate.mjs abc123")).toEqual({ status: 0, stderr: "" }); + expect(implementer("pnpm exec stryker run").status).toBe(2); + expect(implementer("pnpm check").status).toBe(2); + }); + it.each(["implementer", "reviewer", "hardware-verifier"])( "refuses to start a %s in its own worktree", (agent) => { From 33e94bf318d0ffcc91ac465d2db5d19e51bd36ca Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 19 Oct 2026 19:07:47 +0200 Subject: [PATCH 02/15] refactor(agents): split the new scripts' long functions so Fallow's complexity audit passes --- .agents/scripts/recurring-findings.mjs | 24 +++---- .agents/scripts/review-metrics.mjs | 93 +++++++++++++------------- scripts/mutate.mjs | 77 +++++++++++---------- 3 files changed, 96 insertions(+), 98 deletions(-) diff --git a/.agents/scripts/recurring-findings.mjs b/.agents/scripts/recurring-findings.mjs index f50cf3b50..b5319ba2a 100644 --- a/.agents/scripts/recurring-findings.mjs +++ b/.agents/scripts/recurring-findings.mjs @@ -75,22 +75,22 @@ if (recurring.length === 0) console.log(`No class was fixed in ${minPrs} or more function ledgerFindings(pr) { for (let page = 1; ; page++) { const comments = get(`issues/${pr}/comments`, { page }); - for (const { body } of comments) { - const text = (body ?? "").replace(/\r/g, ""); - if (!/^## Review ledger\s*$/m.test(text)) continue; - const block = /```json\n([\s\S]*?)\n```/.exec(text); - if (block === null) return null; - try { - const findings = JSON.parse(block[1]).findings; - return Array.isArray(findings) ? findings : null; - } catch { - return null; - } - } + const ledger = comments.find(({ body }) => /^## Review ledger\s*$/m.test(body ?? "")); + if (ledger !== undefined) return findingsIn(ledger.body.replace(/\r/g, "")); if (comments.length < 100) return null; } } +function findingsIn(text) { + const block = /```json\n([\s\S]*?)\n```/.exec(text); + try { + const findings = block === null ? null : JSON.parse(block[1]).findings; + return Array.isArray(findings) ? findings : null; + } catch { + return null; + } +} + function get(path, fields) { const flags = Object.entries({ per_page: 100, ...fields }).flatMap(([k, v]) => [ "-f", diff --git a/.agents/scripts/review-metrics.mjs b/.agents/scripts/review-metrics.mjs index 8ef2482a5..9850ceba5 100644 --- a/.agents/scripts/review-metrics.mjs +++ b/.agents/scripts/review-metrics.mjs @@ -125,63 +125,61 @@ function tagOf(file) { } function run(file, tag) { - const messages = new Map(); - let startedAt = null; - let finishedAt = null; - for (const line of readFileSync(file, "utf8").split("\n")) { - const rec = parsed(line); - if (rec === null) continue; - if (typeof rec.timestamp === "string") { - startedAt ??= rec.timestamp; - finishedAt = rec.timestamp; - } - const usage = rec.message?.usage; - if (rec.type === "assistant" && usage && rec.message.id) { - messages.set(rec.message.id, { model: rec.message.model, usage }); - } - } + const { messages, startedAt, finishedAt } = read(file); const tokens = { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }; const models = new Set(); let cost = 0; for (const { model, usage } of messages.values()) { models.add(model); - const split = usage.cache_creation; - const write5m = split - ? (split.ephemeral_5m_input_tokens ?? 0) - : (usage.cache_creation_input_tokens ?? 0); - const write1h = split ? (split.ephemeral_1h_input_tokens ?? 0) : 0; - const counts = [ - usage.input_tokens ?? 0, - usage.output_tokens ?? 0, - usage.cache_read_input_tokens ?? 0, - write5m, - write1h, - ]; + const counts = countsOf(usage); tokens.input += counts[0]; tokens.output += counts[1]; tokens.cacheRead += counts[2]; - tokens.cacheWrite += write5m + write1h; - const price = PRICES[model]; - cost = - price === undefined || cost === null - ? null - : cost + counts.reduce((sum, n, i) => sum + n * price[i], 0) / 1e6; + tokens.cacheWrite += counts[3] + counts[4]; + cost = cost === null ? null : priced(model, counts, cost); } + const seconds = + startedAt === null ? null : Math.round((Date.parse(finishedAt) - Date.parse(startedAt)) / 1000); return { file, ...tag, models: [...models], startedAt, finishedAt, - seconds: - startedAt === null - ? null - : Math.round((Date.parse(finishedAt) - Date.parse(startedAt)) / 1000), + seconds, tokens, costUsd: cost === null ? null : Math.round(cost * 1e4) / 1e4, }; } +function read(file) { + const records = readFileSync(file, "utf8").split("\n").map(parsed).filter(Boolean); + const stamps = records.map((r) => r.timestamp).filter((t) => typeof t === "string"); + const messages = new Map(); + for (const { message } of records.filter((r) => r.type === "assistant")) { + if (message?.usage && message.id) messages.set(message.id, message); + } + return { messages, startedAt: stamps[0] ?? null, finishedAt: stamps.at(-1) ?? null }; +} + +// why: PRICES lists its rates in this order: input, output, cache read, 5-minute, 1-hour write. +function countsOf(usage) { + const split = usage.cache_creation; + return [ + usage.input_tokens ?? 0, + usage.output_tokens ?? 0, + usage.cache_read_input_tokens ?? 0, + split ? (split.ephemeral_5m_input_tokens ?? 0) : (usage.cache_creation_input_tokens ?? 0), + split ? (split.ephemeral_1h_input_tokens ?? 0) : 0, + ]; +} + +function priced(model, counts, sofar) { + const price = PRICES[model]; + if (price === undefined) return null; + return sofar + counts.reduce((sum, n, i) => sum + n * price[i], 0) / 1e6; +} + function checked(file) { const list = file.findings; if (!Array.isArray(list)) throw new Error("findings must be an array"); @@ -189,20 +187,21 @@ function checked(file) { for (const f of list) { if (typeof f.id !== "string" || ids.has(f.id)) throw new Error("finding IDs must be unique"); ids.add(f.id); - for (const key of ["role", "class", "path"]) { - if (typeof f[key] !== "string" || f[key] === "") - throw new Error(`finding ${f.id} has no ${key}`); - } - if (!OUTCOMES.includes(f.outcome)) throw new Error(`finding ${f.id} has an invalid outcome`); - } - for (const f of list.filter((x) => x.outcome === "duplicate")) { - if (!list.some((x) => x.id === f.canonicalId && x.outcome !== "duplicate")) { - throw new Error(`duplicate ${f.id} must name a canonical finding`); - } + checkFinding(f, list); } return list; } +function checkFinding(f, list) { + const missing = ["role", "class", "path"].find((k) => typeof f[k] !== "string" || f[k] === ""); + if (missing !== undefined) throw new Error(`finding ${f.id} has no ${missing}`); + if (!OUTCOMES.includes(f.outcome)) throw new Error(`finding ${f.id} has an invalid outcome`); + const canonical = list.some((x) => x.id === f.canonicalId && x.outcome !== "duplicate"); + if (f.outcome === "duplicate" && !canonical) { + throw new Error(`duplicate ${f.id} must name a canonical finding`); + } +} + function promptOf(rec) { const content = rec.message?.content; if (typeof content === "string") return content; diff --git a/scripts/mutate.mjs b/scripts/mutate.mjs index 3c429fbda..fc42f72de 100644 --- a/scripts/mutate.mjs +++ b/scripts/mutate.mjs @@ -153,43 +153,43 @@ print(total, ranges.length, alive); function stryker(mutate, testFiles, incrementalFile) { // why: a killed run leaves its sandboxes behind, and a stale report would be read as this one. rmSync(".stryker-tmp", { force: true, recursive: true }); - const argv = ["exec", "stryker", "run", "--mutate", mutate]; - if (testFiles.length > 0) argv.push("--testFiles", testFiles.join(",")); + const only = testFiles.length > 0 ? ["--testFiles", testFiles.join(",")] : []; + const argv = ["exec", "stryker", "run", "--mutate", mutate, ...only]; argv.push("--incremental", "--incrementalFile", incrementalFile); const run = spawnSync("pnpm", argv, { stdio: ["ignore", "inherit", "pipe"], encoding: "utf8", maxBuffer: 64 * 1024 * 1024, }); - process.stderr.write(run.stderr ?? ""); - if (!existsSync(REPORT)) { - // why: last, so a caller that keeps only the tail of the output still sees the cause. - const tail = (run.stderr ?? "").trimEnd().split("\n").slice(-15).join("\n"); - console.error( - `\nStryker exited ${run.status} without a report: no mutant was tested.\n${tail}`, - ); - process.exit(2); - } + const stderr = run.stderr ?? ""; + process.stderr.write(stderr); + if (!existsSync(REPORT)) failed(run.status, stderr); const report = JSON.parse(readFileSync(REPORT, "utf8")); - const alive = []; - let total = 0; - for (const [path, { mutants }] of Object.entries(report.files)) { - for (const m of mutants) { - total += 1; - if (m.status === "Survived" || m.status === "NoCoverage") { - const { start, end } = m.location; - const replacement = (m.replacement ?? "").replace(/\s+/g, " ").slice(0, 80); - alive.push({ - // why: the report counts columns from 1 and a --mutate range from 0; keeping the end - // as reported widens the range by one column, which can only take this mutant in. - range: `${path}:${start.line}:${start.column - 1}-${end.line}:${end.column}`, - key: `${path}:${start.line}:${start.column}-${end.line}:${end.column} ${m.mutatorName} ${m.replacement}`, - text: `${path}:${start.line} ${m.status} ${m.mutatorName}: ${replacement}`, - }); - } - } - } - return { total, alive }; + const mutants = Object.entries(report.files).flatMap(([path, file]) => + file.mutants.map((m) => ({ path, ...m })), + ); + const alive = mutants + .filter((m) => m.status === "Survived" || m.status === "NoCoverage") + .map(described); + return { total: mutants.length, alive }; +} + +function failed(status, stderr) { + // why: last, so a caller that keeps only the tail of the output still sees the cause. + const tail = stderr.trimEnd().split("\n").slice(-15).join("\n"); + console.error(`\nStryker exited ${status} without a report: no mutant was tested.\n${tail}`); + process.exit(2); +} + +function described({ path, location: { start, end }, mutatorName, replacement, status }) { + const shown = (replacement ?? "").replace(/\s+/g, " ").slice(0, 80); + return { + // why: the report counts columns from 1 and a --mutate range from 0; keeping the end + // as reported widens the range by one column, which can only take this mutant in. + range: `${path}:${start.line}:${start.column - 1}-${end.line}:${end.column}`, + key: `${path}:${start.line}:${start.column}-${end.line}:${end.column} ${mutatorName} ${replacement}`, + text: `${path}:${start.line} ${status} ${mutatorName}: ${shown}`, + }; } // why: a module's own tests reach it through its index.ts (architecture rule 14), so a test @@ -215,16 +215,15 @@ function nearTests(files) { // why: a barrel may re-export another barrel, so a chain is followed to its files. const exposed = new Map(); const exposes = (file) => { - if (!exposed.has(file)) { - exposed.set(file, new Set()); - for (const [, s] of source(file).matchAll(reExport)) { - const target = resolve(file, s); - if (target === undefined) continue; - exposed.get(file).add(target); - for (const t of exposes(target)) exposed.get(file).add(t); - } + if (exposed.has(file)) return exposed.get(file); + const found = new Set(); + exposed.set(file, found); + const targets = [...source(file).matchAll(reExport)].map(([, s]) => resolve(file, s)); + for (const target of targets.filter((t) => t !== undefined)) { + found.add(target); + exposes(target).forEach((t) => found.add(t)); } - return exposed.get(file); + return found; }; const reaches = (test, imported) => targets.has(imported) || From ee4075e7fd94d7d96653c5916a8de49131ad5c49 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 19 Oct 2026 19:09:15 +0200 Subject: [PATCH 03/15] fix(agents): recurring-findings searches and opens issues over REST, which a cloud routine allows --- .claude/skills/recurring-findings/SKILL.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.claude/skills/recurring-findings/SKILL.md b/.claude/skills/recurring-findings/SKILL.md index d75526d63..cf4062402 100644 --- a/.claude/skills/recurring-findings/SKILL.md +++ b/.claude/skills/recurring-findings/SKILL.md @@ -9,6 +9,7 @@ effort: low A defect that reviews keep finding is cheaper to catch with a script than with a reviewer. You open draft tasks; you change no code and open no PR. +Call GitHub through `gh api` REST only: a cloud session refuses GraphQL. ## 1. Count @@ -24,7 +25,7 @@ None: report `Issues: none` and stop. For each class: 1. Skip it when an open issue's title already names it: - `gh issue list --state open --search "Recurring finding: in:title"`. + `gh api -X GET search/issues -f q='repo:/ is:issue is:open in:title "Recurring finding: "' -q .total_count`. 2. Read each listed PR's `## Review ledger` comment and the fix commit of every finding of the class (`git show `): what the code was before the fix, and after. @@ -37,7 +38,8 @@ For each class: 4. Open the task: ```bash - gh issue create --label task:draft --title "Recurring finding: " --body-file /.md + gh api -X POST repos///issues -f title="Recurring finding: " \ + -F body=@/.md -f 'labels[]=task:draft' -q .number ``` The body: the class and the PRs; for each finding its `path:line` and From bba301a85128a8b1f1229f26d031eedbfd7769e3 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 19 Oct 2026 19:43:29 +0200 Subject: [PATCH 04/15] fix(agents): keep the hand probes beside pnpm mutate, which missed every blocking weak-test finding in a replay of 8 --- .agents/briefs/implement-audit.md | 15 ++++++++++----- .agents/briefs/review-code.md | 16 +++++++++------- .agents/scripts/report-guard.mjs | 20 ++++++++++++++++---- .claude/agents/implementer.md | 2 +- src/report-guard.test.ts | 24 ++++++++++++++---------- 5 files changed, 50 insertions(+), 27 deletions(-) diff --git a/.agents/briefs/implement-audit.md b/.agents/briefs/implement-audit.md index 87bae7852..a031a3075 100644 --- a/.agents/briefs/implement-audit.md +++ b/.agents/briefs/implement-audit.md @@ -39,6 +39,12 @@ edited after the red commit: - It proves a claim no other test proves, through the module's surface (testing rules 6 and 7). One that repeats another test's claim is folded into that test. +- Break the code it covers once (flip the condition, drop the filter, delete + the call, move the constant, return early) and run its file: it fails on + the assertion its title names. Restore the code. A test that stays green + is vacuous: fix it. Every test, no skipping: a probe the audit skips, a + reviewer does on a far costlier model. Report `probed

/` on the + `audit:` line: `t` tests added or changed, `p` probed. - A test comparing two outputs also asserts one of them against a literal. **Mutate.** Run `pnpm mutate > /mutants.txt 2>&1` @@ -49,11 +55,10 @@ write one line on why it changes nothing a caller, user, stored record or event can see; in a strict area (`toolchain.md`) only a log line may stay alive. Then put the result in the PR body under `## Mutants`: the summary line, and per mutant still alive its `path:line`, mutator and reason. -Report `mutants alive explained ` on the `audit:` line. -When the script finds no changed source line (a tests-only diff), report -`mutants none` and instead break the code each added or changed test covers -once, run its file, see it fail on the assertion its title names, and -restore the code. +Report `mutants alive explained ` on the `audit:` line, or +`mutants none` when the script finds no changed source line. The script +never deletes a call that takes arguments or is awaited, nor moves a +number: the probes above cover those. Run the "Project checks" in `toolchain.md` that the diff triggers. Then the tests from mode `green` pass, and you push. diff --git a/.agents/briefs/review-code.md b/.agents/briefs/review-code.md index 5efa07d64..88c48ea0a 100644 --- a/.agents/briefs/review-code.md +++ b/.agents/briefs/review-code.md @@ -25,9 +25,8 @@ under "Checks agents never run", and never the slow lane. You may break code to see what a test catches: one edit, one run of its test file, then `git checkout .`. You may also write one new test file in the worktree to prove an interleaving or an input the suite does not cover, run it, and -delete it. Probe budget: four, for behaviour; the tests lens gets four -more only when there is no `mutants.md` (a tests-only diff). Restore the -tree before you report. +delete it. Probe budget: `lens: behaviour` four probes, `lens: tests` six, +`lens: both` nine. Restore the tree before you report. ## Lens: behaviour (`lens: behaviour` or `both`) @@ -51,8 +50,8 @@ ask "what breaks, and for whom?"; no answer means drop it. ## Lens: tests and rules (`lens: tests` or `both`) First list every test the diff adds or changes by line, never by title, -one line per file: `file: 12 read ok, 40 F3`. A file whose tests only -gained a fixture field: `file: fixture only`. +one line per file: `file: 12 read ok, 40 probed ok, 88 probed F3`. A file +whose tests only gained a fixture field: `file: fixture only`. Then answer: @@ -61,8 +60,11 @@ Then answer: see is untested behaviour (testing rule 3). No `mutants.md` on a diff that changes source: `Assessment: incomplete`. 2. For each test: can it fail for the reason its title gives? Read it - against testing rule 10's list. With no `mutants.md`, spend your probes - on the tests most likely to be wired to nothing. + against testing rule 10's list first; those shapes need no probe to + prove. Spend your probes on the tests most likely to be wired to + nothing, on what `pnpm mutate` never tries: delete a call that takes + arguments or is awaited, move a constant to its edge, loosen what the + test compares. A test that stays green is a finding. 3. Does the diff break any rule in `rules/`? Cite the file and the rule number. diff --git a/.agents/scripts/report-guard.mjs b/.agents/scripts/report-guard.mjs index 1d9091186..b0ce946e9 100644 --- a/.agents/scripts/report-guard.mjs +++ b/.agents/scripts/report-guard.mjs @@ -7,10 +7,11 @@ // hardware-verifier PR: # Hardware: ... // // 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 `mutants alive -// explained ` (or `mutants none` when no source line changed) must explain every mutant -// `pnpm mutate` left alive: a test that cannot fail is the most repeated review finding. Other -// agents stop freely. Claude Code ends the turn anyway after eight blocks in a row. +// as well once did the fixes and skipped the audit. That line's `probed

/` must show every +// test it added or changed probed, and its `mutants alive explained ` (or +// `mutants none` when no source line changed) must explain every mutant `pnpm mutate` left +// alive: a test that cannot fail is the most repeated review finding. Other agents stop freely. +// Claude Code ends the turn anyway after eight blocks in a row. import fs from "node:fs"; const REPORT = { @@ -39,6 +40,7 @@ if (!shape.test(message)) { ); } 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]); const mutants = audit === null ? null : /\bmutants (?:none|\d+ alive (\d+) explained (\d+))\b/.exec(audit[0]); if (audit === null) { @@ -46,6 +48,16 @@ if (!shape.test(message)) { "Your report is an audit's but has no `audit:` line, so the audit did not run. Do the " + "audit in implement-audit.md on the whole diff, then report again with its `audit:` line.", ); + } else if (probed === null) { + block( + "Your `audit:` line has no `probed` count. Probe every test you added or changed as " + + "implement-audit.md says, then report again with `probed /`.", + ); + } else if (Number(probed[1]) < Number(probed[2])) { + block( + `You probed ${probed[1]} of ${probed[2]} tests. Probe the rest as implement-audit.md ` + + "says: break the code each covers, run its file, see it fail on its assertion, restore.", + ); } else if (mutants === null) { block( "Your `audit:` line has no `mutants` count. Run `pnpm mutate` as implement-audit.md says, " + diff --git a/.claude/agents/implementer.md b/.claude/agents/implementer.md index c3e01e2f6..06b922af9 100644 --- a/.claude/agents/implementer.md +++ b/.claude/agents/implementer.md @@ -122,7 +122,7 @@ failing: map: -> -audit: stale replaced donewhen / mutants alive explained | mutants none +audit: stale replaced donewhen / probed

/ mutants alive explained | mutants none fixes: -> found fixed run: pass | fail ci: pass | fail | running | unchecked diff --git a/src/report-guard.test.ts b/src/report-guard.test.ts index 193eef28f..33d2aa01b 100644 --- a/src/report-guard.test.ts +++ b/src/report-guard.test.ts @@ -54,22 +54,26 @@ describe("report-guard.mjs", () => { JSON.parse(stop("implementer", `${head}fixes:\n a -> found 1 fixed 1\n`)).reason, ).toContain("no `audit:` line"); expect( - stop("implementer", `${head}audit: stale 0 replaced 0 donewhen 5/5 mutants none\n`), + stop( + "implementer", + `${head}audit: stale 0 replaced 0 donewhen 5/5 probed 4/4 mutants none\n`, + ), ).toBe(""); }); - it("blocks an audit that explained fewer mutants than it left alive, or gives no count", () => { + it("blocks an audit that probed fewer tests than it changed, explained fewer mutants than it left alive, or gives either count", () => { const head = "issue: 472 branch: task/472 pr: 497 mode: audit head: 9d78eb95\ntests: 49/49\n"; - const audit = (mutants: string) => - stop("implementer", `${head}audit: stale 0 replaced 0 donewhen 5/5 ${mutants}\n`); + const audit = (counts: string) => + stop("implementer", `${head}audit: stale 0 replaced 0 donewhen 5/5 ${counts}\n`); + const reason = (counts: string) => JSON.parse(audit(counts)).reason as string; - expect(JSON.parse(audit("mutants 40 alive 3 explained 1")).reason).toContain( - "explained 1 of 3", - ); - expect(JSON.parse(audit("probed 4/4")).reason).toContain("no `mutants` count"); - expect(audit("mutants 40 alive 3 explained 3")).toBe(""); - expect(audit("mutants 40 alive 0 explained 0")).toBe(""); + expect(reason("probed 1/4 mutants none")).toContain("probed 1 of 4"); + expect(reason("mutants none")).toContain("no `probed` count"); + expect(reason("probed 4/4 mutants 40 alive 3 explained 1")).toContain("explained 1 of 3"); + expect(reason("probed 4/4")).toContain("no `mutants` count"); + expect(audit("probed 4/4 mutants 40 alive 3 explained 3")).toBe(""); + expect(audit("probed 0/0 mutants 40 alive 0 explained 0")).toBe(""); }); it("blocks a reviewer whose block belongs to another agent, and lets any other agent stop", () => { From 83b6d87bd2842882340785875ac9481bae2b3fdf Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 19 Oct 2026 19:53:07 +0200 Subject: [PATCH 05/15] =?UTF-8?q?feat(agents):=20build=20one=20test=20at?= =?UTF-8?q?=20a=20time=20=E2=80=94=20a=20build=20mode=20replaces=20red=20a?= =?UTF-8?q?nd=20green,=20each=20Tests=20line=20a=20failing=20test=20commit?= =?UTF-8?q?=20then=20its=20code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .agents/briefs/implement-audit.md | 4 +- .agents/briefs/implement-build.md | 58 ++++++++++++++++++++++++++ .agents/briefs/implement-green.md | 23 ---------- .agents/briefs/implement-red.md | 31 -------------- .agents/briefs/review-spec.md | 4 +- .agents/scripts/delivery-stats.mjs | 4 +- .agents/scripts/review-inputs.sh | 29 ++++++++++--- .agents/scripts/tool-guard.mjs | 7 ++-- .claude/agents/implementer.md | 35 +++++++--------- .claude/skills/deliver/SKILL.md | 7 ++-- .claude/skills/implement/SKILL.md | 2 +- docs/internal/DELIVERY.md | 3 +- docs/internal/agent-rules/toolchain.md | 2 +- src/delivery-stats.test.ts | 11 +++++ src/review-inputs.test.ts | 27 ++++++++++++ src/tool-guard.test.ts | 2 +- 16 files changed, 155 insertions(+), 94 deletions(-) create mode 100644 .agents/briefs/implement-build.md delete mode 100644 .agents/briefs/implement-green.md delete mode 100644 .agents/briefs/implement-red.md diff --git a/.agents/briefs/implement-audit.md b/.agents/briefs/implement-audit.md index a031a3075..083ccc475 100644 --- a/.agents/briefs/implement-audit.md +++ b/.agents/briefs/implement-audit.md @@ -31,7 +31,7 @@ when and assumptions; retain earlier evidence for unaffected claims. under `open`. Never tick a box on intent. **Check every test you added or changed**, including setup or assertions -edited after the red commit: +edited after its `test:` commit: - Its title, assertions and fixture state make the same claim. A title with two claims has an assertion for each. Walk testing rule 10's list against @@ -61,7 +61,7 @@ never deletes a call that takes arguments or is awaited, nor moves a number: the probes above cover those. Run the "Project checks" in `toolchain.md` that the diff triggers. Then the -tests from mode `green` pass, and you push. +tests from mode `build` pass, and you push. **A failing test you did not touch**, locally or in a failed CI run (`gh run view --log-failed`): diff --git a/.agents/briefs/implement-build.md b/.agents/briefs/implement-build.md new file mode 100644 index 000000000..57fe4ecb2 --- /dev/null +++ b/.agents/briefs/implement-build.md @@ -0,0 +1,58 @@ +# Mode `build`: one test, then the code it needs + +Build the issue one Tests line at a time (a bug's triage test already +exists: its line is the first cycle's, with step 1 done). Order the lines: +first the one whose test runs the most of the path the issue builds, from +its seam to its outermost effect; then each next to code already built. +For each line, one cycle: + +1. **Red.** Write the one test the line asks for at the seam it names, or + extend the existing test it names (testing rule 7). A line that lists + cases is one test, its cases a table in its body. Its title is the line's + claim, word for word where it fits. Read it against testing rules 6, 8 + and 10. Run its file: it fails on a named assertion, for the reason the + line gives, not on a typo or a timeout (testing rule 2). Commit the test + alone: `git commit -m "test: (#)"`. A test that + passes before you change code proves behaviour that exists: delete it + when an existing test proves the line, else keep it and list the line + under `open` as `already green: `. +2. **Green.** Make the smallest change that turns it green, and run the + files of every earlier cycle's test: they stay green. Commit: + `(): (#)`. + +Never write the next test before this one is green, and never refactor +between cycles: the audit and the review do that. Changing an earlier +cycle's test to make this one pass is a `spec needs` stop (above). + +After the first cycle, push and open the draft PR: + +```bash +LEFTHOOK_EXCLUDE=fallow git push -u origin / +gh pr create --draft --base --title "():

" --body-file /pr-.md +``` + +The push skips Fallow alone until the last cycle: code a later cycle wires +in is dead code until then. The last cycle's push runs every hook. With a +`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 +per gap); the line `*Written by an agent.*`. Keep it current as you go. + +- Run only the commands under "Tests agents run" in `toolchain.md`. Never + run a check listed under "Checks agents never run", nor a tool behind + it, from any path. A type or lint error shows in the commit hook's + output. +- The commit hook rejects a comment that does not start with `why:` + (architecture rule 17). Delete it; start it with `why:` only when it says + why the code does something a reader would not expect. +- Never wait on CI. Run `node .agents/scripts/ci-failures.mjs ` at most + once, before the report: `none` is `ci: pass`, a failed test `ci: fail`. +- Never use `git stash`: all worktrees share one stash list. Commit instead. + Temporary files go in your scratchpad, not `/tmp`. +- Build what `always-in-scope.md` lists as you go. Anything else you notice + outside the spec becomes a `bug:new` issue or one line under `open`. + +Stop after about ten cycles, or sooner when your context grows long: push, +and list the Tests lines not yet built under `remaining`, in order. A fresh +agent goes on from the branch. diff --git a/.agents/briefs/implement-green.md b/.agents/briefs/implement-green.md deleted file mode 100644 index 9e3d95811..000000000 --- a/.agents/briefs/implement-green.md +++ /dev/null @@ -1,23 +0,0 @@ -# Mode `green`: turn the tests green in checkpoints - -Start from `map`: read the files it names for the next failing test, not -the whole module. Make the smallest change that turns the next test green. -Each commit body states the failing count: `3 failing -> 1 failing`. - -- Run only the commands under "Tests agents run" in `toolchain.md`: the - changed unit tests, and the e2e files you added or edited. Never run a - check listed under "Checks agents never run", nor a tool behind it, from - any path. A type or lint error shows in the commit hook's output. -- The commit hook rejects a comment that does not start with `why:` - (architecture rule 17). Delete it; start it with `why:` only when it says - why the code does something a reader would not expect. -- Push once before your report, in the foreground: commits save progress, - pushes do not need to. Read its whole output, not a tail. Never pass - `--no-verify` or `-n`. -- Never wait on CI: no polling `gh pr checks`, no `gh run watch`, no - `sleep` loops. Run `node .agents/scripts/ci-failures.mjs ` at most - once, before the report: `none` is `ci: pass`, a failed test `ci: fail`. -- Never use `git stash`: all worktrees share one stash list. Commit instead. - Temporary files go in your scratchpad, not `/tmp`. -- Build what `always-in-scope.md` lists as you go. Anything else you notice - outside the spec becomes a `bug:new` issue or one line under `open`. diff --git a/.agents/briefs/implement-red.md b/.agents/briefs/implement-red.md deleted file mode 100644 index 1ac413676..000000000 --- a/.agents/briefs/implement-red.md +++ /dev/null @@ -1,31 +0,0 @@ -# Mode `red`: the spec's tests first - -Write one test per line under Tests (a bug's triage test already exists), -or extend the existing test the line names (testing rule 7). A line that -lists cases is still one test, its cases a table in its body; a line names -one seam, and its claim is tested there only. Each title is -the spec's claim, word for word where it fits. Before you run them, read -each against testing rules 6, 8 and 10: it asserts on the module's output, -not its internals; no log line the user docs do not describe; none of the -shapes that cannot fail. Run them: each -fails on a named assertion, for the reason the spec gives, not on a typo or -a timeout (testing rule 2). Then: - -```bash -git add && git commit -m "test: (#)" -LEFTHOOK_EXCLUDE=fallow git push -u origin / -gh pr create --draft --base --title "(): " --body-file /pr-.md -``` - -The push skips Fallow alone: the placeholders that let the red tests compile -are dead code until green replaces them, and CI's Fallow job reports them -meanwhile. Every other push runs every hook. With a `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 per gap); the line -`*Written by an agent.*`. Keep it current as you go. - -Report the `map`: for each red test, the source files it needs changed, -from what you read while writing it. diff --git a/.agents/briefs/review-spec.md b/.agents/briefs/review-spec.md index 47b6628a8..6c71b13af 100644 --- a/.agents/briefs/review-spec.md +++ b/.agents/briefs/review-spec.md @@ -7,8 +7,8 @@ Apply the precedence and scope in `review-protocol.md` in the inputs. Specification: `issue.md`, `feature.md`, `triage.md` and `spec/`, those that exist. Diff: `diff.patch`. Tests after red: `tests-after-red.patch`, -every change made to the specification's tests after they were first -committed failing. Assumptions: `assumptions.md`, each line a small gap the +every change made to a test file after a `test:` commit first wrote it +failing, but the next cycle's added test. Assumptions: `assumptions.md`, each line a small gap the specification left open, closed by the implementer. `spec/always-in-scope.md` lists what every change includes without its specification asking; treat each item as asked for. `briefs/calibration.md`, when it exists, lists findings diff --git a/.agents/scripts/delivery-stats.mjs b/.agents/scripts/delivery-stats.mjs index 80828f91b..7806d15c2 100755 --- a/.agents/scripts/delivery-stats.mjs +++ b/.agents/scripts/delivery-stats.mjs @@ -174,7 +174,7 @@ function reviewOf(body) { rounds: roundsOf(body, section), tokens: tokensOf(section), rejected: rejectedIn(section), - measured: /^Review checks: (\d+)\. Invocations: (\d+)\. Cost USD: ([\d.]+|unknown)\./m.exec( + measured: /^Runs: (\d+)\. Cost USD: ([\d.]+|unknown)\. Agent seconds: (\d+|unknown)\./m.exec( section, ), }; @@ -279,7 +279,7 @@ function report({ now, prs, handoffs, needsHardware }, weekCount) { "", "## Measured review work (unknown means unavailable; legacy totals remain above)", "", - "| PR | Checks including fix-check | Invocations | Cost USD |", + "| PR | Agent runs | Cost USD | Agent seconds |", "| --- | --: | --: | --: |", ...prs .filter((pr) => inRows(pr, now)) diff --git a/.agents/scripts/review-inputs.sh b/.agents/scripts/review-inputs.sh index 291cb6a01..b1d7f7c47 100755 --- a/.agents/scripts/review-inputs.sh +++ b/.agents/scripts/review-inputs.sh @@ -29,8 +29,9 @@ # and, when there is an issue, always-in-scope.md # rules/ every file in docs/internal/agent-rules/, and the ADR index as # adr-index.md -# tests-after-red.patch what changed in the files of the branch's first `test:` commit (the -# red tests, or a bug's reproduction) between that commit and the head +# tests-after-red.patch each later change to a file a `test:` commit wrote (a build cycle's +# failing test, or a bug's reproduction), but a `test:` commit that +# only adds lines # assumptions.md the PR body's `Assumption:` lines (coding rule 2) # mutants.md the PR body's `## Mutants` section: what `pnpm mutate` left alive at the # audit, and how each was killed or why it changes nothing a caller sees @@ -228,12 +229,30 @@ for n in $adrs; do fi done -# The final diff hides a red test that was deleted or loosened on the way to green. +# why: the final diff hides a test deleted or loosened after it was first committed failing, so +# every later change to a file a `test:` commit wrote goes in, but a later `test:` commit that +# only adds lines: that is the next build cycle's test. red=$(git log --reverse --format='%H %s' "$base..$head" | awk '$2 == "test:" { print $1; exit }') if [ -n "$red" ]; then set -f - git diff "$red" "$head" -- $(git diff-tree --no-commit-id --name-only -r "$red") \ - >"$out/tests-after-red.patch" + written=" " + : >"$out/tests-after-red.patch" + for c in $(git rev-list --reverse --no-merges "$base..$head"); do + files=$(git diff-tree --no-commit-id --name-only -r "$c") + later= + for f in $files; do + case $written in *" $f "*) later="$later $f" ;; esac + done + subject=$(git log -1 --format=%s "$c") + if [ -n "$later" ]; then + removed=$(git diff --numstat "$c^" "$c" -- $later | awk '{ n += $2 } END { print n + 0 }') + case $subject in + test:*) [ "$removed" -eq 0 ] || git show --format='commit %H %s' "$c" -- $later ;; + *) git show --format='commit %H %s' "$c" -- $later ;; + esac >>"$out/tests-after-red.patch" + fi + case $subject in test:*) for f in $files; do written="$written$f "; done ;; esac + done set +f else say "no 'test:' commit on $head_ref: no tests-after-red.patch" diff --git a/.agents/scripts/tool-guard.mjs b/.agents/scripts/tool-guard.mjs index f5fed7e6f..801fdc788 100644 --- a/.agents/scripts/tool-guard.mjs +++ b/.agents/scripts/tool-guard.mjs @@ -6,8 +6,8 @@ // the push hook and CI run them. The main session (a person, a routine) may, and // an implementer may run `pnpm mutate` (its audit and fix modes do). // hooks a sub-agent skips the git hooks: `--no-verify`, `LEFTHOOK=0`, `core.hooksPath`, or -// `LEFTHOOK_EXCLUDE` naming anything but `fallow` (the red stage's one push, whose -// placeholders Fallow calls dead code). +// `LEFTHOOK_EXCLUDE` naming anything but `fallow` (a build stage's push before its +// last cycle, whose code a later cycle wires in Fallow calls dead code). // isolation an `implementer`, `reviewer` or `hardware-verifier` is started with `isolation`. // Every stage of a task works in the task's one worktree, one after another. // @@ -57,7 +57,8 @@ if (tool === "Bash" && typeof input.agent_id === "string") { 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 " + - "red stage may skip Fallow, on its one push: `LEFTHOOK_EXCLUDE=fallow git push ...`.", + "build stage may skip Fallow, on a push before its last cycle: " + + "`LEFTHOOK_EXCLUDE=fallow git push ...`.", ); } const found = forbiddenCheck(shell); diff --git a/.claude/agents/implementer.md b/.claude/agents/implementer.md index 06b922af9..de6eff9c5 100644 --- a/.claude/agents/implementer.md +++ b/.claude/agents/implementer.md @@ -16,24 +16,23 @@ The issue body is the spec. Build exactly what it says, plus what `always-in-scope.md` lists. Do not claim, review, mark ready or merge. Arguments: `issue`, `branch`, `pr` once there is one, `mode`, `base` for a -red stage stacked on another PR's branch; audit/fix inputs also carry +build stage stacked on another PR's branch; audit/fix inputs also carry immutable `base-sha`, `merge-base`, `head`, `previous` and resolution history. -For some modes `map` or +For some modes `remaining` or `fixes`. Anything else is context. Each mode is a fresh agent, so the work runs in short contexts. Steps 1 and 2 are every mode's; step 3 is the mode's own brief in `.agents/briefs/`: -| Mode | Brief | Starts from | Ends with | -| -------------- | --------------------------- | -------------------------------------- | -------------------------------------- | -| `red` | `implement-red.md` | the spec | red commit, draft PR, `map` | -| `green` | `implement-green.md` | the branch and `map` | spec tests green, or `map` of the rest | -| `audit` | `implement-audit.md` | the spec and immutable PR diff | fixes, PR body done | -| `fix` | `implement-fix.md` | the branch and `fixes` | each finding fixed | -| `hardware-fix` | `implement-hardware-fix.md` | the branch and failing slow-lane tests | those tests pass on the lane | - -**Order**, which the caller runs, a fresh agent per mode: `red`; `green` -with the last `map`, again while a report still lists `failing` and turned -a test green since the last; then `audit` with the PR's base, merge-base +| Mode | Brief | Starts from | Ends with | +| -------------- | --------------------------- | --------------------------------------- | -------------------------------------------- | +| `build` | `implement-build.md` | the spec, or the branch and `remaining` | a test and its code per Tests line, draft PR | +| `audit` | `implement-audit.md` | the spec and immutable PR diff | fixes, PR body done | +| `fix` | `implement-fix.md` | the branch and `fixes` | each finding fixed | +| `hardware-fix` | `implement-hardware-fix.md` | the branch and failing slow-lane tests | those tests pass on the lane | + +**Order**, which the caller runs, a fresh agent per mode: `build`, again +with the last `remaining` while a report lists `remaining` and built a +cycle since the last; then `audit` with the PR's base, merge-base and head from `review-inputs.sh`. `fix` applies findings, `hardware-fix` failing slow-lane tests. An agent given `fixes` does them and skips the audit. Every caller stops at `spec needs` under `open`. @@ -97,7 +96,7 @@ and add one line to the PR body's `## Assumptions`: Stop only when building would contradict the body, a rule or an accepted ADR, or would change behaviour a user sees in a way nobody decided. -Changing a spec test after its red commit, or an existing test, so it +Changing a spec test after its `test:` commit, or an existing test, so it asserts behaviour the spec did not decide, is never an assumption: it is a stop. Push what you have and report `spec needs: ` under `open`. Do not work around it. @@ -117,11 +116,9 @@ does not produce. ``` issue: branch: / pr: mode: head: Assessment: complete | incomplete — -tests: / red: -failing: - -map: - -> +tests: / cycles: / +remaining: + audit: stale replaced donewhen / probed

/ mutants alive explained | mutants none fixes: -> found fixed diff --git a/.claude/skills/deliver/SKILL.md b/.claude/skills/deliver/SKILL.md index d2571ed1c..9022644f0 100644 --- a/.claude/skills/deliver/SKILL.md +++ b/.claude/skills/deliver/SKILL.md @@ -18,7 +18,7 @@ rules 1, 2, 3, 9, 14, 15 and 16. whose first line is the ledger tag (`toolchain.md`, "Review measurement"), then `key: value` lines and nothing else: `issue`, `branch`, `pr` once there is one, `mode` or `round`, and what the stage works on, pasted from the last -report (`map`, `fixes`, the reviewer's arguments). Include immutable `base-sha`, `merge-base`, `head` and, after review, +report (`remaining`, `fixes`, the reviewer's arguments). Include immutable `base-sha`, `merge-base`, `head` and, after review, `previous` plus the resolution ledger. No prose, and no reminders of what the agent's own instructions already say. Never pass a model or `isolation`: every stage of a task works in the task's one @@ -135,7 +135,7 @@ gh issue view --json number,title,labels,assignees,body Claim it: `gh issue edit --add-assignee @me`. Its branch is `/`. A task of a feature, or an issue whose feature has an open -ADR PR, is stacked (step 3): its red stage gets `base:` and its PR waits +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` section and the latest `## Handoff` comment, and go on from the next stage they name; never build again or restart review at round 1. @@ -194,6 +194,7 @@ they name; never build again or restart review at round 1. Spec review: blocking, fixed, notes. Code review: blocking, fixed, notes. Claims review: blocking, fixed, notes. Rounds: . Tokens: M. + Rejected: @@ -282,7 +283,7 @@ Runnable: open, `task:ready`, unassigned, and every issue under its Depends on closed or a task of this feature whose PR passed review (open, not a draft). Check the Depends on yourself; no workflow promotes tasks. -- A task's red stage gets `base: `; with no +- A task's build stage gets `base: `; with no PR yet, no `base` (it starts from `main`). - Its review passed (step 2, item 6): `gh pr ready ` and `gh stack link ` (PR numbers, bottom to top; none below yet: nothing to diff --git a/.claude/skills/implement/SKILL.md b/.claude/skills/implement/SKILL.md index 2d2283bb7..87645fb9b 100644 --- a/.claude/skills/implement/SKILL.md +++ b/.claude/skills/implement/SKILL.md @@ -1,6 +1,6 @@ --- name: implement -description: Implement one claimed task or bug tests-first (red, green, audit), or apply review or hardware findings to its branch, through the implementer agent. Use when the user says "implement #N". +description: Implement one claimed task or bug one test at a time (build, audit), or apply review or hardware findings to its branch, through the implementer agent. Use when the user says "implement #N". --- Arguments: $ARGUMENTS diff --git a/docs/internal/DELIVERY.md b/docs/internal/DELIVERY.md index 8b30d4d5e..6c45455d0 100644 --- a/docs/internal/DELIVERY.md +++ b/docs/internal/DELIVERY.md @@ -69,7 +69,8 @@ under Open questions, and the next session starts there. `deliver` writes no code. It hands each stage to a background agent in `.claude/agents/` (`implementer`, `reviewer`, `hardware-verifier`) and reasons only over the report each returns. The implementer works -tests-first, one fresh agent per mode (red, green, audit), and runs only +one test at a time, each committed failing before the code that passes +it, one fresh agent per mode (build, audit), and runs only the tests its change reaches; hooks and CI run every other check ([toolchain.md](agent-rules/toolchain.md)). diff --git a/docs/internal/agent-rules/toolchain.md b/docs/internal/agent-rules/toolchain.md index 27edc69d8..edf345112 100644 --- a/docs/internal/agent-rules/toolchain.md +++ b/docs/internal/agent-rules/toolchain.md @@ -196,7 +196,7 @@ Every prompt a delivery stage or review lens is started with begins with this line, `-` for a key that does not apply yet: ``` -ledger: pr= issue= stage= round=<1|2|fix-check> role= lens= +ledger: pr= issue= stage= round=<1|2|fix-check> role= lens= ``` `node .agents/scripts/review-metrics.mjs --pr --issue [--findings diff --git a/src/delivery-stats.test.ts b/src/delivery-stats.test.ts index 3e2cd8ff1..66a5097ab 100644 --- a/src/delivery-stats.test.ts +++ b/src/delivery-stats.test.ts @@ -35,6 +35,7 @@ const input = { [ "Spec review: 4 findings, 1 fixed. Code review: 2 findings, 2 fixed.", "Rounds: 2. Tokens: 12.5M.", + "Runs: 9. Cost USD: 2.3876. Agent seconds: 284.", "Mutate: 12 mutants, 1 alive.", "", "Rejected:", @@ -239,6 +240,16 @@ describe("delivery-stats.mjs", () => { expect(output).toContain("Medians: 6.0 hours, 120 lines, 1 rounds, 12.5M tokens."); }); + it("lists each PR's agent runs, cost and agent seconds from its review-metrics line, unknown without one", () => { + const rows = run(["--weeks", "2"]) + .split("## Measured review work")[1] + ?.split("\n") + .filter((l) => l.startsWith("| #")); + + expect(rows).toContain("| #10 | 9 | 2.3876 | 284 |"); + expect(rows).toContain("| #11 | unknown | unknown | unknown |"); + }); + it("with --calibration, prints the last four weeks' rejected findings by review, comment ones under claims, and no table", () => { const output = run(["--calibration"]); diff --git a/src/review-inputs.test.ts b/src/review-inputs.test.ts index e70166900..af95ea6ae 100644 --- a/src/review-inputs.test.ts +++ b/src/review-inputs.test.ts @@ -186,6 +186,33 @@ describe("review-inputs.sh over repo-scoped REST", { timeout: 30_000 }, () => { expect(() => inputs(BUG, [], other)).toThrow(/ancestor/); }); + it("puts in tests-after-red.patch every later change to a file a test: commit wrote, but not a later test: commit that only adds", () => { + git(work, "checkout", "-q", "-b", "cycles", "bug/12"); + const commit = (subject: string, path: string, text: string) => { + writeFileSync(join(work, path), text); + git(work, "add", "-A"); + git(work, "commit", "-q", "-m", subject); + }; + commit("test: one", "src/c.test.ts", "assert one exact\n"); + commit("feat: one", "src/c.ts", "one\n"); + commit("test: two", "src/c.test.ts", "assert one exact\nassert two\n"); + commit("feat: two", "src/c.test.ts", "assert one loose\nassert two\n"); + commit("test: three", "src/c.test.ts", "assert one loose\nassert three\n"); + git(work, "push", "-q", "-f", "origin", "cycles:refs/pull/7/head"); + + const patch = inputs(BUG, []).read("tests-after-red.patch") ?? ""; + + git(work, "push", "-q", "-f", "origin", "bug/12:refs/pull/7/head"); + git(work, "checkout", "-q", "bug/12"); + expect(patch).toContain("feat: two"); + expect(patch).toContain("-assert one exact"); + expect(patch).toContain("+assert one loose"); + expect(patch).toContain("test: three"); + expect(patch).toContain("-assert two"); + expect(patch).not.toContain("test: two"); + expect(patch).not.toContain("feat: one"); + }); + it("pins the actual stacked base and limits fix.patch to changes since previous", () => { git(work, "branch", "stack-base", "bug/12"); const previous = execFileSync("git", ["rev-parse", "HEAD"], { diff --git a/src/tool-guard.test.ts b/src/tool-guard.test.ts index 3d00d0ad8..c5174abe6 100644 --- a/src/tool-guard.test.ts +++ b/src/tool-guard.test.ts @@ -86,7 +86,7 @@ describe("tool-guard.mjs", () => { expect(result.stderr).toContain("skips the git hooks"); }); - it("lets the red stage skip Fallow alone on its push", () => { + it("lets a build stage skip Fallow alone on a push", () => { expect(bash("LEFTHOOK_EXCLUDE=fallow git push -u origin task/1")).toEqual({ status: 0, stderr: "", From ff51b1f16fd22db827e0d51517c095f76d77001c Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 19 Oct 2026 20:02:12 +0200 Subject: [PATCH 06/15] fix(agents): a build cycle writes only the code its test needs, one test per commit, and names built-ahead lines --- .agents/briefs/implement-build.md | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/.agents/briefs/implement-build.md b/.agents/briefs/implement-build.md index 57fe4ecb2..d518f6572 100644 --- a/.agents/briefs/implement-build.md +++ b/.agents/briefs/implement-build.md @@ -12,13 +12,16 @@ For each line, one cycle: claim, word for word where it fits. Read it against testing rules 6, 8 and 10. Run its file: it fails on a named assertion, for the reason the line gives, not on a typo or a timeout (testing rule 2). Commit the test - alone: `git commit -m "test: (#)"`. A test that - passes before you change code proves behaviour that exists: delete it - when an existing test proves the line, else keep it and list the line - under `open` as `already green: `. -2. **Green.** Make the smallest change that turns it green, and run the - files of every earlier cycle's test: they stay green. Commit: - `(): (#)`. + alone, one test per commit: `git commit -m "test: +(#)"`. A test that passes before you change code either proves + behaviour the repo had before this issue (delete it when an existing + test proves the line, else keep it and list the line under `open` as + `already green: `), or code an earlier cycle built ahead of its + test: keep it and list the line as `built ahead: `. +2. **Green.** Write only the code this test needs to pass: no branch, + case, field or error path that a later Tests line asks for, even when + you can see it coming. Run the files of every earlier cycle's test: + they stay green. Commit: `(): (#)`. Never write the next test before this one is green, and never refactor between cycles: the audit and the review do that. Changing an earlier From f993a555a2a0cfaf0774c7ed1f7b9d740a67fafc Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 19 Oct 2026 20:08:21 +0200 Subject: [PATCH 07/15] =?UTF-8?q?docs(agents):=20measured=20test=20rules?= =?UTF-8?q?=20=E2=80=94=20no=20new=20test=20for=20as-today=20behaviour,=20?= =?UTF-8?q?no=20tautological=20expected=20value,=20reviews=20ask=20only=20?= =?UTF-8?q?for=20required=20tests,=20internal-file=20tests=20give=20a=20re?= =?UTF-8?q?ason?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .agents/briefs/implement-audit.md | 4 +++- .agents/briefs/review-code.md | 8 +++++--- .agents/briefs/review-spec.md | 8 +++++--- .claude/skills/spec-session/SKILL.md | 4 ++-- docs/internal/agent-rules/testing.md | 23 ++++++++++++++++++----- docs/internal/templates/task.md | 7 ++++--- 6 files changed, 37 insertions(+), 17 deletions(-) diff --git a/.agents/briefs/implement-audit.md b/.agents/briefs/implement-audit.md index 083ccc475..3e22290a5 100644 --- a/.agents/briefs/implement-audit.md +++ b/.agents/briefs/implement-audit.md @@ -38,7 +38,9 @@ edited after its `test:` commit: it. - It proves a claim no other test proves, through the module's surface (testing rules 6 and 7). One that repeats another test's claim is folded - into that test. + into that test. One in a file whose subject the module's `index.ts` does + not export has the `why:` reason rule 6 asks for, or moves to the + surface. - Break the code it covers once (flip the condition, drop the filter, delete the call, move the constant, return early) and run its file: it fails on the assertion its title names. Restore the code. A test that stays green diff --git a/.agents/briefs/review-code.md b/.agents/briefs/review-code.md index 88c48ea0a..f3df6f66f 100644 --- a/.agents/briefs/review-code.md +++ b/.agents/briefs/review-code.md @@ -82,9 +82,11 @@ already supported remain valid; identify unresolved coverage separately. A finding is blocking only when it shows wrong behaviour or state, a false statement, a rule or ADR breach, or a line of the specification left -undelivered or unproven. Testing rules and always-in-scope coverage apply -even when the spec omits a case. Extra coverage beyond those requirements, -a stronger assertion than its claim needs, or defensive code is no +undelivered or unproven. A missing test is a finding only for behaviour a +caller, user, stored record or event can see that the spec, the user docs +or testing rule 3 requires, or for a defect this review found (testing +rule 9). A case that cannot happen, another case of a claim a test already +proves, a stronger assertion than its claim needs, or defensive code is no finding. `F0` is a good result: do not look for findings to have some. diff --git a/.agents/briefs/review-spec.md b/.agents/briefs/review-spec.md index 6c71b13af..f0c27dba6 100644 --- a/.agents/briefs/review-spec.md +++ b/.agents/briefs/review-spec.md @@ -58,9 +58,11 @@ already supported remain valid; identify unresolved coverage separately. A finding is blocking only when it shows wrong behaviour or state, a false statement, a rule or ADR breach, or a line of the specification left -undelivered or unproven. Testing rules and always-in-scope coverage apply -even when the spec omits a case. Extra coverage beyond those requirements, -a stronger assertion than its claim needs, or defensive code is no +undelivered or unproven. A missing test is a finding only for behaviour a +caller, user, stored record or event can see that the spec, the user docs +or testing rule 3 requires, or for a defect this review found (testing +rule 9). A case that cannot happen, another case of a claim a test already +proves, a stronger assertion than its claim needs, or defensive code is no finding. `F0` is a good result: do not look for findings to have some. diff --git a/.claude/skills/spec-session/SKILL.md b/.claude/skills/spec-session/SKILL.md index 3f2ad4eef..06a8e86b4 100644 --- a/.claude/skills/spec-session/SKILL.md +++ b/.claude/skills/spec-session/SKILL.md @@ -252,8 +252,8 @@ the body: - **Ledger.** Every sentence in Scope, In short, Technical spec and Done when that states a behaviour, "as today" included, one row each: the - Tests line and case that prove it, and a wrong build that test would - fail on. A sentence with no row, a case no test covers, or a test no + Tests line and case that prove it, or for "as today" the existing test + that already does, and a wrong build that test would fail on. A sentence with no row, a case no test covers, or a test no wrong build would fail: fix the body. - Every Failure modes row has its Tests line, or says why none; every Shared values row the task touches reads the same in the task. diff --git a/docs/internal/agent-rules/testing.md b/docs/internal/agent-rules/testing.md index 9afe979ea..d00f625a2 100644 --- a/docs/internal/agent-rules/testing.md +++ b/docs/internal/agent-rules/testing.md @@ -77,7 +77,9 @@ prove the behaviour, each at the cheapest level that can prove it what its entry point exports, or through a host surface above it (a command, an HTTP route, a tool call). A test of an internal file earns its place only for pure logic with many cases (a planner, a parser) or - for an ordering the surface cannot force. What the module does to a port + for an ordering the surface cannot force, and says which in a `why:` + comment above its `describe`; any other goes through the surface, or is + deleted when a surface test already proves its claim. What the module does to a port (a driver call, a download, a write) is output, and a fake port's record of it is a fair assertion; the exact sequence of those calls is not, unless the order is the claim. Internal state (a claim, a reservation, a @@ -93,9 +95,11 @@ prove the behaviour, each at the cheapest level that can prove it the cases it lists go in that test's body, as one table when they differ only in data, never one test per case. A contract several implementations share (the worker's and the gateway's handlers of one - operation) is one suite run over each of them. A claim proved at one - level is not proved again at another, nor through a second seam of the - same level: + operation) is one suite run over each of them. Behaviour a change keeps + "as today" is proved by the test that already proves it; a new test + only when none does, or as the control case a new group of tests needs. + A claim proved at one level is not proved again at another, nor through + a second seam of the same level: | Claim | Level | | ---------------------------------------------------------- | --------------------------------------------------- | @@ -115,7 +119,9 @@ prove the behaviour, each at the cheapest level that can prove it finds in a pull request is fixed and proven as rule 7 says: by extending the test that proves the behaviour next to it, and by a new test only when none does; the fix report then names the test it could not extend, - and why. + and why. A review asks for a test only for such a defect, or for + behaviour rule 3 or the spec requires: never for a case that cannot + happen. 10. **The usual ways an assertion cannot fail.** Each of these passes against a broken implementation; check every test you write or change @@ -135,6 +141,13 @@ prove the behaviour, each at the cheapest level that can prove it passes on a rejection; `toBeInstanceOf(Error)` passes on a `TypeError`; `x?.()` passes when nothing was recorded. Assert the exact value, or the named error and its code. + - **An expected value the code under test computed.** `expect(total) + .toBe(blocks * bsize)` with the code's own formula, or a value read + back through the module the test exercises, passes whatever the + formula gets wrong. Take expected values from a literal, a worked + example or the spec. Comparing two outputs of the code is fair only + when the claim is that they match (field order does not matter, routed + equals direct), and one of them is also asserted against a literal. - **State read from a copy taken before the act.** Read it again from the system after. - **A time boundary not straddled.** One advance past both the old and diff --git a/docs/internal/templates/task.md b/docs/internal/templates/task.md index 00c54cb9a..e01169e01 100644 --- a/docs/internal/templates/task.md +++ b/docs/internal/templates/task.md @@ -109,9 +109,10 @@ rule or section, a few quoted words of it, and how the task complies. --> From b3b0dcd1a168d82f8d14619cc10c7b36815fbb22 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 19 Oct 2026 20:09:50 +0200 Subject: [PATCH 08/15] docs(agents): worktree.sh names the build stage --- .agents/scripts/worktree.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.agents/scripts/worktree.sh b/.agents/scripts/worktree.sh index d6d99a221..5de1e7e58 100755 --- a/.agents/scripts/worktree.sh +++ b/.agents/scripts/worktree.sh @@ -12,7 +12,7 @@ # name with a slash is a branch itself (task/118, bug/79): its directory is task-118, and an # existing origin/ is checked out instead of base, so delivery resumes where it stopped. # A branch a worktree already holds gets that worktree back, fast-forwarded to origin/: -# every stage of a task (red, green, audit, fix) works in it, one after another. +# every stage of a task (build, audit, fix) works in it, one after another. # # node_modules of the main checkout is cloned with APFS clonefile(2): no data copied, blocks # shared copy-on-write. pnpm then relinks offline, which takes about a second. Without From 1a325a6d1f9b1da23def4ba47f0c3f5f9dcc38ca Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 19 Oct 2026 20:47:35 +0200 Subject: [PATCH 09/15] =?UTF-8?q?revert(agents):=20mutation=20testing=20le?= =?UTF-8?q?aves=20delivery=20=E2=80=94=20probes=20and=20pnpm=20mutate=20ca?= =?UTF-8?q?ught=202=20of=203=20blocking=20weak=20tests=20at=20about=2015?= =?UTF-8?q?=20min=20a=20file;=20hand=20probes=20stay,=20pnpm=20mutate=20st?= =?UTF-8?q?ays=20weekly?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .agents/briefs/implement-audit.md | 13 ------------- .agents/briefs/review-code.md | 19 ++++++------------ .agents/scripts/report-guard.mjs | 20 +++---------------- .agents/scripts/review-inputs.sh | 11 ----------- .agents/scripts/tool-guard.mjs | 14 ++++--------- .claude/agents/implementer.md | 2 +- docs/internal/REVIEW.md | 5 ++--- docs/internal/agent-rules/testing.md | 3 +-- docs/internal/agent-rules/toolchain.md | 24 ++++++++--------------- src/report-guard.test.ts | 27 +++++++++++++------------- src/review-inputs.test.ts | 21 +------------------- src/tool-guard.test.ts | 9 --------- 12 files changed, 39 insertions(+), 129 deletions(-) diff --git a/.agents/briefs/implement-audit.md b/.agents/briefs/implement-audit.md index 3e22290a5..1ebd1d22c 100644 --- a/.agents/briefs/implement-audit.md +++ b/.agents/briefs/implement-audit.md @@ -49,19 +49,6 @@ edited after its `test:` commit: `audit:` line: `t` tests added or changed, `p` probed. - A test comparing two outputs also asserts one of them against a literal. -**Mutate.** Run `pnpm mutate > /mutants.txt 2>&1` -(after fixes: the same command; it reuses every unchanged result). Exit 2: -`Assessment: incomplete`. For each mutant it lists alive, either kill it -(extend the test that proves the behaviour next to it, testing rule 7) or -write one line on why it changes nothing a caller, user, stored record or -event can see; in a strict area (`toolchain.md`) only a log line may stay -alive. Then put the result in the PR body under `## Mutants`: the summary -line, and per mutant still alive its `path:line`, mutator and reason. -Report `mutants alive explained ` on the `audit:` line, or -`mutants none` when the script finds no changed source line. The script -never deletes a call that takes arguments or is awaited, nor moves a -number: the probes above cover those. - Run the "Project checks" in `toolchain.md` that the diff triggers. Then the tests from mode `build` pass, and you push. diff --git a/.agents/briefs/review-code.md b/.agents/briefs/review-code.md index f3df6f66f..bf5d0cae5 100644 --- a/.agents/briefs/review-code.md +++ b/.agents/briefs/review-code.md @@ -7,9 +7,7 @@ the code. In the inputs directory: the specification (`issue.md`, `feature.md`, `triage.md` and `spec/`, those that exist), so you know what the code is meant to do; `rules/`, every rule file but `delivery.md`, and the ADR index; -`diff.patch`; `mutants.md`, when it exists, what `pnpm mutate` left alive on -the changed lines and the implementer's reason for each; -`briefs/calibration.md`, when it exists, findings like yours that were +`diff.patch`; `briefs/calibration.md`, when it exists, findings like yours that were rejected lately: read it first. Repository: the worktree you were given, checked out at the reviewed commit. @@ -55,21 +53,16 @@ whose tests only gained a fixture field: `file: fixture only`. Then answer: -1. For each mutant `mutants.md` lists alive: does its reason hold? Read the - code: a mutant whose change a caller, user, stored record or event can - see is untested behaviour (testing rule 3). No `mutants.md` on a diff - that changes source: `Assessment: incomplete`. -2. For each test: can it fail for the reason its title gives? Read it +1. For each test: can it fail for the reason its title gives? Read it against testing rule 10's list first; those shapes need no probe to prove. Spend your probes on the tests most likely to be wired to - nothing, on what `pnpm mutate` never tries: delete a call that takes - arguments or is awaited, move a constant to its edge, loosen what the + nothing: delete a call, move a constant to its edge, loosen what the test compares. A test that stays green is a finding. -3. Does the diff break any rule in `rules/`? Cite the file and the rule +2. Does the diff break any rule in `rules/`? Cite the file and the rule number. -Blocking: a survivor's reason that does not hold, a test that cannot fail -for the reason its title gives, or a breach of a rule or an accepted ADR. +Blocking: a test that cannot fail for the reason its title gives, or a +breach of a rule or an accepted ADR. ## Assessment diff --git a/.agents/scripts/report-guard.mjs b/.agents/scripts/report-guard.mjs index b0ce946e9..e1186f437 100644 --- a/.agents/scripts/report-guard.mjs +++ b/.agents/scripts/report-guard.mjs @@ -8,10 +8,9 @@ // // 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, and its `mutants alive explained ` (or -// `mutants none` when no source line changed) must explain every mutant `pnpm mutate` left -// alive: a test that cannot fail is the most repeated review finding. Other agents stop freely. -// Claude Code ends the turn anyway after eight blocks in a row. +// 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. import fs from "node:fs"; const REPORT = { @@ -41,8 +40,6 @@ if (!shape.test(message)) { } 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]); - const mutants = - audit === null ? null : /\bmutants (?:none|\d+ alive (\d+) explained (\d+))\b/.exec(audit[0]); if (audit === null) { block( "Your report is an audit's but has no `audit:` line, so the audit did not run. Do the " + @@ -58,17 +55,6 @@ if (!shape.test(message)) { `You probed ${probed[1]} of ${probed[2]} tests. Probe the rest as implement-audit.md ` + "says: break the code each covers, run its file, see it fail on its assertion, restore.", ); - } else if (mutants === null) { - block( - "Your `audit:` line has no `mutants` count. Run `pnpm mutate` as implement-audit.md says, " + - "then report again with `mutants alive explained `, or " + - "`mutants none` when it found no source line to mutate.", - ); - } else if (mutants[1] !== undefined && Number(mutants[2]) < Number(mutants[1])) { - block( - `You explained ${mutants[2]} of ${mutants[1]} mutants left alive. Kill each of the rest ` + - "with a test, or write why it changes nothing a caller can see, as implement-audit.md says.", - ); } } diff --git a/.agents/scripts/review-inputs.sh b/.agents/scripts/review-inputs.sh index b1d7f7c47..91b87334d 100755 --- a/.agents/scripts/review-inputs.sh +++ b/.agents/scripts/review-inputs.sh @@ -33,8 +33,6 @@ # failing test, or a bug's reproduction), but a `test:` commit that # only adds lines # assumptions.md the PR body's `Assumption:` lines (coding rule 2) -# mutants.md the PR body's `## Mutants` section: what `pnpm mutate` left alive at the -# audit, and how each was killed or why it changes nothing a caller sees # # Nothing else: never the rest of the PR body, commit messages, or any other comment. Rules, ADRs # and briefs come from the base branch, so a PR cannot rewrite what it is judged by. Whatever it looks @@ -198,15 +196,6 @@ else say "PR #$pr closes no issue (branch $head_ref, no 'Closes #n' in the body)" fi -printf '%s\n' "$pr_body" | awk ' - /^## / { inside = ($0 ~ /^## Mutants[ \t]*$/); next } - inside { print } -' >"$out/mutants.md" -grep -q . "$out/mutants.md" || { - rm -f "$out/mutants.md" - say "no ## Mutants section in the PR body" -} - # ADRs by number, wherever the spec names them, or wherever the record an ADR diff adds or edits # names them: "ADR 0014", "ADR-0014", "ADRs 0004 and 0005", or a link to adr/0014-....md. Each # comes as the base has it; one this diff adds is in diff.patch alone. diff --git a/.agents/scripts/tool-guard.mjs b/.agents/scripts/tool-guard.mjs index 801fdc788..9c8081558 100644 --- a/.agents/scripts/tool-guard.mjs +++ b/.agents/scripts/tool-guard.mjs @@ -3,8 +3,7 @@ // // check a sub-agent runs a check from toolchain.md's "Checks agents never run" (`pnpm // check`, `pnpm fallow`, ...) or a tool behind one, from any path. The commit hook, -// the push hook and CI run them. The main session (a person, a routine) may, and -// an implementer may run `pnpm mutate` (its audit and fix modes do). +// the push hook and CI run them. The main session (a person, a routine) may. // hooks a sub-agent skips the git hooks: `--no-verify`, `LEFTHOOK=0`, `core.hooksPath`, or // `LEFTHOOK_EXCLUDE` naming anything but `fallow` (a build stage's push before its // last cycle, whose code a later cycle wires in Fallow calls dead code). @@ -26,9 +25,10 @@ const SCRIPTS = new Set([ "format", "format:check", "fallow", + "mutate", ]); const TOOLS = new Set(["tsc", "oxlint", "depcruise", "oxfmt", "fallow", "stryker"]); -const NODE_SCRIPTS = new Set(["comment-lint.mjs"]); +const NODE_SCRIPTS = new Set(["comment-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"]); @@ -41,7 +41,6 @@ const HOOK_BYPASS = [ const input = JSON.parse(fs.readFileSync(0, "utf8") || "{}"); const tool = input.tool_name; -const mayMutate = input.agent_type === "implementer"; const args = input.tool_input ?? {}; if ((tool === "Agent" || tool === "Task") && args.isolation && STAGES.has(args.subagent_type)) { @@ -100,11 +99,7 @@ function isForbidden(words) { const [first, ...rest] = words; const name = path.basename(first); if (TOOLS.has(name) || NODE_SCRIPTS.has(name)) return true; - if (name === "mutate.mjs") return !mayMutate; - if (name === "node") { - if (rest.some((w) => path.basename(w) === "mutate.mjs")) return !mayMutate; - return rest.some((w) => NODE_SCRIPTS.has(path.basename(w))); - } + if (name === "node") return rest.some((w) => NODE_SCRIPTS.has(path.basename(w))); if (name === "npx" || name === "bunx") return isTool(firstOperand(rest)); return name === "pnpm" && isForbiddenPnpm(rest); } @@ -118,7 +113,6 @@ function isForbiddenPnpm(args) { // why: these scripts are allowed only in their narrow forms: named e2e files, Chromium only. if (sub === "test:e2e") return after.length === 0; if (sub === "test:console") return !args.includes("--project=chromium"); - if (sub === "mutate") return !mayMutate; return SCRIPTS.has(sub); } diff --git a/.claude/agents/implementer.md b/.claude/agents/implementer.md index de6eff9c5..8ce77bdc7 100644 --- a/.claude/agents/implementer.md +++ b/.claude/agents/implementer.md @@ -119,7 +119,7 @@ Assessment: complete | incomplete — tests: / cycles: / remaining: -audit: stale replaced donewhen / probed

/ mutants alive explained | mutants none +audit: stale replaced donewhen / probed

/ fixes: -> found fixed run: pass | fail ci: pass | fail | running | unchecked diff --git a/docs/internal/REVIEW.md b/docs/internal/REVIEW.md index b2107af47..72c81f9f6 100644 --- a/docs/internal/REVIEW.md +++ b/docs/internal/REVIEW.md @@ -49,9 +49,8 @@ without that assessment leave the finding unresolved, never rejected. Select independent lenses by work, not size alone. ADR-only: spec. Docs or comments: spec and claims. Tests: spec, code/tests and affected claims. Other changes: spec and code/both, plus affected claims. No issue: skip spec. -Add a separate code/tests lens for complex fixtures, many survivors in -`mutants.md` or high-risk coverage; split behavior and tests -responsibilities explicitly. +Add a separate code/tests lens for complex fixtures or high-risk coverage; +split behavior and tests responsibilities explicitly. A large mechanical diff alone does not warrant two code reviewers. Record why a lens was added or skipped. Run selected lenses in parallel, with separate worktrees for probes; do not share findings until synthesis. diff --git a/docs/internal/agent-rules/testing.md b/docs/internal/agent-rules/testing.md index d00f625a2..7d33e9d11 100644 --- a/docs/internal/agent-rules/testing.md +++ b/docs/internal/agent-rules/testing.md @@ -49,8 +49,7 @@ prove the behaviour, each at the cheapest level that can prove it of the part that matters — usually because the test exercises an internal callback rather than the observable output the path exists to produce. Write the test that goes red, then restore the code. Mutation - testing does this mechanically for every pull request's changed lines, - and weekly for the newest changes on `main` + testing does this mechanically, weekly, for the newest changes on `main` ([toolchain.md](toolchain.md) says where it runs). 4. **A test that enforces a rule must see everything the rule covers.** A diff --git a/docs/internal/agent-rules/toolchain.md b/docs/internal/agent-rules/toolchain.md index edf345112..4446e61ac 100644 --- a/docs/internal/agent-rules/toolchain.md +++ b/docs/internal/agent-rules/toolchain.md @@ -22,8 +22,7 @@ Hooks and CI run these. Never run them by hand, and never the tools behind them (from `node_modules/.bin` or any other path): `pnpm check`, `pnpm test`, `pnpm test:timing`, `pnpm typecheck`, `pnpm typecheck:e2e`, `pnpm lint`, `pnpm lint:comments`, `pnpm format`, `pnpm format:check`, -`pnpm fallow`, `pnpm mutate` (outside the implementer and the `harden-tests` -routine), the whole +`pnpm fallow`, `pnpm mutate` (outside the `harden-tests` routine), the whole e2e suite, the console lane in every browser (`pnpm test:console`), `tsc`, `oxlint`, `depcruise`, `oxfmt`, `fallow`, `stryker`, `.agents/scripts/comment-lint.mjs` (outside the `garden-comments` routine). A PreToolUse hook @@ -44,20 +43,13 @@ list and this one the same. ## Mutation testing -`pnpm mutate ` mutates the source lines changed since `` and -lists each mutant the unit suite let live. It first runs only the test -files that import a changed file, or reach it through their own module's -`index.ts`, or that the branch changed; a mutant those leave alive gets a -second run with every related test file. It reuses each unchanged result, -so a fix run pays only for what changed. - -- **Delivery:** the implementer's audit runs it on the PR's diff (testing - rule 2) and writes what stays alive, with a reason each, under - `## Mutants` in the PR body; the code review checks those reasons. -- **Weekly:** the `harden-tests` routine runs `pnpm mutate - --max-lines 1500` on `main`, newest files first up to that many lines, - kills the survivors that change behaviour a caller or user can see, and - opens one tests-only PR. +Not part of delivery. The weekly `harden-tests` routine runs `pnpm mutate + --max-lines 1500` on `main` for the source lines changed since +``, newest files first up to that many lines, kills the survivors that +change behaviour a caller or user can see, and opens one tests-only PR. The +script first runs only the test files that import a changed file, or reach +it through their own module's `index.ts`, or that the branch changed; a +mutant those leave alive gets a second run with every related test file. ## Routines diff --git a/src/report-guard.test.ts b/src/report-guard.test.ts index 33d2aa01b..893165ae2 100644 --- a/src/report-guard.test.ts +++ b/src/report-guard.test.ts @@ -54,26 +54,25 @@ describe("report-guard.mjs", () => { JSON.parse(stop("implementer", `${head}fixes:\n a -> found 1 fixed 1\n`)).reason, ).toContain("no `audit:` line"); expect( - stop( - "implementer", - `${head}audit: stale 0 replaced 0 donewhen 5/5 probed 4/4 mutants none\n`, - ), + stop("implementer", `${head}audit: stale 0 replaced 0 donewhen 5/5 probed 4/4\n`), ).toBe(""); }); - it("blocks an audit that probed fewer tests than it changed, explained fewer mutants than it left alive, or gives either count", () => { + it("blocks an audit that probed fewer tests than it added or changed, or gives no count", () => { const head = "issue: 472 branch: task/472 pr: 497 mode: audit head: 9d78eb95\ntests: 49/49\n"; - const audit = (counts: string) => - stop("implementer", `${head}audit: stale 0 replaced 0 donewhen 5/5 ${counts}\n`); - const reason = (counts: string) => JSON.parse(audit(counts)).reason as string; - expect(reason("probed 1/4 mutants none")).toContain("probed 1 of 4"); - expect(reason("mutants none")).toContain("no `probed` count"); - expect(reason("probed 4/4 mutants 40 alive 3 explained 1")).toContain("explained 1 of 3"); - expect(reason("probed 4/4")).toContain("no `mutants` count"); - expect(audit("probed 4/4 mutants 40 alive 3 explained 3")).toBe(""); - expect(audit("probed 0/0 mutants 40 alive 0 explained 0")).toBe(""); + expect( + JSON.parse( + stop("implementer", `${head}audit: stale 0 replaced 0 donewhen 5/5 probed 1/4\n`), + ).reason, + ).toContain("probed 1 of 4"); + expect( + JSON.parse(stop("implementer", `${head}audit: stale 0 replaced 0 donewhen 5/5\n`)).reason, + ).toContain("no `probed` count"); + expect( + stop("implementer", `${head}audit: stale 0 replaced 0 donewhen 5/5 probed 0/0\n`), + ).toBe(""); }); it("blocks a reviewer whose block belongs to another agent, and lets any other agent stop", () => { diff --git a/src/review-inputs.test.ts b/src/review-inputs.test.ts index af95ea6ae..a7c102c75 100644 --- a/src/review-inputs.test.ts +++ b/src/review-inputs.test.ts @@ -91,13 +91,12 @@ function inputs( comments: readonly { body: string }[], previous?: string, base = "main", - prBody: string | null = null, ) { const file = join(mkdtempSync(join(root, "gh-")), "github.json"); writeFileSync( file, JSON.stringify({ - "repos/o/r/pulls/7": { head: { ref: "bug/12" }, base: { ref: base }, body: prBody }, + "repos/o/r/pulls/7": { head: { ref: "bug/12" }, base: { ref: base }, body: null }, "repos/o/r/issues/12": issue, "repos/o/r/issues/12/comments": comments, "repos/o/r/issues/5": { body: "Feature\r\nfive." }, @@ -127,24 +126,6 @@ function inputs( const BUG = { body: "The bug.", labels: [{ name: "bug:ready" }], parent_issue_url: null }; describe("review-inputs.sh over repo-scoped REST", { timeout: 30_000 }, () => { - it("writes the PR body's Mutants section to mutants.md and no other part of the body", () => { - const body = [ - "Closes #12", - "## Mutants", - "40 mutants, 1 alive:", - "- src/a.ts:3 ConditionalExpression: equivalent, the branch only logs", - "## Review", - "Spec review: skipped.", - ].join("\r\n"); - - const { read } = inputs(BUG, [], undefined, "main", body); - - expect(read("mutants.md")).toBe( - "40 mutants, 1 alive:\n- src/a.ts:3 ConditionalExpression: equivalent, the branch only logs\n", - ); - expect(inputs(BUG, []).read("mutants.md")).toBeNull(); - }); - it("writes the issue the branch names, and its parent from parent_issue_url", () => { const parented = { ...BUG, parent_issue_url: "https://api.github.com/repos/o/r/issues/5" }; const result = inputs(parented, []); diff --git a/src/tool-guard.test.ts b/src/tool-guard.test.ts index c5174abe6..f00b57fe6 100644 --- a/src/tool-guard.test.ts +++ b/src/tool-guard.test.ts @@ -97,15 +97,6 @@ describe("tool-guard.mjs", () => { expect(bash("pnpm mutate origin/main", null)).toEqual({ status: 0, stderr: "" }); }); - it("lets an implementer run the mutation script, and no tool behind it", () => { - const implementer = (command: string) => bash(command, "a1", "implementer"); - - expect(implementer("pnpm mutate abc123 > m.txt 2>&1")).toEqual({ status: 0, stderr: "" }); - expect(implementer("node scripts/mutate.mjs abc123")).toEqual({ status: 0, stderr: "" }); - expect(implementer("pnpm exec stryker run").status).toBe(2); - expect(implementer("pnpm check").status).toBe(2); - }); - it.each(["implementer", "reviewer", "hardware-verifier"])( "refuses to start a %s in its own worktree", (agent) => { From d771ca60f0333f44574e5f4fd050662f1c54c9e8 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 19 Oct 2026 21:15:53 +0200 Subject: [PATCH 10/15] docs(agents): build mode describes the tracer bullet, lets a test commit add the stub it calls, and names Fallow as the refactoring check Replayed on #491: the stub let the first test fail on its assertion; the first green shrank from 66 to 39 lines. Built-ahead lines stayed at 8 of 10. --- .agents/briefs/implement-build.md | 35 +++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/.agents/briefs/implement-build.md b/.agents/briefs/implement-build.md index d518f6572..9915a2f9d 100644 --- a/.agents/briefs/implement-build.md +++ b/.agents/briefs/implement-build.md @@ -1,18 +1,33 @@ # Mode `build`: one test, then the code it needs Build the issue one Tests line at a time (a bug's triage test already -exists: its line is the first cycle's, with step 1 done). Order the lines: -first the one whose test runs the most of the path the issue builds, from -its seam to its outermost effect; then each next to code already built. +exists: its line is the first cycle's, with step 1 done). + +**Tracer bullet first.** The first cycle takes the line whose test crosses +the most of the path the issue builds, from its seam to its outermost +effect: the CLI command through the daemon to the stored record, not the +parser alone. Its green builds every layer on that path, each only as thick +as this one test needs: one case, no options, no error path. It proves the +layers connect before any of them grows, so a wrong seam or a missing wire +shows in cycle one, not in the last. Each next cycle takes the line next to +code already built and widens it by one case, field or error path, using +what the earlier cycles showed. Never build a layer ahead of the test that +reaches it, and never write the tests in bulk first: tests written before +the code check an imagined shape. + For each line, one cycle: 1. **Red.** Write the one test the line asks for at the seam it names, or extend the existing test it names (testing rule 7). A line that lists cases is one test, its cases a table in its body. Its title is the line's claim, word for word where it fits. Read it against testing rules 6, 8 - and 10. Run its file: it fails on a named assertion, for the reason the - line gives, not on a typo or a timeout (testing rule 2). Commit the test - alone, one test per commit: `git commit -m "test: + and 10. The commit hook typechecks the whole project, so a test that + calls code not yet written cannot commit: add only the stub it calls, + its real signature returning the type's emptiest value (`[]`, `0`, + `undefined`, or doing nothing), never the logic. Run its file: it fails + on a named assertion, for the reason the line gives, not on a typo, a + thrown stub or a timeout (testing rule 2). Commit the test and its + stubs, one test per commit: `git commit -m "test: (#)"`. A test that passes before you change code either proves behaviour the repo had before this issue (delete it when an existing test proves the line, else keep it and list the line under `open` as @@ -24,7 +39,9 @@ For each line, one cycle: they stay green. Commit: `(): (#)`. Never write the next test before this one is green, and never refactor -between cycles: the audit and the review do that. Changing an earlier +between cycles: the last cycle's push runs Fallow's duplication and +complexity checks on everything the cycles added, and you fix what they +flag then. Changing an earlier cycle's test to make this one pass is a `spec needs` stop (above). After the first cycle, push and open the draft PR: @@ -34,8 +51,8 @@ LEFTHOOK_EXCLUDE=fallow git push -u origin / gh pr create --draft --base --title "():

" --body-file /pr-.md ``` -The push skips Fallow alone until the last cycle: code a later cycle wires -in is dead code until then. The last cycle's push runs every hook. With a +The push skips Fallow alone until the last cycle: stubs and code a later +cycle wires in are dead code until then. The last cycle's push runs every hook. With a `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 From 0b0218035437b90e6e8a3efbd55d93d17b0e1561 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 19 Oct 2026 21:34:20 +0200 Subject: [PATCH 11/15] docs(agents): a green cycle writes general logic and no placeholder for an unasserted field; the audit's surface check covers a test importing an internal file that index.ts also exports Measured: #408 and #411 shipped a hard-coded 0 for a field no test asserted; 73 of 572 new tests in 20 PRs imported an internal file, mostly for a name index.ts exports, and the audit's narrower wording passed them. --- .agents/briefs/implement-audit.md | 6 +++--- .agents/briefs/implement-build.md | 5 ++++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/.agents/briefs/implement-audit.md b/.agents/briefs/implement-audit.md index 1ebd1d22c..7fa52ec03 100644 --- a/.agents/briefs/implement-audit.md +++ b/.agents/briefs/implement-audit.md @@ -38,9 +38,9 @@ edited after its `test:` commit: it. - It proves a claim no other test proves, through the module's surface (testing rules 6 and 7). One that repeats another test's claim is folded - into that test. One in a file whose subject the module's `index.ts` does - not export has the `why:` reason rule 6 asks for, or moves to the - surface. + into that test. One that imports a module's internal file instead of its + `index.ts` has the `why:` reason rule 6 asks for, or moves to the + surface, even when `index.ts` exports the same name. - Break the code it covers once (flip the condition, drop the filter, delete the call, move the constant, return early) and run its file: it fails on the assertion its title names. Restore the code. A test that stays green diff --git a/.agents/briefs/implement-build.md b/.agents/briefs/implement-build.md index 9915a2f9d..00958df48 100644 --- a/.agents/briefs/implement-build.md +++ b/.agents/briefs/implement-build.md @@ -35,7 +35,10 @@ For each line, one cycle: test: keep it and list the line as `built ahead: `. 2. **Green.** Write only the code this test needs to pass: no branch, case, field or error path that a later Tests line asks for, even when - you can see it coming. Run the files of every earlier cycle's test: + you can see it coming. What you write is the general logic the spec + describes, never a value or branch that matches only the test's input; + a field the type needs but no Tests line asserts is computed from + state, never a placeholder (`ready: 0`). Run the files of every earlier cycle's test: they stay green. Commit: `(): (#)`. Never write the next test before this one is green, and never refactor From 57cac9fda75bc71a0b28e1cce72e59a9758bdaf6 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 19 Oct 2026 21:49:18 +0200 Subject: [PATCH 12/15] docs(agents): the audit probes each claim in a test's title and swaps a rule for a near one; a red by timeout does not count Measured on 20 verified weak-test findings review raised after an audit: 5 needed a near-rule swap (another sort key, no tie-break, first not lowest, one side of ||), 2 a second claim in the title, 1 a timeout-only red. --- .agents/briefs/implement-audit.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/.agents/briefs/implement-audit.md b/.agents/briefs/implement-audit.md index 7fa52ec03..97dd11f37 100644 --- a/.agents/briefs/implement-audit.md +++ b/.agents/briefs/implement-audit.md @@ -41,9 +41,12 @@ edited after its `test:` commit: into that test. One that imports a module's internal file instead of its `index.ts` has the `why:` reason rule 6 asks for, or moves to the surface, even when `index.ts` exports the same name. -- Break the code it covers once (flip the condition, drop the filter, delete - the call, move the constant, return early) and run its file: it fails on - the assertion its title names. Restore the code. A test that stays green +- Break the code it covers once per claim in its title (flip the + condition, drop the filter, delete the call, move the constant either + way, return early, or swap a rule for a near one: sort by another key, + drop the tie-break, take the first instead of the lowest, drop one side + of an `||` or `&&`) and run its file: it fails on the assertion its + title names, not by timing out. Restore the code. A test that stays green is vacuous: fix it. Every test, no skipping: a probe the audit skips, a reviewer does on a far costlier model. Report `probed

/` on the `audit:` line: `t` tests added or changed, `p` probed. From 7bfb519fb4ccec10da9fbd88edf032a79980a752 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 19 Oct 2026 21:56:36 +0200 Subject: [PATCH 13/15] feat(agents): a script checks that a contract shape change and a protocol bump go together, on push and in CI Replayed: it flags all 7 past misses (#394, #494, #495, #499, #563, #567, #736) and passes their fixes; on the main merges touching src/contract since the rule (#390) it flags only #524 and #528, the stack that bumped once. CI runs on the PR's merge commit, so two PRs taking the same number fail there. --- .agents/scripts/protocol-bump-check.mjs | 69 ++++++++++++++++++ .github/workflows/ci.yml | 22 ++++++ docs/internal/agent-rules/toolchain.md | 6 +- lefthook.yml | 4 + src/contract/AGENTS.md | 5 +- src/protocol-bump-check.test.ts | 97 +++++++++++++++++++++++++ 6 files changed, 201 insertions(+), 2 deletions(-) create mode 100644 .agents/scripts/protocol-bump-check.mjs create mode 100644 src/protocol-bump-check.test.ts diff --git a/.agents/scripts/protocol-bump-check.mjs b/.agents/scripts/protocol-bump-check.mjs new file mode 100644 index 000000000..32305d8cc --- /dev/null +++ b/.agents/scripts/protocol-bump-check.mjs @@ -0,0 +1,69 @@ +#!/usr/bin/env node +// `node .agents/scripts/protocol-bump-check.mjs [--base ] [--head ]`: the mechanical +// half of the contract-shape rule in toolchain.md. Since (default HEAD) left +// (default origin/main), a code line changed in a contract shape file under src/contract/ and +// a change to PROTOCOL_VERSION_RANGE in src/contract/protocol.ts go together: one without the +// other is printed and the exit code is 1. Shape files are every src/contract/*.ts but tests, +// index.ts and the helpers in NOT_SHAPES. A commit in the range with the trailer +// `Protocol-bump: none ()` lets a contract edit that changes no wire shape through (a +// refactor, a client-only error code); whether the reason holds is the review's question. +import { execFileSync } from "node:child_process"; + +const PROTOCOL = "src/contract/protocol.ts"; +const NOT_SHAPES = new Set(["index.ts", "console-url.ts", "duration.ts", "os-range.ts"]); +const RANGE = /PROTOCOL_VERSION_RANGE[^=]*=\s*\{\s*min:\s*(\d+),\s*max:\s*(\d+)/; +const TRAILER = /^Protocol-bump: none \(.+\)\s*$/m; + +const args = process.argv.slice(2); +const option = (name, fallback) => { + const at = args.indexOf(name); + return at === -1 ? fallback : args[at + 1]; +}; +const git = (...a) => execFileSync("git", a, { encoding: "utf8" }); + +const head = option("--head", "HEAD"); +const base = git("merge-base", option("--base", "origin/main"), head).trim(); + +function range(rev) { + const match = RANGE.exec(git("show", `${rev}:${PROTOCOL}`)); + return match === null ? "none" : `${match[1]}..${match[2]}`; +} + +function isShape(file) { + const name = file.slice("src/contract/".length); + return ( + !name.includes("/") && + name.endsWith(".ts") && + !name.endsWith(".test.ts") && + !NOT_SHAPES.has(name) + ); +} + +function changesCode(file) { + return git("diff", "-U0", "-w", base, head, "--", file) + .split("\n") + .filter((line) => /^[+-]/.test(line) && !/^(\+\+\+|---) /.test(line)) + .map((line) => line.slice(1).trim()) + .some((line) => line !== "" && !/^(\/\/|\/\*|\*)/.test(line) && !RANGE.test(line)); +} + +const shapes = git("diff", "--name-only", base, head, "--", "src/contract/") + .split("\n") + .filter((file) => file !== "" && isShape(file) && changesCode(file)); +const before = range(base); +const after = range(head); +const waived = TRAILER.test(git("log", "--format=%B", `${base}..${head}`)); + +if (shapes.length > 0 && before === after && !waived) { + console.log( + `${shapes.join(", ")}: contract shape changed, but PROTOCOL_VERSION_RANGE stays ${after}. ` + + `Bump it by one in ${PROTOCOL}, or add the trailer "Protocol-bump: none ()" to a commit.`, + ); + process.exit(1); +} +if (shapes.length === 0 && before !== after) { + console.log( + `${PROTOCOL}: PROTOCOL_VERSION_RANGE moves ${before} to ${after} with no contract shape change.`, + ); + process.exit(1); +} diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 087c20679..a8abe2d70 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -251,3 +251,25 @@ jobs: run: pnpm install --frozen-lockfile --ignore-scripts - name: Fallow analysis run: pnpm fallow --ci + + # why: checked on the PR's merge commit, so a PR whose bump main has already taken (two PRs + # moving 34 to 35) fails here although its own branch passes. + protocol-bump: + name: Protocol bump + if: github.event_name == 'pull_request' + runs-on: ubuntu-24.04 + permissions: + contents: read + timeout-minutes: 5 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + fetch-depth: 0 + - uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 + with: + node-version: 22 + - name: Contract shape and protocol range change together + env: + BASE_REF: ${{ github.base_ref }} + run: node .agents/scripts/protocol-bump-check.mjs --base "origin/$BASE_REF" diff --git a/docs/internal/agent-rules/toolchain.md b/docs/internal/agent-rules/toolchain.md index 4446e61ac..dbceb721a 100644 --- a/docs/internal/agent-rules/toolchain.md +++ b/docs/internal/agent-rules/toolchain.md @@ -180,7 +180,11 @@ the tool's docs alone. - **Contract shapes.** Every change to a contract shape bumps `DAEMON_PROTOCOL_VERSION` by one in the same PR (a spec's "protocol +1 at merge" means this too), so two tasks that change contract shapes - overlap even when they touch different files. + overlap even when they touch different files. The push hook and CI run + `.agents/scripts/protocol-bump-check.mjs`: a contract shape change with + no bump, or a bump with no shape change, fails. A contract edit that + changes no wire shape (a refactor, a client-only error code) carries the + commit trailer `Protocol-bump: none ()`. ## Review measurement diff --git a/lefthook.yml b/lefthook.yml index 692b281a8..64f11333d 100644 --- a/lefthook.yml +++ b/lefthook.yml @@ -35,6 +35,10 @@ pre-push: priority: 1 run: pnpm exec fallow audit --base origin/main --format compact fail_text: "Fallow found issues in your changes. Run 'pnpm fallow' to see details." + protocol-bump: + priority: 1 + run: node .agents/scripts/protocol-bump-check.mjs --base origin/main + fail_text: "A contract shape change and a PROTOCOL_VERSION_RANGE bump go together (toolchain.md, Contract shapes)." typecheck-e2e: priority: 2 run: pnpm run typecheck:e2e diff --git a/src/contract/AGENTS.md b/src/contract/AGENTS.md index 734eccdc3..79af650f1 100644 --- a/src/contract/AGENTS.md +++ b/src/contract/AGENTS.md @@ -57,7 +57,10 @@ Three identities: - Now `{ min: 46, max: 46 }`. `DAEMON_PROTOCOL_VERSION` (`src/daemon-protocol/`) is its `max`. - Every change to a contract shape bumps it by one in the same PR. Move `min` too, unless - a compatibility path stays side by side. + a compatibility path stays side by side. `.agents/scripts/protocol-bump-check.mjs` checks + it on push and in CI; it counts every file here as a shape but tests, `index.ts` and the + helpers in its `NOT_SHAPES` (`console-url.ts`, `duration.ts`, `os-range.ts`). A new helper + that carries no wire shape goes in that list. - `hello` picks the highest overlap. None → `PROTOCOL_VERSION_UNSUPPORTED` with both ranges. A protocol-2 daemon is mapped by `mapLegacyProtocolMismatch`. - `daemon.stop` is accepted at any version, so the upgrade path exists. diff --git a/src/protocol-bump-check.test.ts b/src/protocol-bump-check.test.ts new file mode 100644 index 000000000..1969bc7ac --- /dev/null +++ b/src/protocol-bump-check.test.ts @@ -0,0 +1,97 @@ +import { execFileSync } 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", + "protocol-bump-check.mjs", +); + +const PROTOCOL = (n: number) => + `export const PROTOCOL_VERSION_RANGE: ProtocolRange = { min: ${n}, max: ${n} };\n`; +const SCHEMAS = "export const lease = z.object({ id: z.string() });\n"; + +function repo(after: Record, message = "head"): string { + const dir = mkdtempSync(join(tmpdir(), "protocol-bump-")); + const git = (...args: string[]) => + execFileSync("git", ["-c", "user.name=t", "-c", "user.email=t@t", ...args], { cwd: dir }); + const write = (files: Record) => { + for (const [path, text] of Object.entries(files)) { + mkdirSync(dirname(join(dir, path)), { recursive: true }); + writeFileSync(join(dir, path), text); + } + }; + git("init", "-q", "-b", "base"); + write({ + "src/contract/protocol.ts": PROTOCOL(7), + "src/contract/schemas.ts": SCHEMAS, + "src/contract/duration.ts": "export const second = 1000;\n", + }); + git("add", "-A"); + git("commit", "-q", "-m", "base"); + git("switch", "-q", "-c", "work"); + write(after); + git("add", "-A"); + git("commit", "-q", "--allow-empty", "-m", message); + return dir; +} + +function check(dir: string) { + try { + execFileSync("node", [SCRIPT, "--base", "base"], { cwd: dir, encoding: "utf8" }); + return { status: 0, stdout: "" }; + } catch (error) { + const failed = error as { status: number; stdout: string }; + return { status: failed.status, stdout: failed.stdout }; + } +} + +const SHAPE_CHANGE = { + "src/contract/schemas.ts": + "export const lease = z.object({ id: z.string(), ttl: z.number() });\n", +}; + +describe("protocol-bump-check.mjs", () => { + it("fails a contract shape change that leaves the protocol range as it was, and passes it with the range bumped", () => { + const unbumped = check(repo(SHAPE_CHANGE)); + const bumped = check(repo({ ...SHAPE_CHANGE, "src/contract/protocol.ts": PROTOCOL(8) })); + + expect(unbumped).toEqual({ + status: 1, + stdout: expect.stringContaining( + "src/contract/schemas.ts: contract shape changed, but PROTOCOL_VERSION_RANGE stays 7..7.", + ), + }); + expect(bumped).toEqual({ status: 0, stdout: "" }); + }); + + it("fails a protocol bump with no contract shape change", () => { + const result = check(repo({ "src/contract/protocol.ts": PROTOCOL(8) })); + + expect(result).toEqual({ + status: 1, + stdout: expect.stringContaining( + "PROTOCOL_VERSION_RANGE moves 7..7 to 8..8 with no contract shape change.", + ), + }); + }); + + it("passes edits that change no wire shape: comments, helpers, tests, and a change waived by a commit trailer", () => { + const cases = [ + { "src/contract/schemas.ts": `// why: ids are opaque\n${SCHEMAS}` }, + { "src/contract/duration.ts": "export const second = 1_000;\n" }, + { "src/contract/schemas.test.ts": "it('x', () => {});\n" }, + ].map((files) => check(repo(files)).status); + const waived = check(repo(SHAPE_CHANGE, "refactor: x\n\nProtocol-bump: none (rename only)")); + + expect(cases).toEqual([0, 0, 0]); + expect(waived.status).toBe(0); + }); +}); From 0511865914314b553316436d440f082b9fdfd46b Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 19 Oct 2026 22:19:48 +0200 Subject: [PATCH 14/15] docs(agents): point at a value's source instead of copying it; a later review round runs only the lenses its fix touched; audit breaks fail loudly Measured: 31 fixed stale-claim findings since 2026-10-11 were a copied value (protocol range, counts of commands and routes); 17 later-round lens runs whose input the fix never touched found nothing; 14 audit breaks never applied (BSD sed -i). --- .agents/briefs/implement-audit.md | 5 ++++- docs/CLI.md | 4 ++-- docs/HTTP-API.md | 4 ++-- docs/internal/REVIEW.md | 4 +++- docs/internal/agent-rules/documentation.md | 8 ++++++++ src/contract/AGENTS.md | 7 +++---- src/daemon/AGENTS.md | 2 +- src/mcp/AGENTS.md | 2 +- 8 files changed, 24 insertions(+), 12 deletions(-) diff --git a/.agents/briefs/implement-audit.md b/.agents/briefs/implement-audit.md index 97dd11f37..2b311bff2 100644 --- a/.agents/briefs/implement-audit.md +++ b/.agents/briefs/implement-audit.md @@ -46,7 +46,10 @@ edited after its `test:` commit: way, return early, or swap a rule for a near one: sort by another key, drop the tie-break, take the first instead of the lowest, drop one side of an `||` or `&&`) and run its file: it fails on the assertion its - title names, not by timing out. Restore the code. A test that stays green + title names, not by timing out. Apply each break so that one which + does not match fails loudly (a Python `str.replace` with an assert that + the text was found, not `sed -i`): a break that never applied proves + nothing. Restore the code. A test that stays green is vacuous: fix it. Every test, no skipping: a probe the audit skips, a reviewer does on a far costlier model. Report `probed

/` on the `audit:` line: `t` tests added or changed, `p` probed. diff --git a/docs/CLI.md b/docs/CLI.md index f60ce2cbe..7909fc0d5 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -128,7 +128,7 @@ surfaced by `lease renew`) falls back to exit 1; the structured stderr line still reports the specific code — a renew by a running `simlock lease` is the exception, and exits `14`. -The six fleet codes are placed on existing numbers rather than new +The fleet codes are placed on existing numbers rather than new ones, and the numbers are fixed by the contract's error table in the PRs that implement them: @@ -1080,7 +1080,7 @@ install ` and `devicectl device install app ` resolve their path on t artifact has to be there already (a shared volume, a CI checkout on that machine). -**Seven commands refuse outright**, with `UNSUPPORTED_IN_GATEWAY_MODE` (exit +**These commands refuse outright**, with `UNSUPPORTED_IN_GATEWAY_MODE` (exit 2): `simlock nuke`, `simlock cleanup`, `simlock doctor`, `simlock component list`, `simlock component remove`, `simlock device add` and `simlock device remove`. The first five act on one machine's devices or components as a diff --git a/docs/HTTP-API.md b/docs/HTTP-API.md index 622fad3f2..d63e081ae 100644 --- a/docs/HTTP-API.md +++ b/docs/HTTP-API.md @@ -1573,7 +1573,7 @@ On a **gateway** the lease, device, lease-request and event routes are fleet-wide, which is what makes a single console possible: `/v1/leases` and `/v1/devices` return every connected worker's, each record carrying the `workerId` it lives on, `/v1/lease-requests` returns the gateway's queue and every connected -worker's, and the two event routes carry the workers' republished events +worker's, and the event routes carry the workers' republished events (also `workerId`-tagged) interleaved with the gateway's own `worker.*` and `request.dispatched` facts. `/v1/tokens` lists the daemon's own tokens: on a gateway, the gateway's, since tokens never cross machines. @@ -1838,7 +1838,7 @@ behind this is not reachable right now" is the same answer in all four cases, and a client with one retry rule for `transport` should not need a second one because the unreachable thing happened to be a worker. -`UNSUPPORTED_IN_GATEWAY_MODE` comes from five routes in this version, +`UNSUPPORTED_IN_GATEWAY_MODE` comes from these routes in this version: `POST /v1/components/install` without `workers`, `GET /v1/components`, `DELETE /v1/components/{platform}/{version}`, `POST /v1/devices` and `DELETE /v1/devices/{id}` asked of a gateway — `nuke`, diff --git a/docs/internal/REVIEW.md b/docs/internal/REVIEW.md index 72c81f9f6..c1b4a6dbb 100644 --- a/docs/internal/REVIEW.md +++ b/docs/internal/REVIEW.md @@ -79,7 +79,9 @@ Classify actual fix impact (multiple labels allowed): Test files are not automatically mechanical. Claims-only corrections need a focused claims check; unchanged claims need no repeat review. Use actual -hunks, not the intended fix label, to choose follow-up. +hunks, not the intended fix label, to choose follow-up. A lens whose input +`fix.patch` does not touch does not run: claims needs a changed doc, +comment, test title or printed string; code a changed code or test line. There is one full round, one targeted round 2, and one final targeted fix-check. Count every invocation, including that fix-check. After the cap, diff --git a/docs/internal/agent-rules/documentation.md b/docs/internal/agent-rules/documentation.md index dfd60d0db..1f7d80d6e 100644 --- a/docs/internal/agent-rules/documentation.md +++ b/docs/internal/agent-rules/documentation.md @@ -59,3 +59,11 @@ grounds for rejecting a change even if the prose reads well. flow doc under `docs/internal/flows/`, linked from each module's manual. A manual and a flow doc hold no rationale (the ADR does) and no history (git does); a known gap lives in its module's manual. + +8. **Point at a value's source; never copy it.** A doc, manual or comment + that needs a number, default, limit or version that code defines names + the constant and its file (`PROTOCOL_VERSION_RANGE` in + `src/contract/protocol.ts`), not its value, and never counts the items a + list beside it or another file holds ("these commands", not "seven + commands"). An end-user doc that must show a value shows it, and the PR + that changes the value updates it. diff --git a/src/contract/AGENTS.md b/src/contract/AGENTS.md index 79af650f1..ef616c006 100644 --- a/src/contract/AGENTS.md +++ b/src/contract/AGENTS.md @@ -15,7 +15,7 @@ Imported by: daemon, gateway, simlock-client, every frontend, core, leasing, his | `schemas.ts` | Wire shapes, re-declared by hand from core; `fit*` bounders | | `roles.ts` | `Role`, `AuthorizeContext`, `ownsLease` | | `errors.ts` | `ErrorDetailsMap`, `ERROR_TABLE`, `SimlockError`, `fromWireError` | -| `pushes.ts` | `PUSH_SCHEMAS`: the nine push kinds | +| `pushes.ts` | `PUSH_SCHEMAS`: every push kind | | `protocol.ts` | `PROTOCOL_VERSION_RANGE`, negotiation, `hello` schemas | | `worker-view.ts` | `workerViewFields`, view refresh interval and events | | `os-range.ts` | OS constraint grammar: parse, `satisfies`, `compareVersions` | @@ -55,12 +55,11 @@ Three identities: ## Protocol version -- Now `{ min: 46, max: 46 }`. `DAEMON_PROTOCOL_VERSION` (`src/daemon-protocol/`) is its `max`. +- `PROTOCOL_VERSION_RANGE` in `protocol.ts` holds it. `DAEMON_PROTOCOL_VERSION` (`src/daemon-protocol/`) is its `max`. - Every change to a contract shape bumps it by one in the same PR. Move `min` too, unless a compatibility path stays side by side. `.agents/scripts/protocol-bump-check.mjs` checks it on push and in CI; it counts every file here as a shape but tests, `index.ts` and the - helpers in its `NOT_SHAPES` (`console-url.ts`, `duration.ts`, `os-range.ts`). A new helper - that carries no wire shape goes in that list. + helpers its `NOT_SHAPES` lists. A new helper that carries no wire shape goes in that list. - `hello` picks the highest overlap. None → `PROTOCOL_VERSION_UNSUPPORTED` with both ranges. A protocol-2 daemon is mapped by `mapLegacyProtocolMismatch`. - `daemon.stop` is accepted at any version, so the upgrade path exists. diff --git a/src/daemon/AGENTS.md b/src/daemon/AGENTS.md index 0528caecf..23538c28b 100644 --- a/src/daemon/AGENTS.md +++ b/src/daemon/AGENTS.md @@ -53,7 +53,7 @@ An HTTP bind failure after a good start stops the daemon (`http-start-failed`). Principal: `hello.principal`, else `SIMLOCK_AGENT_ID`, else the daemon's pid. Both are fixed for the connection's life. 3. **Version** — ranges overlap → highest common version. `PROTOCOL_VERSION_RANGE` - is `{ min: 46, max: 46 }` (`src/contract/protocol.ts`). No overlap → the + (`src/contract/protocol.ts`) is the daemon's range. No overlap → the connection stays open, and every request but `daemon.stop` gets `PROTOCOL_VERSION_UNSUPPORTED`. 4. **Request** — one frame → one `dispatch()` call. While stopping: `DAEMON_STOPPING`. diff --git a/src/mcp/AGENTS.md b/src/mcp/AGENTS.md index b8bb0f895..86cbf0bb2 100644 --- a/src/mcp/AGENTS.md +++ b/src/mcp/AGENTS.md @@ -1,6 +1,6 @@ # stdio MCP server -Does: one agent session over stdin/stdout — four tools, a renew timer, lazy reconnect. +Does: one agent session over stdin/stdout — its tools, a renew timer, lazy reconnect. Entry: `main.ts` → `runMcpStdio()`; `simlock mcp` loads it on demand (`src/cli/commands/mcp.ts`). Uses: client, lease-policy, agent-identity, contract, ports. Never imports: core, daemon, drivers, http. Surface: [CLI.md](../../docs/CLI.md#simlock-mcp). From a60694e0c7bdc013da4c7e0ec74a225ba70cec01 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 19 Oct 2026 22:21:37 +0200 Subject: [PATCH 15/15] chore(hooks): the push hook's e2e typecheck builds only the daemon's types, 15 s to 6 s e2e imports types from dist/client, dist/core and dist/ports, which build:daemon writes; the UI and capture builds added 9 s per push and nothing the typecheck reads. typecheck:e2e keeps the full build for pnpm check and CI. Measured: about 9 pushes per task. --- .agents/scripts/tool-guard.mjs | 1 + docs/internal/agent-rules/toolchain.md | 2 +- lefthook.yml | 2 +- package.json | 3 ++- 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/.agents/scripts/tool-guard.mjs b/.agents/scripts/tool-guard.mjs index 9c8081558..6c902695b 100644 --- a/.agents/scripts/tool-guard.mjs +++ b/.agents/scripts/tool-guard.mjs @@ -20,6 +20,7 @@ const SCRIPTS = new Set([ "test:timing", "typecheck", "typecheck:e2e", + "typecheck:e2e:types", "lint", "lint:comments", "format", diff --git a/docs/internal/agent-rules/toolchain.md b/docs/internal/agent-rules/toolchain.md index dbceb721a..0ed40b894 100644 --- a/docs/internal/agent-rules/toolchain.md +++ b/docs/internal/agent-rules/toolchain.md @@ -20,7 +20,7 @@ command of their own: they take every command from here. Hooks and CI run these. Never run them by hand, and never the tools behind them (from `node_modules/.bin` or any other path): `pnpm check`, `pnpm test`, -`pnpm test:timing`, `pnpm typecheck`, `pnpm typecheck:e2e`, `pnpm lint`, `pnpm lint:comments`, +`pnpm test:timing`, `pnpm typecheck`, `pnpm typecheck:e2e`, `pnpm typecheck:e2e:types`, `pnpm lint`, `pnpm lint:comments`, `pnpm format`, `pnpm format:check`, `pnpm fallow`, `pnpm mutate` (outside the `harden-tests` routine), the whole e2e suite, the console lane in every browser (`pnpm test:console`), `tsc`, diff --git a/lefthook.yml b/lefthook.yml index 64f11333d..e7e483249 100644 --- a/lefthook.yml +++ b/lefthook.yml @@ -41,4 +41,4 @@ pre-push: fail_text: "A contract shape change and a PROTOCOL_VERSION_RANGE bump go together (toolchain.md, Contract shapes)." typecheck-e2e: priority: 2 - run: pnpm run typecheck:e2e + run: pnpm run typecheck:e2e:types diff --git a/package.json b/package.json index f4835aca6..8a7bcef89 100644 --- a/package.json +++ b/package.json @@ -54,7 +54,8 @@ "test:e2e:physical-manual": "pnpm run build && pnpm run build:e2e && vitest run --project e2e --no-file-parallelism --reporter=default --reporter=./e2e/progress-reporter.ts --tags-filter='physical-manual'", "test:console": "pnpm run build && pnpm run build:e2e && playwright test", "typecheck": "tsc --noEmit --incremental --tsBuildInfoFile node_modules/.cache/tsc/src.tsbuildinfo && tsc -p ui/tsconfig.json --incremental --tsBuildInfoFile node_modules/.cache/tsc/ui.tsbuildinfo", - "typecheck:e2e": "pnpm run build && tsc -p e2e/tsconfig.typecheck.json --incremental --tsBuildInfoFile node_modules/.cache/tsc/e2e.tsbuildinfo", + "typecheck:e2e": "pnpm run build:ui && pnpm run build:capture && pnpm run typecheck:e2e:types", + "typecheck:e2e:types": "pnpm run build:daemon && tsc -p e2e/tsconfig.typecheck.json --incremental --tsBuildInfoFile node_modules/.cache/tsc/e2e.tsbuildinfo", "lint": "oxlint src e2e ui && depcruise src e2e ui --ignore-known", "lint:comments": "node .agents/scripts/comment-lint.mjs --base origin/main", "fallow": "fallow",