fix(retrieval): reserve a relevance budget floor so locks can't starve query-relevant hits (#1014) - #1015
Merged
Conversation
Reviewer's GuideImplements a relevance token budget floor in retrieval so that uncapped locked beliefs cannot consume the entire budget, updates both retrieval paths to respect this floor, and adds tests and changelog entry for the new behavior. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 40 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 Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds Relevance-Budget Floor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The relevance budget calculation (
relevance_budget = max(...)) is duplicated in both retrieval paths; consider extracting this into a small helper to keep the lock-saturation logic consistent and easier to adjust in the future. - The changelog entry for this fix is quite long and implementation-heavy; you could tighten it to focus on the behavioral change (locks no longer starve relevant hits) and the high-level tradeoff, leaving out detailed formulae and test anecdotes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The relevance budget calculation (`relevance_budget = max(...)`) is duplicated in both retrieval paths; consider extracting this into a small helper to keep the lock-saturation logic consistent and easier to adjust in the future.
- The changelog entry for this fix is quite long and implementation-heavy; you could tighten it to focus on the behavioral change (locks no longer starve relevant hits) and the high-level tradeoff, leaving out detailed formulae and test anecdotes.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_relevance_budget_floor.py (1)
38-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a parity regression for
retrieve_with_tiers().This new file only exercises
retrieve(), but the same floor/guard math was duplicated intoretrieve_with_tiers(). A lock-saturation test against the tiered API would keep the two entry points from drifting.🤖 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_relevance_budget_floor.py` around lines 38 - 67, Add a regression test that exercises retrieve_with_tiers() with the same lock-saturation scenario used for retrieve(), since the floor/guard logic was duplicated there and could drift. Reuse the existing test setup patterns in test_relevance_budget_floor.py and verify that a query-relevant non-lock belief still appears under heavy locked-belief saturation when calling retrieve_with_tiers(), while keeping the existing retrieve() coverage intact.
🤖 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 `@src/aelfrice/retrieval.py`:
- Around line 121-133: The public budget contract in the retrieval entry points
is now stale because lock-saturated queries can exceed token_budget by the
reserved relevance floor. Update the docstrings/comments for retrieve() and
retrieve_with_tiers() in retrieval.py so they explicitly describe the new
behavior: locks are never trimmed, and total output may go above token_budget in
the saturated regime to preserve relevance tokens. Keep the wording aligned with
the existing budget logic and the RELEVANCE_BUDGET_FLOOR_FRACTION behavior.
---
Nitpick comments:
In `@tests/test_relevance_budget_floor.py`:
- Around line 38-67: Add a regression test that exercises retrieve_with_tiers()
with the same lock-saturation scenario used for retrieve(), since the
floor/guard logic was duplicated there and could drift. Reuse the existing test
setup patterns in test_relevance_budget_floor.py and verify that a
query-relevant non-lock belief still appears under heavy locked-belief
saturation when calling retrieve_with_tiers(), while keeping the existing
retrieve() coverage intact.
🪄 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
Run ID: 9de5621d-327e-4bf7-a6d6-e2718d835305
📒 Files selected for processing (3)
CHANGELOG/v3.mdsrc/aelfrice/retrieval.pytests/test_relevance_budget_floor.py
…e query-relevant hits (#1014) L0 locked beliefs are injected unconditionally and never trimmed (#379), and their tokens are subtracted from the L2.5/L1 budget. A store whose locks alone met or exceeded the token budget therefore returned ONLY the locks for every prompt — zero query-relevant content (real store: 12 locks = 2485 tokens vs a 2400 budget -> 0 relevance tokens). Reserve a relevance floor: relevance_budget = max(effective_budget * RELEVANCE_BUDGET_FLOOR_FRACTION, effective_budget - locked_used); cap L2.5+L1 fills at locked_used + relevance_budget. Byte-identical outside the lock-saturated regime (lock-free corpora like LoCoMo unaffected); locks still never trimmed. Validated on the real store: query-relevant results went 0 -> 20-27 per query. 885 retrieval/ranking/budget/lock tests pass.
robotrocketscience
force-pushed
the
fix/retrieve-lock-budget-floor
branch
from
June 29, 2026 22:36
9d2b566 to
0a97652
Compare
robotrocketscience
added
the
ready-to-merge
|
merge-train: merged 0a97652 → |
github-actions
Bot
removed
the
ready-to-merge
This was referenced Jun 30, 2026
This was referenced Jul 29, 2026
Closed
robotrocketscience
added a commit
that referenced
this pull request
Jul 30, 2026
…king it The candidate limit is applied by the search — SQL LIMIT on the FTS5 path, top_k on BM25F — so filtering superseded beliefs afterwards dropped the pack size by however many were retired and left current, relevant beliefs stranded just below the cutoff: l1_limit=4, three of the top four retired -> 1 belief returned l1_limit=10, eight of the top ten retired -> 2 beliefs returned Same shape as the lock-budget starvation fixed in #1014/#1015: a filter applied after the budget starves the pack. In the degenerate case every top-l1_limit candidate is retired and the arm returned nothing while the store held the answer. The exclusion arm now widens the fetch and retries, stopping as soon as it has l1_limit survivors or the search runs out of matches, bounded at three rounds. The demote arm is untouched — it reorders a fixed candidate set, which is why it always measured full. This also matters for the ratified three-arm bench: with the arms differing in pack size as well as in treatment, a loss for exclusion could not have been attributed to either. Raised in review on #1191.
robotrocketscience
added a commit
that referenced
this pull request
Jul 30, 2026
…king it The candidate limit is applied by the search — SQL LIMIT on the FTS5 path, top_k on BM25F — so filtering superseded beliefs afterwards dropped the pack size by however many were retired and left current, relevant beliefs stranded just below the cutoff: l1_limit=4, three of the top four retired -> 1 belief returned l1_limit=10, eight of the top ten retired -> 2 beliefs returned Same shape as the lock-budget starvation fixed in #1014/#1015: a filter applied after the budget starves the pack. In the degenerate case every top-l1_limit candidate is retired and the arm returned nothing while the store held the answer. The exclusion arm now widens the fetch and retries, stopping as soon as it has l1_limit survivors or the search runs out of matches, bounded at three rounds. The demote arm is untouched — it reorders a fixed candidate set, which is why it always measured full. This also matters for the ratified three-arm bench: with the arms differing in pack size as well as in treatment, a loss for exclusion could not have been attributed to either. Raised in review on #1191.
robotrocketscience
added a commit
that referenced
this pull request
Jul 30, 2026
…king it The candidate limit is applied by the search — SQL LIMIT on the FTS5 path, top_k on BM25F — so filtering superseded beliefs afterwards dropped the pack size by however many were retired and left current, relevant beliefs stranded just below the cutoff: l1_limit=4, three of the top four retired -> 1 belief returned l1_limit=10, eight of the top ten retired -> 2 beliefs returned Same shape as the lock-budget starvation fixed in #1014/#1015: a filter applied after the budget starves the pack. In the degenerate case every top-l1_limit candidate is retired and the arm returned nothing while the store held the answer. The exclusion arm now widens the fetch and retries, stopping as soon as it has l1_limit survivors or the search runs out of matches, bounded at three rounds. The demote arm is untouched — it reorders a fixed candidate set, which is why it always measured full. This also matters for the ratified three-arm bench: with the arms differing in pack size as well as in treatment, a loss for exclusion could not have been attributed to either. Raised in review on #1191.
robotrocketscience
added a commit
that referenced
this pull request
Jul 30, 2026
…king it The candidate limit is applied by the search — SQL LIMIT on the FTS5 path, top_k on BM25F — so filtering superseded beliefs afterwards dropped the pack size by however many were retired and left current, relevant beliefs stranded just below the cutoff: l1_limit=4, three of the top four retired -> 1 belief returned l1_limit=10, eight of the top ten retired -> 2 beliefs returned Same shape as the lock-budget starvation fixed in #1014/#1015: a filter applied after the budget starves the pack. In the degenerate case every top-l1_limit candidate is retired and the arm returned nothing while the store held the answer. The exclusion arm now widens the fetch and retries, stopping as soon as it has l1_limit survivors or the search runs out of matches, bounded at three rounds. The demote arm is untouched — it reorders a fixed candidate set, which is why it always measured full. This also matters for the ratified three-arm bench: with the arms differing in pack size as well as in treatment, a loss for exclusion could not have been attributed to either. Raised in review on #1191.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes #1014: when a store's locked beliefs (L0) alone meet or exceed the retrieval token budget,
retrieve()/retrieve_v2()returned only the locks for every prompt — zero query-relevant content surfaced.Why
L0 locks are injected unconditionally and never trimmed (#379); their tokens are subtracted from the L2.5/L1 budget. Once locks fill the budget,
l25_roomandl1_remaining_budgetboth compute to 0. Found on a real store: 12 locks = 2485 tokens vs the 2400 default budget → 0 relevance tokens, so every query returned the same 12 locks.Fix
Reserve a relevance floor:
relevance_budget = max(effective_budget × RELEVANCE_BUDGET_FLOOR_FRACTION, effective_budget − locked_used)(fraction 0.25); cap L2.5+L1 fills atlocked_used + relevance_budgetin bothretrieve()andretrieve_with_tiers().locked_used + relevance_budget == effective_budget), i.e. it only fires in the lock-saturated regime. Lock-free corpora (LoCoMo) are unaffected.Verification
test_relevance_budget_floor.py): saturating locks → ≥1 relevant belief surfaces; locks-fit → all relevant surface unchanged.Distinct from prior work
Closes #1014.
Summary by Sourcery
Reserve a relevance token budget floor in retrieval so locked beliefs cannot consume the entire budget and suppress query-relevant results.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests