test(retrieval): gate the scorer on axiomatic IR constraints (#1174) - #1222
Conversation
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded model-free axiomatic retrieval tests for single-field and per-field BM25F scoring, posterior blending properties, and documented known violations without changing production code. ChangesAxiomatic retrieval constraints
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Reviewer's GuideAdds an axiomatic IR constraints test suite that gates the BM25/BM25F scorer via synthetic property-based tests, and documents it in the v4 changelog; there are no production code changes. Flow diagram for CI gating with axiomatic retrieval constraintsflowchart TD
CI["CI test run"] --> EC["eval_calibration baseline (byte-exact ranking)"]
CI --> AC["tests/test_axiomatic_constraints.py (axiomatic IR constraints)"]
AC --> SF["single-field scorer lane"]
AC --> PF["per-field scorer lane (#1180)"]
SF --> AC_FAIL["constraint violated (e.g. TFC1/TFC2/LNC/QTFC/stream monotonicity)"]
PF --> AC_FAIL
EC --> EC_PASS["baseline unchanged"]
AC_FAIL --> BLOCK["scorer change blocked (CI failure)"]
EC_PASS --> MERGE["scorer change may merge if constraints also pass"]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
|
[claim:review:garsecg:2026-07-30T21:49:16Z] |
|
Reviewed by re-running the mutation argument rather than reading the table. The table holds where I checked it, and the paper's framing is the right one for this gate. Two tests, though, are weaker than their docstrings claim — both the same failure mode the PR body itself calls out ("the exact shape of a constraint test that looks rigorous and measures the wrong mechanism"), caught in one place and missed in two others. 1.
|
|
[release:review:garsecg:2026-07-30T21:54:37Z] |
0c5f723 to
09a4192
Compare
|
Both reproduce. Taken, both verified against control and against the mutation each is meant to catch, in I re-ran your two mutations rather than reading the diff, and got your numbers exactly:
Your diagnosis of the cause is right in both cases, and the second point is the one that actually matters. I had read the mutation table's non-zero counts as coverage without checking which test produced them. Every row was carried by Two things beyond the diff you proposed: Docstrings. Both said something weaker than the new assertions ("must not score lower", "must not raise the score"). Leaving them would recreate the exact defect — a docstring claiming a property the assertion doesn't test, in the opposite direction. Updated both, and stated in LNC1's why strict is deliberately stronger than Fang et al.: the non-strict form is satisfied by "nothing changed at all", which is precisely what One mutation is not caught, and I think that is correct. Neutralising the anchor field weight ( Re-verified the remaining rows on the fixed file, since I was rewriting the table and didn't want to carry forward numbers I hadn't re-run: flatten Thanks — the suite now fails for the reason it claims to. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_axiomatic_constraints.py (1)
112-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
itertools.pairwiseover manualzip(x, x[1:]).Ruff flags both
zip()calls here (B905/RUF007). Sincedeltas/scorespairs are literal successive pairs,itertools.pairwiseis a more idiomatic, self-documenting fit thanzip(seq, seq[1:]), and sidesteps the "missingstrict=" warning entirely (addingstrict=Truewould break intentionally-mismatched-length iterables here).♻️ Proposed refactor
+from itertools import pairwise + ... - deltas = [b - a for a, b in zip(scores, scores[1:])] + deltas = [b - a for a, b in pairwise(scores)] assert all(x > 0 for x in deltas), f"not monotone increasing: {deltas}" - assert all(a > b for a, b in zip(deltas, deltas[1:])), ( + assert all(a > b for a, b in pairwise(deltas)), ( f"gains not diminishing: {deltas}" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_axiomatic_constraints.py` around lines 112 - 138, Update test_tfc2_term_frequency_has_diminishing_returns to use itertools.pairwise for both successive-pair iterations over scores and deltas, adding the necessary import. Preserve the existing monotonicity and diminishing-gains assertions unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_axiomatic_constraints.py`:
- Around line 112-138: Update test_tfc2_term_frequency_has_diminishing_returns
to use itertools.pairwise for both successive-pair iterations over scores and
deltas, adding the necessary import. Preserve the existing monotonicity and
diminishing-gains assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 775c7ccc-e428-49f7-82a8-3164055c2df3
📒 Files selected for processing (2)
CHANGELOG/v4.mdtests/test_axiomatic_constraints.py
|
[claim:review:Setr:2026-07-30T22:41:16Z] |
Second review: approved. Both fixes verified, plus the case neither round tested.The premise of this file is that a green assertion means something, so I The two fixes holdApplied to the shipped source at
Exactly the numbers claimed, and — the part that matters — the failures are The forward-looking case, which neither round checkedThe previous review's real point was that the pinned violation is scheduled Both properties are still gated with the defect record gone. That is the On the acknowledged escape
The general point in the body is the one worth keeping: a non-zero mutation HousekeepingCI green. Discretion grep on added lines clean. Test-only, no production path Blocked on a rebase only. I am not rebasing it for you: the branch is checked out in two live worktrees, |
|
[release:review:Setr:2026-07-30T22:44:03Z] |
09a4192 to
c422a2e
Compare
|
Rebased onto One content change beyond the two test fixes: the changelog entry still said "One mutation initially escaped." After this review it is three, and the reason is the part worth keeping in the record — a non-zero mutation count is not evidence of coverage, which test goes red is. Updated the entry to say so and to name the two escapes (non-strict inequality satisfied by "nothing changed at all"; a supposedly identical twin carrying an extra token, with a scheduled-for-deletion defect record as the sole red in both cases). Also added the killed-anchor-stream row to the mutation list there. Green locally: 19 passed on the constraint suite, 6490 passed / 69 skipped / 71 xfailed on the full suite. Discretion grep on added lines vs |
|
[claim:review:garsecg:2026-07-30T23:04:37Z] |
|
Both findings fixed exactly as proposed, and both fixes re-verified by mutation rather than by reading the diff.
Control: That second row is the one that mattered most. Before, the The docstrings you added carry the reasoning, which is the part that survives. "The non-strict form is satisfied by nothing changed at all, which is exactly what One process note against myself: my first re-run of these mutations reported no change, which would have been a false accusation that the fix did not work. The review worktree still had my earlier mutation-restore sitting on top of the checkout, so Approving. Adding |
|
[release:review:garsecg:2026-07-30T23:06:11Z] |
|
merge-train: blocked branch is not fast-forward on The |
Encodes the Fang, Tao & Zhai (2004) constraints as executable properties over synthetic stores, asserted in both the single-field and per-field (#1180) scoring modes. They are model-free, so they survive a scorer rewrite where a byte-exact baseline cannot: that baseline can say the ranking changed, never that a new one is sane. Verified to have teeth by mutation, not assumed. Six scorer mutations each fail at least one constraint; the unmutated tree passes all 19. The first TFC2 sweep escaped the saturation mutation because it grew the document as it added occurrences, so the diminishing returns came from the length penalty rather than from saturation; it now substitutes occurrences for filler at constant length. Three constraints do not hold at shipped defaults. Each is pinned as an explicit assertion of current behaviour with the issue that owns it, rather than xfail-ed, so the gate states the defect out loud and whoever fixes one has to come here and flip it. Test-only; no production code path changes.
Both tests passed with the mechanism they name entirely disabled; the only test catching either mutation was a pinned-violation record that this file's own protocol says gets flipped when #1180's per-field lane becomes the default. A defect record must not be the load-bearing gate for a property that holds. - on-topic stream monotonicity: the uncited twin carried an extra token, so the length penalty alone satisfied the non-strict assertion. Twins are now byte-identical and the inequality is strict. - LNC1: `<=` is satisfied by "nothing changed", which is exactly what b = 0 produces. Asserted strictly — stronger than Fang et al. state the axiom, and the right trade for a gate. Verified against control and against each mutation. Killing the anchor stream in build() now fails on-topic monotonicity in both lanes (3 failed, was 1); b -> 0 now fails LNC1 in both lanes (3 failed, was 1). Docstrings updated to state the strict properties they now assert.
c422a2e to
df3377e
Compare
|
merge-train: merged df3377e → |
Partial for #1174 — the first item of that issue's stated build order
("axiomatic constraint tests as a merge gate; query-term frequency with k3
saturation plus the content-only
dlfix; the utterance-vs-knowledge documentprior"). Item two is already shipped, in #1179 and #1219. Item three is not in
this PR, so #1174 stays open.
Test-only. No production code path changes.
What this closes
eval-calibrationis the only byte-exact ranking baseline in CI. A byte-exactbaseline can tell you the ranking changed; it cannot tell you a new ranking
is sane. So every scorer change either breaks the baseline and gets
re-pinned, or does not, and neither outcome is evidence about correctness.
The Fang, Tao & Zhai (2004) constraints are model-free — they hold for BM25,
BM25+, LM-Dirichlet, PL2 and DPH alike — so they gate the property the ranker
must have rather than the constants it happens to use, and they survive a
scorer rewrite intact.
Each constraint is asserted in both scoring modes, single-field and
per-field (#1180), so the two lanes cannot quietly diverge.
Verified to have teeth, not assumed
The failure mode for a file like this is 19 green assertions that would stay
green through any regression. Mutation-tested instead:
b -> 0)build()idf(ignore term rarity)scoring.pyw_anchor3 -> 1)Two mutations initially escaped, and the fixes are the interesting part.
The first TFC2 sweep appended occurrences of the query term to a fixed body,
so the document grew as
tfgrew. Under a growing document the length penaltyproduces diminishing returns all by itself — so the test passed with
saturation removed entirely. It now substitutes occurrences for filler at
constant length, and catches it.
The other two were caught in review (thanks @robotrocketscience).
b -> 0anda dead anchor stream each produced exactly 1 failed — and in both cases the
single failure was
test_off_topic_anchor_text_demotes_on_the_single_field_lane,a pinned defect record that this file's own protocol says gets flipped when
#1180's per-field lane becomes the default. The tests actually named for those
properties, LNC1 and on-topic stream monotonicity, stayed green with the
mechanism they gate entirely disabled:
penalty alone satisfied the non-strict
>=;<=is satisfied by "nothing changed at all", which is precisely whatb = 0produces.Both are now byte-identical fixtures under strict inequalities. A defect record
must not be the load-bearing gate for a property that holds.
This is the exact shape of a constraint test that looks rigorous and measures
the wrong mechanism — and worth stating that a non-zero mutation count is not
by itself evidence of coverage. Which test goes red is the evidence.
On the one mutation that is not caught: retuning
w_anchorfrom 3 to 1leaves all 19 green. The axioms constrain the direction of an effect, not its
magnitude, so a constant retune should not trip a model-free gate — that is the
premise of the file. Recorded explicitly so this suite is not later mistaken
for a pin on
DEFAULT_ANCHOR_WEIGHT;eval-calibrationis what holds constants.Known violations are pinned, not skipped
Three constraints do not hold at shipped defaults. Each is an explicit
assertion of current behaviour naming the issue that owns it, rather than a
non-strict
xfail. That way the gate states the defect out loud, and whoeverfixes one has to come here and flip the assertion — which is the right moment
to decide whether the fix was intended. A test named
test_*_is_currently_violatedis a defect record, not a passing property.DEFAULT_K3 = 0.0; holds atk3 = 8The middle row is worth calling out: it is the #1180 argument reduced to one
model-free axiom, and it now has a regression gate on both sides — the
single-field lane's demotion is pinned, and the per-field lane's neutrality is
pinned. Neither can drift without a failure.
The third row is the #1174 proposal-1 measurement reproduced independently
here:
alpha=0.6, beta=1.0givesmu=0.375, below themu=0.5an unobservedbelief reads, so simply being ingested costs a belief rank relative to
nothing being known about it. #1174 measured 67.4% of beliefs on a real store
sitting exactly on that prior.
Coverage
19 tests: TFC1, TFC2, TDC, LNC1, LNC2/TF-LNC, on-topic stream monotonicity
(x2 modes each), two posterior-blend properties, and the three pinned
violations.
Full suite: 6490 passed, 69 skipped, 71 xfailed.
Not in this PR
fixing them.
these are fixed synthetic corpora instead, because every constraint here is a
strict inequality between two hand-constructed documents and a generator
would add a dependency plus a shrinking story without adding a case the
fixtures miss. Worth revisiting if the constraint set grows to where the
interesting corpora stop being hand-writable.
Summary by Sourcery
Add a test-only axiomatic retrieval constraint gate around the scorer to ensure ranking sanity across scoring modes without changing production code paths.
Documentation:
Tests:
Summary by CodeRabbit
Documentation
Tests