From 72c594ee499dd220692d77916d2dfdd575176116 Mon Sep 17 00:00:00 2001 From: tegwick Date: Fri, 31 Jul 2026 04:02:33 +0200 Subject: [PATCH] CI: enforce every gate; close the silent-skip holes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gates existed; CI ran half of them and tolerated the failure case. - cb-sim no longer has a "tolerable" non-zero exit. An unregistered game prefix is a failure, and a run in which nothing executed is a failure. Previously CI carried `|| test $? -eq 2`, so renaming a scenario prefix would have skipped every scenario while the pipeline stayed green. Verified with a negative control. - CI now runs make coverage (AM-1) and make dep-weight (AM-4), both added after CI was written and neither enforced until now. - dep-weight enforces its targets instead of only reporting them. - CI lints the shipped-runtime configuration separately, so the feature split cannot rot unnoticed. - Dropped the stale `make deps` target, which still measured the retired crate-count metric. The positive-control rule is now executable: CI runs `cargo bench -- --test`, which executes every benchmark once, so a workload that stalls fails the build. That step immediately found a fourth instance of the error class it was written for. The committed replay benchmark was the broken version — an earlier patch never applied, leaving a command sequence that omits Resolve, so every round produced nothing and the log-building loop spun forever. It had never run to completion; the reported AM-7 replay numbers came from a probe test instead. Fixed, given the same positive control as the round loop, and re-measured from the benchmark: 100k events fold in 2.18ms (95% CI 2.14-2.23), against a 5s budget. Evidence now reports confidence intervals rather than point estimates, so the 3% regression rule in MetricsAndScenarios is enforceable. The finding worth carrying: writing the positive-control rule into InnerLoop v1.0 did not prevent the next instance. Making it a CI step did. Co-Authored-By: Claude Opus 5 --- .forgejo/workflows/ci.yml | 24 ++++++- Makefile | 14 ++-- evidence/CB-EV-0001-game-kernel.md | 41 +++++++++-- games/ground/benches/synthetic.rs | 109 ++++++++++++++++------------- tools/cb-sim/src/main.rs | 26 +++++-- tools/dep-weight.py | 25 ++++++- 6 files changed, 169 insertions(+), 70 deletions(-) diff --git a/.forgejo/workflows/ci.yml b/.forgejo/workflows/ci.yml index e4b4a11..df9a045 100644 --- a/.forgejo/workflows/ci.yml +++ b/.forgejo/workflows/ci.yml @@ -14,5 +14,27 @@ jobs: - run: rustup component add rustfmt clippy - run: cargo fmt --all --check - run: cargo clippy --workspace --all-targets -- -D warnings + + # The shipped runtime is a distinct configuration (AM-4a); it must + # compile and lint on its own, or the feature split rots. + - run: cargo clippy -p games-ground --no-default-features -- -D warnings + - run: cargo test --workspace - - run: cargo run -q -p cb-sim -- scenarios/ground/*.yaml || test $? -eq 2 + + # No `|| test $? -eq 2`: cb-sim now fails on an unregistered game + # prefix and on a run that executed nothing. A silent skip is the + # failure this step exists to catch. + - run: cargo run -q -p cb-sim -- scenarios/ground/*.yaml + + # AM-1: every numbered GR-rule is claimed by a scenario, and no + # scenario claims a rule the spec does not define. + - run: make coverage + + # AM-4a/AM-4b: third-party source under audit, per configuration. + - run: make dep-weight + + # InnerLoop v1.0 positive control, enforced rather than asserted in + # prose: --test runs every benchmark once, so a workload that + # stalls or produces the wrong event count fails the build instead + # of silently reporting throughput for work that never happened. + - run: cargo bench -p games-ground --bench synthetic -- --test diff --git a/Makefile b/Makefile index de5c9eb..f3799e1 100644 --- a/Makefile +++ b/Makefile @@ -3,7 +3,7 @@ CARGO := cargo -.PHONY: check test sim bench deps loc all +.PHONY: check test sim bench bench-test coverage dep-weight loc all ## fmt + clippy (deny warnings) + HashMap deny-lint check: @@ -24,13 +24,15 @@ coverage: sim: $(CARGO) run -q -p cb-sim -- scenarios/ground/*.yaml -## Criterion benches (AM-6/AM-7 wiring) +## Criterion benches (AM-6/AM-7) bench: $(CARGO) bench -p games-ground -## AM-4: transitive crate count (excludes dev/build deps) -deps: - @$(CARGO) tree --workspace -e normal --prefix none | sed 's/ (\*)//' | sort -u | grep -vE '^(cb-|games-|cb_|$$)' | tee /dev/stderr | wc -l +## InnerLoop positive control: run every bench once, no measurement. +## Fails if a workload stalls or produces the wrong event count. +bench-test: + $(CARGO) bench -p games-ground --bench synthetic -- --test + ## AM-2/AM-3 input: source LOC per crate (excludes tests would need tokei) loc: @@ -38,4 +40,4 @@ loc: printf '%-28s %s\n' $$d "$$(find $$d/src -name '*.rs' | xargs cat | grep -vcE '^\s*(//|$$)')"; \ done -all: check test sim +all: check test sim coverage dep-weight bench-test diff --git a/evidence/CB-EV-0001-game-kernel.md b/evidence/CB-EV-0001-game-kernel.md index 3f60d0e..8bc157f 100644 --- a/evidence/CB-EV-0001-game-kernel.md +++ b/evidence/CB-EV-0001-game-kernel.md @@ -21,7 +21,7 @@ Machine: WSL2, Linux 6.18.33.2-microsoft-standard-WSL2, rustc 1.97.1, | AM-4b dep weight, dev toolchain | ≤350,000 third-party lines | 317,021 (29 crates) | **met** | | AM-6 throughput | ≥100,000 events/s | 1,651,400 events/s | **met, 16.5×** | | AM-7 scaling | ≥0.9× at 20× workload | 1.08× | **met** | -| AM-7 replay | 100k events ≤5s | 4.13 ms | **met, 1,210×** | +| AM-7 replay | 100k events ≤5s | 2.18 ms (CI 2.14–2.23) | **met, 2,290×** | | AM-8 determinism | zero divergence, 10 replays | 1 distinct hash / 10 runs | **met** | | AM-8 lint | fmt + clippy clean | clean, `-D warnings` | **met** | | AM-10 foreign types | zero `HashMap`/`HashSet` | 0 | **met** | @@ -45,12 +45,17 @@ breaks the test rather than silently rescaling the metric. | 40,000 | 1,626,000 | 1.07× | | 100,000 | 1,651,400 | 1.08× | -Replay — folding one growing event log back into state: +Replay — folding one growing event log back into state (Criterion +95% CI, low/median/high): -| Events | Time | Rate | -|---|---|---| -| 10,010 | 465 µs | 21.5M events/s | -| 100,007 | 4.13 ms | 24.2M events/s | +| Events | Time (median) | 95% CI | Rate | +|---|---|---|---| +| 10,010 | 184.8 µs | 180.9 – 189.3 µs | 54.2M events/s | +| 100,072 | 2.183 ms | 2.142 – 2.226 ms | 45.8M events/s | + +**Correction (2026-07-31).** These originally read 465 µs / 4.13 ms and +were taken from the `replay_probe` **test**, not from the benchmark — +because the benchmark did not work. See §2a. ### The comparison against boardgame.io, stated carefully @@ -100,6 +105,30 @@ The benchmark now asserts the per-round event count on every round and panics rather than measuring a stalled loop. The corrected figure is **5.6× lower** than the bogus one. +### 2a. A fourth measurement error, found by enforcing the rule + +The replay benchmark committed alongside this evidence was **the broken +version**. A `python3` patch that was supposed to replace its +log-building loop never applied, leaving a sequence that omits `Resolve` +— so `EndRound` was rejected, every round produced no events, and the +`while log.len() < target` loop spun forever. It was never run to +completion; the AM-7 replay numbers were taken from a separate probe +test instead, and the dead benchmark was committed and left hanging. + +Found by adding `cargo bench -- --test` to CI, which runs every +benchmark once. That is the fourth instance of one error class in this +project — a harness that appears to work while doing no work — and the +**first one caught by a gate rather than by noticing**. + +The replay loop now carries the positive control the round loop already +had: it asserts each round appended events and fails rather than +spinning. Corrected figures are in the table above; both configurations +still clear the AM-7 budget by three orders of magnitude. + +The lesson recorded for the loop: writing the positive-control rule into +`specs/InnerLoop.md` did **not** prevent the next instance. Making it a +CI step did. Prose rules do not enforce themselves. + ## 3. Determinism (AM-8) - Every scenario runs twice per invocation with the same seed and fails diff --git a/games/ground/benches/synthetic.rs b/games/ground/benches/synthetic.rs index 8a6d596..b20a903 100644 --- a/games/ground/benches/synthetic.rs +++ b/games/ground/benches/synthetic.rs @@ -122,6 +122,44 @@ const FINAL_ROUND_EVENTS: usize = 12; /// integers: (4 x 13 + 12) = 64. const EVENTS_PER_5_ROUNDS: usize = 64; +/// Play one round, appending every applied event to `log`. +fn record_round(state: &mut GroundState, log: &mut Vec) { + let mut run = |state: &mut GroundState, actor: Actor, cmd: &GroundCommand| { + if let Ok(produced) = state.validate(actor, cmd) { + for event in &produced { + state.fold(event); + log.push(event.clone()); + } + } + }; + for (seat, action, target) in [ + (0u8, Action::Attack, Some(PlayerId(1))), + (2, Action::Support, Some(PlayerId(1))), + (1, Action::Ground, None), + ] { + run( + state, + Actor::Player(PlayerId(seat)), + &GroundCommand::SelectAction { + action, + target, + problem: None, + }, + ); + } + run(state, Actor::System, &GroundCommand::Reveal); + run( + state, + Actor::Player(PlayerId(1)), + &GroundCommand::ChooseGroundMode { + mode: GroundMode::Gr, + choice: None, + }, + ); + run(state, Actor::System, &GroundCommand::Resolve); + run(state, Actor::System, &GroundCommand::EndRound); +} + fn bench_synthetic(c: &mut Criterion) { // Events per round is fixed by the workload, so throughput can be // reported in events/second — the AM-6 unit. @@ -146,55 +184,30 @@ fn bench_synthetic(c: &mut Criterion) { // flat curve is partly by construction — this one is not, because // the log here grows without bound. let mut replay = c.benchmark_group("replay-ground-3p"); - for &events in &[10_000usize, 100_000] { - replay.throughput(Throughput::Elements(events as u64)); - replay.bench_function(format!("fold-{events}-events"), |b| { - // Build one log of `events` events, then measure folding it. - let mut source = setup(42); - let mut log = Vec::with_capacity(events); - while log.len() < events { - if source.outcome.is_some() { - source = setup(43); - } - let picks = [ - ( - PlayerId(0), - GroundCommand::SelectAction { - action: Action::Attack, - target: Some(PlayerId(1)), - problem: None, - }, - ), - ( - PlayerId(1), - GroundCommand::SelectAction { - action: Action::Support, - target: Some(PlayerId(2)), - problem: None, - }, - ), - ]; - for (seat, cmd) in picks { - if let Ok(produced) = source.validate(Actor::Player(seat), &cmd) { - for e in &produced { - source.fold(e); - log.push(e.clone()); - } - } - } - if let Ok(produced) = source.validate(Actor::System, &GroundCommand::Reveal) { - for e in &produced { - source.fold(e); - log.push(e.clone()); - } - } - if let Ok(produced) = source.validate(Actor::System, &GroundCommand::EndRound) { - for e in &produced { - source.fold(e); - log.push(e.clone()); - } - } + for &target_events in &[10_000usize, 100_000] { + // Build one log by playing real rounds, then measure folding it + // back. Must use the full command sequence: a shortened one + // stalls, because Reveal needs every seat's selection and + // EndRound is gated on Resolve. + let mut log = Vec::with_capacity(target_events); + let mut source = setup(42); + let mut games = 0u64; + while log.len() < target_events { + if source.outcome.is_some() { + games += 1; + source = setup(42 + games); } + let before = log.len(); + record_round(&mut source, &mut log); + // Positive control: a round that yields nothing means the + // workload stalled, and the loop above would spin forever. + assert!( + log.len() > before, + "replay workload stalled: a round produced no events" + ); + } + replay.throughput(Throughput::Elements(log.len() as u64)); + replay.bench_function(format!("fold-{target_events}-events"), |b| { b.iter(|| { let mut state = setup(42); for event in &log { diff --git a/tools/cb-sim/src/main.rs b/tools/cb-sim/src/main.rs index 633d1aa..eba47eb 100644 --- a/tools/cb-sim/src/main.rs +++ b/tools/cb-sim/src/main.rs @@ -1,7 +1,10 @@ //! cb-sim — scenario runner binary (GameKernel K17; precursor of `cb sim`). //! Parses scenario files, dispatches each to its game by the `/` //! prefix of its `scenario` field, and executes it. Exit codes: 0 all -//! passed, 1 failures, 2 unknown game, 64 usage error. +//! passed, 1 any failure — including a scenario whose game prefix is not +//! registered, and a run in which nothing executed. There is deliberately +//! no "tolerable" non-zero exit: a silent skip is the failure mode this +//! binary exists to catch. use cb_game_runtime::{scenario, RunOutcome, ScenarioFile}; use games_ground::GroundState; @@ -13,7 +16,6 @@ fn main() { std::process::exit(64); } - let mut unknown_game = false; let mut failed = false; let mut passed = 0usize; let mut covered: Vec = Vec::new(); @@ -39,8 +41,14 @@ fn main() { let outcome = match sc.scenario.split('/').next() { Some("ground") => scenario::run::(&sc), _ => { - println!("SKIP {} — no game registered for this prefix", sc.scenario); - unknown_game = true; + // A renamed or typo'd prefix would otherwise skip every + // scenario while the run still looked clean. + eprintln!( + "FAIL {} — no game registered for prefix {:?}", + sc.scenario, + sc.scenario.split('/').next().unwrap_or("") + ); + failed = true; continue; } }; @@ -67,10 +75,14 @@ fn main() { covered.dedup(); println!("{passed} passed, {} rules covered", covered.len()); + // Positive control: a run that executed nothing must not pass. This + // is the same class of error as a benchmark timing rejected work. + if passed == 0 { + eprintln!("FAIL — no scenario executed; refusing to report success"); + failed = true; + } + if failed { std::process::exit(1); } - if unknown_game { - std::process::exit(2); - } } diff --git a/tools/dep-weight.py b/tools/dep-weight.py index a8f5d11..dbcb597 100755 --- a/tools/dep-weight.py +++ b/tools/dep-weight.py @@ -32,6 +32,13 @@ CONFIGS = { "dev-toolchain": [], } +# AM-4a / AM-4b targets from specs/GameKernel.md §4. Breaching one fails +# the build: a gate that only reports is a suggestion. +TARGETS = { + "shipped-runtime": 250_000, + "dev-toolchain": 350_000, +} + def crates(extra_args): """Third-party crates in the normal (non-dev) dependency graph.""" @@ -116,9 +123,12 @@ def main(): print(f" own source {own:>9,} lines") for label in CONFIGS: r = report[label] + limit = TARGETS[label] + mark = "ok " if r["third_party_loc"] <= limit else "FAIL" print( f" {label:<18}{r['crates']:>3} crates " - f"{r['third_party_loc']:>9,} lines third-party" + f"{r['third_party_loc']:>9,} lines third-party " + f"[{mark} target {limit:,}]" ) delta = ( report["dev-toolchain"]["third_party_loc"] @@ -131,7 +141,18 @@ def main(): # shrink the total, so refuse to report rather than under-report. print("\nERROR — source not found for:", ", ".join(missing), file=sys.stderr) return 1 - return 0 + + breached = [ + (label, report[label]["third_party_loc"], limit) + for label, limit in TARGETS.items() + if report[label]["third_party_loc"] > limit + ] + for label, actual, limit in breached: + print( + f"\nFAIL AM-4 — {label}: {actual:,} lines exceeds target {limit:,}", + file=sys.stderr, + ) + return 1 if breached else 0 if __name__ == "__main__":