feat(retrieval): flip entity-persistence demotion default-on in retrieve_v2 (#1096) - #1108
Conversation
…1096) Flip use_entity_persist_demote to default-on in the retrieve_v2 resolver now that the G2 mixed-corpus eval (#1103) cleared the no-regression gate (durable recall 20->20, ephemeral demoted 20->3 at the tight budget, MRR 0.883->1.000; recall-safe/inert on LoCoMo). The resolver also gains a [retrieval] use_entity_persist_demote TOML rung (env -> kwarg -> TOML -> default True), matching the use_heat_kernel idiom so per-project opt-out works without an env var. Scope: the lane is wired into retrieve_v2() only; the legacy retrieve() hook path does not expose it (production-path gap tracked in #1107). Tests updated for the flipped default: resolver default-on + TOML opt-out coverage, and the byte-identical-when-off assertions now pin the explicit off path instead of the (now default-on) no-flag path.
#1096) Update CONFIG.md, ARCHITECTURE.md, LIMITATIONS.md, and the CHANGELOG for the #1096 default-on flip. State the retrieve_v2-only scope explicitly so the docs do not imply the live retrieve() hook path demotes (it does not; gap tracked in #1107), correct the precedence line to default true with a TOML opt-out rung, and note the origin tie-break remains default-off.
![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
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 |
|
Warning Review limit reached
Next review available in: 28 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 (1)
📝 WalkthroughWalkthroughThe entity-persistence demotion flag now defaults to True (previously False) for ChangesEntity-persist-demote default-on flip
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
🚥 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 |
robotrocketscience
added
the
attn:review
![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_entity_persist_demote.py (1)
207-211: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTest relies on ambient cwd for TOML resolution.
Unlike
test_resolver_toml_opts_out(which isolates viastart=tmp_path), this test callsis_entity_persist_demote_enabled(None)without astart, so the resolver walks up fromPath.cwd(). If the repo root (or any ancestor of the test-runner's cwd) ever gains a.aelfrice.tomlwith[retrieval] use_entity_persist_demote = false, this test would flip to failing even though the resolver logic is correct — a false negative unrelated to the code under test.♻️ Suggested isolation
-def test_resolver_default_on(monkeypatch: pytest.MonkeyPatch) -> None: +def test_resolver_default_on( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: # v4.0 flip (`#1096/`#1103): with no env/kwarg/TOML rung set, the lane # now resolves ON. monkeypatch.delenv("AELFRICE_ENTITY_PERSIST_DEMOTE", raising=False) - assert is_entity_persist_demote_enabled(None) is True + assert is_entity_persist_demote_enabled(None, start=tmp_path) is True🤖 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_entity_persist_demote.py` around lines 207 - 211, The default-on resolver test is picking up TOML from the ambient current working directory, so it can fail due to external config rather than the code under test. Update test_resolver_default_on in tests/test_entity_persist_demote.py to isolate the filesystem lookup the same way test_resolver_toml_opts_out does, by supplying an explicit temporary start/root and ensuring no .aelfrice.toml is visible to is_entity_persist_demote_enabled, while still asserting the default behavior when AELFRICE_ENTITY_PERSIST_DEMOTE is unset.
🤖 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 `@docs/concepts/ARCHITECTURE.md`:
- Around line 125-130: Clarify the scope statement in ARCHITECTURE.md so it does
not claim both rerank modifiers are retrieve_v2-only. Keep the
`use_entity_persist_demote` description tied to `retrieve_v2`, but update the
`use_origin_tiebreak` wording to reflect its support in `retrieve()` as well per
the configuration docs, using the `use_entity_persist_demote`,
`use_origin_tiebreak`, and `retrieve_v2`/`retrieve()` symbols to make the
distinction explicit.
---
Nitpick comments:
In `@tests/test_entity_persist_demote.py`:
- Around line 207-211: The default-on resolver test is picking up TOML from the
ambient current working directory, so it can fail due to external config rather
than the code under test. Update test_resolver_default_on in
tests/test_entity_persist_demote.py to isolate the filesystem lookup the same
way test_resolver_toml_opts_out does, by supplying an explicit temporary
start/root and ensuring no .aelfrice.toml is visible to
is_entity_persist_demote_enabled, while still asserting the default behavior
when AELFRICE_ENTITY_PERSIST_DEMOTE is unset.
🪄 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: d2fd0117-4459-4bee-a34c-02e2debe4091
📒 Files selected for processing (7)
CHANGELOG/v3.mddocs/concepts/ARCHITECTURE.mddocs/user/CONFIG.mddocs/user/LIMITATIONS.mdsrc/aelfrice/retrieval.pytests/bench_gate/test_entity_persist_g2_mixed_corpus.pytests/test_entity_persist_demote.py
robotrocketscience
added
the
ready-to-merge
|
merge-train: blocked 1 review thread(s) are unresolved on these files: docs/concepts/ARCHITECTURE.md. Resolve them on the PR (click 'Resolve conversation' on each) and re-add the label. The |
github-actions
Bot
removed
the
ready-to-merge
CodeRabbit review on #1108 flagged the ARCHITECTURE.md 'retrieve() exposes neither' wording as inconsistent with CONFIG.md. Verified against code: retrieve()'s signature has no use_origin_tiebreak param (only the helpers / retrieve_with_tiers / retrieve_v2 thread it), so ARCHITECTURE.md is correct and CONFIG.md overstated. Fix the CONFIG.md line to retrieve_v2 only, matching the entity-persist scope note.
|
Addressed the CodeRabbit thread on |
robotrocketscience
added
the
ready-to-merge
|
merge-train: merged ad60070 → |
github-actions
Bot
removed
the
ready-to-merge
1 participant
Summary
Flips
use_entity_persist_demotedefault-on inretrieve_v2(#1096), the operator-approved default-on call now that every evidence gate has cleared. The demotion mechanism shipped default-off (#1099); this changes only the resolver default (plus a TOML opt-out rung) and the tests/docs that encoded the old posture.Why now — all gates cleared
demote=Falsearms in the tests.noun_phraseentities).belief_entitiesjoin.What changed
is_entity_persist_demote_enabled): defaultFalse→True, and it now honours a[retrieval] use_entity_persist_demoteTOML rung (precedenceenv → kwarg → TOML → default True), matching theis_heat_kernel_enableddefault-on idiom. This also makes theCONFIG.mdprecedence claim (which already listed a TOML rung) actually true and gives per-project opt-out without an env var.test_resolver_default_on+ a new TOML opt-out test; the byte-identical-when-off assertions now pin the explicitdemote=Falsepath instead of the (now default-on) no-flag path; the G2 gate file's default-posture test reframed to assertdefault == explicit-onand that the lane still reorders vs the off-path.The lane is wired into
retrieve_v2()only. Production retrieval — theUserPromptSubmithook (hook_search.retrieve()),context_rebuilder, andmcp_server— all call the legacyretrieve(), which does not expose the flag. So this flip governsretrieve_v2consumers (benchmarks, the eval suite, direct callers) and does not change what a live host retrieves. Thatretrieve()/retrieve_v2()production-path gap affects all the recently-staged lanes (this one, origin tie-break #1089, temporal spine #1064, HRR) and is filed for disposition in #1107. The docs in this PR state theretrieve_v2-only scope so they are not misleading in the interim.Verification
pytestgreen: 5825 passed, 69 skipped, 75 xfailed.Closes #1096. Refs #1103, #1107.
Summary by CodeRabbit
New Features
retrieve_v2.Bug Fixes
Documentation