feat(recall): eval-validated strict floor + per-call min_score on MCP/hook - #58
Open
123456-farewell wants to merge 8 commits into
Open
feat(recall): eval-validated strict floor + per-call min_score on MCP/hook#58123456-farewell wants to merge 8 commits into
123456-farewell wants to merge 8 commits into
Conversation
Replace hardcoded strict_recall: True with a configurable min_score parameter on both recall surfaces: - MCP server (search_memory): new optional min_score parameter lets the agent override the server default per-call - Recall hook: reads HEBB_RECALL_MIN_SCORE env var; falls back to strict_recall when unset or unparseable Server-side priority logic (search.py) is unchanged — it already prefers explicit min_score over strict_recall -> recall_min_score. Refs: afx-team#31 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Move the hardcoded rerank-floor ratio (0.625) from a module constant in searcher.py to a Settings field, making it tunable via the web console without a code change. - Settings: new rerank_floor_ratio field (float, default 0.625, [0,1]) - MemorySearcher.search(): accept rerank_floor_ratio kwarg instead of reading the module constant; default 0.625 preserves existing behaviour - search.py router: read settings.rerank_floor_ratio and pass to searcher - api.py HebbClient: read self.settings.rerank_floor_ratio - Activate page: add rerank_floor_ratio to the Rerank config group - i18n: EN/ZH hints explaining the ratio - Tests: 2 new tests for custom (tight/lenient) ratio behaviour The field is not in restart_fields — changes take effect immediately on the next request, mirroring recall_min_score. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…_ratio Add --probe-floor / --sweep-floor modes to retrieval_lab that measure how recall_min_score and rerank_floor_ratio affect retrieval quality and query emptiness on labelled data (LoCoMo / LongMemEval / MemBench). _sweep_floor builds each unit's store once, runs one unfiltered search per question, then for each of the 21 (min_score, ratio) configs issues a real searcher.search() with that config's floor so the searcher applies the floor on the correct scale (rerank-sigmoid for the reranked pool, composite for the tail). Reports Recall@1/3/5/10, ΔR@10 vs unfiltered, emptied count and per-category breakdown; writes .txt + .json under eval/reports/floor_probe/. Note: a prior attempt to reuse the unfiltered rerank result and floor-filter in Python produced identical scores across all 21 configs (floor scale mismatched the rerank sigmoid), so the 21x search cost is kept deliberately. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…overflow sentence-transformers 5.x's CrossEncoder.predict no longer truncates (query, content) pairs to the tokenizer's model_max_length — it only honors an explicit max_length set on the instance. Without it, a long pair exceeds the backbone's position-embedding cap (XLM-Roberta = 514) and the forward pass crashes with ``gather: index 514 is out of bounds``. Set max_length=512 at construction (2 slots of headroom below the 514 cap, the standard usable length for bge-reranker-base). This is a production-path crash fix: without it, rerank-on recall crashes on any long-document haystack (e.g. LongMemEval's ~120K-token sessions). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ank pass Two related changes to --sweep-floor, both motivated by the rerank-on floor ineffectiveness surfaced by the longmemeval rerank-on probe: 1. Narrow the sweep grid. The 2-D grid (min_score 0.4-0.9 × ratio 0.3-0.8 = 21 configs) could not separate ratio values: under rerank-on the BGE sigmoid compresses the reranked pool to 0.97-1.0, so any ratio×min_score floor below ~0.97 is a no-op and all 21 strict results come out identical (see longmemeval_20260730-013838). Under rerank-off the floor runs on a single composite scale where ratio is irrelevant. Narrow to a 1-D sweep: min_score 0.3-0.9 (step 0.1, 7 points), ratio fixed at the shipped 0.625. --floor-quick aligned. 2. Reuse the unfiltered rerank pass for the strict leg under rerank. The cross-encoder scores depend only on (query, content), not on min_score/ratio, so all 21 configs share one set of rerank scores; re-running searcher.search() per config is pure overhead. Reuse the single unfiltered search (top_k bumped to reranker.top_n so the pool is fully visible), apply the per-scale floor in Python (rerank_floor = min_score*ratio for the pool, min_score for the tail), and re-sort by score — byte-identical to the 21x-search outcome (verified: BGE sigmoid 0.979-1.0 sits above every min_score*ratio floor, so no config filters the pool). ~21x faster. Without rerank the min_score axis genuinely moves results, so the real per-config search is kept. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Commit the run reports produced by eval/retrieval_lab.py --sweep-floor, covering both production recall surfaces' strict floor on labelled retrieval datasets. Acceptance criterion 1 asks for recall@k under the strict floor and the empty-set fraction, committed to the run report. Reports: - locomo_20260728-111918: rerank-off, min_score 0.3-0.9, ratio 0.625. The shipped default 0.8 empties 26.5% of queries (R@10 0.913 -> 0.657); 0.4-0.5 keep R@10 >= 0.91 with <= 0.1% emptied. - locomo_20260729-032636: rerank-off, min_score 0.45-0.65 (zoom on the peak), ratio 0.625. Confirms 0.5 as the composite-scale sweet spot (R@10 0.917, emptied 0.1%) before the 0.65 cliff (emptied 1.8%). - longmemeval_20260729-055836: rerank-off, min_score 0.3-0.9, ratio 0.625. Long-document haystack; 0.8 empties 55.4% (R@10 0.984 -> 0.426), 0.3-0.4 keep R@10 >= 0.96 with <= 0.4% emptied. - longmemeval_20260730-013838: rerank-ON, min_score 0.4-0.9 × ratio 0.3-0.8 (21-config grid). All 21 strict results are identical (R@10 0.942, emptied 0) — the BGE sigmoid (0.97-1.0) sits above every min_score*ratio floor, so the floor is a no-op under rerank-on. This is the witness that the shipped default cannot be eval-validated on the rerank-on path today; rerank_floor_ratio tuning on the real sigmoid scale is left to a follow-up. Note: the rerank-off reports run the floor on the composite scale, which is NOT the scale the production rerank-on path uses for the reranked pool (sigmoid). They validate the floor mechanism and the composite-scale behaviour, and bound the rerank-off case; they do not retune the production rerank-on default (kept at 0.8 pending the follow-up). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughThe search pipeline now supports configurable rerank floor ratios, integrations can provide minimum-score controls, reranker length is explicit, and evaluation commands generate floor-probe metrics and reports for locomo and longmemeval. ChangesRerank floor configuration and evaluation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SearchEndpoint
participant MemorySearcher
participant LocalReranker
Client->>SearchEndpoint: Submit search request
SearchEndpoint->>MemorySearcher: Pass configured rerank_floor_ratio
MemorySearcher->>LocalReranker: Apply rerank score floor
MemorySearcher-->>SearchEndpoint: Return filtered results
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Author
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hebb/mcp/server.py (1)
94-108: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winComplete the documentation for the touched public Python APIs.
src/hebb/mcp/server.py#L94-L108: addReturnsfor the rendered result string andRaisesfor HTTP failures.src/hebb/retrieval/searcher.py#L85-L85: add a docstring withArgs,Returns, andRaises, includingrerank_floor_ratio.src/hebb/server/routers/search.py#L43-L43: expand the route docstring withArgs,Returns, andRaises.As per coding guidelines, all public APIs in Python must have docstrings with Args, Returns, and Raises sections.
🤖 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 `@src/hebb/mcp/server.py` around lines 94 - 108, Add complete docstrings for the public APIs at src/hebb/mcp/server.py lines 94-108, src/hebb/retrieval/searcher.py line 85, and src/hebb/server/routers/search.py line 43: document arguments, return values, and HTTP-related exceptions. Include rerank_floor_ratio in the searcher API’s Args section, describe the rendered result string and route response in Returns, and document applicable HTTP failures in Raises; update only the relevant public function and route docstrings.Source: Coding guidelines
🧹 Nitpick comments (1)
eval/retrieval_lab.py (1)
750-767: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the retrieval run configuration in the report.
The committed LongMemEval reports have materially different unfiltered recall, but JSON records only floor values. Include rerank/vector mode, model/top-N, channel flags, and limits so results are reproducible.
Suggested metadata
json_data = { "dataset": name, + "run_config": { + "vector": args.vector, + "rerank": args.rerank, + "rerank_model": args.rerank_model, + "rerank_top_n": args.rerank_top_n, + "no_keyword": args.no_keyword, + "no_blend": args.no_blend, + "limit": args.limit, + "qlimit": args.qlimit, + }, "total_q": results[0].total_q if results else 0,🤖 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 `@eval/retrieval_lab.py` around lines 750 - 767, Update the json_data report construction to include the retrieval run configuration alongside the existing metrics, using the already-resolved run/CLI configuration values. Record rerank and vector modes, model and top-N settings, channel enablement flags, and applicable retrieval limits so reports are reproducible; preserve the existing per-config recall and empty-result fields.
🤖 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 `@eval/retrieval_lab.py`:
- Around line 717-719: Update the optimized strict-path response construction in
_ranked_keys to preserve resp_uf.related while filtering only results; then
regenerate eval/reports/floor_probe/longmemeval_20260730-013838.json (lines
16-424) and eval/reports/floor_probe/longmemeval_20260730-013838.txt (lines
8-32) so their metrics and recommendation reflect the corrected response.
In `@src/hebb/integrations/claude_code/recall.py`:
- Around line 137-150: Update the HEBB_RECALL_MIN_SCORE handling in the recall
request builder to accept only finite float values within the inclusive range
[0.0, 1.0]. Treat non-numeric, NaN, infinite, and out-of-range values like
invalid input by logging the existing debug message and setting strict_recall
instead of adding min_score.
In `@src/hebb/retrieval/searcher.py`:
- Line 85: Validate rerank_floor_ratio at the start of search before calculating
the reranked floor, rejecting any value outside the inclusive range 0.0 through
1.0. Preserve the existing search behavior for valid ratios and apply the same
validation to the corresponding search call path.
---
Outside diff comments:
In `@src/hebb/mcp/server.py`:
- Around line 94-108: Add complete docstrings for the public APIs at
src/hebb/mcp/server.py lines 94-108, src/hebb/retrieval/searcher.py line 85, and
src/hebb/server/routers/search.py line 43: document arguments, return values,
and HTTP-related exceptions. Include rerank_floor_ratio in the searcher API’s
Args section, describe the rendered result string and route response in Returns,
and document applicable HTTP failures in Raises; update only the relevant public
function and route docstrings.
---
Nitpick comments:
In `@eval/retrieval_lab.py`:
- Around line 750-767: Update the json_data report construction to include the
retrieval run configuration alongside the existing metrics, using the
already-resolved run/CLI configuration values. Record rerank and vector modes,
model and top-N settings, channel enablement flags, and applicable retrieval
limits so reports are reproducible; preserve the existing per-config recall and
empty-result fields.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 13b7fb6a-6eef-4aa5-bbb2-58dc5f2c646a
📒 Files selected for processing (19)
eval/reports/floor_probe/locomo_20260728-111918.jsoneval/reports/floor_probe/locomo_20260728-111918.txteval/reports/floor_probe/locomo_20260729-032636.jsoneval/reports/floor_probe/locomo_20260729-032636.txteval/reports/floor_probe/longmemeval_20260729-055836.jsoneval/reports/floor_probe/longmemeval_20260729-055836.txteval/reports/floor_probe/longmemeval_20260730-013838.jsoneval/reports/floor_probe/longmemeval_20260730-013838.txteval/retrieval_lab.pysrc/hebb/api.pysrc/hebb/config/settings.pysrc/hebb/integrations/claude_code/recall.pysrc/hebb/mcp/server.pysrc/hebb/retrieval/rerank/local.pysrc/hebb/retrieval/searcher.pysrc/hebb/server/routers/search.pysrc/hebb/static/js/components/activate.jssrc/hebb/static/js/i18n.jstests/unit/test_audit_retrieval.py
Comment on lines
+717
to
+719
| keys_st = _ranked_keys( | ||
| type(resp_uf)(results=top_results, related=[]), metric | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve related records in the optimized strict path.
_ranked_keys() scores both results and related, while the actual strict search filters only results. Reconstructing with related=[] creates an artificial recall loss and invalidates this sweep’s strict metrics.
eval/retrieval_lab.py#L717-L719: retainresp_uf.relatedwhen constructing the filtered response.eval/reports/floor_probe/longmemeval_20260730-013838.json#L16-L424: regenerate after fixing the response reconstruction.eval/reports/floor_probe/longmemeval_20260730-013838.txt#L8-L32: regenerate the metrics and recommendation.
Proposed fix
keys_st = _ranked_keys(
- type(resp_uf)(results=top_results, related=[]), metric
+ type(resp_uf)(results=top_results, related=resp_uf.related), metric
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| keys_st = _ranked_keys( | |
| type(resp_uf)(results=top_results, related=[]), metric | |
| ) | |
| keys_st = _ranked_keys( | |
| type(resp_uf)(results=top_results, related=resp_uf.related), metric | |
| ) |
📍 Affects 3 files
eval/retrieval_lab.py#L717-L719(this comment)eval/reports/floor_probe/longmemeval_20260730-013838.json#L16-L424eval/reports/floor_probe/longmemeval_20260730-013838.txt#L8-L32
🤖 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 `@eval/retrieval_lab.py` around lines 717 - 719, Update the optimized
strict-path response construction in _ranked_keys to preserve resp_uf.related
while filtering only results; then regenerate
eval/reports/floor_probe/longmemeval_20260730-013838.json (lines 16-424) and
eval/reports/floor_probe/longmemeval_20260730-013838.txt (lines 8-32) so their
metrics and recommendation reflect the corrected response.
Comment on lines
+137
to
+150
| body: dict[str, object] = {"query": query, "top_k": top_k} | ||
| env_min_score = os.environ.get("HEBB_RECALL_MIN_SCORE") | ||
| if env_min_score is not None: | ||
| try: | ||
| body["min_score"] = float(env_min_score) | ||
| except ValueError: | ||
| logger.debug("HEBB_RECALL_MIN_SCORE=%r is not a valid float; falling back to strict_recall", env_min_score) | ||
| body["strict_recall"] = True | ||
| else: | ||
| body["strict_recall"] = True | ||
|
|
||
| resp = client.post( | ||
| "/api/v1/search", | ||
| json={"query": query, "top_k": top_k, "strict_recall": True}, | ||
| json=body, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fall back when the environment score is outside the API range.
float() accepts -1, 1.1, and nan; each replaces strict_recall with an invalid min_score, causing the server to reject the request instead of using its configured floor. Require a finite value in [0.0, 1.0].
Proposed fix
env_min_score = os.environ.get("HEBB_RECALL_MIN_SCORE")
if env_min_score is not None:
try:
- body["min_score"] = float(env_min_score)
+ min_score = float(env_min_score)
+ if not 0.0 <= min_score <= 1.0:
+ raise ValueError
+ body["min_score"] = min_score
except ValueError:
- logger.debug("HEBB_RECALL_MIN_SCORE=%r is not a valid float; falling back to strict_recall", env_min_score)
+ logger.debug("HEBB_RECALL_MIN_SCORE=%r is not a valid score; falling back to strict_recall", env_min_score)
body["strict_recall"] = True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| body: dict[str, object] = {"query": query, "top_k": top_k} | |
| env_min_score = os.environ.get("HEBB_RECALL_MIN_SCORE") | |
| if env_min_score is not None: | |
| try: | |
| body["min_score"] = float(env_min_score) | |
| except ValueError: | |
| logger.debug("HEBB_RECALL_MIN_SCORE=%r is not a valid float; falling back to strict_recall", env_min_score) | |
| body["strict_recall"] = True | |
| else: | |
| body["strict_recall"] = True | |
| resp = client.post( | |
| "/api/v1/search", | |
| json={"query": query, "top_k": top_k, "strict_recall": True}, | |
| json=body, | |
| body: dict[str, object] = {"query": query, "top_k": top_k} | |
| env_min_score = os.environ.get("HEBB_RECALL_MIN_SCORE") | |
| if env_min_score is not None: | |
| try: | |
| min_score = float(env_min_score) | |
| if not 0.0 <= min_score <= 1.0: | |
| raise ValueError | |
| body["min_score"] = min_score | |
| except ValueError: | |
| logger.debug("HEBB_RECALL_MIN_SCORE=%r is not a valid score; falling back to strict_recall", env_min_score) | |
| body["strict_recall"] = True | |
| else: | |
| body["strict_recall"] = True | |
| resp = client.post( | |
| "/api/v1/search", | |
| json=body, |
🤖 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 `@src/hebb/integrations/claude_code/recall.py` around lines 137 - 150, Update
the HEBB_RECALL_MIN_SCORE handling in the recall request builder to accept only
finite float values within the inclusive range [0.0, 1.0]. Treat non-numeric,
NaN, infinite, and out-of-range values like invalid input by logging the
existing debug message and setting strict_recall instead of adding min_score.
| self.keyword_blend_enabled = keyword_blend_enabled | ||
|
|
||
| async def search(self, query: MemoryQuery) -> SearchResponse: | ||
| async def search(self, query: MemoryQuery, *, rerank_floor_ratio: float = 0.625) -> SearchResponse: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject invalid per-call floor ratios.
Direct callers bypass Settings validation. A negative ratio disables the reranked strict floor, while a value above 1.0 over-filters. Validate 0.0 <= rerank_floor_ratio <= 1.0 before calculating the floor.
Proposed fix
async def search(self, query: MemoryQuery, *, rerank_floor_ratio: float = 0.625) -> SearchResponse:
+ if not 0.0 <= rerank_floor_ratio <= 1.0:
+ raise ValueError("rerank_floor_ratio must be between 0.0 and 1.0")
+
# Sanitize LLM-generated queries (XML tags, tool artifacts, etc.)Also applies to: 279-279
🤖 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 `@src/hebb/retrieval/searcher.py` at line 85, Validate rerank_floor_ratio at
the start of search before calculating the reranked floor, rejecting any value
outside the inclusive range 0.0 through 1.0. Preserve the existing search
behavior for valid ratios and apply the same validation to the corresponding
search call path.
Cover all public defs missing docstrings in the PR diff: - retrieval_lab.py: NullEmbedder + Doc/Q dataclasses + run_dataset/main_async/main - local.py reranker: top_n property + score() - searcher.py: MemorySearcher.search() (incl. rerank_floor_ratio arg) - test_audit_retrieval.py: FakeEmbedder/FakeStore/FakeReranker mock methods Public-API docstrings follow the repo's Args/Returns/Raises convention; test doubles use one-line intent docstrings. No logic changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
lint: wrap overlong logger.debug in recall.py so `ruff format --check` passes (the env-min-score fallback line exceeded the line limit). e2e: pin mcp SDK to <2. mcp v2.0.0 (2026-07) removed `mcp.server.fastmcp.FastMCP`, so CI's fresh install hit ModuleNotFoundError on import and the MCP stdio handshake failed with "Connection closed". Local .venv was pinned to 1.28.1, which is why it only surfaced in CI. Pin to v1.x until the server migrates to `mcp.server.MCPServer`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
eval/retrieval_lab.py (2)
831-835: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject simultaneous floor modes.
When both
--probe-floorand--sweep-floorare set,main_asyncruns--probe-floorand returns before the sweep. The user receives no error, and the sweep request is silently ignored. Make the mode flags mutually exclusive or report a clear CLI error.Also applies to: 905-909
🤖 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 `@eval/retrieval_lab.py` around lines 831 - 835, Update main_async’s floor-mode argument handling to reject simultaneous args.probe_floor and args.sweep_floor settings with a clear CLI error before either _probe_floor or _sweep_floor runs. Preserve the existing short-circuit behavior when exactly one mode is selected.
592-635: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not select the weakest floor by construction.
If the floor grid is monotonic, sorting by empty-query fraction first and recall delta second favors the least strict point in the grid. That recommendation can apply little or no filtering and does not identify a useful strict floor. Select a Pareto-optimal point or maximize strictness subject to explicit recall and empty-query limits.
🤖 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 `@eval/retrieval_lab.py` around lines 592 - 635, Update the recommendation logic in _print_floor_probe instead of minimizing emptied fraction first, which inherently selects the least strict floor. Choose a Pareto-optimal result or the strictest result satisfying explicit recall-delta and emptied-query limits, using the existing floor parameters to measure strictness, and ensure the displayed recommendation matches that selection.
🧹 Nitpick comments (1)
eval/retrieval_lab.py (1)
910-913: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winKeep evaluation defaults tied to production settings.
The CLI hardcodes
0.8and0.625. If the production floor settings change, the probe command and its reports will silently use stale defaults. Resolve these defaults from shared settings or constants, while keeping explicit CLI values as overrides.🤖 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 `@eval/retrieval_lab.py` around lines 910 - 913, Update the argument definitions for --floor-min-score and --floor-rerank-ratio to use the shared production floor settings or constants as their defaults instead of hardcoded 0.8 and 0.625. Preserve explicit CLI arguments as overrides and keep the help text aligned with the resolved defaults.
🤖 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 `@eval/retrieval_lab.py`:
- Around line 53-90: Complete the public API docstrings for NullEmbedder, Doc,
Q, run_dataset, main_async, and main by adding Args, Returns, and Raises
sections. Use explicit empty sections where a category has no applicable values,
and document run_dataset’s ValueError for unknown datasets; preserve the
existing behavior and descriptions.
- Line 79: Complete strict annotations across the public symbols Doc.metadata,
run_dataset, and main_async: parameterize metadata with its actual key/value
types, annotate args, embedder, kw_cfg, and reranker using the repository’s
concrete types, and replace bare dict returns with a dedicated TypedDict result
alias. Ensure main_async.args is also explicitly typed and preserve existing
behavior.
- Around line 344-346: Update the run_dataset docstring’s CLI option reference
from “--by_cat” to the accepted “--by-cat” spelling, matching the argument
registered by main. Do not change the surrounding option documentation.
In `@tests/unit/test_audit_retrieval.py`:
- Around line 37-49: Complete the public API docstrings for FakeEmbedder,
FakeStore, and FakeReranker, including the required Args, Returns, and Raises
sections for each changed public method and property, while preserving their
existing type hints. Correct FakeEmbedder.embed’s description to document only
its fixed 3-dimensional vector contract, not a vector-search hit; keep
FakeStore.search_by_vector’s no-hit behavior and _searcher_with_vector_hit’s
patched _vector_search behavior accurately described.
---
Outside diff comments:
In `@eval/retrieval_lab.py`:
- Around line 831-835: Update main_async’s floor-mode argument handling to
reject simultaneous args.probe_floor and args.sweep_floor settings with a clear
CLI error before either _probe_floor or _sweep_floor runs. Preserve the existing
short-circuit behavior when exactly one mode is selected.
- Around line 592-635: Update the recommendation logic in _print_floor_probe
instead of minimizing emptied fraction first, which inherently selects the least
strict floor. Choose a Pareto-optimal result or the strictest result satisfying
explicit recall-delta and emptied-query limits, using the existing floor
parameters to measure strictness, and ensure the displayed recommendation
matches that selection.
---
Nitpick comments:
In `@eval/retrieval_lab.py`:
- Around line 910-913: Update the argument definitions for --floor-min-score and
--floor-rerank-ratio to use the shared production floor settings or constants as
their defaults instead of hardcoded 0.8 and 0.625. Preserve explicit CLI
arguments as overrides and keep the help text aligned with the resolved
defaults.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aeac7741-0922-4903-9283-05bd503a385a
📒 Files selected for processing (6)
eval/retrieval_lab.pypyproject.tomlsrc/hebb/integrations/claude_code/recall.pysrc/hebb/retrieval/rerank/local.pysrc/hebb/retrieval/searcher.pytests/unit/test_audit_retrieval.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/hebb/integrations/claude_code/recall.py
- src/hebb/retrieval/searcher.py
- src/hebb/retrieval/rerank/local.py
Comment on lines
+53
to
+90
| """No-op embedder for keyword-only runs. | ||
|
|
||
| Implements the :class:`~hebb.embedding.base.EmbeddingProvider` interface | ||
| but returns empty vectors, so the vector retrieval path yields nothing | ||
| and the lab measures the keyword channel in isolation. | ||
| """ | ||
|
|
||
| @property | ||
| def dimension(self) -> int: | ||
| """Embedding dimensionality reported to callers (unused on the empty path).""" | ||
| return 384 | ||
|
|
||
| async def embed(self, text: str) -> list[float]: | ||
| """Return an empty vector — the vector channel is intentionally disabled.""" | ||
| return [] | ||
|
|
||
| async def embed_batch(self, texts: list[str]) -> list[list[float]]: | ||
| """Return one empty vector per input — batch form of :meth:`embed`.""" | ||
| return [[] for _ in texts] | ||
|
|
||
|
|
||
| @dataclass | ||
| class Doc: | ||
| """A single corpus document: content plus the metadata stored with it.""" | ||
|
|
||
| content: str | ||
| metadata: dict | ||
|
|
||
|
|
||
| @dataclass | ||
| class Q: | ||
| """One retrieval question: its text, id, and the gold relevance keys. | ||
|
|
||
| ``relevant`` holds the gold keys the metric compares recall hits against | ||
| (session ids for session-level datasets, or step ids as strings for | ||
| turn-level datasets). | ||
| """ | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Complete the required public API docstrings.
The new public docstrings do not consistently include Args, Returns, and Raises sections. Add all three sections to NullEmbedder, Doc, Q, run_dataset, main_async, and main. Document the ValueError path for an unknown dataset in run_dataset; use an explicit empty section when no values apply.
As per coding guidelines: **/*.py: “Include docstring with Args, Returns, and Raises sections for all public APIs.”
Also applies to: 337-357, 815-823, 874-874
🤖 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 `@eval/retrieval_lab.py` around lines 53 - 90, Complete the public API
docstrings for NullEmbedder, Doc, Q, run_dataset, main_async, and main by adding
Args, Returns, and Raises sections. Use explicit empty sections where a category
has no applicable values, and document run_dataset’s ValueError for unknown
datasets; preserve the existing behavior and descriptions.
Source: Coding guidelines
| """A single corpus document: content plus the metadata stored with it.""" | ||
|
|
||
| content: str | ||
| metadata: dict |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Complete strict type annotations in the changed public surface.
Doc.metadata: dict is an unparameterized generic. run_dataset leaves args, embedder, kw_cfg, and reranker untyped and returns bare dict; main_async leaves args untyped. Add concrete types and a typed result alias such as a TypedDict.
Proposed local fix
- metadata: dict
+ metadata: dict[str, object]As per coding guidelines: **/*.py: “Add type hints on all public functions (mypy strict is enabled in pyproject.toml).”
Also applies to: 337-357, 815-823
🤖 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 `@eval/retrieval_lab.py` at line 79, Complete strict annotations across the
public symbols Doc.metadata, run_dataset, and main_async: parameterize metadata
with its actual key/value types, annotate args, embedder, kw_cfg, and reranker
using the repository’s concrete types, and replace bare dict returns with a
dedicated TypedDict result alias. Ensure main_async.args is also explicitly
typed and preserve existing behavior.
Source: Coding guidelines
Comment on lines
+344
to
+346
| name: Dataset key — ``"locomo"``, ``"longmemeval"``, or ``"membench"``. | ||
| args: Parsed CLI namespace (drives channel toggles, ``--vector``, | ||
| ``--deep`` k grid, ``--by_cat``, floor-probe flags, etc.). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the accepted CLI option spelling.
The run_dataset docstring documents --by_cat, but main registers --by-cat at Line [882]. Replace the underscore form. Otherwise, copied commands fail argument parsing.
Proposed fix
- --by_cat
+ --by-cat📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| name: Dataset key — ``"locomo"``, ``"longmemeval"``, or ``"membench"``. | |
| args: Parsed CLI namespace (drives channel toggles, ``--vector``, | |
| ``--deep`` k grid, ``--by_cat``, floor-probe flags, etc.). | |
| name: Dataset key — ``"locomo"``, ``"longmemeval"``, or ``"membench"``. | |
| args: Parsed CLI namespace (drives channel toggles, ``--vector``, | |
| ``--deep`` k grid, ``--by-cat``, floor-probe flags, etc.). |
🤖 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 `@eval/retrieval_lab.py` around lines 344 - 346, Update the run_dataset
docstring’s CLI option reference from “--by_cat” to the accepted “--by-cat”
spelling, matching the argument registered by main. Do not change the
surrounding option documentation.
Comment on lines
+37
to
+49
| """Fixed dimensionality matching the constant embedding below.""" | ||
| return 3 | ||
|
|
||
| async def embed(self, text: str) -> list[float]: | ||
| """Return the constant 3-d vector so the vector path produces a hit.""" | ||
| return [0.1, 0.2, 0.3] | ||
|
|
||
| async def embed_batch(self, texts: list[str]) -> list[list[float]]: | ||
| """Return one constant vector per input — batch form of :meth:`embed`.""" | ||
| return [[0.1, 0.2, 0.3] for _ in texts] | ||
|
|
||
| async def aclose(self) -> None: # pragma: no cover - no resources | ||
| """No resources to release.""" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Complete and correct the public API docstrings.
The changed public method and property docstrings omit the required Args, Returns, and Raises sections. This affects FakeEmbedder, FakeStore, and FakeReranker.
Line 41 also says that FakeEmbedder.embed produces a vector hit. FakeStore.search_by_vector returns no hits at Line 75, and _searcher_with_vector_hit patches _vector_search at Lines 151-152. Document only the fixed embedding contract.
As per coding guidelines, Python public APIs must include type hints and Args, Returns, and Raises sections.
Proposed docstring shape
async def embed(self, text: str) -> list[float]:
- """Return the constant 3-d vector so the vector path produces a hit."""
+ """Return the fixed embedding for ``text``.
+
+ Args:
+ text: Text to embed.
+
+ Returns:
+ A fixed three-dimensional embedding.
+
+ Raises:
+ None.
+ """Also applies to: 66-66, 75-75, 84-100, 113-117
🤖 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/unit/test_audit_retrieval.py` around lines 37 - 49, Complete the public
API docstrings for FakeEmbedder, FakeStore, and FakeReranker, including the
required Args, Returns, and Raises sections for each changed public method and
property, while preserving their existing type hints. Correct
FakeEmbedder.embed’s description to document only its fixed 3-dimensional vector
contract, not a vector-search hit; keep FakeStore.search_by_vector’s no-hit
behavior and _searcher_with_vector_hit’s patched _vector_search behavior
accurately described.
Source: Coding guidelines
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.
Summary
Narrows the strict-recall floor work to what remains after PR #24: an eval probe that measures recall@k under the strict floor and the empty-set fraction, the per-call
min_scoreknob threaded through the two production surfaces, andrerank_floor_ratiopromoted to a Settings field. The shipped default floor (recall_min_score=0.8) is intentionally not retuned in this PR — see Known limitations for why.Closes #31
What's done
Acceptance criteria
1. Eval probe reports recall@k under the strict floor + empty-set fraction; results committed to the run report.
eval/retrieval_lab.py --sweep-floorruns each floor config through the realsearcher.search(), so the floor is applied on the correct per-scale (rerank-sigmoid for the pool, composite for the tail). Four reports committed undereval/reports/floor_probe/:locomo_20260728-111918(rerank-off, ms 0.3–0.9): shipped default 0.8 empties 26.5% of queries (R@10 0.913 → 0.657); 0.4–0.5 keep R@10 ≥ 0.91 with ≤ 0.1% emptied.locomo_20260729-032636(rerank-off, ms 0.45–0.65 zoom): confirms 0.5 as the composite-scale sweet spot before the 0.65 cliff (emptied 1.8%).longmemeval_20260729-055836(rerank-off, ms 0.3–0.9): long-doc haystack; 0.8 empties 55.4% (R@10 0.984 → 0.426); 0.3–0.4 keep R@10 ≥ 0.96 with ≤ 0.4% emptied.longmemeval_20260730-013838(rerank-ON, 21-config grid ms 0.4–0.9 × ratio 0.3–0.8): all 21 strict results identical (R@10 0.942, emptied 0) — the BGE sigmoid (0.97–1.0) sits above everymin_score × ratiofloor, so the floor is a no-op under rerank-on.2. Shipped default floor confirmed or retuned so it does not silently empty strict recall.
Default kept at 0.8 — not retuned. See Known limitations for the reason.
3.
_RERANK_FLOOR_RATIOexposed as a config setting alongsiderecall_min_score.Settings.rerank_floor_ratio(default 0.625, console-editable, no restart), wired throughsrc/hebb/server/routers/search.py→searcher.search(rerank_floor_ratio=...).4. MCP + recall hook can pass a per-call
min_score, defaulting to the configured floor.MCP
server.pyacceptsmin_score; the recall hook readsHEBB_RECALL_MIN_SCORE. Both fall back tostrict_recall=True(config floor) when unset.5. Reordering changes A/B-tested; monotonic floor-only changes may ship without A/B.
This PR ships no reordering change — the default floor is unchanged and the floor loop is unchanged. All new code is additive (probe + reports + knob + one crash fix). No A/B required.
Additional fix
fix(rerank): set CrossEncoder max_length(src/hebb/retrieval/rerank/local.py) — production crash fix required for rerank-on to run on long haystacks. sentence-transformers 5.x'sCrossEncoder.predictno longer truncates(query, content)pairs to the tokenizer'smodel_max_length; a long pair overflows XLM-Roberta's 514 position-embedding cap and the forward pass crashes withgather: index 514 is out of bounds. Setmax_length=512at construction. Without this, rerank-on recall crashes on any long-document haystack (e.g. LongMemEval's ~120K-token sessions), so it is a prerequisite for any future rerank-on eval.Known limitations
Acceptance criterion 2 is not met: the shipped default
recall_min_score=0.8is kept, not retuned. The reason is that the eval could not validate the floor on the production rerank-on path, because the BGE-reranker-base sigmoid scores are all too high — they compress into the narrow 0.97–1.0 band.Concretely:
Under rerank-on the floor is inert, so no threshold can be discriminated by eval. The rerank-on grid (
longmemeval_20260730-013838, 21 configs) produces byte-identical strict results across everymin_scoreandrerank_floor_ratio: anymin_score × ratiofloor (max 0.9 × 0.8 = 0.72) sits below the 0.97–1.0 sigmoid band and cuts nothing in the reranked pool. Sincetop_k(10 for the hook, 5 for MCP) is smaller than the rerank pool (rerank_top_n=30), the returned top-k is drawn entirely from the reranked pool, where the floor has no effect. There is nothing to tune against on this path today.The rerank-off data cannot be used to retune the production default. The three rerank-off reports show 0.8 empties 26–55% on the composite scale, with 0.4 looking optimal. But production runs rerank-on: the floor is translated to the sigmoid scale (
0.8 × 0.625 = 0.5), against which the 0.97–1.0 BGE scores are never cut — so 0.8 does not actually empty recall in production. Retuning 0.8 → 0.4 from composite-scale data would be applying a wrong-scale conclusion to a different scale, and risks over-correcting (admitting low-composite tail noise) for a problem that the rerank-on path does not exhibit.Tuning the floor meaningfully requires sweeping
rerank_floor_ratioon the real rerank-on sigmoid scale — raising the sigmoid floor toward the 0.97+ band until it can separate relevant from non-relevant pool entries. That work is outside this issue's scope (the issue places the rerank/sigmoid recalibration at the scope boundary) and is left to a follow-up.Given the above, this PR takes the conservative position: keep the default at 0.8, do not retune from wrong-scale data, and document the rerank-on floor-inertness with the committed grid report as evidence. The
rerank_floor_ratioknob and the per-callmin_scoreknob are both in place, so the follow-up can retune without further code changes.Follow-up
A separate issue will be opened: Tune
rerank_floor_ratioon the rerank-on sigmoid scale — with rerank enabled, sweeprerank_floor_ratio(e.g. 0.4 / 0.5 / 0.625 / 0.7 / 0.8) until the floor can discriminate results within the 0.97–1.0 BGE band, then revisit the defaultrecall_min_score.Test plan
tests/unit/test_audit_retrieval.pydual-scale floor tests still pass — 9 passed locallyeval/retrieval_lab.py --sweep-floor --dataset locomo --vectorreproduces the committed report shape — verified locally, output format matcheslocomo_20260728-111918eval/retrieval_lab.py --sweep-floor --dataset longmemeval --vector --rerankreproduces the flat-line (floor-inert) result — committed reportlongmemeval_20260730-013838(21 configs identical, R@10 0.942, emptied 0)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation