fix(bench): fail the band-check closed when nothing was measured (#1160)

[robotrocketscience]

Fourth acceptance criterion of #1160 — "Band-check fails closed on absent data." Does that half; the one-sided-bands half is deliberately left open, with reasons below. The umbrella stays open.

The defect

tolerance.summarize returned PASS whenever no leaf was FAIL or WARN. Ignoring an individual SKIP is correct and ratified (#479) — one uncomputable metric is not a regression — but the rollup ignored every SKIP. Measured on main:

all SKIP (12)     -> pass    job_exit=0     <-- nothing was compared
empty check list  -> pass    job_exit=0     <-- nothing to compare

That first row is exactly what a failed dataset download on the runner produces: every adapter exits because its data dir is absent, every leaf becomes SKIP, counts read {pass: 0, warn: 0, fail: 0, skip: 12} — and the verdict was PASS. bench-canonical.yml then exited 0 on anything that was not fail, so the nightly went green having measured nothing.

The fix

PASS is a claim that something was measured and stayed in band, so it now requires at least one leaf that actually passed. Otherwise the rollup is a new NO_DATA verdict, and the workflow exits 1 on it.

case	before	after
all SKIP (12)	pass, exit 0	no_data, exit 1
empty check list	pass, exit 0	no_data, exit 1
1 SKIP + 1 PASS (#479)	pass, exit 0	pass, exit 0
1 SKIP + 1 FAIL (#479)	fail, exit 1	fail, exit 1
1 SKIP + 1 WARN	warn, exit 0	warn, exit 0

#479 is preserved exactly. A SKIP beside any real PASS still passes, and FAIL/WARN still dominate — a leaf that was compared to its band is evidence measurement happened, which is why WARN with no PASS stays WARN rather than becoming NO_DATA.

NO_DATA is a distinct verdict rather than reusing FAIL because the two demand different operator responses: a regression means read the diff, no data means fix the runner. Reusing FAIL would also repeat this umbrella's own complaint about reporting a structural zero as if it were a measurement.

Blast radius checked before touching the vocabulary: summarize has exactly one production consumer (bench-canonical.yml:109-114) plus two test modules. benchmarks/badge.py consumes no verdicts, and the other Verdict classes in the repo (reason.py, relationship_detector.py, …) are unrelated types.

Leaf tallying also moved to .get, so a leaf carrying an unexpected verdict is counted and reported rather than raising KeyError inside the gate whose job is to report it.

Verification

• Full suite 6263 passed, 69 skipped (7 new).
• The 28 pre-existing test_bench_tolerance*.py tests pass unchanged — that was the check that mattered, since [v2.1] Bench dispatcher exit-code 3-state contract (ok / skipped / error) #479 is the invariant most at risk.
• Mutation-verified: reverting the rollup fails 3 of the new tests; reverting the workflow's exit line fails the inertness test specifically. That second test exists because fixing summarize alone leaves the verdict inert and the gate still fails open.
• 4 signed commits, ordered so the tree is green at each; discretion grep clean.

Not done: one-sided bands

The same criterion also asks for one-sided bands so an improvement is not classified as a failure. I have not done that, and the reason is not effort.

It needs a per-metric direction table — quality metrics improve upward, latency metrics downward. Inferring direction from metric names would be a guess, and a metric assigned the wrong direction becomes blind to regressions in the real direction — the exact class of defect this umbrella exists to remove. Trading a false failure for a silent blind spot is a bad trade to make implicitly, so it wants a deliberate decision on the direction of each canonical metric. Happy to build the table if you want to specify it, or to propose one for review.

Summary by Sourcery

Ensure the nightly reproducibility band-check fails closed when no metrics were measured by introducing a NO_DATA verdict and treating it as a failing outcome in the canonical bench workflow.

Bug Fixes:

• Prevent all-SKIP or empty band-check runs from rolling up to PASS, so a failed dataset download or missing checks no longer reports a successful calibration.

Enhancements:

• Extend verdict tallying to count unexpected verdict values instead of raising, making the band-check gate robust to surprising leaf states.

Documentation:

• Document the new NO_DATA verdict and band-check behaviour change in the v4 changelog.

Tests:

• Add targeted tests for NO_DATA rollup behaviour, preservation of SKIP semantics, failure precedence over NO_DATA, handling of unexpected leaf verdicts, and enforcement that the workflow exits non-zero on NO_DATA.

[sourcery-ai]

Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[coderabbitai]

Warning

Review limit reached

@robotrocketscience, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 29 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4052483a-6817-4831-bd72-6391a447ed9c

📥 Commits

Reviewing files that changed from the base of the PR and between c52275d and 4e66e15.

📒 Files selected for processing (4)

• .github/workflows/bench-canonical.yml
• CHANGELOG/v4.md
• benchmarks/tolerance.py
• tests/test_bench_tolerance_no_data.py

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[sourcery-ai]

Reviewer's Guide

Introduce a NO_DATA verdict for band-check rollups that have no passing measurements, update the nightly workflow to fail on NO_DATA, and add tests ensuring summarize and the workflow treat absent data as a closed failure while preserving prior SKIP behavior (#479).

Sequence diagram for bench-canonical workflow handling NO_DATA verdict

sequenceDiagram
  actor Runner
  participant BenchCanonicalWorkflow
  participant tolerance_summarize as tolerance.summarize
  participant sys as sys

  Runner->>BenchCanonicalWorkflow: run bench-canonical.yml
  BenchCanonicalWorkflow->>tolerance_summarize: summarize(checks)
  tolerance_summarize-->>BenchCanonicalWorkflow: overall=NO_DATA, counts
  alt overall.value in ('fail','no_data')
    BenchCanonicalWorkflow->>sys: sys.exit(1)
  else overall.value in ('pass','warn')
    BenchCanonicalWorkflow->>sys: sys.exit(0)
  end

Loading

File-Level Changes

Change	Details	Files
Adjust band-check rollup logic to distinguish runs with no successful measurements from true passes and maintain existing SKIP handling semantics.	Add a NO_DATA member to the Verdict enum, documented as rollup-only and distinct from FAIL. Change summarize() to count leaf verdicts via dict.get so unexpected verdicts are tallied rather than raising KeyError. Update summarize() to return NO_DATA when there are no FAIL/WARN leaves and zero PASS leaves, preserving [v2.1] Bench dispatcher exit-code 3-state contract (ok / skipped / error) #479 behavior where SKIP beside any PASS still passes.	benchmarks/tolerance.py
Make the bench-canonical workflow treat NO_DATA as a failing outcome so nightly band-checks fail closed when nothing was measured.	Change the sys.exit decision logic to return exit code 1 when overall verdict is either fail or no_data, keeping warn non-blocking. Add inline documentation in the workflow step explaining why all-SKIP/no-data runs must fail and how warn remains advisory.	.github/workflows/bench-canonical.yml
Document the new NO_DATA behavior and the closed-fail semantics for absent measurement data in the changelog.	Append a detailed 4.2.0 changelog entry describing the previous bug where all-SKIP and empty runs were treated as PASS. Explain that PASS now requires at least one passing leaf, and that NO_DATA is distinct from FAIL to guide operator responses.	CHANGELOG/v4.md
Add targeted tests to lock in NO_DATA rollup behavior, workflow exit semantics, and robustness to unexpected verdicts.	Create a new test module that constructs BandCheck leaves and asserts NO_DATA for all-SKIP and empty check lists, PASS for SKIP+PASS, and WARN/FAIL dominance over NO_DATA. Add tests confirming NO_DATA is treated as rollup-only, that summarize() counts an unexpected NO_DATA leaf instead of crashing, and that the workflow’s sys.exit calls explicitly handle no_data as failing.	tests/test_bench_tolerance_no_data.py

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[github-actions]

PR-size soft cap

This PR is over the advisory size threshold:

• 131 changed lines (limit: 200)
• 4 changed files (limit: 3)

Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated attn:merge-conflict cycles (see #602). When practical, split into smaller PRs that each touch a focused surface.

This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the size:override label and this comment will be removed on the next push.

[robotrocketscience]

[claim:review:Setr:2026-07-30T16:44:52Z]

[robotrocketscience]

Approve. Verified independently rather than on the PR body's word.

The defect reproduces. summarize([]) and an all-SKIP list both return PASS on main, and bench-canonical.yml:114 exited 0 on anything that wasn't fail. The nightly could go green having compared nothing.

The tests are load-bearing. Reverting benchmarks/tolerance.py to main and re-running the new module fails 4 of 7 — the three rollup assertions plus test_no_data_is_rollup_only on the missing enum member. test_the_workflow_fails_the_job_on_no_data is the one that matters most: fixing summarize alone leaves the verdict inert, and that test is what pins the workflow half.

#479 is intact. The 28 pre-existing test_bench_tolerance_skip.py tests pass unchanged, and the re-pinned cases here (SKIP+PASS → PASS, SKIP+FAIL → FAIL, bare WARN → WARN) match the ratified semantics. Treating a compared-and-out-of-band leaf as evidence that measurement happened is the right cut.

Blast radius confirmed. summarize has one production consumer (bench-canonical.yml) plus the two test modules; benchmarks/badge.py reads counts, not verdicts; the other Verdict enums in the tree are unrelated types. Adding a member is safe here.

Mechanics: 4 signed commits, FF on main, [Unreleased] entry, all required checks green, no unresolved threads.

---

One residual, for the umbrella rather than this PR

The band-check step is gated if: steps.bench.outputs.out != '', and the bench step runs set -euo pipefail under continue-on-error: true. If aelf bench all exits nonzero, the echo "out=..." line never runs — so out is empty, the band-check step is skipped rather than run, steps.bandcheck.outcome is skipped (not failure), and the closing "Fail job if band-busting regression" step is skipped along with it. A harness that fails outright still produces a green job.

That is the same acceptance criterion — absent data, gate open — approached from the other side. This PR closes the all-SKIP shape; the crash shape is untouched. Not a reason to hold the merge (it's pre-existing and orthogonal to the rollup logic), but it should be tracked, and I've noted it on #1160.

Related, same area: the closing step at line 168 looks unreachable. bandcheck has no continue-on-error, so an exit 1 there fails the job at that step, and every later step — none of which use a status function in its if: — is skipped, including the one testing outcome == 'failure'. The gate still goes red, so nothing is broken; but it also means a FAIL or NO_DATA night never commits the cron entry, which the comment on the bench step says was the intent. Worth folding into the same follow-up.

Nit, not worth a respin

test_the_workflow_fails_the_job_on_no_data asserts that every sys.exit( line in the workflow mentions no_data. There's exactly one today, so it passes; a future unrelated sys.exit in any other step of that file would false-fail it. Scoping the scan to the band-check step would keep the guard without the coupling.

On the deferred half

Declining to infer band direction from metric names is the right call. A metric assigned the wrong direction is blind to regressions in the real one, which is a worse failure than the false alarm it would prevent. Leaving it for an explicit table is correct.

[robotrocketscience]

[release:review:Setr:2026-07-30T16:49:17Z]

[github-actions]

merge-train: merged 4e66e15 → main via FF push.