perf(test): assert six rules with six rules, not with all 103 — twice over - #834
Conversation
CLOUD-1358 Two `cli.rs` cases are 644s of the Windows suite's 1693s — 38% — and they set an asymptotic floor no sharding can pass
Why Measured on the
Two cases are 644.0s — 38% of the suite. The distribution is even more extreme than CLOUD-1223's: 25% of summed CPU is 2 cases (0.1%), 50% is 62 cases (1.6%). Both are the shape CLOUD-1223 named and this pair escaped. Each reads the committed CLOUD-1223's closing acceptance bullet is refuted for these two. It reads: "The four The estimate that deferred this was wrong by 18x, and recording that is the point. These two were examined during PR #819 and declined as "~18s each, worth ~36s" — a local 4-core reading — with the trade stated as "saves ~9s each and costs the cross-rule sort-order property". The real figure is 644s. They set the suite's asymptotic floor, which kills the sharding lever. A partitioned suite cannot finish faster than its slowest single case: 324.85s. Refinement — Ready
Acceptance
Generated by Claude Code CLOUD-1349 The mediator ran 16 versions behind the tree it was judging, and nothing reported it — a stale engine reads exactly like a working one
§1 SubjectThe absence of any comparison between the The §2 The measurementMeasured 2026-09-02 in a Claude Code web container, over one full session landing CLOUD-1321.
The engine mediated every tool call in that session against a rule table 16 versions old, and nothing anywhere said so. §3 What that silently disabled, measured by the moment it stoppedThe tell is the step change at the install, not an argument. Across roughly six hours the session issued dozens of
All three shapes had been issued repeatedly before the install and had passed. The rules were in §4 Why this is the worst shape in the model rather than an ordinary bug
A stale mediator defeats all of it at once, one layer beneath where any of it can look. The config is correct, the modules are correct, the tests are green, and the process actually adjudicating calls is enforcing a different, older contract. Silence from the hook is the documented signal that it IS mediating (
§5 Why the existing sensors did not catch it
Refinement — Ready (the engine states which contract it is enforcing) Refinement gate: Definition of Ready & Done. This body carries only specializations. {
"source_of_truth": "batten --version reported 0.0.121 while the workspace was 0.0.137, /tmp/session-start-batten.log did not exist so mise.toml:1987 never ran, and CARGO_PKG_VERSION appears in eight source files of which doctor.rs is not one",
"gate": { "task": "doctor", "exits": [0, 1] },
"commit_type": "fix",
"blockers": [],
"tests": [
{ "file": "policy/harness-wiring.rego", "mutation": "stray-unread" },
{ "file": "policy/harness-wiring.rego", "mutation": "stale-unguarded" }
]
}
Acceptance
§6 Cost of not doing itMeasured, this session: every mediated call for ~6 hours adjudicated against a 16-version-old table, three rule classes provably unenforced, and the discovery arriving only because an unrelated task reached for a subcommand that did not exist. A session that never happened to do that would have finished believing it had been mediated throughout — and would have been right about the silence and wrong about what it meant. Found while landing CLOUD-1321 (PR #828). |
|
Warning Review limit reachedNext included review available in 29 minutes. View limit detailsLimit details: You’ve used the included review currently available. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Free Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Essentials by visiting https://app.coderabbit.ai/settings/billing. Comment |
wenzowski
changed the title
… over `the_committed_portability_rules_fire_on_every_banned_shape` and `the_committed_repo_agnosticism_rules_fire_on_every_banned_shape` each ran `enforce` over the whole committed ruleset — twice, for the banned-shape arm and the clean-fixture discriminator — to assert 6 and 4 rows respectively. On the 2-core Windows runner they cost 324.85s and 319.12s: 644s of 1692.8s summed CPU across 3868 cases, so 0.1% of the suite is 38% of its cost, and the longer of the two sets the asymptotic floor that makes sharding useless. Narrow each arm to the rows it asserts. That needs `--rule` to be repeatable, which it was not: both `CHECK_RULE` and `ENFORCE_RULE` declared `ValueDecl::Str`, whose own doc records that `ArgAction::Set` keeps only the LAST occurrence — so a second `--rule` silently discarded the first rather than refusing. `ValueDecl::StrMany` already existed for exactly this; both flags take it, and `CheckFlags`/`EnforceFlags` carry `Vec<String>` read with `get_many`. `select_rules` takes `&[String]` and refuses PER ID: a `--rule` naming no declared row is a usage error naming the first unmatched id, so a typo in a narrowed test cannot pass vacuously. Selection preserves declaration order rather than argument order, so output stays byte-stable under a reordered command line (house-style §6). Engine-side findings are emitted only on the unnarrowed path, where they are still the whole tree's answer. Both arms of both cases keep their clean-fixture anti-vacuity discriminator and their exact-stdout assertions; the case count does not fall. Measured locally: 0.129s and 0.106s. Refs: CLOUD-1358, CLOUD-1223, CLOUD-1225
…-rule `ValueDecl::StrMany` changed both flags' help line, which the committed completions, man pages and golden schema all carry verbatim. Generated by `mise run schema`, `completions` and `man` — no hand edits. Refs: CLOUD-1358
…our sites The narrowing spelled `--rule <id>` inline at all four call sites — a banned-shape arm and a clean-fixture discriminator per case — which pushed `the_committed_portability_rules_fire_on_every_banned_shape` to 116 lines against `clippy::too_many_lines`'s 100 and broke `lint:clippy`. `test:cargo` does not run clippy, so the suite was green over it. Two consts and a fold. Beyond the lint, the four sites were a drift surface the narrowing itself created: a discriminator whose rule set diverged from the arm it discriminates proves nothing about the set that arm ran. Both cases still pass — 0.128s and 0.119s — and both were shown able to fail by dropping one id from each set. Refs: CLOUD-1358
0f4c7d9 to
dd9a3da
Compare
|
❌ The last analysis has failed. |
|
/fast-forward |
1 participant
Closes CLOUD-1358
644s of a 1693s suite was two tests
the_committed_portability_rules_fire_on_every_banned_shapeandthe_committed_repo_agnosticism_rules_fire_on_every_banned_shapeeach ranenforceover the whole 103-row committed ruleset — twice, for the banned-shape arm and its
clean-fixture discriminator — to assert 6 and 4 rows.
On the 2-core Windows runner: 324.85s and 319.12s, against a next-slowest case of
25.13s. That is 0.1% of 3868 cases carrying 38% of summed CPU, and the longer of the
two sets an asymptotic floor that makes sharding the job useless.
--rulewas not actually repeatableBoth
CHECK_RULEandENFORCE_RULEdeclaredValueDecl::Str, whose own doc recordsthat
ArgAction::Setkeeps only the LAST occurrence — so a second--rulewassilently discarded rather than refused.
ValueDecl::StrManyalready existed forexactly this.
select_rulesrefuses per id: a--rulenaming no declared row is a usage errornaming the first unmatched id, so a typo in a narrowed test cannot pass vacuously.
Selection preserves declaration order rather than argument order, so stdout stays
byte-stable under a reordered command line (§6). Engine-side findings are emitted only
on the unnarrowed path, where they are still the whole tree's answer.
Shown able to fail
Dropping one id from each set reddens each case — portability on
no-gnu-sed-z,agnosticism on
no-consumer-account-literal, the latter withone sorted pointer per banned shape per file, and nothing else. Both arms keep their clean-fixturediscriminator; the case count does not fall.
The rule sets are named once as consts rather than spelled at all four call sites. That
is not only the
too_many_linesfix: the four sites were a drift surface the narrowingitself created, and a discriminator whose set diverged from the arm it discriminates
would prove nothing about the set that arm ran.
Coverage lost, stated rather than buried
The unnarrowed
enforcealso proved the other 97 rows emit nothing on that fixture.That is not what either case's name asserts and sibling cases over the committed bytes
still cover it — but it is a loss, not a free win.
What came out of this PR, and why
CLOUD-1349 (stale mediator detection) was implemented here and backed out before
landing. The check was correct and its placement was not: it compared the binary on
PATHagainsttarget/release/batten, which madedoctoranswer differently dependingon when
install:locallast ran — a property of the world inside a gate that asserts aproperty of the commit, which is the
lock-checkdefect.claude/rules/toolchain.mdalready records.
land's ownverifycaught it viathis_repository_is_healthy. Thefull reasoning, and the finding that version equality provably does not discriminate a
stale mediator, are recorded on CLOUD-1349, which is back in Todo.
Verification
mise run verify→fast-forward-greenon the pre-drop head; suite 4129/4129;perf-comparewithin 1.30x of the merge base. Re-verified after the drop byland.🤖 Generated with Claude Code
https://claude.ai/code/session_01CTqon6Q8TXmhGpdtQeaT6V