feat(hook): coverage line surfaces L1 retrieval/index asymmetry (#857) - #863
Conversation
Reviewer's GuideImplements a coverage-line feature in the UserPromptSubmit hook that surfaces when L1 keyword-match candidates were trimmed by the token budget, backed by new LaneTelemetry plumbing, a reset helper for telemetry, and focused unit/integration tests plus changelog entry. Sequence diagram for coverage line in user_prompt_submitsequenceDiagram
actor User
participant Hook as user_prompt_submit
participant Retrieval as _retrieve
participant Tel as LaneTelemetry_store
User->>Hook: user_prompt_submit(prompt, budget)
alt not gate_skip
Hook->>Tel: _reset_last_telemetry(LaneTelemetry())
Hook->>Retrieval: _retrieve(prompt, budget)
Retrieval-->>Hook: hits
Hook->>Tel: last_lane_telemetry()
Tel-->>Hook: LaneTelemetry
Hook->>Hook: _coverage_line(len(hits), tel, prompt)
Hook-->>User: body (+ coverage when delta>0)
else gate_skip
Hook-->>User: no hits, no coverage line
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR implements a retrieval coverage reporting feature for the memory hook. It adds telemetry tracking of L1 candidate counts before token-budget trimming, integrates hook logic to reset and read that state per turn, formats a coverage suffix when candidates are dropped, and provides comprehensive unit and integration tests to validate the behavior. ChangesCoverage Line Feature
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 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 |
![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 2 issues, and left some high level feedback:
- The implementation of
_coverage_linetruncatessearch_topicas well as the display text, which conflicts with the spec/CHANGELOG promise that the search hint uses the verbatim topic; if that contract matters, consider keepingsearch_topicuntruncated while only truncatingdisplay_topic. - Hook code now depends on the private
_reset_last_telemetrysymbol fromretrieval.py; if this reset behavior is intended to be stable, consider either dropping the leading underscore or exposing a small public helper to avoid cross-module reliance on a private API.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The implementation of `_coverage_line` truncates `search_topic` as well as the display text, which conflicts with the spec/CHANGELOG promise that the search hint uses the verbatim topic; if that contract matters, consider keeping `search_topic` untruncated while only truncating `display_topic`.
- Hook code now depends on the private `_reset_last_telemetry` symbol from `retrieval.py`; if this reset behavior is intended to be stable, consider either dropping the leading underscore or exposing a small public helper to avoid cross-module reliance on a private API.
## Individual Comments
### Comment 1
<location path="src/aelfrice/hook.py" line_range="1489-1490" />
<code_context>
+ search_topic = raw_topic[:_COVERAGE_TOPIC_MAX_CHARS] if truncated else raw_topic
+ display_topic = search_topic + "…" if truncated else raw_topic
+ return (
+ f"retrieved {n_injected} of {m_total} matching beliefs for "
+ f'"{display_topic}"; run `aelf search {search_topic}` to see the rest.\n'
+ )
+
</code_context>
<issue_to_address>
**🚨 suggestion (security):** The suggested `aelf search` command may be unsafe or confusing for prompts with shell-sensitive characters.
Because `{search_topic}` is interpolated directly into a shell command, any quotes, backticks, `$`, or other metacharacters in the prompt could change the meaning of the command if users copy-paste it. Consider either quoting and escaping `{search_topic}` or generating a shell-escaped version (e.g., via `shlex.quote`) so the suggested command is safe for arbitrary input.
Suggested implementation:
```python
raw_topic = prompt.strip()
truncated = len(raw_topic) > _COVERAGE_TOPIC_MAX_CHARS
search_topic = raw_topic[:_COVERAGE_TOPIC_MAX_CHARS] if truncated else raw_topic
display_topic = search_topic + "…" if truncated else raw_topic
shell_search_topic = shlex.quote(search_topic)
return (
f"retrieved {n_injected} of {m_total} matching beliefs for "
f'"{display_topic}"; run `aelf search {shell_search_topic}` to see the rest.\n'
)
```
1. Add `import shlex` near the top of `src/aelfrice/hook.py` alongside the other imports, so `shlex.quote` is available.
2. If this module is used in non-POSIX environments and you need platform-specific handling, you may want to document that the suggested command assumes a POSIX-like shell.
</issue_to_address>
### Comment 2
<location path="src/aelfrice/hook.py" line_range="1468" />
<code_context>
+
+def _coverage_line(
+ n_injected: int,
+ tel: Any,
+ prompt: str,
+) -> str:
</code_context>
<issue_to_address>
**suggestion:** Using `Any` for the telemetry parameter obscures the contract of `_coverage_line`.
Because this helper accesses `tel.l1_candidates` and `tel.l1`, please replace `Any` with a concrete type (e.g., `LaneTelemetry`) or a protocol defining those attributes so misuse is caught by the type checker instead of at runtime.
Suggested implementation:
```python
class SupportsL1Telemetry(Protocol):
"""Telemetry required by _coverage_line.
Any telemetry object providing these attributes can be passed to
`_coverage_line`, allowing static type checkers to validate usage.
"""
l1_candidates: int
l1: int
_COVERAGE_TOPIC_MAX_CHARS: Final[int] = 60
```
```python
def _coverage_line(
n_injected: int,
tel: SupportsL1Telemetry,
prompt: str,
) -> str:
```
1. Ensure `Protocol` is imported in this module, e.g., adjust the existing typing imports to include it:
`from typing import Any, Final, Protocol`
(Keep `Any` if used elsewhere; remove it only if no longer needed.)
2. If your codebase prefers a different naming convention, you may want to rename `SupportsL1Telemetry` to align with existing protocol/type names.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[claim:review:idnn:2026-05-18T17:19:36Z] |
robotrocketscience
added
ready-to-merge
|
[release:review:idnn:2026-05-18T17:21:52Z] |
|
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
robotrocketscience
added
the
attn:review
|
[claim:review:idnn:2026-05-18T19:48:35Z] |
robotrocketscience
left a comment
robotrocketscience
left a comment
There was a problem hiding this comment.
Review against #857 acceptance items
All four acceptance items satisfied:
- Hook computes M alongside N — via new
LaneTelemetry.l1_candidates, populated at bothretrieve()andretrieve_with_tiers()callsites inretrieval.py:2650, 2896. - Coverage line appended when M > N —
_coverage_linereturns the line only whendelta = l1_candidates - l1 > 0; empty otherwise. Appended tobodyafter the closing</aelfrice-memory>tag, beforesout.write. - Tests —
tests/test_hook_coverage_line.pycovers M==N omitted, M>N shown, M==0 no-op; plus locked+L25 counted, long-prompt truncated, short-prompt verbatim, no belief content, and a BFS-pad regression. Integration tests viauser_prompt_submitexercise the live hook path. - Privacy-safe text — line contains only counts and the user's own prompt keywords.
test_coverage_line_no_belief_contentpins this.
Design notes
_reset_last_telemetry pre-retrieval reset — solving stale-snapshot leakage from mocked _retrieve test paths by zeroing before the call is the right shape. Real _retrieve overwrites _LAST_TELEMETRY at the end of _cost so the reset is invisible in production; only test paths that mock _retrieve see the zero state, which is precisely the behavior wanted (no spurious coverage line on mocked retrievals). Sourcery's "cross-module dependence on a private symbol" point is mechanical to address (drop the underscore) but the use is constrained to one in-package call site, so the underscore is fine as a "do not depend on from outside aelfrice" signal.
L1-delta formulation — M = n_injected + (l1_candidates − l1) is invariant under BFS / future-lane padding of n_injected, which is correct for trim detection. The BFS regression test pinning "15 of 20" demonstrates this.
Findings
1. CHANGELOG wording vs implementation — minor mismatch
CHANGELOG/v3.md line for this feature says "(truncated to 60 chars for display)" but the implementation truncates both display_topic and search_topic:
search_topic = raw_topic[:_COVERAGE_TOPIC_MAX_CHARS] if truncated else raw_topic
display_topic = search_topic + "…" if truncated else raw_topicSo the suggested aelf search <topic> command also uses the truncated form, not just the displayed quote. Two options:
- (a) Tighten CHANGELOG: drop "for display" → "(truncated to 60 chars)".
- (b) Pass
raw_topic(full) toaelf searchand truncate onlydisplay_topic.
Functionally equivalent for BM25 recall, but (a) or (b) closes the spec/impl gap. Sourcery flagged the same point.
2. test_coverage_line_counts_include_locked_and_l25 docstring is misleading
def test_coverage_line_counts_include_locked_and_l25() -> None:
"""M sums locked + l25 + l1_candidates; N is total injected."""The implementation computes M = n_injected + (l1_candidates − l1), not "locked + l25 + l1_candidates." The test happens to verify M=5 because in this setup 3 + (3 − 1) = 5, which equals 1 + 1 + 3. A future reader reading the docstring would extract a wrong formula. Suggest re-wording to e.g. "L1 trim delta is reported even when locked / L25 contributed to N."
3. BFS-padded N vs spec definition of M (informational)
#857's spec defines M = total beliefs matching the same keyword scoring at any non-zero relevance. With BFS hops added to n_injected, the reported "M" includes those BFS-pad-derived items: e.g. n_injected=15 (5 BFS) + delta=5 → "15 of 20". A user running aelf search <topic> after seeing "15 of 20" will get matches without BFS expansion and may see counts that don't reconcile.
The behavior is consistent and the trim signal surfaces correctly — this is the feature's primary value. But the reported numbers aren't literally "matches at any non-zero relevance" from the spec; they're "injected (incl. BFS) + L1 trim." Worth a small CHANGELOG / docstring note that the count reflects the L1-budget asymmetry, not a literal aelf search row count. Non-blocking.
4. Sourcery's shlex.quote(search_topic) suggestion — operator call
The line is a display hint the user reads; it is not exec'd. A copy-paste attack would require the user to type adversarial input into their own prompt and then paste their own bytes into their own shell. Real but narrow. shlex.quote is mechanical defense-in-depth; the trade-off is uglier output for the common case (no special chars). Defer to operator.
CI / merge readiness
- pytest 3.12 / 3.13: SUCCESS
- bench-smoke: SUCCESS
- CodeQL (python / actions): SUCCESS
- All Staging Gate checks (secrets / pattern / history / release-docs / commit-msg / pr-title / pr-body-issue-link): SUCCESS
- migration-policy-check: SUCCESS
- deptry / vulture / typos: SUCCESS
- size-check (final commit): SUCCESS
- Mergeable: true
Discretion grep on the diff is clean.
Recommendation
Findings #1 and #2 are doc-only and would be nicer to fix before merge (one CHANGELOG word + one test docstring). #3 is a doc/spec reconciliation that can land later. #4 is operator judgment. The implementation itself is correct against the acceptance items and CI is green.
Not adding ready-to-merge — leaving the disposition on #1/#2/#4 to the operator.
|
[release:review:idnn:2026-05-18T19:51:40Z] |
robotrocketscience
added
attn:decisions-needed
|
[claim:review:Idnn:2026-05-18T22:24:28Z] |
Disposition (Idnn)Operator delegated the call. Disposition: approve as-is, merge-train. Walking the four findings from the prior review:
Acceptance items from #857 all satisfied (M alongside N, line gated on M>N, edge cases tested, no belief content surfaced). CI green across pytest 3.12/3.13, bench-smoke, CodeQL, Staging Gate, deptry/vulture/typos, size-check. Mergeable: true. Findings #1 and #2 can be picked up by anyone in a 2-line doc-tidy follow-up if the cosmetics matter — flagging here so they don't get lost. Adding |
robotrocketscience
added
ready-to-merge
|
[release:review:Idnn:2026-05-18T22:27:04Z] |
|
merge-train: blocked branch is not fast-forward on The |
github-actions
Bot
removed
the
ready-to-merge
After each UserPromptSubmit retrieval, emit a one-line suffix outside the </aelfrice-memory> block when M > N (M = total L1 candidates before token-budget trim, N = injected). Format: retrieved N of M matching beliefs for "topic"; run `aelf search topic` to see the rest. Mechanism: - New `l1_candidates` field on `LaneTelemetry` (populated in both `retrieve()` and `retrieve_with_tiers()`) tracks the pre-budget L1 candidate count alongside the existing packed `l1` count. - New `_reset_last_telemetry()` in `retrieval.py` lets the hook reset the process-level snapshot before each UPS retrieval so mocked `_retrieve` callers in tests see a clean zero-state, not stale data. - New `_coverage_line(n_injected, tel, prompt)` in `hook.py` computes M = tel.locked + tel.l25 + tel.l1_candidates and returns the line when M > N. Prompt is truncated to 60 chars for display and the search command argument. No belief content is included. - `user_prompt_submit` calls `_reset_last_telemetry` before `_retrieve`, reads `last_lane_telemetry()` after, and appends the coverage line to `body` when non-empty, before `sout.write(body)`.
10 tests in tests/test_hook_coverage_line.py covering all three acceptance items from the spec: Unit tests on _coverage_line(): - M == N: line omitted - M > N: line emitted with counts and prompt topic - M == 0: no-op edge case - locked + L2.5 counted in M - no belief content in coverage line - long prompt truncated at 60 chars - short prompt preserved verbatim Integration tests via user_prompt_submit(): - token-budget truncation triggers line (M > N) - all candidates fit → line omitted (M == N) - no hits → empty output (M == 0)
…adding The old formula compared m_total (locked+l25+l1_candidates) against n_injected, but n_injected includes BFS hops that are not counted in m_total. When BFS pads n_injected to match or exceed m_total the guard fires and the coverage line is suppressed even though L1 candidates were genuinely trimmed by the token budget. New formula: delta = l1_candidates - l1. When delta <= 0 nothing was cut; line is omitted. When delta > 0, m_total = n_injected + delta. This is independent of any non-L1 surfaced lane.
…rage line Proves that when BFS hops push n_injected above l1_candidates but L1 was still trimmed by budget, the coverage line fires correctly and reports the right counts.
0bd104d to
934559d
Compare
robotrocketscience
added
the
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
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged 934559d → |
1 participant
Closes #857. Sub-issue of #855 — defect 2 (retrieval vs index recall asymmetry).
What this ships
When the per-turn memory hook truncates L1 keyword-match candidates because of the token budget, it now appends one coverage line after the
</aelfrice-memory>block:Nis what was injected this turn;M = N + (l1_candidates − l1_packed)— total surfaced plus the count cut by the budget cap. The line is suppressed when nothing was cut (delta == 0), so existing turns where every L1 candidate already fit emit byte-identical output.The topic in the line is the user's own prompt (truncated at 60 chars for the display portion, kept verbatim for the search hint). The line never echoes belief content, IDs, or any data from the store — only counts plus the prompt the user already typed.
Why this formula
First pass computed
M = locked + l2.5 + l1_candidatesand compared againstlen(hits). That breaks the moment another lane (BFS hops, future expansion modes) padsn_injectedabovem_total: the comparison short-circuits and the line silently disappears even when L1 was genuinely trimmed. The delta formulation is invariant under non-L1 lanes because it only measures what L1 dropped.The regression test
test_coverage_line_fires_when_bfs_pads_n_above_l1_candidatesexercises exactly that case (locked=2, l25=3, l1=5, l1_candidates=10, n=15→retrieved 15 of 20).Surface touched
src/aelfrice/retrieval.py—LaneTelemetrygainsl1_candidates: int._reset_last_telemetry()helper added so the hook can zero the per-process snapshot before each retrieval (defensive for the mocked-_retrievepath; production retrieval overwrites it immediately).src/aelfrice/hook.py—_coverage_line()(new), called inuser_prompt_submitafter_format_hitsbuilds the block. The existinglast_lane_telemetry()read was moved earlier in the function so the coverage suffix lands inside the same write as the audit body.tests/test_hook_coverage_line.py— 9 tests: unit (M==N, M>N, M==0, locked/l25 in counts, BFS-padding regression, no-belief-content, long-prompt truncation, short-prompt verbatim), integration (budget-truncates, all-fit, no-hits).CHANGELOG/v3.md— Unreleased entry.Verification
uv run pytest -x— 4191 passed, 62 skipped, 75 xfailed (was 4190; delta is the new regression test).git diff github/main...HEAD— empty.Acceptance items (from #857)
Summary by Sourcery
Add a per-turn coverage line in the user prompt hook to surface when keyword-matching beliefs were dropped due to L1 token-budget limits.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
Release Notes
New Features
aelf searchto view all results.