fix(bench): enforce tolerance bands on the regression side only (#1160)

[robotrocketscience]

Part of #1160 — the "use one-sided bands so improvements are not failures" half of the band-check criterion. The fail-closed half shipped in #1200.

compute_band is symmetric, so a metric can bust its band by getting better. The canonical cut makes that unavoidable rather than hypothetical:

• mab.Accurate_Retrieval.exact_match is 0.0 and the absolute floor is ±0.02, so every possible improvement lands outside the band.
• LoCoMo category_f1.5 is pinned at 0.0 the same way — the class [Umbrella] The measurement apparatus cannot detect the defects it exists to catch #1160 separately identifies as structurally broken. Fixing it would have registered as a band-busting FAIL.

Verified against the real canonical file

Not a synthetic fixture — benchmarks/results/v2.0.0.json, mutated one leaf at a time:

scenario	before	now
unchanged (identity)	PASS	PASS
ranking win: substring_exact_match 0.153 → 0.20	FAIL	WARN
LoCoMo cat-5 fixed 0.0 → 0.31	FAIL	WARN
substring_exact_match 0.153 → 0.05	FAIL	FAIL
avg_latency_ms 5.55 → 40.0	FAIL	FAIL
corpus shrank: total_questions 500 → 250	FAIL	FAIL

Design decisions

Unclassified metrics stay two-sided. A metric given the wrong direction goes blind to regressions in its real direction, which is strictly worse than the false failure this fixes. So a new metric fails loudly as out-of-band until someone classifies it, rather than silently inheriting a guess. This is also why the table is exact-name keyed rather than substring matched — the existing DEFAULT_RELATIVE_BANDS substring approach would give an oddly named metric the wrong direction silently.

Leaf name, falling back to parent. Bucketed metrics key their leaves by bucket id, not metric name: LoCoMo per-category F1 lands at category_f1.1….5, so the leaf name is "1" and only the parent says what is being measured. Leaf wins on a tie, so count.correct resolves as a score while temporal-reasoning.count resolves as a corpus invariant. The table was built from every metric name across all committed results files, not just v2.0.0.json.

Improving-side exit is WARN, not PASS. A large unexplained gain in this harness is as likely to be an artifact as a win — #1160's own finding is that token-F1 over a retrieval blob rises when the token budget falls, so halving the budget doubles reported F1 while retrieving strictly less. WARN keeps that visible without failing the nightly on a genuine improvement.

Corpus-size metrics stay two-sided deliberately. A shrinking corpus is not an improvement; it invalidates the comparison in either direction. Same for avg_beliefs_per_query, which is exactly the quantity that inflates token-F1.

Two pre-existing tests updated

Both drove their FAIL from a metric improving, which only failed because bands were two-sided:

• test_summarize_fail_dominates — exact_match 0.3 → 0.99
• test_summarize_fail_still_dominates_skip — f1 0.5 → 0.99, commented "huge regression"

The rollup precedence each one tests is unchanged; the leaf now regresses for real (0.3 → 0.01 and 0.5 → 0.05). Flagging explicitly since editing an assertion alongside the change it covers deserves a look.

Verification

• benchmarks/tolerance.py + docs/design/v2_reproducibility_harness.md updated (the spec's band and nightly-fail bullets both stated symmetric semantics).
• 8 new tests: unknown-metric default, parent fallback, leaf-wins-over-parent, improvement WARNs, regression still FAILs, latency inverted, end-to-end through check_report, corpus drift two-sided.
• Full suite: 6331 passed, 69 skipped, 71 xfailed, 4 xpassed.
• tests/regression/test_install_uninstall_e2e.py::test_full_lifecycle_setup_onboard_search_uninstall fails on unmodified github/main too — pre-existing, unrelated to this change.

Summary by Sourcery

Adjust benchmark tolerance bands to treat only regressions as failures, introducing per-metric direction handling and warnings for out-of-band improvements.

New Features:

• Add per-metric direction classification for tolerance bands, including support for bucketed metrics via leaf and parent names.

Bug Fixes:

• Prevent genuine benchmark improvements from being classified as failures when leaving symmetric tolerance bands on the improving side.
• Ensure latency and other cost metrics are treated as regressions on increases rather than decreases.
• Keep corpus-size and retrieval-volume metrics two-sided so drift in either direction continues to fail comparisons.

Enhancements:

• Extend band classification to distinguish between regression-side failures and improving-side warnings, while preserving soft-warning behavior inside bands.
• Expose enforced direction in BandCheck results and propagate it through end-to-end report checking and summarization.

Documentation:

• Update reproducibility harness design docs to describe one-sided, direction-aware tolerance bands and their impact on nightly CI.
• Document the new behavior and rationale in the v4 changelog, including the treatment of unclassified metrics and cost/invariant metrics.

Tests:

• Add targeted tests for direction resolution, one-sided band behavior, latency regression handling, corpus-size drift, and end-to-end report wiring.
• Update existing summarize-dominance tests so they now exercise true regressions instead of improvements that only failed under two-sided bands.

[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: 0ebc2f57-d0b5-49e7-a08f-d9926ca32336

📥 Commits

Reviewing files that changed from the base of the PR and between 5ed19d8 and d8a8856.

📒 Files selected for processing (5)

• CHANGELOG/v4.md
• benchmarks/tolerance.py
• docs/design/v2_reproducibility_harness.md
• tests/test_bench_tolerance.py
• tests/test_bench_tolerance_skip.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

Implements one-sided tolerance bands for benchmark metrics so only regressions fail CI, introduces per-metric direction handling, wires direction through band classification and report checking, updates tests to validate the new semantics, and amends design docs and changelog accordingly.

Sequence diagram for one-sided band classification in check_report

sequenceDiagram
    participant Bench as check_report
    participant Band as compute_band
    participant Dir as direction_for
    participant Classify as classify

    Bench->>Band: compute_band(metric_name, cano_val, overrides, floor)
    Band-->>Bench: lower, upper, kind

    Bench->>Dir: direction_for(path)
    Dir-->>Bench: direction

    Bench->>Classify: classify(cano_val, obs_val, lower, upper, direction)
    alt observed outside band and regressed
        Classify-->>Bench: Verdict.FAIL, note
    else observed outside band and improved
        Classify-->>Bench: Verdict.WARN, note
    else inside band
        Classify-->>Bench: Verdict.PASS or Verdict.WARN
    end

    Bench-->Bench: create BandCheck(path, canonical, observed, lower, upper, band_kind, verdict, note, direction)

Loading

File-Level Changes

Change	Details	Files
Introduce per-metric direction semantics and one-sided band enforcement for regression detection.	Add Direction enum for metric behaviour (higher-is-better, lower-is-better, two-sided). Define METRIC_DIRECTIONS mapping keyed by leaf metric name with parent fallback for bucketed metrics. Implement direction_for(path) helper that resolves metric direction using leaf then parent, defaulting to two-sided. Extend BandCheck to record the enforced direction for each metric path.	benchmarks/tolerance.py
Change band classification logic to fail only on regression side and warn on improving side, and wire direction into report checking.	Update classify() to take a Direction argument and distinguish regression vs improvement outside the band. Ensure two-sided behaviour is preserved as the default for unknown or unclassified metrics. Modify check_report() to compute direction per metric path and pass it into classify(), attaching it to BandCheck instances. Include improved explanatory notes in WARN messages for improvements beyond the band.	benchmarks/tolerance.py
Add and adjust tests to cover one-sided band behaviour, direction resolution, and end-to-end wiring.	Update existing summarize_* tests so their failure comes from true regressions rather than improvements. Add tests for default two-sided behaviour on unknown metrics and empty paths. Add tests for parent fallback, leaf-over-parent precedence, improvement → WARN, regression → FAIL, and inverted latency direction. Add an end-to-end check_report test for LoCoMo category-5 fix and corpus-size drift remaining two-sided failures.	tests/test_bench_tolerance.py tests/test_bench_tolerance_skip.py
Update design documentation and changelog to describe one-sided bands and direction table.	Document direction semantics, improving-side WARN behaviour, and per-metric direction table in the reproducibility harness design doc. Clarify nightly cron behaviour to fail only on regression-side band exits and warn on improvements. Add a changelog entry explaining the motivation, canonical examples, and details of the direction table and corpus metric treatment.	docs/design/v2_reproducibility_harness.md CHANGELOG/v4.md

---

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.

[robotrocketscience]

[claim:review:Gylf:2026-07-30T19:30:16Z]

[robotrocketscience]

Reviewed against the branch, not the description.

Verified:

• The direction logic is right on all six (side x direction) combinations. regressed = (direction is not LOWER_IS_BETTER) if below else (direction is not HIGHER_IS_BETTER) gives FAIL for below/HIGHER, above/LOWER, and both sides of TWO_SIDED; WARN for below/LOWER and above/HIGHER. No case falls through.
• WARN is genuinely non-blocking, so the fix actually lands. .github/workflows/bench-canonical.yml:118 is sys.exit(1 if overall.value in ('fail','no_data') else 0), and summarize promotes WARN above PASS but below FAIL. A band-busting improvement now prints in the per-leaf listing and exits 0. Worth stating because if the nightly had treated WARN as blocking, the whole change would have been a no-op rename.
• The direction table is correct on every leaf check_report actually visits in the canonical. I enumerated them from check_report(cano, cano) on benchmarks/results/v2.0.0.json rather than from a hand-read of the table: 78 leaves, 30 HIGHER, 8 LOWER, 40 TWO_SIDED. Every one of the 40 is a genuine corpus invariant (total_questions, total_qa, total_episodes, total_cases, total_queries, total_ingest_turns, n_runs, count, domain_counts/*, type_counts/*) or the deliberately-ambiguous avg_beliefs / avg_beliefs_per_query. Nothing is misclassified and nothing that should be one-sided is stranded.
• Both edited pre-existing tests were forced edits, not weakened ones. Under this diff the old values resolve to WARN — exact_match 0.3 -> 0.99 and f1 0.5 -> 0.99 both come back warn, higher_is_better. Left alone, assert overall == Verdict.FAIL would have failed, so the choice was to change the input or lose the test. The replacements (0.01, 0.05) keep the leaf out of band on the regression side, which is exactly what FAIL-dominates-WARN and FAIL-dominates-SKIP need in order to still be tested. Flagging it in the body was the right call and the answer holds up.
• 36 tolerance tests green locally.

One correction to the body, not a defect: "The table was built from every metric name across all committed results files" does not hold. benchmarks/results/v3.0.1-structmemeval-answer-correctness.json and -backfill.json carry output/per_task_pct/{accounting,location,recommendations,tree} and _aggregate/output/{mean_pct,query_weighted_pct,task_mean_pct} — quality scores, unambiguously higher-is-better — and every one resolves TWO_SIDED, because the leaf is a task name and neither per_task_pct nor _aggregate is a key in METRIC_DIRECTIONS.

Inert today: the canonical is v2.0.0.json and contains none of them, which is why the 78-leaf enumeration above is clean. And the conservative default is doing exactly what it is documented to do — an unclassified metric fails loudly rather than inheriting a guess, so this is safe-by-construction rather than a bug. The reason it is worth a line: those leaves reproduce the precise #1160 defect if the canonical is ever recut on a structmemeval-shaped report — a per-task score improving would FAIL — and a coverage claim of "all committed results files" is what would stop the next person from re-checking. Either add the two parent keys or soften the claim; I would not hold the PR for it.

Approving.

[github-actions]

PR-size soft cap

This PR is over the advisory size threshold:

• 271 changed lines (limit: 200)
• 5 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.

[github-actions]

merge-train: merged d8a8856 → main via FF push.

[robotrocketscience]

[release:review:Gylf:2026-07-30T19:48:17Z]