feat(retrieval): adaptive expansion-gate for broad prompts (#741) - #743
Conversation
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
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ 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 |
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 |
|
[claim:review:pascal:2026-05-13T20:34:03Z] |
robotrocketscience
left a comment
There was a problem hiding this comment.
Review
Substance — looks good
The expansion-gate design is clean and matches #741 spec:
- Resolver precedence (force → no-gate → TOML → heuristics) implemented exactly as the issue body specifies.
_env_force_expansionand_env_no_expansion_gateuse a shared truthy/falsy frozenset; clean. - Heuristics honor PHILOSOPHY #605 — stdlib only, deterministic, regex-based. No embeddings, no model calls.
- Three signals OR'd (long-prompt, no-markers, question-form). Any "broad" trip wins. Conservative-by-design as the issue mandates.
ExpansionDecisionfrozen dataclass withreasonfield for telemetry — exactly the surface #741 asked for.LaneTelemetryadditive fields (expansion_gate_reason,expansion_gate_skipped_bfs) preserve byte-identical surface for pre-#741 callers; defaults are safe.- BFS-internal test fixture override (
AELFRICE_FORCE_EXPANSION=1intest_bfs_multihop.py's isolated env) is the right call — keeps AC1-AC11 BFS-behavior tests asserting what they were built to assert rather than implicitly testing gate behavior. should_run_expansionis called from bothretrieve()andretrieve_with_tiers()— full coverage of the retrieval surface._starts_with_question_formrespects word boundaries (rejectswhatevermatchingwhat). Good.- HRR-structural lane intentionally not gated in v1 — comment explains the
run_hrr_structuralfield is forward-compat. Defensible v1 scope per the issue.
BLOCKER — rebase required before merge
Branch base is 13a57ac (pre-#738). github/main is now at 8ea9ab5 after #738 merged today. The wider diff github/main..HEAD therefore shows the inverse of #738 plus the three feature commits. If this is FF-pushed in its current state via merge-train, #738 ships in reverse.
The bot's FF check will catch this — merge-train.yml requires merge-base --is-ancestor github/main github/<branch>, which is false here. So in practice the bot will reject and unlabel. But: don't add ready-to-merge until after rebase.
Action:
git fetch github main
git rebase github/main
# run unit suite again (expect AC4 test in test_search_tool_hook.py to need adjustment if rebasing on top of #740 once that lands, but that's not in main yet)
git push --force-with-lease
Minor observations (non-blocking)
-
_read_toml_flaghas atry: import tomllib / except ImportError: return Nonefallback, butpyproject.tomlpinsrequires-python >= 3.11and the docstring comment notes "aelfrice ships >=3.11". The ImportError path is unreachable. Not a real bug — fail-soft principle covers it — just dead defensiveness. Up to you whether to drop it. -
Token count is
text.split()for the 80-token threshold. Naive whitespace-split; for a heuristic that's fine. Just noting for the record that the threshold is "roughly 80 whitespace-separated chunks" not "80 lexemes". -
Interaction with PreToolUse search-tool hook. The gate fires inside
retrieve(), which is called fromhook_search_tool._do_search. PreToolUse queries are 1-5 extracted tokens (snake_case identifiers from the original Grep pattern), so they hit the structural-marker heuristic and gate verdict isnarrow— which is correct. Just calling out that the gate's scope includes the PreToolUse fire as a happy side-effect; the bench fixture from #741 should sanity-check this. -
Interaction with #740 (in-flight as PR #744). Both PRs touch
hook.pyaround the_write_hook_audit_recordcallsite. After this PR lands and PR #744 rebases, the conflict resolution is mechanical (both adding kwargs afterlatency_ms=, no semantic overlap). Flagging so whoever lands second knows the rebase is trivial.
Verdict
Substance: approve. Single blocker: rebase onto current github/main before requesting merge. Otherwise looks ready.
|
[release:review:pascal:2026-05-13T20:36:30Z] |
|
[claim:review:pascal:2026-05-13T20:39:10Z] |
|
Re-checking on rebase: base is still |
|
[release:review:pascal:2026-05-13T20:40:17Z] |
Deterministic stdlib-only resolver + heuristics for the BFS / HRR- structural expansion gate. should_run_expansion(query) returns an ExpansionDecision with run_bfs, run_hrr_structural, and a reason tag for telemetry. Resolver precedence: AELFRICE_FORCE_EXPANSION > AELFRICE_NO_EXPANSION_GATE > [retrieval] expansion_gate_enabled TOML > heuristics. Heuristics: prompt length > 80 tokens, absence of structural markers (#NNN, src/..., snake_case, camelCase, edge-type names), or question-form prefix (what/why/how/which/who/tell me/explain) trips broad. Honors PHILOSOPHY (#605): no embeddings, no model calls. This commit ships the resolver only; retrieve() wiring + tests land in the next two commits.
retrieve() and retrieve_with_tiers() now consult should_run_expansion() after is_bfs_enabled(); a broad-prompt verdict short-circuits the BFS lane even when bfs_enabled=True. L0 / L1 / L2.5-entity always run. LaneTelemetry gains two additive fields (expansion_gate_reason and expansion_gate_skipped_bfs) so callers and the UserPromptSubmit hook can surface the decision. The UPS audit-record callsite reads the post-retrieve() telemetry snapshot and threads the two fields into hook_audit.jsonl — visible from `aelf tail`. Defaults preserve byte-identical behaviour for callers who set bfs_enabled=False (gate has nothing to suppress) and for explicit narrow queries (gate returns run_bfs=True).
27 tests covering: - Heuristic gates (length, structural-marker presence, question-form, empty-query passthrough) - Resolver precedence (env-force > env-no-gate > toml > heuristics) - TOML fail-soft on malformed input - Integration with retrieve() (broad short-circuits BFS, narrow keeps it, bfs_enabled=False makes gate a no-op) - LaneTelemetry contract (additive fields default to safe values) Existing BFS-internal suite (test_bfs_multihop.py) now sets AELFRICE_FORCE_EXPANSION=1 in its isolated-env fixture so the AC1-AC11 tests continue to exercise BFS behaviour rather than gate behaviour. The gate's own assertions live in test_expansion_gate.py.
8885486 to
bf288f5
Compare
|
Rebased onto current
New tip: |
|
[claim:review:pascal:2026-05-13T21:07:51Z] |
ReviewVerdict: code-LGTM with two non-blocking flags. CI green (pytest 3.12/3.13, CodeQL, staging-gate full suite). Discretion clean on the diff. What works
Flags1. Branch is behind The merge-train bot will refuse 2. Bench deferred but
PR body acknowledges the bench is deferred ("layered on top of #739's BFS-flip bench re-run"). If this merges with
Operator call. I'd lean (a) since #741 explicitly defers to #739 for the bench, but (b) keeps the merge-train moving. Nits (non-blocking)
RecommendationResolve flag #2 (operator decision on the bench / Closes-link). Then rebase on |
|
[release:review:pascal:2026-05-13T21:10:32Z] |
|
[claim:review:curie:2026-05-13T21:13:18Z] |
robotrocketscience
left a comment
There was a problem hiding this comment.
Review
LGTM. Read all three commits + 27 expansion-gate tests + retrieval.py wiring + hook.py audit thread; design matches #741 spec and is well-bounded.
Approved aspects:
expansion_gate.py— deterministic stdlib-only heuristics (#605 PHILOSOPHY honored). Length / marker-absence / question-form gates. Edge-type regex compiled once frommodels.EDGE_TYPES. Resolver precedence matches issue spec: env-force > env-no-gate > TOML > heuristics. Each branch returns anExpansionDecisionwithreasonpopulated for telemetry.- TOML walk is parent-bounded by
seenset +cur.parent == curroot-stop. Fail-soft onTOMLDecodeError,OSError,ValueError. should_run_expansion(query, *, start=None)—startkwarg lets callers (and tests) override the cwd-walk root. Empty/whitespace query short-circuits torun_bfs=Truewithreason="empty-query"so upstream L0-only retrieve behavior is unaffected.retrieval.pywiring —gate_skipped_bfs = bfs_on and not gate_decision.run_bfsis computed against the pre-gatebfs_on, thenbfs_on = bfs_on and gate_decision.run_bfsapplies it. Correct semantic: the flag tracks whether the gate forced a skip the user would otherwise have seen, not whether BFS would have run for other reasons.LaneTelemetryextension —expansion_gate_reason: str = ""andexpansion_gate_skipped_bfs: bool = Falsedefaults preserve the pre-#741 constructor contract; existing callers don't need to know about the new fields.- BFS test isolation —
tests/test_bfs_multihop.pysetsAELFRICE_FORCE_EXPANSION=1in the isolated-env fixture so the gate doesn't shadow BFS-internal assertions. Right call: those tests assert BFS behavior, not gate behavior. - HRR-structural lane stays on regardless of gate verdict in v1; field is reserved for forward-compat. Matches issue spec.
Verification:
- CI: pytest (3.12) ✅, pytest (3.13) ✅, CodeQL python ✅, staging-gate (secrets-scan, pattern-scan, history-scan, commit-msg-prefix, pr-title-prefix, pr-body-issue-link, release-docs-check) ✅, deptry + vulture ✅, typos ✅
mergeable: MERGEABLE, all 3 commits SSH-signed (visible in git log)- Discretion grep on diff: clean
Minor observations (non-blocking):
- N-gram entropy gate from issue §Design is not implemented; issue marked it optional/behind-flag so this matches scope.
- Bench (broad/narrow p95 ≥30% improvement) deferred per PR body — gated on #739 BFS-flip ratification, which makes the gate load-bearing. Code path is observable via
aelf tailin the meantime.
Adding ready-to-merge.
|
[release:review:curie:2026-05-13T21:14:51Z] |
|
merge-train: merged bf288f5 → |
Summary
Adds the adaptive expansion-gate from #741: a cheap deterministic
prompt-shape check that short-circuits the BFS multi-hop lane on
broad natural-language prompts while leaving L0 / L1 / L2.5-entity
always on. Sets up the precondition #739's BFS default-flip needs —
once the BFS flip ratifies, broad prompts won't blow the latency
band because the gate suppresses BFS on them.
Closes #741.
What landed
src/aelfrice/expansion_gate.py— new module withshould_run_expansion(query)→ExpansionDecision(run_bfs, run_hrr_structural, reason).src/aelfrice/retrieval.py—retrieve()andretrieve_with_tiers()consult the gate afteris_bfs_enabled();bfs_on = bfs_on and gate_decision.run_bfs.Two additive fields on
LaneTelemetry(
expansion_gate_reason,expansion_gate_skipped_bfs) fordownstream surfaces.
src/aelfrice/hook.py—_write_hook_audit_recordaccepts thetwo new fields; UPS callsite reads
last_lane_telemetry()post-retrieve and threads them into
hook_audit.jsonl(visible viaaelf tail).tests/test_expansion_gate.py— 27 tests covering heuristics,resolver precedence, TOML fail-soft, retrieve() integration,
LaneTelemetry contract.
tests/test_bfs_multihop.py— BFS-internal tests setAELFRICE_FORCE_EXPANSION=1in theirisolated_envfixture sothe gate doesn't interfere with assertions about BFS behaviour.
Heuristics
Deterministic, stdlib-only (honors PHILOSOPHY #605):
BROAD_PROMPT_TOKEN_THRESHOLD(default 80tokens) → broad.
#NNN, nosrc/..., notests/..., no snake_case / camelCase identifier, no edge-typename (
SUPPORTS,CONTRADICTS, …) → broad.what,why,how,which,who,tell me,explain→ broad.Any "broad" signal →
run_bfs=False. Conservative by design;escape hatches via env override or
aelf reason.Resolver precedence
AELFRICE_FORCE_EXPANSION=1>AELFRICE_NO_EXPANSION_GATE=1>[retrieval] expansion_gate_enabledin.aelfrice.toml(defaultTrue) > heuristics.
Malformed TOML fails soft.
Bench (deferred)
Per the issue body, the load-bearing bench (labelled-corpus
broad/narrow split with p95 improvement target ≥30%) is layered on
top of #739's BFS-flip bench re-run. This PR ships the code +
unit/integration tests; the bench is wired separately when #739's
gate ratification runs. The code path is observable via
aelf doctor(LaneTelemetry) andaelf tail(hook_audit.jsonl)in the meantime.
Test plan
uv run pytest tests/test_expansion_gate.py— 27/27 passuv run pytest tests/test_bfs_multihop.py tests/test_retrieve_v2.py— 39/39 pass (BFS-internal contracts unchanged with
AELFRICE_FORCE_EXPANSION=1fixture)NameError) — 3775 pass / 57 skipped / 75 xfail
ratification per issue body)
Risk
benefited from expansion. Mitigated by env escape hatch +
telemetry surface (
aelf tailshows what got gated and why).LaneTelemetrygains two additive fields withsafe defaults; pre-Adaptive expansion-gate: skip BFS/HRR-expensive lanes on broad prompts #741 callers keep working.
AELFRICE_FORCE_EXPANSION=1in the affected test module'sisolated-env fixture so the gate doesn't shadow what those tests
are actually asserting.