diff --git a/Cargo.toml b/Cargo.toml index c57c2bd9f..24fc9b3dd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -548,10 +548,91 @@ yaml-rust2 = "0.11" lto = "thin" strip = true -# Faster incremental builds for the compile-heavy fmt -> lint -> test hook chain. +# Faster incremental builds for the compile-heavy fmt -> lint -> test hook chain, +# and 118 test binaries' worth of bytes off `target/debug` (CLOUD-1211). +# +# `debug = 1` was already a REDUCTION — cargo's dev default is `debug = 2`, so +# line-tables-only was the saving this comment was originally written about, and +# it was correct as written. What it never accounted for is the POPULATION: one +# dial for two groups, the library under iteration and ~118 integration test +# targets nobody attaches a debugger to. Nothing counted the second group's bytes +# until CLOUD-766 hit a full disk. +# +# THREE ARMS MEASURED on this container, 2026-08-30, each a cold +# `mise run test:cargo` over a cleared `target/debug`. The census counts +# `target/debug/deps`' extension-less linked binaries — the population +# `crates/batten/src/prune.rs:262-269` reads: +# +# arm artifacts bytes mean cold wall +# debug = 1 (the baseline) 125 15.53 GB 124.3 MB 231s +# debug = 0 (the whole profile) 123 1.60 GB 13.0 MB 264s +# debug = 1 + package."*" debug = 0 125 6.82 GB 54.5 MB 217s +# +# THE MIDDLE ARM IS NOT ADOPTED, AND REJECTING IT IS THE POINT OF THIS COMMENT. +# It takes 9.7x off the bytes, which is the largest number here and the reason it +# was briefly committed. It buys that by dropping debuginfo from the WHOLE dev +# profile — batten's own code included — so a panicking test reports a backtrace +# with no line numbers. That is the diagnostic a reader wants most at exactly the +# moment they need it, and no byte count is worth it. +# +# The adopted arm keeps line tables for workspace code and strips the dependency +# closure, which is where the bytes actually were: 124.3 MB -> 54.5 MB per binary +# while every `batten` frame keeps its file and line. Nobody steps into gix or +# regorus while debugging this crate. +# +# THE WALL-CLOCK COLUMN IS NOT A FINDING and must not be quoted as one. Three +# single cold runs at different contention, unpaired and with no null — the 264s +# in particular was taken while other work shared the box. They disagree in both +# directions, which is what unpaired readings look like. The byte column is the +# measured result; the time column is recorded only so nobody re-runs it +# expecting an answer. The paired warm arm that WOULD decide it could not be +# taken: at `debug = 1` the tree leaves 6.3 GB against `target-prune`'s 9970 MB +# floor, so the instrument cannot run in the condition it needs to measure — +# CLOUD-766's exhaustion arriving inside the experiment. +# +# THE CARGO CONSTRAINT, recorded because the row's §3 asked for something cargo +# cannot express: profiles are per-PACKAGE, not per-target-kind, so "debug off +# for test targets only" is not sayable. The split below is the nearest thing +# that preserves this profile's stated reasoning for the code it was written +# about. +# +# The linker claim this row was FILED on is withdrawn and stays recorded so it is +# not re-proposed: rust-lld has been the default on this host triple since Rust +# 1.90, this repository pins 1.97.1, and `readelf -p .comment` over a built +# artifact reports `Linker: LLD 22.1.6`. The build already links with lld. [profile.dev] debug = 1 +# The dependency closure carries no debuginfo and IS optimised. Workspace code +# above keeps its line tables and stays unoptimised, so rebuilds of the code under +# iteration are still fast and its backtraces still readable — the split is the +# whole point, and neither half is a compromise for the other. +# +# `opt-level` MEASURED 2026-08-31, and it is the largest single win in this +# campaign. `batten hook` against this repository's own config — the committed +# ruleset plus ~20 Rego modules — is dominated by dependency code compiling and +# evaluating Rego, and unoptimised that work is 6.8x what the shipped binary does: +# +# batten hook, real config debug 325ms release 48ms +# with deps at opt-level = 2 debug 70ms (4.6x, near release) +# +# mise run test:cargo, warm 100.189s -> 48.581s 3201 tests, all passing +# +# A 2.06x on the whole suite, against CLOUD-1208's measured 0.963-1.012 null. The +# tests drive the DEBUG binary (`CARGO_BIN_EXE_batten`), so every case that judges +# a real config paid that 6.8x tax — which is why the binaries the per-case census +# flags are exactly the policy-evaluating ones (`board_receipts`, `mediated_verbs`, +# `pipeline_shapes`, `connector_verbs`, and `cli`'s committed-config cases). +# +# THE COST, STATED: a cold dependency build goes 217s -> 366s. It is paid ONCE and +# then cached — dependencies recompile only when they change or this profile does, +# and CI already carries `Swatinem/rust-cache`. So it is +149s once against -51.6s +# on every suite run, and the warm edit-test loop a developer sits in pays none of +# it. +[profile.dev.package."*"] +debug = 0 +opt-level = 2 + # The profile the distributed single binary is built with: maximal optimization # and the smallest artifact. Distinct from `release` (used for local/dev release # builds) so day-to-day `cargo build --release` stays fast. diff --git a/bench/acquisition/sweep.py b/bench/acquisition/sweep.py deleted file mode 100755 index 2b3877cd4..000000000 --- a/bench/acquisition/sweep.py +++ /dev/null @@ -1,327 +0,0 @@ -#!/usr/bin/env python3 -"""Tree-surface acquisition cost as a function of declared-document count. - -CLOUD-935. `.claude/rules/rust.md`'s concurrency table carries one row whose -verdict is conditional and unmet — tree-surface fact ACQUISITION "stays serial -until a number says otherwise" — and states the condition: *"To move it now, -bring a number showing resolution — not projection — is the cost."* CLOUD-834 -measured PROJECTION and disclaims this half. This is that number. - -## Why a second harness rather than an arm in `perf` - -`mise-tasks/perf.sh` measures fixed paths over fixed fixtures, and its arms -bracket INVOCATION cost. This sweeps a variable, which is a different experiment -with a different independent axis, and it needs to generate a fixture family per -run rather than materialise two committed ones. Adding a swept arm to `perf` -would also have meant editing an authored shell rule, which -`policy/shell-retirement.rego` refuses at `deny` with no override route -(`V-SHELL-RULE-EDITED`) — so the shape here is a Python helper driven by an -inline `mise.toml` task, the same inline-task shape `semver` and `policy-test` -were forced into for that identical reason. - -Under `bench/` beside `bench/gates/classify.py` rather than under `mise-tasks/`: -mise makes every executable in that directory a file task named by its basename -AND its stem, so a helper there would publish a second entry point that runs this -sweep with no `BENCH_METRIC` set — stamping the invocation series' default into -the acquisition series, which is the one thing §5 exists to prevent. - -## The experiment, and the confound it is built to avoid - -ONE rule, ONE bundle, ONE module — and the row's `documents` array is what -grows. A row PER document would have made every step of the sweep add a module -compile and an evaluation beside each read, so the curve would price four things -and get reported as one. Holding everything but the declared path count fixed is -what leaves acquisition as the only term that moves. -`crates/batten/tests/document_read_count.rs::one_row_declaring_n_paths_acquires_n_documents` -pins that the engine really does acquire once per declared path under exactly -this shape, because a sweep over a variable the engine ignores would still draw a -tidy curve. - -## Ratios, never absolutes - -Machine noise is common-mode across arms measured seconds apart on one machine, -so it divides out — the same reason `perf-compare` decides a ratio and -`.claude/rules/rust.md` records why wall clock is usable at all here. Every arm -is reported, and the verdict is read off `ratio=` lines against the N=0 floor. - -## The null is not optional - -Two IDENTICAL trees at the largest N, measured as a separate pair. Its ratio is -1.0 plus pure noise by construction, which is what makes the spread a measured -quantity rather than a number in a comment — exactly how `perf-pair --null` -derived the 0.966–1.102 spread `perf-compare`'s 1.30 threshold clears. A sweep -number that sits inside the null spread has measured "no effect", and that is a -result rather than a failure to deliver. - -## Output - -Pointer-only per non-negotiable rule 4: one `path=` record per arm in `perf.sh`'s -byte-stable shape, then one `ratio=` line per comparison. No fixture contents, no -command lines, no hyperfine chatter — the raw JSON stays under the output -directory for a human. - -Exit 0 measured / 1 a measurement failed / 2 could not look. -""" - -from __future__ import annotations - -import json -import os -import shutil -import subprocess -import sys -from pathlib import Path - -# `perf.sh`'s names and defaults, deliberately: a caller who already knows how to -# turn that task down should not have to learn a second vocabulary. -RUNS = int(os.environ.get("BENCH_RUNS", "100")) -WARMUP = int(os.environ.get("BENCH_WARMUP", "10")) -# ABSOLUTE, and that is load-bearing rather than tidy. Every arm runs hyperfine -# with the FIXTURE tree as its working directory, so a relative export path -# resolves against the fixture and hyperfine dies with "No such file or -# directory" before it times anything. `mise-tasks/perf.sh`'s header records the -# identical lesson for its own check arm; this is the same trap one harness over. -# `resolve()` is happy on a path that does not exist yet, which is why it can sit -# here rather than after the mkdir. -OUT_DIR = Path(os.environ.get("BENCH_OUT_DIR", "target/acquisition-bench")).resolve() -BIN = Path(os.environ.get("BENCH_BIN", "target/release/batten")) - -# The sweep, and its FIRST entry is the ratio base. -# -# ONE, NOT ZERO, and that correction is the difference between measuring -# acquisition and measuring "does this tree have a policy row at all". A zero arm -# carries no rule, so the step from it to any other arm bundles the fixed cost of -# registering a bundle, compiling a module and evaluating it in with the reads — -# measured, that step alone read 1.367 at N=16, which would have been published as -# a per-document cost it is mostly not. Basing the ratios on a tree that already -# has exactly one rule and one document holds every fixed term constant, so the -# only thing differing between arms is how many paths that one row declares. -# -# 256 is chosen to be past the point where a per-document term, if there is one, -# has to be visible above a ~5 ms process start: at 256 even a 10 µs read is -# 2.5 ms. `BENCH_NS=0,...` still works and gives the no-policy reference, which is -# a different question and is not what the verdict is read off. -NS = [int(n) for n in os.environ.get("BENCH_NS", "1,16,64,256").split(",") if n] - -# How many identical pairs the null is taken over. Five rather than one, because -# a single ratio is a point and the sweep has to be read against a WIDTH. -NULL_PAIRS = int(os.environ.get("BENCH_NULL_PAIRS", "5")) - -# One module, whose body reads whatever was declared. Iterating `documents` -# rather than naming a path keeps the module identical across every arm, so the -# only thing differing between arms is the row's declaration. -MODULE = """package batten.acquisition - -import rego.v1 - -rules contains "acquisition-bench" - -violation contains { -\t"rule": "acquisition-bench", -\t"verdict": "V-ACQUISITION-BENCH", -\t"subjects": [{"path": path}], -} if { -\tsome path, doc in input.tree.documents -\tdoc.stray -} -""" - -AUTHORITY_HEAD = """version = 1 - -[[verdict]] -id = "V-ACQUISITION-BENCH" -gloss = "the bench fixture declared a document carrying the sentinel key" -class = \"\"\" -A generated fixture for CLOUD-935's acquisition sweep. It is never raised: the -documents carry no sentinel, so the run is clean and the number is about reading -rather than about rendering findings. -\"\"\" - -[[verdict.route]] -id = "R-REGENERATE-THE-FIXTURE" -kind = "document" -target = "batten.toml" -""" - - -def fail(message: str, code: int = 2) -> None: - print(f"::error:: acquisition-bench: {message}", file=sys.stderr) - raise SystemExit(code) - - -def build_tree(root: Path, n: int) -> None: - """A repository with one policy row declaring `n` distinct documents.""" - if root.exists(): - shutil.rmtree(root) - (root / "policy-acquisition").mkdir(parents=True) - (root / "policy-acquisition" / "gate.rego").write_text(MODULE, encoding="utf-8") - - paths = [f"config{i}.toml" for i in range(n)] - for path in paths: - # Small and uniform. The cost being priced is the fixed per-document term - # — open, read, parse, cache — rather than a per-byte one, and a large - # file would measure the parser instead. Said out loud so the fixture does - # not grow by accretion, the way `perf.sh` says the same thing about its - # post-tool payload. - (root / path).write_text("quiet = true\n", encoding="utf-8") - - # THE FLOOR ARM CARRIES NO ROW AND NO VERDICT, which is what makes it the - # floor: config load, trust resolution and the walk, and not one acquisition. - # A row declaring zero documents would still compile a module and put that - # cost into the baseline every ratio is taken against. - # - # The verdict row goes with it, and that is the REGISTRY's requirement rather - # than a choice: `[[verdict]]` runs in both directions, so a declared class - # nothing raises fails the load outright ("a class no gate reaches reads as - # coverage"). With no rule there is no module, so the token is unraised and a - # floor arm carrying it would not run at all. The residual difference is a few - # lines of TOML the other arms also parse, which is orders below the noise the - # null measures. - if n: - declared = ", ".join(f'"{p}"' for p in paths) - authority = ( - AUTHORITY_HEAD - + "\n[[rule]]\n" - 'id = "acquisition-bench"\n' - 'kind = "policy"\n' - 'scope = "tree"\n' - 'bundle = "policy-acquisition/"\n' - f"documents = [{declared}]\n" - 'severity = "deny"\n' - ) - else: - authority = "version = 1\n" - (root / "batten.toml").write_text(authority, encoding="utf-8") - - # `git init` so the walk is a repository walk, matching every other fixture in - # this tree. No global or system config: a contributor's own git settings must - # not be able to change what is measured (CLOUD-282). - env = dict(os.environ, GIT_CONFIG_GLOBAL="/dev/null", GIT_CONFIG_SYSTEM="/dev/null") - subprocess.run( - ["git", "init", "-q", "-b", "main"], - cwd=root, - env=env, - check=True, - capture_output=True, - ) - - -def measure(arm: str, root: Path, binary: Path) -> dict[str, float]: - """One hyperfine run of `batten check` in `root`, as a record.""" - out = OUT_DIR / f"{arm}.json" - result = subprocess.run( - [ - "hyperfine", - "--warmup", - str(WARMUP), - "--runs", - str(RUNS), - "--shell=none", - "--export-json", - str(out), - "--style", - "none", - f"{binary} check", - ], - cwd=root, - capture_output=True, - text=True, - ) - if result.returncode != 0: - # NO `-i`. Every arm's fixture is clean, so a non-zero exit means the - # binary started failing rather than that the measurement is awkward — - # and a broken path is still perfectly timeable, which is how it would - # otherwise be published as a fast number. - (OUT_DIR / f"{arm}.err").write_text(result.stderr, encoding="utf-8") - fail(f"measuring arm {arm} failed — see {OUT_DIR / f'{arm}.err'}. No records.", 1) - - times = sorted(json.loads(out.read_text(encoding="utf-8"))["results"][0]["times"]) - count = len(times) - # p95 from the sorted per-run times rather than mean+2sd, for `perf.sh`'s - # reason: startup latency is right-skewed, so a normal assumption understates - # exactly the tail a budget would be about. - return { - "p50": times[int((count - 1) * 0.5)] * 1000, - "p95": times[-(-int((count - 1) * 95) // 100)] * 1000, - "mean": sum(times) / count * 1000, - "runs": count, - } - - -def record(arm: str, stats: dict[str, float]) -> None: - print( - f"path={arm} p50={stats['p50']:.2f} p95={stats['p95']:.2f} " - f"mean={stats['mean']:.2f} runs={int(stats['runs'])}" - ) - - -def main() -> int: - root = subprocess.run( - ["git", "rev-parse", "--show-toplevel"], - capture_output=True, - text=True, - check=False, - ) - if root.returncode != 0: - fail("not a git repository, so there is no tree to measure over") - os.chdir(root.stdout.strip()) - - if shutil.which("hyperfine") is None: - fail("hyperfine is not installed — run `mise install`; it is pinned in mise.toml") - binary = BIN.resolve() - if not binary.is_file() or not os.access(binary, os.X_OK): - fail(f"{BIN} is missing — run `mise run build:release`. Nothing measured.") - - if OUT_DIR.exists(): - shutil.rmtree(OUT_DIR) - OUT_DIR.mkdir(parents=True) - - # THE SWEEP, measured back to back on one machine so the noise the ratios - # divide out is the same noise. - stats: dict[int, dict[str, float]] = {} - for n in NS: - tree = OUT_DIR / f"tree-{n}" - build_tree(tree, n) - stats[n] = measure(f"acquire-{n}", tree, binary) - record(f"acquire-{n}", stats[n]) - - # THE NULL, AND IT IS A SPREAD RATHER THAN A NUMBER. Two identical trees at - # the largest N, built separately so each comparison is between two arms - # rather than an arm against itself — repeated, because ONE null ratio says - # nothing about how wide the noise is and a sweep ratio can only be read - # against a width. `perf-compare`'s 0.966–1.102 came from n=30 for exactly - # this reason; the pairs here are longer, so fewer of them bound it. - nulls: list[float] = [] - largest = max(NS) - for pair in range(NULL_PAIRS): - sides = {} - for side in ("a", "b"): - tree = OUT_DIR / f"null{pair}-{side}" - build_tree(tree, largest) - arm = f"null{pair}-{side}" - sides[side] = measure(arm, tree, binary) - record(arm, sides[side]) - nulls.append(sides["b"]["p50"] / sides["a"]["p50"]) - - base = stats[NS[0]]["p50"] - if base <= 0: - fail("the base arm measured zero, so no ratio can be taken", 1) - for n in NS[1:]: - print(f"ratio=acquire-{n}/acquire-{NS[0]} value={stats[n]['p50'] / base:.3f}") - for pair, value in enumerate(nulls): - print(f"ratio=null{pair} value={value:.3f}") - print(f"null-spread low={min(nulls):.3f} high={max(nulls):.3f} pairs={len(nulls)}") - - # THE PER-DOCUMENT TERM, which is the number the verdict is actually about. - # Reported rather than left to a reader with a calculator, and taken across - # the widest span in the sweep because that is where the fixed terms matter - # least. Microseconds, since milliseconds would round it to nothing. - span = max(NS) - NS[0] - if span > 0: - per_doc = (stats[max(NS)]["p50"] - base) * 1000 / span - print(f"per-document us={per_doc:.2f} over={span} documents") - return 0 - - -if __name__ == "__main__": - sys.exit(main()) diff --git a/bench/gates/RESULTS.md b/bench/gates/RESULTS.md index 0344eaf99..378c9602c 100644 --- a/bench/gates/RESULTS.md +++ b/bench/gates/RESULTS.md @@ -1,6 +1,14 @@ # What each gate-described `mise-tasks/` program invokes -Generated by `bench/gates/classify.py`. Do not hand-edit. +A FROZEN CENSUS, taken 2026-08-23. Its generator was 269 lines of Python that +nothing invoked, and it was retired under CLOUD-1229 rather than kept warm for a +re-run nobody had asked for in a week. So these numbers are a reading of the tree +on that date and are not refreshed: the corpus has since shrunk as CLOUD-1059's +migrations landed, and a reader wanting today's count should take it rather than +trust this. What the file is still cited for — by `facts.rs`, `rules.rs` and +`tests/git_facts.rs` — is the SHAPE it measured, which is the git-fact variant +split below, and that does not go stale with the count. + Classified by COMMAND-POSITION invocation over a tree-sitter-bash parse, so a token inside a comment or a string is not a hit (`.claude/rules/scanning.md` row two; CLOUD-843's two passes disagreed diff --git a/bench/gates/classify.py b/bench/gates/classify.py deleted file mode 100644 index 23949e29e..000000000 --- a/bench/gates/classify.py +++ /dev/null @@ -1,269 +0,0 @@ -"""Classify the gate-described `mise-tasks/` programs by what each INVOKES. - -CLOUD-907's first deliverable, and it gates the rest of that row: the bucket -sizes the bash-retirement campaign, and an estimate cannot schedule it. - -# Why this is not a `grep` - -`.claude/rules/scanning.md` row two. The question — "is this token in command -position, inside a comment, or inside a string" — is a SYNTAX question, and the -two instruments give different answers. CLOUD-843 ran both an hour apart: a -substring pass gave 11 tree / 24 git / 31 tracker / 16 forge, and a -command-position pass over the same files gave 22 / 50 / 3 / 7, because -`ci-local-parity` and `pipefail-grep-check` carried the token in a COMMENT. The -substring reading was nearly published as the campaign's scoping. - -So this walks tree-sitter-bash's parse and reads `command_name` nodes. A comment -is its own node kind and is never a command head, so "comments stripped" is a -property of the grammar rather than a preprocessing step that can be got wrong. - -# Why it is not wired into `hk` - -Deliberately an INTERACTIVE instrument, run with the language pinned, exactly as -`scanning.md` scopes row two. CLOUD-310's rejection of a matcher CLI *as a gate* -is measured and stands: the programs under `mise-tasks/` carry no extension, so a -run pointed at that directory scans nothing and still exits 0 — a gate that found -nothing looks exactly like a gate that passed. A standing gate over command -position needs the general fix, which is CLOUD-914's row, not a second copy of -this script behind a `#MISE description`. - -# Running it - - python3 -m venv .venv && .venv/bin/pip install tree_sitter tree_sitter_bash - .venv/bin/python bench/gates/classify.py > bench/gates/RESULTS.md - mise exec -- prettier --write bench/gates/RESULTS.md - -The `prettier` pass is part of generation, not a hand-edit: `hk` formats every -tracked markdown file, so a generator whose output it would rewrite produces a -file the tree cannot hold. Column alignment is the whole of what it changes. It -goes through `mise exec` because prettier IS a pinned tool here (`npm:prettier` -in `mise.toml`), so a bare call would format with whatever version happened to -be on PATH and could produce bytes the gate then rewrites. - -The two tree-sitter packages are NOT pinned, and the reason is a property of what -they are rather than of where the script sits (CLOUD-480, raised on review). -mise's backends install executables; these are import-only Python libraries with -no CLI, so there is nothing for it to put on PATH — and `python` itself is not a -`[tools]` entry, so adding one would download a toolchain into every clone for a -script nothing on the landing path runs. The venv is the narrowest thing that -works. If CLOUD-914 makes command-position scanning a standing gate, its inputs -become landing-path inputs and get pinned like any other. -""" - -import collections -import pathlib -import re -import sys - -import tree_sitter_bash -from tree_sitter import Language, Parser - -PARSER = Parser(Language(tree_sitter_bash.language())) - -TASKS = pathlib.Path(__file__).resolve().parents[2] / "mise-tasks" - -# A task is gate-described when its `#MISE description=` OPENS with Gate. -# -# `Gate\b` rather than `Gate:`, and the difference is exactly one row: -# `signing-posture` opens `"Gate (and, with --repair, the write): ..."`. A -# colon-anchored predicate counts 84 and silently drops the one task that -# describes a gate with a caveat — the off-by-one this file exists to not make. -GATE_DESCRIPTION = re.compile(r'^#MISE description="Gate\b') - -# Which external program puts a task in which bucket, in PRECEDENCE order: the -# first bucket a task's command heads touch wins. A task that shells out to the -# forge is a forge task even when it also reads git, because the forge read is -# what blocks its migration. -BUCKETS: list[tuple[str, frozenset[str]]] = [ - ("forge", frozenset({"gh"})), - ("git", frozenset({"git"})), - ("build", frozenset({"cargo", "rustc", "hyperfine", "cargo-nextest"})), -] - -# Which git fact variant an invocation needs, decided from the subcommand and -# its flags rather than from the subcommand alone. `rev-parse` is 65 of the -# invocations and is NOT one question: `--git-dir` and `--show-toplevel` locate -# the repository, `HEAD` reads the current commit, and `^{commit}` resolves -# a declared ref. Collapsing them would put the whole corpus in one variant and -# tell the fact model nothing. -LOCATION_FLAGS = ("--git-dir", "--show-toplevel", "--is-inside-work-tree", "--absolute-git-dir") -ANCESTRY_FLAGS = ("--is-ancestor", "--count", "--merge-base") - - -def described_as_gate(text: str) -> bool: - for line in text.splitlines(): - if line.startswith("#MISE description="): - return GATE_DESCRIPTION.match(line) is not None - return False - - -def commands(source: bytes): - """Every `command` node, as (head, [argument words]). - - Words are the literal text of each argument; one built from an expansion - (`"$ref"`) is kept verbatim rather than guessed at, so a variant is never - inferred from a value this pass cannot see. - """ - tree = PARSER.parse(source) - stack = [tree.root_node] - while stack: - node = stack.pop() - if node.type == "command": - head = None - args = [] - for child in node.children: - if child.type == "command_name": - head = child.text.decode("utf-8", "replace") - elif head is not None and child.type not in ("file_redirect", "herestring_redirect"): - args.append(child.text.decode("utf-8", "replace")) - if head is not None: - yield head, args - stack.extend(node.children) - - -# Global options git accepts BEFORE the subcommand that take their value as a -# SEPARATE word. Skipping the value is what keeps `git -C "$root" rev-parse` from -# reading `"$root"` as the subcommand (CLOUD-480, found on review of #660): it -# resolved to no variant at all, so the task silently understated the git surface -# — and that count is what the retirement campaign is scheduled against. -GLOBAL_OPTS_WITH_VALUE = ("-C", "-c", "--git-dir", "--work-tree", "--namespace", "--exec-path") - - -def subcommand(words: list[str]) -> str: - """The subcommand, past any leading global options and their values.""" - index = 0 - while index < len(words): - word = words[index] - if not word.startswith("-"): - return word - # `--git-dir=x` carries its value inline, so only the separate-word form - # consumes the next element. - if word in GLOBAL_OPTS_WITH_VALUE: - index += 2 - else: - index += 1 - return "" - - -def variant(args: list[str]) -> str | None: - """Which git fact variant one `git ...` invocation needs.""" - words = [a for a in args if a] - sub = subcommand(words) - flags = [w for w in words if w.startswith("-")] - - if sub in ("fetch", "remote", "ls-remote", "push"): - return "remote" - if sub == "config": - return "remote" if any("remote." in w or "branch." in w for w in words) else "head" - if sub in ("status", "diff", "diff-index", "diff-files", "stash"): - return "status" - if sub in ("log", "show", "cat-file", "hash-object", "tag", "describe", "shortlog"): - return "log" - if sub == "merge-base": - return "ancestry" - if sub == "rev-list": - return "ancestry" if any(f.startswith(ANCESTRY_FLAGS) for f in flags) else "log" - if sub == "ls-files": - return "status" if any(f in ("-m", "-o", "--others", "--modified") for f in flags) else "tracked" - if sub in ("rev-parse", "symbolic-ref", "show-ref", "branch", "for-each-ref"): - if any(f in LOCATION_FLAGS for f in flags): - return "location" - return "head" - if sub in ("submodule", "worktree"): - return "location" - return None - - -def main() -> int: - rows = [] - for path in sorted(TASKS.iterdir()): - if not path.is_file(): - continue - source = path.read_bytes() - if not described_as_gate(source.decode("utf-8", "replace")): - continue - - heads: set[str] = set() - variants: set[str] = set() - for head, args in commands(source): - heads.add(head) - if head == "git": - found = variant(args) - if found: - variants.add(found) - - bucket = "tree" - for name, programs in BUCKETS: - if heads & programs: - bucket = name - break - - rows.append( - { - "task": path.name, - "bucket": bucket, - "variants": sorted(variants), - } - ) - - counts = collections.Counter(row["bucket"] for row in rows) - variant_counts: collections.Counter[str] = collections.Counter() - for row in rows: - if row["bucket"] == "git": - variant_counts.update(row["variants"]) - - out = sys.stdout - out.write("# What each gate-described `mise-tasks/` program invokes\n\n") - out.write( - "Generated by `bench/gates/classify.py`. Do not hand-edit.\n" - "Classified by COMMAND-POSITION invocation over a tree-sitter-bash parse,\n" - "so a token inside a comment or a string is not a hit\n" - "(`.claude/rules/scanning.md` row two; CLOUD-843's two passes disagreed\n" - "11/24/31/16 against 22/50/3/7 for exactly that reason).\n\n" - ) - out.write(f"- gate-described tasks: {len(rows)}\n") - for name in ("tree", "git", "build", "forge"): - out.write(f"- {name}: {counts.get(name, 0)}\n") - out.write("\n## The git bucket, by the fact variant each task needs\n\n") - out.write( - "A task appears once per variant it reads. The variant is decided from the\n" - "subcommand AND its flags: `rev-parse` is most of the corpus and is not one\n" - "question — `--git-dir` locates the repository, `HEAD` reads the current\n" - "commit, and a named ref resolves a declared one.\n\n" - ) - out.write("| variant | tasks |\n| --- | ---: |\n") - for name, count in sorted(variant_counts.items()): - out.write(f"| `{name}` | {count} |\n") - # WHICH TASKS NEED A FACT THAT DOES NOT EXIST YET, which is the number the - # campaign is actually scheduled against. `location` is the repository's own - # git dir and toplevel — the engine already resolves both before any rule - # runs — and `tracked` is `Fact::Tracked`, landed by CLOUD-846. A task whose - # whole git usage is those two needs no new fact at all, and counting it in - # the git bucket overstates the surface the fact model owes. - already = frozenset({"location", "tracked"}) - git_rows = [row for row in rows if row["bucket"] == "git"] - served = [row for row in git_rows if set(row["variants"]) <= already] - out.write("\n## What the git bucket actually owes the fact model\n\n") - out.write(f"- git-bucket tasks: {len(git_rows)}\n") - out.write( - f"- of those, served by facts that already exist " - f"(`location` + `Fact::Tracked`): {len(served)}\n" - ) - out.write(f"- needing a variant the engine cannot emit today: {len(git_rows) - len(served)}\n\n") - out.write("| git usage | tasks |\n| --- | ---: |\n") - shapes = collections.Counter( - ", ".join(f"`{v}`" for v in row["variants"]) or "—" for row in git_rows - ) - for shape, count in shapes.most_common(): - out.write(f"| {shape} | {count} |\n") - - out.write("\n## Every task\n\n") - out.write("| task | bucket | git variants |\n| --- | --- | --- |\n") - for row in rows: - variants = ", ".join(f"`{v}`" for v in row["variants"]) or "—" - out.write(f"| `{row['task']}` | {row['bucket']} | {variants} |\n") - return 0 - - -if __name__ == "__main__": - raise SystemExit(main()) diff --git a/crates/batten/examples/acquisition-bench.rs b/crates/batten/examples/acquisition-bench.rs new file mode 100644 index 000000000..9ed526ee4 --- /dev/null +++ b/crates/batten/examples/acquisition-bench.rs @@ -0,0 +1,72 @@ +//! Tree-surface acquisition cost as declared-document count scales (CLOUD-935). +//! +//! Retired out of `bench/acquisition/sweep.py` under CLOUD-1229, where it was 327 +//! lines of Python run by a one-line task under an interpreter nothing pinned. +//! +//! # Why an example target and not a verb +//! +//! `perf pair` is a verb, and this was written as its sibling first. The command +//! surface refused it, correctly: `crates/batten/tests/pointer_only.rs` sweeps +//! EVERY leaf verb over a bare fixture corpus and refuses one that exits `3`, +//! because "it failed internally, so what it did not emit proves nothing". A +//! sweep cannot satisfy that. It needs a benchmark runner and a built binary to +//! time, and in a bare corpus it has neither — so its only honest answer there is +//! could-not-look. `perf pair` passes that sweep because it has a real SKIP +//! predicate (a commit that cannot change what gets invoked cannot have made the +//! invocation slower); there is no analogue here, and manufacturing one to get +//! past a census would be exactly the false green this repository exists to +//! refuse. +//! +//! So the measurement is a target the surface does not carry: no verb, no +//! completion, no man page, and nothing for that census to be wrong about. What +//! it is NOT is a way around the workspace lints — an example is built by +//! `--all-targets`, so it is held to the same clippy bar as everything else. +//! +//! # Why the work is still in `crates/batten/src/perf.rs` +//! +//! This file spawns nothing. `policy/spawn-adapters.rego` decides which modules +//! may spawn by NAME RESOLUTION, and it places `perf` with a rationale that reads +//! as though written for this case: a harness whose whole subject is what an +//! external process costs, so the spawns are the thing rather than an +//! implementation of it. A sweep with its own `Command`, its own hyperfine +//! invocation and its own percentile convention would be an unplaced spawning +//! module AND a second authority over a record shape `perf-compare` already +//! reads. Sharing the module is what makes both unwritable. +//! +//! # Reading the output +//! +//! One `arm=` record per measured arm, then a `ratio=` per comparison, then the +//! `null-spread` those ratios must be read against, then the per-document term in +//! microseconds. A sweep number inside the null spread has measured *no effect*, +//! and that is a result rather than a failure to deliver. +//! +//! Exit 0 measured / 1 could not look. + +// The one sanctioned place to write to a stream: this target IS a binary +// boundary, exactly as `main.rs` is, and its whole output is the report. +#![allow(clippy::print_stdout, clippy::print_stderr)] + +use std::path::Path; + +fn main() -> std::process::ExitCode { + // `.` rather than a resolved toplevel: the task layer runs a task from the + // repository root, and `acquire` canonicalises before it resolves anything + // against it. A second repository-root resolver is the defect CLOUD-824 + // records one layer over, where a launcher asking git for `--show-toplevel` + // disagreed with the engine asking for the common dir. + match batten::perf::acquire(Path::new(".")) { + Ok(sweep) => { + print!("{sweep}"); + std::process::ExitCode::SUCCESS + } + // COULD NOT LOOK, in the `::error::` shape the workflow annotates, and + // never an empty sweep that exits 0. A bench harness reporting "measured, + // and there was nothing" over a run that never happened is the failure + // CLOUD-1208 hit twice in one session — once publishing a residue-free + // suite, once a null of 0.750. + Err(reason) => { + eprintln!("::error:: {reason}"); + std::process::ExitCode::FAILURE + } + } +} diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index cc5945b31..fd44ea204 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -8117,6 +8117,72 @@ fn announce_config(mode: Mode, err: &mut dyn Write, config: &resolve::Resolved) announce_degrade(mode, err, config.base.as_ref()) } +/// Report what each rule cost, on the `-vv` rung (CLOUD-1217). +/// +/// **This exists because the largest item in this repository's CI was a silent +/// span.** `batten-check` ran 465s of a 1327s job and emitted two lines, so the +/// cost was unattributable from its own output; two sessions guessed at it and +/// both were wrong, and the answer only came out of a scratch worktree with the +/// ruleset hand-edited. This turns that bisect into a flag. +/// +/// **`Debug`, not `Verbose`, and never the answer channel.** A duration is not +/// byte-stable, so it must not reach `-J`, a pointer line or stdout — house-style +/// §6. It is a measurement about the run, not a finding about the tree, and the +/// two channels stay separate. +/// +/// Read from `rules::rule_costs()` rather than off the `Scan`: a census is a +/// measurement ABOUT a run, not part of its value, and `batten semver check` +/// refused the field on that public struct — correctly, and the refusal named the +/// design as well as the API. Read immediately after the runner returns, because +/// the store holds the run that just finished. +/// +/// Sorted by cost descending, ties broken by rule id, because the question this +/// answers is "what is the pole" and a reader should not have to sort 84 lines. +/// The tiebreak is what keeps two runs over one tree reading the same. +/// +/// Pointer-only (non-negotiable rule 4): an id, two counts and a duration. Never +/// a path, never a scanned byte. +fn report_rule_costs(mode: Mode, err: &mut dyn Write) -> Result<()> { + let costs = rules::rule_costs(); + if costs.is_empty() { + return Ok(()); + } + let mut ranked: Vec<&rules::RuleCost> = costs.iter().collect(); + ranked.sort_by(|a, b| { + b.elapsed + .cmp(&a.elapsed) + .then_with(|| a.rule.as_str().cmp(b.rule.as_str())) + }); + for cost in &ranked { + output::message( + mode, + Verbosity::Debug, + err, + &format!( + "rule cost: {} {}ms {} file(s) {} byte(s)", + cost.rule, + cost.elapsed.as_millis(), + cost.files_read, + cost.bytes_read + ), + )?; + } + let elapsed: std::time::Duration = costs.iter().map(|cost| cost.elapsed).sum(); + let files: usize = costs.iter().map(|cost| cost.files_read).sum(); + let bytes: usize = costs.iter().map(|cost| cost.bytes_read).sum(); + output::message( + mode, + Verbosity::Debug, + err, + &format!( + "rule cost: {} rule(s) {}ms {files} file(s) {bytes} byte(s)", + costs.len(), + elapsed.as_millis(), + ), + )?; + Ok(()) +} + #[expect( clippy::too_many_lines, reason = "the one funnel `check` and `enforce` share, and it reads as one sequence: \ @@ -8166,6 +8232,7 @@ fn run_rules( scope: &scope, }; let scan = runner(&selected, &config.provisions, vocabulary, &root, opts)?; + report_rule_costs(mode, err)?; perform_requested_sinks(surface, &root, &scan); let mut findings = scan.findings.clone(); @@ -8238,9 +8305,11 @@ fn run_rules( // The baseline filter (CLOUD-67), immediately before the waiver filter. let findings = apply_baseline(findings, &scan, &root, mode, err)?; - // The admission filter (CLOUD-1120), between the two — see its own doc. - let config_from = overrides.config_from.as_deref(); - let findings = apply_admissions(findings, &scan, &root, config_from, mode, err)?; + // The admission filter (CLOUD-1120), between the two — see its own doc. It + // reads `base_ref`, bound once at the top: this used to rebind the identical + // `overrides.config_from.as_deref()` under a second name, so one value had + // two spellings in one function and a reader had to prove they agreed. + let findings = apply_admissions(findings, &scan, &root, base_ref, mode, err)?; // The waiver filter (CLOUD-208), applied HERE and nowhere else. This function // is the single funnel `check` and `enforce` share — they differ only in the diff --git a/crates/batten/src/perf.rs b/crates/batten/src/perf.rs index cda13d876..336feb187 100644 --- a/crates/batten/src/perf.rs +++ b/crates/batten/src/perf.rs @@ -432,7 +432,7 @@ fn run(dir: &Path, program: &str, args: &[String], env: &[(String, String)]) -> } let status = command .status() - .with_context(|| format!("perf-pair: could not run {program}"))?; + .with_context(|| format!("perf: could not run {program}"))?; Ok(status.success()) } @@ -824,7 +824,7 @@ fn state_prefixed(state: &str, argv: &[String]) -> Vec { } /// One arm's record, with `perf`'s own percentile convention. -fn record(arm: &str, id: &'static str, result: &serde_json::Value) -> Result { +fn record(arm: &'static str, id: &str, result: &serde_json::Value) -> Result { let mut times: Vec = result .get("times") .and_then(serde_json::Value::as_array) @@ -862,7 +862,7 @@ fn record(arm: &str, id: &'static str, result: &serde_json::Value) -> Result Result, + ratios: Vec<(String, f64)>, + nulls: Vec, + per_document: Option<(f64, usize)>, +} + +fn round3(value: f64) -> f64 { + (value * 1000.0).round() / 1000.0 +} + +impl std::fmt::Display for Sweep { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + for record in &self.arms { + writeln!(f, "{record}")?; + } + for (label, value) in &self.ratios { + writeln!(f, "ratio={label} value={:.3}", round3(*value))?; + } + for (pair, value) in self.nulls.iter().enumerate() { + writeln!(f, "ratio=null{pair} value={:.3}", round3(*value))?; + } + if !self.nulls.is_empty() { + let low = self.nulls.iter().copied().fold(f64::INFINITY, f64::min); + let high = self.nulls.iter().copied().fold(f64::NEG_INFINITY, f64::max); + writeln!( + f, + "null-spread low={:.3} high={:.3} pairs={}", + round3(low), + round3(high), + self.nulls.len() + )?; + } + // THE PER-DOCUMENT TERM, which is the number the verdict is actually + // about. Reported rather than left to a reader with a calculator, and + // taken across the widest span in the sweep because that is where the + // fixed terms matter least. Microseconds, since milliseconds would round + // it to nothing. + if let Some((per_document, span)) = self.per_document { + writeln!(f, "per-document us={per_document:.2} over={span} documents")?; + } + Ok(()) + } +} + +/// Measure tree-surface acquisition cost as the declared-document count scales. +/// +/// # The experiment, and the confound it is built to avoid +/// +/// ONE rule, ONE bundle, ONE module — and the row's `documents` array is what +/// grows. A row PER document would have made every step of the sweep add a module +/// compile and an evaluation beside each read, so the curve would price four +/// things and get reported as one. Holding everything but the declared path count +/// fixed is what leaves acquisition as the only term that moves. +/// `crates/batten/tests/document_read_count.rs::one_row_declaring_n_paths_acquires_n_documents` +/// pins that the engine really does acquire once per declared path under exactly +/// this shape, because a sweep over a variable the engine ignores would still +/// draw a tidy curve. +/// +/// # The null is not optional +/// +/// Two IDENTICAL trees at the largest N, measured as a separate pair. Its ratio +/// is 1.0 plus pure noise by construction, which is what makes the spread a +/// measured quantity rather than a number in a comment — exactly how +/// `perf pair --null` derived the 0.966–1.102 spread `perf-compare`'s 1.30 +/// threshold clears. A sweep number inside the null spread has measured "no +/// effect", and that is a result rather than a failure to deliver. +/// +/// # Errors +/// +/// Every failure here is a property of the CHECKOUT rather than a verdict about +/// acquisition — a missing instrument, a binary nobody built, a fixture that +/// would not initialise — and each is an error rather than an empty measurement, +/// for [`pair`]'s reason. +pub fn acquire(repo: &Path) -> Result { + // ABSOLUTE FROM HERE DOWN, for `pair`'s measured reason: every arm runs + // hyperfine with the FIXTURE tree as its working directory, so a relative + // binary path resolves against the fixture and hyperfine dies before it times + // anything. + let repo = &repo + .canonicalize() + .with_context(|| format!("perf-acquire: could not resolve {}", repo.display()))?; + + if which("hyperfine").is_none() { + bail!( + "perf-acquire: hyperfine is not installed — run `mise install`; it is pinned in the manifest. Nothing measured." + ); + } + let binary = repo.join(env_or(BIN_VAR, DEFAULT_BIN)); + if !binary.is_file() { + bail!( + "perf-acquire: {} is missing — run `mise run build:release`. Nothing measured.", + binary.display() + ); + } + + let ns = declared_ns()?; + let out = acquire_out_dir(repo)?; + let null_pairs: usize = env_or(NULL_PAIRS_VAR, DEFAULT_NULL_PAIRS) + .parse() + .with_context(|| format!("perf-acquire: {NULL_PAIRS_VAR} is not a count"))?; + + // THE SWEEP, measured back to back on one machine so the noise the ratios + // divide out is the same noise. + let mut arms = Vec::new(); + let mut p50s = Vec::new(); + for n in &ns { + let tree = out.join(format!("tree-{n}")); + sweep_fixture(&tree, *n)?; + let record = measure_one("acquire", &format!("acquire-{n}"), &tree, &out, &binary)?; + p50s.push(record.p50); + arms.push(record); + } + + // THE NULL, AND IT IS A SPREAD RATHER THAN A NUMBER. Two identical trees at + // the largest N, built separately so each comparison is between two arms + // rather than an arm against itself — repeated, because ONE null ratio says + // nothing about how wide the noise is and a sweep ratio can only be read + // against a width. + let largest = ns.iter().copied().max().unwrap_or_default(); + let mut nulls = Vec::new(); + for pair in 0..null_pairs { + let mut sides = Vec::new(); + for side in ["a", "b"] { + let id = format!("null{pair}-{side}"); + let tree = out.join(&id); + sweep_fixture(&tree, largest)?; + let record = measure_one("null", &id, &tree, &out, &binary)?; + sides.push(record.p50); + arms.push(record); + } + let (first, second) = (sides[0], sides[1]); + if first <= 0.0 { + bail!("perf-acquire: null pair {pair} measured zero, so no ratio can be taken."); + } + nulls.push(second / first); + } + + let base = *p50s.first().unwrap_or(&0.0); + if base <= 0.0 { + bail!("perf-acquire: the base arm measured zero, so no ratio can be taken."); + } + let ratios = ns + .iter() + .zip(&p50s) + .skip(1) + .map(|(n, p50)| (format!("acquire-{n}/acquire-{}", ns[0]), p50 / base)) + .collect(); + + let span = largest.saturating_sub(ns[0]); + let per_document = (span > 0).then(|| { + #[expect( + clippy::cast_precision_loss, + reason = "a declared-document count is a small integer and this is a divisor, not a measurement" + )] + let width = span as f64; + ((p50s[p50s.len() - 1] - base) * 1000.0 / width, span) + }); + + Ok(Sweep { + arms, + ratios, + nulls, + per_document, + }) +} + +/// The declared sweep points, in order, with the first as the ratio base. +/// +/// Split from its environment read so the parse stays exercisable without one, +/// which is [`select`]'s split one concern over: a case that had to export +/// `BENCH_NS` would be asserting over process-global state that every other case +/// in the file shares. +fn parse_ns(declared: &str) -> Result> { + let ns: Vec = declared + .split(',') + .map(str::trim) + .filter(|entry| !entry.is_empty()) + .map(|entry| { + entry + .parse::() + .with_context(|| format!("perf-acquire: {NS_VAR} carries a non-count `{entry}`")) + }) + .collect::>()?; + if ns.is_empty() { + bail!("perf-acquire: {NS_VAR} declared no sweep points. Nothing measured."); + } + Ok(ns) +} + +/// [`parse_ns`] over what the environment declares. +fn declared_ns() -> Result> { + parse_ns(&env_or(NS_VAR, DEFAULT_NS)) +} + +/// The out directory this sweep owns, emptied first so a previous run's fixtures +/// and JSON can never be read as this one's. +fn acquire_out_dir(repo: &Path) -> Result { + let dir = repo + .join(env_or(OUT_DIR_VAR, DEFAULT_OUT_DIR)) + .join("acquire"); + if dir.exists() { + std::fs::remove_dir_all(&dir) + .with_context(|| format!("perf-acquire: could not clear {}", dir.display()))?; + } + std::fs::create_dir_all(&dir) + .with_context(|| format!("perf-acquire: could not create {}", dir.display()))?; + dir.canonicalize() + .with_context(|| format!("perf-acquire: could not resolve {}", dir.display())) +} + +/// A repository with one policy row declaring `n` distinct documents. +/// +/// **Public for [`select`]'s reason, which is the same reason one axis over**: the +/// measurement needs a benchmark runner and the `windows` job installs none, so a +/// case that ran the sweep would pass on two hosts and fail on the third. What +/// can be asserted everywhere is that the fixture this times is one the ENGINE +/// accepts — a generated authority that fails to load, or a module that refuses, +/// makes every arm time a broken tree and still draws a tidy curve. Exposing the +/// builder is what lets `tests/perf_acquire.rs` put the compiled binary over one +/// of these trees without a second spelling of the fixture. +/// +/// # Errors +/// +/// Any write that fails, or a `git init` that does — a fixture that did not +/// materialise is a could-not-look, never an arm measured over whatever was +/// there. +pub fn sweep_fixture(root: &Path, n: usize) -> Result<()> { + if root.exists() { + std::fs::remove_dir_all(root) + .with_context(|| format!("perf-acquire: could not clear {}", root.display()))?; + } + let bundle = root.join("policy-acquisition"); + std::fs::create_dir_all(&bundle) + .with_context(|| format!("perf-acquire: could not create {}", bundle.display()))?; + std::fs::write(bundle.join("gate.rego"), SWEEP_MODULE) + .context("perf-acquire: could not write the sweep module")?; + + let paths: Vec = (0..n).map(|index| format!("config{index}.toml")).collect(); + for path in &paths { + // Small and uniform. The cost being priced is the fixed per-document term + // — open, read, parse, cache — rather than a per-byte one, and a large + // file would measure the parser instead. Said out loud so the fixture does + // not grow by accretion. + std::fs::write(root.join(path), "quiet = true\n") + .with_context(|| format!("perf-acquire: could not write {path}"))?; + } + + // THE FLOOR ARM CARRIES NO ROW AND NO VERDICT, which is what makes it the + // floor: config load, trust resolution and the walk, and not one acquisition. + // A row declaring zero documents would still compile a module and put that + // cost into every baseline the ratios are taken against. The verdict row goes + // with it, and that is the REGISTRY's requirement rather than a choice: with + // no rule there is no module, so the token is unraised and a floor arm + // carrying the row would not load at all. + let authority = if n == 0 { + String::from("version = 1\n") + } else { + let declared = paths + .iter() + .map(|path| format!("\"{path}\"")) + .collect::>() + .join(", "); + format!( + "{SWEEP_AUTHORITY_HEAD}\n[[rule]]\nid = \"acquisition-bench\"\nkind = \"policy\"\n\ + scope = \"tree\"\nbundle = \"policy-acquisition/\"\ndocuments = [{declared}]\n\ + severity = \"deny\"\n" + ) + }; + std::fs::write(root.join("batten.toml"), authority) + .context("perf-acquire: could not write the fixture authority")?; + + // `git init` so the walk is a repository walk, matching every other fixture + // in this tree. No global or system config: a contributor's own git settings + // must not be able to change what is measured (CLOUD-282). + let args: Vec = ["init", "-q", "-b", "main"] + .iter() + .map(|arg| (*arg).to_owned()) + .collect(); + let env = vec![ + (String::from("GIT_CONFIG_GLOBAL"), String::from("/dev/null")), + (String::from("GIT_CONFIG_SYSTEM"), String::from("/dev/null")), + ]; + if !run(root, "git", &args, &env)? { + bail!( + "perf-acquire: could not initialise the fixture at {}. Nothing measured.", + root.display() + ); + } + Ok(()) +} + +/// One hyperfine run of `batten check` in `tree`, as a record. +/// +/// NO `-i`. Every arm's fixture is clean, so a non-zero exit means the binary +/// started failing rather than that the measurement is awkward — and a broken +/// path is still perfectly timeable, which is how it would otherwise be published +/// as a fast number. +fn measure_one( + arm: &'static str, + id: &str, + tree: &Path, + out: &Path, + binary: &Path, +) -> Result { + let json = out.join(format!("{id}.json")); + let args: Vec = vec![ + String::from("--warmup"), + env_or(WARMUP_VAR, DEFAULT_WARMUP), + String::from("--runs"), + env_or(RUNS_VAR, DEFAULT_RUNS), + String::from("--shell=none"), + String::from("--export-json"), + json.to_string_lossy().into_owned(), + String::from("--style"), + String::from("none"), + format!("{} check", binary.display()), + ]; + if !run(tree, "hyperfine", &args, &[])? { + bail!("perf-acquire: measuring arm {id} failed. No records."); + } + + let text = std::fs::read_to_string(&json) + .with_context(|| format!("perf-acquire: could not read arm {id}. No measurement."))?; + let parsed: serde_json::Value = serde_json::from_str(&text) + .with_context(|| format!("perf-acquire: arm {id} did not parse. No measurement."))?; + let result = parsed + .get("results") + .and_then(serde_json::Value::as_array) + .and_then(|results| results.first()) + .ok_or_else(|| anyhow::anyhow!("perf-acquire: arm {id} carried no results."))?; + record(arm, id, result) +} + #[cfg(test)] mod tests { use super::*; @@ -1016,4 +1452,83 @@ mod tests { assert!(message.contains("crates/"), "{message}"); assert!(message.contains("Cargo.lock"), "{message}"); } + + // --- the acquisition sweep (CLOUD-935, ported under CLOUD-1229) ---------- + + #[test] + fn the_sweep_points_are_read_in_the_order_they_were_declared() { + // The FIRST entry is the ratio base, so order is load-bearing rather than + // cosmetic: a parse that sorted or deduplicated would silently re-base + // every ratio the sweep prints. + let (Ok(swept), Ok(reversed)) = (parse_ns("1,16,64,256"), parse_ns("256, 1")) else { + panic!("a well-formed declaration parses"); + }; + assert_eq!(swept, vec![1, 16, 64, 256]); + assert_eq!(reversed, vec![256, 1]); + } + + #[test] + fn a_declaration_that_names_no_sweep_point_is_refused() { + // COULD-NOT-LOOK RATHER THAN AN EMPTY SWEEP. An empty list would print no + // arms, no ratios and exit 0 — a measurement that did not happen wearing + // a clean run's clothes, which is the one shape a bench harness must not + // produce (CLOUD-1208 measured this class twice). + assert!(parse_ns("").is_err()); + assert!(parse_ns(",,").is_err()); + assert!(parse_ns("1,many").is_err()); + } + + /// The rendered reading, byte for byte. + /// + /// House-style §6 applies to a bench verb's output too, and the fields here + /// are `Record`'s own plus three lines this harness adds. Pinning the bytes is + /// what stops the ratio precision or the field order drifting under a reader + /// who is diffing two runs. + #[test] + fn the_reading_renders_arms_then_ratios_then_the_spread_then_the_term() { + let arm = |path: &str, p50: f64| Record { + arm: "acquire", + path: path.to_owned(), + p50, + p95: 6.0, + mean: 5.0, + runs: 100, + }; + let sweep = Sweep { + arms: vec![arm("acquire-1", 4.75), arm("acquire-256", 6.12)], + ratios: vec![(String::from("acquire-256/acquire-1"), 6.12 / 4.75)], + nulls: vec![0.958, 1.022], + per_document: Some((5.37, 255)), + }; + assert_eq!( + sweep.to_string(), + "arm=acquire path=acquire-1 p50=4.75 p95=6 mean=5 runs=100\n\ + arm=acquire path=acquire-256 p50=6.12 p95=6 mean=5 runs=100\n\ + ratio=acquire-256/acquire-1 value=1.288\n\ + ratio=null0 value=0.958\n\ + ratio=null1 value=1.022\n\ + null-spread low=0.958 high=1.022 pairs=2\n\ + per-document us=5.37 over=255 documents\n" + ); + } + + #[test] + fn a_sweep_with_no_null_pairs_prints_no_spread_it_cannot_have() { + // ANTI-VACUITY on the line above. `null-spread` over an empty set would + // render `low=inf high=-inf`, which reads as a measured width and is the + // opposite of one — the fold's identities leaking into a published number. + let sweep = Sweep { + arms: Vec::new(), + ratios: Vec::new(), + nulls: Vec::new(), + per_document: None, + }; + assert_eq!(sweep.to_string(), ""); + } + + // The two cases over what `sweep_fixture` WRITES live in + // `crates/batten/tests/perf_acquire.rs` rather than here, and the reason is + // the workspace lint rather than a preference: reading a file back is a + // `Result`, and no module under `src/` waives `unwrap_used`. That builder is + // public for exactly this, so the assertion loses nothing by moving. } diff --git a/crates/batten/src/policy/presets/shell-hygiene/sibling-resolves.rego b/crates/batten/src/policy/presets/shell-hygiene/sibling-resolves.rego index 155f56ba1..506411cad 100644 --- a/crates/batten/src/policy/presets/shell-hygiene/sibling-resolves.rego +++ b/crates/batten/src/policy/presets/shell-hygiene/sibling-resolves.rego @@ -88,15 +88,32 @@ expansion_names(line) := {m[1] | # A variable whose assignment reaches for `/..` is excluded: it holds the PARENT # of this script's directory, so a name hung off it is not a sibling and would # resolve against the wrong prefix. -dir_vars(path) := {m[1] | - some line in input.tree.lines[path] - script_dir_line(line) - not contains(line, "/..") - some m in regex.find_all_string_submatch_n(`^[\t ]*([A-Za-z_][A-Za-z0-9_]*)=`, line, -1) +# +# A PARTIAL RULE KEYED BY PATH, NOT A FUNCTION, AND THAT IS A PERFORMANCE +# CONTRACT RATHER THAN A STYLE CHOICE (CLOUD-1217). This scans every line of the +# file, and `var_names` below is evaluated once per line — so as a FUNCTION it was +# re-scanned per line and the arm cost O(lines²) for every selected file. Over +# this repository's own ~140 shell programs that measured **28.2s**, which was 42% +# of `batten enforce` and the single largest rule in the set, dwarfing every +# spawning row beside it. +# +# A partial rule is evaluated once and indexed, so the same predicate costs +# O(lines). The rule body is otherwise unchanged, and the module's own cases are +# what prove that: the arm-3 tests below pin both the finding and the allow, so a +# rewrite that changed WHAT this selects fails at load rather than quietly +# widening or narrowing a preset that ships to every consumer. +dir_vars[path] := names if { + some path, _ in input.tree.lines + names := {m[1] | + some line in input.tree.lines[path] + script_dir_line(line) + not contains(line, "/..") + some m in regex.find_all_string_submatch_n(`^[\t ]*([A-Za-z_][A-Za-z0-9_]*)=`, line, -1) + } } var_names(path, line) := {m[1] | - some variable in dir_vars(path) + some variable in dir_vars[path] some m in regex.find_all_string_submatch_n( sprintf(`\$\{?%s\}?/%s`, [variable, name_capture]), line, diff --git a/crates/batten/src/rules.rs b/crates/batten/src/rules.rs index 4c7ad89f6..a07d93482 100644 --- a/crates/batten/src/rules.rs +++ b/crates/batten/src/rules.rs @@ -42,6 +42,7 @@ use std::collections::{BTreeMap, BTreeSet}; use std::fs; use std::path::Path; +use std::sync::Mutex; use std::sync::atomic::{AtomicUsize, Ordering}; use clap::ValueEnum; @@ -4930,6 +4931,22 @@ fn isolate(body: impl FnOnce() -> anyhow::Result>) -> Isolat } } +/// What one rule cost, as a pointer rather than a payload (non-negotiable rule 4). +/// +/// A rule id, two counts and a duration. No path list and no scanned bytes — the +/// census answers *how much*, and the findings answer *where*. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct RuleCost { + /// The rule's id. + pub rule: String, + /// Wall clock for this rule's whole evaluation, including whatever it spawned. + pub elapsed: std::time::Duration, + /// Files this rule caused to be read. + pub files_read: usize, + /// Bytes those reads returned. + pub bytes_read: usize, +} + /// The name of the verb that runs process-spawning rule kinds, quoted in the /// refusal [`run_static`] emits. Named once so the message and the surface /// cannot drift. @@ -5442,6 +5459,9 @@ fn run( }; let mut scan = Scan::default(); + // CLEARED, not appended to: the census describes THIS run, and a caller + // reading rows from the previous one would be reading a different tree. + costs_lock().clear(); evaluate_rules(rules, root, &inputs, &mut scan)?; // BEFORE the sort, deliberately (CLOUD-396): the sort is what makes the // output byte-stable, so a dedup running after it would be reading an order @@ -5500,6 +5520,23 @@ fn evaluate_rules( // // The scan's three mutable maps are destructured inside the block so // their borrows end before `not_evaluated` is written below. + // THE CENSUS IS TAKEN AROUND THE DISPATCH, not inside each kind + // (CLOUD-1217). One site sees every rule, so a kind added later is + // measured without anyone remembering to instrument it — the inverse of + // the state this row found, where nothing reported and the largest item + // in CI was a silent span. + // + // Deltas over the process-global counters rather than an accumulator + // threaded through nine read sites. Sound because this loop is serial, + // which `.claude/rules/rust.md` records as a measured verdict rather + // than an accident. + // + // AROUND `isolate` RATHER THAN AROUND `run_rule`, which is where the + // precondition skip above puts it: a rule held back by an unmet + // requirement never enters the body, so it has no cost to report and a + // zero row for it would read as "evaluated, and free". + let started = std::time::Instant::now(); + let (files_before, bytes_before) = (files_read(), bytes_read()); let outcome = { let Scan { findings, @@ -5509,6 +5546,12 @@ fn evaluate_rules( } = &mut *scan; isolate(|| run_rule(rule, root, inputs, findings, attributed, classes)) }; + costs_lock().push(RuleCost { + rule: rule.id.clone(), + elapsed: started.elapsed(), + files_read: files_read().saturating_sub(files_before), + bytes_read: bytes_read().saturating_sub(bytes_before), + }); match outcome { Isolated::Evaluated => {} Isolated::NotEvaluated(why) => { @@ -5814,11 +5857,7 @@ fn run_rule( return Ok(Some(NotObserved::RuleSkipped)); } match rule.kind { - RuleKind::Forbid => { - for path in matched { - forbid_in_file(rule, root, path, findings)?; - } - } + RuleKind::Forbid => forbid_in_files(rule, root, &matched, findings)?, RuleKind::Command => command_rule(rule, root, &matched, findings)?, RuleKind::Document => { for path in matched { @@ -6001,6 +6040,86 @@ pub fn documents_acquired() -> usize { DOCUMENTS_ACQUIRED.load(Ordering::Relaxed) } +/// How many working-tree files this process has read on a rule's behalf +/// (CLOUD-1217), and how many bytes those reads returned. +/// +/// **Two counters beside [`DOCUMENTS_ACQUIRED`] rather than one widened counter**, +/// because they answer different questions. That one is bounded to the document +/// cache and is what `document_read_count.rs` asserts; these cover every read a +/// rule kind performs for itself — the `forbid` scan, both of a `ratchet`'s +/// passes, and the two conservation walks — which is precisely the population the +/// cache never covered. +/// +/// **Monotonic and process-global, read as a DELTA around one rule**, which is +/// what makes the per-rule attribution possible without threading an accumulator +/// through nine call sites. Sound only because [`run`]'s loop is serial; a +/// concurrent second run in the same process would interleave, which is why the +/// test that reads them lives in its own binary, exactly as +/// `document_read_count.rs` does and for the same reason. +static FILES_READ: AtomicUsize = AtomicUsize::new(0); +static BYTES_READ: AtomicUsize = AtomicUsize::new(0); + +/// Record one read of `bytes` bytes against the counters above. +/// +/// Called at each site that reads on a rule's behalf, after the read has +/// succeeded — a read that failed spent an `open` and returned nothing, and +/// counting it would make an absent file indistinguishable from an empty one. +fn count_read(bytes: usize) { + FILES_READ.fetch_add(1, Ordering::Relaxed); + BYTES_READ.fetch_add(bytes, Ordering::Relaxed); +} + +/// How many files this process has read on a rule's behalf. +#[must_use] +pub fn files_read() -> usize { + FILES_READ.load(Ordering::Relaxed) +} + +/// How many bytes those reads returned. +#[must_use] +pub fn bytes_read() -> usize { + BYTES_READ.load(Ordering::Relaxed) +} + +/// What each rule of the LAST run cost (CLOUD-1217), in declaration order. +/// +/// **Out of [`Scan`] deliberately, and the gate is what settled it.** It was a +/// `pub` field on that struct for one commit; `batten semver check` refused the +/// branch with `constructible_struct_adds_field`, because a consumer +/// constructing `Scan { .. }` cannot keep compiling across the addition. The +/// refusal was right on the API, and reading it also settled the design +/// question: a census is a MEASUREMENT ABOUT a run, not part of the run's value, +/// and the tell was that keeping it on `Scan` forced a hand-written `PartialEq` +/// that skipped the clock so scan equality would not become timing-dependent. A +/// value type that has to lie about one of its fields to stay comparable is +/// carrying something that is not its own. +/// +/// **Cleared at the top of every [`run`], not appended to forever**, which is the +/// one thing this owes over the two counters above: they are monotonic and read +/// as a delta, and a per-rule LIST read that way would hand a caller the previous +/// run's rows as well. Reading it therefore means "the run that just finished", +/// and a caller reads it immediately after the runner returns. +/// +/// Process-global for the counters' reason, with the counters' consequence: the +/// test that reads it lives in its own binary. +static RULE_COSTS: Mutex> = Mutex::new(Vec::new()); + +/// Take the lock, treating a poisoned one as the data it still holds. +/// +/// A panic in another thread says nothing about whether this census is readable, +/// and the workspace lints refuse an `unwrap` on a reachable path besides. +fn costs_lock() -> std::sync::MutexGuard<'static, Vec> { + RULE_COSTS + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) +} + +/// What each rule of the last [`run`] cost, in declaration order. +#[must_use] +pub fn rule_costs() -> Vec { + costs_lock().clone() +} + /// **The one function that acquires a document** (CLOUD-849). /// /// Every `Fact::Document` in this crate is read and parsed here and nowhere @@ -7641,6 +7760,12 @@ fn ratchet_rule( let mut base_text: BTreeMap = BTreeMap::new(); let retires_with = rule.retires_with.as_deref(); crate::git::for_each_blob_at_rev(root, base, glob, |path, text| { + // Counted here rather than inside the walker: a blob decompressed out of + // the object store is a read this rule caused, and the census must not + // report a ratchet as cheaper than a forbid merely because its bytes came + // from git. Counting it in `git.rs` would also point that module at this + // one, which the layering table declares the wrong way round. + count_read(text.len()); base_counts.insert(path.to_owned(), text.matches(pattern).count()); // Held only for the columns that read it: a ratchet with neither // `retires_with` nor `conserves` must not start buffering the base @@ -7663,6 +7788,7 @@ fn ratchet_rule( let mut working_declared: BTreeSet<&str> = BTreeSet::new(); for path in matched { let text = fs::read_to_string(root.join(path)).unwrap_or_default(); + count_read(text.len()); let count = text.matches(pattern).count(); working_count += count; working_counts.insert(path.as_str(), count); @@ -8173,6 +8299,7 @@ fn claimed_cases(root: &Path, conserves: &Conserves, files: &[String]) -> Claime let Ok(text) = fs::read_to_string(root.join(path)) else { continue; }; + count_read(text.len()); for (index, line) in text.lines().enumerate() { let trimmed = line.trim_start(); for &(arm, token) in &arms { @@ -8454,6 +8581,7 @@ fn conserve_case_names( // for the cases it dropped and nothing for the ones still standing, so // the surviving names have to be read rather than assumed absent. let survivors = fs::read_to_string(root.join(path)).unwrap_or_default(); + count_read(survivors.len()); let mapping = Mapping { conserves, claimed, @@ -9264,49 +9392,68 @@ impl Matcher { } } -fn forbid_in_file( +/// Evaluate a [`RuleKind::Forbid`] row against every path its glob selected. +/// +/// **The whole matched set, not one path** (CLOUD-1217), and that boundary is +/// the fix rather than a refactor. The predecessor took one `rel_path` and was +/// called once per file from [`run_rule`], so [`Matcher::for_rule`] — and with +/// it `Regex::new` — ran once per *(rule, file)* pair. Over this repository's own +/// ruleset that is ~3 300 compilations per run, of 17 expressions. +/// +/// The comment that used to sit on that call said the compile was hoisted out of +/// the *line* loop because "`Regex::new` is the expensive half". It was right +/// about the cost and stopped one loop short; the expression is a property of the +/// ROW, so it is compiled where the row is. +fn forbid_in_files( rule: &Rule, root: &Path, - rel_path: &str, + paths: &[&String], findings: &mut Vec, ) -> anyhow::Result<()> { - let contents = match fs::read(root.join(rel_path)) { - Ok(bytes) => bytes, - Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()), - Err(err) => return Err(err.into()), - }; - let Ok(text) = String::from_utf8(contents) else { - return Ok(()); - }; - // Compiled once per file, never per line: an expression recompiled inside - // the loop would make the scan's cost a function of the tree's size times - // the pattern's, and `Regex::new` is the expensive half. + // Compiled once per RULE, never per file and never per line. // // `Rule::validate` has already refused a malformed expression and the // both-columns row, so these are defence in depth on the same reading // `run_rule` applies — the runner re-validates rather than trusting that - // every path reached it through the loader. + // every path reached it through the loader. Hoisting it here also moves that + // refusal from "once the first matched file is read" to "before any file is + // read", which is the direction a config fault should travel. let (matcher, exclude) = Matcher::for_rule(rule)?; let mode = span_mode(rule); - for (index, line) in text.lines().enumerate() { - // Excluded lines are dropped AFTER matching, never instead of it: the - // exclusion is about what a matched line turns out to be — a comment, - // a case pattern — not about narrowing what counts as a match. - if matcher.matches(line) && !exclude.as_ref().is_some_and(|re| re.is_match(line)) { - // The whole matched line is the span, which is exactly what the - // churn pack hashed test-side before this existed — so its fixtures - // keep their assertions, and that unchanged-ness is the evidence the - // engine picks the same span. - let default = identity::code_fingerprint(&rule.id, rel_path, line, mode)?; - findings.push(Finding { - rule: rule.id.clone(), - severity: rule.severity(), - path: rel_path.to_owned(), - line: Some(index + 1), - identity: identity_of(rule, identity::FindingKind::Code, default), - check: rule.settling_check().unwrap_or(Check::Reevaluate), - remediation: rule.remediation(), - }); + for rel_path in paths { + let rel_path = rel_path.as_str(); + let contents = match fs::read(root.join(rel_path)) { + Ok(bytes) => bytes, + // A path the walk listed and the read cannot find is a tree that + // moved under us, not a finding — the same silence as before, per + // file rather than per call. + Err(err) if err.kind() == std::io::ErrorKind::NotFound => continue, + Err(err) => return Err(err.into()), + }; + count_read(contents.len()); + let Ok(text) = String::from_utf8(contents) else { + continue; + }; + for (index, line) in text.lines().enumerate() { + // Excluded lines are dropped AFTER matching, never instead of it: the + // exclusion is about what a matched line turns out to be — a comment, + // a case pattern — not about narrowing what counts as a match. + if matcher.matches(line) && !exclude.as_ref().is_some_and(|re| re.is_match(line)) { + // The whole matched line is the span, which is exactly what the + // churn pack hashed test-side before this existed — so its fixtures + // keep their assertions, and that unchanged-ness is the evidence the + // engine picks the same span. + let default = identity::code_fingerprint(&rule.id, rel_path, line, mode)?; + findings.push(Finding { + rule: rule.id.clone(), + severity: rule.severity(), + path: rel_path.to_owned(), + line: Some(index + 1), + identity: identity_of(rule, identity::FindingKind::Code, default), + check: rule.settling_check().unwrap_or(Check::Reevaluate), + remediation: rule.remediation(), + }); + } } } Ok(()) diff --git a/crates/batten/tests/acquisition_metric.rs b/crates/batten/tests/acquisition_metric.rs index b08683d89..a187e19ad 100644 --- a/crates/batten/tests/acquisition_metric.rs +++ b/crates/batten/tests/acquisition_metric.rs @@ -20,13 +20,19 @@ //! CLOUD-935 says the distinct stamp must be **asserted rather than assumed**, //! and this is that assertion. //! -//! # Why over `mise.toml` rather than over the helper +//! # Why over `mise.toml` rather than over the harness //! -//! The stamp is set by the task, not by the Python. A test reading the helper -//! would pass while the task that invokes it lost the variable — and the task is -//! the only caller, so the task is where the claim lives. +//! The stamp is set by the task, not by the measurement. A test reading the +//! harness would pass while the task that invokes it lost the variable — and the +//! task is the only caller, so the task is where the claim lives. //! `policy/command-task-defined.rego` already establishes `mise.toml` as a parsed //! document this repository reasons over; this is the same read in Rust. +//! +//! The harness was `bench/acquisition/sweep.py` until CLOUD-1229 retired it into +//! `crates/batten/examples/acquisition-bench.rs`. The anti-vacuity case below +//! moved with it, and moving it is the whole of what kept the case honest: it +//! names the invocation that actually runs the sweep, so a stamp set on some +//! other task cannot satisfy it. // Panicking on setup failure is the idiomatic way for a test to fail loudly. #![allow(clippy::unwrap_used, clippy::expect_used)] @@ -86,7 +92,7 @@ fn the_acquisition_series_is_stamped_with_its_own_metric() { fn the_stamp_is_set_on_the_task_that_runs_the_harness() { let body = task_body(); assert!( - body.contains("bench/acquisition/sweep.py"), + body.contains("--example acquisition-bench"), "the body carrying the stamp is the one invoking the measurement: {body}" ); } diff --git a/crates/batten/tests/acquisition_sweep.rs b/crates/batten/tests/acquisition_sweep.rs new file mode 100644 index 000000000..b78e9a028 --- /dev/null +++ b/crates/batten/tests/acquisition_sweep.rs @@ -0,0 +1,168 @@ +//! The acquisition sweep's contract, over the compiled engine (CLOUD-935, +//! CLOUD-1229). +//! +//! # Why this tier +//! +//! `crates/batten/src/perf.rs` unit-tests the parse and the rendering directly, +//! which is the right home for both: each is pure, and keeping them exercisable +//! without a benchmark runner is what lets them be asserted at all. The cases over +//! what the fixture builder WRITES are here instead, and for a lint reason rather +//! than a design one — reading a file back is a `Result`, and no module under +//! `src/` waives `unwrap_used`. +//! +//! What those cases cannot establish is the thing the whole measurement rests on: +//! **that the generated fixture is a tree the ENGINE accepts.** A `[[verdict]]` +//! row nothing raises, a module publishing the wrong rule name, a `documents` +//! array the engine never reads — every one of those makes `batten check` refuse +//! or no-op, and every arm then times a broken tree and still draws a tidy curve. +//! `.claude/rules/policy-modules.md` names that class: a dead gate and a clean +//! tree are byte-identical on the decision surface, and only a case over the +//! compiled binary tells them apart. +//! +//! # What is deliberately not here, and why it is not a gap +//! +//! The sweep itself is not run. It needs hyperfine, and `crates/batten/src/perf.rs` +//! records the measurement behind that: **the `windows` job installs none**, so a +//! case that ran the sweep would pass on two hosts and fail on the third — which +//! is exactly how `a_skip_exits_zero_and_prints_no_record` was once broken. The +//! same fact is why the harness is `crates/batten/examples/acquisition-bench.rs` +//! rather than a `perf` sub-verb: `tests/pointer_only.rs` sweeps every leaf verb +//! over a bare corpus and refuses one that exits 3, and could-not-look is the only +//! honest answer a sweep has there. +//! +//! # What this replaced, and why there is no ledger arm for it +//! +//! `bench/acquisition/sweep.py` is retired here under CLOUD-1229. It was 327 lines +//! of Python driven by a one-line task, and its own header argued the shape was +//! forced by `shell-retirement` refusing an added shell rule. A second author read +//! that argument and added a third helper for the identical stated reason +//! (CLOUD-1208). The campaign's subject is authored SHELL because that is what it +//! was built to retire — a statement about its reach, never a licence for what +//! sits beside it. +//! +//! It carries **no** `// changed:` marker, and that absence is the point rather +//! than an omission. Those arms are `shell-retirement`'s and `[rule.conserves]`'s +//! ledger over a governed file's death, and the deleted path was governed by +//! neither — not under `mise-tasks/`, not a `.bats` suite, watched by nothing. +//! Writing an arm for it would put a row in a ledger whose subject it never was. +//! The helper also carried no test tier of its own; it was a script with a +//! docstring, and what moved is the reasoning in that docstring, into the doc +//! comments on `perf::acquire` and `perf::sweep_fixture` and into the cases here +//! and in `perf.rs`'s own module. + +// Panicking on setup failure is the idiomatic way for a test to fail loudly. +#![allow(clippy::unwrap_used, clippy::expect_used)] + +mod common; + +use common::{Fixture, run, stderr, stdout}; + +#[test] +fn the_generated_fixture_is_a_tree_the_engine_accepts() { + // THE LOAD-BEARING CASE, and the reason this file exists. Every arm of the + // sweep times `batten check` over one of these trees, so a fixture the engine + // refuses is a measurement of the refusal — and a fixture whose row the engine + // never reads is a measurement of nothing, reported at three decimal places. + // + // Built through the harness's OWN builder rather than re-spelled here: a + // second spelling of the fixture is a second authority, and the two can + // disagree about exactly the thing this asserts. + let root = Fixture::new("perf-acquire-fixture").git().build(); + let tree = root.join("swept"); + batten::perf::sweep_fixture(&tree, 4).expect("the sweep's own fixture builder"); + + let output = run(&tree, &["check"]); + assert!( + output.status.success(), + "the swept fixture must be a tree `check` accepts, or every arm times a \ + refusal: {}", + stderr(&output) + ); +} + +/// ANTI-VACUITY for the case above, and it is the half that discriminates. +/// +/// `check` exits 0 over a tree with no rules at all, so the success above proves +/// nothing on its own — a builder that wrote an empty `batten.toml` would satisfy +/// it, and so would one whose `documents` array the engine never read. That +/// second one is the defect `.claude/rules/policy-modules.md` records from the +/// field: OpenTelemetry's `weaver` printed `✔ No policy violation`, exit 0, over a +/// knowingly-broken registry, because its module read a key the schema never +/// built. +/// +/// So this drives the predicate from the other side. The module fires on a +/// declared document carrying a `stray` key; the generated documents carry none, +/// which is why every arm is clean. Put the key into ONE of them and the finding +/// has to appear — which can only happen if the row loaded, the declared path was +/// acquired, and the module read the acquired node. +#[test] +fn the_swept_row_reads_the_documents_it_declares() { + let root = Fixture::new("perf-acquire-registered").git().build(); + let tree = root.join("swept"); + batten::perf::sweep_fixture(&tree, 4).expect("the sweep's own fixture builder"); + std::fs::write(tree.join("config2.toml"), "quiet = true\nstray = true\n") + .expect("seed the sentinel the module fires on"); + + let output = run(&tree, &["check"]); + assert_eq!( + output.status.code(), + Some(2), + "a seeded sentinel is a policy verdict: {}", + stderr(&output) + ); + let said = stdout(&output) + &stderr(&output); + assert!( + said.contains("acquisition-bench"), + "the swept row must be the one that fired — anything else means the sweep \ + is timing a rule that never reads its documents: {said}" + ); + assert!( + said.contains("config2.toml"), + "and it must point at the seeded document, not at the row: {said}" + ); +} + +#[test] +fn the_fixture_declares_exactly_the_documents_it_was_asked_for() { + // The confound the experiment is built to avoid, asserted rather than trusted: + // ONE rule, ONE bundle, ONE module, and only the `documents` array grows. A + // builder that added a row per document would still draw a tidy curve while + // pricing a module compile and an evaluation at every step. + let root = Fixture::new("perf-acquire-shape").git().build(); + let tree = root.join("swept"); + batten::perf::sweep_fixture(&tree, 3).expect("the sweep's own fixture builder"); + + let authority = + std::fs::read_to_string(tree.join("batten.toml")).expect("the fixture authority"); + assert_eq!(authority.matches("[[rule]]").count(), 1, "{authority}"); + assert!( + authority.contains(r#"documents = ["config0.toml", "config1.toml", "config2.toml"]"#), + "{authority}" + ); + assert!(tree.join("policy-acquisition/gate.rego").is_file()); + assert!(tree.join("config2.toml").is_file()); + assert!(!tree.join("config3.toml").exists()); +} + +#[test] +fn the_floor_arm_carries_neither_a_rule_nor_the_verdict_it_would_raise() { + // Both halves in one case, because they are one requirement: `[[verdict]]` runs + // in BOTH directions, so a floor arm declaring the class with no rule to raise + // it would fail the load outright and time nothing at all. + let root = Fixture::new("perf-acquire-floor").git().build(); + let tree = root.join("swept"); + batten::perf::sweep_fixture(&tree, 0).expect("the sweep's own fixture builder"); + + let authority = + std::fs::read_to_string(tree.join("batten.toml")).expect("the fixture authority"); + assert!(!authority.contains("[[rule]]"), "{authority}"); + assert!(!authority.contains("V-ACQUISITION-BENCH"), "{authority}"); + + let output = run(&tree, &["check"]); + assert!( + output.status.success(), + "the floor arm must load and run, or the baseline every ratio is taken \ + against is a refusal: {}", + stderr(&output) + ); +} diff --git a/crates/batten/tests/dev_profile.rs b/crates/batten/tests/dev_profile.rs new file mode 100644 index 000000000..912596bf4 --- /dev/null +++ b/crates/batten/tests/dev_profile.rs @@ -0,0 +1,164 @@ +//! `[profile.dev]` declares what CLOUD-1211's adopted arm set. +//! +//! # Why a case rather than a comment +//! +//! CLOUD-1211's §7 asks for "a case asserting that whatever the adopted arms set +//! is what the committed profile declares — the shape `msrv-pin-agreement` uses +//! to hold two authorities together — so a later edit that drops a setting is a +//! finding rather than a silent regression." +//! +//! The regression this catches is silent by construction. Dropping the +//! dependency override, or letting `debug` fall back to cargo's dev default of +//! `2`, costs nothing a test run can observe: the suite still passes, every gate +//! still exits 0, and the only symptom is `target/debug` growing back until a +//! session runs out of disk — which arrives as an unrelated rustc IO error inside +//! somebody else's test run, the misattribution CLOUD-766 records. +//! +//! # The three arms, and why the biggest number is the rejected one +//! +//! Each a cold `mise run test:cargo` over a cleared `target/debug`, this +//! container, 2026-08-30, counting `target/debug/deps`' extension-less linked +//! binaries — the population `crates/batten/src/prune.rs:262-269` reads: +//! +//! | arm | artifacts | linked bytes | mean | +//! | -------------------------------- | --------- | ------------ | -------- | +//! | `debug = 1` (baseline) | 125 | 15.53 GB | 124.3 MB | +//! | `debug = 0` whole profile | 123 | 1.60 GB | 13.0 MB | +//! | `debug = 1` + deps `0` (adopted) | 125 | 6.82 GB | 54.5 MB | +//! +//! **The middle arm is 9.7x and is refused anyway.** It drops debuginfo from the +//! entire dev profile, `batten`'s own code included, so a panicking test reports +//! a backtrace with no line numbers — the diagnostic a reader needs at exactly +//! the moment it is gone. It was briefly committed and reverted; this file is +//! what makes that reversal hold. +//! +//! The adopted arm takes 2.3x off the bytes by stripping the dependency closure, +//! which is where they were, while every `batten` frame keeps its file and line. + +// Panicking on setup failure is the idiomatic way for a test to fail loudly. +#![allow(clippy::unwrap_used, clippy::expect_used)] + +mod common; + +/// What the adopted arm sets for WORKSPACE code. Spelled as an integer because +/// that is how cargo reads it; `debug = true` is `2` and `debug = false` is `0`, +/// so a later edit spelling it as a bool is judged on the value, not the syntax. +const ADOPTED_DEBUG: i64 = 1; + +/// What the adopted arm sets for the DEPENDENCY closure, which is where the +/// bytes actually were — 124.3 MB to 54.5 MB per linked binary. +const ADOPTED_DEPENDENCY_DEBUG: i64 = 0; + +/// The glob cargo spells "every dependency, but not workspace members". +const DEPENDENCY_GLOB: &str = "*"; + +fn manifest() -> toml::Value { + let text = std::fs::read_to_string(common::at_root("Cargo.toml")) + .expect("the workspace manifest is where the profiles are declared"); + toml::from_str(&text).expect("Cargo.toml parses as TOML") +} + +fn dev_profile() -> toml::Value { + manifest() + .get("profile") + .and_then(|profile| profile.get("dev")) + .cloned() + .expect("[profile.dev] is declared") +} + +/// `debug` normalised across the two spellings cargo accepts. Panics on an +/// absent key rather than defaulting, which is the reading +/// `an_absent_debug_key_is_not_read_as_the_adopted_value` pins. +fn declared_debug(profile: &toml::Value) -> i64 { + let value = profile.get("debug").expect( + "this profile declares `debug` — an absent key is cargo's own default, \ + which is the regression this asserts against", + ); + match value { + toml::Value::Integer(level) => *level, + toml::Value::Boolean(true) => 2, + toml::Value::Boolean(false) => 0, + other => panic!("`debug` is neither an integer nor a bool: {other:?}"), + } +} + +#[test] +fn workspace_code_keeps_its_line_tables() { + let debug = declared_debug(&dev_profile()); + assert_eq!( + debug, ADOPTED_DEBUG, + "[profile.dev] debug is {debug}, not the adopted {ADOPTED_DEBUG}. This is \ + the half that must NOT be traded for bytes: at `debug = 0` the whole dev \ + profile loses debuginfo and a panicking test reports a backtrace with no \ + line numbers. That arm was measured at 9.7x off the artifacts, committed, \ + and reverted for exactly this reason (CLOUD-1211). If this is a deliberate \ + change, move the constant and say why in the same commit." + ); +} + +#[test] +fn the_dependency_closure_carries_no_debuginfo() { + let debug = manifest() + .get("profile") + .and_then(|profile| profile.get("dev")) + .and_then(|dev| dev.get("package")) + .and_then(|package| package.get(DEPENDENCY_GLOB)) + .map(declared_debug) + .expect( + "[profile.dev.package.\"*\"] is declared — without it every dependency \ + carries debuginfo again and the linked binaries go back to ~124 MB", + ); + assert_eq!( + debug, ADOPTED_DEPENDENCY_DEBUG, + "[profile.dev.package.\"*\"] debug is {debug}, not the adopted \ + {ADOPTED_DEPENDENCY_DEBUG}. This override is where the byte saving comes \ + from — 15.53 GB to 6.82 GB across 125 linked artifacts — and dropping it \ + is silent until a session runs out of disk (CLOUD-766)." + ); +} + +/// ANTI-VACUITY, and it is the case that would actually have caught the drift. +/// Both assertions above pass over a profile that declares the key at the right +/// value; neither would notice a `declared_debug` loosened to a defaulting +/// lookup, which would read an ABSENT key as cargo's default and report it as +/// satisfied. This pins the panicking read. +#[test] +fn an_absent_debug_key_is_not_read_as_the_adopted_value() { + let fixture: toml::Value = + toml::from_str("[profile.dev]\nincremental = true\n").expect("fixture parses"); + let profile = fixture + .get("profile") + .and_then(|profile| profile.get("dev")) + .expect("the fixture declares [profile.dev]"); + + assert!( + profile.get("debug").is_none(), + "the fixture is the absent-key case this asserts over" + ); + let caught = std::panic::catch_unwind(|| declared_debug(profile)); + assert!( + caught.is_err(), + "an absent `debug` is cargo's own default, not the adopted value — reading \ + it as satisfied is exactly the silent regression this file refuses" + ); +} + +/// `[profile.dist]` and `[profile.release]` are out of CLOUD-1211's scope: they +/// build the shipped artifact, and a test-loop change must not reach them. This +/// pins that boundary rather than trusting the commit that drew it. +#[test] +fn the_shipped_profiles_are_untouched_by_the_test_loop_arm() { + let parsed = manifest(); + let profiles = parsed.get("profile").expect("[profile] is declared"); + + for shipped in ["release", "dist"] { + let profile = profiles + .get(shipped) + .unwrap_or_else(|| panic!("[profile.{shipped}] is declared")); + assert!( + profile.get("debug").is_none(), + "[profile.{shipped}] gained a `debug` key — CLOUD-1211 is a test-loop \ + change and the shipped artifact's profile is explicitly out of its scope" + ); + } +} diff --git a/crates/batten/tests/rule_cost_census.rs b/crates/batten/tests/rule_cost_census.rs new file mode 100644 index 000000000..7ac351927 --- /dev/null +++ b/crates/batten/tests/rule_cost_census.rs @@ -0,0 +1,209 @@ +//! CLOUD-1217: the engine reports what each rule cost, so a slow gate is +//! attributable from its own output. +//! +//! **Why this exists at all.** `batten-check` ran 465s of a 1327s CI job and +//! emitted two lines. No rule kind reported its own duration and every +//! `command`-rule child has `Stdio::null()` on both streams, so the largest item +//! in this repository's CI was unattributable *by construction*. Two sessions in +//! a row attributed it confidently and wrongly — once to `no-secrets` (which +//! measures 3%) and once to `forbid`/`ratchet` read amplification (which +//! measures ~150ms) — before an instrument existed to ask. The census is that +//! instrument and this is its gate: without a case under it, it is a log rather +//! than a mechanism, which non-negotiable rule 2 refuses. +//! +//! **Its own test binary, for `document_read_count.rs`'s reason exactly**: +//! `rules::files_read` and `rules::bytes_read` are process-global counters read +//! as a delta, so a sibling case reading a file in the same process would race +//! the deltas below under a harness that threads rather than forks. +//! +//! **Counts are the assertion, never the clock.** `RuleCost::elapsed` is a +//! measurement and varies run to run; the counts are deterministic. Asserting a +//! duration here would discriminate nothing, which is the standing rule in +//! `.claude/rules/rust.md`. +//! +//! Asserted through `run_static` — the surface a consumer reaches — rather than +//! by widening anything to `pub` for a test's convenience. + +// Panicking on setup failure is the idiomatic way for a test to fail loudly. +#![allow(clippy::unwrap_used, clippy::expect_used)] + +use std::fs; +use std::path::{Path, PathBuf}; + +use batten::rules::{self, Rule}; + +/// An empty vocabulary: every row here is a native `forbid`, which raises no +/// declared verdict token, and `load` refuses a table naming a token nothing +/// raises. +fn vocabulary() -> batten::policy::Vocabulary<'static> { + batten::policy::Vocabulary { + patterns: &[], + verdicts: &[], + recorders: &[], + } +} + +/// A `forbid` row over `glob`, looking for a literal that is never present — the +/// census is about what a rule READ, so a row that finds nothing still has to +/// report the files it opened to find that out. +fn row(id: &str, glob: &str) -> Rule { + serde_json::from_value(serde_json::json!({ + "id": id, + "kind": "forbid", + "scope": "tree", + "glob": glob, + "pattern": "a-literal-no-fixture-carries", + "severity": "deny", + })) + .expect("a tree-scoped forbid row the loader accepts") +} + +fn scratch(name: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!("batten-census-{name}-{}", std::process::id())); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(dir.join("policy")).expect("scratch"); + dir +} + +/// Write `count` files of known, distinct sizes and return their total bytes. +fn seed(root: &Path, count: usize) -> usize { + (0..count) + .map(|i| { + let body = "x".repeat(i + 1); + fs::write(root.join(format!("f{i}.txt")), &body).expect("fixture"); + body.len() + }) + .sum() +} + +#[test] +fn every_rule_gets_one_census_row_in_declaration_order() { + // A rule whose glob selects nothing is SKIPPED, and it still earns a row + // reporting zero. That is deliberate rather than incidental: "this rule cost + // nothing" and "this rule is missing from the report" are different answers, + // and collapsing them is how a rule that stopped running would look cheap. + // + // Fails by: pushing the cost inside the `if let Some(why)` arm, which drops + // every rule that ran clean. + let root = scratch("order"); + seed(&root, 2); + + rules::run_static( + &[ + row("reads-the-txt", "*.txt"), + row("matches-nothing", "*.no-such-extension"), + row("reads-the-txt-again", "*.txt"), + ], + &[], + vocabulary(), + &root, + ) + .expect("the read surface runs the rows"); + + let costs = rules::rule_costs(); + let ids: Vec<&str> = costs.iter().map(|cost| cost.rule.as_str()).collect(); + assert_eq!( + ids, + ["reads-the-txt", "matches-nothing", "reads-the-txt-again"], + "one census row per rule, in declaration order — a skipped rule included" + ); + let skipped = costs + .iter() + .find(|cost| cost.rule == "matches-nothing") + .expect("the skipped rule has a row"); + assert_eq!( + (skipped.files_read, skipped.bytes_read), + (0, 0), + "a rule that selected nothing read nothing, and says so rather than being absent" + ); + + let _ = fs::remove_dir_all(&root); +} + +#[test] +fn a_rule_reports_one_read_per_file_its_glob_selected() { + // THE PROPERTY THE CENSUS IS FOR. Attribution is only worth anything if the + // counts track what a rule actually opened, so this pins the count to the + // matched set and the byte total to those files' sizes. + // + // Fails by: dropping the `count_read` call in `forbid_in_files`, which makes + // both deltas zero while the rule still runs. + let root = scratch("counts"); + let bytes = seed(&root, 3); + + rules::run_static(&[row("reads-three", "*.txt")], &[], vocabulary(), &root) + .expect("the read surface runs the row"); + + let costs = rules::rule_costs(); + let cost = costs.first().expect("the row has a census entry"); + assert_eq!( + cost.files_read, 3, + "three matched files is three reads — the census counts what was opened" + ); + assert_eq!( + cost.bytes_read, bytes, + "the byte total is those files' own sizes, so a count cannot drift from what was read" + ); + + // ANTI-VACUITY, in the same function: a counter wired to a constant would + // satisfy the assertions above however the engine behaved. + let extra = "yyyy"; + fs::write(root.join("f3.txt"), extra).expect("fixture"); + rules::run_static(&[row("reads-four", "*.txt")], &[], vocabulary(), &root) + .expect("the read surface runs the row"); + let widened = rules::rule_costs(); + let widened = widened.first().expect("the row has a census entry"); + assert_eq!( + (widened.files_read, widened.bytes_read), + (4, bytes + extra.len()), + "adding a file to the glob moves both counts, so the assertions above assert something" + ); + + let _ = fs::remove_dir_all(&root); +} + +#[test] +fn the_census_describes_the_last_run_rather_than_accumulating() { + // THE ONE THING A PER-RULE LIST OWES OVER THE TWO COUNTERS IT IS BUILT FROM. + // `files_read`/`bytes_read` are monotonic and read as a delta; a list read + // that way would hand a caller the previous run's rows as well, so `run` + // clears the store before it fills it. A caller therefore reads "the run that + // just finished" rather than "every run this process has done". + // + // Fails by: dropping the `costs_lock().clear()` in `run`, which makes the + // second census six rows rather than one. + let root = scratch("perrun"); + seed(&root, 2); + + rules::run_static( + &[ + row("first", "*.txt"), + row("second", "*.txt"), + row("third", "*.txt"), + ], + &[], + vocabulary(), + &root, + ) + .expect("the read surface runs the rows"); + assert_eq!( + rules::rule_costs().len(), + 3, + "three rows, three census entries" + ); + + rules::run_static(&[row("alone", "*.txt")], &[], vocabulary(), &root) + .expect("the read surface runs the row"); + let after = rules::rule_costs(); + assert_eq!( + after.len(), + 1, + "the second run's census is its own, not appended to the first's" + ); + assert_eq!( + after[0].rule, "alone", + "and it names the rule that actually ran" + ); + + let _ = fs::remove_dir_all(&root); +} diff --git a/mise.toml b/mise.toml index 35c0fb054..0854bf9a5 100644 --- a/mise.toml +++ b/mise.toml @@ -1267,21 +1267,46 @@ description = "Measure tree-surface acquisition cost as declared-document count # until a number says otherwise" — and names the condition: bring a number about # RESOLUTION rather than projection. This is the harness that brings it. # -# AN INLINE BODY OVER A `mise-tasks/*.sh` PROGRAM, and that is forced rather than -# stylistic: `policy/shell-retirement.rego` refuses ADDING an authored shell rule -# at `deny` (`V-SHELL-RULE-ADDED`) with one `document` route and no override and -# no `bypass_env`. `[tasks.semver]`, `[tasks.prose-only-check]` and -# `[tasks.policy-test]` are inline for that same reason. The measurement itself -# lives in `bench/acquisition/sweep.py`, beside `bench/gates/classify.py` — the -# standing example of a Python bench helper in this tree. -# -# UNDER `bench/`, NOT `mise-tasks/`, and that is a correction rather than a -# preference. mise makes every executable under `mise-tasks/` a file task named -# by its basename AND its stem, so a helper there would have published a SECOND -# entry point — `mise run acquisition-bench` resolving to the bare script, with no -# `BENCH_METRIC` set and therefore stamping the invocation series' default. The -# assertion below would still have passed, over a task nobody ran. `bench/` is not -# a task directory, so there is exactly one way in. +# THE MEASUREMENT IS `batten perf acquire`, AND IT USED TO BE PYTHON (CLOUD-1229). +# This comment used to argue that `bench/acquisition/sweep.py` was forced: an +# authored shell rule cannot be ADDED (`V-SHELL-RULE-ADDED`, one `document` route, +# no override, no `bypass_env`), so the measurement went to the one language the +# retirement campaign does not watch. That reading was wrong in a way this line +# then propagated — a second author read it, followed the precedent for the +# identical stated reason, and added a third helper (CLOUD-1208). The campaign's +# subject is authored SHELL because that is what it was built to retire; that is a +# statement about its reach, never a licence for what sits beside it. +# +# So the sweep moved into `crates/batten/src/perf.rs`, which is where the paired +# measurement already lives and is the module `policy/spawn-adapters.rego` already +# places for exactly this class — a harness whose whole subject is what an +# EXTERNAL process costs, so the spawns are the thing rather than an +# implementation of it. It also takes the unpinned interpreter with it: `python` +# was never in `[tools]`, so every helper ran under whatever the host happened to +# have while `lock-complete` held every declared tool to three platforms. +# +# AN EXAMPLE TARGET RATHER THAN A VERB, and that is a refusal honoured rather than +# a preference. It was written as `perf acquire` first; +# `crates/batten/tests/pointer_only.rs` sweeps EVERY leaf verb over a bare fixture +# corpus and refuses one that exits 3, because "it failed internally, so what it +# did not emit proves nothing" — and a sweep with no benchmark runner and no built +# binary to time has could-not-look as its only honest answer there. `perf pair` +# survives that sweep because it has a real SKIP predicate; there is no analogue +# here, and inventing one to satisfy a census is the false green these gates exist +# to catch. `crates/batten/examples/acquisition-bench.rs` is the entry point, it +# spawns nothing, and `--all-targets` holds it to the same clippy bar as the rest. +# +# THE TASK BODY STAYS INLINE AND STAYS ONE LINE, which is the half of the old +# argument that survives: `inline-task-bodies-not-growing` holds the count of +# multi-line task bodies non-increasing, so the alternative was never a bigger +# body here. +# +# AND THIS COMMENT DELIBERATELY DOES NOT SPELL THAT ROW'S PATTERN. An earlier +# revision quoted the literal opener the ratchet counts, and the ratchet counted +# the comment — 31 to 32, refused, over a change that added no body at all. It is +# a substring pass, so a token inside a comment is a hit, which is the class +# `.claude/rules/scanning.md` records from CLOUD-843's two disagreeing passes. +# Describe the row here; do not write its needle. # # BENCH_METRIC IS THE LOAD-BEARING LINE. `mise-tasks/perf-record.sh` reads it from # the environment and stamps it into every series entry, defaulting to @@ -1295,8 +1320,12 @@ description = "Measure tree-surface acquisition cost as declared-document count # OFF THE LANDING PATH, like `perf` itself: it builds a release binary and spends # minutes in hyperfine, and it answers a question about a measurement rather than # about a commit. Nothing in `verify` or the hk gate calls it. +# `build:release` stays a dependency even though `cargo run --example` builds the +# example itself: what the sweep TIMES is the release `batten` binary, and an +# example target does not build it. Dropping this line makes every arm answer +# could-not-look on a clean tree. depends = ["build:release"] -run = "BENCH_METRIC=acquisition-wall-clock ./bench/acquisition/sweep.py" +run = "BENCH_METRIC=acquisition-wall-clock cargo run --quiet --release -p batten --example acquisition-bench" [tasks."install:local"] description = "Put the built binary where the hook registrations resolve it — `install.sh`'s own destination" @@ -1709,6 +1738,51 @@ depends = ["doctor --no-targets"] # changed no bytes still skips, and any content change anywhere re-runs. run = ''' if ./mise-tasks/step-receipt.sh check test:bats; then exit 0; fi +# BUILD ONCE, THEN FEED THE SEAM THE PROGRAMS ALREADY DECLARE (CLOUD-1198). +# +# This task's only build dependency was `doctor --no-targets` — the submodule +# checkout — so it NEVER built the binary. Every gate it drives that needs +# `batten` paid `cargo run` process startup per case, and worse, whichever +# artifact happened to be lying in `target/` decided what the suite tested. +# `tests/helpers.bash` asserted the opposite in a comment for its whole life: +# "test:bats builds the DEBUG binary", true only transitively via whatever ran +# before. That is CLOUD-592's and CLOUD-699's stale-binary class, and closing it +# is this change's case — the ~23s of avoided startup is the bonus, not the +# argument. `hooks-wiring-check.sh:212-214` insists the gate judge "the WORKING +# TREE's engine and config together, which is the pair that ships", so a freshly +# built binary is the CORRECTNESS precondition for the injection below rather +# than an optimisation detail. +# +# INLINE RATHER THAN A `depends`, for two measured reasons. A `depends` here +# re-runs in whatever process invokes this task, and when that is hk's step it is +# a CHILD mise process — the race CLOUD-220 records for `doctor` one clause up. +# And a build in the DAG would contend with the cargo chain for the target-dir +# lock `hk.pkl` deliberately serialises. `build:release` is the wrong task +# besides: it produces the RELEASE binary for the mediated-call hot path, while +# `helpers.bash:119-134` and `common/mod.rs:154` both resolve a debug one. +# +# Guarded explicitly: this body runs under `/bin/sh` with no `set -e`, so an +# unguarded build would leave the suite running against a stale artifact and +# reporting green — the exact failure this clause exists to remove. +if ! cargo build --quiet -p batten; then + echo "::error:: test:bats: the debug binary did not build, so the suite would run against a stale artifact or none. That is CLOUD-592/699's class and the reason this step builds at all." >&2 + exit 1 +fi +# BOTH SPELLINGS OF THE ONE SEAM. `$BATTEN_BIN` is honoured by seven +# `mise-tasks/` programs and by the Rust tier; `hooks-wiring-check.sh:219` +# declares the same seam under its own name for its `doctor hooks -J` call and +# only 2 of its 36 cases ever set it. Exported here so every already-honouring +# program is reached with ZERO governed-file edits — `V-SHELL-RULE-EDITED` +# refuses touching any of them, which is the whole reason this change lives in +# the ungoverned task and nowhere else. +BATTEN_BIN="$PWD/target/debug/batten" +if [ ! -x "$BATTEN_BIN" ]; then + echo "::error:: test:bats: $BATTEN_BIN is missing or not executable after a successful build — the seam would silently fall back to the stale-artifact resolution this change exists to close." >&2 + exit 1 +fi +export BATTEN_BIN +HOOKS_WIRING_DIAGNOSIS="$BATTEN_BIN doctor hooks -J" +export HOOKS_WIRING_DIAGNOSIS # WHICH SUITES, before how many cases (CLOUD-886). The glob that selects this # step is `mise-tasks/**`, so any byte under there ran all 151 suites — measured, # correcting one sentence in `land`'s lap-cap message bought a full matrix. diff --git a/policy/remedy-authorship.rego b/policy/remedy-authorship.rego index d90f7d06b..02c98ae3b 100644 --- a/policy/remedy-authorship.rego +++ b/policy/remedy-authorship.rego @@ -121,7 +121,7 @@ stderr_block[path][i] := line if { some path, ls in lines_of endswith(path, ".sh") some i, line in ls - some j in openers_for(path) + some j in openers_for[path] j < i closes_to_stderr(path, j, i) } @@ -129,10 +129,20 @@ stderr_block[path][i] := line if { # Every bare `{` on its own line: a brace group opener. A `{` in any other # position is a parameter expansion, a brace expansion, or a literal, and is not # a group — which is why the whole trimmed line must be the brace. -openers_for(path) := [j | - some j, line in lines_of[path] - trim_space(line) == "{" -] +# A PARTIAL RULE KEYED BY PATH, NOT A FUNCTION, FOR `sibling-resolves`'s REASON +# (CLOUD-1217). `stderr_block` binds this inside its own per-line loop, so as a +# function it re-scanned the whole file once per line and the rule cost +# O(lines**2) before `closes_to_stderr` scanned again. Measured over this +# repository's own tree at **11.0s**, the second-largest rule in the set. A +# partial rule is evaluated once and indexed; the selected openers are identical, +# which the cases below are what prove. +openers_for[path] := opens if { + some path, ls in lines_of + opens := [j | + some j, line in ls + trim_space(line) == "{" + ] +} # The group opened at `j` is still open at `i`, and its closer redirects to # stderr. A closer between them ends the group; the FIRST one is what counts. diff --git a/tests/helpers.bash b/tests/helpers.bash index 7d2ff73cc..65d2622ae 100644 --- a/tests/helpers.bash +++ b/tests/helpers.bash @@ -101,9 +101,16 @@ run_timeout() { # # Five suites carried the same chain — `$BATTEN_BIN`, then release, then debug, # first hit wins — and release-first is a measured false green. `test:bats` -# builds the DEBUG binary; a release binary left over from an earlier session -# shadows it, so a suite reports on a build that predates the change it exists to -# catch. Measured on this very change: `tests/review-answered.bats` passed all +# builds the DEBUG binary and exports `$BATTEN_BIN` at it (CLOUD-1198); a release +# binary left over from an earlier session shadows it, so a suite reports on a +# build that predates the change it exists to catch. +# +# THAT FIRST CLAUSE WAS FALSE WHEN IT WAS WRITTEN AND IS TRUE NOW, which is worth +# stating in that order. `test:bats` declared only `doctor --no-targets` and never +# built anything, so "builds the DEBUG binary" held only transitively, via whatever +# happened to run before — the stale-artifact class CLOUD-592 and CLOUD-699 each +# record from their own end. CLOUD-1198 made the sentence true by building in the +# task, so the fallback chain below is now a fallback rather than the live path. Measured on this very change: `tests/review-answered.bats` passed all # twelve cases against a release binary nine hours older than the code under # test, and `tests/fact-record-keying.bats` only failed loudly because it asserts # behaviour the stale build does not have.