Repository navigation
Conversation
Per review: a fetch section took one ranked page of `wanted` hits and let settle drop the excluded ones (the prompt's thread window, shown ids), so a page made of in-window turns left the section empty even when eligible hits sat on the next page. Exclusions now apply page by page, and only when a page lost hits to them and too few remain is the next page read (cap 5), so the common case is still one request. Revert-checked test.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughRanked recall can fetch up to five pages when exclusions leave fewer hits than requested. It requests beliefs on the first page only. A regression test covers recall returning an eligible turn after earlier results are excluded. ChangesRanked Recall
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~12 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to Ranked recall can continue past excluded hits, and no actionable merge-blocking risk remains after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Pagination preserves the existing access scope and exclusions and stops after at most five pages. No access-control regression was identified. Additional reads increase load, and consistency during concurrent updates is not guaranteed. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
A rabbit checks the ranked page, Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
2 participants
Summary
Follow-up to #206, which merged one commit early: GitHub never synced its last push to the PR, so this fix was reviewed on #206 but not merged. A ranked (
fetch) section now pages past hits its exclusions drop, aslatestalready does since #206.Related issue
Part of tinyhumansai/openhuman#7023 / tinyhumansai/openhuman#6718. Addresses the CodeRabbit finding on #206 ("Apply exclusions before limiting fetched hits").
The bug
fetchasked for one ranked page ofwantedhits and letsettledrop the excluded ones afterwards: the prompt's thread window, and ids already shown. If that page was mostly in-window turns, the section came back short or empty even when eligible hits sat on the next page.The fix
fetchapplies the request's exclusions page by page.next_cursoronly when a page lost hits to them and fewer thanlimitremain. It is capped atFETCH_MAX_PAGES = 5.&(dyn Fn(&Hit) -> bool + Sync)predicate, so the recall future staysSend;a_recall_future_can_cross_threadsstill passes.API or behavior changes
None to the public API. Behaviour: a ranked section can now return eligible hits from later pages instead of fewer or none.
Validation
cargo fmt --all -- --check: passcargo clippy --all-targets --all-features -- -D warnings: passcargo build --all-targets --all-features: covered by clippy and testcargo test --all-features: pass (1132 passed, 0 failed)Tests
recall::tests::a_ranked_section_reads_past_a_page_the_thread_window_empties: the in-window turns outrank the eligible ones, so the first three pages are all in-window; the section must still find turn 0 or 1. Revert-checked: fails without the page-by-page filter, passes with it.Documentation
Not needed: internal recall behaviour; the doc comments on
fetchandFETCH_MAX_PAGESexplain it.Checklist
#[allow(...)],#[ignore], or relaxed lints.envcontents in the diff or the descriptionSummary by CodeRabbit