feat(retrieval): budgeted maximum-coverage pack selector, default off (#1176) - #1238
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
robotrocketscience
added
the
attn:review
Reviewer's GuideImplements a budgeted maximum-coverage pack selector for L1 retrieval, wires it behind a new, default-off flag that takes precedence over clustering, and adds comprehensive unit/integration tests plus changelog documentation, ensuring the selector is exact, deterministic, and inert unless explicitly enabled. Sequence diagram for L1 pack selection with max-coverage vs clusteringsequenceDiagram
actor User
participant Retrieval as retrieve_with_tiers
participant Flag as is_max_coverage_pack_enabled
participant Cov as _coverage_inputs
participant MaxCover as pack_max_coverage
participant Cluster as pack_with_clusters
User->>Retrieval: retrieve_with_tiers(query)
Retrieval->>Flag: is_max_coverage_pack_enabled()
Flag-->>Retrieval: max_coverage_on
Retrieval->>Retrieval: resolve_use_intentional_clustering()
Retrieval-->>Retrieval: cluster_on
alt max_coverage_on and l1
Retrieval->>Cov: _coverage_inputs(query, l1, bm25f_cache)
Cov-->>Retrieval: coverage, term_weights
Retrieval->>MaxCover: pack_max_coverage(l1, token_budget, coverage, term_weights, _cost)
MaxCover-->>Retrieval: l1_packed
Retrieval->>Retrieval: append l1_packed to out
else cluster_on and l1
Retrieval->>Cluster: pack_with_clusters(l1, _cost, ...)
Cluster-->>Retrieval: l1_packed
Retrieval->>Retrieval: append l1_packed to out
end
Retrieval-->>User: packed beliefs
File-Level Changes
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 |
|
Cross-reference: this also covers #1177's "facility-location submodular coverage as the L1 pack objective", which is the same intervention filed on a second umbrella — same seam, same replaced code, same inputs, same greedy family. The objective's shape is the only difference, and it is a special case rather than a rival: max-coverage is binary per term (
|
|
Warning Review limit reached
Next review available in: 5 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 (4)
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 |
|
[claim:review:Toug:2026-07-31T14:00:32Z] |
|
[claim:review:Setr:2026-07-31T16:36:50Z] |
|
[release:review:Setr:2026-07-31T16:36:57Z] |
|
Reviewed by rebuilding the load-bearing claims independently rather than Exactness: confirmed, and my first two attempts to break it were my own bugsWrote an eager cost-benefit greedy from the spec — no heap, no lazy bounds —
Recording the two false alarms, because both are easy to repeat:
The (1 − 1/e) claim is false as shipped
So the greedy maximises a quantity that is not a set function at all (scaling Brute-forced the true optimum over all feasible subsets. Random inputs stay
Smallest reproduction ( The mechanism: I am not arguing against the rank weight. It is a deliberate relevance
|
| budget | arm | n | identical list | same set, new order | different set | mean L1 packed | hit the 50 ceiling |
|---|---|---|---|---|---|---|---|
| 2000 | user | 300 | 33.3% | 39.7% | 27.0% | 34.4 | 0 / 300 |
| 2000 | harness | 23 | 4.3% | 47.8% | 47.8% | 30.6 | 0 / 23 |
2400 (DEFAULT_TOKEN_BUDGET) |
user | 300 | 33.3% | 47.7% | 19.0% | 39.6 | 1 / 300 |
On prompts where the set changes, median Jaccard 0.89, median 2 beliefs swapped
in.
Three things follow:
- The ceiling is not reached. 0 of 323 prompts at budget 2000 pack 50 L1
beliefs; mean is 34.4. The budget binds,l1_limitdoes not. The pack grows
to 47.4 only around budget 4000 and 48.3 at 8000 — which is where a mean of
~47.8 would come from, not from 2000. - The lane changes the pack on two-thirds of prompts at the default. A
flat near-duplicate-pair count is a null on that metric; it is not evidence
the selector is inert. Both readings are consistent with the PR's own table —
swapping 2 of 34 beliefs plausibly does not move a pair count — but only one
of them supports "no A/B can measure anything here", and it is not the one
the data shows. DEFAULT_TOKEN_BUDGETis 2400, not 2000. The PR calls 2000 "the shipped
default" in four places.
Why it matters: the Recommendation tells the operator that flipping this needs
"probably a budget or l1_limit change alongside". On this measurement the
l1_limit half is the wrong knob, and a retrieval-quality A/B at today's
defaults is perfectly measurable — it would be scored on ~66% of prompts, which
is the experiment #1176 actually wants.
On the wiring test
The stated reason for asserting call-iff-flag rather than diffing the pack is
that "constructing a small fixture where the coverage pack differs from a rank
fill is hard, and the reason is the finding itself: BM25's idf weighting already
pushes term-diverse beliefs to the top, so the two agree on most inputs."
The replay says they disagree on 66.7% of real prompts, so that justification
does not hold — whatever makes the fixture awkward, it is not that the two
agree. I would not block a default-off flag on this, and the selector's own
behaviour is well covered by the unit tests. But the comment should not record
a reason the corpus contradicts.
Everything else checks out
- 3 commits, all signed (
G), conventional prefixes, atomic and readable. - CI green on the current head:
pytest (3.12),pytest (3.13),secrets-scan,
pattern-scan,history-scan, CodeQL, CodeRabbit,bench-smoke. The
CANCELLEDrows are superseded runs. - No unresolved review threads. Discretion grep on added lines clean.
- Default genuinely off:
is_max_coverage_pack_enabled()returns False with no
env, no kwarg, no toml;_coverage_inputsis only reached inside the branch. - Precedence over
use_intentional_clusteringis implemented as documented
(if max_coverage_on ... elif cluster_on), so no double-packing. - Zero-cost beliefs are handled and cannot divide by zero; verified they also
cannot be selected past budget.
Disposition
Not adding ready-to-merge yet, for two reasons that are both cheap to clear:
- The (1 − 1/e) statement is a formal guarantee in a user-facing CHANGELOG
entry and a module docstring, and it is not true of the shipped function. - The
l1_limitdiagnosis points the operator's next step at the wrong knob.
Both are prose. No code change requested — the selector itself passed
everything I threw at it, including 3,000 adversarial cases it had no reason to
survive. Correct the two claims and I will label it.
Separately, the branch is behind github/main and will need a rebase before
the merge train can fast-forward it.
|
[release:review:Toug:2026-07-31T16:56:46Z] |
|
[claim:review:Gylf:2026-07-31T16:58:30Z] |
Correction to my review above — I had the constant wrong, and the diagnosis with itI said the The pairing carries (1 − 1/√e), not (1 − 1/e)Khuller, Moss & Naor give two algorithms. The one this PR implements — the From Krause, A Note on the Budgeted Maximization of Submodular Functions
Primary source: Khuller, Moss & Naor, The budgeted maximum coverage problem, So the sentence in Measured, with each variant scored against its own objectiveBrute-forced the true optimum over all feasible subsets, 2,500 adversarial
The unweighted variant violates (1 − 1/e) too. That is the line that kills Correction to my own numbers as well: the The rank weight is still worth a sentence, on a different axisIt is not a bound violation, but it does cost coverage. Against the plain The specific defect is that the multiplier is applied per element: This is not the marginal of any set function, so no submodularity argument Path-dependent, so no The clean form, if the relevance floor is worth keeping (and I think it is), is Submodular plus modular is submodular, and monotone for What I would actually ask for now
Everything else in my review stands unchanged: exactness verified (0 divergences |
Second review — concurring, with one seam neither review has namedPicked this up before seeing the review above, so the exactness work was done Exactness: independently confirmed, wider regime coverageReference re-derived from the docstring spec, not from the shipped tests. 5,000
0 divergences over all budgets > 0. Recording my own false alarm, since it is easy to repeat: I first logged 213 Also confirmed: the re-push loop terminates (a re-pushed element is compared New: coverage is measured on text the pack does not necessarily render
This is the seam #878 closed for the cost currency between the cluster pack Measured incidence today: nil. Replaying 103 real prompts from the audit log
So this is latent, not live, and I would not ask for a code change on it. What I Nit, not a request
DispositionAgreeing with the review above and adding nothing to its ask: the prose Everything I independently re-checked — default off, precedence over the cluster |
|
[release:review:Gylf:2026-07-31T17:07:43Z] |
|
[claim:review:Garsecg:2026-07-31T17:10:29Z] |
1f8deee to
7de146a
Compare
Third pickup — the two prior reviews agree and the ask is prose, so I applied it rather than writing a third concurring reviewBoth reviews above reach the same disposition: the selector code is correct What I verified before touching anythingI did not re-run the exactness suite — it has been independently I did re-check the two facts the corrections actually rest on:
The correction to the correction is the right one, and it matters: the first Three commits, prose only
The PR body carried the same claims and has been rewritten, with the What I did not doThe additive relevance floor. Behaviour-changing, wants its own replay, The State
DispositionI cannot label this. I have commits on the branch now, so the label is not The (1 − 1/e) error also originates in #1176's proposal 2, which states the |
|
[release:review:Garsecg:2026-07-31T17:21:33Z] |
Follow-up R&D: why the objective does nothing, and it is not
|
| λ (× mean term weight) | packs differing from the shipped multiplicative form |
|---|---|
| 0.0 | 44/150 (29.3%) |
| 0.1 | 92/150 (61.3%) |
| 0.5 | 92/150 (61.3%) |
| 1.0 | 92/150 (61.3%) |
| 4.0 | 92/150 (61.3%) |
A 40× range in λ producing byte-identical packs is not a result, it is a symptom.
First hypothesis, refuted
I guessed the coverage map was mostly empty — every marginal would then be
λ·rank_weight, the ratio would scale uniformly, and λ would cancel. Measured
it: 5.3% of candidates have empty coverage, mean coverage-set size 2.96
terms, and 0 of 149 prompts have an all-empty map. Wrong.
What is actually happening
The universe U is only the query's own stems. On these prompts:
coverable query stems (universe) : mean 8.2 median 8
L1 candidates per prompt : mean 40.3
picks to SATURATE the universe : mean 2.6 median 2 max 6
prompts saturated within 5 picks : 148/149 (99.3%)
A median of 2 beliefs covers everything the objective can ever reward. After
that every remaining marginal is exactly 0, so the greedy's ratio is
λ·rank_weight / cost for all survivors — and λ cancels uniformly, which is the
invariance above. The remaining ~37 of 40 slots are filled by the
relevance-over-cost tie-break, i.e. by a rank-greedy fill.
A hard bound, independent of the greedy's choices: each pick carrying non-zero
gain must bring at least one new term, so at most 8.2 picks of 40.3 can be
decided by coverage at all. Greedily it is 2.6. Either way the objective
decides ~7% of the pack and rank order decides the rest.
What this means for #1238
The PR's conclusion — "at the shipped default budget the lane does essentially
nothing" — is right, and its own near-duplicate-pair table is exactly what
you would expect when 37 of 40 slots are picked identically to rank-greedy.
But the stated reason is wrong twice over. It is not that l1_limit binds
(0 of 323 prompts reach the 50-candidate ceiling — measured earlier in this
review), and it is not something a budget change fixes. The objective
saturates two picks in because the universe is a handful of query stems.
Raising the budget adds slots the objective has no opinion about; it makes the
ratio worse, not better.
That also retires my own follow-up suggestion in its current form. Switching
multiplicative → additive changes 61% of packs, but every one of those changes
happens in the post-saturation region where the "coverage objective" is not
doing coverage — it is a cost-normalised rank sort under a different constant.
Cleaning up the submodularity argument is still correct on the merits and the
docstring still needs it, but it is not a retrieval-quality lever and should not
be sold as one.
The lever, if anyone wants one
For budgeted maximum coverage to bind, the universe has to be comparable in size
to the number of slots. U = query stems gives ~8 against ~40. Candidates:
U= all terms appearing in the candidate pool, weighted by idf — turns the
objective into "cover the topic the pool is about", which is the diversity
problem the cluster pack was reaching for.U= query stems plus expansion terms.- Keep
Uas-is and cap the pack near the saturation point, treating coverage
as a selector for the first few slots only and rank-greedy for the rest —
which is what it already does, just made explicit and cheap.
The first is the only one that makes the (1 − 1/√e) machinery earn its keep. All
three are design changes, not tuning, and belong on #1176 rather than in this
PR.
Method note
Every number above is from replaying real ingest_log user prompts against a
WAL-safe snapshot of the live store, comparing l1_ids rather than the merged
output — the merged top-k is dominated by the constant 25-belief lock set and
hides the pack. Saturation is measured as a greedy set-cover ceiling over the
coverable universe, so it is the best case for the objective, not the worst.
|
[claim:review:Setr:2026-07-31T17:31:02Z] |
Frames pack selection as budgeted maximum coverage over query terms (Khuller-Moss-Naor 1999) instead of ranked fill: the value of adding a belief is the query-term mass nothing already selected covers. Pure function -- coverage and term weights are passed in, so it carries no BM25 or store coupling and is testable without a lane. Cost-benefit greedy plus the best single feasible element, whichever scores higher, which is what carries the (1 - 1/e) bound. CELF lazy evaluation over a max-heap of stale upper bounds; submodularity makes those bounds valid, so the lazy form is exact rather than approximate. That exactness is the load-bearing test, and it failed first: comparing recomputed ratios alone rather than the full (-ratio, rank, id) key diverged from the eager greedy on 13 of 60 random inputs, because on a ratio tie the eager form takes the lower-ranked element. Now 0 divergences over 2000 seeded cases against an independent reference implementation, with a guard test that the reference does not call the implementation. Not yet wired into retrieval.
…2, default off (#1176) Wires pack_max_coverage behind AELFRICE_MAX_COVERAGE_PACK / [retrieval] use_max_coverage_pack, defaulting off. It takes precedence over the cluster pack when both resolve on, since the two are alternative answers to the same question and running both would pack twice. Coverage is the query's stems intersected with each belief's, using the same tokenize_stemmed the BM25 lane indexes with. Term weights are idf from the built index when one exists; without an index every term weighs 1.0, which degrades to unweighted coverage rather than to a no-op, so the FTS5 lane still gets redundancy suppression. The wiring is asserted by observing the call rather than by diffing the pack. Constructing a fixture where the coverage pack differs from a rank fill is hard, and the reason is the measurement itself: BM25 idf already pushes term-diverse beliefs to the top, so the two agree on most inputs.
|
[claim:review:Toug:2026-07-31T17:33:44Z] |
|
[release:review:Toug:2026-07-31T17:33:51Z] |
Cost-benefit greedy paired with the best single feasible element is Khuller-Moss-Naor's modified greedy, which carries (1 - 1/sqrt(e)) ~= 0.3935. The docstring and the arm test claimed (1 - 1/e) ~= 0.6321, which is a different algorithm: partial enumeration over subsets up to cardinality d = 3, each completed by this greedy. Over-claimed by 0.24. Also names the rank-weight relevance floor as a deviation rather than a free addition: the multiplier is applied per element, so the weighted gain is path-dependent and is not the marginal of any set function -- no submodularity argument covers it, and neither bound is claimed for the weighted objective. Records the additive form as the honest alternative, as follow-up rather than a claim about today's code. Prose only; no behaviour change.
`_coverage_inputs` computes coverage from `b.content`, but the pack emits `compress_for_retrieval(b).rendered` and type-aware compression defaults on. On a retention class that does not render verbatim -- snapshot (headline) or transient (stub) -- the objective can credit a term the agent never receives and mark it covered, suppressing a later belief that would have delivered it. This is the seam #878 closed for the cost currency; `cost_fn=_cost` inherits that fix and coverage has no counterpart. Latent, not live: 0 of 5,150 replayed L1 candidates rendered non-verbatim, because the corpus is 86.0% fact and 13.9% unknown against 0.09% snapshot and zero transient. Documented so whoever raises that share finds the tripwire instead of a silent objective drift. Docstring only; no behaviour change.
#1176) Three factual corrections to the entry, all surfaced by review: - The bound is (1 - 1/sqrt(e)), not (1 - 1/e); the stronger constant needs the d = 3 partial enumeration. The rank weight is a deviation that no submodularity argument covers, so it is named as one. - "l1_limit is the binding constraint, ~47.8 of 50 candidates packed, so any A/B here measures a no-op" does not reproduce. A replay of 323 real prompts reaches the 50-candidate ceiling 0 times, mean pack 34.4; the budget binds and the pack only nears 47 around budget 4,000. The L1 pack differs on two-thirds of prompts at the default, so a retrieval-quality A/B is measurable today -- it would score on ~66% of prompts, which is the experiment #1176 wants. - DEFAULT_TOKEN_BUDGET is 2,400. The entry called 2,000 the shipped default; 2,000 is LEGACY_TOKEN_BUDGET. The recommendation still stands (ship off, A/B before flipping), but it no longer points the next step at l1_limit, which is the wrong knob.
7de146a to
3b78609
Compare
|
Picked this up as the "session with no commits here" the third pickup asked for. I had none when I claimed — I do now, because the branch went Rebased. The conflict was collateral from #1175's PR merging ~10 minutes earlier, and both branches append to Two conflicts, and the second one mattered. The first was a plain concat — kept main's #1175 bullet, appended this branch's #1176 bullet. The second was on Verified the resolution rather than trusting it: Note the corrected text also contains one Confirming the corrections, which is what was actually asked
I did not re-run the exactness suite — it has been independently reproduced twice against separately-derived eager references, and a third pass adds nothing. State after the rebase
Rollback: Adding |
robotrocketscience
added
ready-to-merge
|
[release:review:Setr:2026-07-31T17:37:21Z] |
3b78609 to
ea36f9b
Compare
|
merge-train: blocked branch head moved during merge-train queue (event= The |
github-actions
Bot
removed
the
ready-to-merge
I caused the merge-train block above — no work was lostTwo of us rebased this branch onto the same The one piece of luck is that it cost nothing but the queue slot: Both rebases resolved the two Verification on the current head
Re-adding |
|
merge-train: merged ea36f9b → |
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged ea36f9b → |
1 participant
Builds #1176 proposal 2 (budgeted maximum coverage as the L1 pack selector),
default off. Stage 1 of the proposal's own kill experiment is
on the issue;
stage 2 is below and is mixed enough that the flag stays off.
What ships
clustering.pack_max_coverage— a pure function. Coverage and term weights arepassed in, so it carries no BM25 or store coupling and is testable without a
lane. Cost-benefit greedy (Khuller–Moss–Naor 1999) plus the best single
feasible element, whichever scores higher; that pair is KMN's modified
greedy and carries (1 − 1/√e) ≈ 0.3935. Dropping the single-element arm
is exactly how even that bound is lost, so the case is pinned.
CELF lazy evaluation over a heap of stale upper bounds. Submodularity makes
those bounds valid, so the lazy form is exact, not an approximation of the
greedy.
Wired into
retrieve_with_tiersbehindAELFRICE_MAX_COVERAGE_PACK/[retrieval] use_max_coverage_pack, taking precedence over the cluster packwhen both resolve on (two answers to the same question; running both would
pack twice).
The exactness claim is the load-bearing test, and it failed first
The proposal names CELF-equals-eager as its own unit test. Mine did not pass
initially: comparing recomputed ratios alone rather than the full
(−ratio, rank, id)key diverged from the eager greedy on 13 of 60 randominputs, because on a ratio tie the eager form takes the lower-ranked element
and CELF took whichever happened to be popped.
Fixed and re-verified: 0 divergences over 2,000 seeded cases against an
independent reference implementation, plus a guard test asserting the reference
does not call the implementation — otherwise the equality test becomes a
tautology that passes against any bug.
Stage 2: measured, and the result is mixed
523 distinct prompts from
hook_audit.jsonlreplayed against the live44,584-belief store. User and harness arms reported separately — pooling
has flipped a gate on this repo before, and they diverge here too.
Near-duplicate pairs per pack among unlocked beliefs (the ones the selector
actually chooses), 4-gram Jaccard ≥ 0.50:
At a 2000-token budget the near-duplicate pair count barely moves.
Where selection binds it helps modestly, and only on the user arm. At budget
1000 the coverage pack carries 28.7 unlocked beliefs against rank-greedy's
27.8 while cutting ≥0.50 pairs from 5.48 to 5.24 — more beliefs and less
redundancy, ≈7% fewer redundant pairs per belief. At 600: 18.7 against 17.5,
2.17 → 2.10. On the harness arm it is neutral at best and loses to plain
rank-greedy at the ≥0.25 threshold.
Two things worth carrying forward
The kill experiment's own metric would have killed this. It specifies mean
intra-pack Jaccard, which reads 0.0050 vs 0.0051 — "no redundancy, no
difference". A ~72-belief pack has ~2,500 pairs, almost all unrelated, so the
mean is diluted by three orders of magnitude. Pair counts and max pairwise
Jaccard are what bind. Recommended amendment is on the issue.
A third of every pack is the 25-belief locked set, which the selector
cannot touch — that bounds the reachable headroom. The 3.00 near-duplicate
pairs among the locks are constant in every single pack, since the same locks
are injected every turn. That is lock hygiene, not pack selection.
On the wiring test
Asserted by observing that
pack_max_coverageis called iff the flag is on,rather than by diffing the pack. The selector's own behaviour is covered
directly by the unit tests.
Mutation-checked: forcing the branch off fails the wiring test and nothing else.
Also documented after review
_coverage_inputscomputes coverage fromb.content, but the pack renderscompress_for_retrieval(b).renderedand type-aware compression defaults on. Ona retention class that does not render verbatim —
snapshot(headline) ortransient(stub) — the objective can credit a term the agent never receivesand mark it covered, suppressing a later belief that would have delivered it.
This is the seam #878 closed for the cost currency;
cost_fn=_costinheritsthat fix and coverage has no counterpart.
Measured incidence is nil — 0 of 5,150 replayed L1 candidates render
non-verbatim, the corpus being 86.0%
fact/ 13.9%unknownagainst 0.09%snapshotand zerotransient— so this ships as a docstring tripwire, not acode change.
Recommendation
Keep the default off. Flipping it needs a retrieval-quality A/B — which is
measurable at today's defaults, since it would be scored on the ~66% of prompts
where the pack differs. That is the experiment #1176 wants. An
l1_limitchange is not the knob this measurement points at.
Full suite: 6755 passed, 69 skipped, 71 xfailed (rebased onto current
main).Summary by Sourcery
Introduce a budgeted maximum-coverage pack selector for L1 retrieval, gated behind a new flag and default-off, with supporting coverage utilities and wiring into the retrieval pipeline plus tests and documentation updates.
New Features:
pack_max_coverageas a pure budgeted maximum-coverage selector over query terms for L1 packs..aelfrice.toml, taking precedence over clustering when on.Enhancements:
retrieve_with_tiersas an alternative to the cluster-based pack, using BM25-derived coverage and term weights when available.Documentation:
Tests:
pack_max_coverageproperties (exactness vs eager greedy, determinism, tie-break behavior, budget adherence) and retrieval wiring including flag resolution and coverage-input behavior.