diff --git a/.agents/briefs/implement-audit.md b/.agents/briefs/implement-audit.md index e85277685..b07090a69 100644 --- a/.agents/briefs/implement-audit.md +++ b/.agents/briefs/implement-audit.md @@ -78,6 +78,11 @@ edited after its `test:` commit: Run the "Project checks" in `toolchain.md` that the diff triggers. Then the tests from mode `build` pass, and you push. +Every test command runs in the foreground with `timeout: 600000` set on +the Bash call, and nothing is left running: no `run_in_background`, no +trailing `&`, no loop that sleeps, no bare `cat > file`. The tool guard +refuses each; a background job would hold your report until it ends or +hits its 30-minute cap, which stalls the whole delivery. **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 index ba87756c9..dc1492ade 100644 --- a/.agents/briefs/implement-build.md +++ b/.agents/briefs/implement-build.md @@ -82,6 +82,11 @@ targets that branch. The PR body, in this order: `Closes #`; a as an unchecked list; `## Assumptions` ("none", or one `- Assumption:` line per gap); the line `*Written by an agent.*`. Keep it current as you go. +- Every test command runs in the foreground with `timeout: 600000` set on + the Bash call, and nothing is left running: no `run_in_background`, no + trailing `&`, no loop that sleeps, no bare `cat > file`. The tool guard + refuses each; a background job would hold your report until it ends or + hits its 30-minute cap, which stalls the whole delivery. - 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 diff --git a/.agents/briefs/implement-fix.md b/.agents/briefs/implement-fix.md index 1346c2f5c..20ca92a89 100644 --- a/.agents/briefs/implement-fix.md +++ b/.agents/briefs/implement-fix.md @@ -31,11 +31,20 @@ mechanical. Inputs include immutable `base`, `merge-base`, `previous` and test; - a `ci:` line: the named test failed in CI; a line asking to merge the base branch: merge it and resolve the conflicts. + - an `adr: to ` line: the feature is merging (delivery + rule 10); set that ADR's `**Status:**` line and its row in + `docs/internal/adr/README.md` to the status named, and change nothing + else. - a `stack:` line naming a parent branch: in your worktree, `git merge origin/` into the branch, resolve the conflicts, run the tests of the files the resolution touched, commit the merge and push. Never rebase, never force-push, never touch another branch: the orchestrator carries the merge up the stack with `stack-sync.sh`. + Every test command runs in the foreground with `timeout: 600000` set on + the Bash call, and nothing is left running: no `run_in_background`, no + trailing `&`, no loop that sleeps, no bare `cat > file`. The tool guard + refuses each; a background job would hold your report until it ends or + hits its 30-minute cap, which stalls the whole delivery. 3. 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: the commit and push hooks run them, and their errors show diff --git a/.agents/briefs/review-code.md b/.agents/briefs/review-code.md index 9d08b3744..7a394255e 100644 --- a/.agents/briefs/review-code.md +++ b/.agents/briefs/review-code.md @@ -24,7 +24,10 @@ 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. +`lens: both` nine. Restore the tree before you report. Every test run is a +foreground Bash call with `timeout: 600000` set, never a background job or +a loop that sleeps: the tool guard refuses those, because a background job +holds your report until it ends. ## Lens: behaviour (`lens: behaviour` or `both`) diff --git a/.agents/scripts/ci-triage.mjs b/.agents/scripts/ci-triage.mjs index 78c63a4f9..b1823dbe1 100644 --- a/.agents/scripts/ci-triage.mjs +++ b/.agents/scripts/ci-triage.mjs @@ -10,8 +10,17 @@ // reach the test file, or a source file in its directory, is in the PR's diff: a fix run // unknown none of the above: rerun the failed jobs once; a second failure gets a fix run // +// A failed job whose log names no test (lint, typecheck, format) prints as ` job ` +// followed by up to 8 indented lines from its log: the lines naming a file:line or an error, else +// its last 3 lines (none for Quality and Console, which only sum up other jobs); nobody needs +// `gh run view --log-failed` by hand. +// // Ends with `rerun ` lines for the runs whose failures are all flaky or unknown, and -// `none` when nothing failed. The repo is GH_REPO, else the origin remote's owner/name. +// `none` when nothing failed. Exits 0, or 3 when the result is stale: when a workflow's newest +// finished run on the branch is for a commit that is not the PR's head (a push cancels the run +// before it, so the head's own run may not have finished), one line `stale: run is , +// head is ; checks for the head are ` comes first, and the lines after it +// describe that older commit. The repo is GH_REPO, else the origin remote's owner/name. import { execFileSync } from "node:child_process"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; @@ -47,14 +56,18 @@ function failures(args) { } return runs; } +// why: GitHub's stored logs spell the escape byte as the two characters `^[`. +const ANSI = new RegExp(`(?:${String.fromCharCode(27)}|\\^\\[)\\[[0-9;]*m`, "g"); const testFile = (test) => /^((?:e2e|src|ui)\/[\w./-]+\.(?:test|spec)\.tsx?)/.exec(test)?.[1] ?? null; const pull = api(`pulls/${pr}`); +const stale = staleRuns(); +for (const line of stale) console.log(line); const mine = failures([pr]); if (mine.length === 0) { console.log("none"); - process.exit(0); + process.exit(stale.length > 0 ? 3 : 0); } const onBase = new Set(failures(["--branch", pull.base.ref]).flatMap((r) => r.tests)); const flakyIssues = api( @@ -92,7 +105,77 @@ for (const r of mine) { else if (reached(file)) cls = "reach"; if (cls === "reach" || cls === "base") needsFix = true; console.log(`${cls} ${test} (${r.workflow} run ${r.id})`); + if (test.startsWith("job ")) for (const l of excerpt(r.id, test)) console.log(` ${l}`); } if (!needsFix) reruns.push(r.id); } for (const id of reruns) console.log(`rerun ${id}`); +process.exit(stale.length > 0 ? 3 : 0); + +// why: ci-failures.mjs reads only a workflow's newest run that ended in success or failure, so +// while the head's run is pending or was cancelled it reports an older commit as if current. +function staleRuns() { + const listed = api( + "actions/runs", + "-f", + `branch=${pull.head.ref}`, + "-f", + "per_page=100", + ).workflow_runs; + const head = pull.head.sha; + const finished = (r) => r.conclusion === "success" || r.conclusion === "failure"; + const newest = new Map(); + for (const r of listed.filter(finished)) { + const seen = newest.get(r.name); + if (seen === undefined || r.created_at > seen.created_at) newest.set(r.name, r); + } + // why: a run GitHub skipped on the head (Issue state skips most events) is that workflow's + // whole answer for the head, so an older run of it is not stale. + const settled = (name) => + listed.some( + (r) => + r.name === name && + r.head_sha === head && + r.status === "completed" && + r.conclusion !== "cancelled", + ); + return [...newest.values()] + .filter((r) => r.head_sha !== head && !settled(r.name)) + .map((r) => { + const pending = listed.some( + (x) => x.name === r.name && x.head_sha === head && x.status !== "completed", + ); + return `stale: run ${r.id} is ${r.head_sha.slice(0, 7)}, head is ${head.slice(0, 7)}; checks for the head are ${pending ? "pending" : "missing"}`; + }); +} + +function excerpt(id, test) { + const job = /^job (.+?)(?: \(log unavailable: .*\))?$/.exec(test)[1]; + if (/^(Quality|Console)$/.test(job)) return []; + let log; + try { + log = run("gh", ["run", "view", id, "--log-failed"]); + } catch { + return []; + } + const lines = log + .split("\n") + .filter((l) => l.startsWith(`${job}\t`)) + .map((l) => + l + .replace(ANSI, "") + .replace(/^[^\t]*\t[^\t]*\t\uFEFF?\S+Z ?/, "") + .trimEnd(), + ) + .filter( + (l) => + l.trim() !== "" && + !/Process completed with exit code|ELIFECYCLE|^##\[(?:end)?group\]/.test(l), + ); + const named = lines.filter((l) => + /(?:^|[\s[(])[\w./-]+\.(?:[cm]?[jt]sx?|mdx?|json|sh|ya?ml)(?::\d|\(\d|: )|\berror\b|^\s*[×✖]/i.test( + l, + ), + ); + return (named.length > 0 ? named : lines.slice(-3)).slice(0, 8).map((l) => l.trim()); +} diff --git a/.agents/scripts/issue-body.mjs b/.agents/scripts/issue-body.mjs new file mode 100644 index 000000000..f88b30dc1 --- /dev/null +++ b/.agents/scripts/issue-body.mjs @@ -0,0 +1,215 @@ +#!/usr/bin/env node +// `node .agents/scripts/issue-body.mjs [--pr] [--dry-run] ...`: edits one issue's or +// PR's body in place, so nobody rewrites a body by hand to change one line. +// +// replace must occur exactly once, else exit 1 saying how often +// set-section the section becomes the file's text under the same heading +// get-section prints the section's text, without its heading line +// append adds the file's text at the end +// +// , may be `@` to read the text from a file; a file's one trailing newline is +// dropped. A section runs from its heading line to the next heading of the same or a higher +// level, code blocks skipped. `Status` matches `## Status` or `### Status`; `### +// Status` matches that level only. Two matches: exit 1. A missing section is created as `## +// ` (or the level given) at the end. The body's last line stays `*Written by an +// agent.*` (`_Written by an agent._` is kept as it is), added when missing, and the end of the +// body means just above it. +// +// Refuses (exit 1) to touch an issue labelled `request:*` or `bug:*`: its body is the reporter's +// (delivery rule 5); a PR is never refused. `--pr` says is a PR; a mismatch exits 2. Before +// writing it reads the body again and exits 1 when someone changed it meanwhile. Prints `updated +// #`, or `unchanged #`. `--dry-run` prints the new body and writes nothing. `--body-file +// ` reads the body from a file instead of GitHub and implies --dry-run (an offline test). +// Usage errors exit 2. Writes through REST, PATCH issues/; the repo is GH_REPO, else the +// origin remote's owner/name. +import { execFileSync } from "node:child_process"; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +const FOOTER = "*Written by an agent.*"; +const IS_FOOTER = /^[*_]Written by an agent\.[*_]$/; +const USAGE = + "usage: issue-body.mjs [--pr] [--dry-run] [--body-file ] replace | set-section | get-section | append "; + +const argv = process.argv.slice(2); +const flag = (name) => { + const at = argv.indexOf(name); + if (at === -1) return false; + argv.splice(at, 1); + return true; +}; +const isPr = flag("--pr"); +let dryRun = flag("--dry-run"); +let bodyFile = null; +const fileAt = argv.indexOf("--body-file"); +if (fileAt !== -1) { + bodyFile = argv[fileAt + 1] ?? null; + argv.splice(fileAt, 2); + dryRun = true; +} +const [n, op, ...rest] = argv; +const arity = { replace: 2, "set-section": 2, "get-section": 1, append: 1 }; +if ( + n === undefined || + !/^\d+$/.test(n) || + !(op in arity) || + rest.length !== arity[op] || + (fileAt !== -1 && bodyFile === null) +) { + usage(); +} + +const run = (cmd, args) => + execFileSync(cmd, args, { + encoding: "utf8", + maxBuffer: 64 * 1024 * 1024, + stdio: ["ignore", "pipe", "pipe"], + }); +const nwo = () => + process.env.GH_REPO ?? + run("git", ["remote", "get-url", "origin"]) + .trim() + .replace(/\.git$/, "") + .replace(/.*[/:]([^/:]+\/[^/]+)$/, "$1"); +// why: a cloud session's GitHub proxy refuses GraphQL, which `gh issue edit` and `gh pr edit` +// use; issues/ on REST reads and writes a PR's body as well as an issue's. +const fetchIssue = () => JSON.parse(run("gh", ["api", "-X", "GET", `repos/${nwo()}/issues/${n}`])); + +let original; +if (bodyFile !== null) { + original = readFileSync(bodyFile, "utf8"); +} else { + const issue = fetchIssue(); + const pull = issue.pull_request !== undefined; + if (pull !== isPr) { + console.error(`issue-body: #${n} is ${pull ? "a PR: pass --pr" : "an issue: drop --pr"}`); + process.exit(2); + } + const reporter = (issue.labels ?? []).map((l) => l.name).find((l) => /^(request|bug):/.test(l)); + if (!pull && reporter !== undefined) { + die( + `refusing: #${n} is labelled ${reporter}, and its body stays the reporter's (delivery rule 5); post a comment instead`, + ); + } + original = issue.body ?? ""; +} + +const { main, footer } = split(original); +let next = main; +if (op === "replace") { + const [old, now] = rest.map(text); + if (old === "") die(" is empty"); + const count = main.split(old).length - 1; + if (count !== 1) die(` occurs ${count} times in #${n}'s body; it must occur once`); + next = main.replace(old, () => now); +} else if (op === "append") { + next = joined([main, text(`@${rest[0]}`).trim()]); +} else { + const want = target(rest[0]); + const lines = main.split("\n"); + const found = headings(lines).filter( + (h) => + h.text === want.text && + (want.level === null ? h.level === 2 || h.level === 3 : h.level === want.level), + ); + if (found.length > 1) { + die(`${found.length} sections are headed "${want.text}"; name its level: "### ${want.text}"`); + } + const at = found[0]; + if (op === "get-section") { + if (at === undefined) die(`no section headed "${want.text}" in #${n}'s body`); + console.log( + lines + .slice(at.at + 1, end(lines, at)) + .join("\n") + .trim(), + ); + process.exit(0); + } + let content = text(`@${rest[1]}`).replace(/\s+$/, ""); + const first = /^\s*(#{1,6})\s+(.*\S)\s*(?:\n|$)/.exec(content); + if (first !== null && first[2] === want.text) content = content.slice(first[0].length); + const level = at?.level ?? want.level ?? 2; + const section = joined([`${"#".repeat(level)} ${want.text}`, content.trim()]); + next = + at === undefined + ? joined([main, section]) + : joined([lines.slice(0, at.at).join("\n"), section, lines.slice(end(lines, at)).join("\n")]); +} + +const body = joined([next.replace(/\s+$/, ""), footer]); +if (dryRun) { + console.log(body); + process.exit(0); +} +if (body === original.replace(/\r/g, "")) { + console.log(`unchanged #${n}`); + process.exit(0); +} +if ((fetchIssue().body ?? "") !== original) { + die(`#${n}'s body changed while this edit was being made; run it again`); +} +const dir = mkdtempSync(join(tmpdir(), "issue-body-")); +try { + const payload = join(dir, "body.json"); + writeFileSync(payload, JSON.stringify({ body })); + run("gh", ["api", "-X", "PATCH", `repos/${nwo()}/issues/${n}`, "--input", payload]); +} finally { + rmSync(dir, { recursive: true, force: true }); +} +console.log(`updated #${n}`); + +function usage() { + console.error(USAGE); + process.exit(2); +} + +function die(message) { + console.error(`issue-body: ${message}`); + process.exit(1); +} + +function text(value) { + if (!value.startsWith("@")) return value; + return readFileSync(value.slice(1), "utf8").replace(/\r/g, "").replace(/\n$/, ""); +} + +function split(raw) { + const lines = raw.replace(/\r/g, "").replace(/\s+$/, "").split("\n"); + const last = (lines.at(-1) ?? "").trim(); + if (IS_FOOTER.test(last)) { + return { main: lines.slice(0, -1).join("\n").replace(/\s+$/, ""), footer: last }; + } + return { main: lines.join("\n"), footer: FOOTER }; +} + +function joined(parts) { + return parts + .filter((p) => p.trim() !== "") + .map((p) => p.replace(/^\n+|\s+$/g, "")) + .join("\n\n"); +} + +function target(heading) { + const h = /^(#{1,6})\s+(.*\S)\s*$/.exec(heading); + return h === null ? { level: null, text: heading.trim() } : { level: h[1].length, text: h[2] }; +} + +function headings(lines) { + const out = []; + let fence = false; + lines.forEach((l, at) => { + if (/^\s*(```|~~~)/.test(l)) fence = !fence; + else if (!fence) { + const h = /^(#{1,6})\s+(.*\S)\s*$/.exec(l); + if (h !== null) out.push({ at, level: h[1].length, text: h[2] }); + } + }); + return out; +} + +function end(lines, at) { + const after = headings(lines).find((h) => h.at > at.at && h.level <= at.level); + return after === undefined ? lines.length : after.at; +} diff --git a/.agents/scripts/merge-pr.sh b/.agents/scripts/merge-pr.sh index 41b183954..6f5d1c39b 100755 --- a/.agents/scripts/merge-pr.sh +++ b/.agents/scripts/merge-pr.sh @@ -8,7 +8,10 @@ # nothing. Gates, in order, for each: the PR is open and not a draft; it carries no needs-hardware label; its body # passes pr-body-check.mjs (a `## Review` section with its counts line, no "spec needs" line, # the agent footer); every CI check passed or was skipped; and GitHub calls it mergeable. Waits up to 30 minutes for pending checks. Prints the first gate -# that fails and exits 1, or exits 2 when checks are still pending at the deadline. +# that fails and exits 1, or exits 2 when checks are still pending at the deadline. While it +# waits it prints one line per poll to stderr, `merge-pr: # pending check(s) (), min waited`, and `merge-pr: # re-running the failed jobs of run : ` +# for each run it re-runs. # # Red CI gets one re-run of its failed jobs when no failure is this PR's to fix: every red # check is a job GitHub cancelled (no runner came), or every test that failed is named by an @@ -64,7 +67,8 @@ checks() { # Waits for every check to settle, then sets `bad` to the ones that did not pass or skip. # Called directly, never in $( ), so its exits end the script. red_checks() { - deadline=$(($(date +%s) + 1800)) + started=$(date +%s) + deadline=$((started + 1800)) while :; do all=$(checks) checks=$(printf '%s' "$all" | sed 's/^\([^ ]*\) [^ ]* /\1 /') @@ -74,6 +78,9 @@ red_checks() { echo "not merged: checks still pending after 30 minutes" >&2 exit 2 } + pending=$(echo "$checks" | sed -n 's/^pending //p') + echo "merge-pr: #$pr pending $(echo "$pending" | grep -c '') check(s) ($(echo "$pending" | + head -n 3 | paste -sd , - | sed 's/,/, /g')), $((($(date +%s) - started) / 60)) min waited" >&2 sleep "$poll" done bad=$(echo "$checks" | grep -v -e '^pass ' -e '^skipping ' || true) @@ -119,7 +126,12 @@ gates() { runs=$(rerunnable_runs) if [ -n "$runs" ] && ! echo "$runs" | grep -qx none; then echo "PR #$pr: re-running the failed jobs once: cancelled, or failed only on open flaky-test issues" - for run in $runs; do gh run rerun "$run" --failed; done + for run in $runs; do + jobs=$(echo "$all" | grep -e '^fail ' -e '^cancel ' | grep "/actions/runs/$run/" | + sed 's/^[^ ]* [^ ]* //' | paste -sd , - | sed 's/,/, /g') + echo "merge-pr: #$pr re-running the failed jobs of run $run: $jobs" >&2 + gh run rerun "$run" --failed + done sleep "$poll" red_checks fi diff --git a/.agents/scripts/pr-shas.sh b/.agents/scripts/pr-shas.sh new file mode 100755 index 000000000..c8061505a --- /dev/null +++ b/.agents/scripts/pr-shas.sh @@ -0,0 +1,88 @@ +#!/usr/bin/env sh +# Prints, in one call, the commits and states a delivery step needs about a PR, read fresh: +# +# .agents/scripts/pr-shas.sh +# +# Fetches origin quietly first, then prints one line each: +# +# base the tip of origin/ now (not the PR's stored base sha) +# merge-base git merge-base of that tip and the head +# head the PR's head commit on GitHub +# base-ref the PR's base branch +# mergeable true, false, or null while GitHub has not computed it +# draft true or false +# ci for the head's check runs and statuses, in merge-pr.sh's buckets: pending +# when any is not finished, fail when any failed or was cancelled, pass +# when all passed or were skipped, none when there are none +# stack #a -> #b the open PRs of its GitHub stack, bottom to top; left out when unstacked +# +# A value it cannot read prints as `unknown`. Exits 0 always: it reports, it decides nothing. +# The repo is GH_REPO, else the origin remote's owner/name. +set -u + +pr=${1:-} +case $pr in '' | *[!0-9]*) echo "usage: pr-shas.sh " >&2; exit 0 ;; esac +# why: a cloud session's GitHub proxy refuses GraphQL, which `gh pr view` uses, so this script +# calls repo-scoped REST only. +nwo=${GH_REPO:-$(git remote get-url origin | sed -E 's#\.git$##; s#.*[/:]([^/:]+/[^/]+)$#\1#')} +api() { + at=$1 + shift + gh api -X GET "repos/$nwo/$at" "$@" 2>/dev/null +} +or_unknown() { [ -n "$1" ] && echo "$1" || echo unknown; } + +git fetch -q origin 2>/dev/null +info=$(api "pulls/$pr" -q '"\(.head.sha) \(.base.ref) \(.draft) \(.mergeable) \(.state)"') +set -- $info +head=${1:-unknown} ref=${2:-unknown} draft=${3:-unknown} mergeable=${4:-unknown} state=${5:-} +# why: GitHub computes mergeability lazily; `null` (not computed yet) settles within seconds on +# an open PR, and stays null on a closed one. +for _ in 1 2 3; do + [ "$mergeable" = null ] && [ "$state" = open ] || break + sleep 2 + mergeable=$(or_unknown "$(api "pulls/$pr" -q '.mergeable | tostring')") +done +git fetch -q origin "+refs/pull/$pr/head:refs/pr-shas/$pr" 2>/dev/null +base=$(or_unknown "$(git rev-parse -q --verify "origin/$ref^{commit}" 2>/dev/null)") +fork=$(or_unknown "$(git merge-base "$base" "refs/pr-shas/$pr" 2>/dev/null)") +git update-ref -d "refs/pr-shas/$pr" 2>/dev/null + +echo "base $base" +echo "merge-base $fork" +echo "head $head" +echo "base-ref $ref" +echo "mergeable $mergeable" +echo "draft $draft" + +# why: `gh api --paginate` follows Link headers to numeric-ID repository URLs, which the proxy +# also refuses, so pages are requested by number. The filter must print one line per item. +paged() { + at=$1 filter=$2 page=1 + while :; do + batch=$(api "$at" -f per_page=100 -f page="$page" -q "$filter") || return 1 + [ -z "$batch" ] || printf '%s\n' "$batch" + [ "$(printf '%s' "$batch" | grep -c '')" -ge 100 ] || return 0 + page=$((page + 1)) + done +} +ci=unknown +if [ "$head" != unknown ] && + buckets=$(paged "commits/$head/check-runs" '.check_runs[] | if .status != "completed" then + "pending" elif .conclusion == "success" or .conclusion == "neutral" or .conclusion == + "skipped" then "pass" else "fail" end') && + statuses=$(paged "commits/$head/status" '.statuses[] | if .state == "pending" then "pending" + elif .state == "success" then "pass" else "fail" end'); then + all=$(printf '%s\n%s\n' "$buckets" "$statuses" | grep .) + if [ -z "$all" ]; then ci=none + elif echo "$all" | grep -qx pending; then ci=pending + elif echo "$all" | grep -qx fail; then ci=fail + else ci=pass + fi +fi +echo "ci $ci for $head" + +stack=$(api "stacks?pull_request=$pr" \ + -q '[.[] | select(.open)][0].pull_requests // [] | .[] | select(.state == "open") | "#\(.number)"') +[ -z "$stack" ] || echo "stack $(echo "$stack" | tr '\n' ' ' | sed 's/ $//; s/ / -> /g')" +exit 0 diff --git a/.agents/scripts/review-ledger.mjs b/.agents/scripts/review-ledger.mjs new file mode 100644 index 000000000..9592572cf --- /dev/null +++ b/.agents/scripts/review-ledger.mjs @@ -0,0 +1,168 @@ +#!/usr/bin/env node +// `node .agents/scripts/review-ledger.mjs --issue --findings --round +// : [--round : ...] [--dry-run]`: posts a PR's `## Review ledger` comment +// (docs/internal/REVIEW.md, "Measurement") and then checks the PR body, so neither is built or +// run by hand. +// +// The comment: one `Round : ` line per --round, in the order given ( is 1, 2 or +// fix-check; a short sha is expanded), then what `review-metrics.mjs --pr --issue +// --findings ` prints, in a code block, then the findings file in a `json` code block, +// then `*Written by an agent.*`. It refuses (exit 2, nothing posted) a sha that is no commit +// after fetching the PR's head, a findings file review-metrics.mjs rejects, and a finding with no +// `round` or a round no --round names; it warns on stderr when a sha is not in the PR head's +// history. It updates the PR's existing comment headed `## Review ledger`, else posts a new one, +// prints `ledger: updated ` or `ledger: posted `, then runs `pr-body-check.mjs +// `, prints its output and exits with its code. `--dry-run` prints the comment and does +// neither. Usage errors exit 2. The repo is GH_REPO, else the origin remote's owner/name. +import { execFileSync, spawnSync } from "node:child_process"; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { option } from "./cli-args.mjs"; + +const USAGE = + "usage: review-ledger.mjs --issue --findings --round : [--round ...] [--dry-run]"; +const argv = process.argv.slice(2); +const pr = argv[0]; +const issue = option("--issue"); +const findingsFile = option("--findings"); +const dryRun = argv.includes("--dry-run"); +const rounds = argv.flatMap((a, i) => (a === "--round" ? [argv[i + 1] ?? ""] : [])); +if ( + !/^\d+$/.test(pr ?? "") || + !/^\d+$/.test(issue ?? "") || + findingsFile === null || + rounds.length === 0 +) { + refuse(USAGE); +} + +const here = dirname(fileURLToPath(import.meta.url)); +const run = (cmd, args) => + execFileSync(cmd, args, { + encoding: "utf8", + maxBuffer: 64 * 1024 * 1024, + stdio: ["ignore", "pipe", "pipe"], + }); +const nwo = + process.env.GH_REPO ?? + run("git", ["remote", "get-url", "origin"]) + .trim() + .replace(/\.git$/, "") + .replace(/.*[/:]([^/:]+\/[^/]+)$/, "$1"); + +try { + run("git", ["fetch", "-q", "origin", `+refs/pull/${pr}/head:refs/review-ledger/${pr}`]); +} catch { + refuse(`cannot fetch PR #${pr}'s head from origin`); +} +const lines = []; +let wrong = null; +for (const round of rounds) { + const m = /^(1|2|fix-check):([0-9a-f]{7,40})$/.exec(round); + if (m === null) { + wrong = `--round ${round}: want <1|2|fix-check>:`; + break; + } + const full = spawnSync("git", ["rev-parse", "--verify", "-q", `${m[2]}^{commit}`], { + encoding: "utf8", + }); + if (full.status !== 0) { + wrong = `--round ${round}: ${m[2]} is no commit here, even after fetching PR #${pr}'s head`; + break; + } + const sha = full.stdout.trim(); + const reached = spawnSync("git", [ + "merge-base", + "--is-ancestor", + sha, + `refs/review-ledger/${pr}`, + ]); + if (reached.status !== 0) { + console.error(`review-ledger: warning: round ${m[1]} ${sha} is not in PR #${pr}'s history`); + } + lines.push(`Round ${m[1]}: ${sha}`); +} +spawnSync("git", ["update-ref", "-d", `refs/review-ledger/${pr}`]); +if (wrong !== null) refuse(wrong); + +let findings; +try { + findings = JSON.parse(readFileSync(findingsFile, "utf8")); +} catch (error) { + refuse(`--findings ${findingsFile}: ${error.message}`); +} +const named = new Set(rounds.map((r) => r.split(":")[0])); +for (const f of Array.isArray(findings.findings) ? findings.findings : []) { + if (f.round === undefined) refuse(`finding ${f.id} has no round`); + if (!named.has(String(f.round))) { + refuse(`finding ${f.id} names round ${f.round}, which no --round gives`); + } +} +const metrics = spawnSync( + "node", + [join(here, "review-metrics.mjs"), "--pr", pr, "--issue", issue, "--findings", findingsFile], + { encoding: "utf8" }, +); +if (metrics.status !== 0) { + const why = metrics.stderr.split("\n").find((l) => /^\w*Error: /.test(l)) ?? metrics.stderr; + refuse(`review-metrics.mjs rejected the findings: ${why.trim()}`); +} + +const comment = [ + "## Review ledger", + lines.join("\n"), + `\`\`\`\n${metrics.stdout.trim()}\n\`\`\``, + `\`\`\`json\n${JSON.stringify(findings)}\n\`\`\``, + "*Written by an agent.*", +].join("\n\n"); +if (dryRun) { + console.log(comment); + process.exit(0); +} + +// why: `gh api --paginate` follows Link headers to numeric-ID repository URLs, which a cloud +// session's GitHub proxy refuses, so pages are requested by number. +let existing; +for (let page = 1; existing === undefined; page += 1) { + const batch = JSON.parse( + run("gh", [ + "api", + "-X", + "GET", + `repos/${nwo}/issues/${pr}/comments`, + "-f", + "per_page=100", + "-f", + `page=${page}`, + ]), + ); + existing = batch.find((c) => /^## Review ledger\s*$/m.test(c.body ?? "")); + if (batch.length < 100) break; +} +const dir = mkdtempSync(join(tmpdir(), "review-ledger-")); +try { + const payload = join(dir, "comment.json"); + writeFileSync(payload, JSON.stringify({ body: comment })); + const [method, path] = + existing === undefined + ? ["POST", `issues/${pr}/comments`] + : ["PATCH", `issues/comments/${existing.id}`]; + const posted = JSON.parse( + run("gh", ["api", "-X", method, `repos/${nwo}/${path}`, "--input", payload]), + ); + console.log(`ledger: ${existing === undefined ? "posted" : "updated"} ${posted.html_url}`); +} finally { + rmSync(dir, { recursive: true, force: true }); +} + +const check = spawnSync("node", [join(here, "pr-body-check.mjs"), pr], { encoding: "utf8" }); +process.stdout.write(check.stdout); +process.stderr.write(check.stderr); +process.exit(check.status ?? 1); + +function refuse(message) { + console.error(message.startsWith("usage:") ? message : `review-ledger: ${message}`); + process.exit(2); +} diff --git a/.agents/scripts/tool-guard.mjs b/.agents/scripts/tool-guard.mjs index 6c902695b..c28edb136 100644 --- a/.agents/scripts/tool-guard.mjs +++ b/.agents/scripts/tool-guard.mjs @@ -9,6 +9,13 @@ // 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. +// waiting a sub-agent runs a test runner without `timeout: 600000` on the Bash call (the +// default 120 s moves a two-minute run to the background), starts a background job +// (`run_in_background`, a trailing `&`), polls in a loop that sleeps, or runs a bare +// `cat > file` with no input (it reads stdin until the tool times out). Each of +// these turned into a stall: a backgrounded job holds the agent's report until the +// job ends or hits its 30-minute cap. The slow lane is exempt: `slow-e2e.sh` runs +// detached by design and its agents wait on `.exit` (delivery rule 16). // // A refusal exits 2 with the reason on stderr, which the agent reads. import fs from "node:fs"; @@ -33,6 +40,14 @@ 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"]); +// why: the runners agents are allowed to run; `slow-e2e.sh` is excluded above, not here. +const TEST_RUNNER = + /\bvitest\s+(?:run|--)|\bplaywright\s+test\b|\bpnpm\s+(?:run\s+)?test(?::[\w-]+)?(?:\s|$)/; +// why: a `&` that is not `&&` and ends a command or a line starts a background job. +const BACKGROUND_JOB = /(?:^|[^&])&\s*(?:$|\n|\)|;)/m; +const SLEEP_LOOP = /\b(?:until|while|for)\b[^\n]*\bdo\b[\s\S]*?\bsleep\b/; +// why: `cat > file` followed by a terminator, with no `<<` heredoc and no `<` input, hangs. +const BARE_CAT = /(?:^|[;&|]\s*|\n\s*)cat\s*>{1,2}\s*\S+(?:\s+\d?>{1,2}\S+)*\s*(?:;|&&|\|\||\n|$)/; const HOOK_BYPASS = [ /--no-verify\b/, /\bLEFTHOOK=(?:0|false)\b/, @@ -70,6 +85,36 @@ if (tool === "Bash" && typeof input.agent_id === "string") { 'the commands under "Tests agents run".', ); } + const raw = args.command ?? ""; + const slowLane = /slow-e2e\.sh|\.exit\b/.test(raw); + if (!slowLane) { + if (TEST_RUNNER.test(shell) && !(Number(args.timeout) >= 300000)) { + refuse( + "Refused: a test run needs `timeout: 600000` on this Bash call. The default 120 s " + + "moves a two-minute run to the background, and a background job holds your report " + + "until it ends. Run it again in the foreground with the timeout set.", + ); + } + if (args.run_in_background === true || BACKGROUND_JOB.test(shell)) { + refuse( + "Refused: nothing runs in the background in a delivery stage. A background job holds " + + "your report until it ends or hits its 30-minute cap, and polling it costs the turn. " + + "Run the command in the foreground with `timeout: 600000` on the Bash call.", + ); + } + if (SLEEP_LOOP.test(shell)) { + refuse( + "Refused: no loop that sleeps. Nothing you started runs in the background, so there is " + + "nothing to wait for: run the command itself in the foreground with `timeout: 600000`.", + ); + } + if (BARE_CAT.test(raw)) { + refuse( + "Refused: `cat > file` with no input reads stdin until the tool times out. Write the " + + "file with the Write tool, or give cat a heredoc (`cat > file <<'EOF'`).", + ); + } + } } process.exit(0); @@ -113,7 +158,9 @@ function isForbiddenPnpm(args) { if (isTool(sub)) return true; // 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"); + // why: the whole lane is CI's; a stage runs only the spec files its diff touched. + if (sub === "test:console") + return !args.includes("--project=chromium") || !after.some((w) => w.endsWith(".spec.ts")); return SCRIPTS.has(sub); } diff --git a/.claude/agents/bug-triager.md b/.claude/agents/bug-triager.md index e8352597c..b1e6c2852 100644 --- a/.claude/agents/bug-triager.md +++ b/.claude/agents/bug-triager.md @@ -1,6 +1,6 @@ --- name: bug-triager -description: Triages one bug:triage issue — reproduces it as a failing test, finds the root cause, and posts a report proposing the simplest fix. Spawned through the triage-bug skill. +description: Triages one bug:new issue — reproduces it as a failing test, finds the root cause, and posts a report proposing the simplest fix. Spawned through the triage-bug skill. model: opus effort: high background: true @@ -12,16 +12,20 @@ You produce a report, not a fix. Rules in `docs/internal/agent-rules/`: `delivery.md` rule 8 defines the report, `testing.md` what a reproduction is worth, `toolchain.md` the commands. -Argument: an issue number. Without one, take the oldest: +Argument: an issue number. Without one, take the oldest `bug:new` issue +with no triage report on its thread: ```bash -gh issue list --label bug:triage --search 'no:assignee' --json number,title --jq 'sort_by(.number)[0]' +gh issue list --label bug:new --search 'no:assignee' --json number,title,comments \ + --jq '[.[] | select(all(.comments[]; .body | test("^## Root cause"; "m") | not)) | {number, title}] | sort_by(.number)[0]' ``` ## 1. Claim -The issue must carry exactly `bug:triage` and have no assignee. Otherwise -stop and say why. Then: +The issue must be open, carry exactly `bug:new` and have no assignee. +Otherwise (`bug:ready`, `bug:needs-info`, `bug:blocked`, any other label, +closed, or assigned) stop and say why. Triage is the only work on +`bug:new`; `bug:ready` is the maintainer's call. Then: ```bash gh issue edit --add-assignee @me @@ -65,7 +69,7 @@ Cannot reproduce after a genuine attempt: gh issue comment --body " *Written by an agent.*" -gh issue edit --add-label bug:needs-info --remove-label bug:triage --remove-assignee @me +gh issue edit --add-label bug:needs-info --remove-label bug:new --remove-assignee @me ``` and stop. @@ -161,7 +165,7 @@ is one paragraph. gh issue edit --remove-assignee @me ``` -Leave the label at `bug:triage`: `bug:ready` is the maintainer's decision. +Leave the label at `bug:new`: `bug:ready` is the maintainer's decision. ## Stopping early @@ -170,9 +174,9 @@ a slow-lane run): push whatever is on `bug/-repro`, leave one `## Handoff` comment (Done, Not done, Findings, Blocked on), and unassign yourself. Findings holds a partial root cause or a rejected hypothesis, so the next agent does not redo it. Waiting on the maintainer: also move the -issue to `bug:blocked`; the maintainer re-adds `bug:triage` when the answer +issue to `bug:blocked`; the maintainer re-adds `bug:new` when the answer is in: ```bash -gh issue edit --add-label bug:blocked --remove-label bug:triage +gh issue edit --add-label bug:blocked --remove-label bug:new ``` diff --git a/.claude/skills/deliver/SKILL.md b/.claude/skills/deliver/SKILL.md index d110da303..21cd06000 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 (`remaining`, `fixes`, the reviewer's arguments). Include immutable `base-sha`, `merge-base`, `head` and, after review, +report (`remaining`, `fixes`, the reviewer's arguments). Include `base-sha`, `merge-base` and `head`, copied from the `base`, `merge-base` and `head` lines of `.agents/scripts/pr-shas.sh ` run just before the stage starts (never a SHA typed from memory or `git rev-parse`), and, after review, `previous` plus the resolution ledger. Last, the text the agent would otherwise have to open: for an `implementer`, `brief:` and the full text of `.agents/briefs/implement-.md`; for a `reviewer`, `protocol:` and the @@ -85,7 +85,8 @@ nobody decided) does not stop the run either: 1. Take the conservative option: the one closest to what the code does today and the easiest to reverse. -2. Fold it into the issue body as rule 3 says, the line ending +2. Fold it into the issue body as rule 3 says (`issue-body.mjs +replace` or `set-section`), the line ending `(decided in delivery, pending the maintainer)`, with a Tests line that pins it. 3. Start the next stage on it, with a `decision:` line naming it. @@ -131,6 +132,12 @@ gh issue view --json number,title,labels,assignees,body - `task:ready` whose Depends on allow (delivery rule 1), `bug:ready`: one issue; go to step 2. - `feature:planned`: walk its open sub-issues (step 3). +- `bug:new`, unassigned: triage is the only work (delivery rules 1 and 8). + A triage report already on the thread with no newer reply: stop, it + waits on the maintainer's `bug:ready`. Otherwise run the `triage-bug` + procedure: start one `bug-triager` in the background with the issue + number, wait for its report, print it, and stop. Never add `bug:ready`: + it is the maintainer's call after reading the report. - Anything else, or assigned to someone else: stop and say why (rule 1). ## 2. Deliver one issue @@ -148,16 +155,22 @@ they name; never build again or restart review at round 1. `open` in any report: decide, then tell (above). 2. **Review**: `reviewer` with the PR number; it picks the lenses (`docs/internal/REVIEW.md`). Before every review round - and every fix run, run `node .agents/scripts/ci-triage.mjs ` and - `gh pr view --json mergeable` once. The triage reads the newest - finished CI run, which is often older than the head (newer pushes cancel - theirs), and sorts each failed test: `flaky` and `unknown` get its + and every fix run, run `.agents/scripts/pr-shas.sh ` and + `node .agents/scripts/ci-triage.mjs ` once. The triage reads the + newest finished CI run; exit 3 with a `stale:` line first means that + run is older than the head and the lines after it describe an older + commit: act on none of them. With `pending`, triage again before the + next fix run or the gate; with `missing`, the head's run was + cancelled, and the gate re-runs cancelled jobs once. A current run's + failed tests are sorted: `flaky` and `unknown` get its `rerun ` lines run as `gh run rerun --failed`, no fix run; `base` means the branch inherited the failure: run `.agents/scripts/stack-sync.sh ` and let CI run again; only `reach`, and `unknown` red a second time, joins the fix run's `fixes` - as `ci: run `. A conflict joins as `stack: merge -origin/`. With no fix run due, they get one before the + as `ci: run `; a `job ` line's indented lines + are the log's errors and join the same way, never read by hand. + `mergeable false` from pr-shas joins as `stack: merge +origin/`. With no fix run due, they get one before the round. 3. **Decide** on the review report: - Incomplete assessment: preserve findings and missing scope; park. It @@ -174,6 +187,11 @@ base-sha the previous check used>`, so the inputs script can map the 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. + - Every body edit goes through `node .agents/scripts/issue-body.mjs + --pr set-section ` (`replace ` for + one line; `` without `--pr` for an issue): the PR body's `## +Review`, `### Status` and `Notes so far:`, and a decision folded into + a task body. Never rebuild a body by hand or with inline python. - After each check persist the resolution ledger, the findings file (`docs/internal/REVIEW.md`, "Measurement"), counts and dispositions in `## Review`. @@ -214,9 +232,11 @@ base-sha the previous check used>`, so the inputs script can map the 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. Post the `## Review ledger` comment - as `docs/internal/REVIEW.md` "Measurement" says, fix-checks included: - one `Round : ` line per check first, and `round` on - every finding. Add + with `node .agents/scripts/review-ledger.mjs --issue --findings + --round : ...`, one `--round` per check in order, + fix-checks included, each SHA the `Commit` its report gave; it posts or + updates the comment as `docs/internal/REVIEW.md` "Measurement" says and + then runs `pr-body-check.mjs `, exiting with its code. 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 @@ -224,8 +244,9 @@ base-sha the previous check used>`, so the inputs script can map the Keep duplicate outcomes in the ledger, not Rejected. Tag rejections `[incorrect]`, `[intentional]` or `[nonblocking]`. Every blocking finding not fixed or open has its disposition recorded; omit - "Rejected:" when there is none. Then run - `node .agents/scripts/pr-body-check.mjs `: it checks the shape + "Rejected:" when there is none. The body check `review-ledger.mjs` + ends with (`node .agents/scripts/pr-body-check.mjs `; never run it + before the ledger is posted) checks the shape `.agents/scripts/delivery-stats.mjs` parses, the footer `*Written by an agent.*`, and the rule 12 budget of 200 words outside the checklist, Status, Assumptions and Review. Fix what it fails on; cut @@ -256,15 +277,17 @@ base-sha the previous check used>`, so the inputs script can map the .agents/scripts/merge-pr.sh ``` - Exit 0: merged. A conflict: an `implementer` fix run that merges the + While it waits it prints one `merge-pr: # pending check(s) ..., + min waited` line per poll and one `re-running ... run ` line + per re-run; nothing to act on. Exit 0: merged. A conflict: an `implementer` fix run that merges the base branch. Red CI: one `implementer` fix run with the failing test titles and the run id. Either way, the `fix-check` review in item 3 (none for a fix to tests outside the PR's diff), then the gate again. Any other exit, or red CI a second time: park with the line it printed. -After every stage, update the PR body's `### Status` section so any session -can resume from GitHub alone: the status line, then the Fix lines the next -stage works on, if any. +After every stage, update the PR body's `### Status` section (`issue-body.mjs + --pr set-section Status `) so any session can resume from GitHub +alone: the status line, then the Fix lines the next stage works on, if any. ``` ### Status @@ -323,11 +346,18 @@ origin/`, then a fix check on that PR, then the script again. stack stays open and nothing merges; the maintainer decides. **Merging the stack.** When every task has joined the stack and nothing is -in flight, run `.agents/scripts/stack-sync.sh ` once more (another -feature may have merged meanwhile), resolve a conflict as above, then the -gate on the top PR: `merge-pr.sh ` gates every PR in the stack and -merges them all, bottom to top. Red CI: the fix run goes to the lowest red -PR, then the same sync carries it up. +in flight: first the feature's ADRs. Each ADR under its Decisions is +_Accepted — not yet implemented_; send the top PR's branch one +`implementer` fix run with `fixes: adr: to Accepted` per ADR (a +status line and the index row, nothing else; rule 10), then the +coordinator's mechanical fix-check. Then run +`.agents/scripts/stack-sync.sh ` once more (another feature may have +merged meanwhile), resolve a conflict as above, then the gate on the top +PR: `merge-pr.sh ` gates every PR in the stack and merges them all, +bottom to top. Red CI: the fix run goes to the lowest red PR, then the +same sync carries it up. The feature closes itself: the issue-state +workflow closes a `feature:planned` issue when its last sub-issue closes +(rule 9), so post the run summary (step 4) on it and leave it. Stop when nothing is runnable and nothing is in flight. diff --git a/.claude/skills/triage-bug/SKILL.md b/.claude/skills/triage-bug/SKILL.md index eb2b1b45d..5401b9730 100644 --- a/.claude/skills/triage-bug/SKILL.md +++ b/.claude/skills/triage-bug/SKILL.md @@ -1,6 +1,6 @@ --- name: triage-bug -description: Triage a bug:triage issue — reproduce it as a failing test, find the root cause, and post a report proposing the simplest fix, without fixing it. Use when the user says "triage #N" or "triage the next bug". +description: Triage a bug:new issue — reproduce it as a failing test, find the root cause, and post a report proposing the simplest fix, without fixing it. Use when the user says "triage #N" or "triage the next bug". --- Arguments: $ARGUMENTS diff --git a/.github/labels.json b/.github/labels.json index c5c0f6f7f..3381dae0f 100644 --- a/.github/labels.json +++ b/.github/labels.json @@ -7,23 +7,13 @@ { "name": "bug:new", "color": "d73a4a", - "description": "Reported. Nobody has looked yet." - }, - { - "name": "bug:triage", - "color": "d73a4a", - "description": "An agent may reproduce it and write the triage report." + "description": "Reported. An agent may triage it (rule 8), and nothing more." }, { "name": "bug:needs-info", "color": "d73a4a", "description": "Could not reproduce. Waiting on the reporter." }, - { - "name": "bug:triaged", - "color": "d73a4a", - "description": "Triage report posted. Waiting on the maintainer to accept or send back." - }, { "name": "bug:ready", "color": "d73a4a", diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 20a8c76b9..18a9ac6c1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -175,7 +175,7 @@ jobs: # --no-renames lists a moved file's old path too. changed=$(git diff --name-only --no-renames HEAD^1 HEAD) if printf '%s\n' "$changed" | grep -v -E '^(src|ui)/.*\.test\.tsx?$' | grep -q -E \ - '^(src/|ui/|e2e/(console|fake-driver|helpers)/|playwright\.config\.ts$|package\.json$|pnpm-lock\.yaml$|pnpm-workspace\.yaml$|tsconfig[^/]*\.json$|\.github/workflows/ci\.yml$)'; then + '^(src/|ui/|e2e/(console|fake-driver|helpers)/|playwright\.config\.ts$|package\.json$|pnpm-lock\.yaml$|pnpm-workspace\.yaml$|turbo\.json$|tsconfig[^/]*\.json$|\.github/workflows/ci\.yml$)'; then echo "console=true" >>"$GITHUB_OUTPUT" else echo "console=false" >>"$GITHUB_OUTPUT" diff --git a/.github/workflows/issue-state.yml b/.github/workflows/issue-state.yml index 3cb027492..ea1304504 100644 --- a/.github/workflows/issue-state.yml +++ b/.github/workflows/issue-state.yml @@ -5,12 +5,12 @@ name: Issue state # - one : label per issue (adding the next label drops the old); # - a task:ready task whose Technical spec is emptied goes back to task:draft # (the spec session sets task:ready; Depends on is the deliver skill's order); -# - a reporter's reply moves bug:needs-info back to bug:triage, reopening the +# - a reporter's reply moves bug:needs-info back to bug:new, reopening the # issue if the stale job closed it (agents post under a maintainer's # account, so comments carrying the agent footer from rule 12 do not count); -# - a feature whose last sub-issue closed gets a note to check completion -# conditions and close; -# - a feature closed as completed closes the request it came from; +# - a feature whose last sub-issue closed is closed as completed, and so is +# the request it came from; +# - a feature a person closes as completed closes the request it came from; # - a PR closed without merge releases the claim on the issue it would close; # - a claim with no activity for three days is released; # - bug:needs-info with no activity for two weeks closes as not planned; @@ -118,12 +118,12 @@ jobs: await github.rest.issues.update({ ...context.repo, issue_number, state: "open" }); core.info(`#${issue_number}: reopened`); } - await github.rest.issues.addLabels({ ...context.repo, issue_number, labels: ["bug:triage"] }); + await github.rest.issues.addLabels({ ...context.repo, issue_number, labels: ["bug:new"] }); await github.rest.issues.removeLabel({ ...context.repo, issue_number, name: "bug:needs-info" }); - core.info(`#${issue_number}: bug:needs-info -> bug:triage`); + core.info(`#${issue_number}: bug:needs-info -> bug:new`); feature-complete: - name: Last sub-issue closed + name: Last sub-issue closed, close its feature and request if: github.event_name == 'issues' && github.event.action == 'closed' runs-on: ubuntu-24.04 permissions: @@ -136,7 +136,7 @@ jobs: const { repository } = await github.graphql(` query($owner:String!,$repo:String!,$number:Int!){ repository(owner:$owner,name:$repo){ issue(number:$number){ - parent{ number state labels(first:10){nodes{name}} subIssuesSummary{ total completed } } + parent{ number state body labels(first:10){nodes{name}} subIssuesSummary{ total completed } } }}}`, { ...context.repo, number: context.payload.issue.number }); const parent = repository.issue.parent; if (!parent || parent.state !== "OPEN") return; @@ -145,10 +145,29 @@ jobs: if (completed < total) return; await github.rest.issues.createComment({ ...context.repo, issue_number: parent.number, - body: `All ${total} sub-issues are closed. If the completion conditions hold on \`main\`, close this feature and flip its ADRs to *Accepted*.`, + body: `Closing: all ${total} sub-issues are closed. The PR that closed the last one walks the completion conditions (delivery rule 9); reopen this if one does not hold on \`main\`. Flip its ADRs to *Accepted* if delivery has not.`, }); - core.info(`#${parent.number}: all sub-issues closed`); + await github.rest.issues.update({ + ...context.repo, issue_number: parent.number, state: "closed", state_reason: "completed", + }); + core.info(`#${parent.number}: all sub-issues closed, closed as completed`); + // why: a close made with the workflow token starts no workflow run, so request-close never + // sees this one; this repeats its steps for the feature closed above. + const m = (parent.body ?? "").match(/^Request:\s*#(\d+)/im); + if (!m) return; + const issue_number = Number(m[1]); + const request = await github.rest.issues.get({ ...context.repo, issue_number }); + if (request.data.state !== "open") return; + await github.rest.issues.createComment({ + ...context.repo, issue_number, body: `Shipped as #${parent.number}.`, + }); + await github.rest.issues.update({ + ...context.repo, issue_number, state: "closed", state_reason: "completed", + }); + core.info(`#${issue_number}: closed, shipped as #${parent.number}`); + # why: only a feature a person closes reaches this job; feature-complete closes the request of + # a feature this workflow closed, since that close starts no run. request-close: name: Feature completed, close its request if: >- diff --git a/.gitignore b/.gitignore index 917ef5a24..07725b18c 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,6 @@ node_modules/ dist/ +.turbo/ *.log .DS_Store .fallow/ diff --git a/docs/internal/DELIVERY.md b/docs/internal/DELIVERY.md index ec038fe5c..caf94729a 100644 --- a/docs/internal/DELIVERY.md +++ b/docs/internal/DELIVERY.md @@ -8,17 +8,17 @@ holds its own steps. This page links to them rather than restating them. ## The model GitHub Issues is the spec and the queue. Every issue carries exactly one -`:` label, and agents act only on `bug:triage`, `bug:ready` -and `task:ready`. The body is the spec; a comment changes nothing until a -spec session folds it in. A reporter's issue is never rewritten. After -`ready`, state is read from GitHub itself (assignee, linked PR, closed). +`:` label, and agents act only on `bug:new` (to triage it, +nothing more), `bug:ready` and `task:ready`. The body is the spec; a +comment changes nothing until a spec session folds it in. A reporter's +issue is never rewritten. After `ready`, state is read from GitHub itself +(assignee, linked PR, closed). Issue `` is built on branch `/`. | Label | Meaning | | ----------------- | -------------------------------------------------------- | | `request:new` | Inbox. Never picked up, body never edited. | -| `bug:new` | Reported. Nobody has looked yet. | -| `bug:triage` | An agent may reproduce it and write the triage report. | +| `bug:new` | An agent may triage it, and nothing more. | | `bug:needs-info` | Could not reproduce. Waiting on the reporter. | | `bug:ready` | An agent may fix it. | | `bug:blocked` | Waiting on the maintainer to clear a blocker. | @@ -31,13 +31,14 @@ Issue `` is built on branch `/`. ## A bug -1. A bug arrives as `bug:new`. If it sounds real, add `bug:triage`. -2. `triage-bug` reproduces it as a failing test on `bug/-repro` and - posts one report comment (delivery rule 8). If it cannot reproduce, it - moves the issue to `bug:needs-info`. +1. A bug arrives as `bug:new`, and an agent may triage it from there. +2. `triage-bug` (or `deliver` on a `bug:new` issue) reproduces it as a + failing test on `bug/-repro` and posts one report comment (delivery + rule 8). The label stays `bug:new`. If it cannot reproduce, it moves the + issue to `bug:needs-info`. 3. You read the report. Agree: add `bug:ready`, which also accepts the - shape the report proposes for a gap. Disagree: reply and re-add - `bug:triage`. + shape the report proposes for a gap. Disagree: reply and run + `triage-bug` again; the new report supersedes the old one. 4. `deliver` builds the fix on `bug/` from the repro branch, reviews it and merges it. The triage test becomes the regression test. @@ -117,15 +118,17 @@ What any change includes without its spec asking is listed once, in - adding a state label removes the previous one; - an emptied Technical spec sends a `task:ready` task back to `task:draft`; -- a reporter's reply moves `bug:needs-info` back to `bug:triage`, and two +- a reporter's reply moves `bug:needs-info` back to `bug:new`, and two silent weeks close it; -- a closed last sub-issue asks you to close its feature, and a closed - feature closes its request; +- a closed last sub-issue closes its feature as completed (reopen it if a + completion condition does not hold), and a completed feature closes its + request; - a closed PR, or a claim silent for three days, releases the claim; - a PR from `/` must close `#` and nothing else. -Two transitions stay manual on purpose, one click each: `bug:new` → -`bug:triage` and `bug:triage` → `bug:ready`. +One transition stays manual on purpose, one click: `bug:new` → +`bug:ready`, after you read the triage report. Flipping a closed feature's +ADRs to _Accepted_ (delivery rule 10) is yours too, unless delivery did it. ## Watching the pipeline diff --git a/docs/internal/PROCESS-REVIEW-2026-10-25.md b/docs/internal/PROCESS-REVIEW-2026-10-25.md new file mode 100644 index 000000000..44a4d1983 --- /dev/null +++ b/docs/internal/PROCESS-REVIEW-2026-10-25.md @@ -0,0 +1,645 @@ +# Process review, 2026-10-25 + +Where the time went in every `deliver` and `spec-session` run on this +machine since #904 merged (2026-10-23 11:00), what each stage spends it +on, and what to change. Every number comes from the transcripts; every +proposal names the number it is judged by. An adversary pass (section 7) +ran on the first draft; the proposals below are what survived it, with +the corrections it forced. + +## 1. Scope and method + +Runs read: `deliver` for #905 (console data loading, 8 PRs), #906 +(device stream, 5 PRs), #961 (per-platform lease limit, 1 PR), #962 +(stopped at once), bugs #817 and #865; `spec-session` for #147, #946, +#947, #949, #950. That is 16 PRs, 160 tagged sub-agent runs (872 +agent-minutes), 6 orchestrator transcripts, 5 spec sessions with 65 +helper runs. + +Tooling: a transcript-to-timeline script (elapsed minute, gap, tool call, +duration, truncated text) over every main and `subagents/*.jsonl` file; +deterministic aggregates over the timelines (tool time by command class, +sub-agents running per minute, per-stage wall); one Haiku agent per task +cutting each timeline into labelled segments and reporting facts anchored +to minutes; one Fable agent attacking the draft. The grouping and the +judgement are mine. Scripts, reports and the adversary's report sit under +`/tmp/dlv` on this machine; the timeline script is worth promoting to +`.agents/scripts/` (item 22). + +## 2. Where the time went + +### 2.1 Runs + +| Run | Wall | Sub-agents running | No agent running | Of which human idle | Of which CI gate | Outcome | +| ---- | ------- | ------------------ | ---------------- | ------------------- | ---------------- | --------------------------------------------- | +| #905 | 563 min | 343 min | 221 min (39%) | 62 min | 91 min | 8 PRs merged at 405 min; then #906's #911 | +| #906 | 384 min | 166 min | 218 min (57%) | 185 min | 2 min | parked on disk at 137 min; 5 PRs merged later | +| #961 | 66 min | 51 min | 15 min | 0 | 9 min | 1 PR merged | +| #817 | 60 min | 15 min | 45 min | 15 min | 26 min, killed | PR #953 merged by hand | +| #865 | 20 min | 4 min | 16 min | 15 min | 0 | no PR: already fixed by #883 | +| #962 | 1 min | 0 | 1 min | 0 | 0 | waited on #966 of another feature | + +"Human idle" is time with no tool call and no sub-agent while a person was +needed. It splits: 160 min after #906 stopped on disk (the maintainer was +away from both sessions for three hours; no message in #905 either from +12:15Z to 16:44Z), 25 min after the hand purge, 21 min after #905 stopped +on a cross-feature dependency (the maintainer came back unprompted), 41 +min after #905's final merge with nothing waiting, 30 min across the two +bug runs waiting for a label. All four `PushNotification` calls answered +`Mobile push not sent (Remote Control inactive)`. + +The gate: #905 ran `merge-pr.sh` six times and merged on the fourth; two +failures were Console shards cancelled at CI's 15-minute job limit, two +were flaky tests, one a merge conflict after another feature landed. CI +runs take 6 to 19 min normally, with 61-minute outliers on queued +runners. Console shard 1 takes 9 min on every run; shards 2 to 4 take 2 to +3 (section 3.5, O2). + +### 2.2 Stages, all six runs + +| Stage | Runs | Agent-min | Avg | Of which model | Of which Bash | +| ------------------ | ---- | --------- | ---- | -------------- | ------------- | +| build | 14 | 281 | 20.0 | 102 | 163 | +| review coordinator | 35 | 152 | 4.3 | 26 | 26 | +| audit | 13 | 142 | 10.9 | 25 | 117 | +| fix | 27 | 138 | 5.1 | 31 | 105 | +| code lens (both) | 17 | 87 | 5.1 | 63 | 25 | +| claims lens | 19 | 17 | 0.9 | 12 | 5 | +| spec lens | 17 | 15 | 0.9 | 15 | 0 | +| tests lens | 5 | 13 | 2.5 | 11 | 2 | +| hardware | 3 | 3 | 0.9 | 1 | 2 | + +Model time and tool time are each about half of the 872 minutes. Tool +time by command, sub-agents and orchestrators together (519 min; about +half sits in compound commands the classifier calls `other` or `write`, +where edits, probes and test runs share one call): + +| Command class | Calls | Minutes | Note | +| ------------------------------------------------- | ----- | ------- | ------------------------------------------------ | +| vitest, test:changed, test:e2e | 168 | 101 | 36 s average; `test:changed` picked 176 files for `ui/` edits | +| local console lane (`test:console`) | 51 | 86 | whole lane 24 runs, 41 min; targeted 33 runs, 44 min; each run rebuilds first (about 20 s) | +| git commit (hooks) | 125 | 34 | 16 s average; the hooks alone take 3 to 6 s | +| stack-sync, merge-pr (orchestrator) | 32 | 24 | the gate's own wait runs in the background | +| PR and issue reads and writes | 116 | 17 | mostly the orchestrator's body edits | +| review-inputs.sh | 49 | 11 | 13 s average | +| ci-triage, ci-failures, gh run view | 74 | 10 | | +| git push (hooks) | 20 | 6 | | +| worktree.sh | 54 | 5 | 6 s average: clonefile plus offline pnpm | + +### 2.3 Per PR + +| Issue | PR | Changed lines | Files | Build | Audit | Fix | Review (coord + lenses) | Total | Ledger findings fixed | +| ----- | --- | ------------- | ----- | ----- | ----- | ---- | ----------------------- | ----- | ------------------------------ | +| 910 | 928 | 2221 | 52 | 38.1 | 15.2 | 25.9 | 47.6 | 127 | 1 (testing 3) | +| 915 | 937 | 2240 | 57 | 38.4 | 24.6 | 6.6 | 25.7 | 95 | 2 (manual stale, acceptance) | +| 911 | 943 | 792 | 29 | 46.7 | 14.7 | 2.1 | 23.2 | 87 | 1 (behaviour) | +| 909 | 923 | 989 | 31 | 19.5 | 5.8 | 30.5 | 26.5 | 85 | 1 (testing 10); slow lane 29.5 | +| 912 | 924 | 300 | 17 | 34.8 | 16.8 | 8.4 | 24.2 | 84 | 2 (testing 10, false comment) | +| 916 | 927 | 620 | 18 | 13.7 | 8.0 | 40.3 | 18.5 | 81 | 2 (behaviour) | +| 914 | 926 | 1339 | 28 | 30.3 | 7.3 | 8.2 | 28.6 | 74 | 4 (2 behaviour, 2 claims) | +| 966 | 975 | 1367 | 49 | 22.1 | 15.1 | 5.7 | 15.3 | 58 | 1 (comment) | +| 918 | 940 | 146 | 3 | 15.1 | 20.1 | 1.0 | 5.9 | 42 | 0 | +| 913 | 922 | 356 | 18 | 12.6 | 4.6 | 4.0 | 13.3 | 35 | 2 (manual, comment) | +| 917 | 939 | 293 | 5 | 4.8 | 5.9 | 1.4 | 12.9 | 25 | 3 (testing 3 twice, debug output) | +| 919 | 920 | 80 | 4 | 2.4 | 1.9 | 1.6 | 15.4 | 21 | 0 | +| 817 | 953 | 37 | 4 | 2.1 | 1.8 | 1.1 | 10.9 | 16 | 1 (docs example) | +| 886 | 938 | 3 | 1 | 0 | 0 | 0 | 10.3 | 14 | 0 | +| 905 | 907 | 161 | 3 | ADR | ADR | 0.6 | 11.0 | 12 | 1 (ADR text) | +| 906 | 908 | 83 | 2 | ADR | ADR | 0.8 | 7.0 | 8 | 0 | + +The five PRs under 150 changed lines cost 51 agent-minutes of review and +post-review fixes plus 24 of audit (20 of them #940's console-lane audit) +for one fixed finding, a docs example. The eight under 300 lines cost 105 +plus 46 for seven: three text fixes, three test-logic or coverage fixes +(`testing 3` twice, `testing 10`), one debug line; five of the seven were +the Opus code lens's. In this sample no behaviour finding sits under 600 +changed lines; in the previous review's sample #751 (194 lines) carried a +safety finding and #753 (176 lines) a behaviour one. Size does not predict +risk; path does. + +### 2.4 Spec sessions + +| Session | Agent time | Human answering | Interview rounds | Checker launches | Checker wall | Blocking / notes | Fix cycles | +| ------- | ---------- | --------------- | ---------------- | ---------------- | ------------ | ---------------- | ---------- | +| #147 | 30 min | 9 min | 4 (10 questions) | 4 + 4 rechecks | 31 min | 23 / 9 | 5 | +| #947 | 25 min | 12 min | 1 (3) | 3 + 2 | 23 min | 5 / 11 (1 task) | 3 | +| #949 | 28 min | 10 min | 3 (8) | 4 + 4 | 34 min | 14 / 15 | 3 | +| #950 | 47 min | 5 min | 4 (10) | 7 + 3 | 42 min | 19 / 18 | 3 | +| #946 | 49 min | 21 min | 2 (5) | 3 + 3 | 45 min | 10 / 9 (1 task) | 3 | + +Overnight approval waits (#946: 550 min; #949: 625 min inside the +delivery that followed) are the maintainer's time. Explorers ran 1 to 5 +per session, 2 to 16 min, beside the interview; two never reported back +(#949, #946), one was interrupted (#947). + +## 3. Findings by stage + +Classes: **expected** (the cost of a step we want), **improvable** (a +wanted step done dearer than needed), **unexpected** (cost no rule asked +for), **problem** (a defect in the process or its tools). + +### 3.1 Specification + +- **S1 — Checkers are half the session (improvable).** Checker wall is 23 + to 45 min of sessions whose agent time is 25 to 49 min. Rounds run in + parallel (5 to 10 min each) and every session ran 3 to 5 of them. The + cost is the cycle count (5 in #147), and the previous review's finding + that half of round-2 findings sit in round-1 fixes still holds. For a + one-task feature (#947, #946) the feature checker's cross-task + categories (overlap, shared values, contradiction between tasks) have + no input, though its completion-condition and ADR-fact checks do. +- **S2 — Round 0 pays (expected).** #947's round 0 merged two tasks into + one before they were written; #950's round 1 reopened a decision + (`Explain`, then a private profile file) and rewrote the ADR and three + bodies: 4.5 min of drafting repeated. Round 0 exists for that case. +- **S3 — Body editing by hand (improvable).** Every session and every + delivery edits issue bodies the same way: `gh issue view --json body`, + python `replace` with `assert count == 1`, `gh issue edit --body-file`. + #950 had two Write refusals and #949 an AssertionError on it; #905's + orchestrator did it 6 times, #961's twice. A minute per session, and the + place where the one-label and footer rules could be mechanical. +- **S4 — ADR number race (expected, rare).** #949 drafted ADR 0046, found + #954 held it, renumbered (0.7 min). Five sessions were started in two + batches the same evening; a branch push reserves nothing because the + branch is named for the feature, not the ADR. +- **S5 — Helpers that never report (problem, small).** Two background + explorers' results never reached the session (#949, #946); the session + read the sources itself. +- **S6 — Environment gaps (unexpected, small).** `simlock` not on PATH + (#147 ran an old dist build for a Facts row), `prettier` not found + (#949). +- **S7 — Human answering is 5 to 21 min (expected).** Rounds are few and + questions carry recommendations. +- **S8 — Dependency width is set here and nothing reports it (problem).** + #905's seven tasks formed a line (913, 914, 915, 916, 917, 918), so two + tasks were in flight for 72 of 343 busy minutes despite "up to two". + #906 waited on #909's review before #910 could start. The orchestrator + cannot widen a chain later; the spec session never saw its length. + +### 3.2 Implementation: build + +- **B1 — A healthy build is 85% engineering (expected).** #966's + 22-minute build: 51 segments, 35 commits, 9 pushes, 12.7% of wall on + rules, hooks and PR bookkeeping. Seven of 35 commits bounced on a hook + (3 typecheck, 4 comment lint) and were fixed in the next call. Hooks + run 3 to 6 s; the 16 s average commit is hooks plus git. 125 commits + cost 34 minutes: the price of the red-green record `review-inputs.sh` + reads (`tests-after-red.patch`). +- **B2 — The local console lane (unexpected).** 51 `test:console` runs + cost 86 agent-minutes: 24 whole-lane runs (41 min) and 33 targeted runs + (44 min). Toolchain allows the lane when the diff touches `ui/`; #905 + was all UI. The whole-lane runs are the waste: #915's audit started the + lane in the background and waited 10.3 min through a polling loop to + the 600 s tool limit; #918's audit spent 17.4 of 20 min in console + runs; #910's build 6.7 min. CI runs the same lane on every push. The + targeted runs are the brief's red-green and stay. Every run rebuilds + first (`pnpm run build && build:e2e`, about 20 s, `swift build` on a + Mac): 51 runs, about 17 min of rebuilding unchanged code. +- **B3 — Blocking commands and polling (problem).** The chain has four + links. (1) The Bash tool's default timeout is 120 s and 59% of test + commands in stage agents set none (5,482 of 9,354 across this + machine's worktree sessions); a two-minute test run is therefore + "moved to the background" (6 such moves in these runs, 3 more at 600 + s). (2) A bare `sleep N` is refused by the harness with the advice to + use Monitor or `run_in_background`; stage agents have no Monitor tool, + so they take the second route or write `until ... pgrep` loops (28 + loop or sleep calls, 48 min, 13 of them the slow-lane wait rule 16 + designs). (3) A loop runs to the 600 s limit and is itself moved to + the background. (4) A sub-agent with a live background job is not + finished: its completion reaches the orchestrator only when the job + ends or hits its 30-minute cap. #911's build shows all four: a + `cat > /tmp/x` with no input hung the push for 600 s, the push was + redone, the agent wrote its report at 30.8 min, and the orchestrator + heard from it at 46.5 when the orphaned job's 30-minute cap fired: + 26 of that build's 47 minutes. Stage agents lost about 36 min to this + chain; #906's hand-requested purge another 10. +- **B4 — Edits through Bash (unexpected, harness-driven, small).** + Deliver sub-agents made 274 edits with python heredocs, `perl -pi` or + `sed -i`, and 3 with the Edit tool. Failed calls across 145 runs: 60, + 226 s in all (`sed -i` without `''`, `sed: bad flag`, swallowed stdin). + Bypass-permissions mode tells agents to prefer Bash; an allowlist + instead would prompt inside background agents and stall the run. +- **B5 — `test:changed` is not "run its file" (unexpected).** 59 test + commands in builds averaged 40 s; `pnpm test:changed` selected 176 + files for a `ui/` edit. The brief says to run the cycle's file (5 to + 10 s). Either the brief is not followed or the import graph makes every + `ui/` edit touch everything; the number to read is files per + `test:changed` call. + +### 3.3 Implementation: audit and fix + +- **A1 — The audit is test execution (expected, bounded by B2).** 10.9 + min average, 117 of 142 minutes in tools: mutation probes and Project + checks. Audits found 1 to 11 test fixes on every code PR, 3 spec needs + on #966, 6 fixes and a spec need on #913. `report-guard` sent 4 audits + back to probe the rest, each under a minute. +- **A2 — Fix runs are cheap when they are fixes (expected).** 16 of 27 + under 2 min. The long ones were not findings: #916 twice (14.5 and + 18.5 min) on a toast test that hangs under load and a stack merge; + #909's 29.5 min was the slow lane (two test-code bugs, a lane run + each); #910's 15.7 min a hook replacement. +- **A3 — Flaky tests are the top gate killer (problem).** #916's toast + test cost two fix runs (33 min) and two gate cycles (33 min); + `screen.spec` failed a gate; `actions.spec.ts:166` (the disk-99% case) + cost three local whole-lane runs in #912, one in #914, a bug (#886), a + PR (#938) and its 14-min review. About 100 agent-plus-gate minutes + across #905 and #906. `ci-triage` classifies; nothing quarantines. +- **A4 — Spec needs decided in delivery (expected).** 15 decisions in + #905 and 1 in #961, each folded into the body with a `Decision taken` + comment; the maintainer accepted all at 446 min. About a minute each. +- **A5 — Round 2 for a fix's own side effect (improvable).** #924's round + 2 found a false `why:` comment the round-1 fix had introduced; + `implement-fix.md` step 2 already tells the fix run to check its hunks. + Five PRs had three checks; 35 coordinator runs for 16 PRs. + +### 3.4 Review + +- **R1 — The review floor does not scale down (improvable, bounded).** A + round is a Sonnet coordinator (4.3 min, half of it waiting), a spec + lens (0.9), a claims lens (0.9), an Opus code lens (5.1), then a fix + and a fix-check. #938 (3 lines) got three lenses, 10.3 min, 0 findings; + #920 (80 lines) 15.4 min, 0 findings. But the five smallest PRs + together cost 75 agent-minutes of audit and review, 20 of them the + console lane; a lighter path would recover 10 to 15 of the rest, and + five of the seven findings on PRs under 300 lines were the Opus lens's. + The floor is real and small; the lenses earn their keep by path, not + size. +- **R2 — Coordinator overhead (improvable).** A fifth of a coordinator's + time is `review-inputs.sh`, ledger, findings file and report. The + orchestrator posts the `## Review ledger` comment by hand (11 times in + #905, SHAs typed from memory) and twice ran `pr-body-check.mjs` before + the ledger existed (#817, #961), a failed call and a retry each. +- **R3 — Fix-checks are already cheap (expected).** Mechanical and + claims-only fix-checks ran 0.4 to 1.4 min with no lens. #975's one + finding (a comment) cost 0.5 min of fix and 0.4 min of check. +- **R4 — Lenses read what they were given (expected).** Code lenses spend + half their time reading code and a fifth verifying; a tenth is rule + files already in the prompt, which they cite. + +### 3.5 Orchestration and the gate + +- **O1 — The notification channel is broken (problem).** Every run's push + failed. The idle it explains is the 21 and 25 minute returns and part + of the 160; the three-hour absence it would not have cut, and the 41 + min after the final merge had nothing waiting. Still the one channel a + stopped run has, and it is off. +- **O2 — Gates fail more than they merge (problem).** Five of six gate + runs in #905 failed: two Console shards cancelled at CI's 15-minute + job limit (`ci.yml:192`), two flaky tests, one conflict; 91 min of + gate waiting, 22% of the 405 minutes to the merge. #906's stack was + merged by hand after a sixth gate was killed. Console shard 1 runs 9 + min on every `main` run while shards 2 to 4 run 2 to 3: `--shard` splits + spec files in order and the heavy files sit first, so the lane's wall + is one shard's, and that shard is the one the 15-minute limit cancels + on a slow runner. +- **O3 — Hand-built bookkeeping (improvable).** #905's orchestrator made + 97 bookkeeping calls (37 min of tool time plus the model time to write + them): 13 PR body writes, 11 ledger comments, 44 head/base SHA checks, + 27 ci-triage-plus-head calls, 17 stack-syncs, 10 reads of a background + gate's output, 13 `gh run view --log-failed` by hand. Between a + completion and the next launch it takes 0.3 to 0.9 min when it only + launches, 3 to 6 min when it also edits. Three times it read a CI + result older than the head and reasoned it out (`ci-triage` reads the + newest finished run by design). +- **O4 — Cross-feature dependencies stop runs (problem).** #905 stopped + because #915 depended on #912 of feature #906 (a loop the maintainer + cut by hand, 28 min); #962 stopped in one minute because #968 depends + on #966 of #961. Correct behaviour, no follow-through. +- **O5 — Bugs through the front door (improvable).** #817 and #865 were + `bug:new`; the orchestrator stopped, the maintainer typed "run + triage", read the report, typed "make it ready": 20 min before the + first implementer, most of it a person reading and deciding, as rule 8 + intends. #865's triage found the bug already fixed by #883. + +### 3.6 Environment: worktrees, disk, hooks + +- **E1 — Creation is fast; nothing removes (problem, hygiene).** + `worktree.sh` costs 6 s on average (clonefile of `node_modules`, + offline pnpm relink). No script removes one: 196 had accumulated by + 2026-10-23 and the orchestrator purged them by hand in 18.8 min through + a 600 s timeout, freeing 21.7 GiB (113 MB each). Today 31 remain besides + `main`; at least 10 are removable (2 task worktrees whose PRs merged, 5 + detached review probes the reviewer was told to remove, 3 T3 worktrees + of finished chores), about 1.1 GiB. Branch tips are never ancestors of + `main` (squash merges), so "merged" must be read from the PR. +- **E2 — Disk is not worktrees (problem).** The data volume was at 99% + (12.9 GiB free) when the slow lane needed 15 and #909 parked; it is at + 98% again today (22 GiB free). The weight is simulators (30 GB in + CoreSimulator), the pnpm store (48 GB), `.android` (29 GB). Worktree GC + keeps the count sane; a disk budget needs `pnpm store prune` and old + runtime removal, and a check before a run starts. +- **E3 — Umbrella issues stay open (expected by rule, unwanted).** + Until this review, delivery rule 9 said the maintainer closes the + feature; the deliver skill posted a run summary and stopped. All eight features delivered since + 2026-10-22 were closed by hand, three together in one session on + 2026-10-24. The rule 10 ADR status flip is manual too. The last PR's + body already walks the completion conditions (rule 9), so the check the + maintainer makes is the one the reviewer made. +- **E4 — Hooks are cheap (expected).** Typecheck 3 s, depcruise 4 s, + oxlint 2.4 s, fallow 2.8 s, typecheck:e2e 6.3 s, comment lint 0.1 s. + +## 4. What we change + +Grouped by stage; each item names its mechanism and the number it is +judged by. Savings are per run of the kind named, after the adversary's +corrections (section 7). Dropped first-draft items are listed in 4.6. + +### 4.1 CI and the gate (zero agent minutes; the gate was 22% of #905) + +1. **Balance the Console shards and lift the shard limit.** Playwright + splits whole files in alphabetical order: shard 1 holds + `actions.spec.ts` (16 tests, the slow polls), `activity`, `inbox`, + `layout`, `big-fleet` and `daemon-start-failure`, 54 tests; shard 4 + holds `workers` and `long-lists`, 46; shards 2 and 3 run in 2 to 3 + min. Shard by test (`fullyParallel: true` for the console project) or + by a file list sorted by measured duration, and set `timeout-minutes` + so the slowest shard on a slow runner fits (20). Mechanism: `ci.yml`, + `playwright.config.ts`. Saving: the lane's wall from 9 min to about 4 + on every CI run, and the two cancellation gate failures in #905 (about + 30 min of gate plus a hand merge of #906's stack). Measure: Console + shard max minutes per run; gates failed on `cancel`. +2. **Quarantine a flaky test on the second `unknown`.** When + `ci-triage.mjs` classes the same test `unknown` twice in one delivery, + the orchestrator opens the `flaky-test` issue and skips it (the + mechanism testing rule 5 already defines) instead of a third CI cycle + or a fix run. Mechanism: `ci-triage.mjs` output plus one skill line. + Saving: #916's toast test alone cost 33 min of fixes and 33 of gates. + Measure: fix runs that changed no production code; gate cycles per PR. +3. **`merge-pr.sh` reports progress** (one line per poll: pending checks + and minutes), so a 32-minute gate reads as a status and "what's the + status?" has an answer. Mechanism: script. Saving: nil in minutes. + +### 4.2 Specification + +4. **The feature checker on a one-task feature runs only its live + categories** (completion conditions, ADR facts), not overlap, shared + values or cross-task contradiction. Mechanism: `spec-checker.md` scope + text. Saving: a few minutes in 2 of 5 sessions. Measure: checker + minutes per one-task session. +5. **`issue-body.sh set-section ` and + `issue-body.sh replace `:** read, edit, assert one + match, keep the footer, keep exactly one state label, write. Used by + the spec session, the orchestrator's decisions and fold-ins. + Mechanism: script. Saving: about a minute per session and decision, + and the three assertion or refusal retries. Measure: hand + `gh issue edit --body-file` calls per session. +6. **A `chain` category in the feature check:** the longest Depends on + path against the task count, in the report and the approval summary, + so the maintainer sees "7 tasks, chain of 6" before approving. + Mechanism: `spec-checker.md`, skill step 9. Saving: none by itself; + #905 would have run two tasks at a time for most of its 343 busy + minutes if the chain had been 3. Measure: in-flight tasks per busy + minute in the next feature runs. +7. **A `depends-outside` finding** when a task's Depends on names an + open issue of another feature, so the maintainer decides at spec time + whether to wait or cut. Mechanism: `spec-checker.md` category. + Saving: the #905 loop (28 min) and #962's one-minute stop. +8. **The approval summary ends with the command to run next** + (`/deliver ` in a new session), and the end-of-run summary of a + delivery that waits on another feature names the issue to watch + (`Waiting: #968 — on #966`). Mechanism: text in two skills. Saving: a + human round trip per session (#949 and #950 both typed the command a + minute later). + +### 4.3 Implementation + +9. **Never the whole console lane locally, never in the background.** A + build or fix runs only the spec files it changed, Chromium only, in + the foreground under `timeout 550`; an audit probes the same way; the + full lane is CI's. Mechanism: `toolchain.md` "Tests agents run" row + (1) and the three briefs. Saving: the 24 whole-lane runs, 41 + agent-minutes across these runs, about half on the critical path; + #918's audit and #915's audit lose 10 to 17 min each. Measure: + `test:console` runs without a file argument (today 24). +10. **`pnpm test:console:fast`** skips `pnpm run build` and `build:e2e` + when `dist/` is newer than every file under `src/`, `ui/`, + `vendor/` and `e2e/`. Measured warm with nothing changed: the full + build is 13 s (daemon 4.5, ui 4.3, capture's Swift step cached) and + 20 s when the Swift step runs; `test:e2e` pays the same build on + each of its 17 runs. No task runner is installed (no turbo, nx or + wireit); turbo would give the same skip through an input-hash cache + and could share it across worktrees, at the price of a dependency and + its config, so the mtime check comes first and turbo is the follow-up + if cold builds in fresh worktrees (about 25 s each, 54 worktrees + created here) turn out to matter. Mechanism: package.json script, + toolchain row. Saving: 13 to 20 s on each of 68 console and e2e runs, + about 17 min across these runs. Measure: seconds per targeted console + run (today about 80, of which 13 to 20 is the build). +11. **Break the polling chain at its first link.** `tool-guard.mjs` + refuses, for a stage agent, (a) a Bash call that runs a test runner + (`vitest`, `test:*`, `playwright`, `slow-e2e.sh` excepted) with no + `timeout` parameter or one under 300000, telling it to pass + `timeout: 600000`; (b) `run_in_background: true` and a trailing `&`, + except for `slow-e2e.sh`, which is detached by design and waited on + through its `.exit` file (`hardware-verifier.md`, + `implement-hardware-fix.md`); (c) an `until`/`while` loop around + `pgrep` or a file test outside those two agents; (d) a bare + `cat > file` with no heredoc or input. The briefs say the same in + one line: foreground, `timeout` set, nothing left running. Mechanism: + guard rules plus a brief line. Saving: about 36 min of stage-agent + wall across these runs (#911's build 26, #915's audit 10), all on the + critical path, and the orphaned-job stall that holds a finished + agent's report for up to 30 min. Measure: tool calls moved to the + background (9 here); polling calls outside the slow-lane agents (15); + sub-agents whose report reached the orchestrator more than a minute + after their last tool call. +12. **Edit with the Edit tool.** One line in the implementer and lens + agent files. Mechanism: agent files. Saving: the 60 failed calls (4 + min) and reviewer context; small. Measure: Bash edits per build. +13. **`test:changed` for `ui/` edits is measured, then fixed in the + script.** Count files per `test:changed` call in the next builds; if + `ui/` edits select the whole suite, the script restricts by project + or the brief's "run its file" gets a hook that refuses `test:changed` + before the cycle's own file ran. Mechanism: measurement first. + Saving: up to 30 of the 39 build minutes spent in test runs in #905. + +### 4.4 Review + +14. **A fix run ends with the claims check on its own hunks**, as + `implement-fix.md` step 2 already asks; the fix report names it, and + a fix-check whose hunks are tests and comments only is then the + coordinator's mechanical check. Mechanism: enforce the brief's own + line in `report-guard.mjs` (the fix report carries `claims checked: + `). Saving: a round on fixes like #924's (its round 2 found + the false comment the round-1 fix wrote). Measure: round-2 findings + whose line the round-1 fix added. +15. **`review-ledger.sh :...`** posts the + ledger comment with `review-metrics.mjs` output, then runs + `pr-body-check.mjs`, in that order. Mechanism: script. Saving: 1 to 2 + min per PR; the two ordering failures. Measure: `pr-body-check` + failures per run. +16. **`ci-triage.mjs` says when its run is stale** (`stale: run is + , head is `, exit 3) and the skill says what follows: wait + for the next sub-agent's completion and triage again, never poll. + Mechanism: script plus one skill line. Saving: three stale reads in + #905, a minute each and a wrong fix run avoided. +17. **The router, as measurement first.** Build `task-size.mjs ` + (Tests lines, Modules touched, strict-area paths, contract or event + changes, ADRs) and give `diff-kind.mjs` a `small` kind (under 150 + changed lines, none in `src/leasing/`, the named `src/core/` files, + `src/contract/`, or a module whose manual names ownership, deletion, + persistence, credentials or concurrency). Record both verdicts in the + PR body for every PR, and route nothing differently yet. Then freeze + the `small` snapshots and replay them with a single Sonnet run + (`review-replay.sh --lens`, three runs each) under REVIEW.md's own + pilot bar: at least 20 snapshots, 10 with known bugs, recall not + below Opus, no higher clean-PR false-positive rate. Adopt a lighter + path only on that evidence. Why not now: in this sample `small` + routes two PRs (#938, #940), the saving is 10 to 15 agent-minutes + per feature run, and the previous review's #751 (194 lines) carried a + safety finding the Sonnet coordinators downgraded in replay. The + router's first use is the other direction: a `risk` verdict that + forces the Opus code lens and a second tests lens on a small diff in a + safety path, which REVIEW.md asks for in prose today. Measure: the + two verdicts per PR against the ledger over the next 20 PRs. + +### 4.5 Orchestration and environment + +18. **A notification that arrives.** The deliver skill tries + `PushNotification`, then `osascript -e 'display notification'` on + macOS, and always writes why the run stopped into the run-summary + comment. Mechanism: skill step 4. Saving: the 21 and 25 minute + returns and some of the 160; not the three-hour absence. Measure: + minutes from a run's last status line to the next human message. +19. **`pr-shas.sh `** prints base, merge-base, head, mergeable and the + head's CI state in one call, replacing the 44 SHA checks and 27 + triage-plus-head calls of #905. Mechanism: script. Saving: 3 to 5 min + of orchestrator turns per feature run; the API time stays. +20. **`/deliver ` on `bug:new` runs the triage and stops with the + report**, leaving `bug:ready` to the maintainer as rules 1 and 8 + require. Mechanism: skill step 1. Saving: one human round trip per + bug (the "run triage" message). Measure: messages before the first + implementer. +21. **`worktree-gc.sh`** removes a worktree when its branch's PR is merged + or closed (asked from GitHub), or it is detached and older than a day, + or its branch is a `*-repro`, `rv*` or `review-*` whose commits are on + origin; never with unpushed commits or tracked changes; prints what it + kept and why; `--t3` lists T3's worktrees for the person. Runs at the + end of every `deliver` run and in the reviewer's synthesis step + (which already says to remove probes). With it, a disk check at run + start: free GiB against the slow lane's 15, with `pnpm store prune` + and the simulator runtime list as the named remedies. Mechanism: + script plus two call sites plus skill step 1. Saving: the 18.8-minute + hand purge; the disk stall only together with the disk budget. + Measure: worktrees at run start; free GiB at run start. +22. **Delivery closes the feature; the ADR flip rides in the stack.** When + the stack merges and the last PR's body walked the completion + conditions, the orchestrator closes the feature as completed and says + so in the run summary (rule 9 amended: "the delivering agent closes + the feature when the last PR's body walks the completion conditions; + the maintainer may reopen"). The ADR's status line flips to + _Accepted_ in the last task PR before the gate, as a mechanical hunk + the fix-check verifies, so nothing is written to `main` outside the + gate (rule 15) and no ADR-only review runs. Mechanism: rules 9 and + 10 text, deliver skill steps 3 and 4. Saving: one hand session per + feature (eight since 2026-10-22). Measure: features closed by the + agent, reopened by the maintainer. +23. **The timeline script joins `.agents/scripts/`** as + `transcript-timeline.mjs`, with the command classes this review + lacked (`playwright test`, compound edit-and-run calls), and + `delivery-stats.mjs` gains: no-agent minutes per run, gate minutes + and outcomes, `test:console` runs with and without a file, 600 s + timeouts, Bash edits per build, files per `test:changed` call. + Mechanism: scripts. Saving: the next review is a command. + +### 4.6 Dropped from the first draft + +- `adr-next.sh` (a branch named for the feature reserves no number; the + collision came from five sessions in two batches). +- Replacing bypass mode with an allowlist (prompts inside background + agents stall an unattended run). +- A Sonnet single-run review path for `small` PRs (item 17 keeps the + router and defers the path to REVIEW.md's pilot bar). +- `--then-fix` on a bug delivery (it would label `bug:ready` before the + triage report exists, against rules 1 and 8). +- A `tool-guard` rule requiring the ledger tag on every `Agent` call + (explorers, triagers and EXPLORATION helpers are legitimately untagged; + the four untagged runs were bug-triager launches). +- Starting `/deliver` from inside the spec session (it would carry the + session's context and run from a checkout the skill may refuse). +- An ADR flip as a docs commit on `main` (a write outside the gate). + +## 5. What is not changed + +- The audit stays a separate fresh agent on every code PR: it found 1 to + 11 test fixes each time and the reviewer still found more. +- The commit-per-test rhythm stays: it is what `tests-after-red.patch` + reads, at 3 to 6 s of hooks a commit. +- Three independent lenses, the round cap and round 0 of the spec check + stay; rule 14 is not amended by anything here. +- The targeted console runs in builds stay: they are the brief's red and + green. + +## 6. Order + +1. Items 1 and 2 (CI shards, flaky quarantine): no agent minutes, and + five of six gates failed on exactly these. +2. Items 9, 10, 11 (whole-lane ban, fast console script, polling): about + 90 min of agent time and 36 of critical path across these runs. +3. Items 18, 21 (notification, worktree GC with the disk check): the + stalls. +4. Items 7, 22, 20 (depends-outside, feature close, bug triage entry): + the human round trips. +5. Items 15, 16, 19, 5 (the orchestrator's scripts), then 14. +6. Item 17 (router as measurement) and 23 (the stats), so the next review + measures this one. +7. Items 4, 6, 8, 12, 13, 3 as they come. + +## 7. Decisions, 2026-10-25 + +The maintainer read sections 1 to 6 and the adversary's report and decided +the same day; the first PR carries these: + +| Decision | Implemented as | +| --------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Turbo, not an mtime check (item 10) | `turbo.json` with per-step inputs and outputs; `build`, `build:*`, `test:e2e*`, `test:console`, `typecheck:e2e*` and `openapi` run through `turbo run`. Warm build with nothing changed 15 s to 2 s; one changed `src/` file 5 s. | +| Whole console lane never by a stage agent (item 9) | `toolchain.md` row (1) names the changed spec files; `tool-guard.mjs` refuses `test:console` without a spec file. Targeted red-green runs stay. | +| Break the polling chain (item 11) | `tool-guard.mjs` refuses, for sub-agents: a test runner without `timeout` of at least 300 s, `run_in_background` and a trailing `&`, a loop that sleeps, a bare `cat > file`; `slow-e2e.sh` and `.exit` waits exempt. One line in each brief. | +| Features close themselves; `bug:triage` goes | `issue-state.yml` closes a `feature:planned` issue when its last sub-issue closes and closes its request in the same job; `bug:new` is the triage queue (rules 1, 8, 9; `labels.json`; triage skill and agent; `deliver` step 1). | +| ADR flip rides in the stack (item 22) | `deliver` "Merging the stack": one fix run with `fixes: adr: to Accepted` on the top branch, then the mechanical fix-check; `implement-fix.md` knows the line. | +| Orchestrator scripts (items 15, 16, 19, 5, 3) | `issue-body.mjs`, `review-ledger.mjs`, `pr-shas.sh`; `ci-triage.mjs` exits 3 on a stale run and prints a failed job's log lines; `merge-pr.sh` prints a progress line per poll. The `deliver` skill names each where it replaces a hand step. | +| Rules that can be checks (item 19) | [RULES-TO-LINTS-2026-10-25.md](RULES-TO-LINTS-2026-10-25.md) lists 46 candidates with effort and confidence, and eight drifts a check would have caught; the first conversions follow in their own PR. | +| Not yet: the router (17), CI shards and quarantine (1, 2) | Proposed with numbers; the maintainer decides the CI change separately. | + +## 8. Adversary review + +A Fable agent read the first draft, the data and the rules with the brief +to prove each proposal brings little, overfits these runs, or breaks a +rule. Its report is `/tmp/dlv/ADVERSARY.md`. Verdicts on the first-draft +items and what changed: + +| First-draft item | Verdict | Objection, and what this document does | +| ---------------------------------------- | ---------------- | ---------------------------------------------------------------------------------------------------------------------------------------- | +| One checker for one-task features | weaken | The feature scope still checks completion coverage and ADR facts; and it was 2 of 5 sessions, not 3. Now item 4: drop the categories, keep the run. | +| `issue-body.sh` | hold | Make it enforce the one-label rule. Done (item 5). | +| `adr-next.sh` | drop | The branch is named for the feature, so a push reserves nothing. Dropped. | +| Approve-and-deliver in the spec session | weaken | Step 9 already ends on green; chaining `/deliver` inside the session carries its context. Now item 8: print the command. | +| Never the whole console lane; one run per spec file | weaken | "Once per spec file" contradicts red-green (floor is 44 min, not 25); ban the whole lane and the background only. Items 9 and 10. | +| No background jobs, no polling | weaken | The slow lane waits on `.exit` by rule 16; the harness itself backgrounds over-timeout calls; net 36 min, not 55. Item 11 exempts the lane. | +| Edit tool; allowlist instead of bypass | drop the allowlist | Measured cost 3.8 min of 872; prompts would stall background agents. Item 12 keeps the Edit line. | +| Router with a Sonnet single-run path | drop as specified | The gate as written routes #938 and #940 only (net 10 to 15 min); #751 (194 lines) carried a safety finding Sonnet downgraded in replay; 8 snapshots with no known bug cannot test it; rule 14 and REVIEW.md's pilot bar are skipped. Item 17 keeps the router as measurement and the path behind the pilot bar. | +| `review-ledger.sh` | hold | Item 15. | +| `ci-triage` stale exit | hold | Say what follows exit 3. Item 16 does. | +| A notification that arrives | weaken | 41 of the 247 min were after the final merge; 160 overlapped a three-hour absence. Item 18 claims the 21 and 25 minute returns only. | +| Stopped run says when to come back | weaken | Keep the `Waiting:` line; drop `schedule_task`. Item 8. | +| `depends-outside` spec check | hold | Cheapest real saving (28 min in #905). Item 7. | +| `pr-shas.sh` | weaken | API time stays; the saving is turns, 3 to 5 min. Item 19 says so. | +| Gate progress | hold | Nil saving, harmless. Item 3. | +| `/deliver ` with `--then-fix` | weaken | Labels `bug:ready` before the report exists (rules 1, 8). Item 20: triage, then stop. | +| `worktree-gc.sh` | weaken | 196 worktrees were 113 MB each; today's 10 removable are 1.1 GiB on a volume whose weight is simulators, the pnpm store and `.android`. Item 21 adds the disk budget and drops the disk-stall claim. | +| Delivery closes the feature, flips ADRs on `main` | weaken | A docs commit on `main` is a write outside the gate (rule 15); an ADR-only PR costs an Opus spec lens (11 min on #907). Item 22 rides the flip in the stack. | +| Ledger-tag guard on `Agent` calls | weaken | Untagged helpers are legitimate. Dropped. | +| Timeline script in the repo | hold | Add the missing classes: half the tool time sat unclassified. Item 23. | + +Levers the adversary found that the first draft missed, now items 1, 2, +6, 10, 13 and 14: the Console shard limit and imbalance, flaky-test +quarantine, dependency chain length at approval, the console rebuild on +every run, `test:changed` selecting 176 files for a `ui/` edit, and a +round 2 spent on a fix's own comment. + +Corrections it forced on the numbers: the small-PR set and its findings +(2.3), `test:console` 51 runs and 86 min with the whole-lane/targeted +split (2.2, B2), the polling minutes (B3), the idle decomposition (2.1, +O1), the worktree sizes (E1, E2), the one-task session count (S1), the +unclassified half of the tool-time table (2.2), and that behaviour +findings under 300 lines exist in the previous sample (2.3). diff --git a/docs/internal/REVIEW.md b/docs/internal/REVIEW.md index 7a26bb985..6a420d719 100644 --- a/docs/internal/REVIEW.md +++ b/docs/internal/REVIEW.md @@ -131,7 +131,10 @@ fix commit, once fixed) and `canonicalId` for a duplicate. Before the gate, post one PR comment headed `## Review ledger`: one line `Round : ` per check in order, the output of `review-metrics.mjs --pr --issue --findings `, then the findings file in a `json` -code block. The round lines are what lets a check be frozen and replayed +code block. `node .agents/scripts/review-ledger.mjs --issue +--findings --round : [--round ...]` builds the comment in +exactly this shape, posts it or updates the existing one, and then runs +`pr-body-check.mjs `; nobody writes the comment by hand. The round lines are what lets a check be frozen and replayed later (`replay-freeze.mjs`). The weekly `recurring-findings` routine reads these comments. Record escaped defects after merge. Do not call raised-minus-fixed a false-positive rate. diff --git a/docs/internal/RULES-TO-LINTS-2026-10-25.md b/docs/internal/RULES-TO-LINTS-2026-10-25.md new file mode 100644 index 000000000..cb997bde2 --- /dev/null +++ b/docs/internal/RULES-TO-LINTS-2026-10-25.md @@ -0,0 +1,580 @@ +# Rules agents must remember that a check could enforce + +Scouted 2026-10-25 on `chore/audit-delivery-process` (working tree, with its uncommitted edits to the briefs, `tool-guard.mjs`, `ci-triage.mjs`, and the untracked `issue-body.mjs`, `review-ledger.mjs`, `pr-shas.sh`). Read: every file in `docs/internal/agent-rules/`, `REVIEW.md`, `templates/*.md`, `.agents/briefs/*.md`, `.claude/agents/*.md`, `.claude/skills/*/SKILL.md`. Also read, to know what is already enforced: `lefthook.yml`, `.claude/settings.json`, every `.agents/scripts/*` header, `.github/workflows/{issue-state,ci}.yml`, `.dependency-cruiser.mjs`, `.oxlintrc.json`, and the repo-invariant tests in `src/*.test.ts`. I measured baselines with greps on the tree and on eight live task bodies (#917 to #919, #956 to #959, #966) and three feature bodies (#905, #952, #961). + +Effort: S is under an hour (one regex or one test). M is about half a day (a script with cases). L is a day or more. Confidence says how well the check matches what the rule means. Value is my guess at defects or agent-minutes saved per month. + +## Verdicts (maintainer review, 2026-10-25) + +Each item below is **apply** (in the next PRs, grouped as listed), **later** (right, not now) or **ignore** (with the reason). The eight drifts are fixed in the first conversion PR. + +| Verdict | Items | Reason | +| --- | --- | --- | +| apply, PR A: tests and catalogs, plus the drift fixes | R1, R13, R14, R15, R16, R17, R18, R19, R20, R22, R25, R30, R38, R45 | Repo tests and a diff-scoped `test-lint.mjs`. Five of them fail on real drift today; R13, R15, R38 and R45 are review-finding classes from the ledgers (time bounds, vacuous assertions, internal-file imports, unbounded wire fields) that build agents do not read the rule for. R30 widens an existing depcruise rule and needs a known-violations list that only shrinks. | +| apply, PR B: guards and PR checks | R3, R4, R5, R7, R8, R9, R10 (sub-agents only), R11, R12, R23, R24, R27, R28, R35, R39, R41, R42, R47 (the merge check only), R48, R2 | One regex or one function each in `tool-guard.mjs`, `report-guard.mjs`, `pr-body-check.mjs`, `issue-state.yml` or a `commit-msg` hook. R5 and R8 close silent holes (a slugged branch skips every delivery check; a `+refspec` force push is allowed). R4 makes the gate read what rule 15 says it "cannot read". R2 is the only M-sized one and the one most often broken (the footer and budgets on comments). | +| apply, PR C: the spec lint | R31, R32, R33, R34, R43 | `spec-lint.mjs` takes the structural half of the checker's categories before an Opus checker starts, with `runnable.mjs` replacing the orchestrator's hand check of Depends on, and the `chain` and `depends-outside` findings the process review asked for. R43 adds the website trailer check and the config-keys catalog test. Checker rounds are half a spec session; this is where the minutes are. | +| later | R29, R36, R37, R40, R46 | R36 needs a findings-schema field (severity) first; R37 would fight legitimate tables and the red-green record already serves the intent; R40 depends on SubagentStop firing for nested lenses (verify, then do); R29 is an input the spec lens can count itself; R46 needs the TS API for a rule that only applies when an index grows. | +| ignore | R21, R44, R49, R50 | R21 has no value (casing never drifted); R44 is covered by R25's test on every commit; R49 costs CI minutes for noise the weekly mutation run already produces; R50 is an AST comparison for a judgement call (one claim, one level). | +| ignore, R10's main-session half | — | The maintainer asks this session to set labels by hand; an `ask` rule would only add prompts. The sub-agent half stays. | + +## Drift the scout found (a check would have caught each) + +- **`SERVICE_FAILED` is missing from `website/src/content/docs/docs/reference/errors.mdx`.** The exit-code table lists 55 of `ERROR_TABLE`'s 56 codes. The code is only on `using-devices/errors.mdx:78`. See R17. +- **`docs/internal/VENDORED-CODE.md:17` names `vendor/agent-device-proxy-0.21.28-dev-253dca0.tgz`.** That file does not exist. `package.json` now takes `@agent-device/proxy` `0.21.28` from npm (#603). See R25. +- **Three unit tests assert a real-time bound outside a `*.timing.test.ts` file.** They are `src/agent-device/host/agent-device-supervisor.test.ts:511`, `src/agent-device/host/health.test.ts:62` and `src/daemon/dispatcher.test.ts:3893`. Testing rule 5 forbids it. See R13. +- **Three manuals name private members.** `src/gateway/AGENTS.md:104` (`#fleetLeases()`), `src/history/AGENTS.md:25` (`#start`) and `src/leasing/AGENTS.md:30` (`#refuseUnderHold`). The manual template forbids it. See R19. +- **A slugged branch escapes every delivery check.** The `branch-matches-issue` job (`issue-state.yml:291`) and `pr-body-check.mjs` match only `^(bug|feature|task)/\d+$`. So `task/118-foo` skips the Closes check, the Status and Assumptions checks, and the ledger check, all in silence. See R5. +- **`git push origin +task/1` (a force push by refspec) is not denied.** `settings.json` denies `--force` and `-f` only. Nothing guards `git stash` or `git rebase` either. See R8. +- **The tool guard has a false positive.** `ls .dependency-cruiser* vitest*` was refused as "a test run needs `timeout: 600000`", because `TEST_RUNNER` matches the bare word `vitest` anywhere in the command. The fix is to match it only in command position, the way `forbiddenCheck` already splits segments. +- **`bus/index.test.ts:276` checks the event names against `docs/internal/EVENTS.md` only.** The website catalog has the same row format and the same 53 names today, but nothing checks it. See R1. + +## Summary table + +Sorted by effort, then by value. "Where" names the place the check runs. + +| # | Rule | Check | Where | Effort | Conf. | +|---|---|---|---|---|---| +| R1 | events 8, always-in-scope 3: both catalogs | website `events.mdx` names == `EVENT_NAMES` | `src/bus/index.test.ts` (one path added to the `it.each`) | S | high | +| R4 | delivery 15: no blocking finding open, assessment complete | ledger JSON has no `outcome: open`; `## Review` holds `Assessment: complete` | `pr-body-check.mjs` (so `merge-pr.sh` too) | S | high | +| R13 | testing 5: no real-time bound outside timing files | added `expect(` on `Date.now()`/`performance.now()`/`hrtime` in a non-timing test | new `test-lint.mjs`, diff-scoped, in the commit hook and CI | S | high | +| R14 | testing 5: never skip or disable a test | added `.only`, `.skip`, `.todo`, `skipIf` in a test file | `test-lint.mjs` (or the oxlint vitest plugin) | S | high (`.only`), medium (`.skip`) | +| R15 | testing 10: assertions a failure also satisfies | added `toBeInstanceOf(Error)`, `?.(` inside `expect`, `not.toBe("pending")` | `test-lint.mjs` | S | medium-high | +| R3 | toolchain "Review measurement"; deliver "Delegating" | Agent prompt line 1 is the ledger tag; implementer prompt holds `brief:` + the file text; reviewer holds `protocol:` + REVIEW.md | `tool-guard.mjs` (Agent branch) | S | high | +| R5 | coding 3: branch from the issue only | a `bug|task|feature/` head must be exactly `/`, `bug/-repro` or `feature/-adr*`; a PR closing a task or bug must come from `/` | `issue-state.yml` branch job | S | high | +| R7 | build brief: commit shapes | on `/`: subject is `()?: … (#)`; merges exempt | lefthook `commit-msg` | S | high | +| R8 | implementer 6; delivery 15; fix brief | sub-agents: no `git stash`, `git rebase`, or `git push … +ref` | `tool-guard.mjs` | S | high | +| R9 | delivery 14; implementer "do not mark ready"; delivery 8 | sub-agent `gh pr create` needs `--draft`; no `gh pr ready` in sub-agents; no PR from `*-repro` | `tool-guard.mjs` | S | high | +| R10 | delivery 1 "agents never label their way into work" | refuse `--add-label bug:ready` for every agent; `task:ready`, `feature:planned` from sub-agents | `tool-guard.mjs` | S | high | +| R11 | delivery 16; triager, code lens: never the slow lane | vitest naming `e2e/slow-*` or `--project e2e` without `!slow`; `slow-e2e.sh` only for hardware-verifier and implementer | `tool-guard.mjs` (by `agent_type`) | S | high | +| R12 | implementer "helpers run in the foreground"; deliver "never pass a model" | sub-agent Agent call with `run_in_background: true`; `model` on implementer, reviewer or hardware-verifier | `tool-guard.mjs` | S | high | +| R17 | documentation 9: errors reference | every `ERROR_TABLE` code sits in `reference/errors.mdx`, in the row of its `cliExitCode` | new test in `src/contract/` | S | high | +| R25 | vendor 11: inventory true | every `vendor/` first-level entry has a VENDORED-CODE row and a `vendor/README.md` entry; every "Where it lives" path exists | `src/source-hygiene.test.ts` | S | high | +| R16 | documentation 3, always-in-scope 2: printed text names no repo path | no string literal in non-test `src/`, `ui/src/` names `docs/internal`, `agent-rules`, `AGENTS.md`, `CLAUDE.md` or a repo `.md` path | repo test | S | high | +| R23 | coding 2; delivery 12 "cut before posted, not excused after" | `## Assumptions` is `none` or only `- Assumption: … — …` lines; the 200-word budget fails instead of warning | `pr-body-check.mjs` | S | high | +| R24 | build brief `remaining`; audit "Promised" | `cycles b/t` with b`: sections, placeholders, footer, Tests count, hardware lines, contract bump wording, and more (see below) | spec-session before the checker; `issue-state` on `labeled task:ready`; deliver step 1 | M | high | +| R32 | task template Facts; spec-checker `fact` | every `path:line` in the body resolves at the body's `Commit `; Source never docs-only, "memory", "to re-check" or empty | `spec-lint.mjs` | M | high (existence), not truth | +| R33 | delivery 1; proposals 6 and 7 of the 2026-10-25 review | Depends on: exists, no cycle, open issue of another feature means `depends-outside`, chain length printed; `runnable.mjs ` | `spec-lint.mjs` feature scope; deliver step 3 | M | high | +| R34 | feature template Behaviours; spec-checker `untested` | per task: Behaviours rows naming it == bullets under `### Tests`; every row's Task is a sub-issue | `spec-lint.mjs` feature scope | M | medium-high | +| R2 | delivery 2, 8, 12; deliver step 5; spec-session | body guard: footer on every agent-written body and comment; Handoff with 4 sections ≤150 words; triage sections ≤300 words, ≤3 alternatives, ≤3 risks; "Spec updated" one line | `tool-guard.mjs` on `gh issue|pr comment|create|edit` and `gh api … body=@` | M | high | +| R35 | coding 5; deliver step 4; audit "Promised" | every issue `## Done when` bullet is a checklist line in the PR body; ready PR has all `[x]`; last sub-issue PR lists the parent's Completion conditions | `pr-body-check.mjs` | M | high (presence), not truth | +| R36 | deliver step 4; REVIEW "Measurement" | the `## Review` counts line is generated from the findings file; `pr-body-check` cross-checks it | `review-ledger.mjs` + `pr-body-check.mjs` | M | high | +| R37 | build brief "one test per commit" | each `test:` commit on `/` adds at most one new `it(`/`test(`/`it.each(` call site | lefthook `pre-push` | M | high (count) | +| R38 | testing 6; audit brief | a test that adds an import of a sibling internal file (component dir, not `index`, `testing` or `test-*`) has `why:` in the 3 lines above its first `describe(` | `test-lint.mjs`, diff-scoped | M | high (shape) | +| R39 | REVIEW "Routing"/"Select lenses" | `skipped` reviews match the `diff-kind.mjs` kind of merge-base..head | `pr-body-check.mjs` | M | medium-high | +| R40 | review briefs "Output" | lens output: list lines, `F B|N path:line … [class] ev: …` or `F0`, last line `Assessment:`, ≤400 words, ≤2 notes; code lens leaves its worktree clean | `report-guard.mjs` for the three lens agents | M | high | +| R41 | delivery 1, 2; triager step 1 | `claim.mjs `: assign only when unassigned and the label allows it; the guard refuses a raw `--add-assignee` | script + `tool-guard.mjs` | M | high | +| R42 | delivery 14, REVIEW cap | ledger `Round` lines: full SHAs, ancestors of head, at most one `1` and one `2`; at most one `## Review notes` comment | `pr-body-check.mjs` | M | medium | +| R43 | documentation 9 public surface | diff touches `tree.ts`, config defaults or validator, `ERROR_TABLE`, MCP tools, HTTP routes, client/admin index, `EventName` → diff touches `website/src/content/docs/`, or a `Docs: none ()` trailer; plus a config-keys catalog test | CI job (like protocol-bump) + repo test | M | medium (diff rule), high (catalog) | +| R44 | vendor 11 | diff adds, removes or renames a file under `vendor//` or edits `SHA256SUMS.txt` → diff touches its `NOTICE.md`, `VENDORED-CODE.md` and `vendor/README.md` | CI job | M | medium | +| R45 | always-in-scope 8, safety 10 | an added `z.string()`, `z.array(` or `z.record(` in `src/contract/*.ts` without `.max`, `.length`, `.regex` or a bounded helper | `test-lint`-style diff lint | M | medium | +| R46 | architecture 18 | an `index.ts` the diff grows exports no class | diff lint with the TS API | M | medium | +| R47 | delivery 10 | at stack merge every ADR in the stack's diff says `Accepted`; weekly: an ADR at "not yet implemented" has an open feature linking it | `merge-pr.sh`; weekly routine | M | medium | +| R48 | REVIEW "Routing" | a code-reviewer Agent call with `model` other than opus while `/diff.patch` touches strict-area paths | `tool-guard.mjs` | M | medium | +| R49 | testing 3 (strict areas), audit "Covered" | mutation on strict-area changed lines per PR; survivors annotate or fail | CI job | L | medium | +| R50 | testing 1, 7: cases as one table | sibling `it()` bodies in the diff with the same token shape apart from literals | diff lint, AST | L | medium | + +## Top five by value for effort + +1. **`test-lint.mjs` (R13, R14, R15, R38).** One diff-scoped script, built like `comment-lint.mjs`, run in the commit hook and in CI. It enforces the testing rules that build agents do not read (`testing.md` was opened 3 times in 29 runs). The time-bound and skip lints are about an hour each. Three real violations exist today. Diff-scoped, it needs no allow-list. +2. **The gate reads the ledger (R4).** `merge-pr.sh` says the agent "also checks what the script cannot read". The ledger comment now carries that data: a finding with `outcome: open`, and the Assessment line. Adding them is a few lines in `pr-body-check.mjs`, and it removes an unchecked step before merge. +3. **`spec-lint.mjs` before the Opus checker (R31 to R34).** The live bodies are regular enough to parse: the same 13 headings in all eight tasks, Facts tables with path:line sources and a `Commit .` line. About half of the checker's categories have a structural part. The Facts-resolve check (R32) and the Depends-on chain (R33) are the parts with most value. Checker rounds are half of a spec session today (process review S1). +4. **The body and comment guard (R2).** One PreToolUse rule covers the footer on every comment and body, the Handoff shape and budget, the triage-report shape and budget, the one-line "Spec updated" comment, and the Review notes shape. Today none of these is checked; only the PR body is. +5. **Catalog tests for the public surface (R1, R17, R25).** Each is under an hour, and R17 and R25 fail today on real drift. R1 is adding one path to an existing `it.each`. + +Close behind: the branch and commit shape checks (R5 and R7, which close the slugged-branch hole), and the tool-guard additions (R8 to R12, each a regex with tests). + +## Per-item detail + +The format for each item: rule, then the check (inputs and pass/fail condition), then false-positive risk, then an example that fails and one that passes, then the residual left to judgement. + +### S items + +**R1. Website event catalog.** +- **Rule:** always-in-scope 3, "A new or changed event updates the website's events reference … and `docs/internal/EVENTS.md`"; events 8; documentation 4. +- **Check:** in `src/bus/index.test.ts`, change `it.each(["docs/internal/EVENTS.md"])` (lines 276 and 282) to also take `"website/src/content/docs/docs/reference/events.mdx"`. `documentedEventNames` already parses that row shape. Both files hold the same 53 names today. +- **FP risk:** none. **Fail:** a new `EventName` with a row in EVENTS.md only. **Pass:** a row in both. +- **Residual:** that the payload columns agree. A second step could compare the payload field list per row. + +**R3. Prompt shape for stage and lens agents.** +- **Rule:** toolchain "Review measurement", "Every prompt a delivery stage or review lens is started with begins with this line". Deliver, "the full text of `.agents/briefs/implement-.md`" under `brief:`, and REVIEW.md under `protocol:`. Reviewer step 2 pastes each lens brief. +- **Check:** in the `tool-guard.mjs` Agent branch, when `subagent_type` is implementer, reviewer, hardware-verifier, spec-reviewer, code-reviewer or claims-reviewer: + - the prompt's first line matches `^ledger: (?=.*\bpr=\S+)(?=.*\bstage=(build|audit|fix|hardware-fix|review|hardware|replay)\b)(?=.*\brole=\S+)`; + - for an implementer, the prompt contains `brief:`, and the text after it contains the first 200 characters of `.agents/briefs/implement-.md`, with `` read from the prompt's `mode:` line; + - for a reviewer, it contains `protocol:` and REVIEW.md's first heading line. +- **FP risk:** the `implement` and `review` skills started by a person also go through here, but they already require the tag. **Fail:** `issue: 966\nmode: build`. **Pass:** `ledger: pr=- issue=966 stage=build round=- role=implementer lens=-\nissue: 966 …\nbrief:\n# Mode \`build\`…`. +- **Residual:** none for the shape. This is what makes "the rules reach the agent" mechanical, given that the agents do not open the rule files themselves. + +**R4. The gate reads the ledger.** +- **Rule:** delivery 15, "Before calling it the agent also checks what the script cannot read: no blocking finding is open and assessment is complete". REVIEW, "no incomplete assessment may be marked ready or merged". +- **Check:** in `pr-body-check.mjs`, when not `--draft-ok` and on a `/` branch: + - parse the json code block of the `## Review ledger` comment (the comments are already fetched for the ledger-exists check). Fail on any finding whose `outcome` is `open`; + - fail when the `## Review` section has no line `^Assessment: complete\b`. Today it fails only on `incomplete`. +- **FP risk:** routine PRs have no ledger; keep the `/` scope. **Fail:** a ledger with `{"id":"R2","outcome":"open",…}`. **Pass:** every outcome is `fixed` or another closed state, and the line `Assessment: complete` is present. +- **Residual:** whether a `nonblocking` disposition was right. + +**R5. Branch name strictness.** +- **Rule:** coding 3, "Work on issue `` of kind `` happens on `/` … No slug, no author prefix, no date. A PR from such a branch closes that issue and no other". +- **Check:** in the `branch-matches-issue` job, when the head starts with `(bug|feature|task)/` but does not match `^(bug|task|feature)/\d+$`, fail. The exceptions are `^feature/\d+-adr[\w-]*$` (ADR PRs, which have their own rule) and nothing else: `bug/-repro` must never have a PR (R9). Second, when the body closes an issue labelled `task:*` or `bug:*`, the head must be `/` with that kind and number. That needs one `issues.get` call. +- **FP risk:** maintainer chore branches do not start with a kind prefix, so they are unaffected. **Fail:** `task/118-fix-queue` and `Closes #118`. **Pass:** `task/118`. + +**R7. Commit message shape.** +- **Rule:** build brief, `git commit -m "test: (#)"`, and `(): (#)`. Triager, `test: reproduce # — `. +- **Check:** a lefthook `commit-msg` step `node .agents/scripts/commit-msg-check.mjs {1}`. With the branch from `git branch --show-current`: + - on `^(bug|task|feature)/(\d+)$`: the subject matches `^(feat|fix|test|docs|chore|refactor|perf|build|ci)(\([\w./-]+\))?: .+ \(#\2\)$`, or the parent feature's number for ADR commits on a stacked branch (allow any `(#\d+)`, warn on a mismatch); + - on `bug/-repro`: `^test: reproduce # — .+`. + - Merge commits (`Merge `) are exempt. +- **FP risk:** low. All 34 subjects on `origin/task/968` conform. **Fail:** `wip`. **Pass:** `test: a new simulator's marks name this instance (#968)`. + +**R8. Git commands sub-agents never run.** +- **Rule:** implementer checklist 6 "Never `git stash`"; build brief "all worktrees share one stash list"; fix brief "Never rebase, never force-push"; delivery 15 "a rebase a force push nobody may make". +- **Check:** in `tool-guard.mjs`, for a sub-agent, refuse a segment starting `git stash` (any form), `git rebase`, or `git push` with an argument matching `^\+` or `:\S+` (a refspec force or a remote delete). This uses the existing `forbiddenCheck` segment split. +- **FP risk:** `git rebase --abort` after a person's own rebase. Not a delivery case. **Fail:** `git stash push -u`. **Pass:** `git commit -m …`. + +**R9. Draft, ready and repro PRs.** +- **Rule:** delivery 14, "Every PR starts in draft". Implementer, "Do not claim, review, mark ready or merge". Delivery 8, the repro branch "opens no pull request". +- **Check:** in `tool-guard.mjs`, for a sub-agent: + - `gh pr create` without `--draft`: refuse; + - `gh pr ready`: refuse; + - `gh pr create` whose `--head` or current branch ends in `-repro`: refuse; + - the REST form `gh api … /pulls -F draft=` must carry `draft=true`. +- **FP risk:** routines run in the main session, so they are not affected. **Fail:** `gh pr create --title …` from the implementer. **Pass:** the same with `--draft`. + +**R10. Labels into work.** +- **Rule:** delivery 1, "Agents never label their way into work"; "only the maintainer moves a bug to `bug:ready`". Deliver step 1, "Never add `bug:ready`". +- **Check:** in `tool-guard.mjs`: + - any session (Claude is the actor): refuse `gh issue edit … --add-label bug:ready` and the REST form `labels[]=bug:ready`; + - sub-agents also may not add `task:ready` or `feature:planned`, which only the spec-session main session sets. +- **FP risk:** the maintainer asking Claude to do it by hand. Make it an `ask` rule in `settings.json` instead of a deny, for the main session. **Fail:** a triager adding `bug:ready`. **Pass:** `--add-label bug:needs-info --remove-label bug:new`. + +**R11. No slow lane outside its script and its agents.** +- **Rule:** delivery 16, "Agents run it only through the script". Triager, "Never run the slow lane". Code review, "never the slow lane". Implementer checklist 6. +- **Check:** in `tool-guard.mjs`: + - any agent: refuse `vitest` with an `e2e/slow-*.test.ts` operand, or with `--project e2e` and no `--tags-filter` excluding `slow`. Only `pnpm test:e2e:slow` is on `ask` today; `pnpm exec vitest run --project e2e e2e/slow-ios.test.ts` bypasses both the lock and the ask. + - `slow-e2e.sh` (other than `status`) when `agent_type` is neither hardware-verifier nor implementer. The hook input carries `agent_type`, as `tool-guard.test.ts:24` shows. +- **FP risk:** the implementer's non-hardware modes can still run it. Narrowing that needs the mode, read from the transcript's first user message. **Fail:** a bug-triager running `scripts/slow-e2e.sh e2e/slow-ios.test.ts`. **Pass:** a hardware-verifier running the same. + +**R12. Helpers in the foreground; no model on stage agents.** +- **Rule:** implementer, "Helpers run in the foreground … each an Agent call with `run_in_background: false`". Deliver, "Never pass a model or `isolation`". +- **Check:** in `tool-guard.mjs`, refuse an Agent call from a sub-agent with `run_in_background` not equal to `false`. In any session, refuse `model` on `subagent_type` implementer, reviewer or hardware-verifier. Lens model overrides are the reviewer's job, so leave lenses free. +- **FP risk:** `spec-session` starts background explorers, but it is the main session, so it is not affected. **Fail:** the implementer starting an Explore helper in the background. **Pass:** the same with `run_in_background: false`. + +**R13. Real-time bounds in unit tests.** +- **Rule:** testing 5, "A bound on real time belongs in a `*.timing.test.ts` file or the slow lane, never in a unit or e2e assertion". Build brief says the same. +- **Check:** `test-lint.mjs`, which reuses the `comments.mjs` machinery for added lines. For files matching `\.test\.tsx?$`, not `\.timing\.test\.ts$` and not `^e2e/slow-`, an added line fails when it matches `expect\([^;]*\b(Date\.now|performance\.now|process\.hrtime)\b`. It also fails on `expect\((\w+)\)\.toBe(Less|Greater)Than` where `\1` was assigned from those calls in the same test. Run it in the commit hook (`{staged_files}`) and in CI against `origin/`. +- **FP risk:** a `Date.now()` used to build fixture data, not a bound. The rule only fires inside `expect(`. **Fail:** `expect(Date.now() - started).toBeLessThan(2_000)`, as in `health.test.ts:62`. **Pass:** the same line in `health.timing.test.ts`, or `clock.advance(2_000)` with an exact assertion. +- **Residual:** bounds hidden in helpers. **Baseline:** 3, all left alone because the lint is diff-scoped. + +**R14. Focused and skipped tests.** +- **Rule:** testing 5, "Never skip, disable, or quarantine a test to get to green". +- **Check:** in `test-lint.mjs`, an added line matching `\b(it|test|describe)\.(only|skip|todo)\b`, or a `skipIf(`/`runIf(` with no `SIMLOCK_` environment gate on the same line. Alternatively, enable oxlint's `vitest` plugin with `no-focused-tests` and `no-disabled-tests` set to error. +- **FP risk:** the live-device gates (`process.env.SIMLOCK_LIVE_ANDROID === "1" ? it : it.skip`, `android/index.test.ts:4842`), which is why the environment exemption exists. **Fail:** `it.skip("grants …")`. **Pass:** `const live = process.env.SIMLOCK_LIVE_X === "1" ? it : it.skip`. +- **Baseline:** 30 matches tree-wide, most of them in `expected-failures.test.ts` fixtures. + +**R15. Assertions a failure also satisfies.** +- **Rule:** testing 10, "`not.toBe(pending)` passes on a rejection; `toBeInstanceOf(Error)` passes on a `TypeError`; `x?.()` passes when nothing was recorded". +- **Check:** in `test-lint.mjs`, added lines in test files matching `toBeInstanceOf\(Error\)`, `\.not\.toBe\(\s*["']pending`, or `\?\.\(` inside an `expect(…)` or in the statement right after an `await`. +- **FP risk:** `?.(` in setup code, so restrict it to `expect` arguments. **Fail:** `expect(failure).toBeInstanceOf(Error)`, as in `optional-modules.test.ts:461`. **Pass:** `expect(failure).toMatchObject({ code: "PROVISION_FAILED" })`. +- **Baseline:** 15 `toBeInstanceOf(Error)` tree-wide. + +**R16. Printed text names no repo path.** +- **Rule:** documentation 3, "Nothing the running tool prints or returns ever names a file path in this repository". Always-in-scope 2. +- **Check:** a repo test that walks `git ls-files 'src/*.ts' 'ui/src/*.ts*'` minus test, `testing.ts` and `test-*` files. Every string literal must not match `docs/internal|agent-rules|\bAGENTS\.md|\bCLAUDE\.md|\b(src|docs|e2e|scripts)/[\w/.-]+\.(md|ts)`. +- **FP risk:** `SKILL.md` (`src/instructions/setup.ts:32`) is a file the tool writes, not a repo doc, so allow it. **Fail:** `throw usage("see docs/internal/EVENTS.md")`. **Pass:** `"… https://callstackincubator.github.io/simlock/docs/reference/events/"`. +- **Baseline:** 0 hits. + +**R17. Error codes in the reference.** +- **Rule:** documentation 9, "HTTP routes, bodies and error codes … updates every website page that describes it — … the reference table that lists it". +- **Check:** a test in `src/contract/` that imports `ERROR_TABLE` and parses the `## Exit codes` table of `website/src/content/docs/docs/reference/errors.mdx`. Each code must sit in exactly one row, and that row's Exit cell must equal `cliExitCode`. +- **Fails today:** `SERVICE_FAILED` (`errors.ts:419`, exit 1) is in no row. **Pass:** `INTERNAL` in the `| 1 |` row. + +**R18. CLI commands in the reference.** +- **Rule:** documentation 9. +- **Check:** every `name: ""` in `src/cli/tree.ts` occurs in `reference/cli.mdx`, as a heading or as `` `` ``. Passes today. +- **FP risk:** hidden or internal commands, which need an exclusion list. +- **Residual:** whether each flag is described. A later step could compare flags per command from the tree. + +**R19. Manual shape.** +- **Rule:** manual template, "Name files and exported names, never private members (`#field`)"; "Drop a section that would be empty"; `Does`, `Entry`, `Uses` lines. Documentation 7. +- **Check:** in `src/docs-manuals.test.ts`: + - no `` `#[A-Za-z]\w*` `` token in a manual or flow doc; + - lines `^Does: `, `^Entry: `, `^Uses: ` present in each `src/**/AGENTS.md`; + - no `## ` heading followed by another heading with nothing in between. +- **Fails today:** the three private-member mentions. The other parts pass. + +**R20. ADR index status.** +- **Rule:** delivery 10; fix brief, "set that ADR's `**Status:**` line and its row in `docs/internal/adr/README.md`". +- **Check:** in `src/adr-index.test.ts`, compare each row's Status cell, up to the first `;` or `(`, with the ADR file's `- **Status:**` line, also cut at the first `;` or `(`. They match today (0 mismatches in a scratch check). +- **Fail:** the file says Accepted and the row says Accepted — not yet implemented. + +**R21. Doc file casing.** +- **Rule:** documentation 5. +- **Check:** a repo test: files directly under `docs/internal/` match `^[A-Z0-9-]+\.md$`; `adr/`, `agent-rules/` and `flows/` match `^[a-z0-9-]+\.md$` (plus `README.md`); `website/src/content/docs/**/*.mdx` are lower-case. Passes today. + +**R22. Event name shape.** +- **Rule:** events 1, "Names are `subject.past-tense-fact`, lowercase, dot-separated". +- **Check:** every key of `EVENT_NAMES` (`src/bus/index.test.ts:12`) matches `^[a-z][a-z-]*\.[a-z][a-z-]*$`, and its last hyphen word ends in `ed`. A short allow-list covers the exceptions there today (`ready`, `stopping`, `shutdown`). +- **Residual:** "facts, never commands" (events 2) is judgement once the word is allowed. + +**R23. Assumptions and the PR body budget.** +- **Rule:** coding 2, "recorded as one `Assumption:` line under `## Assumptions`". Implementer, `- Assumption: — `. Delivery 12, "Text over budget is cut before it is posted, not excused after". +- **Check:** in `pr-body-check.mjs`: + - the Assumptions section's non-blank lines are exactly `none`, or each matches `^- Assumption: .+ — .+`; + - move the 200-word budget from `warnings` to `failures` when the PR is not a draft. +- **Fail:** `## Assumptions\n- I picked the shorter message`. **Pass:** `- Assumption: refusal wording — today's text, the smallest change.` + +**R24. Report blocks, beyond the first key.** +- **Rule:** implementer Report; build brief "list the Tests lines not yet built under `remaining`"; audit "None: … leave the line unticked and say why under `open`"; reviewer Report. +- **Check:** in `report-guard.mjs`: + - **implementer:** `cycles: (\d+)/(\d+)` with b` exists on disk. Each `Spec|Code|Claims review:` is `n findings (b blocking)` or `skipped`. +- **Fail:** `cycles: 4/9` and `remaining: none`. **Pass:** `cycles: 4/9` with five `remaining` lines. + +**R25. The vendored inventory.** +- **Rule:** vendor 11, "updates the notice and the inventory in the same change". VENDORED-CODE.md's own header. +- **Check:** in `src/source-hygiene.test.ts`: + - each first-level directory under `vendor/`, and each archive at vendor's root, appears in a backticked path in the "Where it lives" column of a VENDORED-CODE.md inventory row and in `vendor/README.md`; + - every backticked `vendor/…` path in those rows exists. +- **Fails today:** row 17's `vendor/agent-device-proxy-0.21.28-dev-253dca0.tgz` is gone. + +**R27. The `adr:` flip is mechanical.** +- **Rule:** fix brief, "set that ADR's `**Status:**` line and its row … and change nothing else". Deliver, "then the coordinator's mechanical fix-check". +- **Check:** `adr-flip-check.mjs `. Each hunk of the commit changes only a `^- \*\*Status:\*\*` line in `docs/internal/adr/NNNN-*.md`, or one row of `docs/internal/adr/README.md`. The coordinator then records `verified by: coordinator` from the script's exit code. +- **Fail:** the commit also edits a paragraph. **Pass:** two one-line hunks. + +**R28. Routine PRs keep their kind.** +- **Rule:** garden-comments, "The review's kind must be `comments-only`". Audit-tests, "only test files changed". Harden-tests, "Never edit source code in this PR". Calibrate-reviews, a "docs-only PR". +- **Check:** in `pr-body-check.mjs`, by branch prefix, run `diff-kind.mjs ` and require: `comments-only` for `chore/garden-comments-`; `tests-only` for `chore/(audit|harden)-tests-`; for `chore/calibrate-reviews-`, `git diff --name-only` equal to `.agents/briefs/calibration.md`. +- **Fail:** an audit PR that touched `src/core/registry.ts`. + +**R29. Test count as a review input.** +- **Rule:** spec review Q3, "A line proven by more than one new test, or a new test no Tests line asks for … is a finding [testing 7]". +- **Check:** in `review-inputs.sh`, write `tests-count.txt` with the new `it(`/`test(` call sites in `diff.patch` and the bullet count under `### Tests` in `issue.md`. This is an input fact for the spec lens, not a gate. +- **Confidence:** medium. Extending a test adds no call site, which is the intended outcome, so a lower count is fine. + +**R30. Ports for external APIs.** +- **Rule:** architecture 9, "never import `fs`/`child_process` or call `Date.now()`/`setTimeout` directly in application code". Always-in-scope 7. +- **Check:** + - widen the depcruise `no-fs-or-process` rule's `from` to `^src/` with `pathNot` `^src/ports/|test` (today it covers six modules). Add `src/http/server.ts` to the known violations, or move it behind a port; + - add a grep test: no `\bDate\.now\(`, `new Date\(\s*\)`, `\bset(Timeout|Interval)\(` in non-test `src/` outside `src/ports/`, with a known list (`src/core/usage/index.ts:115` today) that only shrinks. +- **FP risk:** CLI-only code (`src/cli/`) may be deliberate. Decide which hosts count as application code. **Fail:** `import { readFileSync } from "node:fs"` in `src/gateway/x.ts`. + +### M items + +**R2. Body and comment guard.** +- **Rule:** + - delivery 12: "Every comment, issue body, and PR body an agent writes ends with the line `*Written by an agent.*`"; budgets: "a triage report 300 words, a handoff 150, … a 'Spec updated' comment one line, the review notes comment one line per note"; + - delivery 2: "exactly one comment headed `## Handoff` with four sections: Done, Not done, Findings, Blocked on"; + - delivery 8: triage sections "*Reproduction*, *Root cause* (with file and line), *Simplest fix*, *Alternatives rejected* (at most three, one line each), *Risk* (at most three bullets), and *Side findings*". +- **Check:** in `tool-guard.mjs`, for `gh (issue|pr) (comment|create|edit)` with `--body ` or `--body-file `, and `gh api … -F body=@` or `-f body=…`, read the body from the raw command (not `code()`, which blanks quotes) or from the file. Then: + 1. The last non-empty line is `*Written by an agent.*` or `_Written by an agent._`. This is the same regex as `pr-body-check`. + 2. A body starting `## Handoff` has exactly the four labels, as bold or headings, in order, and at most 150 words outside code blocks. A `Blocked on` that is not `nothing` prints a reminder to move the label. Better: an `issue-state` job on `issue_comment.created` moves the issue to `:blocked` itself, which makes the label half automatic. + 3. A body whose first heading is `## Reproduction` has the six headings in order (Side findings optional). Alternatives rejected has at most 3 non-blank lines, and Risk at most 3 bullets. Root cause contains `\S+\.\w+:\d+`. The whole report is at most 300 words outside code, and starts `Supersedes the report above.` when the thread already has one (needs a fetch: skip the check, or warn). + 4. A body starting `Spec updated:` is one line plus the footer. + 5. In a `## Review notes` body, each bullet is one line. +- **FP risk:** + - bodies built in a variable (`--body "$(cat x)"`) cannot be read. Refuse with "use `--body-file`"; + - `gh issue edit --add-label` carries no body and is not checked. +- **Fail:** `gh issue comment 9 --body "Done."`. **Pass:** a `--body-file` whose Handoff has the four parts, 120 words and the footer. +- **Residual:** "lead with the conclusion", "no narration". + +**R31. `spec-lint.mjs [--feature]`.** +- **Rule:** the task and feature templates, delivery 7, the spec-session self-check, toolchain "Slow lane" and "Contract shapes", and the structural halves of the spec-checker categories. Input is the posted body (`gh issue view` or `--body-file`) plus the GraphQL parent and sub-issues the checker already reads. +- **Pass/fail conditions.** Each is a regex once the body is split by heading, so each is S on its own; the parser and the cases make the whole M. + 1. **Sections.** A task has `Part of #N.`, then `## Scope`, `## In short`, `## Technical spec` with `### Facts`, `### Modules touched`, `### Contract and event changes`, `### Other code on the same state`, `### Failure modes`, `### Rules in play`, `### Tests`, then `## Done when`, `## Out of scope`, `## Depends on`, in that order. `### Surface and extension points` is optional. A feature has `Request: #N|none` and its eleven business sections, then Decisions, Shared values, Behaviours and Tasks. All eight live tasks pass. + 2. **Template leftovers.** Fail on `#NNN`, a table row of only `...` cells, a bullet that is only `- ...`, or a template HTML comment still present. + 3. **Footer and parent.** The last line is the agent footer. `Part of #N` equals the GitHub parent. + 4. **Tests size.** The `### Tests` bullet count is a note above about 10 and blocking above 30, unless Scope holds a "why it cannot split" line (checker `size`; spec-session's "about 10"). The first line after the heading names an existing test file: a backticked path that exists at the body's commit. + 5. **Modules size.** Distinct `src//` paths under Modules touched: more than 20 without a Scope reason is a finding. The section and Other code on the same state each hold a `Searches:` line (template: "one line names the searches"). + 6. **Failure modes.** A table with the header `Call | Outcome | What happens | Limit, counted from`, or the word `none`. + 7. **Contract wording.** When Contract and event changes is not `none`, it holds `bumps the protocol by one` and no `protocol (version )?\d+` or `PROTOCOL_VERSION_RANGE.*\d` (template: "never a version number"). When it names an event, `EVENTS.md` appears in it. + 8. **Hardware lines.** A Done when line matching `slow lane|real (simulator|emulator|device|iPhone|phone)|on a Mac with` names an `e2e/slow-[\w-]+\.test\.ts` that exists (checker `hardware`; toolchain "names the slow-lane test that proves it"). + 9. **Feature, planned.** With `feature:planned` or past it, Open questions is empty. + 10. **Feature words.** Words used is not empty (an entry, or `none`). + 11. **Diagrams.** In short holds a ```` ```mermaid ```` block (template). +- **Where:** spec-session step 7, after "Self-check", before posting: blocking findings are fixed before any checker starts. The `issue-state` workflow, on `labeled` `task:ready`: fail, then demote to `task:draft` with a comment listing the lines, as the emptied-spec job does now. Deliver step 1: refuse a task that fails. +- **Confidence:** high for each rule. **Residual:** the content, which is the Opus checker's. + +**R32. Facts resolve.** +- **Rule:** task template, "Source: … a `path:line` in this repo, … or a command and the output line it printed … Docs, memory or 'to re-check' is no source". Spec-session step 7. Checker `fact`. +- **Check:** in `spec-lint.mjs`: + - read `Commit .` under `### Facts`; fail when it is absent; + - for every Facts row, the Source cell is non-empty; + - it does not match `\b(docs?|memory|to re-?check|TBD|todo)\b` as its whole content; + - it is not only `website/…mdx` or `docs/…md` paths. A doc may back a fact about docs, so this one is a note when the Fact cell names code; + - every `` `path:line[-line]` ``, and each `:line` continuation that inherits the last path, resolves: `git cat-file -e :` succeeds and the line is at most the file's length at that sha; + - every other `path:line` in the body (Modules touched, Rules in play) resolves the same way at the same sha; + - Facts numbers are unique. A note for order: F20 sits after F22 in #966. +- **FP risk:** another tool's source (`git -C show v:`, or `node_modules/@agent-device/proxy/dist/...`) cannot be resolved in this repo. Skip paths under `node_modules/` or with a `v:` prefix, but require one of those two forms for a fact about another tool (toolchain "Other tools' source"). +- **Fail:** `| F3 | … | docs |`, or `src/core/config.ts:9999`. **Pass:** `src/core/config.ts:1088-1091` at `3de46d1ff`. +- **Residual:** whether the line bears out the fact, which the checker still reads. The lint removes the cheap half of the `fact` category. + +**R33. The Depends on graph.** +- **Rule:** + - delivery 1, "A `task:ready` task runs once every issue under its Depends on is closed, or is a task of the same feature whose PR passed review"; + - spec-session, "A Depends on only when …"; + - the 2026-10-25 process review, proposals 6 (`chain`) and 7 (`depends-outside`), which it puts in the Opus checker. +- **Check:** in `spec-lint.mjs --feature`, for each open sub-issue, parse `## Depends on` lines `^- #(\d+)`. Then: + - each number exists; + - an open issue that is not a sub-issue of this feature is a `depends-outside` finding, printed for the maintainer; + - a closed one is a note (needless); + - a cycle fails; + - the longest path and the task count print as `chain 6 of 7 tasks`. +- `runnable.mjs ` prints the tasks that are open, `task:ready`, unassigned, and whose every dependency is closed or a sub-issue with an open, non-draft PR. That replaces deliver's "Check the Depends on yourself". +- **Fail:** #913 → #914 → #915 → #916 → #917 → #918 prints `chain 6 of 7`. That is a printed note, not a failure. +- **Residual:** whether a Depends on is needed (overlap), which needs code. + +**R34. Behaviours rows and Tests lines.** +- **Rule:** feature template, "Every task's Tests lines are copied from this table, one line per row". Checker `untested`, "A Tests line that no Behaviours row backs, or one line per case, is a finding". +- **Check:** parse the feature's Behaviours table. Per sub-issue, the count of rows whose Task cell names it equals that task's `### Tests` bullet count. Each Tests bullet shares at least 60% of its content words with one row of its task. Every Task cell names a sub-issue, and every sub-issue has a row. +- **FP risk:** a Tests line naming an existing test to extend may be prose, not a bullet. Count bullets only. +- **Fail:** #956 has 5 Tests bullets and 4 rows. **Pass:** equal counts. +- **Residual:** whether the row's seam can see the claim. + +**R35. The PR walks Done when.** +- **Rule:** coding 5, "Every task PR walks its Done when lines". Build brief, "the Done when lines as an unchecked list". Audit, "Never tick a box on intent". Delivery 9 on the last sub-issue. +- **Check:** in `pr-body-check.mjs` with a PR number on a `/` branch: + - fetch issue `` and take its `## Done when` bullets; + - each must appear, normalized (case, backticks and whitespace folded, first 60 characters), as a `- [ ]` or `- [x]` line in the PR body; + - not draft: every one is `[x]`; + - when the issue's parent has no other open sub-issue, the parent's `## Completion conditions` bullets must appear too. +- **FP risk:** the orchestrator may rephrase a line when it walks it. Use a prefix match and allow an appended `— `. +- **Residual:** whether each tick is true. + +**R36. Review counts from data.** +- **Rule:** deliver step 4, the counts line, where "`` counts the blocking findings that review raised over all checks, `` those confirmed and fixed, `` its notes". +- **Check:** `review-ledger.mjs`, untracked today, already holds the findings JSON. Let it also write the `## Review` section's first two lines with `issue-body.mjs --pr set-section Review`. `pr-body-check` then recomputes them from the ledger JSON and fails on a mismatch. The findings schema needs one more field (`severity: blocking|note`) or a separate notes count, because notes are not in the file today. +- **Residual:** none for counting. The Rejected lines stay prose. + +**R37. One test per `test:` commit.** +- **Rule:** build brief, "Commit the test and its stubs, one test per commit". A line that lists cases is one test. +- **Check:** a pre-push step on `/`. For each commit in `origin/..HEAD` whose subject starts `test: `, count added lines in test files matching `^\+\s*(it|test)(\.each\(|\()`; fail when it is above 1. When the commit changes a file outside the tests, its added non-test lines should hold only `export` declarations or stub bodies. That second part is a warning only. +- **Fail:** a `test:` commit adding three `it(` calls. **Pass:** one `it.each(` with a table. +- **Residual:** extend-not-copy (testing 7). + +**R38. Internal-file test imports need `why:`.** +- **Rule:** testing 6, "A test of an internal file earns its place only for pure logic with many cases … or for an ordering the surface cannot force, and says which in a `why:` comment above its `describe`". Audit brief, the same. +- **Check:** in `test-lint.mjs`, for a test file the diff adds or whose import lines it changes: + - the file sits in a directory with `index.ts`; + - a new import has a `./x.js` specifier that is not `index`, `testing` or `test-*`; + - then a `why:` comment must sit within the 3 lines above the first `describe(`. +- **Baseline:** 93 such files tree-wide, 3 with the comment. Diff-scoped, so the old ones are left alone, the same deal comment-lint makes. +- **Fail:** a new `src/leasing/wait-queue.test.ts` importing `./wait-queue.js` with no comment. **Pass:** `// why: pure planner, 14 cases` above the describe. +- **Residual:** whether the reason is true. + +**R39. Lenses match the diff kind.** +- **Rule:** REVIEW "Routing", "ADR-only: spec. Docs or comments: spec and claims. Tests: spec, code/tests and affected claims. Other changes: spec and code/both …". Deliver step 4, "`skipped` … for a review the diff's kind left out". +- **Check:** in `pr-body-check.mjs`, run `diff-kind.mjs `. Fail when: + - `Code review: skipped` and the kind is `full`, `large` or `tests-only`; + - `Spec review: skipped` on a `/` branch, where an issue exists; + - `Claims review` is not `skipped` on `adr-only`. That is a warning; extra lenses are allowed. +- **Residual:** "affected claims" (whether claims had to run on a full diff). + +**R40. Lens output shape.** +- **Rule:** the three review briefs' Output sections: "One line per defect, in this form and no other"; "at most 400 words besides the list"; "at most two" notes; "End with `Assessment: …`". Code brief, "Restore the tree before you report". +- **Check:** extend `report-guard.mjs`'s SubagentStop matcher with `spec-reviewer|code-reviewer|claims-reviewer`, assuming nested sub-agents fire SubagentStop (to verify). + - The last line matches `^Assessment: (complete|incomplete — .+)$`. + - Each line starting `F\d` matches `^F\d+ [BN] \S+:\d+ .+ \[[^\]]+\] ev: .+`, or the line is `F0`. + - At most 2 `N` lines. + - At most 400 words outside the list lines (`path: … ok|F\d`). + - For a code-reviewer, `git -C status --porcelain` is empty. +- **Residual:** content. + +**R41. Claim script.** +- **Rule:** delivery 2, "it never touches an issue that already has an assignee". Delivery 1, the labels that are work. Triager step 1. +- **Check:** `claim.mjs [--triage]` runs `gh api issues/`, then refuses (exit 1) when there is an assignee, when the label is not one of `bug:new` (with `--triage`), `bug:ready` or `task:ready`, when the issue is closed, or, for a task, when R33's runnable test fails. Then it assigns `@me`. `tool-guard.mjs` refuses a raw `gh issue edit … --add-assignee` and points at the script. +- **Residual:** none. + +**R42. Round lines and notes once.** +- **Rule:** REVIEW, "one full round, one targeted round 2, and one final targeted fix-check"; "Notes go out once, after the last check". Deliver, "one `Round : ` line per check". +- **Check:** in `pr-body-check.mjs`, ledger `^Round (1|2|fix-check): [0-9a-f]{40}$` lines: + - at most one `1` and one `2`, and `1` first; + - every sha is an ancestor of head, which needs `git merge-base --is-ancestor` after a fetch, so warn when the sha is unavailable; + - at most one comment headed `## Review notes`. +- **FP risk:** the hardware loop adds a fix-check after the cap (deliver item 3), so do not cap fix-checks at one. Medium confidence. + +**R43. Public surface reaches the website.** +- **Rule:** documentation 9, "Nothing generates or checks these pages: the author and the reviews keep them true". Always-in-scope 1. +- **Check, part 1:** a CI job like protocol-bump. When the diff changes any of these, it must also change a file under `website/src/content/docs/`, or a commit must carry the trailer `Docs: none ()`: + - `src/cli/tree.ts`; + - the config defaults or validator in `src/core/config.ts`; + - the `ERROR_TABLE` in `src/contract/errors.ts`; + - an MCP tool definition; + - a route declaration under `src/http/`; + - `src/client/index.ts` or `src/admin/index.ts` exports; + - the `EventName` union. +- **Check, part 2:** a catalog test: every key path the config validator accepts appears as `` | `key` | `` in `reference/configuration.mdx`. +- **FP risk:** an internal refactor of a listed file. That is what the trailer is for. +- **Residual:** whether the pages are right. + +**R44. Vendored change, same-change inventory.** +- **Rule:** vendor 11. +- **Check:** a CI job. When `git diff --name-status` shows an `A`, `D` or `R` under `vendor//` (other than `NOTICE.md`), or an `M` of `SHA256SUMS.txt` or of the upstream commit line in `NOTICE.md`, the diff must also touch `vendor//NOTICE.md`, `docs/internal/VENDORED-CODE.md` and `vendor/README.md`. +- **FP risk:** a change to "our" files in `vendor/screen-capture/` needs no inventory change. Hence the `A`/`D`/`R` restriction. Medium confidence. + +**R45. Wire input bounded.** +- **Rule:** always-in-scope 8, "validated for shape and bounds before it is used or stored". Safety 10. Task template, "Every new field read from the wire … its bounds". +- **Check:** in a diff lint over added lines in `src/contract/*.ts` (not tests), `z\.string\(\)` with no `.max(`, `.length(`, `.regex(`, `.uuid(`, `.datetime(` or a known bounded helper on the same expression fails. So do `z\.array\(` with no `.max(` and `z\.record\(`, unless a `why:` sits on the line above. +- **Baseline:** 197 of 214 `z.string()` have no `.max` tree-wide, so it must be diff-scoped. Bounds may also live in refinements. Medium confidence. +- **Residual:** "match by structure, never substring", and "unknown fails closed". + +**R46. Factory surface.** +- **Rule:** architecture 18, "It does not export the classes it is built from … An index that predates this rule is not a finding until a change grows it". +- **Check:** for each `src/**/index.ts` in the diff, resolve the added export names with the TS compiler API. Fail when one is a class declaration, or when there is an added `export class`. Allow error classes (`extends Error`). +- **Residual:** "the composition root imports … only the factory". + +**R47. ADR status at merge.** +- **Rule:** delivery 10, "flips to *Accepted* when the feature closes". README, "An ADR that is *Accepted — not yet implemented* with no open feature behind it is a gap". +- **Check:** in `merge-pr.sh`, when merging a stack whose top PR closes the last open sub-issue of a feature, every `docs/internal/adr/*.md` the stack adds or changes must read `**Status:** Accepted` (not Proposed, not "not yet implemented"). Also a weekly report of ADRs at "not yet implemented" that no open feature's Decisions section names. +- **Confidence:** medium. The process review proposal 22 moves the flip into the last task PR, and the check holds either way. + +**R48. Opus on high-risk code review.** +- **Rule:** REVIEW "Routing", "Ownership, destruction, persistence/recovery, concurrency, trust boundaries … require Opus/high code review, regardless of LOC". +- **Check:** in `tool-guard.mjs`, for an Agent call with `subagent_type: code-reviewer` and `model` set and not `opus`, read the `inputs:` path from the prompt. When `/diff.patch` touches the strict areas (toolchain "Project checks") or `src/contract/` validators, refuse. +- **FP risk:** the `pilot: sonnet` replay runs, which can be exempted when the prompt says `stage=replay`. +- **Residual:** semantic risk outside the named paths. + +### L items + +**R49. Per-PR mutation on strict areas.** +- **Rule:** testing 3, "In the strict areas … production code that can be deleted with a green suite is untested". Audit "Covered" with `covered /`. +- **Check:** a CI job, only on PRs whose diff touches the strict areas, runs `pnpm mutate --max-lines 300` restricted to those files. Surviving mutants post as annotations, then as a failure once it is calibrated. +- **Cost:** CI minutes and noise from equivalent mutants. Today it runs weekly on `main` only. + +**R50. Copies that should be a table.** +- **Rule:** testing 1 and 7, "as one table when the cases differ only in data, never one test per case". Audit-tests `TABLE`. +- **Check:** parse added `it(` and `test(` blocks with the TS AST. Normalize literals and identifiers. Two sibling blocks in one `describe` with the same token sequence are a finding. +- **Residual:** whether they claim one behaviour. + +## Spec-checker categories: what a lint can take before the Opus checker + +| Category | Structural part (`spec-lint.mjs`) | Needs code or judgement (stays with the checker) | +|---|---|---| +| contradiction | none worth it | all | +| unverifiable | a Done when line with no command, test path or file: a note only | whether a reviewer could check it | +| untested | Behaviours rows == Tests bullets per task (R34); a Tests line per case: a bullet whose text is a single literal case | each sentence that states visible behaviour has a line; the seam can see the claim | +| failure-mode | the Failure modes table has its columns or `none` (R31.6) | which failures are missing; bounds | +| interaction | a `Searches:` line with hit counts, or `None` (R31.5) | which writers of the state exist | +| derived-list | Modules touched has a `Searches:` line | whether the list is complete | +| overlap | Depends on refs, cycle, depends-outside, chain (R33) | two tasks touching the same lines | +| hardware | a slow-lane Done when line names an existing `e2e/slow-*.test.ts` (R31.8) | whether a line needs the lane but does not say so | +| undefined | Words used present (R31.10) | which terms need defining; "as today" claims | +| size | Tests bullets above 10 or 30; distinct modules above 20 without a Scope reason (R31.4, R31.5) | none | +| fact | a commit line, a non-empty non-docs Source, every `path:line` resolves (R32) | the line bears the fact out | +| boundary | none | the receiver accepts the value | +| shared-values | Written by and Read by cells name sub-issues; a value cell repeated verbatim in a task (low confidence) | values two tasks use that the table lacks | +| (feature) completion | each Completion condition bullet appears in the Tasks section with a `#n` | whether that task delivers it | + +The structural share is roughly half of the categories by count, and most of the "writing findings" the spec session fixes itself. It is none of the decisions, which are what round 0 exists for. + +## Not convertible (needs judgement) + +- testing 1, the body proves the title: needs the meaning of the title. +- testing 2, the test fails for the right reason: only a probe shows it. `probed p/t` is guarded; R49 is the mechanical version. +- testing 3, what a caller can see: visibility is semantic. +- testing 4, an invariant test sees its whole rule: needs the rule's scope. +- testing 6, a fake port's sequence is not asserted unless order is the claim. +- testing 7, extend instead of copy; one claim, one level. R37 counts; the choice is judgement. +- testing 8, a property shared by every instance is one test. +- testing 9, a regression test only for a bug that reached `main`. +- testing 10, the remaining shapes: computed expected values, state read from a copy, an unstraddled time boundary, order read from the end state. +- architecture 1, 2, 3, the core is platform-agnostic. A term grep in core hits 14 lines, all comments or event names: too noisy. +- architecture 4, 5, a module boundary for observers versus a workflow step. +- architecture 6, state only through the transition function. A grep for direct `state =` writes is possible, but the false positives need a look. +- architecture 7, 8, destructive verbs only through the core; a thin CLI. +- architecture 10, one place for a rule. Fallow duplication catches copies of code, not of decisions. +- architecture 11, 12, bounded waits, budgets that do not reset, exits that leave one named state. +- architecture 13, claims true. That is the claims lens; `stale-refs.sh` feeds it. +- architecture 15, 16, 19, 20, 21, dependency direction for "optional" components (partly depcruise already), pure policy, when to add an extension point, a host naming an implementation. +- architecture 17's content: a `why:` that says what. The shape is mechanical already. +- safety 1 to 4, 6 to 10: ownership, leased devices, rules that propose, consent, attribution, reconcile, fail closed, wire claims. Behaviour, covered by tests. +- safety 5: probably pinned by `src/cli/index.test.ts` already (it mentions `--yes`); check. +- events 2 to 6: facts not commands, post-commit emit, never await, isolated handlers, stable payloads. +- coding 1 and delivery 3: read the body, not the thread. A guard could refuse `gh issue view --comments` for the implementer, but the handoff and decision reads make it messy. +- coding 2 and delivery 3: what counts as a small gap. +- delivery 4: reconcile first (needs the comment meaning). +- delivery 5: never rewrite a reporter's body. `issue-body.mjs` refuses this now; a raw `gh issue edit --body*` could be refused once that script is the only path (S). +- delivery 6: outcomes, not implementation. +- delivery 12: short, plain, conclusion first. +- documentation 1, 6, 8: which audience; links run one way (the website side is mechanical already); point at a value's source. A number-word lint for "seven commands" is too noisy. +- REVIEW: risk routing semantics; deduplication by defect class; verification evidence; impact classes; the safety-class probe before a downgrade. +- vendor 1 to 10: what counts as third-party; deviations with reasons; "never patch a binary" (the SHA check covers it); planned consumers. +- spec-session: interview rules, ADR need, Explain. +- deliver: "decide, then tell", park reasons, status lines. +- briefs: four Red questions, tracer bullet order, "never build a layer ahead", "never refactor between cycles", fix-every-instance search. +- implementer: "read code narrowly". A guard on `cat` of large tracked files is possible (M), but the value is low. + +## Appendix: already mechanical (the baseline) + +**`tool-guard.mjs` (PreToolUse, sub-agents):** +- the "Checks agents never run" list (scripts, tools, node scripts, `npx` and `pnpm exec`); +- git hook bypass (`--no-verify`, `LEFTHOOK=0`, `core.hooksPath`, `LEFTHOOK_EXCLUDE` other than `fallow`); +- `isolation` on stage agents; +- a test runner with `timeout` under 300000; +- background jobs (`run_in_background`, a trailing `&`); +- loops that sleep; +- bare `cat > file`; +- `test:e2e` with no files, and `test:console` without Chromium and spec files. + +**`report-guard.mjs` (SubagentStop for implementer, reviewer, hardware-verifier):** the report block's first key; the reviewer's `Assessment:` line; an audit report's `audit:` line with `probed p/t` and p equal to t. + +**`budget-guard.mjs`:** stall on the same Bash command and result; token and turn limits per agent and per session; hard kill. + +**`settings.json`:** +- deny: `--no-verify` and `-n`, `push --force` and `-f`, `reset --hard`, `branch -D`, `gh pr merge`, merge APIs, `gh release`, `gh workflow run`, rulesets and protection, `gh repo delete`, npm and pnpm publish; +- ask: `test:e2e:slow`. + +**lefthook pre-commit:** +- oxfmt; +- oxlint (the gateway's import allow-lists); +- depcruise: `past-index` (architecture 14), `testing-only-in-tests`, `definition-is-light`, direction rules including `no-fs-or-process` for core, leasing, agent-device, host-load, stream and alerts, the cycle check, known lists that only shrink; +- `comment-lint.mjs` (the shape of architecture 17); +- typecheck. + +**lefthook pre-push:** `fallow audit`, `protocol-bump-check.mjs`, `typecheck:e2e:types`. + +**CI (`ci.yml`):** typecheck and the e2e typecheck, lint and `lint:comments`, format check, unit, timing project, three e2e shards, four console shards, Fallow, protocol bump. `website.yml` runs the build and `check-openapi.mjs`; `real-proxy.yml` runs the proxy lane. + +**`issue-state.yml`:** +- one state label; +- demote a task whose Technical spec is emptied; +- the needs-info reply and the stale close; +- close a feature on its last sub-issue, then its request; +- a closed PR releases its claim; +- a three-day stale claim is released; +- a `/` PR closes `#n` only; +- `pr-body-check.mjs`: footer; Review counts line; Rejected tags; no "spec needs"; Status and Assumptions sections on delivery branches; a ledger comment exists; `Assessment: incomplete` fails; budget (a warning only). + +**`merge-pr.sh`:** open and ready; no `needs-hardware`; `pr-body-check`; CI green with a flaky rerun; mergeable; the stack merged all or nothing. + +**Scripts that remove a by-hand step:** +- `issue-body.mjs` (untracked): one-match replace, keeps the footer, refuses `request:` and `bug:` bodies; +- `review-ledger.mjs` (untracked): posts the ledger, refuses unknown SHAs and round mismatches, runs `pr-body-check`; +- `review-metrics.mjs --findings`: the findings schema (unique id; role, class, path; outcome and round enums; a duplicate names its canonical); +- `review-inputs.sh`: blind inputs from the pinned base, `tests-after-red.patch`, `assumptions.md`; +- `stale-refs.sh`, `diff-kind.mjs`, `ci-triage.mjs` and `ci-failures.mjs`, `stack-sync.sh` (merge forward only), `slow-e2e.sh` (lock, detached, machine checks), `worktree.sh`, `pr-shas.sh`. + +**Repo tests:** +- `docs-manuals.test.ts`: a manual per module, CLAUDE.md that only imports it, at most 150 lines, backticked paths and links exist, ARCHITECTURE links every manual; +- `docs-audience.test.ts`: website pages and README name no internal path or ADR; markdown under `docs/` sits only in `internal/`; +- `source-hygiene.test.ts`: a vendor directory has NOTICE.md and LICENSES texts, no bare LICENSE, SHA256SUMS digests match, no committed binaries or NUL bytes; +- `build-capture.test.ts`: NOTICE and LICENSES copied to `dist` (vendor 8); +- `expected-failures.test.ts`: no test written to pass by failing; +- `adr-index.test.ts`: a row per ADR file, ascending; +- `lint-rules.test.ts`: the depcruise and oxlint configs themselves; +- `test-lanes.test.ts`: timing files are out of unit and mutation, slow and physical tags; +- `labels.test.ts`; +- `bus/index.test.ts`: EVENTS.md names equal `EVENT_NAMES`; +- `host-load/boundary.test.ts`; +- `http/openapi.test.ts`. + +**Weekly routines:** `harden-tests` (mutation), `audit-tests`, `garden-comments` (`comment-lint --all`), `calibrate-reviews`, `recurring-findings`, which already turns recurring review classes into proposed checks. diff --git a/docs/internal/agent-rules/delivery.md b/docs/internal/agent-rules/delivery.md index 385c64f1d..d2c880ee5 100644 --- a/docs/internal/agent-rules/delivery.md +++ b/docs/internal/agent-rules/delivery.md @@ -15,8 +15,7 @@ it before anything else happens to it. | Label | Set by | Meaning | | ------------------ | ----------------------------------- | --------------------------------------------------------- | | `request:new` | feature request form | Inbox. Never picked up, body never edited. | -| `bug:new` | bug report form | Reported. Nobody has looked yet. | -| `bug:triage` | maintainer | An agent may reproduce it and write the triage report. | +| `bug:new` | bug report form | An agent may triage it (rule 8), and nothing more. | | `bug:needs-info` | triage agent | Could not reproduce. Waiting on the reporter. | | `bug:ready` | maintainer, after the triage report | An agent may fix it. | | `bug:blocked` | agent, on a blocked handoff | Waiting on the maintainer to clear a blocker. | @@ -36,15 +35,16 @@ Transitions per kind: - **request**: `new` until closed. Closed as not planned, or as completed with a comment naming the feature it became. -- **bug**: `new` → `triage` → `ready`, with `needs-info` as a side-trip that - returns to `triage` when the reporter answers. +- **bug**: `new` → `ready` when the maintainer accepts the triage report, + with `needs-info` as a side-trip that returns to `new` when the reporter + answers. - **feature**: `spec` → `planned` when the spec session has set its tasks - ready, then closed. + ready, then closed when its last sub-issue closes (rule 9). - **task**: `draft` → `ready` when its spec check passes, and back to `draft` if the technical spec is emptied or the maintainer moves it. -- **any kind**: `triage` or `ready` → `blocked` when an agent stops on a - blocker it names in its handoff. The maintainer clears it by re-adding the - label it came from. +- **any kind**: `ready` → `blocked` (for a bug under triage, `new` → + `blocked`) when an agent stops on a blocker it names in its handoff. The + maintainer clears it by re-adding the label it came from. Everything after `ready` is read from GitHub itself, not from a label: in progress means an assignee is set, in review means a linked pull request @@ -52,11 +52,13 @@ is open, done means closed as completed. ## Rules -1. **An agent acts only on `bug:triage`, `bug:ready` and `task:ready`.** - Nothing else is work. Agents never label their way into - work: only a maintainer, automation a maintainer configured, or the spec - session that wrote a task and saw its spec check pass (rule 7) moves an - issue to a state an agent may act on. The maintainer's say on a task is +1. **An agent acts only on `bug:new` (triage only), `bug:ready` and + `task:ready`.** Nothing else is work. The work on `bug:new` is the + triage report (rule 8); only the maintainer moves a bug to `bug:ready`, + after reading it. Agents never label their way into work: only a + maintainer, automation a maintainer configured, or the spec session + that wrote a task and saw its spec check pass (rule 7) moves an issue + to a state an agent may act on. The maintainer's say on a task is the spec session, which they sit through; they can move a task back to `task:draft` at any time. A `task:ready` task runs once every issue under its Depends on is closed, or is a task of the same feature whose @@ -139,9 +141,9 @@ is open, done means closed as completed. changed. The maintainer approves the feature, its ADRs and its tasks once, at the end. -8. **Triage produces a report, not a fix.** An agent working `bug:triage` - reproduces the bug as a failing test whose title states the claim the - test proves, finds the root cause, and posts one comment with exactly +8. **Triage produces a report, not a fix.** An agent triaging a `bug:new` + issue reproduces the bug as a failing test whose title states the claim + the test proves, finds the root cause, and posts one comment with exactly these sections: *Reproduction*, *Root cause* (with file and line), *Simplest fix*, *Alternatives rejected* (at most three, one line each), *Risk* (at most three bullets), and *Side findings* when there are any. The root cause says first whether this is a defect @@ -152,8 +154,9 @@ is open, done means closed as completed. its own `bug:new` issue and listed under Side findings, not described in the report. It pushes the failing test to a `bug/-repro` branch and opens no pull request. It then unassigns - itself. If it cannot reproduce, it moves the issue to `bug:needs-info`, - says exactly what is missing, and unassigns itself. + itself and leaves the label at `bug:new`. If it cannot reproduce, it + moves the issue to `bug:needs-info`, says exactly what is missing, and + unassigns itself. 9. **Done is defined per kind.** A task or bug is done when the pull request that closes it is merged; for a bug the failing test from triage is the @@ -163,7 +166,8 @@ is open, done means closed as completed. delivering agent merges its own PR, but only through the gate in rule 15. Verification is part of delivery, not a step after it ([coding.md](coding.md) - rule 5). The maintainer closes the feature. + rule 5). The issue-state workflow closes the feature as completed when + its last sub-issue closes; the maintainer may reopen it. 10. **ADR status follows the feature.** A decision made during a spec session that constrains more than one task, or would be expensive to reverse, @@ -256,17 +260,19 @@ gh issue edit --add-assignee @me The PR body contains `Closes #`. For a bug, branch from `bug/-repro` when it exists so the triage test is carried forward. -**Finding triage work.** +**Finding triage work.** A `bug:new` issue with no assignee and no triage +report on its thread: ```bash -gh issue list --label bug:triage --search 'no:assignee' +gh issue list --label bug:new --search 'no:assignee' --json number,title,comments \ + --jq '[.[] | select(all(.comments[]; .body | test("^## Root cause"; "m") | not))]' ``` **Moving a label.** Add the new one and remove the old one in the same command, so the one-label invariant never breaks in between: ```bash -gh issue edit --add-label bug:needs-info --remove-label bug:triage +gh issue edit --add-label bug:needs-info --remove-label bug:new ``` The repo's own skills encode these procedures; use them rather than @@ -287,13 +293,13 @@ rules mechanically, so nobody has to remember them: - A `task:ready` issue with no assignee goes back to `task:draft` when its Technical spec section is emptied. A claimed task is left alone. - A comment by the reporter on a `bug:needs-info` issue moves it back to - `bug:triage`, reopening the issue if it had been closed. Two weeks of + `bug:new`, reopening the issue if it had been closed. Two weeks of silence closes it as not planned. - When the last sub-issue of a `feature:planned` issue closes, the workflow - comments that the feature can be closed once the completion conditions - hold. -- A feature closed as completed closes the `request:new` issue it names on - its `Request:` line, with a comment pointing at the feature. + closes the feature as completed, with a comment. +- A feature closed as completed, by that job or by a person, closes the + `request:new` issue it names on its `Request:` line, with a comment + pointing at the feature. - A pull request closed without merging releases the claim on every issue its body closes, with a comment. The label is left as it was. - A claim on a `*:ready` issue with no comment, label change, or commit on @@ -301,6 +307,7 @@ rules mechanically, so nobody has to remember them: - A pull request from a `/` branch fails its check unless its body closes `#` and closes nothing else. -The judgment calls stay manual by design: `bug:new` → `bug:triage`, -and `bug:triage` → `bug:ready`. A task needs none: the spec session the -maintainer sits through sets `task:ready` once its check passes (rule 1). +The judgment call stays manual by design: `bug:new` → `bug:ready`, once +the maintainer has read the triage report. A task needs none: the spec +session the maintainer sits through sets `task:ready` once its check +passes (rule 1). diff --git a/docs/internal/agent-rules/toolchain.md b/docs/internal/agent-rules/toolchain.md index 3d1398f7f..d3fd8106e 100644 --- a/docs/internal/agent-rules/toolchain.md +++ b/docs/internal/agent-rules/toolchain.md @@ -11,11 +11,16 @@ command of their own: they take every command from here. | One unit test file | `pnpm exec vitest run --project unit ` | | One timing file (`*.timing.test.ts`) | `pnpm exec vitest run --project timing ` | | Fast e2e files (builds first) | `pnpm test:e2e ` | -| Console lane, Chromium only (1) | `pnpm test:console --project=chromium` | +| Console specs the diff touched, Chromium only (1) | `pnpm test:console --project=chromium ` | | Website build and its HTTP API check (2) | `pnpm --dir website install && pnpm --dir website build && node website/scripts/check-openapi.mjs` | (1) Only when the diff touches `ui/`, `e2e/console/`, `src/http/` or -`src/contract/`. +`src/contract/`, and only the spec files the diff changed or whose page +it changes: the whole lane is CI's, on every push. A stage agent never +runs it without a spec file (the tool guard refuses), never in the +background, and sets `timeout: 600000` on the Bash call: the default +120 s moves the run to the background, where it holds the agent's +report until it ends. (2) Only when the diff touches `website/`, `src/http/`, `src/contract/`, `src/modules/` or a module's `definition.ts`. @@ -108,6 +113,14 @@ Swift build is skipped and iOS streams report unavailable. Neither build checks a CoreDevice version: the device program is the one place that enforces that pin. +The build steps run through Turborepo (`turbo.json`): `pnpm build`, +`pnpm build:e2e` and the test scripts that build first run the +`build:*:run` scripts and skip each one whose inputs (listed per step in +`turbo.json`) did not change, restoring its `dist/` files from `.turbo/cache` +in this worktree. Any `package.json` change reruns every step. A capture +build that skipped Swift (no Xcode selected) is cached too: once Xcode is +selected, run `pnpm build:capture --force`. + Neither runs in the unit, e2e or console lanes: both read a booted device's screen, which means real devices and the slow lane. The lanes cover what this repo writes -- the stream module, the drivers' capture code against fakes, the diff --git a/package.json b/package.json index 1e46881d5..fecf75b22 100644 --- a/package.json +++ b/package.json @@ -38,25 +38,29 @@ } }, "scripts": { - "build": "pnpm run build:daemon && pnpm run build:ui && pnpm run build:capture", - "build:daemon": "tsc -p tsconfig.build.json", - "build:ui": "vite build ui", - "build:capture": "node scripts/build-capture.mjs", - "build:e2e": "tsc -p e2e/fake-driver/tsconfig.json", + "build": "TURBO_TELEMETRY_DISABLED=1 turbo run build:daemon:run build:ui:run build:capture:run", + "build:daemon": "TURBO_TELEMETRY_DISABLED=1 turbo run build:daemon:run", + "build:daemon:run": "tsc -p tsconfig.build.json", + "build:ui": "TURBO_TELEMETRY_DISABLED=1 turbo run build:ui:run", + "build:ui:run": "vite build ui", + "build:capture": "TURBO_TELEMETRY_DISABLED=1 turbo run build:capture:run", + "build:capture:run": "node scripts/build-capture.mjs", + "build:e2e": "TURBO_TELEMETRY_DISABLED=1 turbo run build:e2e:run", + "build:e2e:run": "tsc -p e2e/fake-driver/tsconfig.json", "dev:ui": "vite ui", "test": "pnpm run test:unit && pnpm run test:timing && pnpm run test:e2e", "test:timing": "vitest run --project timing", "test:unit": "vitest run --project unit", "test:changed": "vitest run --project unit --changed origin/main", - "test:e2e": "pnpm run build && pnpm run build:e2e && vitest run --project e2e --tags-filter='!slow'", - "test:e2e:slow": "pnpm run build && pnpm run build:e2e && vitest run --project e2e --no-file-parallelism --reporter=default --reporter=./e2e/progress-reporter.ts --tags-filter='!physical && !physical-manual'", - "test:e2e:physical": "pnpm run build && pnpm run build:e2e && vitest run --project e2e --no-file-parallelism --reporter=default --reporter=./e2e/progress-reporter.ts --tags-filter='physical'", - "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", - "test:real-proxy": "pnpm run build && pnpm run build:e2e && playwright test --config playwright.real-proxy.config.ts", + "test:e2e": "TURBO_TELEMETRY_DISABLED=1 turbo run build:daemon:run build:ui:run build:capture:run build:e2e:run && vitest run --project e2e --tags-filter='!slow'", + "test:e2e:slow": "TURBO_TELEMETRY_DISABLED=1 turbo run build:daemon:run build:ui:run build:capture:run build:e2e:run && vitest run --project e2e --no-file-parallelism --reporter=default --reporter=./e2e/progress-reporter.ts --tags-filter='!physical && !physical-manual'", + "test:e2e:physical": "TURBO_TELEMETRY_DISABLED=1 turbo run build:daemon:run build:ui:run build:capture:run build:e2e:run && vitest run --project e2e --no-file-parallelism --reporter=default --reporter=./e2e/progress-reporter.ts --tags-filter='physical'", + "test:e2e:physical-manual": "TURBO_TELEMETRY_DISABLED=1 turbo run build:daemon:run build:ui:run build:capture:run build:e2e:run && vitest run --project e2e --no-file-parallelism --reporter=default --reporter=./e2e/progress-reporter.ts --tags-filter='physical-manual'", + "test:console": "TURBO_TELEMETRY_DISABLED=1 turbo run build:daemon:run build:ui:run build:capture:run build:e2e:run && playwright test", + "test:real-proxy": "TURBO_TELEMETRY_DISABLED=1 turbo run build:daemon:run build:ui:run build:capture:run build:e2e:run && playwright test --config playwright.real-proxy.config.ts", "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: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", + "typecheck:e2e": "TURBO_TELEMETRY_DISABLED=1 turbo run build:ui:run build:capture:run && pnpm run typecheck:e2e:types", + "typecheck:e2e:types": "TURBO_TELEMETRY_DISABLED=1 turbo run build:daemon:run && 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", @@ -65,7 +69,7 @@ "prepare": "lefthook install", "check": "pnpm run typecheck && pnpm run typecheck:e2e && pnpm run lint && pnpm run lint:comments && pnpm run format:check && pnpm run test", "mutate": "node scripts/mutate.mjs", - "openapi": "pnpm run build:daemon && node scripts/openapi.mjs" + "openapi": "TURBO_TELEMETRY_DISABLED=1 turbo run build:daemon:run && node scripts/openapi.mjs" }, "dependencies": { "@agent-device/proxy": "0.21.28", @@ -103,6 +107,7 @@ "react-dom": "^19.3.0", "react-is": "^19.3.0", "recharts": "^3.10.1", + "turbo": "^2.11.10", "typescript": "^7.0.2", "verdaccio": "^6.10.6", "vite": "^8.3.2", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index cf853632c..2feacdada 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -313,6 +313,9 @@ importers: recharts: specifier: ^3.10.1 version: 3.10.1(@types/react@19.3.0)(react-dom@19.3.0(react@19.3.0))(react-is@19.3.0)(react@19.3.0)(redux@5.0.1) + turbo: + specifier: ^2.11.10 + version: 2.11.10 typescript: specifier: ^7.0.2 version: 7.0.2 @@ -1299,6 +1302,36 @@ packages: '@tanstack/virtual-core@3.17.11': resolution: {integrity: sha512-+ILjvtHup6Y2hzQ6YzwMgX1Q+oQpxEGOXCEsCNaPoIP0VxMbizIBTmYTDtkerkIQS8/CbP1BRuyt8V/8BCsy1g==} + '@turbo/darwin-64@2.11.10': + resolution: {integrity: sha512-By4idRap5+nLnMuRzLB4qkOPRlvNdyY1ne4VJYDyUoUB11H94k7uif9mbe3JT8JSNXs1D68uMcBj6hJ/jMQRvA==} + cpu: [x64] + os: [darwin] + + '@turbo/darwin-arm64@2.11.10': + resolution: {integrity: sha512-pyEYlVcuexOPa92p0Wlwl9YT1Is5D60lxN3ItWm3g30YLvFaUq86TF4BBWg5ZAc0uMTnrOzCvuCsOBTdzUWnRQ==} + cpu: [arm64] + os: [darwin] + + '@turbo/linux-64@2.11.10': + resolution: {integrity: sha512-1tYeR/IDZ5HEdsb0jNq2L2xam5nMQ+g80tlV2BZmv4OFw+pMcUM0mMmUcfc5wXPPNHVNPuDMjh9Y8G8HgXPLZA==} + cpu: [x64] + os: [android, linux] + + '@turbo/linux-arm64@2.11.10': + resolution: {integrity: sha512-HuekQxvVGhUd46g1Im/DKdOtglHr4/gguZAvzEpPUgtxNkztSa8lWhQEGQ+EmPQ0n7w19qjThFU+OfEEd0uvlw==} + cpu: [arm64] + os: [android, linux] + + '@turbo/windows-64@2.11.10': + resolution: {integrity: sha512-UJOP3BkbreY3/JlMgJR6ZiimxYIBn+c+B4tnBIxD0E9c3Estmk+XoVNVBNBottZLNRXe3/ScgOU4+/k8tIWysg==} + cpu: [x64] + os: [win32] + + '@turbo/windows-arm64@2.11.10': + resolution: {integrity: sha512-xOkEjF3ws/b/YNlc+bvJHMYBn+vL+QbEISZFVi0qS+ImmhA1X3/gmNmBrjjIG3dg6aeN+n6gQitzSkXQ286qJQ==} + cpu: [arm64] + os: [win32] + '@types/chai@5.2.3': resolution: {integrity: sha512-Mw558oeA9fFbv65/y4mHtXDs9bPnFMZAL/jxdPFUpOHHIXX91mcgEHbS5Lahr+pwZFR8A7GQleRWeI6cGFC2UA==} @@ -3460,6 +3493,10 @@ packages: resolution: {integrity: sha512-1h/Lnq9yajKY2PEbBadPXj3VxsDDu844OnaAo52UVmIzIvwwtBPIuNvkjuzBlTWpfJyUbG3ez0KSBibQkj4ojg==} engines: {node: '>=0.6.11 <=0.7.0 || >=0.7.3'} + turbo@2.11.10: + resolution: {integrity: sha512-3GJYwig8ygEI89oaOZdduJX8/aw6O9eRPYHoIt6T3FZl9hkKjiyIspufJoVgqXihPoJu8M5CSV0DlZvzfohBCw==} + hasBin: true + tweetnacl@0.14.5: resolution: {integrity: sha512-KXXFFdAbFXY4geFIwoyNK+f5Z1b7swfXABfL7HXCmoIWMKU3dmS26672A4EeQtDzLKy7SXmfBu51JolvEKwtGA==} @@ -4579,6 +4616,24 @@ snapshots: '@tanstack/virtual-core@3.17.11': {} + '@turbo/darwin-64@2.11.10': + optional: true + + '@turbo/darwin-arm64@2.11.10': + optional: true + + '@turbo/linux-64@2.11.10': + optional: true + + '@turbo/linux-arm64@2.11.10': + optional: true + + '@turbo/windows-64@2.11.10': + optional: true + + '@turbo/windows-arm64@2.11.10': + optional: true + '@types/chai@5.2.3': dependencies: '@types/deep-eql': 4.0.2 @@ -6742,6 +6797,15 @@ snapshots: tunnel@0.0.6: {} + turbo@2.11.10: + optionalDependencies: + '@turbo/darwin-64': 2.11.10 + '@turbo/darwin-arm64': 2.11.10 + '@turbo/linux-64': 2.11.10 + '@turbo/linux-arm64': 2.11.10 + '@turbo/windows-64': 2.11.10 + '@turbo/windows-arm64': 2.11.10 + tweetnacl@0.14.5: {} typanion@3.14.0: {} diff --git a/src/tool-guard.test.ts b/src/tool-guard.test.ts index f00b57fe6..c62147b70 100644 --- a/src/tool-guard.test.ts +++ b/src/tool-guard.test.ts @@ -17,14 +17,23 @@ function call(input: object): { status: number | null; stderr: string } { return { status: result.status, stderr: result.stderr }; } -function bash(command: string, agentId: string | null = "a1", agentType = "reviewer") { +function bash( + command: string, + agentId: string | null = "a1", + agentType = "reviewer", + input: Record = {}, +) { return call({ tool_name: "Bash", - tool_input: { command }, + tool_input: { command, ...input }, ...(agentId && { agent_id: agentId, agent_type: agentType }), }); } +// why: a test run is allowed only with the Bash tool's timeout set (the default 120 s moves it to +// the background), so the "lets run" cases pass it the way an agent must. +const TIMEOUT = { timeout: 600_000 }; + describe("tool-guard.mjs", () => { it.each([ "pnpm check", @@ -60,7 +69,7 @@ describe("tool-guard.mjs", () => { "pnpm test:changed", "pnpm exec vitest run --project unit src/a.test.ts", "pnpm test:e2e e2e/http-api.test.ts", - "pnpm test:console --project=chromium", + "pnpm test:console --project=chromium e2e/console/shell.spec.ts", 'git commit -qm "run pnpm check in CI"', 'git commit -m "chore(fallow): remove four stale suppressions"', 'gh pr create --title "chore(fallow): x" --body-file b.md', @@ -70,9 +79,41 @@ describe("tool-guard.mjs", () => { "python3 - <<'E'\nprint(\"pnpm check\")\nE\ngit status --short", "cat > x.sh < { - expect(bash(command)).toEqual({ status: 0, stderr: "" }); + expect(bash(command, "a1", "reviewer", TIMEOUT)).toEqual({ status: 0, stderr: "" }); + }); + + it.each([ + ["pnpm exec vitest run --project unit src/a.test.ts", {}, "timeout: 600000"], + ["pnpm test:changed", { timeout: 120_000 }, "timeout: 600000"], + ["pnpm test:console --project=chromium", TIMEOUT, "Checks agents never run"], + ["ls", { run_in_background: true }, "background"], + ["(git push origin x > log 2>&1; echo done) &", {}, "background"], + ["until ! pgrep -f lefthook >/dev/null; do sleep 5; done; tail x", {}, "loop that sleeps"], + ["for _ in $(seq 18); do [ -f out ] && break; sleep 30; done", {}, "loop that sleeps"], + ["cat > /tmp/x 2>/dev/null; mkdir -p .agents/scratch", {}, "no input"], + ["cat > ../pr-911.md", {}, "no input"], + ])("refuses a sub-agent's wait or background job: %s", (command, input, reason) => { + const result = bash(command, "a1", "implementer", input); + + expect(result.status).toBe(2); + expect(result.stderr).toContain(reason); }); + it.each([ + ["git add -A && git commit -m x", {}], + ["ls vitest* ui/vitest.config.ts", {}], + ["cat vitest.config.ts | head", {}], + ['for _ in $(seq 18); do [ -f "$log.exit" ] && break; sleep 30; done; cat "$log.exit"', {}], + ["scripts/slow-e2e.sh e2e/slow-ios.test.ts", {}], + ["cat > ../pr-911.md <<'EOF'\nCloses #1\nEOF\ngit status", {}], + ["python3 - <<'EOF'\nfor i in range(3):\n time.sleep(1)\nEOF", {}], + ])( + "lets a sub-agent run a foreground command, the slow lane wait included: %s", + (command, input) => { + expect(bash(command, "a1", "implementer", input)).toEqual({ status: 0, stderr: "" }); + }, + ); + it.each([ "git push --no-verify -u origin task/1", "git commit -q --no-verify -m x", diff --git a/turbo.json b/turbo.json new file mode 100644 index 000000000..42198b88c --- /dev/null +++ b/turbo.json @@ -0,0 +1,54 @@ +{ + "$schema": "./node_modules/turbo/schema.json", + "cacheDir": ".turbo/cache", + "cacheMaxSize": "2GB", + "noUpdateNotifier": true, + "tasks": { + "build:daemon:run": { + "inputs": [ + "src/**/*.ts", + "!src/**/*.test.ts", + "!src/test-support/**", + "tsconfig.json", + "tsconfig.build.json", + "package.json", + "pnpm-lock.yaml" + ], + "outputs": ["dist/**", "!dist/ui/**", "!dist/vendor/**", "!dist/bin/**", "!dist/e2e/**"], + "outputLogs": "new-only" + }, + "build:ui:run": { + "inputs": [ + "ui/**", + "!ui/**/*.test.ts", + "!ui/**/*.test.tsx", + "!ui/**/*.md", + "src/contract/**/*.ts", + "!src/contract/**/*.test.ts", + "tsconfig.json", + "package.json", + "pnpm-lock.yaml" + ], + "outputs": ["dist/ui/**"], + "outputLogs": "new-only" + }, + "build:capture:run": { + "inputs": ["vendor/**", "!vendor/screen-capture/.build/**", "scripts/build-capture.mjs"], + "outputs": ["dist/vendor/**", "dist/bin/**"], + "env": ["DEVELOPER_DIR"], + "outputLogs": "new-only" + }, + "build:e2e:run": { + "dependsOn": ["build:daemon:run"], + "inputs": [ + "e2e/fake-driver/*.ts", + "e2e/fake-driver/tsconfig.json", + "tsconfig.json", + "package.json", + "pnpm-lock.yaml" + ], + "outputs": ["dist/e2e/**"], + "outputLogs": "new-only" + } + } +}