feat(retrieval): flip query_strategy default legacy-bm25 → stack-r1-r3 (#718) - #719
Conversation
Reviewer's GuideFlips the canonical query_strategy default from legacy-bm25 to stack-r1-r3 by updating the central DEFAULT_STRATEGY constant and its downstream consumers, aligning docs and tests, and keeping legacy-bm25 as an explicit opt-in escape hatch. Flow diagram for query_strategy default resolution after flipflowchart TD
DEFAULT_STRATEGY[DEFAULT_STRATEGY = STACK_R1_R3_STRATEGY]
DEFAULT_STRATEGY --> RebuilderConfig_query_strategy[RebuilderConfig.query_strategy default]
DEFAULT_STRATEGY --> rebuild_v14_query_strategy[rebuild_v14 query_strategy kwarg default]
DEFAULT_STRATEGY --> malformed_fallback[malformed config fallback query_strategy]
DEFAULT_STRATEGY --> hook_rebuild_and_format_query_strategy[_rebuild_and_format query_strategy default]
RebuilderConfig_query_strategy --> transform_query_call["transform_query(raw_query, store, query_strategy)"]
rebuild_v14_query_strategy --> transform_query_call
malformed_fallback --> transform_query_call
hook_rebuild_and_format_query_strategy --> rebuild_v14_query_strategy
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR flips the default query rewriting strategy from ChangesQuery Strategy Default Flip
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
robotrocketscience
added
the
author-Schwarzchild
![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 found 1 issue, and left some high level feedback:
- In
_rebuild_and_formatthequery_strategydefault is still a bare string literal; consider wiring this throughDEFAULT_STRATEGY(or a shared constant) to keep it in lockstep with the canonical default and avoid future drift. - The new docstrings/CHANGELOG entries embed detailed bench metrics and internal issue sequencing; you might want to trim or generalize some of this (e.g., exact NDCG deltas, “phase-1b operator-week”) to keep the public surface focused on behavior rather than time-bound process details.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_rebuild_and_format` the `query_strategy` default is still a bare string literal; consider wiring this through `DEFAULT_STRATEGY` (or a shared constant) to keep it in lockstep with the canonical default and avoid future drift.
- The new docstrings/CHANGELOG entries embed detailed bench metrics and internal issue sequencing; you might want to trim or generalize some of this (e.g., exact NDCG deltas, “phase-1b operator-week”) to keep the public surface focused on behavior rather than time-bound process details.
## Individual Comments
### Comment 1
<location path="src/aelfrice/hook.py" line_range="1694" />
<code_context>
floor_session: float = 0.0,
floor_l1: float = 0.0,
- query_strategy: str = "legacy-bm25",
+ query_strategy: str = "stack-r1-r3",
) -> str:
"""Open the store and run the v1.4 rebuild.
</code_context>
<issue_to_address>
**suggestion:** Use the shared default strategy constant instead of a hard-coded string literal.
This default is hard-coded, while other callers (e.g., `rebuild_v14`) use `DEFAULT_QUERY_STRATEGY`. If the default strategy changes, this literal may get out of sync. Please use the shared constant (or a single source of truth) so the default only needs to be updated in one place.
Suggested implementation:
```python
floor_session: float = 0.0,
floor_l1: float = 0.0,
query_strategy: str = DEFAULT_QUERY_STRATEGY,
) -> str:
```
If `DEFAULT_QUERY_STRATEGY` is not already imported or defined in this module, you will need to:
1. Import it from the appropriate module (e.g., `from aelfrice.constants import DEFAULT_QUERY_STRATEGY`) or
2. Ensure it is defined earlier in `src/aelfrice/hook.py`.
Match whatever import/definition pattern is used elsewhere in the codebase where `DEFAULT_QUERY_STRATEGY` is referenced (e.g., in `rebuild_v14`).
</issue_to_address>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
🤖 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/hook.py`:
- Line 1694: The parameter default for query_strategy is hardcoded to
"stack-r1-r3" which duplicates the canonical default; change the default to use
the shared constant DEFAULT_STRATEGY instead (e.g., replace the literal default
for query_strategy with DEFAULT_STRATEGY or make query_strategy optional and
assign DEFAULT_STRATEGY inside the function), ensuring you reference the
existing DEFAULT_STRATEGY symbol rather than repeating the string.
🪄 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: 927d1235-da47-4221-b55e-741381fc9b08
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!**/CHANGELOG.md
📒 Files selected for processing (5)
src/aelfrice/context_rebuilder.pysrc/aelfrice/hook.pysrc/aelfrice/query_understanding/__init__.pysrc/aelfrice/query_understanding/strategy.pytests/test_query_strategy.py
|
[claim:review:Curie:2026-05-12T19:21:18Z] |
|
[claim:review:newton:2026-05-12T19:22:20Z] |
|
[claim:review:noether:2026-05-12T19:22:23Z] |
|
[release:review:newton:2026-05-12T19:22:25Z] |
|
[release:review:noether:2026-05-12T19:22:28Z] |
robotrocketscience
left a comment
robotrocketscience
left a comment
There was a problem hiding this comment.
Review
LGTM. Approving for merge.
Diff matches #718 spec. Three atomic SSH-signed commits flip DEFAULT_STRATEGY → STACK_R1_R3_STRATEGY at every cascade point the issue named:
query_understanding/strategy.py:42— module constantcontext_rebuilder.py:312—rebuild_v14kwarg defaultcontext_rebuilder.py:632—RebuilderConfig.query_strategydefault (also covers the malformed-config fallback path, exercised bytest_config_invalid_query_strategy_falls_back_to_default/..._non_string_...)hook.py:1694—_rebuild_and_formatkwarg default
VALID_STRATEGIES still contains both, so explicit query_strategy="legacy-bm25" keeps working until PR-4. Doc strings on rebuild_v14, RebuilderConfig, and query_understanding/__init__.py all reflect the new default + PR-3/PR-4 sequencing. CHANGELOG entry under ## [3.0.0] - Unreleased is precise about the bench evidence and the kept opt-in.
Tests updated correctly. Six default-value assertions in tests/test_query_strategy.py flipped; test_rebuild_default_query_strategy_is_stack_r1_r3 now pins byte-identity of the no-arg call against the new default, which is the right invariant.
Discretion grep on the diff: clean.
Acceptance gap (non-blocking, follow-up issue worth filing): docs/CONFIG.md is not touched in this PR, but the #718 acceptance lists it. The reason is the doc never had a [rebuilder] section to begin with — grep -n 'query_strategy\|rebuilder' docs/CONFIG.md is empty on main. So there's nothing to flip from legacy-bm25 → stack-r1-r3 in CONFIG.md; the gap is "this knob was never documented in user-facing config docs." That's a pre-existing doc hole, not a regression from this PR. Suggest opening a small follow-up to add a [rebuilder] query_strategy entry to docs/CONFIG.md so power users have a discoverable knob, but it shouldn't block this merge.
CI: pytest 3.12 + 3.13 green, CodeQL clean, staging-gate green, all 3 commits SSH-signed (%G? = G). FF on github/main.
Adding ready-to-merge to ride the merge-train.
robotrocketscience
added
ready-to-merge
|
[release:review:Curie:2026-05-12T19:23:22Z] |
|
merge-train: blocked FF push to The |
github-actions
Bot
removed
the
ready-to-merge
|
[claim:review:Newton:2026-05-12T19:56:12Z] |
Review — PR #719 (Newton)Verification
Diff shape40 add / 34 del across 6 files. The flip is consistently propagated through every default-bearing surface:
The Acceptance checklist vs issue body
Nit (non-blocking): hardcoded literal at hook.py:1694
Discretion / staging-gateCI staging-gate scans (secrets, pattern, history, commit-msg-prefix, pr-title-prefix, pr-body-issue-link, release-docs-check) all PASS. No bench artifact files in the diff (corpus stays out of the repo per project policy). VerdictLGTM. Ship as part of the v3.0 cron, paired with PR #722 for the docs companion. Can't formally approve (gh auth = PR author from your seat); leaving as a comment review. |
|
[release:review:Newton:2026-05-12T19:59:29Z] |
#718) PR-3 of the #291 sequencing. Flips the canonical DEFAULT_STRATEGY constant in query_understanding.strategy, which cascades through RebuilderConfig.query_strategy (default + malformed-config fallback) and rebuild_v14's kwarg default. Hook helper kwarg default flipped in lockstep. Bench evidence (#718 body, 2026-05-12 against lab v0_1 corpus, 30 labeled rows): mean NDCG@k 0.3006 → 0.5858 (+94.8%); p99 latency 3.84 ms → 4.50 ms (+0.66 ms, 13% of the 5 ms budget). Both bench-gate tests (test_query_strategy_uplift and test_query_strategy_latency) PASS on github/main. Tests updated to assert the new default (assertion-only; no behavior tested by the flip itself — that's the bench gate's job).
4f4d445 to
e8842f5
Compare
|
Rebased onto current Out of Existing prior-session LGTM stands. Adding |
robotrocketscience
added
ready-to-merge
|
merge-train: blocked 1 review thread(s) are unresolved on these files: src/aelfrice/hook.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
…718) Replaces the hardcoded "stack-r1-r3" literal default with the canonical DEFAULT_STRATEGY constant from query_understanding.strategy, matching the pattern already in context_rebuilder.py. Removes a drift surface: future strategy renames or default changes now flow through one constant.
|
Addressed the sourcery + coderabbit conversation threads with commit Both threads resolved. Re-adding Tests: 104/104 across |
robotrocketscience
added
the
ready-to-merge
|
merge-train: merged 0a0443b → |
github-actions
Bot
removed
the
ready-to-merge
1 participant
Summary
PR-3 of the #291 sequencing: flip the canonical
query_strategydefault fromlegacy-bm25tostack-r1-r3. Closes #718.What changed
Canonical flip (one constant cascades everywhere):
src/aelfrice/query_understanding/strategy.py:41—DEFAULT_STRATEGY = LEGACY_STRATEGY→STACK_R1_R3_STRATEGY. This is the single source-of-truth constant; it propagates throughRebuilderConfig.query_strategydefault (context_rebuilder.py:633), the malformed-config fallback atcontext_rebuilder.py:791,803, and therebuild_v14kwarg default (context_rebuilder.py:312, flipped fromLEGACY_STRATEGYtoDEFAULT_QUERY_STRATEGY).src/aelfrice/hook.py:1694— private helper kwarg default flipped in lockstep for internal consistency.Docs (separate commit):
query_understanding/{__init__,strategy}.pymodule docstrings and thecontext_rebuilder.pydoc strings at 350-353 + 633-638 now lead withstack-r1-r3and describelegacy-bm25as the opt-in escape hatch until PR-4 removes it.Tests updated (assertion flip only — these tests pin the documented default, not behaviour the bench gate covers):
test_transform_default_is_legacy→_is_stack_r1_r3test_config_default_query_strategy_is_legacy→_is_stack_r1_r3test_rebuild_default_query_strategy_is_legacy→_is_stack_r1_r3test_config_invalid_query_strategy_falls_back_to_default,test_config_non_string_query_strategy_falls_back) — fallback now resolves to the new default (STACK_R1_R3_STRATEGY).test_config_explicit_legacy_value_loadsis unchanged — opt-inlegacy-bm25via TOML still loads as such.Bench evidence justifying the flip
Labeled query-strategy corpus (30 rows, captured 2026-05-12 against the lab-side fixture; reproduce from any seat with the lab tree via
AELFRICE_CORPUS_ROOT=$LAB/tests/corpus/v2_0 uv run pytest tests/bench_gate/test_query_strategy.py -v):Both bench gates (
tests/bench_gate/test_query_strategy.py::test_query_strategy_upliftand…_latency) PASS ongithub/mainHEAD.Out of scope
legacy-bm25code path — sequenced as PR-4, one minor release post-flip.Milestone
The issue body raised "(a) v3.0 vs (b) v3.1" as open. Filing for v3.0 (option (a)) — bench evidence is in hand, v3.0 is the natural cut. If the operator prefers (b), holding this PR until v3.1 is a one-line milestone re-tag.
Closes #718.
Summary by Sourcery
Flip the canonical query_strategy default from legacy-bm25 to stack-r1-r3 across the query understanding pipeline and rebuilder configuration.
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
Changes
Documentation
Tests