test(retrieval): the R3 IDF-clip boost arm is unreachable by construction (#1281) - #1282
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
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: 59 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)
📝 WalkthroughWalkthroughThe PR adds a deterministic benchmark for R3 IDF clipping and stemmed vocabulary lookup. It also adds regression tests for IDF behavior, clipping reachability, duplicate emission, and lower-threshold boosting. ChangesR3 IDF clipping analysis
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AuditJSONL
participant Benchmark
participant BM25Index
participant JSONOutput
AuditJSONL->>Benchmark: load and deduplicate prompts
Benchmark->>BM25Index: build index and compute IDF values
Benchmark->>BM25Index: evaluate reachability and query census
Benchmark->>JSONOutput: optionally write sorted results
Possibly related issues
🚥 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 |
robotrocketscience
added
the
attn:review
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 |
Reviewer's GuideAdds a benchmark harness and a characterization test suite to empirically and structurally demonstrate that the R3 IDF-clip boost arm is unreachable against a real BM25Index, and to pin current behaviour for future quantile-policy changes without altering production retrieval behaviour. Flow diagram for R3 IDF-clip reachability benchmark harnessflowchart TD
subgraph Inputs
A_store["--store path (MemoryStore)"]
A_audit["--audit paths (hook_audit.jsonl)"]
A_window["--window"]
A_json["--json-out (optional)"]
end
A_audit --> B_load[load_prompts]
B_load --> B_prompts["prompts: list[str]"]
A_store --> C_store[MemoryStore]
C_store --> C_index[BM25Index.build]
C_index --> C_bm25["index: BM25Index"]
C_bm25 --> D_idf[index.idf]
D_idf --> D_quant[compute_idf_quantile_thresholds]
D_quant --> D_band["low, high"]
C_bm25 --> E_reach[reachability]
D_band --> E_reach
E_reach --> E_result["reachability_result"]
%% Arm A: raw prompts
B_prompts --> F_armA[census]
C_bm25 --> F_armA
D_band --> F_armA
F_armA --> F_resultA["arm_a_raw_prompts"]
%% Arm B: production-shaped queries
B_prompts --> G_windows["build windows of RecentTurn"]
A_window --> G_windows
G_windows --> G_query[_query_for_recent_turns]
G_query --> G_prod_queries["production_shape_queries"]
G_prod_queries --> H_armB[census]
C_bm25 --> H_armB
D_band --> H_armB
H_armB --> H_resultB["arm_b_production_shape"]
%% Optional JSON output
E_result --> J_pack["json.dumps results"]
F_resultA --> J_pack
H_resultB --> J_pack
A_window --> J_pack
A_json --> J_write["write_text(json_out)"]
J_pack --> J_write
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
57857d5 to
37307bc
Compare
|
[claim:review:Setr:2026-07-31T21:48:41Z] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/test_r3_idf_clip_reachability.py (3)
117-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard against a vacuous pass.
len(out) == len(set(out))also holds whenoutis empty. If a futurelowpolicy drops the whole vocabulary, this test still passes and stops characterising the boost arm. Assert that the clip keeps some terms.♻️ Proposed refactor
out = clip_with_quantile_thresholds( terms, index.vocabulary, index.idf, low, high, ) + assert out, "clip dropped the entire vocabulary; the assertion below is vacuous" assert len(out) == len(set(out))🤖 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_r3_idf_clip_reachability.py` around lines 117 - 122, Add a non-empty assertion for the clipped result in the test around compute_idf_quantile_thresholds and clip_with_quantile_thresholds, ensuring out retains at least one term before checking uniqueness. Keep the existing uniqueness assertion unchanged.
64-80: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueClose the store even when setup fails.
store.close()only runs after theyield. If aderivecall, an assertion, orBM25Index.buildraises, the SQLite connection stays open for the rest of the session. Wrap the body intry/finally.♻️ Proposed refactor
store = MemoryStore(str(tmp_path / "r3.db")) - for i, text in enumerate(_corpus()): - out = derive( - DerivationInput( - source_kind=INGEST_SOURCE_FILESYSTEM, - raw_text=text, - source_path=f"doc{i}.md", - session_id=None, - ts="2026-01-01T00:00:00+00:00", - ), - ) - assert out.belief is not None - store.insert_or_corroborate(out.belief, source_type="filesystem_ingest") - yield BM25Index.build(store) - store.close() + try: + for i, text in enumerate(_corpus()): + out = derive( + DerivationInput( + source_kind=INGEST_SOURCE_FILESYSTEM, + raw_text=text, + source_path=f"doc{i}.md", + session_id=None, + ts="2026-01-01T00:00:00+00:00", + ), + ) + assert out.belief is not None + store.insert_or_corroborate( + out.belief, source_type="filesystem_ingest", + ) + yield BM25Index.build(store) + finally: + store.close()The fixture uses a real
MemoryStoreon atmp_pathSQLite file and no storage mocks, which matches the path instructions.As per path instructions: "Tests must hit a real SQLite DB, not mocks."
🤖 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_r3_idf_clip_reachability.py` around lines 64 - 80, Update the index fixture so all store setup, belief insertion, and BM25Index.build work are wrapped in a try/finally, with store.close() executed in the finally block even when setup raises.Source: Path instructions
155-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe monotonicity assertion tests the test's own arithmetic.
idf_atrecomputes the Robertson closed form inside the test. Line 159 then checks that this locally computed list is descending, which holds by construction and says nothing aboutBM25Index.build. Only line 160 links the closed form back to the index. Derive the ordering fromindex.idfinstead, so a change to the shipped IDF form fails this test.One option: compute the document frequency per term from
index.tf, then assert that IDF decreases as document frequency increases across the observed values.Also note
range(1, min(n_docs, 12))excludesdf == n_docswhenn_docs <= 12, so the frequency of the shared core terms falls outside the range for small corpora.N_DOCSis 40 today, so this only matters if the fixture shrinks.🤖 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_r3_idf_clip_reachability.py` around lines 155 - 159, The monotonicity assertion should validate BM25Index.build output rather than recomputing the Robertson formula. Replace the locally calculated idf_at list with document frequencies derived from index.tf, then compare the corresponding values from index.idf to ensure IDF decreases as observed document frequency increases, including the shared-core frequency when present.
🤖 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.
Inline comments:
In `@benchmarks/r3_idf_clip_bound.py`:
- Around line 207-213: Update the df_at_low calculation in the IDF cutoff
reporting block to invert the shipped IDF formula, using E = exp(low) - 1 and
solving for df as (n_docs + 0.5 - 0.5 * E) / (E + 1). Preserve the existing low
> 0.0 guard and NaN fallback.
- Around line 130-132: Update the path-processing loop in load_prompts to emit a
warning to stderr before continuing when an audit path does not exist. Include
the missing path in the warning, while preserving the existing behavior for
valid paths and skipped missing paths.
- Around line 394-408: Update the JSON payload built in the --json-out path to
normalize df_at_low_cutoff NaN values to null before json.dumps serialization,
while preserving numeric values for non-NaN results. Locate the value’s
construction and ensure the serialized reachability data uses this normalized
representation.
---
Nitpick comments:
In `@tests/test_r3_idf_clip_reachability.py`:
- Around line 117-122: Add a non-empty assertion for the clipped result in the
test around compute_idf_quantile_thresholds and clip_with_quantile_thresholds,
ensuring out retains at least one term before checking uniqueness. Keep the
existing uniqueness assertion unchanged.
- Around line 64-80: Update the index fixture so all store setup, belief
insertion, and BM25Index.build work are wrapped in a try/finally, with
store.close() executed in the finally block even when setup raises.
- Around line 155-159: The monotonicity assertion should validate
BM25Index.build output rather than recomputing the Robertson formula. Replace
the locally calculated idf_at list with document frequencies derived from
index.tf, then compare the corresponding values from index.idf to ensure IDF
decreases as observed document frequency increases, including the shared-core
frequency when present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 43a78e59-d897-409a-b2ad-e8e14ef26709
📒 Files selected for processing (2)
benchmarks/r3_idf_clip_bound.pytests/test_r3_idf_clip_reachability.py
Review — approving. Every number re-derives, and that is not a coincidence: this PR shipped its harness.I have spent this session failing to reproduce headline figures on these Structural claim — exact, on the live indexThe unreachability argument is not a sampling result and I checked it as an Because hapax terms are 38.5% of the vocabulary and Robertson smoothed IDF is Also confirms the drop-arm framing: the low cutoff drops any query term The tests pin production, and the distinguishing arm is real
The safety claim reproduces, and it is the important halfRan the shipped harness against my own My window count differs from the body's (n=7 vs 25, so 57% vs 36%) because my "The stemming bug is load-bearing" is a genuinely counter-intuitive result and Two small things, neither blocking
Mechanics
VerdictAdding |
robotrocketscience
added
ready-to-merge
|
[release:review:Setr:2026-07-31T21:52:38Z] |
|
merge-train: blocked 1 review thread(s) are unresolved on these files: benchmarks/r3_idf_clip_bound.py. Resolve them on the PR (click 'Resolve conversation' on each) and re-add the label. The |
github-actions
Bot
removed
the
ready-to-merge
merge-train bounced on the unresolved bot threads — and one of them is right. Correcting my own review.The label was stripped with "1 review thread(s) are unresolved on these files:
|
| idf(low) | harness | correct | rel. err |
|---|---|---|---|
| 8.565 (this store) | 8.004 | 8.002 | 0.02% |
| 6.000 | 110.3 | 110.0 | 0.25% |
| 4.000 | 831.5 | 816.3 | 1.87% |
| 2.000 | 6,979 | 6,035 | 15.65% |
| 1.000 | 25,952 | 16,405 | 58.20% |
A smaller corpus, a lower low_quantile, or a store whose vocabulary is less
Zipfian all move left on that table. The harness is explicitly built to be
re-run — that is its whole value — so a formula that is only accurate at today's
operating point is worth fixing now rather than after someone quotes it from a
different one.
Correcting my review
I wrote "Also confirms the drop-arm framing: the low cutoff drops any query
term appearing in ≥ 8 of 44,593 beliefs, matching the body." The conclusion
stands (the empirical check gives 8), but I presented the harness's figure as
corroborated when the code producing it is wrong for the general case. I should
have derived it before quoting it, which is the standard I have been applying to
everyone else's numbers today.
Re-labelling
Not re-adding ready-to-merge. The three threads are all quick fixes and two
are cosmetic (warn on a missing --audit path; NaN → null in --json-out,
which RFC 8259 does not permit), but the inversion should be corrected in the
harness before it merges, since re-runnability is the point of shipping it.
Everything else in my review is unaffected: the unreachability identity, the
test pinning production symbols, the distinguishing arm, and the empty-rebuild-query
safety result all stand as verified.
|
[claim:review:Toug:2026-08-01T03:41:31Z] |
|
[claim:review:Gylf:2026-08-01T03:44:05Z] |
|
[release:review:Gylf:2026-08-01T03:44:10Z] |
… fix's blast radius Both #1158 §4 and #1174 item 19 claim the R3 half of stack-r1-r3 is inert on the strength of one hand-built query. It is not: 68.5% of query terms already resolve in the vocabulary and 97% of those are dropped. The inert half is the boost arm, and it is unreachable by construction -- high_threshold is the 0.75 quantile of the vocabulary IDF vector, Robertson smoothed IDF is maximised at df == 1, and hapax terms are 38.5% of the vocabulary, so the quantile collapses onto max(idf) and idf > high is unsatisfiable. Reports both input shapes because the production input is _query_for_recent_turns output, not a raw prompt. Aggregate counts only -- no prompt or belief text reaches stdout or --json-out.
…M25Index test_query_understanding.py exercises the boost arm only against hand-built vocabulary/idf pairs, where it fires. It never composes the clip with the index production hands it, which is where the collapse happens. Includes a distinguishing arm: lowering high_quantile below 1 - hapax_share makes the same corpus, index and clip start emitting boosted copies, so the assertions measure the shipped 0.75 policy rather than restating something vacuously true of the fixture.
df_at_low inverted `log(1 + (N + 0.5)/(df + 0.5))`, dropping the `- df` from the numerator of the shipped Robertson form. With E = exp(low) - 1 the exact inverse is (N + 0.5 - 0.5E)/(E + 1); it now round-trips through idf() at every cutoff. No number in this PR moves: at this store E >> 1, both forms are dominated by N/E and agree to 0.02% (8.0015 vs 8.0000, and the empirical minimum df is 8 either way). The error grows as the cutoff falls — 1.9% at idf 4, 15.7% at idf 2, 58.2% at idf 1 — so a smaller or less Zipfian corpus would have been reported wrongly. Re-runnability is why the harness ships, so it gets the exact inverse rather than one that happens to hold today. Also: warn on a missing --audit path instead of silently measuring a partial corpus, and serialise a NaN df_at_low_cutoff as null, which json.dumps otherwise writes as the bare token NaN that RFC 8259 forbids.
37307bc to
ce68983
Compare
Adopted the three open threads and pushed the fixes in
|
idf(low) |
old | corrected | rel. err | idf(corrected) |
|---|---|---|---|---|
| 8.565 (this store) | 8.0015 | 8.0000 | 0.02% | 8.565289 |
| 6.000 | 110.3 | 110.0 | 0.25% | 6.000000 |
| 4.000 | 831.5 | 816.3 | 1.87% | 4.000000 |
| 2.000 | 6,979.2 | 6,034.6 | 15.65% | 2.000000 |
| 1.000 | 25,951.9 | 16,404.7 | 58.20% | 1.000000 |
That reproduces the review's table exactly, and the round-trip column is the
part the algebra alone does not give you: it fails for the old form and holds
to 6 dp for the new one.
No number in this PR moves. E ≫ 1 here, both forms are dominated by
N/E, and the empirical minimum df is 8 either way — so the body's "drops
any query term seen in ≥ 8 beliefs" stands, and stood independently of the
formula. The derivation is now in a comment at the call site, including why the
error is invisible at this operating point, so the next person to re-run this on
a smaller corpus does not rediscover it.
2 & 3 — the two minor threads
- Missing
--auditpath now warns to stderr instead of skipping silently,
so a misspelt path in a multi-path invocation cannot quietly produce numbers
over a partial corpus. Verified it fires once per missing path. --json-outconverts a NaNdf_at_low_cutofftonull.json.dumps
was writing the bare tokenNaN, which RFC 8259 does not permit. Verified the
output now parses under a strict parser that rejectsNaN/Infinity.
Both are on the serialisation/reporting surface only.
Verification
tests/test_r3_idf_clip_reachability.py— 6 passed.- Round-trip identity above, and the corrected value matches the empirical
minimumdfon the live index. - Strict-parse check on the
--json-outpayload. - Rebased onto current
main(which has moved since this branch was cut);
fast-forward, all three commits signed, discretion grep clean. - Pushed with
--force-with-leasepinned to the previous head SHA, so a
concurrent author push could not have been clobbered.
Everything else in the review stands untouched: the unreachability identity,
the test pinning production symbols, the distinguishing arm, and the
empty-rebuild-query safety result.
Threads resolved, attn:unblock cleared. Re-adding ready-to-merge once CI is
green, and I will watch it to a terminal state rather than labelling and walking
away.
robotrocketscience
added
ready-to-merge
|
merge-train: merged ce68983 → |
github-actions
Bot
removed
the
ready-to-merge
|
[release:review:Toug:2026-08-01T03:49:17Z] |
Pushed
|
| df | pre-fix | correct |
|---|---|---|
| 1 | 1.0380 | 1.0000 |
| 40 | 3280.0 | 40.0 |
It is mildest exactly where this store's cutoff sits, which is why checking
that one operating point did not surface it. The new test feeds idf(df)
back for every df in the fixture; the old form fails it at the first one.
--json-out's NaN handling cannot fire on the reachable input range
Worth recording, because it is the same shape as this PR's own thesis.
Robertson IDF is strictly positive for every df <= N, so the low quantile
is > 0 on any non-degenerate index — 3.3081 on the fixture — and
df_at_low_cutoff is finite across the whole reachable range. NaN requires
low == 0.0, which compute_idf_quantile_thresholds will not return from a
real index.
So the null conversion is defensive, not live. I kept it (it is correct,
and cheap insurance if the quantile policy ever changes) and kept a strict
RFC 8259 parse test as a payload-wide guard against a future non-finite
field — but the test docstring states plainly that it does not exercise that
branch and would pass with the conversion deleted, rather than implying
coverage it does not have. The NaN branch is covered directly against
reachability.
Verification
- 10 tests in the module, 3 of the 4 new ones verified to fail against the
pre-fix code by reverting each fix in turn. - Full suite: 6910 passed, 69 skipped, 71 xfailed.
- Discretion grep on added lines: clean.
- Fast-forward on top of the fix commit — no history rewritten.
Not re-labelling ready-to-merge, since the review claim is not mine to
close out. From my side as author the three threads are resolved and the PR
is ready.
1 participant
Closes #1281.
Measurement + pin for the R3 IDF-clip defect that #1158 §4 and #1174 item
19both carry. No product behaviour changes.What this refutes
Both umbrellas claim R3 is inert, on the evidence of one hand-built query, and both prescribe the same one-line stemming fix. Measured on the 44,593-belief development store over 198 real user-turn prompts:
BM25Index.vocabulary, and 97% of those are dropped. The 0.25-quantile cutoff drops any query term appearing in ≥ 8 of 44,593 beliefs.high_thresholdis the 0.75 quantile of the vocabulary IDF vector; Robertson smoothed IDF is maximised atdf == 1; hapax terms are 38.5% of the vocabulary. The quantile collapses ontomax(idf) == 10.2999andidf > highis unsatisfiable. Zero boosts across 4,096 measured query terms — structural, not sampling.context_rebuilder.py:389has no empty check and_query_for_recent_turns's docstring records the consequence:retrieve()returns L0 only. The stemming bug is load-bearing.Full numbers and the quantile-policy decision are on #1281.
Contents
935b71f6benchmarks/r3_idf_clip_bound.py— the harness, so the numbers are re-derivable rather than quoted. Reports both input shapes, because production feeds_query_for_recent_turnsoutput, not a raw prompt.efceccb0tests/test_r3_idf_clip_reachability.py— pins the boost arm as unreachable against a realBM25Index.Why the existing tests missed it
tests/test_query_understanding.pyexercises the boost arm only against hand-builtvocabulary/idfpairs, where it fires happily. It never composes the clip with the index production hands it, and the collapse lives exactly in that composition.The new test carries a distinguishing arm: lowering
high_quantilebelow1 - hapax_sharemakes the same corpus, index and clip start emitting boosted copies. Without it the assertions could pass vacuously on any fixture.Verification
uv run pytest tests/test_r3_idf_clip_reachability.py— 6 passed.--json-out.Not in scope
Changing the shipped clip behaviour. The quantile policy — currently "drop everything seen in ≥ 8 beliefs, boost nothing" — is a default-retrieval change and is gated on #1281's decision criterion.
Summary by Sourcery
Add a measurement harness and characterization tests to demonstrate that the R3 IDF-clip boost arm is unreachable with the current quantile policy and to pin existing retrieval behaviour.
Enhancements:
Tests:
Summary by CodeRabbit
Tests
Chores