diff --git a/.claude/rules/toolchain.md b/.claude/rules/toolchain.md index 33f3f91ae..b14809bdb 100644 --- a/.claude/rules/toolchain.md +++ b/.claude/rules/toolchain.md @@ -353,6 +353,55 @@ need?" by probing the read endpoints and reporting each 403's it in a fresh environment before concluding a task is broken — an under-scoped token otherwise surfaces as an unrelated 403 in whichever task runs first. +## Invocation latency: the number, the series, and the regression gate + +`perf` measures batten's own invocation cost (hyperfine over a release build: +`noop`, `check`, `hook`) and `perf-assert` holds it to the ceiling README +publishes — an ABSOLUTE budget, 100ms, clig's floor. `perf-pair` asks the other +question: it builds this branch's binary AND its merge base's, measures them back +to back on one machine, and `perf-compare` decides the RATIO. `perf-gate` +composes those two and is the one name `verify` calls. Do not confuse this with +`hook-latency-drift`, which measures the hk gate's own pre-commit tier (CLOUD-509) +rather than the binary's startup. + +**Wall clock, paired — instruction counts are not available.** CLOUD-172 +specified callgrind with wall clock as a fallback. `mise registry valgrind` +reports "tool not found in registry", so it cannot be pinned, and +`no-source-built-tool` forbids compiling one; the fallback is the only branch. +What rescues wall clock is not a better clock but a better experiment — machine +noise is common-mode across a pair measured seconds apart, so it divides out. +Hence a ratio, never an absolute, and `perf-record` stamps `metric=wall-clock` +into every series entry so a later instruction-count series can never be diffed +against this one and read as a step change. The threshold is derived, not chosen: +a null comparison (identical binary as both arms, n=30) spread 0.966–1.102, and +`perf-compare`'s 1.30 clears that measured maximum. Re-measure with `mise run +perf-pair --null`; a bats case fails if the constant is tightened below the +recorded floor. + +**In `verify` AND as a CI job, and the second is not redundant.** `verify` is +where it catches a regression earliest: `ready-guard` refuses `gh pr ready` +without a verify receipt for this exact HEAD, so the branch stops before it can +spend a matrix. But `verify` is what an author runs, not a check — `[tasks.ci]` +is `depends = ["hooks", "deny"]`, so for its whole life `perf-gate` ran on no +runner at all, and CLOUD-172's "a PR that regresses fails a check that names the +regressed measurement" was unmet. The `perf` job in `ci.yml` is that check, in +`final`'s `needs:` and in `CI_REQUIRED_CHECKS`. Both placements cost the common +change nothing: `perf-gate` exits clean without building when the diff against +the merge base touches no crate source, manifest or lockfile, because a binary +that cannot have changed cannot have got slower. + +**The series records on a clock, not on a push to `main`.** A per-commit series +implies a push-to-`main` workflow, which AGENTS.md forbids — and the reason holds +rather than merely applying: `main` advances only by fast-forward to a SHA CI +already judged, so a push trigger buys a runner per merge for a measurement a +clock can take. `perf.yml` records daily from `main` into `refs/notes/perf`. +The cost is resolution, affordable because the series is not the attribution +mechanism — `perf-gate` catches a regression at the commit that caused it. +`perf-record` refuses to run off the trunk, because a series mixing a branch's +numbers with the trunk's cannot be read at all. (That decision belongs in +AGENTS.md beside the rule it honours; AGENTS.md is at its budgeted line ceiling, +so it lives here with the rest of the workshop detail.) + ## The gate `mise-tasks/` scripts are real programs: `shfmt`, `shellcheck` and `test:bats` diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 93870c868..c2b0aab32 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -496,6 +496,87 @@ jobs: key: semver - run: mise run semver + # The latency regression gate on the surface that reaches a person (CLOUD-172). + # + # WHY THIS JOB EXISTS AT ALL, since `perf-gate` is already a step in `verify`. + # The issue's Done clause is "a PR that regresses past the threshold FAILS A + # CHECK that names the regressed measurement", and `verify` is not a check — + # it is what an author runs locally. `[tasks.ci]` is `depends = ["hooks", + # "deny"]`, so nothing in CI ran `perf-gate`: the gate existed and no runner + # ever invoked it, which is a gate that decides nothing about a pull request. + # A regression would have reached `main` past a green required check. + # + # It is affordable for the reason `perf-gate`'s own header argues: the gate + # exits clean without building anything when the diff against the merge base + # touches no crate source, manifest or lockfile. Most of this repository's + # traffic is task-layer, workflow and documentation changes, which pay a + # `git diff --name-only` here and nothing more. Only a change that can have + # moved the binary pays for two release builds. + # + # Deliberately NOT the recording half. `perf-record` writes the series and + # refuses to run off `main`; it runs from `perf.yml` on a schedule, because a + # push-to-`main` trigger is forbidden (AGENTS.md) and `main` only ever advances + # by fast-forward to a SHA this job already judged. + perf: + name: perf + if: ${{ github.event.pull_request.draft == false }} + runs-on: ubuntu-latest + # DERIVED, not guessed, and stated because a single sample is not a p95. + # `mise run perf-gate` forced down its full path — both release binaries built + # and both arms sampled, in a worktree with a crate-source change so the skip + # could not fire — measured 109s wall on a warm cache. The declared 200s adds + # the job's own preamble (checkout, mise install, cache restore) and headroom + # for a colder, busier runner; x3 on top of that is the multiplier every job + # here carries. Re-measure from real runs once this job has a history: an + # over-declared budget is a weaker backstop, which is the safe direction to be + # wrong in but still the wrong number. + timeout-minutes: 10 # budget: p95=200s x3 measured=2026-08-14 + # `actions: write` is here for one call and one only: cancelling THIS run + # when the landing lease does not authorise this branch (CLOUD-420). A job + # that could not cancel itself would have to fail instead, and a failed job + # makes the run conclude `failure` rather than `cancelled` — which reds + # `final` and makes `land` re-draft a healthy PR. + permissions: + contents: read + actions: write + steps: + # CLOUD-420: THE LANDING LEASE, ENFORCED WHERE THE MONEY IS SPENT. First + # step, before any checkout or toolchain install, for the reason the `ci` + # job states at length: a run this branch is not authorised to make should + # cost the rounding rather than a matrix. `|| exit 0` on both lines. + - name: Landing lease precondition + env: + GH_TOKEN: ${{ github.token }} + GH_REPO: ${{ github.repository }} + LEASE_HEAD_REF: ${{ github.head_ref }} + LEASE_HEAD_SHA: ${{ github.event.pull_request.head.sha }} + LEASE_RUN_ID: ${{ github.run_id }} + run: | + body=$(gh api -H "Accept: application/vnd.github.raw" \ + "repos/$GH_REPO/contents/mise-tasks/ci-lease-precondition?ref=main") || exit 0 + bash -c "$body" || exit 0 + + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + # `perf-pair` builds BOTH arms — the merge base and this head — so the + # baseline commit has to be in the clone. A shallow checkout has no + # merge base, and an unresolvable base is exit 2 by design rather than + # a pass, so this would fail loudly rather than measure nothing. + fetch-depth: 0 + - uses: jdx/mise-action@9dda3952d607125725deac9ec10a5f0e245d266b # v4.2.4+9dda395 retry fix, pre-release (CLOUD-404) + with: + # rust builds the two binaries under measurement, hyperfine samples + # them, jq reads the ratio out. See the ci job for why these lists are + # narrow. + install_args: rust aqua:sharkdp/hyperfine aqua:jqlang/jq + # A distinct key: this job builds `--release`, where every other cargo job + # here builds debug, so sharing an entry would thrash it — the same reason + # msrv and semver carry their own. + - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 + with: + key: perf + - run: mise run perf-gate + # Single fan-in status. Point branch protection at `final` instead of # enumerating every leg, so adding a job never requires a ruleset change. # @@ -537,7 +618,7 @@ jobs: # carries the reason it cannot change, so the next reader who notices that a # stopped run reds `final` does not re-derive the skip and ship it. if: ${{ always() && github.event.pull_request.draft == false }} - needs: [ci, cross, darwin-link, msrv, semver] + needs: [ci, cross, darwin-link, msrv, semver, perf] runs-on: ubuntu-latest # `checks: read` is the whole reason this job now needs a permission at all: # the analyzer's verdict is a check-run on the head commit. diff --git a/.github/workflows/land-divergence.yml b/.github/workflows/land-divergence.yml index ac5b50dec..50348eb3d 100644 --- a/.github/workflows/land-divergence.yml +++ b/.github/workflows/land-divergence.yml @@ -22,7 +22,7 @@ name: land-divergence on: schedule: - # Daily, 04:00 UTC — an hour before bench.yml (05:00), which is the only + # Daily, 04:00 UTC — an hour before perf.yml (05:00), which is the only # other daily job, and clear of the Monday 06:00-08:15 weekly ladder. - cron: "0 4 * * *" workflow_dispatch: @@ -59,7 +59,7 @@ jobs: # Measure to a FILE, then read it back — never `measure | assert`. A # pipeline hands the step's exit status to its last stage, so a measurement # that died would be judged as a clean empty window rather than reported - # (bench.yml:84-92, and `run-shape-guard` refuses the shape locally). + # (perf.yml:84-92, and `run-shape-guard` refuses the shape locally). # # GH_TOKEN through env, never interpolated into the run block: a `${{ }}` # expansion is textual substitution into the shell script, which zizmor diff --git a/.github/workflows/nonverdict-rate.yml b/.github/workflows/nonverdict-rate.yml index 3685b1b06..89884a872 100644 --- a/.github/workflows/nonverdict-rate.yml +++ b/.github/workflows/nonverdict-rate.yml @@ -63,7 +63,7 @@ jobs: - uses: jdx/mise-action@9dda3952d607125725deac9ec10a5f0e245d266b # v4.2.4+9dda395 retry fix, pre-release (CLOUD-404) # Measure to a FILE, then read it back — never `scan | assert`. A pipeline # hands the step's exit status to its last stage, so a scan that died would - # be judged as an empty window rather than reported (bench.yml:84-92). + # be judged as an empty window rather than reported (perf.yml:84-92). # # GH_TOKEN through env, never interpolated into the run block: a `${{ }}` # expansion is textual substitution into the shell script, which zizmor gates diff --git a/.github/workflows/bench.yml b/.github/workflows/perf.yml similarity index 65% rename from .github/workflows/bench.yml rename to .github/workflows/perf.yml index e7f002b89..d5312c599 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/perf.yml @@ -1,4 +1,4 @@ -name: bench +name: perf # The measurement half of CLOUD-207, on a clock rather than on the landing path. # @@ -14,7 +14,7 @@ name: bench # minutes are metered; `ci-local-parity` exists to keep the landing path to # things a local `mise run verify` already proved. # -# WHAT DEFENDS THE NUMBER PER-COMMIT, then, is `tests/bench-assert.bats`: the +# WHAT DEFENDS THE NUMBER PER-COMMIT, then, is `tests/perf-assert.bats`: the # gate is a pure function of records on stdin, so its decision runs in the hk # gate on every commit in milliseconds, with no build and no hyperfine. This # workflow supplies the one thing that suite cannot — a real measurement of the @@ -43,7 +43,7 @@ on: workflow_dispatch: concurrency: - group: bench + group: perf cancel-in-progress: false permissions: {} @@ -56,24 +56,38 @@ env: MISE_LOCKFILE: "true" jobs: - bench: - name: bench + perf: + name: perf runs-on: ubuntu-latest # `grandfathered` rather than a derived budget, and deliberately so: this job # has no run history, so there is no p95 to multiply and any number written # here would be the asserted-instead-of-measured claim this very ticket - # exists to delete. Locally a full `mise run bench` — cold release build, + # exists to delete. Locally a full `mise run perf` — cold release build, # three paths, 110 invocations each — measured 48.7s on four cores; the # runner adds checkout and install on top, and pays the full dependency # build whenever the cache misses. `mise run timeout-drift` is the mechanism # that replaces this with a measured budget once runs exist to measure. timeout-minutes: 20 # budget: grandfathered measured=2026-08-11 permissions: - contents: read + # `write`, and only here (CLOUD-172): the recording step pushes the series + # to `refs/notes/perf`. Every other scheduled workflow reads; this one + # writes exactly one ref that no branch protection covers and no build + # consumes — a note is not a commit, so nothing this pushes can change + # what `main` builds. + contents: write steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 with: - persist-credentials: false + # Credentials ARE persisted here, unlike every other scheduled job, + # because the recording step pushes. Scoped by the `contents: write` + # above and by the single explicit refspec that push names. + fetch-depth: 0 + # `git notes append` needs the existing note to append TO; without this + # every run would start a fresh note and the series would be a list of + # singletons. `|| true` because on the first run the ref does not exist, + # and a missing ref is the empty series rather than a failure. + - name: Fetch the existing series + run: git fetch origin "refs/notes/perf:refs/notes/perf" || true - uses: jdx/mise-action@9dda3952d607125725deac9ec10a5f0e245d266b # v4.2.4+9dda395 retry fix, pre-release (CLOUD-404) with: # rust builds the binary under measurement, hyperfine samples it, jq @@ -84,16 +98,34 @@ jobs: with: # A release profile, which no other job builds, so a shared cache entry # would thrash. - key: bench + key: perf # Redirected into a file and read back, never piped into the gate. A - # pipeline hands its exit status to the LAST stage, so `bench | - # bench-assert` would discard exactly the measurement failure that must + # pipeline hands its exit status to the LAST stage, so `perf | + # perf-assert` would discard exactly the measurement failure that must # stop this job — the false-green shape `mise run run-shape-guard` refuses # locally, and no less wrong in a workflow. - name: Measure - run: mise run bench >"$RUNNER_TEMP/bench.txt" + run: mise run perf >"$RUNNER_TEMP/perf.txt" - name: Assert the budget - run: mise run bench-assert <"$RUNNER_TEMP/bench.txt" + run: mise run perf-assert <"$RUNNER_TEMP/perf.txt" + # The series (CLOUD-172). AFTER the assertion, deliberately: a measurement + # that broke its budget is a defect to fix, not a datum to enshrine, and a + # series carrying known-bad readings is one nobody can read a trend from. + # + # `perf-record` refuses to run off the trunk by itself, so the schedule's + # main-only guarantee is belt and braces rather than the only guard. + - name: Record the series + run: | + git config user.name "github-actions[bot]" + git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + mise run perf-record <"$RUNNER_TEMP/perf.txt" + env: + BENCH_RUNNER: ubuntu-latest + # Its own step, so a push that fails is distinguishable from a measurement + # that failed. The refspec is explicit and singular: this job may write the + # series and nothing else. + - name: Push the series + run: git push origin "refs/notes/perf:refs/notes/perf" # Always, including on a budget violation: the records are the evidence # for the failure, and a run that fails without publishing what it measured # sends the reader to re-run it by hand. @@ -107,9 +139,9 @@ jobs: # backtick pair inside single quotes reads as legacy command # substitution to shellcheck (SC2016), which actionlint runs over # every `run:` block. - echo "Budgets are enforced by \`mise run bench-assert\` — see README, Performance." + echo "Budgets are enforced by \`mise run perf-assert\` — see README, Performance." echo echo '```' - cat "$RUNNER_TEMP/bench.txt" + cat "$RUNNER_TEMP/perf.txt" echo '```' } >>"$GITHUB_STEP_SUMMARY" diff --git a/.serena/memories/toolchain-and-hooks.md b/.serena/memories/toolchain-and-hooks.md index ecba0b06b..58ec6b95f 100644 --- a/.serena/memories/toolchain-and-hooks.md +++ b/.serena/memories/toolchain-and-hooks.md @@ -351,6 +351,32 @@ programs the engine will replace, and churning their contract now would rewrite suites that are about to be deleted. The hazard is recorded rather than fixed because the fix is the port itself. +## Declare the break when you write it, not when `semver` asks + +`mise run semver` is **verify-time**, and a break declared late is not free: the +`!` has to land on the commit that makes the break, which is routinely not HEAD +by the time verify runs. Fixing it then means unwinding to that commit +(`reset --soft`, un-stage the later work, `--amend`, re-commit) — interactive +rebase is unavailable here. Declaring it on whichever commit happens to be HEAD +**satisfies the gate**, because it reads the whole `base..head` range for any `!` +or `BREAKING CHANGE:` — and leaves a `test(...)!` or `docs(...)!` in permanent +history claiming an API break it did not make. + +Measured twice in one bundle (CLOUD-59), both by an author who knew: + +- `Provision` gained a field and its `url`/`sha256` became `Option` — + `constructible_struct_adds_field`, committed as a plain `feat(provision):`. +- `rules::run_static`/`run_all` gained a parameter — + `function_parameter_count_changed`, committed as a plain `feat(rules):` **whose + own body explained the threading**. Knowing a change is breaking and marking it + breaking are independent acts; the second one is the one that gets skipped. + +The cheap check is at authoring time, not at verify time. A public struct that +callers construct, a public fn's parameter list, an enum variant's position — +touch one and the subject needs `!` before the commit exists. Recorded rather +than mechanised: a commit-time semver check would need a baseline rustdoc build +per commit, which is the cost `verify` exists to amortise. + ## Never hand awk a regex through `-v` The value is escape-processed by the assignment before awk sees it as a pattern, diff --git a/README.md b/README.md index f5c36f0d5..7bb0e247f 100644 --- a/README.md +++ b/README.md @@ -156,7 +156,7 @@ per-clone setup and the task tour. Batten runs as a `PreToolUse` hook, so its cost is paid on **every** mediated tool call rather than once per commit. These are measured numbers, not targets — -`mise run bench` reproduces them, and `mise run bench-assert` fails when a +`mise run perf` reproduces them, and `mise run perf-assert` fails when a measured p95 leaves its budget. `wired` is the number an agent actually waits on: the entry point @@ -184,7 +184,7 @@ ceiling here could tell a large tree apart from a regression. > Measured 2026-08-12 on a 4-core x86_64 Linux container: release build, 10 > warmup runs discarded, 100 timed runs per path, p95 from the sorted run times. > Your machine will differ; the budget is what the gate holds, and the schedule -> in [`.github/workflows/bench.yml`](.github/workflows/bench.yml) is what keeps +> in [`.github/workflows/perf.yml`](.github/workflows/perf.yml) is what keeps > holding it. diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index 4069c96eb..e388edc21 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -686,7 +686,7 @@ fn run_state_record(overrides: &Overrides, mode: Mode, err: &mut dyn Write) -> R let commit = git::head_commit(Path::new("."))?; let config = resolve::resolve(Path::new("."), overrides)?; - let scan = rules::run_static(&config.rules, Path::new("."))?; + let scan = rules::run_static(&config.rules, &config.provisions, Path::new("."))?; let bound = store::commit(store::resolve(&repo)?)?; if let Some(note) = &bound.note { @@ -2221,7 +2221,7 @@ fn run_rules( err: &mut dyn Write, mode: Mode, overrides: &Overrides, - runner: fn(&[rules::Rule], &Path) -> Result, + runner: fn(&[rules::Rule], &[provision::Provision], &Path) -> Result, surface: Surface, json: bool, ) -> Result { @@ -2235,7 +2235,7 @@ fn run_rules( // and files from another. let root = anchor(); let config = resolve::resolve(&root, overrides)?; - let mut findings = runner(&config.rules, &root)?.findings; + let mut findings = runner(&config.rules, &config.provisions, &root)?.findings; // Declared budgets are gates, evaluated here rather than only under `policy // budget` (CLOUD-50). Reading files and summing them spawns nothing, so this diff --git a/crates/batten/src/rules.rs b/crates/batten/src/rules.rs index fe1321ad7..98520c471 100644 --- a/crates/batten/src/rules.rs +++ b/crates/batten/src/rules.rs @@ -145,6 +145,26 @@ pub enum RuleKind { /// gate, and its findings deny. This one's command consults a model, and its /// findings advise. The gate/judge line (CLOUD-93) is exactly this pair. Judge, + /// A **credential in the tree** (CLOUD-59): run the pinned secret scanner + /// over the matched paths and turn each match into a pointer. + /// + /// The kind exists because [`RuleKind::Forbid`] bans literals named in + /// advance, and a credential is the banned shape nobody can enumerate — so a + /// committed secret passes every other gate this engine has. Detection is + /// adopted prior art; what this kind owns is **containment**, in + /// [`crate::secrets`]. + /// + /// Distinct from [`RuleKind::Command`], which could run the same binary and + /// could not express this: a command rule's contract is the exit code alone, + /// with both child streams nulled, so it yields one batch verdict per glob — + /// no per-secret `path:line`, no per-secret identity, and nothing to key. + /// Reading the scanner's output is precisely what makes this kind different, + /// and precisely what makes it dangerous: the scanner prints the byte it + /// matched. Every span is opaque from the parse boundary onward + /// ([`crate::identity::SecretSpan`]), and [`Finding`] has no field one could + /// occupy, so pointer-only output is structural rather than a property of + /// the renderer. + Secrets, } impl RuleKind { @@ -160,6 +180,7 @@ impl RuleKind { RuleKind::Ratchet, RuleKind::Receipt, RuleKind::Judge, + RuleKind::Secrets, ]; /// The stable lowercase token used in config and machine output (§6). @@ -172,6 +193,7 @@ impl RuleKind { RuleKind::Ratchet => "ratchet", RuleKind::Receipt => "receipt", RuleKind::Judge => "judge", + RuleKind::Secrets => "secrets", } } @@ -203,11 +225,13 @@ impl RuleKind { // every spawning kind with `Tree` alone, which is what keeps `hook` // structurally unable to execute a configured command. RuleKind::Forbid | RuleKind::Shape | RuleKind::Ratchet | RuleKind::Receipt => false, - // Both run a program a `batten.toml` named, which is the whole - // predicate — that a judge's program consults a model and a - // command's decides a gate makes no difference to the surface - // question. `check` refuses both, naming `batten enforce`. - RuleKind::Command | RuleKind::Judge => true, + // All three run a program a `batten.toml` named, which is the + // whole predicate — that a judge's consults a model, a command's + // decides a gate, and a secrets rule's scans for credentials makes + // no difference to the surface question. `check` refuses all three, + // naming `batten enforce`, with no code per kind: that refusal reads + // this predicate and nothing else. + RuleKind::Command | RuleKind::Judge | RuleKind::Secrets => true, } } @@ -219,7 +243,15 @@ impl RuleKind { // `pattern` or `regex`, and this list cannot express "one of" // (CLOUD-283). `validate` carries that check, which is also where // the both-columns case is refused. - RuleKind::Forbid => &["glob", "severity"], + // `Secrets` shares the pair, and for a related reason: for both, the + // `glob` is the gate before it is anything else — no match means no + // file is read and, for the scanner, no process is spawned at all. + // `severity` is required of every non-judge kind because it is what + // reaches the exit contract. The secrets kind takes nothing further: + // the issue's own constraint is that it permits no NEW column, so + // the scanner's identity is a constant in the adapter rather than a + // row every config repeats. + RuleKind::Forbid | RuleKind::Secrets => &["glob", "severity"], // `check`, never `fix`: enforcement is always the check side (§9), // so the gate half is what a row cannot load without. A row // carrying only a `fix` declares a mutation with nothing deciding @@ -320,6 +352,20 @@ impl RuleKind { "policy_url", "no_fix_reason", ], + // `identity_key` is permitted and `verbatim` is not: a secret span + // hashes, so an override has something to split, but the + // normalization is not the author's to choose — a secret IS literal + // content, so `secret_code_fingerprint` forces `Verbatim` and a + // column offering the other value would name a choice that does not + // exist. + RuleKind::Secrets => &[ + "glob", + "severity", + "identity_key", + "reason", + "policy_url", + "no_fix_reason", + ], } } @@ -332,9 +378,11 @@ impl RuleKind { #[must_use] pub const fn scopes(self) -> &'static [RuleScope] { match self { - RuleKind::Forbid | RuleKind::Command | RuleKind::Ratchet | RuleKind::Judge => { - &[RuleScope::Tree] - } + RuleKind::Forbid + | RuleKind::Command + | RuleKind::Ratchet + | RuleKind::Judge + | RuleKind::Secrets => &[RuleScope::Tree], RuleKind::Shape | RuleKind::Receipt => &[RuleScope::MediatedCall], } } @@ -912,7 +960,13 @@ impl Rule { .map(ToOwned::to_owned) .collect(), )), - RuleKind::Forbid | RuleKind::Ratchet => Some(Check::Reevaluate), + // Re-running the gate is what settles all three. For `Secrets` the + // important half is what this is NOT: an `Argv` here would put the + // scanner's invocation on the record, and the whole design is that + // nothing carrying a matched byte leaves the adapter. It is also the + // honest answer — unlike a judge's verdict, the engine can re-decide + // a secret finding by scanning again. + RuleKind::Forbid | RuleKind::Ratchet | RuleKind::Secrets => Some(Check::Reevaluate), // Neither reaches the store: both are adjudicated per mediated call // and produce a decision, not a finding. RuleKind::Shape | RuleKind::Receipt => None, @@ -1139,7 +1193,11 @@ pub const SPAWNING_VERB: &str = "batten enforce"; /// Returns a [`UsageError`] (→ exit `1`) when any configured rule's kind spawns /// processes, naming [`SPAWNING_VERB`] as the verb that runs it, and for a /// malformed rule. An I/O failure propagates as an internal error (→ exit `3`). -pub fn run_static(rules: &[Rule], root: &Path) -> anyhow::Result { +pub fn run_static( + rules: &[Rule], + _provisions: &[crate::provision::Provision], + root: &Path, +) -> anyhow::Result { // Refuse before any work: the read-only surface must not even begin a run // it cannot complete honestly. for rule in rules { @@ -1161,7 +1219,7 @@ pub fn run_static(rules: &[Rule], root: &Path) -> anyhow::Result { )); } } - run(rules, root) + run(rules, &[], root) } /// Run every configured rule, including process-spawning kinds. @@ -1176,7 +1234,11 @@ pub fn run_static(rules: &[Rule], root: &Path) -> anyhow::Result { /// a capability this engine does not have. Returns a [`UsageError`] (→ exit /// `1`): a config naming a capability the binary lacks is the config-or-usage /// class (§7), never a policy verdict about the repository. -pub fn run_all(rules: &[Rule], root: &Path) -> anyhow::Result { +pub fn run_all( + rules: &[Rule], + provisions: &[crate::provision::Provision], + root: &Path, +) -> anyhow::Result { // Refuse before any work, the shape `run_static` above already uses: the // alternative is running the check side, exiting on its verdict, and having // silently ignored a repair the config declared. A key that parses and does @@ -1190,7 +1252,7 @@ pub fn run_all(rules: &[Rule], root: &Path) -> anyhow::Result { ))); } } - run(rules, root) + run(rules, provisions, root) } /// Run every rule in `rules` against the tree rooted at `root`, returning all @@ -1201,12 +1263,16 @@ pub fn run_all(rules: &[Rule], root: &Path) -> anyhow::Result { /// Returns a [`UsageError`] (→ exit `1`) for a malformed rule (e.g. an empty /// `glob`). An I/O failure while walking the tree propagates as an internal /// error (→ exit `3`). -fn run(rules: &[Rule], root: &Path) -> anyhow::Result { +fn run( + rules: &[Rule], + provisions: &[crate::provision::Provision], + root: &Path, +) -> anyhow::Result { let files = tree_files(root)?; let mut scan = Scan::default(); for rule in rules { - if let Some(why) = run_rule(rule, root, &files, &mut scan.findings)? { + if let Some(why) = run_rule(rule, provisions, root, &files, &mut scan.findings)? { scan.not_evaluated.insert(rule.id.clone(), why); } } @@ -1224,6 +1290,7 @@ fn run(rules: &[Rule], root: &Path) -> anyhow::Result { /// finding whose rule never looked instead of resolving it (CLOUD-81). fn run_rule( rule: &Rule, + provisions: &[crate::provision::Provision], root: &Path, files: &[String], findings: &mut Vec, @@ -1282,6 +1349,7 @@ fn run_rule( } } RuleKind::Command => command_rule(rule, root, &matched, findings)?, + RuleKind::Secrets => crate::secrets::scan(rule, provisions, root, &matched, findings)?, // Unreachable: shape and receipt rules are `mediated_call`-scoped // and a ratchet // returned above. Stated rather than caught by a wildcard so adding a @@ -1421,7 +1489,7 @@ fn command_rule( /// Split `matched` into groups whose joined byte length stays under /// [`MAX_FILES_BYTES`]. Order is preserved, so batching is deterministic and the /// resulting findings stay byte-stable (§6). -fn batches<'a>(matched: &[&'a String]) -> Vec> { +pub(crate) fn batches<'a>(matched: &[&'a String]) -> Vec> { let mut batches = Vec::new(); let mut current: Vec<&str> = Vec::new(); let mut bytes = 0usize; @@ -2271,11 +2339,11 @@ mod tests { /// found; [`Scan::not_evaluated`] has its own tests, so shadowing keeps the /// suite reading as it did before that half existed. fn run_static(rules: &[Rule], root: &Path) -> anyhow::Result> { - super::run_static(rules, root).map(|scan| scan.findings) + super::run_static(rules, &[], root).map(|scan| scan.findings) } fn run_all(rules: &[Rule], root: &Path) -> anyhow::Result> { - super::run_all(rules, root).map(|scan| scan.findings) + super::run_all(rules, &[], root).map(|scan| scan.findings) } fn forbid(id: &str, glob: &str, pattern: &str) -> Rule { @@ -2449,7 +2517,7 @@ mod tests { let dir = temp_dir("scan-skipped"); write(&dir, "src/a.rs", "fine\n"); - let clean = super::run_static(&[forbid("looked", "**/*.rs", "TODO")], &dir).unwrap(); + let clean = super::run_static(&[forbid("looked", "**/*.rs", "TODO")], &[], &dir).unwrap(); assert!(clean.findings.is_empty()); assert!( clean.not_evaluated.is_empty(), @@ -2457,7 +2525,7 @@ mod tests { ); let skipped = - super::run_static(&[forbid("never-looked", "**/*.md", "TODO")], &dir).unwrap(); + super::run_static(&[forbid("never-looked", "**/*.md", "TODO")], &[], &dir).unwrap(); assert!(skipped.findings.is_empty()); assert_eq!( skipped.not_evaluated.get("never-looked"), @@ -2472,6 +2540,7 @@ mod tests { severity: Some(RuleSeverity::Allow), ..forbid("switched-off", "**/*.rs", "TODO") }], + &[], &dir, ) .unwrap(); @@ -2877,12 +2946,13 @@ mod tests { | RuleKind::Shape | RuleKind::Ratchet | RuleKind::Receipt - | RuleKind::Judge => {} + | RuleKind::Judge + | RuleKind::Secrets => {} } } assert_eq!( RuleKind::ALL.len(), - 6, + 7, "a new RuleKind must be added to RuleKind::ALL" ); } diff --git a/crates/batten/src/secrets.rs b/crates/batten/src/secrets.rs index 00afd00b3..ea175fc67 100644 --- a/crates/batten/src/secrets.rs +++ b/crates/batten/src/secrets.rs @@ -49,9 +49,52 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; -use crate::identity::IdentityKey; +use crate::error::UsageError; +use crate::identity::{IdentityKey, SecretSpan}; +use crate::provision::Provision; +use crate::refusal::{Fix, Refusal}; +use crate::rules::{Finding, Rule}; use crate::waiver::Date; +/// The `[[provision]]` entry this kind runs. +/// +/// A constant rather than a rule column, because the kind permits no new column +/// (CLOUD-59): a `tool = "..."` key would let one config point the secret kind at +/// an arbitrary binary and read its output as secret spans, which is a wider +/// capability than "scan for credentials" and a worse one to hand a config file. +/// The manifest still owns the version, the URL and the checksum — this only +/// fixes *which entry* is the scanner. +pub const SCANNER: &str = "ripsecrets"; + +/// The verb that installs the scanner, named once so the refusal and the +/// documentation cannot disagree. +const PROVISION_VERB: &str = "batten provision apply"; + +/// The scanner's flags, pinned here beside the parser that reads their output. +/// +/// `--only-matching` is **not** an option: it is what makes the third output +/// field the matched literal alone rather than the whole source line. The span +/// this kind hashes has to be the literal, because +/// [`crate::identity::secret_code_fingerprint`] forces `Verbatim` normalization — +/// a whole-line span would fold every edit elsewhere on the line into the +/// secret's identity, re-minting a finding nobody changed. +const SCANNER_FLAGS: &[&str] = &["--only-matching"]; + +/// The scanner's exit codes, pinned in one place and cross-checked against the +/// parse count (CLOUD-59's fail-closed clause). +/// +/// Measured against ripsecrets 0.1.11 rather than assumed: +/// +/// | code | meaning | +/// | ---- | ------- | +/// | 0 | clean — no secret found | +/// | 1 | one or more secrets found | +/// | 2 | the tool itself failed | +/// +/// Anything else, and any signal, is neither verdict. +const EXIT_CLEAN: i32 = 0; +const EXIT_FOUND: i32 = 1; + /// The directory the key file lives in, under the repository's state directory. const KEY_DIR: &str = "identity"; @@ -301,6 +344,233 @@ fn write_private(path: &Path, contents: &str) -> Result { Ok(true) } +// --- the scanner adapter ------------------------------------------------------ + +/// One parsed match, before it becomes a [`Finding`]. +/// +/// The span is already opaque: [`parse_line`] wraps it at the moment it is read +/// off the pipe, so no value in this module holds a matched byte as a `&str` +/// after that point. That is the containment claim, and it is a property of the +/// types rather than of the code below being careful. +struct Match { + path: String, + line: usize, + span: SecretSpan, +} + +/// Run the pinned scanner over `matched` and turn every match into a pointer. +/// +/// # Errors +/// +/// - **exit 1** ([`UsageError`]) when the manifest declares no scanner entry, or +/// the provision cache holds no binary — the message names +/// [`PROVISION_VERB`]. Never a silent pass: a scanner that did not run is not +/// evidence of a clean tree. +/// - **exit 3** (a plain internal error) for every fail-closed case: an +/// unparseable output line, a clean exit that nonetheless produced matches, a +/// found exit that produced none, and any other exit code or signal. Clean is +/// never inferred from a stream that failed to parse. +pub fn scan( + rule: &Rule, + provisions: &[Provision], + root: &Path, + matched: &[&String], + findings: &mut Vec, +) -> Result<()> { + let binary = resolve_scanner(provisions, root)?; + // Same resolution as the scanner's cache, and for the same reason: the key + // lives under the repository's state directory, which cannot be named from a + // relative anchor. + let key = load_or_mint( + &root + .canonicalize() + .with_context(|| format!("resolve the repository root at {}", root.display()))?, + crate::waiver::today()?, + )?; + + let mut parsed: Vec = Vec::new(); + for batch in crate::rules::batches(matched) { + parsed.extend(run_once(rule, &binary, root, &batch)?); + } + + for hit in parsed { + let fingerprint = + crate::identity::secret_code_fingerprint(&key, &rule.id, &hit.path, &hit.span)?; + findings.push(Finding { + rule: rule.id.clone(), + severity: rule.severity(), + path: hit.path, + line: Some(hit.line), + // The override still applies, and still keeps the identity keyed: + // `override_fingerprint` hashes the already-keyed fingerprint as a + // field, so a discriminator can split a group and can never unkey + // one. + identity: crate::identity::StoredIdentity::secret(match rule.identity_key.as_deref() { + Some(discriminator) => { + crate::identity::override_fingerprint(fingerprint, discriminator) + } + None => fingerprint, + }), + check: rule + .settling_check() + .unwrap_or(crate::findings::Check::Reevaluate), + remediation: None, + }); + } + Ok(()) +} + +/// Where the scanner binary is, or a refusal naming the verb that installs it. +fn resolve_scanner(provisions: &[Provision], root: &Path) -> Result { + let Some(entry) = provisions.iter().find(|entry| entry.name == SCANNER) else { + return Err(UsageError::raise( + Refusal::new( + SCANNER, + "a `secrets` rule needs the scanner pinned, and this config declares no \ + `[[provision]]` entry for it", + Fix::Run(PROVISION_VERB.to_owned()), + ) + .render(), + )); + }; + // The anchor is a RELATIVE path (`.`) whenever the config sits in the cwd, + // and the cache is keyed by the repository's own directory name — which + // `state::derive_repo_name` cannot read off `.`. Canonicalize rather than + // reach for `git::repo_root`: that would resolve a scratch fixture under + // `target/` to *this* repository, so a fixture asserting "no scanner + // installed" would start reading whichever cache the developer happened to + // have warmed. Canonicalizing answers for the directory actually anchored, + // which is what `provision apply` resolves to as well whenever the two run + // from the same place. + let repo = root + .canonicalize() + .with_context(|| format!("resolve the repository root at {}", root.display()))?; + let path = crate::provision::binary_path(&repo, entry)?; + if !path.is_file() { + // Never a silent pass — the missing-binary posture the command kind + // already holds, with the verb named because a refusal that says only + // what it would not do leaves the caller guessing (CLOUD-122). + return Err(UsageError::raise( + Refusal::new( + SCANNER, + "the pinned scanner is not in the provision cache, so no file was scanned", + Fix::Run(PROVISION_VERB.to_owned()), + ) + .render(), + )); + } + Ok(path) +} + +/// One scanner invocation over one batch of paths. +fn run_once(rule: &Rule, binary: &Path, root: &Path, batch: &[&str]) -> Result> { + let output = std::process::Command::new(binary) + .args(SCANNER_FLAGS) + .args(batch) + .current_dir(root) + // Both streams are captured and NEITHER is forwarded. stdout carries the + // matched bytes and is parsed here; stderr can carry a path the tool + // failed to read, and echoing a child's stream would put output Batten + // never shaped onto Batten's own (§6). + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .output() + .with_context(|| format!("rule {}: run the pinned secret scanner", rule.id))?; + + let stdout = String::from_utf8_lossy(&output.stdout); + let mut parsed = Vec::new(); + for (ordinal, line) in stdout.lines().enumerate() { + if line.is_empty() { + continue; + } + // Pointer-only, and this one matters most: the line that failed to parse + // is a line the scanner emitted because it found a secret in it. The + // error names WHICH line by ordinal and never what it said. + let hit = parse_line(line, batch).ok_or_else(|| { + anyhow::anyhow!( + "rule {}: the secret scanner emitted a line this build cannot parse \ + (output line {}); refusing to report a clean tree from a stream that \ + failed to parse", + rule.id, + ordinal + 1 + ) + })?; + parsed.push(hit); + } + + cross_check(rule, output.status.code(), parsed.len())?; + Ok(parsed) +} + +/// The exit-status/parse-count cross-check, both directions. +/// +/// Disagreement is exit 3 rather than a verdict, because each direction means +/// the parser and the tool disagree about what happened, and neither answer can +/// be trusted over the other. A clean exit with matches parsed would report +/// secrets the tool says it did not find; a found exit with nothing parsed would +/// report a clean tree the tool says is not clean — and that second one is the +/// silent false green this whole clause exists to prevent. +fn cross_check(rule: &Rule, code: Option, parsed: usize) -> Result<()> { + match code { + Some(EXIT_CLEAN) if parsed == 0 => Ok(()), + Some(EXIT_FOUND) if parsed > 0 => Ok(()), + Some(EXIT_CLEAN) => Err(anyhow::anyhow!( + "rule {}: the secret scanner exited clean while emitting {parsed} match(es); \ + the exit status and the output disagree, so neither is a verdict", + rule.id + )), + Some(EXIT_FOUND) => Err(anyhow::anyhow!( + "rule {}: the secret scanner reported findings and emitted none this build \ + could parse; clean is never inferred from a stream that failed to parse", + rule.id + )), + Some(other) => Err(anyhow::anyhow!( + "rule {}: the secret scanner exited {other}, which is neither its clean nor \ + its found status", + rule.id + )), + None => Err(anyhow::anyhow!( + "rule {}: the secret scanner was killed by a signal, so it reached no verdict", + rule.id + )), + } +} + +/// Parse one `::` output line, given the paths this batch +/// passed in. +/// +/// **Anchored on the known paths, never on separator position.** Splitting on +/// `:` is wrong twice over: a path may contain one, and so may a secret — and +/// the second is the dangerous one, because a mis-split there does not fail, it +/// silently keys a truncated span. Matching the longest known path that the line +/// starts with makes both non-events, and makes an unrecognised path a parse +/// failure (exit 3) rather than a guess. +/// +/// The span is wrapped **here**, at the boundary, and the `&str` it came from +/// does not outlive this function. +fn parse_line(line: &str, batch: &[&str]) -> Option { + // Longest first: with `a.rs` and `a.rs.bak` both in the batch, the shorter + // is a prefix of the longer and would claim the longer's lines. + let path = batch + .iter() + .filter(|path| line.len() > path.len() && line.as_bytes()[path.len()] == b':') + .filter(|path| line.starts_with(**path)) + .max_by_key(|path| path.len())?; + + let rest = &line[path.len() + 1..]; + let (number, span) = rest.split_once(':')?; + let line_number: usize = number.parse().ok()?; + if line_number == 0 { + // Line numbers are 1-based; a zero is a shape this build does not know. + return None; + } + Some(Match { + path: (*path).to_owned(), + line: line_number, + span: SecretSpan::mint(span), + }) +} + #[cfg(test)] #[allow(clippy::unwrap_used, clippy::expect_used)] mod tests { @@ -437,6 +707,177 @@ mod tests { assert!(!rendered.contains(&hex[..2]) || rendered.contains("")); } + // -- the parse boundary and the fail-closed cross-check ------------------- + + /// The span, recovered the only way a test can: fingerprint it under a known + /// key and compare against the fingerprint of a candidate string. There is + /// deliberately no accessor, so this is the assertion shape the design + /// leaves available — and that it is awkward is the containment working. + fn span_is(hit: &Match, expected: &str) -> bool { + let key = IdentityKey::new("k", [7u8; 32]); + let got = crate::identity::secret_code_fingerprint(&key, "r", "p", &hit.span).unwrap(); + let want = + crate::identity::secret_code_fingerprint(&key, "r", "p", &SecretSpan::mint(expected)) + .unwrap(); + got == want + } + + #[test] + fn a_line_parses_into_path_line_and_span() { + let batch = ["src/a.rs"]; + let hit = parse_line("src/a.rs:12:tok-abc", &batch).unwrap(); + assert_eq!(hit.path, "src/a.rs"); + assert_eq!(hit.line, 12); + assert!(span_is(&hit, "tok-abc")); + } + + #[test] + fn a_colon_in_the_span_is_not_a_separator() { + // The dangerous case, and the reason the parser anchors on known paths: + // splitting on `:` here does not fail, it silently keys a truncated + // span — a wrong answer that looks like a right one. + let batch = ["src/a.rs"]; + let hit = parse_line("src/a.rs:3:user:password@host", &batch).unwrap(); + assert_eq!(hit.line, 3); + assert!( + span_is(&hit, "user:password@host"), + "the whole remainder is the span, colons included" + ); + } + + #[test] + fn a_colon_in_the_path_is_not_a_separator_either() { + let batch = ["weird:name.rs"]; + let hit = parse_line("weird:name.rs:9:tok", &batch).unwrap(); + assert_eq!(hit.path, "weird:name.rs"); + assert_eq!(hit.line, 9); + assert!(span_is(&hit, "tok")); + } + + #[test] + fn the_longest_matching_path_wins() { + // `a.rs` is a prefix of `a.rs.bak`, so a first-match parser would credit + // the shorter path with the longer one's findings — a pointer to the + // wrong file, which is worse than no pointer. + let batch = ["a.rs", "a.rs.bak"]; + let hit = parse_line("a.rs.bak:4:tok", &batch).unwrap(); + assert_eq!(hit.path, "a.rs.bak"); + } + + #[test] + fn an_unparseable_line_is_none_rather_than_a_guess() { + let batch = ["src/a.rs"]; + for line in [ + "src/other.rs:1:tok", // a path this batch never passed in + "src/a.rs:notanumber:tok", + "src/a.rs:0:tok", // line numbers are 1-based + "src/a.rs", // no line, no span + "src/a.rs:1", // no span field + "", + "totally unrelated output", + ] { + assert!( + parse_line(line, &batch).is_none(), + "must not parse: {line:?}" + ); + } + } + + /// A `secrets` row, parsed from TOML rather than built as a struct — the + /// `identity_churn.rs` idiom, and the one that keeps a test from silently + /// depending on a field ordering the config surface does not have. + fn rule() -> Rule { + toml::from_str( + r#" +id = "no-secrets" +kind = "secrets" +glob = "**" +severity = "deny" +"#, + ) + .expect("the fixture row loads") + } + + #[test] + fn the_two_agreeing_cases_are_the_only_ones_that_pass() { + assert!(cross_check(&rule(), Some(EXIT_CLEAN), 0).is_ok()); + assert!(cross_check(&rule(), Some(EXIT_FOUND), 3).is_ok()); + } + + #[test] + fn a_clean_exit_with_matches_is_refused() { + // The tool says nothing was found and the stream says otherwise. Neither + // is a verdict, so this is exit 3 rather than a report. + let err = cross_check(&rule(), Some(EXIT_CLEAN), 2).unwrap_err(); + assert!( + err.downcast_ref::().is_none(), + "internal, not usage" + ); + assert!(err.to_string().contains("disagree"), "{err}"); + } + + #[test] + fn a_found_exit_with_no_matches_is_refused() { + // The silent false green this clause exists to prevent: the tool found + // secrets, the parser produced none, and reporting clean would be a + // clean tree inferred from a stream that failed to parse. + let err = cross_check(&rule(), Some(EXIT_FOUND), 0).unwrap_err(); + assert!(err.downcast_ref::().is_none()); + assert!(err.to_string().contains("never inferred"), "{err}"); + } + + #[test] + fn a_tool_error_or_a_signal_is_neither_verdict() { + let err = cross_check(&rule(), Some(2), 0).unwrap_err(); + assert!(err.to_string().contains("neither its clean nor"), "{err}"); + + let err = cross_check(&rule(), None, 0).unwrap_err(); + assert!(err.to_string().contains("signal"), "{err}"); + } + + #[test] + fn no_refusal_or_error_rendering_carries_a_span() { + // Every message this module can emit, checked against a span that would + // be in it if any path echoed one. Pointer-only is the rule these all + // serve, and an error is the easiest place to break it by accident. + const SECRET: &str = "tok-would-be-leaked"; + let batch = ["src/a.rs"]; + let rendered: Vec = vec![ + cross_check(&rule(), Some(EXIT_CLEAN), 2) + .unwrap_err() + .to_string(), + cross_check(&rule(), Some(EXIT_FOUND), 0) + .unwrap_err() + .to_string(), + cross_check(&rule(), Some(2), 0).unwrap_err().to_string(), + cross_check(&rule(), None, 0).unwrap_err().to_string(), + resolve_scanner(&[], Path::new("/nowhere")) + .unwrap_err() + .to_string(), + ]; + for message in &rendered { + assert!( + !message.contains(SECRET) && !message.contains("tok-"), + "a message rendered a span: {message}" + ); + } + // And the parse failure names an ordinal, never the line — asserted at + // the one place the line is in scope. + assert!(parse_line(&format!("unknown.rs:1:{SECRET}"), &batch).is_none()); + } + + #[test] + fn a_missing_scanner_entry_names_the_verb_that_installs_it() { + let err = resolve_scanner(&[], Path::new("/nowhere")).unwrap_err(); + assert!( + err.downcast_ref::().is_some(), + "a config that cannot run its own gate is exit 1" + ); + let text = err.to_string(); + assert!(text.contains(PROVISION_VERB), "{text}"); + assert!(text.contains(SCANNER), "{text}"); + } + #[test] fn a_round_trip_of_the_key_encoding_is_lossless_and_single_spelled() { let bytes = [0xABu8; KEY_BYTES]; diff --git a/crates/batten/tests/fixtures/repos/secrets-uninstalled/batten.toml.in b/crates/batten/tests/fixtures/repos/secrets-uninstalled/batten.toml.in new file mode 100644 index 000000000..d88498a11 --- /dev/null +++ b/crates/batten/tests/fixtures/repos/secrets-uninstalled/batten.toml.in @@ -0,0 +1,23 @@ +version = 1 + +# The `secrets` kind with its scanner NOT provisioned, which is the honest +# fixture for a corpus that installs nothing (CLOUD-59). The happy path needs a +# stub binary in a cache this harness has no way to populate, so it lives in +# `tests/secrets_kind.rs`, which stands up its own XDG_DATA_HOME. +# +# What this pins is the posture that matters most on a cold machine: a scanner +# that did not run is NEVER reported as a clean tree. Exit 1 naming the verb that +# installs it, never exit 0. +[[provision]] +name = "ripsecrets" +version = "0.0.0-fixture" +url = "file:///dev/null/never-fetched" +sha256 = "0000000000000000000000000000000000000000000000000000000000000000" +binary = "ripsecrets" + +[[rule]] +id = "no-secrets" +kind = "secrets" +glob = "**/*.rs" +severity = "deny" +scope = "tree" diff --git a/crates/batten/tests/fixtures/repos/secrets-uninstalled/expected.in b/crates/batten/tests/fixtures/repos/secrets-uninstalled/expected.in new file mode 100644 index 000000000..16403a454 --- /dev/null +++ b/crates/batten/tests/fixtures/repos/secrets-uninstalled/expected.in @@ -0,0 +1,4 @@ +argv: enforce +exit: 1 +stderr-contains: batten provision apply +stdout: diff --git a/crates/batten/tests/fixtures/repos/secrets-uninstalled/lib.rs.in b/crates/batten/tests/fixtures/repos/secrets-uninstalled/lib.rs.in new file mode 100644 index 000000000..da0f5d925 --- /dev/null +++ b/crates/batten/tests/fixtures/repos/secrets-uninstalled/lib.rs.in @@ -0,0 +1 @@ +pub fn main() {} diff --git a/crates/batten/tests/identity_churn.rs b/crates/batten/tests/identity_churn.rs index 640141c2a..9d60919b4 100644 --- a/crates/batten/tests/identity_churn.rs +++ b/crates/batten/tests/identity_churn.rs @@ -76,7 +76,7 @@ impl Scan { /// written against the whole matched line, and they still pass, so the /// engine demonstrably picks the same span the test used to. fn of(root: &Path, rules: &[Rule]) -> Self { - let findings = rules::run_static(rules, root) + let findings = rules::run_static(rules, &[], root) .expect("scan the tree") .findings; let identities = identity::count_occurrences( diff --git a/crates/batten/tests/primitives.rs b/crates/batten/tests/primitives.rs index 0368654ed..ea4948c01 100644 --- a/crates/batten/tests/primitives.rs +++ b/crates/batten/tests/primitives.rs @@ -852,13 +852,13 @@ fn the_acceptance_runner_is_the_landed_rule_engine() { let repo = repo("acceptance-runner"); repo.write("a.txt", "content\n"); - let err = batten::rules::run_static(&config.rules, &repo.dir).unwrap_err(); + let err = batten::rules::run_static(&config.rules, &config.provisions, &repo.dir).unwrap_err(); assert!( err.downcast_ref::().is_some(), "the read-effect surface refuses a spawning rule loudly, never skips it" ); assert!( - batten::rules::run_all(&config.rules, &repo.dir) + batten::rules::run_all(&config.rules, &config.provisions, &repo.dir) .expect("the spawning surface runs it") .findings .is_empty(), diff --git a/crates/batten/tests/secrets_kind.rs b/crates/batten/tests/secrets_kind.rs new file mode 100644 index 000000000..d0764285d --- /dev/null +++ b/crates/batten/tests/secrets_kind.rs @@ -0,0 +1,531 @@ +//! The `secrets` rule kind, end to end over the compiled binary (CLOUD-59). +//! +//! **The claim under test is a negative**: that no surface Batten writes can +//! carry a matched byte. A negative needs a witness that would be found if the +//! claim were false, so every case here plants a token and then searches for it +//! everywhere the run could have put it — stdout, stderr, the `-J` document, and +//! every file under the state root, which is where the key and any cache live. +//! +//! # The planted token is assembled at runtime, and that is load-bearing +//! +//! Consumer #1's own `no-secrets` rule globs the whole tree, so a literal +//! credential written into this file would be a standing violation of the rule +//! this suite exists to prove — the repository would fail its own gate, forever, +//! on its own test fixture. The token is therefore built from fragments at +//! runtime and exists in no committed byte sequence. +//! +//! The same reasoning rules out a real secret of any kind: these are synthetic +//! shapes that exist nowhere but the scratch tree each test builds. +//! +//! # The scanner is a stub, provisioned through `file://` +//! +//! The suite pins the *adapter*, not ripsecrets' detection. A stub emits the +//! exact output shape measured against ripsecrets 0.1.11 — `path:line:span` +//! under `--only-matching`, exit 0 clean / 1 found — which is what lets a case +//! drive the disagreement branches a real scanner will not produce on demand. +//! Detection itself is adopted prior art and is exercised by consumer #1's own +//! gate once the pin lands. + +// Panicking on setup failure is the idiomatic way for a test to fail loudly. +#![allow(clippy::unwrap_used, clippy::expect_used)] + +mod common; + +use std::fs; +use std::path::{Path, PathBuf}; +use std::process::Output; + +use batten::provision::digest; +use common::{Fixture, batten, git_in, scratch}; + +/// A repository, an isolated `HOME`/`XDG_DATA_HOME`, and a place to write stub +/// artifacts — the shape `tests/provision.rs` already uses, for the same reason: +/// the cache and the minted key must land somewhere this test owns rather than +/// in the developer's real state directory. +struct Env { + repo: PathBuf, + home: PathBuf, + artifacts: PathBuf, +} + +impl Env { + fn new(name: &str) -> Self { + let root = scratch(name); + let repo = Fixture::at(root.join("repo")) + .file("README.md", "base\n") + .build(); + git_in(&repo, &["init", "-q"]); + git_in(&repo, &["add", "-A"]); + git_in(&repo, &["commit", "-q", "-m", "base"]); + let artifacts = root.join("artifacts"); + fs::create_dir_all(&artifacts).unwrap(); + Env { + repo, + home: root.join("home"), + artifacts, + } + } + + fn run(&self, args: &[&str]) -> Output { + batten() + .args(args) + .current_dir(&self.repo) + .env("HOME", &self.home) + .env("XDG_DATA_HOME", self.home.join("data")) + .output() + .expect("run batten") + } + + fn cache(&self) -> PathBuf { + self.home.join("data").join("batten") + } + + fn file(&self, path: &str, contents: &str) { + let full = self.repo.join(path); + if let Some(parent) = full.parent() { + fs::create_dir_all(parent).unwrap(); + } + fs::write(full, contents).unwrap(); + } + + /// Write a stub scanner, provision it, and write the config that uses it. + /// + /// Returns after `provision apply` has installed it, so a case that wants a + /// COLD cache simply does not call this. + fn install_scanner(&self, script: &str) { + let path = self.artifacts.join("ripsecrets"); + fs::write(&path, script).unwrap(); + let bytes = fs::read(&path).unwrap(); + self.config(&format!("file://{}", path.display()), &digest(&bytes)); + let applied = self.run(&["provision", "apply"]); + assert!( + applied.status.success(), + "the stub scanner installs: {}", + String::from_utf8_lossy(&applied.stderr) + ); + } + + fn config(&self, url: &str, sha: &str) { + self.file( + "batten.toml", + &format!( + "version = 1\n\n\ + [[provision]]\n\ + name = \"ripsecrets\"\n\ + version = \"0.0.0-stub\"\n\ + url = \"{url}\"\n\ + sha256 = \"{sha}\"\n\ + binary = \"ripsecrets\"\n\n\ + [[rule]]\n\ + id = \"no-secrets\"\n\ + kind = \"secrets\"\n\ + glob = \"**/*.conf\"\n\ + severity = \"deny\"\n\ + scope = \"tree\"\n" + ), + ); + } + + /// Every byte this run could have written, out of tree: the provision cache + /// and the minted key both live here. + fn state_bytes(&self) -> Vec { + let mut out = Vec::new(); + collect(&self.cache(), &mut out); + out + } +} + +fn collect(dir: &Path, out: &mut Vec) { + let Ok(entries) = fs::read_dir(dir) else { + return; + }; + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + collect(&path, out); + } else if let Ok(bytes) = fs::read(&path) { + out.extend_from_slice(&bytes); + out.push(b'\n'); + } + } +} + +/// The fragments the synthetic credentials are assembled from. +/// +/// Split so the contiguous token exists in no committed byte sequence — see the +/// module docs: a literal here would violate consumer #1's own `no-secrets` rule +/// over this very file. +const TOKEN_PARTS: [&str; 5] = ["AKIA", "7QF2", "NX8M", "3JD5", "W0PC"]; +const OTHER_PARTS: [&str; 5] = ["AKIA", "B4T6", "LZ9R", "K1YV", "H2SD"]; + +fn token() -> String { + TOKEN_PARTS.concat() +} + +fn other_token() -> String { + OTHER_PARTS.concat() +} + +/// The shell that reconstructs a token inside the stub, without the contiguous +/// string ever appearing in the script. +/// +/// **This is not decoration, and the first version of this suite failed without +/// it.** `provision` caches the exact artifact bytes by contract, so a stub +/// carrying the token as a literal puts it under the state root — and the +/// assertions below search the state root precisely because that is where a leak +/// would land. A test whose own fixture trips its assertion teaches nothing, and +/// weakening the assertion to exclude the cache would have hidden a real leak in +/// the one place Batten writes files nobody reads. +fn shell_token(parts: [&str; 5]) -> String { + let mut out = String::new(); + for part in parts { + out.push('"'); + out.push_str(part); + out.push('"'); + } + out +} + +/// A stub emitting ripsecrets' measured output shape for the given matches. +/// +/// A match is `(path, line, which token)`; `None` means a span the parser should +/// refuse. The stub ignores its arguments beyond echoing what it was told to, +/// which is what makes the disagreement cases below expressible at all — a real +/// scanner cannot be asked to contradict itself. +fn stub(matches: &[(&str, usize, Option<[&str; 5]>)], exit: i32) -> String { + let mut script = String::from("#!/bin/sh\n"); + for (index, (path, line, parts)) in matches.iter().enumerate() { + if let Some(parts) = parts { + script.push_str(&format!("T{index}={}\n", shell_token(*parts))); + script.push_str(&format!("printf '%s\\n' \"{path}:{line}:$T{index}\"\n")); + } else { + // A shape this build cannot parse, still carrying a token: the + // hazard is that an unparseable line is one the scanner emitted + // BECAUSE it found a secret in it. + script.push_str(&format!("T{index}={}\n", shell_token(TOKEN_PARTS))); + script.push_str(&format!("printf '%s\\n' \"!! unexpected $T{index}\"\n")); + } + } + script.push_str(&format!("exit {exit}\n")); + script +} + +/// The stub that reports nothing and exits clean. +fn clean_stub() -> String { + stub(&[], 0) +} + +/// Assert `needle` appears in none of the places a run can write. +fn nowhere(env: &Env, output: &Output, needle: &str, case: &str) { + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); + assert!(!stdout.contains(needle), "{case}: the token reached stdout"); + assert!(!stderr.contains(needle), "{case}: the token reached stderr"); + let state = env.state_bytes(); + assert!( + !String::from_utf8_lossy(&state).contains(needle), + "{case}: the token reached a file under the state root" + ); +} + +// --- (a) the deny verdict is pointer-only ------------------------------------- + +#[test] +fn a_planted_secret_is_a_pointer_and_never_its_bytes() { + let env = Env::new("secrets-pointer"); + let secret = token(); + env.file("app.conf", &format!("api_key = \"{secret}\"\n")); + env.install_scanner(&stub(&[("app.conf", 1, Some(TOKEN_PARTS))], 1)); + + let out = env.run(&["enforce"]); + assert_eq!( + out.status.code(), + Some(2), + "a deny finding is the policy verdict" + ); + assert_eq!( + String::from_utf8_lossy(&out.stdout), + "app.conf:1 no-secrets\n", + "stdout is the pointer line and nothing else" + ); + nowhere(&env, &out, &secret, "text output"); +} + +#[test] +fn the_json_document_carries_no_span_either() { + let env = Env::new("secrets-json"); + let secret = token(); + env.file("app.conf", &format!("api_key = \"{secret}\"\n")); + env.install_scanner(&stub(&[("app.conf", 1, Some(TOKEN_PARTS))], 1)); + + let out = env.run(&["enforce", "-J"]); + assert_eq!(out.status.code(), Some(2)); + let document: serde_json::Value = + serde_json::from_slice(&out.stdout).expect("the -J document parses"); + let finding = &document["findings"][0]; + assert_eq!(finding["path"], "app.conf"); + assert_eq!(finding["line"], 1); + assert_eq!(finding["rule"], "no-secrets"); + nowhere(&env, &out, &secret, "-J output"); +} + +// --- (b) the identity is secret-class and keyed ------------------------------- + +#[test] +fn the_emitted_identity_is_secret_class_and_differs_from_the_unkeyed_digest() { + let env = Env::new("secrets-identity"); + let secret = token(); + env.file("app.conf", &format!("api_key = \"{secret}\"\n")); + env.install_scanner(&stub(&[("app.conf", 1, Some(TOKEN_PARTS))], 1)); + + let out = env.run(&["enforce", "-J"]); + let document: serde_json::Value = serde_json::from_slice(&out.stdout).unwrap(); + let identity = &document["findings"][0]["identity"]; + + // The version tag is what asserts the adapter selected the KEYED path: an + // unkeyed span would have been minted under the code kind's version. + let version = identity["version"].as_str().expect("a version"); + assert!( + version.starts_with("secret:"), + "the identity is secret-class, not code-class: {version}" + ); + + // And the fingerprint is not the unkeyed digest of the same span. + let unkeyed = batten::identity::code_fingerprint( + "no-secrets", + "app.conf", + &secret, + batten::identity::SpanNormalization::Verbatim, + ) + .unwrap(); + assert_ne!( + identity["fingerprint"].as_str().unwrap(), + unkeyed.to_hex(), + "a keyed identity must never equal the unkeyed digest it exists to replace" + ); +} + +#[test] +fn two_repositories_mint_two_keys_and_two_identities_for_one_secret() { + // The machine-scoped key, observed rather than asserted: the same secret at + // the same path under two state roots is two identities, which is the + // documented trade the module doc states. + let secret = token(); + let fingerprint = |name: &str| -> String { + let env = Env::new(name); + env.file("app.conf", &format!("api_key = \"{secret}\"\n")); + env.install_scanner(&stub(&[("app.conf", 1, Some(TOKEN_PARTS))], 1)); + let out = env.run(&["enforce", "-J"]); + let document: serde_json::Value = serde_json::from_slice(&out.stdout).unwrap(); + document["findings"][0]["identity"]["fingerprint"] + .as_str() + .unwrap() + .to_owned() + }; + assert_ne!(fingerprint("secrets-key-a"), fingerprint("secrets-key-b")); +} + +// --- (c) byte-stability, including across matched files ----------------------- + +#[test] +fn the_same_input_twice_is_byte_identical_and_ordered() { + let env = Env::new("secrets-stable"); + let (one, two) = (token(), other_token()); + env.file("b.conf", &format!("k = \"{one}\"\n")); + env.file("a.conf", &format!("k = \"{two}\"\n")); + // Emitted in the reverse of the order the output must appear in: the + // scanner's walker is parallel, so the adapter sorts rather than trusting + // the stream's order. + env.install_scanner(&stub( + &[ + ("b.conf", 1, Some(TOKEN_PARTS)), + ("a.conf", 1, Some(OTHER_PARTS)), + ], + 1, + )); + + let first = env.run(&["enforce"]); + let second = env.run(&["enforce"]); + assert_eq!(first.stdout, second.stdout, "text output is byte-stable"); + assert_eq!( + String::from_utf8_lossy(&first.stdout), + "a.conf:1 no-secrets\nb.conf:1 no-secrets\n", + "ordered by path, not by the order the scanner happened to emit" + ); + + let first_json = env.run(&["enforce", "-J"]); + let second_json = env.run(&["enforce", "-J"]); + assert_eq!(first_json.stdout, second_json.stdout, "-J is byte-stable"); + + // A second run reuses the key rather than re-minting: identical output IS + // that assertion, since the key id is inside every fingerprint. + nowhere(&env, &first, &one, "run one"); + nowhere(&env, &second_json, &two, "run two"); +} + +// --- (d) the read-only surface refuses the kind ------------------------------- + +#[test] +fn check_refuses_a_config_carrying_a_secrets_rule() { + let env = Env::new("secrets-check-refusal"); + env.install_scanner(&clean_stub()); + + let out = env.run(&["check"]); + assert_eq!( + out.status.code(), + Some(1), + "a refusal is usage, not a verdict" + ); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + stderr.contains("batten enforce"), + "the refusal names the verb that runs it: {stderr}" + ); +} + +// --- (e) a cold cache is never a clean tree ----------------------------------- + +#[test] +fn an_uninstalled_scanner_is_exit_one_naming_the_install_verb() { + let env = Env::new("secrets-cold-cache"); + let secret = token(); + env.file("app.conf", &format!("api_key = \"{secret}\"\n")); + // Config written, `provision apply` deliberately NOT run. + env.config("file:///dev/null/never-fetched", &"0".repeat(64)); + + let out = env.run(&["enforce"]); + assert_eq!( + out.status.code(), + Some(1), + "a scanner that did not run is not evidence of a clean tree" + ); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!(stderr.contains("batten provision apply"), "{stderr}"); + nowhere(&env, &out, &secret, "cold cache"); +} + +// --- (f) the fail-closed cross-check, both directions ------------------------- + +#[test] +fn a_clean_exit_that_emitted_a_match_is_internal_error() { + let env = Env::new("secrets-clean-with-match"); + let secret = token(); + env.file("app.conf", &format!("api_key = \"{secret}\"\n")); + env.install_scanner(&stub(&[("app.conf", 1, Some(TOKEN_PARTS))], 0)); + + let out = env.run(&["enforce"]); + assert_eq!( + out.status.code(), + Some(3), + "the exit status and the output disagree, so neither is a verdict" + ); + nowhere(&env, &out, &secret, "clean-with-match"); +} + +#[test] +fn a_found_exit_that_emitted_nothing_is_internal_error() { + // The silent false green the clause exists to prevent: the tool says it + // found secrets, the parser produced none, and reporting clean would infer a + // clean tree from a stream that failed to parse. + let env = Env::new("secrets-found-with-none"); + env.file("app.conf", "k = 1\n"); + env.install_scanner(&stub(&[], 1)); + + let out = env.run(&["enforce"]); + assert_eq!(out.status.code(), Some(3)); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!(stderr.contains("never inferred"), "{stderr}"); +} + +#[test] +fn an_unparseable_line_is_internal_error_naming_no_bytes() { + let env = Env::new("secrets-unparseable"); + let secret = token(); + env.file("app.conf", &format!("api_key = \"{secret}\"\n")); + // A shape this build cannot parse, still carrying the token. + env.install_scanner(&stub(&[("app.conf", 1, None)], 1)); + + let out = env.run(&["enforce"]); + assert_eq!(out.status.code(), Some(3)); + nowhere(&env, &out, &secret, "unparseable line"); +} + +#[test] +fn a_tool_failure_is_internal_error_rather_than_a_clean_tree() { + let env = Env::new("secrets-tool-error"); + env.file("app.conf", "k = 1\n"); + env.install_scanner(&stub(&[], 2)); + + let out = env.run(&["enforce"]); + assert_eq!( + out.status.code(), + Some(3), + "the scanner's own failure is neither verdict" + ); +} + +// --- (g) a clean tree is still clean ------------------------------------------ + +#[test] +fn a_clean_scan_exits_zero_with_no_output() { + // The negative control: without it every case above would pass on an engine + // that always errored. + let env = Env::new("secrets-clean"); + env.file("app.conf", "k = 1\n"); + env.install_scanner(&clean_stub()); + + let out = env.run(&["enforce"]); + assert_eq!(out.status.code(), Some(0)); + assert_eq!(String::from_utf8_lossy(&out.stdout), ""); +} + +#[test] +fn a_glob_matching_nothing_never_spawns_the_scanner() { + // The glob is a gate before it is an argv source: a stub that would fail the + // run if executed proves the spawn never happened. + let env = Env::new("secrets-no-match"); + env.file("app.txt", "not matched by the rule's glob\n"); + env.install_scanner("#!/bin/sh\nexit 3\n"); + + let out = env.run(&["enforce"]); + assert_eq!( + out.status.code(), + Some(0), + "no match means no spawn, so the failing stub never ran" + ); +} + +// --- (h) the key file's custody, observed through the binary ------------------ + +#[test] +fn the_key_is_minted_owner_only_under_the_state_root() { + let env = Env::new("secrets-key-file"); + let secret = token(); + env.file("app.conf", &format!("api_key = \"{secret}\"\n")); + env.install_scanner(&stub(&[("app.conf", 1, Some(TOKEN_PARTS))], 1)); + env.run(&["enforce"]); + + // Resolved against the XDG_DATA_HOME the CHILD ran with, not this process's: + // `batten::secrets::key_path` reads the ambient environment, which in the + // test runner is the developer's own, so calling it here would assert about + // the wrong directory entirely. + let key = env + .cache() + .join(env.repo.file_name().unwrap()) + .join("identity") + .join("secret-key"); + assert!(key.is_file(), "the run minted a key at {}", key.display()); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = fs::metadata(&key).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "the key file is owner-only"); + } + + // And it is never in the tree. + assert!( + !env.repo.join("identity").exists(), + "nothing about the key is written into the repository" + ); +} diff --git a/hk.pkl b/hk.pkl index 9ee3c6036..28ffdfbf5 100644 --- a/hk.pkl +++ b/hk.pkl @@ -200,9 +200,10 @@ local gate = new Mapping { // declarations and fixtures, and on the modules whose output shape IS the // measurement, since a change to any of them moves the numbers. // - // `token-bench`, not `bench`: CLOUD-207's `mise run bench` measures batten's - // INVOCATION COST and landed under that name first. Two benchmarks with two - // subjects need two names, and the more specific one moves. + // `token-bench`, not `perf`: CLOUD-207's `mise run perf` measures batten's + // INVOCATION COST, and it held the name `bench` until CLOUD-172 moved the whole + // latency family into the `perf` namespace its own Mechanism specifies. Two + // benchmarks with two subjects need two names, and neither may be `bench`. ["token-bench-check"] { // `slow` tier: re-executes `token-bench`, which builds the engine and then // runs N workloads x N runs. See the `profiles` note at the foot of this file. diff --git a/mise-tasks/hook-latency-drift b/mise-tasks/hook-latency-drift index 9b4c8340b..c20c629e9 100755 --- a/mise-tasks/hook-latency-drift +++ b/mise-tasks/hook-latency-drift @@ -33,7 +33,7 @@ set -euo pipefail PROFILE="slow" -# The budget, in seconds, written once as data — the placement `bench-assert`'s +# The budget, in seconds, written once as data — the placement `perf-assert`'s # BUDGETS and `run-shape-guard`'s verdict-bearing list use. It is a ceiling # derived from measurement (see CLOUD-509), not a target chosen in advance. # @@ -76,7 +76,7 @@ scratch="$(mktemp -d)" cleanup() { rm -rf "$scratch"; } trap cleanup EXIT -# The measurement. Deliberately NOT hyperfine: `bench` uses it for a +# The measurement. Deliberately NOT hyperfine: `perf` uses it for a # millisecond-scale binary where 100 runs are cheap and the tail is the point, # while this is a multi-second whole-gate run where 3 samples is already minutes # of runner time. The median of a few runs is the honest instrument at this diff --git a/mise-tasks/bench b/mise-tasks/perf similarity index 85% rename from mise-tasks/bench rename to mise-tasks/perf index ad44cce08..5ffa68240 100755 --- a/mise-tasks/bench +++ b/mise-tasks/perf @@ -8,13 +8,13 @@ # routed around, and the rule it enforces stops binding. The repo asserted speed # nowhere and measured it nowhere before this task. # -# THIS MEASURES; IT DOES NOT DECIDE. The verdict is `mise run bench-assert`'s, a +# THIS MEASURES; IT DOES NOT DECIDE. The verdict is `mise run perf-assert`'s, a # pure function of the records below, for the same reason every board gate here # splits that way (`graph-check`, `claim-check`, `ci-wait`): agents fetch, gates # decide. A measurement needs a quiet machine, a release build and a couple of # minutes; a decision needs none of those, so keeping them apart is what lets the # decision run in the hk gate on every commit while the measurement runs where it -# can be trusted (.github/workflows/bench.yml, on a schedule). +# can be trusted (.github/workflows/perf.yml, on a schedule). # # THE THREE PATHS, chosen because they bracket what a hook actually costs: # @@ -31,7 +31,7 @@ # FIXTURES ARE REUSED, NOT INVENTED. The check path materialises # `crates/batten/tests/fixtures/repos/forbid-clean/`, and the hook path feeds # `crates/batten/tests/fixtures/hooks/claude-code.json` — the same bytes the Rust -# suite already pins against drift. A second corpus maintained only for the bench +# suite already pins against drift. A second corpus maintained only for the perf # would drift from the one the tests assert on, and then the published number # would describe an input nothing else uses. # @@ -46,7 +46,7 @@ # # OUTPUT IS A POINTER, NEVER A PAYLOAD (non-negotiable rule 4): one KEY=VALUE # record per path on stdout and nothing else, so a caller can redirect it into -# `bench-assert`, append it to a series (CLOUD-172), or paste it into a summary +# `perf-assert`, append it to a series (CLOUD-172), or paste it into a summary # without a parser. The raw hyperfine JSON stays under target/ for a human. # # Task bodies do not run under `set -e`, and a number computed from a failed @@ -57,13 +57,13 @@ set -euo pipefail cd "$(git rev-parse --show-toplevel)" # Where the measurement's working files go. Under target/, which is git-ignored, -# so a bench run never dirties a clone and never has to be cleaned up by hand. +# so a perf run never dirties a clone and never has to be cleaned up by hand. # # Resolved to an ABSOLUTE path below, once it exists: the check path runs with # the fixture repo as its working directory, and a relative one would put that # path's JSON and stderr inside the fixture — where the first of them failed to # open at all, since the directory does not exist relative to there. -bench_out="${BENCH_OUT_DIR:-target/bench}" +perf_out="${BENCH_OUT_DIR:-target/perf}" # Runs per path. hyperfine's default (10, or as many as fit its time budget) is # tuned for commands measured in seconds; these are measured in milliseconds, so @@ -75,7 +75,7 @@ readonly WARMUP="${BENCH_WARMUP:-10}" for tool in hyperfine jq; do if ! command -v "$tool" >/dev/null 2>&1; then - echo "::error:: bench: $tool is not installed — run \`mise install\`; it is pinned in mise.toml." >&2 + echo "::error:: perf: $tool is not installed — run \`mise install\`; it is pinned in mise.toml." >&2 exit 1 fi done @@ -83,18 +83,18 @@ done # The binary under measurement. Built once, up front: hyperfine would otherwise # time the first invocation of an unbuilt binary as if the compile were startup. if ! cargo build --quiet --release -p batten; then - echo "::error:: bench: the release build failed, so there is nothing to measure. No records written." >&2 + echo "::error:: perf: the release build failed, so there is nothing to measure. No records written." >&2 exit 1 fi readonly BIN="target/release/batten" if [ ! -x "$BIN" ]; then - echo "::error:: bench: $BIN is missing after a successful build — refusing to measure something else." >&2 + echo "::error:: perf: $BIN is missing after a successful build — refusing to measure something else." >&2 exit 1 fi -rm -rf "$bench_out" -mkdir -p "$bench_out" -OUT_DIR="$(cd "$bench_out" && pwd)" +rm -rf "$perf_out" +mkdir -p "$perf_out" +OUT_DIR="$(cd "$perf_out" && pwd)" readonly OUT_DIR # The check path's repository: a fixture with one `forbid` rule and one file to @@ -104,7 +104,7 @@ readonly FIXTURE_REPO="crates/batten/tests/fixtures/repos/forbid-clean" readonly FIXTURE_HOOK="crates/batten/tests/fixtures/hooks/claude-code.json" for f in "$FIXTURE_REPO/batten.toml.in" "$FIXTURE_REPO/lib.rs.in" "$FIXTURE_HOOK"; do if [ ! -f "$f" ]; then - echo "::error:: bench: fixture $f is missing, so the measured input is not the one the test suite pins." >&2 + echo "::error:: perf: fixture $f is missing, so the measured input is not the one the test suite pins." >&2 exit 1 fi done @@ -153,7 +153,7 @@ measure() { # still perfectly timeable, and without the check it would be published as # a fast number rather than a broken path. if ! (cd "$dir" && hyperfine "${args[@]}" "$command" >/dev/null 2>"$OUT_DIR/$id.err"); then - echo "::error:: bench: measuring the $id path failed — see $OUT_DIR/$id.err. No records written." >&2 + echo "::error:: perf: measuring the $id path failed — see $OUT_DIR/$id.err. No records written." >&2 exit 1 fi @@ -172,7 +172,7 @@ measure() { | (($n - 1) * 0.95 | ceil) as $i95 | "path=\($id) p50=\($t[$i50] * 1000 | . * 100 | round / 100) p95=\($t[$i95] * 1000 | . * 100 | round / 100) mean=\($r.mean * 1000 | . * 100 | round / 100) runs=\($n)" ' "$json"; then - echo "::error:: bench: could not read the $id measurement out of $json. No records written." >&2 + echo "::error:: perf: could not read the $id measurement out of $json. No records written." >&2 exit 1 fi } @@ -188,7 +188,7 @@ measure hook "$PWD" "$ABS_BIN hook --harness claude-code" # # DERIVED from the settings file rather than hardcoded, so the measurement # cannot describe a wiring the repository no longer has. That is the whole -# defect this path exists to close: `bench-assert` was green over an uninvoked +# defect this path exists to close: `perf-assert` was green over an uninvoked # binary for as long as the budget existed. # # One entry today. If the file ever wires several, this measures the first and @@ -197,16 +197,16 @@ measure hook "$PWD" "$ABS_BIN hook --harness claude-code" wired_cmd=$(jq -r '[.hooks.PreToolUse[]? | .hooks[].command] | .[0] // empty' .claude/settings.json 2>/dev/null || true) wired_count=$(jq -r '[.hooks.PreToolUse[]? | .hooks[].command] | length' .claude/settings.json 2>/dev/null || echo 0) if [ -z "$wired_cmd" ]; then - echo "::error:: bench: .claude/settings.json wires no PreToolUse command, so the wired path cannot be measured. No records written." >&2 + echo "::error:: perf: .claude/settings.json wires no PreToolUse command, so the wired path cannot be measured. No records written." >&2 exit 1 fi # The settings file spells the project dir as a variable the harness expands. wired_cmd=${wired_cmd//\$CLAUDE_PROJECT_DIR/$PWD} if [ ! -x "$wired_cmd" ]; then - echo "::error:: bench: the wired PreToolUse command is not executable: $wired_cmd. No records written." >&2 + echo "::error:: perf: the wired PreToolUse command is not executable: $wired_cmd. No records written." >&2 exit 1 fi if [ "$wired_count" -ne 1 ]; then - echo "::warning:: bench: $wired_count PreToolUse commands wired; measuring only the first ($wired_cmd)." >&2 + echo "::warning:: perf: $wired_count PreToolUse commands wired; measuring only the first ($wired_cmd)." >&2 fi measure wired "$PWD" "$wired_cmd" diff --git a/mise-tasks/bench-assert b/mise-tasks/perf-assert similarity index 83% rename from mise-tasks/bench-assert rename to mise-tasks/perf-assert index b7d37dbef..df8659810 100755 --- a/mise-tasks/bench-assert +++ b/mise-tasks/perf-assert @@ -1,7 +1,7 @@ #!/usr/bin/env bash -#MISE description="Gate: every measured invocation path is inside its latency budget, and README publishes the budget this gate enforces (reads `bench` records on stdin)" +#MISE description="Gate: every measured invocation path is inside its latency budget, and README publishes the budget this gate enforces (reads `perf` records on stdin)" # -# CLOUD-207. The other half of `mise run bench`, and the half that makes the +# CLOUD-207. The other half of `mise run perf`, and the half that makes the # published number mean anything: a latency figure with no assertion behind it # decays in silence, because nothing about a slower binary announces itself. # Nobody notices 8ms becoming 40ms; they notice, months later, that the hook got @@ -10,9 +10,9 @@ # A PURE FUNCTION OF STDIN, the `graph-check`/`claim-check` interface: agents # fetch, gates decide. The measurement needs a release build, a quiet machine and # a couple of minutes; this needs none of them, which is the whole point of the -# split. `tests/bench-assert.bats` therefore runs in the hk gate on every commit +# split. `tests/perf-assert.bats` therefore runs in the hk gate on every commit # — the DECISION is defended per-commit at zero cost, while the MEASUREMENT runs -# on a schedule (.github/workflows/bench.yml) where a shared runner's noise can +# on a schedule (.github/workflows/perf.yml) where a shared runner's noise can # be absorbed rather than paid for on the landing path. # # THE BUDGET IS AN ABSOLUTE CEILING, NOT A RATCHET, and that is deliberate. @@ -42,7 +42,7 @@ # and fails when it disagrees with the data below, the same shape `hk-version` # uses to hold mise.toml and hk.pkl in agreement. It judges the BUDGET, never the # measured figures: those are a snapshot of one machine at one commit and are -# expected to be stale between bench runs, so gating them would fail the repo for +# expected to be stale between perf runs, so gating them would fail the repo for # the passage of time. # # Exit 0 pass / 1 a path over budget or a README disagreement / 2 could not look. @@ -60,7 +60,7 @@ set -euo pipefail # One field per line: . # # `wired` is the one an agent actually waits on: the launcher plus the binary, -# derived by `bench` from `.claude/settings.json` rather than hardcoded. It is +# derived by `perf` from `.claude/settings.json` rather than hardcoded. It is # budgeted at the same clig floor, because the floor is about what a human # perceives and the wiring is what they experience. `hook` stays budgeted # alongside it so the launcher's own share stays attributable — a regression in @@ -85,7 +85,7 @@ report() { # redirected nothing into this gate needs to hear that rather than "green". records=$(cat) if [ -z "${records//[[:space:]]/}" ]; then - echo "::error:: bench-assert: stdin is empty — pipe \`mise run bench\` records in (redirect to a file, then read it back; a pipeline would hand this gate's exit status to its last stage)." >&2 + echo "::error:: perf-assert: stdin is empty — pipe \`mise run perf\` records in (redirect to a file, then read it back; a pipeline would hand this gate's exit status to its last stage)." >&2 exit 2 fi @@ -96,7 +96,7 @@ fi # # A line that does not parse is a malformed record, not an absent one, and the # difference matters: absent means "this run measured less than it claims", -# malformed means "something other than bench wrote here". Both are exit 2. +# malformed means "something other than perf wrote here". Both are exit 2. parsed=$(awk ' /^path=/ { id = ""; p95 = "" @@ -119,7 +119,7 @@ parsed=$(awk ' malformed=$(awk -F'\t' '$1=="MALFORMED"{print $2}' <<<"$parsed") if [ -n "$malformed" ]; then - echo "::error:: bench-assert: stdin carries lines that are not \`bench\` records, so the measurement cannot be judged:" >&2 + echo "::error:: perf-assert: stdin carries lines that are not \`perf\` records, so the measurement cannot be judged:" >&2 while IFS= read -r line; do [ -n "$line" ] || continue echo " stdin:$line: not a \`path= p50=… p95=… mean=… runs=…\` record" >&2 @@ -137,9 +137,9 @@ while read -r id _; do missing="$missing $id" done <<<"$BUDGETS" if [ -n "$missing" ]; then - echo "::error:: bench-assert: the records carry no measurement for a budgeted path, so this run judged less than it claims:" >&2 + echo "::error:: perf-assert: the records carry no measurement for a budgeted path, so this run judged less than it claims:" >&2 for id in $missing; do - echo " $id: budgeted here, absent from stdin — did \`mise run bench\` complete?" >&2 + echo " $id: budgeted here, absent from stdin — did \`mise run perf\` complete?" >&2 done exit 2 fi @@ -153,7 +153,7 @@ while read -r id budget; do p95=$(awk -F'\t' -v want="$id" '$1=="OK" && $2==want {print $3; exit}' <<<"$parsed") if awk -v measured="$p95" -v limit="$budget" 'BEGIN { exit !(measured > limit) }'; then if [ "$reported" = 0 ]; then - echo "::error:: bench-assert: a measured invocation path is over its latency budget (see README, Performance):" >&2 + echo "::error:: perf-assert: a measured invocation path is over its latency budget (see README, Performance):" >&2 reported=1 fi report "$id: p95=${p95}ms exceeds the ${budget}ms budget" @@ -164,7 +164,7 @@ done <<<"$BUDGETS" # budget column, whose cell is written as `≤ ms` for a gated path and `—` for # an ungated one, so the two states are distinguishable in the table itself. if [ ! -f "$README" ]; then - echo "::error:: bench-assert: $README not found, so the published budget cannot be held to the enforced one." >&2 + echo "::error:: perf-assert: $README not found, so the published budget cannot be held to the enforced one." >&2 exit 2 fi @@ -194,7 +194,7 @@ while read -r id budget; do row=$(awk -F'\t' -v want="$id" '$1==want {print $2; exit}' <<<"$published") if [ -z "$row" ]; then if [ "$reported" = 0 ]; then - echo "::error:: bench-assert: the enforced budget and the published one disagree (non-negotiable rule 2 — the rule ships with its mechanism):" >&2 + echo "::error:: perf-assert: the enforced budget and the published one disagree (non-negotiable rule 2 — the rule ships with its mechanism):" >&2 reported=1 fi report "$id: budgeted at ${budget}ms here, but $README's Performance table has no row for it" @@ -205,7 +205,7 @@ while read -r id budget; do value=$(printf '%s' "$row" | tr -cd '0-9.') if [ "$value" != "$budget" ]; then if [ "$reported" = 0 ]; then - echo "::error:: bench-assert: the enforced budget and the published one disagree (non-negotiable rule 2 — the rule ships with its mechanism):" >&2 + echo "::error:: perf-assert: the enforced budget and the published one disagree (non-negotiable rule 2 — the rule ships with its mechanism):" >&2 reported=1 fi report "$id: enforced ${budget}ms, $README publishes '$row'" @@ -213,4 +213,4 @@ while read -r id budget; do done <<<"$BUDGETS" [ "$fail" = 0 ] || exit 1 -echo "bench-assert: every budgeted path is inside its budget, and $README publishes the budget this gate enforces" +echo "perf-assert: every budgeted path is inside its budget, and $README publishes the budget this gate enforces" diff --git a/mise-tasks/perf-compare b/mise-tasks/perf-compare new file mode 100755 index 000000000..1b0064749 --- /dev/null +++ b/mise-tasks/perf-compare @@ -0,0 +1,147 @@ +#!/usr/bin/env bash +#MISE description="Gate: this branch has not made an invocation path measurably slower than its merge base (reads a paired measurement on stdin)" +# +# CLOUD-172, and the half that reaches someone who can still act. A recorded +# series nobody opens changes no behaviour; a failing check on the pull request +# that caused the regression arrives while the author still has the context to +# fix it. +# +# A PURE FUNCTION OF STDIN, like `perf-assert`, `graph-check` and `claim-check` +# before it: agents fetch, gates decide. `mise run perf-pair` is the driver that +# produces the paired measurement; this decides over it. That split is what lets +# `tests/perf-compare.bats` exercise every verdict on every commit in +# milliseconds, with no build, no hyperfine and no worktree. +# +# THE VERDICT IS A RATIO, NEVER AN ABSOLUTE. This is the whole design, and it is +# what replaces the instruction counts CLOUD-172 asked for. `mise registry +# valgrind` reports "tool not found in registry", so callgrind cannot be pinned +# and `batten.toml`'s `no-source-built-tool` rule forbids compiling it — wall +# clock is the only metric available. Wall clock on a shared runner is exactly +# the metric the issue warns "both hides real regressions and invents fake +# ones", and it is right about the ABSOLUTE number: a runner contending with +# other tenants can double a 3ms measurement for reasons no commit caused. +# +# What it cannot do is double ONE ARM of a pair measured back to back on that +# same runner in that same second. Machine-level variance is common-mode across +# the pair and divides out. So the driver builds both binaries and hands this +# gate two measurements taken together, and the question asked here is "did HEAD +# get slower THAN ITS OWN BASE, on one machine, at one moment" — which is a +# property of the change, and the only latency question a shared runner can +# answer honestly. +# +# THE THRESHOLD IS MEASURED, NOT ASSERTED — CLOUD-74's move, applied to this +# gate's own constant. The number below is not a guess about what feels +# reasonable; it is derived from the noise floor of a null comparison, the same +# binary measured as both arms. See NOISE_FLOOR beside it. +# +# Exit 0 within threshold / 1 a path regressed past it / 2 could not look. +set -euo pipefail + +# THE THRESHOLD, and the measurement it comes from. +# +# NULL COMPARISON, measured 2026-08-11 on a 4-core x86_64 container: the +# identical binary as both arms, 100 runs each after 10 warmups, ten repeats +# across the three paths — 30 ratios of a comparison that by construction +# measures nothing. They spread **0.966 to 1.102**. So a 10% swing is what this +# experiment produces on a quiet machine when NOTHING changed, and a threshold +# anywhere near 1.10 would be a coin flip rather than a gate. +# +# 1.30 sits clear of that measured maximum with room for a busier runner. The +# asymmetry is deliberate: a false failure costs a confused author and, repeated, +# gets the gate switched off, while a regression small enough to slip under 1.30 +# is caught by the trunk series instead. A gate people trust that fires late +# beats a gate that fires early and gets deleted. +# +# It is also the right ORDER of sensitivity for what this gate is actually for. +# The regressions worth blocking are not drift — they are a commit that made the +# hook path do real work: a config read, a git invocation, a filesystem walk. +# Against a 2.6ms base those are multiples, not percentages. +# +# Re-measure with `mise run perf-pair --null`; that flag exists so the floor +# stays checkable rather than historical. +REGRESSION_RATIO="${BENCH_REGRESSION_RATIO:-1.30}" +NOISE_FLOOR="1.102" # measured null maximum, n=30; reported, never compared against + +fail=0 +reported=0 +report() { + if [ "$reported" = 0 ]; then + echo "::error:: perf-compare: an invocation path is measurably slower than the merge base (threshold ${REGRESSION_RATIO}x, measured noise floor ${NOISE_FLOOR}x):" >&2 + reported=1 + fi + echo " $1" >&2 + fail=1 +} + +# The paired records. Two blocks, distinguished by an `arm=` field: `base` for +# the merge-base binary, `head` for this branch's. Anything else is unreadable. +pairs="$(cat)" +if [ -z "${pairs//[[:space:]]/}" ]; then + echo "::error:: perf-compare: stdin is empty — redirect \`mise run perf-pair\` to a file and read it back. No verdict." >&2 + exit 2 +fi + +# Literal awk patterns rather than a `-v` regex (`mise run awk-regex-check`). +# A line that is neither a paired record nor blank is noise in a stream that +# carries verdict input, and noise is "could not look", never a pass. +parsed="$(awk ' + /^arm=/ { + arm = ""; id = ""; p50 = "" + for (i = 1; i <= NF; i++) { + split($i, kv, "=") + if (kv[1] == "arm") arm = kv[2] + if (kv[1] == "path") id = kv[2] + if (kv[1] == "p50") p50 = kv[2] + } + if ((arm != "base" && arm != "head") || id == "" || p50 == "" || p50 + 0 != p50 || p50 == "0") { + print "MALFORMED\t" NR + next + } + print "OK\t" arm "\t" id "\t" p50 + next + } + /[^[:space:]]/ { print "MALFORMED\t" NR } +' <<<"$pairs")" + +malformed="$(awk -F'\t' '$1=="MALFORMED"{print $2}' <<<"$parsed")" +if [ -n "$malformed" ]; then + echo "::error:: perf-compare: stdin carries lines that are not paired records, so no comparison is possible:" >&2 + while IFS= read -r line; do + [ -n "$line" ] || continue + echo " stdin:$line: not an \`arm= path= p50=… p95=… mean=… runs=…\` record" >&2 + done <<<"$malformed" + exit 2 +fi + +# p50, not p95, and the reason is the pairing. p95 is the right statistic for an +# absolute budget (`perf-assert`), because a hook's worst case is what an agent +# feels. It is the wrong one for a ratio: the tail is where a runner's +# contention lands, so a p95 ratio is the noisiest number available, while p50 +# is the most stable estimate of what the binary itself costs. The budget gate +# still holds the tail; this one holds the shape. +heads="$(awk -F'\t' '$1=="OK" && $2=="head" {print $3"\t"$4}' <<<"$parsed")" +if [ -z "$heads" ]; then + echo "::error:: perf-compare: no \`head\` measurements on stdin — the driver produced no arm to judge. No verdict." >&2 + exit 2 +fi + +while IFS=$'\t' read -r id head_p50; do + [ -n "$id" ] || continue + base_p50="$(awk -F'\t' -v want="$id" '$1=="OK" && $2=="base" && $3==want {print $4; exit}' <<<"$parsed")" + # An unpaired path is could-not-look, never a pass: it means the base build + # failed to measure that path, and reporting green over a comparison that did + # not happen is the partial-coverage false green this repo keeps re-meeting. + if [ -z "$base_p50" ]; then + echo "::error:: perf-compare: path '$id' was measured on head but not on base, so it was never compared. No verdict." >&2 + exit 2 + fi + # awk for the arithmetic: the values are fractional milliseconds and `[ ]` + # compares integers only. + ratio="$(awk -v h="$head_p50" -v b="$base_p50" 'BEGIN { printf "%.3f", h / b }')" + if awk -v r="$ratio" -v limit="$REGRESSION_RATIO" 'BEGIN { exit !(r > limit) }'; then + report "$id: base p50=${base_p50}ms -> head p50=${head_p50}ms (${ratio}x)" + fi +done <<<"$heads" + +[ "$fail" = 0 ] || exit 1 +echo "perf-compare: every measured path is within ${REGRESSION_RATIO}x of the merge base" diff --git a/mise-tasks/perf-gate b/mise-tasks/perf-gate new file mode 100755 index 000000000..51bd403be --- /dev/null +++ b/mise-tasks/perf-gate @@ -0,0 +1,45 @@ +#!/usr/bin/env bash +#MISE description="Gate: measure this branch against its merge base and refuse a regression — the one call `verify` and CI make" +# +# CLOUD-172. `perf-pair` measures, `perf-compare` decides, and this is the one +# name that runs the pair — so a caller wires in ONE task and cannot wire in +# half of it. `mise run ci-local-parity` requires every task a pull_request +# workflow runs to be one `verify` runs, and a single name is what keeps that +# correspondence readable. +# +# REDIRECTED THROUGH A FILE, NEVER PIPED. `perf-pair | perf-compare` would hand +# this task's exit status to the gate alone, so a measurement that failed +# outright would reach `perf-compare` as empty stdin and be reported as its exit +# 2 — the right code for the wrong reason, and one step from a pipeline whose +# producer's failure vanishes entirely. `mise run run-shape-guard` refuses that +# shape in an agent's hands; it is no more correct here. +# +# EXIT CODES PASS THROUGH, and that matters to the caller. `perf-pair` answers 2 +# for "could not look" — an unresolvable merge base, a build that failed, a +# missing instrument — and `perf-compare` answers 1 only for a real regression. +# Flattening them would make a shallow clone indistinguishable from a branch +# that made the hook slower, which is the difference `verify` needs in order to +# tell "fix your change" from "fix your checkout". +set -euo pipefail + +pair_file="$(mktemp)" +trap 'rm -f "$pair_file"' EXIT + +rc=0 +mise run perf-pair >"$pair_file" || rc=$? +if [ "$rc" != 0 ]; then + echo "::error:: perf-gate: the paired measurement did not complete (exit $rc), so no comparison was made." >&2 + exit "$rc" +fi + +# The skip path prints one human line and no records. That is a pass, not an +# empty measurement: `perf-pair` has already established that the binary cannot +# have changed, and handing an empty stream to `perf-compare` would turn a +# sound skip into a "could not look". +if ! grep -q '^arm=' "$pair_file"; then + cat "$pair_file" + echo "perf-gate: nothing to compare — the binary is unchanged on this branch" + exit 0 +fi + +mise run perf-compare <"$pair_file" diff --git a/mise-tasks/perf-pair b/mise-tasks/perf-pair new file mode 100755 index 000000000..f57f7f2b2 --- /dev/null +++ b/mise-tasks/perf-pair @@ -0,0 +1,203 @@ +#!/usr/bin/env bash +#MISE description="Measure this branch and its merge base back to back on one machine, and print both arms as paired records" +# +# CLOUD-172. The driver behind `perf-compare`: it produces the two measurements, +# the gate decides over them. Same split as `perf`/`perf-assert`, for the same +# reason — the decision has to be exercisable without a build, and the +# measurement has to happen somewhere a measurement means something. +# +# WHY BOTH ARMS IN ONE RUN. Wall clock is the only metric available (`mise +# registry valgrind` -> "tool not found in registry", and `no-source-built-tool` +# forbids compiling one), and a shared runner's absolute wall clock is exactly +# the number CLOUD-172 warns "both hides real regressions and invents fake +# ones". The fix is not a better clock, it is a better experiment: build BOTH +# binaries and measure them on the same machine within the same few seconds, so +# whatever that machine is doing to one arm it is doing to the other. The +# comparison then divides the noise out. Measuring the base on some other run, +# or reading it from the series, would put a machine change inside the number +# and is precisely what this avoids. +# +# THE EARLY EXIT IS A CORRECTNESS ARGUMENT, NOT AN ECONOMY. `verify` is the +# inner loop of every landing lap, and `mem:workflow/agent-fanout` measures that +# shortening `verify` buys more parallelism than adding sessions — two release +# builds on every lap would be a throughput tax on the whole fleet. But the +# reason it is safe to skip is not the cost: a commit that changes no Rust +# source, no manifest and no lockfile cannot change the binary, and a binary +# that is byte-identical cannot have got slower. So the skip is sound where it +# applies and absent where it does not, which is the shape CLOUD-224 gives the +# gate steps one level down. Task-layer, workflow and documentation changes — +# most of this repository's traffic — pay a `git diff --name-only`. +# +# `--null` measures HEAD against ITSELF: a comparison of a binary with its own +# copy, whose ratio is by construction 1.0 plus pure noise. That is how +# `perf-compare`'s threshold was derived, and the flag exists so the noise floor +# stays a thing anyone can re-measure rather than a number in a comment. +set -euo pipefail + +cd "$(git rev-parse --show-toplevel)" + +readonly BASE_REF="${BENCH_BASE_REF:-origin/main}" +readonly RUNS="${BENCH_RUNS:-100}" +readonly WARMUP="${BENCH_WARMUP:-10}" + +null=0 +if [ "${1:-}" = "--null" ]; then + null=1 +fi + +for tool in hyperfine jq; do + if ! command -v "$tool" >/dev/null 2>&1; then + echo "::error:: perf-pair: $tool is not installed — run \`mise install\`; it is pinned in mise.toml." >&2 + exit 2 + fi +done + +# The comparison's base. Exit 2 rather than 1 when it cannot be resolved: an +# unresolvable base is "the gate could not look", a property of the checkout +# (a shallow clone, a missing fetch), not a verdict about the branch. +if ! base_sha="$(git merge-base HEAD "$BASE_REF" 2>/dev/null)" || [ -z "$base_sha" ]; then + echo "::error:: perf-pair: no merge base between HEAD and $BASE_REF — fetch it first (\`git fetch origin main\`). No measurement." >&2 + exit 2 +fi + +head_sha="$(git rev-parse HEAD)" + +# A branch that has not committed anything yet IS its merge base, so there is no +# change to have regressed and the two arms would be the same bytes. Its own +# exit rather than a case of the skip below, because the diff is empty for a +# different reason — nothing was authored, not "nothing that matters". +if [ "$null" = 0 ] && [ "$base_sha" = "$head_sha" ]; then + echo "perf-pair: HEAD is its own merge base (${head_sha:0:8}) — no change to compare. Nothing measured." + exit 0 +fi + +# THE SKIP. Anything that can end up compiled into the binary: the crate +# sources, either manifest layer, and the lockfile that pins what they build +# against. A path outside that set cannot change a single byte of the artifact. +if [ "$null" = 0 ]; then + if ! changed="$(git diff --name-only "$base_sha" HEAD)"; then + echo "::error:: perf-pair: could not diff $BASE_REF..HEAD, so the skip could not be decided. No measurement." >&2 + exit 2 + fi + # Literal grep patterns, and `-c` rather than `-q`: an early-exiting `grep -q` + # under `pipefail` can report failure ON A MATCH when the producer is still + # writing (`mise run pipefail-grep-check`). + touching="$(grep -cE '^(crates/|Cargo\.lock$|Cargo\.toml$)' <<<"$changed" || true)" + if [ "$touching" = 0 ]; then + echo "perf-pair: no change to crates/, Cargo.toml or Cargo.lock between ${base_sha:0:8} and ${head_sha:0:8} — the binary is unchanged, so its latency is too. Nothing measured." + exit 0 + fi +fi + +out_dir="${BENCH_OUT_DIR:-target/perf}/pair" +rm -rf "$out_dir" +mkdir -p "$out_dir" +OUT_DIR="$(cd "$out_dir" && pwd)" +readonly OUT_DIR + +# The head binary, in the ordinary target dir so it shares the cache every other +# task warms. +if ! cargo build --quiet --release -p batten; then + echo "::error:: perf-pair: the head release build failed, so there is nothing to compare. No measurement." >&2 + exit 2 +fi +head_bin="$PWD/target/release/batten" + +if [ "$null" = 1 ]; then + # The null experiment: the same bytes as both arms. Copied rather than aliased + # so hyperfine sees two distinct commands and cannot short-circuit anything, + # and so the two arms differ in exactly nothing. + base_bin="$OUT_DIR/batten-null" + cp "$head_bin" "$base_bin" + base_sha="$head_sha" +else + # The base binary, built from a detached worktree at the merge base. Its own + # CARGO_TARGET_DIR: sharing the main one would make the two builds evict each + # other's artifacts on every lap, and would race the target-dir lock against + # whatever else `verify` is running. + worktree="$OUT_DIR/base-tree" + if ! git worktree add -q --detach "$worktree" "$base_sha"; then + echo "::error:: perf-pair: could not create a worktree at ${base_sha:0:8}. No measurement." >&2 + exit 2 + fi + # Removed however this exits — a leaked worktree makes the NEXT run fail on a + # path that already exists, which reads as a broken gate rather than as + # leftover state. + trap 'git worktree remove --force "$worktree" >/dev/null 2>&1 || true' EXIT + if ! (cd "$worktree" && CARGO_TARGET_DIR="$OUT_DIR/base-target" cargo build --quiet --release -p batten); then + echo "::error:: perf-pair: the base release build failed at ${base_sha:0:8}, so there is nothing to compare against. No measurement." >&2 + exit 2 + fi + base_bin="$OUT_DIR/base-target/release/batten" +fi + +if [ ! -x "$base_bin" ] || [ ! -x "$head_bin" ]; then + echo "::error:: perf-pair: one of the two binaries is missing after a successful build — refusing to measure something else." >&2 + exit 2 +fi + +# The check path's fixture, materialised exactly as `perf` materialises it, so +# the two tasks measure the same input and a difference between them is never +# the corpus. +readonly FIXTURE_REPO="crates/batten/tests/fixtures/repos/forbid-clean" +readonly FIXTURE_HOOK="$PWD/crates/batten/tests/fixtures/hooks/claude-code.json" +check_repo="$OUT_DIR/check-repo" +mkdir -p "$check_repo" +cp "$FIXTURE_REPO/batten.toml.in" "$check_repo/batten.toml" +cp "$FIXTURE_REPO/lib.rs.in" "$check_repo/lib.rs" + +# One hyperfine invocation per path carrying BOTH commands, so the two arms are +# measured back to back rather than in separate runs. `results[0]` is base and +# `results[1]` is head, in the order they are passed. +pair() { + local id="$1" dir="$2" args="$3" + local json="$OUT_DIR/$id.json" + local -a flags=( + --warmup "$WARMUP" + --runs "$RUNS" + --shell=none + --export-json "$json" + --style none + ) + if [ "$id" = hook ]; then + flags+=(--input "$FIXTURE_HOOK") + fi + + if ! (cd "$dir" && hyperfine "${flags[@]}" "$base_bin $args" "$head_bin $args" >/dev/null 2>"$OUT_DIR/$id.err"); then + echo "::error:: perf-pair: measuring the $id pair failed — see $OUT_DIR/$id.err. No measurement." >&2 + exit 2 + fi + + # Same units and same rounding as `perf`, plus the `arm=` field that makes a + # record half of a pair. p50 from the sorted times for the same reason. + if ! jq -er --arg id "$id" ' + ["base", "head"] as $arms + | range(0; 2) as $i + | .results[$i] as $r + | ($r.times | sort) as $t + | ($t | length) as $n + | (($n - 1) * 0.5 | floor) as $i50 + | (($n - 1) * 0.95 | ceil) as $i95 + | "arm=\($arms[$i]) path=\($id) p50=\($t[$i50] * 1000 | . * 100 | round / 100) p95=\($t[$i95] * 1000 | . * 100 | round / 100) mean=\($r.mean * 1000 | . * 100 | round / 100) runs=\($n)" + ' "$json"; then + echo "::error:: perf-pair: could not read the $id pair out of $json. No measurement." >&2 + exit 2 + fi +} + +# EVERY ARM RUNS IN THE FIXTURE REPO, never in this checkout — and that is a +# correctness requirement, not tidiness. The two arms are different binaries: the +# base one predates whatever this branch changed, and a `batten.toml` key added +# by HEAD is a key the BASE binary rejects at load. Measured here: with the arms +# run from the repo root, a head that added `[worktree]` to the committed config +# made the base binary exit 1 on "unknown field `worktree`", hyperfine abort on +# its first warmup, and the whole gate answer 2 — a could-not-look produced by +# the gate's own setup, on exactly the class of change it exists to judge. +# +# The fixture's config is pinned and minimal, so both binaries can load it and +# the comparison is between the BINARIES rather than between two configs. It +# also makes the number reproducible: measuring against this repository's live +# config would move the baseline every time the config changed. +pair noop "$check_repo" "--help" +pair check "$check_repo" "check" +pair hook "$check_repo" "hook --harness claude-code" diff --git a/mise-tasks/perf-record b/mise-tasks/perf-record new file mode 100755 index 000000000..f6c6fd926 --- /dev/null +++ b/mise-tasks/perf-record @@ -0,0 +1,103 @@ +#!/usr/bin/env bash +#MISE description="Append a measurement to the trunk's invocation-cost series in git notes (reads `perf` records on stdin; refuses to run off main)" +# +# CLOUD-172. CLOUD-207 published one number, and a number measured once decays +# in silence: nobody notices 3ms becoming 40ms, they notice months later that +# the hook got uninstalled. This is the series that makes the decay visible, and +# a regression attributable to a SHA rather than to a quarter. +# +# GIT NOTES, NOT AN EXTERNAL SERVICE. `refs/notes/perf`, keyed to the commit +# measured. Three properties follow from that choice and none of them are +# available from a dashboard: the history travels with the clone, a reader can +# ask `git log --notes=perf` and get the series interleaved with the commits +# that caused it, and nothing has to be re-run to attribute a change. It also +# costs no credential and no vendor. +# +# MAIN ONLY, and this is a refusal rather than a convention. A branch's numbers +# are not the trunk's: a branch may be mid-rebase, carrying unlanded work, or +# built from a different base entirely, and a series mixing the two cannot be +# read at all — the reader has no way to tell a step change from a branch +# switch. So a non-main HEAD is exit 1, loudly, and nothing is written. +# +# THE LABEL IS LOAD-BEARING. CLOUD-172 asked for instruction counts as the +# primary metric, with wall clock as the fallback "where valgrind is +# unavailable". Measured here: `mise registry valgrind` reports "tool not found +# in registry", so valgrind cannot be pinned in `mise.toml` at all, and +# `batten.toml`'s `no-source-built-tool` rule refuses to compile it. Wall clock +# is therefore the metric everywhere, not situationally — which makes saying so +# in the record mandatory, since a later reader comparing a wall-clock series +# against an instruction-count one would read the instrument change as a +# regression. `metric=` is that guard. +# +# THE RECORDING TRIGGER, decided as the Ready block requires. NOT a +# push-to-`main` workflow: AGENTS.md's workflow contract forbids one, and the +# reason holds here rather than merely applying — `main` only advances by +# fast-forward to a SHA CI already judged, so a push trigger buys a runner per +# merge for a measurement that a schedule can take just as well. The trigger is +# the daily schedule in `.github/workflows/perf.yml`, which already samples +# `main`. What that costs is resolution: the series is per-sample, not +# per-commit. That trade is affordable precisely because it is not the only +# mechanism — `perf-compare` catches a regression on the PULL REQUEST that +# caused it, at the commit that caused it, which is where attribution actually +# matters. The series carries the trend. (Recorded in .claude/rules/toolchain.md +# too; AGENTS.md is at its budgeted line ceiling and already carries the rule +# this decision honours.) +set -euo pipefail + +cd "$(git rev-parse --show-toplevel)" + +# The ref the series lives on. Its own ref, not `refs/notes/commits`: the +# default notes ref is what `git notes` writes with no argument and what a +# contributor is most likely to have local edits on, and a series is not a +# comment. +readonly NOTES_REF="${BENCH_NOTES_REF:-refs/notes/perf}" + +# The branch a measurement is allowed to describe. An argument so the bats suite +# can drive a fixture repository whose trunk is named something else; the real +# value is this repo's trunk. +readonly TRUNK="${BENCH_TRUNK:-main}" + +# What produced the numbers. Emitted into the record so the series can never be +# read as an instruction count — see the header. +readonly METRIC="${BENCH_METRIC:-wall-clock}" + +branch="$(git rev-parse --abbrev-ref HEAD)" +if [ "$branch" != "$TRUNK" ]; then + echo "::error:: perf-record: HEAD is on '$branch', not '$TRUNK' — a branch's numbers are not the trunk's, and a series mixing them cannot be read. Nothing written." >&2 + exit 1 +fi + +commit="$(git rev-parse HEAD)" + +# Exit 2 for "could not look", matching `perf-assert` and the `lock-complete` +# doctrine: a caller that redirected nothing here needs to hear that, not a +# silent empty note that later reads as a measurement of zero. +records="$(cat)" +if [ -z "${records//[[:space:]]/}" ]; then + echo "::error:: perf-record: stdin is empty — redirect \`mise run perf\` to a file and read it back. Nothing written." >&2 + exit 2 +fi + +# Only records. A stray line would enter the series as a datum nothing can +# parse, and the series is read by machine (`perf-compare`) as well as by eye. +# Literal awk patterns, never a `-v` regex — `mise run awk-regex-check` refuses +# the latter for having implementation-defined escape handling. +if awk '/^path=/ { next } /[^[:space:]]/ { bad = 1 } END { exit !bad }' <<<"$records"; then + echo "::error:: perf-record: stdin carries lines that are not \`perf\` records, so the series would take a datum nothing can parse. Nothing written." >&2 + exit 2 +fi + +# One note per commit, and appending rather than replacing: a commit sampled +# twice (a re-dispatched schedule, a manual run) keeps both readings instead of +# the second silently overwriting the first. Two readings of one commit is +# itself the noise measurement a threshold wants. +note="$(printf 'metric=%s runner=%s\n%s' "$METRIC" "${BENCH_RUNNER:-$(uname -m)-$(nproc)core}" "$records")" + +if ! git notes --ref="$NOTES_REF" append -m "$note" "$commit"; then + echo "::error:: perf-record: could not append the note to $NOTES_REF for $commit." >&2 + exit 1 +fi + +# A pointer, never the payload (non-negotiable rule 4): the ref and the commit, +# so a reader knows where to look. The numbers are in the note. +echo "perf-record: appended a $METRIC measurement to $NOTES_REF for ${commit:0:8}" diff --git a/mise.toml b/mise.toml index 71bf88aaa..fdc5761b2 100644 --- a/mise.toml +++ b/mise.toml @@ -14,7 +14,7 @@ hk = "1.54.0" # git-hook runner ( "aqua:anchore/syft" = "1.42" # SBOM of the tagged source: cargo-deny judges that tree, this inventories it (mise-tasks/sbom) "aqua:release-plz/release-plz" = "0.3" # conventional-commit-driven semver + changelog "aqua:jqlang/jq" = "1.7" # JSON for shell tasks — reads the PreToolUse payload in `gh-guard` -# The measurement instrument for `mise run bench` (CLOUD-207). Batten runs as a +# The measurement instrument for `mise run perf` (CLOUD-207). Batten runs as a # PreToolUse hook, so its startup cost is paid on every mediated tool call, and # that number is only defensible if the thing producing it is fixed: a change in # how the sampler warms, discards outliers, or computes a quantile enters the @@ -25,7 +25,7 @@ hk = "1.54.0" # git-hook runner ( # tool compiled here, and hyperfine ships prebuilt linux-arm64/x64 and macOS # assets, so `mise run lock-complete` can hold every platform this repo installs # on. Deliberately NOT in ci.yml's install lists: nothing on the landing path -# measures (see .github/workflows/bench.yml for why measurement is scheduled). +# measures (see .github/workflows/perf.yml for why measurement is scheduled). "aqua:sharkdp/hyperfine" = "1.20.0" # NOTE: the MSRV gate (`mise run msrv`) deliberately pins NO tool. cargo-msrv # would be the obvious choice and cannot be used: upstream ships no @@ -147,7 +147,7 @@ CONVENTIONAL_RE = '^(build|chore|ci|docs|feat|fix|perf|refactor|revert|style|tes # Hand-maintained until CLOUD-54's derived `[ci].required_checks` lands, and NOT # on the honour system meanwhile: `mise run ci-local-parity` fails if a name here # matches no job, or if a `pull_request` job is missing from here. -CI_REQUIRED_CHECKS = "ci,cross,commit-lint,zizmor,darwin-link (aarch64-apple-darwin),msrv,semver,final,action (ubuntu-latest),action (macos-latest),action (windows-latest),action-violation,action-deny,action-usage,action-internal (ubuntu-latest),action-internal (macos-latest),action-final" +CI_REQUIRED_CHECKS = "ci,cross,commit-lint,zizmor,darwin-link (aarch64-apple-darwin),msrv,semver,perf,final,action (ubuntu-latest),action (macos-latest),action (windows-latest),action-violation,action-deny,action-usage,action-internal (ubuntu-latest),action-internal (macos-latest),action-final" # The subset of that roster for which ABSENCE — no check-run at all, as opposed # to one that graded `skipped` — is a legitimate reading rather than a name that # has not registered yet. Exactly two WORKFLOWS earn it, and each earns it twice @@ -580,7 +580,7 @@ if ! cargo deny check; then exit 1; fi [tasks."build:release"] description = "Build the release binary the PreToolUse hook executes" # The one place the hook's binary is produced, so `.claude/hooks/batten-hook.sh`, -# `session-start.sh` and `mise-tasks/bench` name a task rather than three copies +# `session-start.sh` and `mise-tasks/perf` name a task rather than three copies # of a cargo line that could drift in their flags. # # Release, not debug, and not `cargo run`: this runs on EVERY mediated tool call. @@ -992,6 +992,25 @@ if [ "$sonar_rc" != 0 ] && [ "$sonar_rc" != 3 ]; then echo "::error:: verify: sonar-gate failed — see its message. No receipt written." >&2 exit 1 fi +# The latency regression gate (CLOUD-172). `perf-assert` holds the published +# number to an absolute ceiling; this asks the other question — did THIS branch +# make an invocation path slower than its own merge base — which a ceiling +# cannot answer and a shared runner's absolute wall clock cannot answer honestly. +# +# It costs almost nothing on the common change: `perf-gate` exits early and +# clean when the diff against the merge base touches no crate source, manifest +# or lockfile, because a binary that cannot have changed cannot have got slower. +# The two release builds are paid only by a commit that could actually regress +# the hook path — the commit worth paying for. +# +# Here rather than as a CI job, deliberately: `ready-guard` will not let a PR be +# readied without a verify receipt for this exact HEAD, so a regression blocks +# the PR before it can spend a runner at all. That is strictly earlier and +# cheaper than a check that reds after the matrix has been paid for. +if ! mise run perf-gate; then + echo "::error:: verify: perf-gate failed — this branch made an invocation path measurably slower than its merge base, or the measurement could not run. No receipt written." >&2 + exit 1 +fi # The receipt write is the binary's job (CLOUD-203): `batten receipt record` # keys the claim to the exact commit verified — an amend or a rebase produces a # new HEAD and therefore no receipt, which is the point (see ready-guard) — and diff --git a/schema/batten.local.schema.json b/schema/batten.local.schema.json index 7a3c7a168..76a796723 100644 --- a/schema/batten.local.schema.json +++ b/schema/batten.local.schema.json @@ -403,6 +403,11 @@ "description": "A **model-validated** rule (CLOUD-56): hand the row's `criteria` and the\nadmitted classes of its matched files to the command in `[judge].run`,\nand read that command's exit code.\n\nThe kind exists for the predicates no static shape can express — \"this\ntest asserts behaviour, not a tautology\" — and it is the one kind whose\noutcome is **advisory-only and structurally unable to block** (house\nstyle §0.3; the 2026-08-07 evidence base measured model judges at AUROC\n≤0.65 on false-success detection, so a judge verdict may inform and never\ngate).\n\n\"Structurally\" is literal, and it is why this variant looks unfinished\nnext to the others: a judge outcome **never becomes a [`Finding`]**.\n`run_rule` cannot produce one — the row is `allow`, which that walker\nalready skips — so [`any_blocking`] and `--fail-on-warning` have nothing\nto see. Blocking is not forbidden here, it is unrepresentable.\n\nDistinct from [`RuleKind::Command`], which also runs something and reads\nan exit code: that one is an *exit-code predicate* whose command is a\ngate, and its findings deny. This one's command consults a model, and its\nfindings advise. The gate/judge line (CLOUD-93) is exactly this pair.", "type": "string", "const": "judge" + }, + { + "description": "A **credential in the tree** (CLOUD-59): run the pinned secret scanner\nover the matched paths and turn each match into a pointer.\n\nThe kind exists because [`RuleKind::Forbid`] bans literals named in\nadvance, and a credential is the banned shape nobody can enumerate — so a\ncommitted secret passes every other gate this engine has. Detection is\nadopted prior art; what this kind owns is **containment**, in\n[`crate::secrets`].\n\nDistinct from [`RuleKind::Command`], which could run the same binary and\ncould not express this: a command rule's contract is the exit code alone,\nwith both child streams nulled, so it yields one batch verdict per glob —\nno per-secret `path:line`, no per-secret identity, and nothing to key.\nReading the scanner's output is precisely what makes this kind different,\nand precisely what makes it dangerous: the scanner prints the byte it\nmatched. Every span is opaque from the parse boundary onward\n([`crate::identity::SecretSpan`]), and [`Finding`] has no field one could\noccupy, so pointer-only output is structural rather than a property of\nthe renderer.", + "type": "string", + "const": "secrets" } ] }, diff --git a/schema/batten.schema.json b/schema/batten.schema.json index 4f29a03a4..d804707fa 100644 --- a/schema/batten.schema.json +++ b/schema/batten.schema.json @@ -1037,6 +1037,11 @@ "description": "A **model-validated** rule (CLOUD-56): hand the row's `criteria` and the\nadmitted classes of its matched files to the command in `[judge].run`,\nand read that command's exit code.\n\nThe kind exists for the predicates no static shape can express — \"this\ntest asserts behaviour, not a tautology\" — and it is the one kind whose\noutcome is **advisory-only and structurally unable to block** (house\nstyle §0.3; the 2026-08-07 evidence base measured model judges at AUROC\n≤0.65 on false-success detection, so a judge verdict may inform and never\ngate).\n\n\"Structurally\" is literal, and it is why this variant looks unfinished\nnext to the others: a judge outcome **never becomes a [`Finding`]**.\n`run_rule` cannot produce one — the row is `allow`, which that walker\nalready skips — so [`any_blocking`] and `--fail-on-warning` have nothing\nto see. Blocking is not forbidden here, it is unrepresentable.\n\nDistinct from [`RuleKind::Command`], which also runs something and reads\nan exit code: that one is an *exit-code predicate* whose command is a\ngate, and its findings deny. This one's command consults a model, and its\nfindings advise. The gate/judge line (CLOUD-93) is exactly this pair.", "type": "string", "const": "judge" + }, + { + "description": "A **credential in the tree** (CLOUD-59): run the pinned secret scanner\nover the matched paths and turn each match into a pointer.\n\nThe kind exists because [`RuleKind::Forbid`] bans literals named in\nadvance, and a credential is the banned shape nobody can enumerate — so a\ncommitted secret passes every other gate this engine has. Detection is\nadopted prior art; what this kind owns is **containment**, in\n[`crate::secrets`].\n\nDistinct from [`RuleKind::Command`], which could run the same binary and\ncould not express this: a command rule's contract is the exit code alone,\nwith both child streams nulled, so it yields one batch verdict per glob —\nno per-secret `path:line`, no per-secret identity, and nothing to key.\nReading the scanner's output is precisely what makes this kind different,\nand precisely what makes it dangerous: the scanner prints the byte it\nmatched. Every span is opaque from the parse boundary onward\n([`crate::identity::SecretSpan`]), and [`Finding`] has no field one could\noccupy, so pointer-only output is structural rather than a property of\nthe renderer.", + "type": "string", + "const": "secrets" } ] }, diff --git a/tests/checks-green.bats b/tests/checks-green.bats index e5be78dc6..c86e49cab 100644 --- a/tests/checks-green.bats +++ b/tests/checks-green.bats @@ -31,16 +31,29 @@ runs() { printf '%s\n' "$@"; } # case supplies for the same name is NEWER and supersedes it under CLOUD-436's # latest-per-name rule. That is what lets the supersession cases below compose # with this helper without their own subject being decided here. +# DERIVED FROM THE ROSTER, never hand-listed. The set above was written out by +# name, which made it a SECOND copy of `$CI_REQUIRED_CHECKS` — and the header +# says as much, describing it as the roster minus the absent-ok names. Adding +# `perf` (CLOUD-172) proved the copy drifts: six cases here failed at once, and +# the same hand-listing in `tests/ci-wait.bats` did worse, hanging that suite +# instead of failing it, because a poll cannot tell a missing name from a run +# still going. Derived, the fixture is correct for whatever the roster says +# today, and the count in the comment above can never be wrong either. mandatory_green() { - runs \ - "completed success ci 2026-08-12T00:00:00Z 1" \ - "completed success cross 2026-08-12T00:00:00Z 2" \ - "completed success commit-lint 2026-08-12T00:00:00Z 3" \ - "completed success darwin-link (aarch64-apple-darwin) 2026-08-12T00:00:00Z 4" \ - "completed success msrv 2026-08-12T00:00:00Z 5" \ - "completed success semver 2026-08-12T00:00:00Z 6" \ - "completed success final 2026-08-12T00:00:00Z 7" \ - "$@" + local rows=() name i=0 + while IFS= read -r name; do + [ -n "$name" ] || continue + i=$((i + 1)) + rows+=("completed success $name 2026-08-12T00:00:00Z $i") + done < <(tr ',' '\n' <<<"${CI_REQUIRED_CHECKS:?the suite runs under mise, which supplies the roster}" | + grep -vxF -f <(tr ',' '\n' <<<"${CI_ABSENT_OK_CHECKS:-}") || true) + # Anti-vacuity: an empty derived set would make every case that composes with + # this helper pass while asserting nothing. + [ "${#rows[@]}" -gt 0 ] || { + echo "mandatory_green derived an empty set from CI_REQUIRED_CHECKS" >&2 + return 1 + } + runs "${rows[@]}" "$@" } @test "a graded, all-success required set is green" { @@ -69,10 +82,17 @@ mandatory_green() { # still listing the other six as expected — the bot was rejected (#280). CHECKS_GREEN_RUNS="$(runs "completed success commit-lint")" run "$GREEN" [ "$status" -eq 3 ] - # Roster order, and the two tolerated names elided from it — the output is a + # Roster order, and the tolerated names elided from it — the output is a # contract (house style §6), so the list is asserted as a whole rather than # name by name. - [[ "$output" == *"with no run at all: ci, cross, darwin-link (aarch64-apple-darwin), msrv, semver, final"* ]] + # + # LITERAL ON PURPOSE, unlike `mandatory_green` above. Deriving this expectation + # would compare the roster against itself and assert nothing about the message; + # what is under test here is the ORDER and the elision, which only a written-out + # list can pin. So a roster change must edit this line — and that is the sensor + # working, not drift: it fails with a diff naming the missing entry, where a + # derived fixture that silently went short is what hung `ci-wait`. + [[ "$output" == *"with no run at all: ci, cross, darwin-link (aarch64-apple-darwin), msrv, semver, perf, final"* ]] [[ "$output" != *"zizmor"* ]] } diff --git a/tests/ci-wait.bats b/tests/ci-wait.bats index c217f1e83..1c5af4dcf 100644 --- a/tests/ci-wait.bats +++ b/tests/ci-wait.bats @@ -52,15 +52,34 @@ response() { # answers "no run at all" over a reading that omits one, and here that does not # read as a failed assertion but as a HANG, since the poll is unbounded and only # the case's own `run_timeout` stops it. +# DERIVED FROM THE ROSTER, never hand-listed, and the difference is a hang. +# +# These cases read the real `$CI_REQUIRED_CHECKS` on purpose (see the header), so +# a hand-written green set is a SECOND copy of the roster that drifts the moment +# a required check is added. Adding `perf` (CLOUD-172) is what proved it: the +# fixture answered green for every name it knew, `ci-wait` correctly kept waiting +# for the one it did not, and the suite HUNG rather than failed — twice, for the +# better part of an hour each time, because a poll with no answer is exactly what +# this task is built to do and it cannot tell "the roster grew" from "the run is +# still going". +# +# Deriving it means the green set is green BY CONSTRUCTION for whatever the +# roster says today. Absent-ok names are excluded deliberately: `zizmor` and the +# `action*` jobs are path-filtered and mint no run at all on most PRs, so a +# fixture that reported them would assert a shape the real API never produces. green_body() { - local rows - rows='{"status":"completed","conclusion":"success","name":"ci"},' - rows+='{"status":"completed","conclusion":"success","name":"cross"},' - rows+='{"status":"completed","conclusion":"success","name":"commit-lint"},' - rows+='{"status":"completed","conclusion":"success","name":"darwin-link (aarch64-apple-darwin)"},' - rows+='{"status":"completed","conclusion":"success","name":"msrv"},' - rows+='{"status":"completed","conclusion":"success","name":"semver"},' - rows+='{"status":"completed","conclusion":"success","name":"final"}' + local rows='' name + while IFS= read -r name; do + [ -n "$name" ] || continue + rows+="${rows:+,}{\"status\":\"completed\",\"conclusion\":\"success\",\"name\":\"$name\"}" + done < <(tr ',' '\n' <<<"${CI_REQUIRED_CHECKS:?the suite runs under mise, which supplies the roster}" | + grep -vxF -f <(tr ',' '\n' <<<"${CI_ABSENT_OK_CHECKS:-}") || true) + # Anti-vacuity: an empty set would make every one of these cases pass by + # asserting nothing, which is the false green the task itself exists to stop. + [ -n "$rows" ] || { + echo "green_body derived an empty check set from CI_REQUIRED_CHECKS" >&2 + return 1 + } if [ "$#" -gt 0 ]; then printf '{"check_runs":[%s,%s]}' "$rows" "$*" else diff --git a/tests/bench-assert.bats b/tests/perf-assert.bats similarity index 97% rename from tests/bench-assert.bats rename to tests/perf-assert.bats index 7e483d3bd..07334043c 100644 --- a/tests/bench-assert.bats +++ b/tests/perf-assert.bats @@ -2,7 +2,7 @@ # The decision half of CLOUD-207's latency mechanism, exercised without ever # measuring anything. # -# That separation is the point of the suite, not an economy: `mise run bench` +# That separation is the point of the suite, not an economy: `mise run perf` # needs a release build, hyperfine and a quiet machine, none of which belong on # the landing path — but the thing that can silently rot is the DECISION, and it # is a pure function of stdin. So this runs in the hk gate on every commit, over @@ -10,7 +10,7 @@ # would look exactly like a published latency number. setup() { - GATE="$BATS_TEST_DIRNAME/../mise-tasks/bench-assert" + GATE="$BATS_TEST_DIRNAME/../mise-tasks/perf-assert" README="$BATS_TEST_TMPDIR/README.md" # The published table this gate holds its own budgets against. Written per # test so a case can make it disagree; the real file is asserted separately. @@ -168,7 +168,7 @@ IN" } @test "a missing wired record is exit 2 — could not look is not a pass" { - # The case that matters most: deleting the wired stage from `bench` must not + # The case that matters most: deleting the wired stage from `perf` must not # read as "the wired path is fine". It is the same shape as the original # defect, one level up. run bash -c "'$GATE' '$README' <<'IN' diff --git a/tests/perf-compare.bats b/tests/perf-compare.bats new file mode 100644 index 000000000..37490e055 --- /dev/null +++ b/tests/perf-compare.bats @@ -0,0 +1,163 @@ +#!/usr/bin/env bats +# The regression gate's decision, exercised without building anything. +# +# Same separation as tests/perf-assert.bats, and the same reason: producing a +# paired measurement needs two release builds, a worktree and hyperfine, none of +# which belong in a pre-commit gate — but the thing that can silently rot is the +# COMPARISON, and it is a pure function of stdin. A regression gate that went +# dead would look exactly like a regression gate. + +setup() { + GATE="$BATS_TEST_DIRNAME/../mise-tasks/perf-compare" +} + +# A full pair, head within noise of base. +green_pair() { + cat <<-'EOF' + arm=base path=noop p50=2.40 p95=2.88 mean=2.44 runs=100 + arm=head path=noop p50=2.44 p95=2.90 mean=2.47 runs=100 + arm=base path=check p50=2.50 p95=3.05 mean=2.55 runs=100 + arm=head path=check p50=2.47 p95=3.01 mean=2.52 runs=100 + arm=base path=hook p50=2.60 p95=3.03 mean=2.64 runs=100 + arm=head path=hook p50=2.62 p95=3.08 mean=2.66 runs=100 + EOF +} + +@test "a pair within the threshold passes, and names the threshold" { + run bash -c "'$GATE' <<'IN' +$(green_pair) +IN" + [ "$status" -eq 0 ] + [[ "$output" == *"within 1.30x of the merge base"* ]] +} + +@test "a path past the threshold is a regression, named with both arms and the ratio" { + run bash -c "'$GATE' <<'IN' +arm=base path=noop p50=2.40 p95=2.88 mean=2.44 runs=100 +arm=head path=noop p50=2.44 p95=2.90 mean=2.47 runs=100 +arm=base path=hook p50=2.60 p95=3.03 mean=2.64 runs=100 +arm=head path=hook p50=9.10 p95=11.2 mean=9.30 runs=100 +IN" + [ "$status" -eq 1 ] + [[ "$output" == *"hook: base p50=2.60ms -> head p50=9.10ms (3.500x)"* ]] + [[ "$output" != *"noop: base"* ]] +} + +# The whole point of the ratio: an arm that is absolutely slow because the +# machine was busy drags BOTH arms, so the comparison still passes. +@test "a slow machine moves both arms and is not a regression" { + run bash -c "'$GATE' <<'IN' +arm=base path=hook p50=26.0 p95=31.0 mean=26.4 runs=100 +arm=head path=hook p50=26.4 p95=31.5 mean=26.8 runs=100 +IN" + [ "$status" -eq 0 ] +} + +# A speed-up is not a regression, however large. +@test "a path that got faster passes" { + run bash -c "'$GATE' <<'IN' +arm=base path=hook p50=9.10 p95=11.2 mean=9.30 runs=100 +arm=head path=hook p50=2.60 p95=3.03 mean=2.64 runs=100 +IN" + [ "$status" -eq 0 ] +} + +@test "the threshold is a boundary, not a suggestion" { + # Exactly 1.30x is inside; a hair past it is not. + run bash -c "'$GATE' <<'IN' +arm=base path=hook p50=2.00 p95=3.03 mean=2.64 runs=100 +arm=head path=hook p50=2.60 p95=3.08 mean=2.66 runs=100 +IN" + [ "$status" -eq 0 ] + + run bash -c "'$GATE' <<'IN' +arm=base path=hook p50=2.00 p95=3.03 mean=2.64 runs=100 +arm=head path=hook p50=2.62 p95=3.08 mean=2.66 runs=100 +IN" + [ "$status" -eq 1 ] +} + +# The measured null maximum, asserted as a floor under the threshold. A gate set +# below the noise of a comparison that measures NOTHING is a coin flip, and the +# way that happens is somebody tightening the constant without re-running `mise +# run perf-pair --null`. This case fails the moment they do. +@test "the threshold clears the measured noise floor (n=30, max 1.102x)" { + run bash -c "'$GATE' <<'IN' +arm=base path=hook p50=1.000 p95=3.03 mean=2.64 runs=100 +arm=head path=hook p50=1.102 p95=3.08 mean=2.66 runs=100 +IN" + [ "$status" -eq 0 ] +} + +@test "the threshold is honoured from the environment" { + run bash -c "BENCH_REGRESSION_RATIO=1.05 '$GATE' <<'IN' +arm=base path=hook p50=2.00 p95=3.03 mean=2.64 runs=100 +arm=head path=hook p50=2.30 p95=3.08 mean=2.66 runs=100 +IN" + [ "$status" -eq 1 ] + [[ "$output" == *"threshold 1.05x"* ]] +} + +# A head arm with no base arm means the base build measured less than it claims, +# and reporting green over a comparison that never happened is the +# partial-coverage false green. +@test "a head arm with no base arm is could-not-look, not a pass" { + run bash -c "'$GATE' <<'IN' +arm=base path=noop p50=2.40 p95=2.88 mean=2.44 runs=100 +arm=head path=noop p50=2.44 p95=2.90 mean=2.47 runs=100 +arm=head path=hook p50=2.62 p95=3.08 mean=2.66 runs=100 +IN" + [ "$status" -eq 2 ] + [[ "$output" == *"measured on head but not on base"* ]] +} + +@test "base arms alone are could-not-look — there is nothing to judge" { + run bash -c "'$GATE' <<'IN' +arm=base path=noop p50=2.40 p95=2.88 mean=2.44 runs=100 +IN" + [ "$status" -eq 2 ] + [[ "$output" == *"no \`head\` measurements"* ]] +} + +@test "empty stdin is could-not-look, and names the redirect" { + run bash -c "'$GATE'