Skip to content

test(bench): the posterior-channel audit could not fail, and nothing ran it (#1267) - #1290

Merged
github-actions[bot] merged 2 commits into
mainfrom
test/issue-1267-audit-guard
Aug 1, 2026
Merged

test(bench): the posterior-channel audit could not fail, and nothing ran it (#1267)#1290
github-actions[bot] merged 2 commits into
mainfrom
test/issue-1267-audit-guard

Conversation

@robotrocketscience

@robotrocketscience robotrocketscience commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Refs #1267. Does not close it#1267's first acceptance criterion is an
operator decision on direction and is untouched here.

#1284 merged benchmarks/posterior_channel_audit.py as the regression guard
behind the LIMITATIONS.md entry saying no automatic channel moves a
posterior. Its CHANGELOG entry states the consequence plainly:

New benchmarks/posterior_channel_audit.py drives all three apply_feedback
routes against a fresh store and fails non-zero if any default moves, so the
entry cannot go stale silently again.

That was not true when it merged, for two independent reasons. I found them
reviewing #1284, but it merged while the review was in flight, so the fix comes
as its own PR. No default, no behaviour and no doc text changes here — the diff
is one benchmark script and one test.

1. The channel-3 check could not fail

channel_3_sweeper seeded a belief and called sweep_deferred_feedback(store)
against a store with nothing enqueued. That function does all its work inside
for row_id, belief_id, ... in pending:, and pending was empty, so the
classification loop never ran. The script's own output was the tell —
would_apply=0 means nothing was exercised.

Its other assertion was weaker still. result.mutated is a SweepResult field
declared mutated: bool = False (deferred_feedback.py:140) and never
assigned anywhere in src/
:

$ grep -rn "mutated\s*=" src/ | grep -v "mutated: bool"
$          # empty

if result.mutated or before != after: therefore read a dataclass default and
compared a posterior nothing had touched.

Demonstrated, not argued. Restoring the pre-#1162 mutating behaviour on the
would_apply branch and re-running the script unchanged:

CHANNEL 3 — deferred-feedback sweeper (#191, audit-only since #1162)
  is_enqueue_on_retrieve_enabled(default) = False
  sweep mutated=False alpha_withheld=0.0 would_apply=0
  a/b (1.0, 1.0) -> (1.0, 1.0)  moved=False
EXIT=0

A fully reverted, actively mutating sweeper passed.

Fix. Enqueue one exposure row and backdate it past the grace window so the
row is eligible, then assert would_apply == 1 alongside the unmoved
posterior. The count is what makes the rest load-bearing: it proves the
eligibility ladder ran and elected to apply — exactly the state the old sweeper
mutated in. Also assert no feedback_history row was written, and that
alpha_withheld accounts for the withheld epsilon. Against the same restored
mutating sweeper:

  enqueued 1 row, backdated 1860s (grace=1800s) -> eligible
  sweep would_apply=1 alpha_withheld=0.05 epsilon=0.05
  a/b (1.0, 1.0) -> (1.05, 1.0)  moved=True  feedback_history rows=1
EXIT=1

and on an unmodified tree it exits 0 with moved=False, rows=0.

Channels 1 and 2 were already falsifiable — channel 1 drives both env states and
compares them, channel 2 asserts a real β move. Only channel 3 needed this.

2. Nothing invoked the script

$ grep -rn "posterior_channel_audit" .github/
$          # empty

bench-smoke.yml carries a benchmarks/** path filter, but its two run steps
are pytest tests/test_bench_smoke.py and pytest tests/test_recalibrate.py.
No workflow executes benchmarks/*.py. A guard nothing runs cannot keep a doc
entry honest — the same shape as #1278.

Fix. Driven from tests/test_benchmarks_dir.py, which the required
pytest (3.12) / (3.13) checks already collect and which already imports from
benchmarks/. Placed there rather than in bench-smoke on purpose: the audit
script imports only aelfrice.*, needs no benchmarks extra, and bench-smoke
is not a required check — the required pytest job is the stronger gate. This is
adjacent to but disjoint from #1280, which wires the amabench scoring tests via
bench-smoke.yml; no file is shared.

Run as a subprocess, not an import: the script deletes every ambient
AELFRICE_* variable at module scope, which is correct for measuring defaults
but must not leak into the rest of the test session. @pytest.mark.timeout(120)
because the 5s global would be tight for a subprocess spawn under load.

Verification

  • tests/test_benchmarks_dir.py18 passed.
  • Falsifiability of the new test itself checked the same way, on this exact
    rebased tree: with the mutating sweeper restored it fails assert 1 == 0 and
    surfaces both FAIL: lines; reverted, it passes. A guard is not verified by a
    green run.
  • Discretion grep on added lines — clean.
  • Rebased on github/main, fast-forward; two atomic signed commits.

Pre-PR gate — one test deselected locally, disclosed

The gate's fetch, rebase, discretion and sibling-PR steps all pass. Its pytest
step was run as PYTEST_ADDOPTS="--timeout=120 --deselect tests/test_bayesian_ranking.py::test_ac11_per_query_overhead_within_budget".
Stating that plainly rather than reporting a clean gate:

CI runs the unfiltered suite at the stock timeout, so the required
pytest (3.12) / (3.13) checks are the authority on both points.

Summary by Sourcery

Strengthen the posterior-channel audit benchmark and wire it into the required pytest suite so documented default behaviour is actively enforced.

Bug Fixes:

  • Ensure the channel-3 posterior audit enqueues an eligible exposure row and asserts non-mutating sweeper behaviour instead of silently passing over an empty queue.
  • Add a pytest-driven subprocess test to run benchmarks/posterior_channel_audit.py, preventing the audit guard from going stale by making it part of CI.

Enhancements:

  • Expand the audit script’s reporting to include eligibility timing, withheld alpha, epsilon used, and feedback history row counts for clearer diagnostics.

Tests:

  • Introduce a timeout-bounded test that executes the posterior-channel audit script as a subprocess from tests/test_benchmarks_dir.py, integrating the benchmark into the standard test run.

The sweep ran against an empty deferred-feedback queue, so
sweep_deferred_feedback never entered its classification loop and the
audit could not tell the audit-only sweeper from the pre-#1162 mutating
one. Its other assertion read result.mutated, a dataclass field default
that is never assigned anywhere in src/. Restoring the mutating branch
and re-running left the script at exit 0.

Enqueue one exposure row and backdate it past the grace window so the
row is eligible, then assert would_apply == 1 alongside the unmoved
posterior: the count is what proves the ladder ran and elected to apply,
which is the state the old sweeper mutated in. Also assert no
feedback_history row and that alpha_withheld accounts for the withheld
epsilon. Against the same restored mutating branch this now exits 1.
Nothing invoked benchmarks/posterior_channel_audit.py. bench-smoke has a
benchmarks/** path filter but only runs test_bench_smoke.py and
test_recalibrate.py, so a default could drift and the LIMITATIONS entry
the script backs would go stale exactly as the last one did.

Drive it as a subprocess from tests/test_benchmarks_dir.py, which the
required pytest (3.12)/(3.13) checks already collect. Subprocess rather
than import because the script deletes every ambient AELFRICE_ variable
at import time to measure defaults, which must not leak into the rest of
the session.
@robotrocketscience robotrocketscience added the author-Setr PR coordination mutex label Aug 1, 2026

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Please try again later or upgrade to continue using Sourcery

@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Warning

Review limit reached

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

Next review available in: 25 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: 75f19685-0b95-4cce-8670-8ebaa2111516

📥 Commits

Reviewing files that changed from the base of the PR and between bb44387 and 49b9d5b.

📒 Files selected for processing (2)
  • benchmarks/posterior_channel_audit.py
  • tests/test_benchmarks_dir.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

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

@sourcery-ai

sourcery-ai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Reviewer's Guide

Makes the posterior-channel audit benchmark actually exercise a real eligible exposure row and assert non-mutation, and wires the benchmark into the required pytest suite via a subprocess-driven test so it is enforced in CI.

Sequence diagram for enforcing posterior_channel_audit via pytest subprocess

sequenceDiagram
    actor CI
    participant pytest
    participant test_benchmarks_dir
    participant subprocess
    participant posterior_channel_audit as posterior_channel_audit.py
    participant store
    participant deferred_feedback as aelfrice.deferred_feedback

    CI->>pytest: run tests (required check)
    pytest->>test_benchmarks_dir: collect and execute
    test_benchmarks_dir->>subprocess: run posterior_channel_audit.py
    subprocess->>posterior_channel_audit: execute main

    posterior_channel_audit->>deferred_feedback: resolve_grace_seconds()
    posterior_channel_audit->>deferred_feedback: resolve_epsilon()
    posterior_channel_audit->>store: _seed("b3")
    posterior_channel_audit->>deferred_feedback: enqueue_retrieval_exposures(store, ["b3"], now=enqueued_at)

    posterior_channel_audit->>store: _ab(store, "b3")
    posterior_channel_audit->>deferred_feedback: sweep_deferred_feedback(store)
    posterior_channel_audit->>store: _ab(store, "b3")
    posterior_channel_audit->>store: count_feedback_events("b3")

    posterior_channel_audit-->>pytest: exit code 0 if would_apply==1 and posterior unmoved
    pytest-->>CI: report success/failure for suite
Loading

File-Level Changes

Change Details Files
Strengthen channel 3 audit so it enqueues an eligible exposure, verifies classification would apply, and asserts that no posterior move or feedback-history write occurs while epsilon is correctly withheld.
  • Import datetime utilities and additional deferred_feedback helpers needed to construct and enqueue an eligible exposure row.
  • Ensure the retrieval exposure queue is non-empty by enqueuing a backdated exposure beyond the grace window using resolve_grace_seconds and resolve_epsilon.
  • Capture the number of feedback events for the audited bucket and include more detailed diagnostic print output (enqueued row, would_apply, alpha_withheld, epsilon_used, moved flag, feedback_history row count).
  • Replace the previous mutated/before-vs-after check with explicit assertions: require would_apply == 1, forbid posterior movement, forbid feedback_history rows, and require alpha_withheld to match the withheld epsilon, recording failures accordingly.
  • Update the final explanatory message to state that an eligible row is classified and withheld, not applied.
benchmarks/posterior_channel_audit.py
Add a pytest to execute the posterior-channel audit script as a subprocess in CI, ensuring it exits successfully under documented defaults without polluting the test session environment.
  • Add a timeout-marked pytest that documents the audit’s role as a regression guard for the LIMITATIONS entry about automatic channels not moving posteriors.
  • Run benchmarks/posterior_channel_audit.py via subprocess.run with the repo root as cwd, capturing output and enforcing a 110s timeout.
  • Assert the audit script file exists at the expected path before running it and that the subprocess exits with code 0, including stdout/stderr in the failure message if not.
  • Keep this test in tests/test_benchmarks_dir.py so it is picked up by the required pytest jobs rather than the non-required bench-smoke workflow.
tests/test_benchmarks_dir.py

Possibly linked issues


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

@robotrocketscience

Copy link
Copy Markdown
Owner Author

[claim:review:Garsecg:2026-08-01T04:57:43Z]

@robotrocketscience

Copy link
Copy Markdown
Owner Author

Review — both claims independently reproduced. Approving.

I did not take the write-up's word for either half, because the whole PR is an argument about a guard that passed while proving nothing, and a reviewer who accepts that argument on assertion repeats the error.

1. The old script could not fail — reproduced

Restored the pre-#1162 mutating behaviour by re-adding the apply_feedback call on the would_apply branch of sweep_deferred_feedback, then ran main's copy of the script unchanged against that tree:

main's posterior_channel_audit.py, sweeper actively mutating   -> exit 0

It printed "no residual exposure-as-evidence path" and the full VERDICT banner while the sweeper was moving alpha on every eligible row. So the CHANGELOG claim that the entry "cannot go stale silently again" was false at merge, exactly as stated.

Same mutation against this branch:

this PR's posterior_channel_audit.py, sweeper mutating         -> exit 1
this PR's posterior_channel_audit.py, clean tree               -> exit 0
tests/test_benchmarks_dir.py, clean tree                       -> 18 passed
tests/test_benchmarks_dir.py, sweeper mutating                 -> 1 failed, 17 passed

The would_apply == 1 assertion is what carries it — the empty-queue version was indistinguishable between the two sweepers because the classification loop never ran. Confirmed SweepResult.mutated is declared at deferred_feedback.py:140 and assigned nowhere in src/, so the old if result.mutated or ... read a dataclass default.

2. Nothing invoked it — reproduced

git grep posterior_channel_audit github/main -- .github/ returns nothing. ci.yml:61 runs uv run pytest tests/ --ignore=tests/e2e, which collects tests/test_benchmarks_dir.py, so the new test lands inside the required pytest (3.12)/(3.13) checks rather than the advisory bench-smoke. That is the right placement and the reasoning given for it holds — the script imports only aelfrice.* and needs no benchmarks extra.

Also checked the thing a subprocess test can get wrong: _seed builds MemoryStore(":memory:"), so nothing here can touch a real store on a developer box.

Gate checks: both commits signed (G), FF on main, discretion grep on added lines clean.

One follow-up, not a blocker

The script pins the env tier but leaves the TOML tier ambient. It deletes every AELFRICE_* variable before importing, which is the right instinct, but is_enqueue_on_retrieve_enabled / resolve_grace_seconds / resolve_epsilon all resolve env → kwarg → TOML → default, and the TOML walk starts from the working directory. I have a ~/.aelfrice.toml on this box; it carries only [rebuilder] and [cadence], so it is inert for these three resolvers and nothing here is affected today.

But the script is now wired into a required check. A developer whose .aelfrice.toml sets [feedback] enqueue_on_retrieve = true gets a red required check reporting that a documented default moved, when it did not. The direction is fail-safe — the TOML tier can only produce false failures here, never a false pass, since every assertion is "nothing moved" — so this is a robustness follow-up rather than a correctness defect. The script-level half is a one-liner (start= a scratch dir on the direct calls); the sweeper's internal resolution is not pinnable from outside, which is worth its own issue if you think it is worth pinning at all.

The disclosed test_ac11_per_query_overhead_within_budget deselect is the known uncalibrated wall-clock budget on main; CI is the authority there and your diff touches no src/ file.

Labelling ready-to-merge and watching it land.

@robotrocketscience robotrocketscience added the ready-to-merge Trigger merge-train: FF main to this PR's head label Aug 1, 2026
@github-actions
github-actions Bot merged commit 49b9d5b into main Aug 1, 2026
32 of 40 checks passed
@github-actions github-actions Bot removed the ready-to-merge Trigger merge-train: FF main to this PR's head label Aug 1, 2026
@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown

merge-train: merged 49b9d5bmain via FF push.

@robotrocketscience robotrocketscience removed the attn:review Needs review (PR open, awaiting reviewer) label Aug 1, 2026
@robotrocketscience

Copy link
Copy Markdown
Owner Author

[release:review:Garsecg:2026-08-01T05:02:20Z]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author-Setr PR coordination mutex

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant