feat(retrieval): live close-the-loop relevance-signal infrastructure (#779) - #789
Conversation
![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
|
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 ignored due to path filters (1)
📒 Files selected for processing (9)
✨ 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
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
[claim:review:mondragon:2026-05-14T15:51:32Z] |
robotrocketscience
left a comment
robotrocketscience
left a comment
There was a problem hiding this comment.
Approve
Read all 1923 added lines across the four source files (store.py, hook.py, retrieval.py, relevance_detection.py), the design doc, and CHANGELOG. Ran the four new test files locally on a fresh uv env — 63 passed in 6.76s. CI was already green on all required gates (pytest 3.12/3.13, CodeQL, staging-gate, bench-smoke, secrets-scan, history-scan, deptry, vulture, typos).
Branch is FF-clean on github/main; all six commits signed (%G? = G for every SHA). Discretion grep on the actual additions is clean — the lone "subagent" hit in the diff output is context from #778's existing CHANGELOG entry above, already on main.
Code-level observations (none blocking)
(1) Sweeper windowing is intentionally loose, and is a known recall-vs-precision trade. _sweep_relevance_signal reads transcript text since the oldest pending event's injected_at and scores every pending belief against that bulk text. So a belief first injected on turn N+1 can substring-match content from turn N's response. The bias is upward on referenced, never downward, and the 8-char floor in relevance_detection._MIN_NORMALIZED_LENGTH plus the posterior decay window absorb most of it — fine for v1. Worth a follow-up sub-issue if bench evidence later shows the half-life / anchor-weight posteriors drifting hot under live traffic; the tighter design groups pending events by turn_id and bounds each turn's response window to [turn_injected_at, next_turn_injected_at).
(2) update_meta_belief → update_injection_referenced is not crash-atomic. Each commits independently. A process death between the per-consumer update_meta_belief loop and the update_injection_referenced stamp would cause the next sweep to re-bump the same Beta-Bernoulli posterior. Acceptable per the file's fail-soft posture (this is feedback substrate, not retrieval), and the test suite verifies the no-crash idempotency, but the comment above the stamp could note the window explicitly. Mentioning in case a future "wrap both writes in a transaction" refactor is queued.
(3) Minor — _record_injection_events opens the store once and commits per-hit (1 commit per row in the auto-commit-on-execute style elsewhere in MemoryStore). Typical UPS injections are O(10) beliefs, so this is fine, but if the hit count ever spikes (e.g., a future broad-expansion path), a batched executemany + single commit would amortize fsync. Not for this PR.
What I confirmed
- Schema correct:
referencedtri-state, FK cascade onbeliefs(id), partial indexWHERE referenced IS NULLmatches the sweeper's hot query. record_injection_eventcanonical-sort-deduplicatesactive_consumersbefore JSON-encoding — same determinism property asencode_signal_weights. Emptysourcerejected.list_pending_injection_events.before_turn_idlexicographic slicing is correct because both writers (transcript_logger._new_turn_idand the new_new_injection_event_turn_id) prefix%Y%m%dT%H%M%S%fZ→ lex = chrono. Documented inline.update_injection_referencedis idempotent at theUPDATE ... WHERE referenced IS NULLlevel.referenced ∈ {0, 1}validated.normalize_textis a fixed point (NFC + casefold + whitespace collapse); German ß fold and NFC compose/decompose covered in tests.STRATEGY_NGRAM_OVERLAPcorrectly raisesValueErrorrather than silently fallback-dispatching.- Sweeper placement honors the design: runs after
apply_sentiment_feedback, before this turn's retrieval, so shifted posteriors influence the very next reranker call. get_active_meta_belief_consumers()returns sorted output → determinism-replay friendly.- Honors locked PHILOSOPHY (#605,
c06f8d575fad71fb): pure stdlib, no embeddings, no LLM judges, no wall-clock state in the scoring layer.
Tagging ready-to-merge.
robotrocketscience
added
ready-to-merge
|
[release:review:mondragon:2026-05-14T15:55:01Z] |
|
merge-train: blocked 1 review thread(s) are unresolved on these files: tests/test_relevance_sweeper.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
|
[claim:review:bagheera:2026-05-14T16:57:58Z] |
CodeQL flagged the unused import on tests/test_relevance_sweeper.py (only META_HALF_LIFE_KEY is referenced). Collapses the multi-name import to a single-line form. Unblocks merge-train on PR #789.
robotrocketscience
added
the
ready-to-merge
|
[release:review:bagheera:2026-05-14T16:59:23Z] |
|
Surfacing the merge-train block diagnosis (yeats, 2026-05-14T17:00Z) without modifying the PR — author / next claimant decides whether to act. The unresolved review thread on
Two problems with the finding:
The alert appears to reference a symbol that existed on an earlier revision of this branch and was renamed during development. The current branch state is clean. Two more blockers separate from the CodeQL thread:
I haven't claimed this PR — surfacing only. Author or next reviewer can: (a) dismiss the CodeQL alert as false-positive via Security tab; (b) rebase; (c) re-add |
|
[claim:review:bagheera:2026-05-14T17:05:42Z] |
First commit of the #779 close-the-loop relevance-signal infrastructure. Per the schema ratification in #779#issuecomment-4448107904 (2026-05-14): - One row per (UPS-or-pre_compact event, injected belief). FK to beliefs(id) ON DELETE CASCADE, autoincrement PK — same pattern as belief_corroborations (#190) and deferred_feedback_queue (#191). - active_consumers is a TEXT column carrying a canonical-sorted JSON array of meta-belief keys whose retrieval consumer fired on this turn. JSON over sidecar table because every read path is "one row per event, all consumers"; no query asks "find events where consumer X was active" today. - referenced is tri-state (NULL / 0 / 1) — NULL means the sweeper hasn't scored this row yet. - Three indexes: (session_id, turn_id) for sweeper joins, belief_id for FK cascade hygiene, and a partial index on pending rows so the sweeper hot path stays bounded. No store API yet — that lands in the next commit. CREATE TABLE IF NOT EXISTS is idempotent so pre-#779 stores migrate forward on first open.
…nced (#779) Three methods on MemoryStore: - record_injection_event(session_id, turn_id, belief_id, *, injected_at, source, active_consumers) — inserts one row. active_consumers is dedup-sorted into a canonical-JSON column value so two stores with the same subscriptions produce byte- identical column bytes (same determinism property as meta_beliefs.encode_signal_weights). - list_pending_injection_events(session_id, *, before_turn_id=None, limit=1000) — returns [(id, turn_id, belief_id, injected_at, source, active_consumers)] for rows with referenced IS NULL in the given session. before_turn_id slices to "prior turns only" via string-lexicographic comparison; this works because transcript_logger._new_turn_id() is `{utc-compact-ts}-{hex4}` and the timestamp prefix sorts chronologically. Ordered by id ASC. - update_injection_referenced(event_id, *, referenced, referenced_at) — stamps referenced ∈ {0, 1} on a row that still has referenced IS NULL. Returns True on first call, False on re-call or unknown id (idempotent at the UPDATE WHERE level — a scored row's prior score is never overwritten). 19 new tests in tests/test_injection_events.py cover schema presence, columns, indexes, canonical-JSON encoding, dedup, empty-consumer default, FK cascade on belief delete, pending filter, session filter, before_turn_id slicing, limit, decode round-trip, deterministic order, idempotent update, bad-value rejection, and unknown-id no-op.
Wires the close-the-loop relevance-signal write-path on every
UPS retrieval that produces a non-empty hits list. Mirrors the
existing `_emit_user_prompt_submit_rebuild_log` call site at
hook.py:865 — both fire on the same `if hits:` branch.
Adds:
- `_new_injection_event_turn_id()` in hook.py — same
`{utc-compact-ts}-{hex4}` shape as transcript_logger's turn ids
so the lexicographic-sort-is-chronological invariant
`list_pending_injection_events.before_turn_id` relies on holds
across writers. Generated fresh per UPS fire; not shared with
transcript_logger's user-line id (the sweeper joins on
session_id + temporal order, not turn-id equality).
- `_record_injection_events(...)` in hook.py — opens the store,
writes one row per hit, fail-soft. Skips :memory: DBs (the
rebuild_log emit applies the same guard) and empty session ids.
- `get_active_meta_belief_consumers()` in retrieval.py — returns
the canonical-sorted list of meta-belief keys whose env flag is
truthy. v1 covers `#756` half-life + `#757` bm25f_anchor_weight;
siblings (#758-#760) drop in here as they ship.
Wiring change in `user_prompt_submit`: between the rebuild_log
emit and the `total_chars` measurement, fire one
`_record_injection_events` call carrying:
- the session_id
- a fresh turn_id (shared across the batch)
- the post-dedup `hits` list
- source="ups"
- active_consumers from `get_active_meta_belief_consumers()`
Sweeper (Layer 3 / next commit's neighbour) will read these rows
on the *next* UPS turn and update each active consumer's
`relevance` sub-posterior.
10 new tests in tests/test_hook_injection_events_wiring.py cover
turn-id shape + uniqueness, get_active_meta_belief_consumers
under all env-flag permutations (sorted output), fail-soft on
missing session_id / empty hits / bad DB path, end-to-end UPS
fire records one event per hit with source='ups' and
referenced=NULL, threading the half-life consumer when its env
flag is on, and the batch-shares-a-turn-id invariant.
New module src/aelfrice/relevance_detection.py implements the deterministic detection strategy ratified in #779#issuecomment-4448107904 (Q5 / Q6): - `normalize_text(s)` — NFC + casefold + whitespace collapse. Idempotent (running twice is a fixed point). casefold over lower() so German ß → ss folds correctly. NFC so composed and decomposed café both normalise to the same byte string. - `is_referenced(belief_content, response_text)` — True iff the normalised belief content appears verbatim inside the normalised response. Beliefs shorter than 8 normalised chars return False (precision bias — a 1-3 char belief would match almost any response and shift posteriors on noise). - `score_references(belief_pairs, response_text, *, strategy=...)` — bulk variant returning `[(event_id, referenced)]` in input order. Short beliefs score 0 (not filtered) so the sweeper's idempotent update_injection_referenced stamps them as scored rather than leaving them pending forever. - `STRATEGY_EXACT_SUBSTRING` / `STRATEGY_NGRAM_OVERLAP` constants — n-gram is reserved for a future sub-issue (Q5 deferral). The v1 dispatch raises ValueError on anything other than the exact-substring strategy rather than silently picking a fallback. Pure stdlib, no embeddings, no LLM judges — honours the locked PHILOSOPHY (#605, c06f8d575fad71fb). Same inputs yield byte-identical output. 23 new tests cover: normalize_text idempotency, casefold, NFC compose/decompose match, whitespace-variant collapse (CRLF / tabs / multi-space), leading/trailing strip, German ß fold, empty string; is_referenced positive / case-insensitive / whitespace-tolerant / negative / short-belief boundary / unicode diacritic match / empty-response; score_references one-pair-per- input, input-order preservation, empty list, short-belief scoring, unsupported-strategy ValueError, explicit substring strategy, determinism across runs.
Final wiring: at the *start* of every UPS hook (right after the
sentiment-feedback pass, before this turn's retrieval), the
sweeper scores the prior turn's pending injection_events and
pushes `relevance` evidence into each event's active consumers.
Adds in hook.py:
- `_read_assistant_text_since(session_id, since_iso, *, stderr)` —
reads `turns.jsonl` (transcript_logger's Stop-hook output),
filters by session_id + role=='assistant' + ts > since_iso,
concatenates in file order. Fail-soft on missing file /
malformed lines.
- `_sweep_relevance_signal(*, session_id, stderr)` —
1. list_pending_injection_events(session_id)
2. _read_assistant_text_since(session_id, oldest.injected_at)
3. join event_id → belief.content via get_belief()
4. score_references(pairs, response_text)
5. for each (event_id, referenced):
- update_meta_belief(consumer_key, SIGNAL_RELEVANCE,
evidence=float(referenced), now_ts) per active_consumer
- update_injection_referenced(event_id, referenced,
referenced_at) — idempotent at the UPDATE-WHERE-NULL level
Fail-soft on every layer: store-open, transcript-read,
update_meta_belief, and update_injection_referenced errors each
print one stderr line and return. The sweeper is feedback
substrate; a write failure must not break the user-visible
retrieval contract that follows in the same UPS hook.
Substrate compatibility: meta-beliefs that didn't subscribe to
SIGNAL_RELEVANCE (e.g. the v1 #756 half-life consumer, which is
latency-only) silently no-op per `update_meta_belief`'s
no-row-no-write contract. The event still gets stamped as scored
so it never re-sweeps. This keeps the sweeper single-sourced via
`active_consumers` — siblings that DO subscribe to relevance
(test consumers, future #757 / #758 / #760 once they add
relevance) light up automatically.
11 new tests cover:
- _read_assistant_text_since: missing file, session/role
filtering, file-order concatenation, malformed-line skip.
- _sweep_relevance_signal: no-session no-op, no-pending no-op,
no-transcript leaves events pending, hit case shifts
posterior (alpha+beta > 1 off prior), miss case scores 0,
second run is idempotent (no double-count), latency-only
consumer stamps event but doesn't materialise relevance row.
Wiring into `user_prompt_submit`: one line inserted between
`apply_sentiment_feedback(...)` and the prompt-shape gate, in
the same fail-soft posture as the sentiment lane.
Adds the CHANGELOG entry summarising the three-layer architecture shipped across the four prior atomic commits + the design doc at docs/design/relevance-signal.md covering data flow, schema rationale, determinism contract, and the explicit deferral list (GC, pre_compact source, n-gram detection, embedding/LLM-judge).
CodeQL flagged the unused import on tests/test_relevance_sweeper.py (only META_HALF_LIFE_KEY is referenced). Collapses the multi-name import to a single-line form. Unblocks merge-train on PR #789.
a70d6fc to
c345f3d
Compare
robotrocketscience
removed
the
ready-to-merge
|
Rebased on Verification on the rebased branch:
Re-adding |
|
merge-train: merged c345f3d → |
robotrocketscience
added
the
ready-to-merge
|
[release:review:bagheera:2026-05-14T17:11:06Z] |
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged c345f3d → |
2 participants
Closes #779
Schema ratified at #779#issuecomment-4448107904 (2026-05-14). Six atomic commits, all signed:
injection_eventstable + 3 indexesMemoryStoreAPI:record_injection_event/list_pending_injection_events/update_injection_referencedget_active_meta_belief_consumers()src/aelfrice/relevance_detection.py— exact-substring strategy_sweep_relevance_signalwired into UPS hookdocs/design/relevance-signal.mdThree-layer architecture
Layer 1 —
injection_events. New SQL table with(id, session_id, turn_id, belief_id, injected_at, source, active_consumers, referenced, referenced_at).active_consumersis a canonical-sorted JSON array of meta-belief keys whose retrieval consumer was env-gated ON for the turn. Three indexes:(session_id, turn_id),(belief_id), and a partial(session_id, referenced) WHERE referenced IS NULLfor the sweeper's hot path.CREATE TABLE IF NOT EXISTSmigrates pre-#779 stores forward on first open.Layer 2 —
relevance_detection.py. Pure-function reference detection:normalize_text(s)— NFC + casefold + whitespace collapse. Fixed-point idempotent (running twice yields the same bytes).is_referenced(belief, response)— True iff normalised belief content appears verbatim in normalised response. 8-char minimum belief length to prevent short-belief false positives.score_references(pairs, response_text, *, strategy=STRATEGY_EXACT_SUBSTRING)— bulk variant; preserves input order.STRATEGY_NGRAM_OVERLAPreserved (raisesValueErrorin v1; Q5 deferral).Layer 3 —
_sweep_relevance_signal. Fires at the start of every UPS hook, right afterapply_sentiment_feedback, before this turn's retrieval. Reads pending events for the session, joinsevent.belief_id → belief.contentviaget_belief(), reads the assistant transcript via_read_assistant_text_since(session_id, oldest.injected_at)(filtersturns.jsonlby session + role + ts), scores, firesupdate_meta_belief(consumer_key, SIGNAL_RELEVANCE, evidence=float(referenced), ...)once per consumer key inactive_consumers, then idempotently stamps the row.The substrate's "no-op on non-subscribed consumer" contract (
update_meta_beliefreturns False if the signal class isn't in the consumer's subscription) keeps the wiring single-sourced via the env flags. v1 ships withlatency-only #756 +bm25_l0_ratio-only #757; neither materialises arelevanceposterior row, but both get their events stamped as scored. Siblings that DO subscribe to relevance (test consumers, future #758–#760) light up automatically.Determinism contract (#605,
c06f8d575fad71fb)normalize_textis pure stdlib.score_referencesis a pure function over(events, response_text)→ byte-identical output, in input order.update_meta_belief'snow_ts(substrate decay math is wall-clock-independent at the function level) andreferenced_at(audit-only, never re-read).Fail-soft posture
Every layer is fail-soft. Store-open, transcript-read,
update_meta_belief, andupdate_injection_referencederrors each print one stderr line and return. The sweeper is feedback substrate — never breaks retrieval.Ratification log (Q1–Q6, from #779#issuecomment-4448107904)
pre_compactaccommodated by TEXT-not-CHECK column.Verification
pytest: 4077 passed, 62 skipped, 75 xfailed (full suite, 78s).test_injection_events.py, 23 intest_relevance_detection.py, 10 intest_hook_injection_events_wiring.py, 11 intest_relevance_sweeper.py= 63 new tests.%G?=G).Coverage
before_turn_idslicing, decode round-trip, deterministic order, limit, idempotent update, bad-value rejection.Deferred (sub-issues path)
source='pre_compact'— schema accommodates without migration.STRATEGY_NGRAM_OVERLAPreserved.relevance— separate migration decision perinstall_meta_belief's immutable-config contract.Out of scope
relevanceaxis — gated on [v2.0] Reproducibility harness —benchmarks/results/v2.0.0.jsonis canonical,uv sync && aelf bench all#437 corpus evidence + the sub-issue path above.injection_eventslive — operator SQL oraelf doctor --jsonextension if needed.