feat(retrieval): wire the exploration slot into the UPS pack (#1279) - #1285
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
robotrocketscience
added
the
attn:review
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughChangesExploration slots
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant UserPromptSubmit
participant RetrievalConfig
participant MemoryStore
participant Exploration
participant ExplorationLedger
UserPromptSubmit->>RetrievalConfig: resolve exploration settings
UserPromptSubmit->>Exploration: check cadence and derive draw
Exploration->>MemoryStore: query exploration pool
MemoryStore-->>Exploration: return candidate beliefs
Exploration->>UserPromptSubmit: return substitutions
UserPromptSubmit->>ExplorationLedger: record exploration event
UserPromptSubmit->>UserPromptSubmit: render retrieved hits
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Reviewer's GuideWires the previously inert exploration-slot mechanism into the UPS retrieval hook, adds configuration resolvers and documentation for its controls, and introduces a focused test suite that pins its behavioral invariants and fail-soft guarantees. Sequence diagram for the exploration slot during user prompt submissionsequenceDiagram
actor User
participant Hook as user_prompt_submit
participant Slot as _substitute_exploration_slots
participant Ring as read_ring_state
participant RetrCfg as retrieval_config
participant Expl as exploration
participant Store as MemoryStore
participant Ledger as Store.record_exploration
participant Inj as _record_injection_events
User->>Hook: submit prompt
Hook->>Hook: retrieval produces hits
Hook->>Slot: _substitute_exploration_slots(hits, session_id, query, store, serr)
Slot->>RetrCfg: is_exploration_enabled()
RetrCfg-->>Slot: enabled?
Slot->>Ring: read_ring_state(session_id)
Ring-->>Slot: state (next_fire_idx)
Slot->>Expl: should_explore(fire_idx, cadence=resolve_exploration_cadence())
Expl-->>Slot: explore?
Slot->>Store: exploration_pool(query)
Store-->>Slot: pool (never_injected beliefs)
Slot->>Expl: derive_seed(session_id, fire_idx, query)
Expl-->>Slot: seed
Slot->>RetrCfg: resolve_exploration_slots()
RetrCfg-->>Slot: slots
Slot->>Expl: draw_uniform(pool, seed, count=slots)
Expl-->>Slot: drawn_ids
Slot->>Store: get_belief(drawn_id)
Store-->>Slot: drawn beliefs
Slot->>Slot: compute need via _belief_tokens(drawn)
Slot->>Slot: displace non_locked tail until freed_tokens >= need
Slot->>Ledger: record_exploration(fire_idx, seed, query, candidate_ids, drawn_ids, displaced_ids)
Ledger-->>Slot: (best-effort, errors logged)
Slot-->>Hook: updated hits (substitution, never append)
Hook->>Inj: _record_injection_events(session_id, hits,...)
Inj-->>Hook: injection_events
Hook-->>User: rendered response with pack
Note over Slot,Hook: On any error, Slot returns original hits (fail-soft)
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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:Garsecg:2026-08-01T03:45:11Z] |
|
[claim:review:Toug:2026-08-01T03:49:32Z] |
|
[release:review:Toug:2026-08-01T03:49:41Z] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/aelfrice/hook.py (1)
1640-1640: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten the
storetype hint to match the sibling function.
storeis typedobject | Nonehere, while_record_injection_events(line 1760) types the same kind of parameterMemoryStore | None. The function callsstore.exploration_pool(...),store.get_belief(...), andstore.record_exploration(...), none of which exist onobject. The outertry/exceptkeeps this from crashing at runtime, but the loose type hint gives up static checking on theMemoryStoreAPI for this call path.♻️ Proposed fix
def _substitute_exploration_slots( hits: list[Belief], *, session_id: str, query: str, - store: object | None, + store: MemoryStore | None, serr: IO[str], ) -> list[Belief]:🤖 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/aelfrice/hook.py` at line 1640, Update the store parameter type in the function containing this signature from object | None to MemoryStore | None, matching _record_injection_events and enabling static checking for the store API calls.tests/test_exploration_slot_1279.py (1)
252-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a genuine SQLite failure over monkeypatching the storage method.
This test replaces
MemoryStore.exploration_poolwith a raising stub viamonkeypatch.setattr. It is notunittest.mock, so it does not hit the literal pattern the guideline names, but it does substitute real storage-layer behavior with a stub function for the duration of the test, which runs against the stated intent that tests hit a real SQLite DB rather than mocks.Closing the store's connection before the call (
store.close()) would raise a genuinesqlite3.ProgrammingErrorfrom the real DB layer and exercise the same fail-soft path without stubbing out the method.♻️ Proposed fix
def test_a_raising_pool_query_leaves_the_pack_untouched( store, monkeypatch, capsys, ) -> None: """A research lane must never be why a hook fails.""" _fire(monkeypatch, 20) monkeypatch.setenv("AELFRICE_EXPLORATION", "1") - def _boom(*a, **k): - raise RuntimeError("pool query exploded") - - monkeypatch.setattr(MemoryStore, "exploration_pool", _boom) + store.close() hits = [_mk("h1", "ranked hit one"), _mk("h2", "ranked hit two")] assert _run(store, hits, capsys) == hitsAs per path instructions, "Tests must hit a real SQLite DB, not mocks. Flag any introduction of unittest.mock for the storage layer."
🤖 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/test_exploration_slot_1279.py` around lines 252 - 265, Update test_a_raising_pool_query_leaves_the_pack_untouched to remove the monkeypatch of MemoryStore.exploration_pool and trigger a genuine SQLite failure by closing the store connection before invoking _run. Preserve the existing assertions that the original hits are returned and the fail-soft behavior remains intact.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aelfrice/hook.py`:
- Around line 1083-1095: Update _substitute_exploration_slots to accept cwd:
Path | None = None and pass start=cwd to is_exploration_enabled(),
resolve_exploration_cadence(), and resolve_exploration_slots(). At the
user_prompt_submit call site, provide cwd=payload_cwd so all exploration
settings resolve relative to the payload project directory.
In `@src/aelfrice/retrieval.py`:
- Around line 1254-1292: Update resolve_exploration_cadence and
resolve_exploration_slots so their environment-variable tier accepts zero and
negative values instead of filtering them through _env_positive_int. Preserve
the existing precedence order and integer conversion, ensuring non-positive
environment values reach the same disabling or substitution behavior as kwarg,
TOML, and default values.
---
Nitpick comments:
In `@src/aelfrice/hook.py`:
- Line 1640: Update the store parameter type in the function containing this
signature from object | None to MemoryStore | None, matching
_record_injection_events and enabling static checking for the store API calls.
In `@tests/test_exploration_slot_1279.py`:
- Around line 252-265: Update
test_a_raising_pool_query_leaves_the_pack_untouched to remove the monkeypatch of
MemoryStore.exploration_pool and trigger a genuine SQLite failure by closing the
store connection before invoking _run. Preserve the existing assertions that the
original hits are returned and the fail-soft behavior remains intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c4c7d1e-28d8-4218-97f7-43de73e1efdc
📒 Files selected for processing (5)
CHANGELOG/v4.mddocs/user/CONFIG.mdsrc/aelfrice/hook.pysrc/aelfrice/retrieval.pytests/test_exploration_slot_1279.py
Review — one finding needs an author decision before this can land as "the caller"The code is correct, default-OFF and fail-soft, and I could not fault the implementation. What I could fault is the shipped cadence default: measured against the real workload, this wiring does not fire. What I verified clean first
So the pool, the funding and the plumbing are all fine. The problem is upstream of all of it. The finding: at
|
| window | sessions | UPS injection turns | turns/session p50 | p90 | max | sessions ever reaching fire_idx == 20 |
|---|---|---|---|---|---|---|
| all time | 216 | 956 | 2 | 9 | 66 | 5 (2.3%) |
| since 2026-06-30 (#1016-B regime) | 81 | 259 | 2 | 7 | 12 | 0 (0.0%) |
Fires at the shipped default: 8 / 956 turns (0.84%) all time, 0 / 259 (0.00%) in the current regime.
fire_idx == 0 does not rescue it. On a session's first UPS turn the ring file still belongs to the previous session, so read_ring_state returns {}, fire_idx is None, and the guard returns early — and append_ids never writes 0 (its own docstring: "returns fire_idx + 1 (1 on a freshly-created ring, not 0)"). The first reachable multiple is fire_idx == 20, i.e. a session's 21st injecting turn.
The one path that could see 0 is a session whose ring gets created by a non-append_ids writer (push_classification, update_bytes_at_last_fire) before its second UPS turn. Bounding it both ways rather than guessing: 0 fires if that never happens, 81/259 = 31.3% if it always does. The live ring for the current session shows next_fire_idx: 4 with classifications: [], so the low end is the realistic one. Either way the number the default advertises — 1 in 20, 5% — is not what the workload produces.
This is the umbrella's own recurring failure mode: DEFAULT_K3 = 0.0, DEFAULT_BOOST_QF, the R3 boost arm on #1281 — all wired, all unreachable at their shipped defaults. The AC "exactly one exploration_events row per firing turn" is satisfied by the tests because _fire() pins the counter; nothing pins that a firing turn is reachable from a real ring.
What I need from you (three ways out, all yours to pick — I am not guessing at intent):
- Reachable default — a cadence at or below the median session length (2–3), so the slot fires from a session's 3rd/4th turn.
- Cross-session counter — a store-level monotonic fire counter, which is what makes "1 in 20 turns" mean what it says; cadence 20 then stays.
- Keep 20 deliberately as a long-sessions-only mechanism — but then the coverage claim needs re-pricing, because 84.1% of the store cannot be covered at 0.84% of turns.
Flagging attn:unblock for that decision rather than blocking the diff, since the default is OFF and nothing here is unsafe to merge.
Minor, independent of the above
The resolvers have no tests. is_exploration_enabled, resolve_exploration_cadence and resolve_exploration_slots are not referenced anywhere in tests/test_exploration_slot_1279.py — the suite reaches them only indirectly through AELFRICE_EXPLORATION. AC 1 ("resolves through env → kwarg → TOML → default … a cadence <= 0 disables rather than raising") is checked but unpinned.
And the second half of that AC is not true on the env tier, which is the highest-precedence one:
AELFRICE_EXPLORATION_CADENCE=0 -> _env_positive_int rejects (<= 0) -> returns None
-> falls through to DEFAULT_EXPLORATION_CADENCE = 20
So an operator setting 0 to disable gets cadence 20 instead (with a stderr trace). <= 0 disables via the kwarg and TOML tiers only, where should_explore's guard is reachable. Either honour 0 in the env tier or narrow the docstring — a test on that line would have caught it.
Holding the review claim until the cadence question is answered.
robotrocketscience
added
attn:unblock
|
[claim:review:Setr:2026-08-01T04:24:46Z] |
|
[release:review:Setr:2026-08-01T04:24:52Z] |
|
Thanks — the cadence finding is correct and it is the important one. Both items are addressed; the branch is pushed. The cadence: option 1, with option 2 filedPut to the operator, who chose the reachable default now and the cross-session counter as follow-up.
The guard you said was missing now exists ( While there I found the literals were load-bearing in a way nobody would have noticed: fourteen call sites wrote the fire index as Cross-session counter filed as #1294, with the three decisions it has to make written down rather than left to discovery: the seed contract ( The minor: confirmed, and it was worse than "docstring or behaviour"You were right, and I reproduced it before fixing: An operator asking for "off" on the highest-precedence tier silently got exploration every 20th turn. I fixed the behaviour rather than narrowing the docs ( Resolver tests added ( Full suite green on the branch. Re-review when you have a moment — and thanks for bounding the fire rate both ways rather than guessing, that is what made the decision straightforward to put to the operator. |
|
[claim:review:Toug:2026-08-01T05:02:43Z] |
robotrocketscience
added
attn:review
|
[release:review:Toug:2026-08-01T05:02:49Z] |
Operator ruling 2026-08-01 — make the fire counter cross-session.
|
|
[release:review:Garsecg:2026-08-01T05:05:58Z] |
|
[claim:review:Garsecg:2026-08-01T05:07:27Z] |
The exploration slot's three library halves all merged without a caller, and one reason is that there was nothing to configure: exploration.py is deliberately free of config and IO imports, so it carries the DEFAULT_ constants and no resolvers. Adds is_exploration_enabled / resolve_exploration_cadence / resolve_exploration_slots here instead, beside every other [retrieval] lane flag and following the same env -> kwarg -> TOML -> default precedence. Default OFF: the slot changes what is injected into a live conversation, and its purpose is not ranking -- 84.1% of the store has never been injected and therefore can never earn evidence, and the slot is the intervention that breaks that loop. Flipping the default is a separate operator call, gated on coverage growth counted from exploration_events rather than on a relevance score. A cadence at or below zero disables exploration rather than raising, matching should_explore's own guard: a misconfigured cadence should degrade to 'never explore', not put a ZeroDivisionError inside a retrieval.
The pool query, the exploration_events ledger and the seeded uniform draw are
all on main and nothing in src/ imports aelfrice.exploration -- a complete,
tested mechanism that cannot reach a pack. This is the missing caller.
Placed after dedup and upstream of both the rebuild log and
_record_injection_events. That position is the point, not an implementation
detail: evidence accrues on exposure, so substituting a never-injected belief
without recording the exposure would leave the loop exactly as closed as it
was.
Three properties are load-bearing, and each was a way for this to be worse
than useless:
- Substitution, never append, accounted in TOKENS rather than slots. A drawn
belief can be longer than the hit it replaces, so a one-for-one swap still
grows the block. It frees at least what it spends from the lowest-ranked
non-locked tail, and skips when the tail cannot fund the draw. A slot that
grew the block would be a budget increase wearing an exploration costume,
and would confound the coverage measurement the slot exists to produce.
- User locks are never displaced. The pool excludes them and the
displacement scan skips them, so an all-locked pack is a no-op rather than
an eviction.
- Fail-soft end to end, including a raising pool query: a research lane must
never be why a hook fails.
Twelve tests, each asserting an outcome that differs from the exploration-off
arm rather than counting hits -- a counting test would pass against the no-op
this fixes, which is how three mechanisms on #1176 shipped inert. Verified by
mutation: deleting the lock skip reddens the lock test, and deleting the token
guard reddens both the too-cheap-pack test and the lock test. The token guard
also caught its own first fixtures, which were too short to fund a draw; that
refusal is now pinned as a property instead of being tuned away.
Adds the exploration_enabled / exploration_cadence / exploration_slots section, with the three contractual properties stated rather than left to be rediscovered from the code: substitution accounted in tokens (not slots, since a drawn belief can be longer than the hit it replaces), locks never displaced, and the exposure recorded upstream of the injection ledger. States what the default flip must be gated on, because getting this wrong is the likely failure: coverage of the never-injected pool over time, counted from exploration_events, not a relevance A/B. It is not a ranking change and scoring it as one would produce a null on an instrument that cannot express what the lane does. Records why the draw is uniform, so the A-Res weighting is not re-proposed: uncertainty_score is Beta differential entropy, which is <= 0 on [0,1], so the reservoir key divides by zero -- and after either sign repair the weighting sits 0.0586 total-variation from uniform.
The documented contract is that a cadence of 0 or less disables exploration. On the env tier -- the highest-precedence one -- it did the opposite: _env_positive_int discards non-positive values, so AELFRICE_EXPLORATION_CADENCE=0 fell through to the default and an operator asking for 'off' got exploration every 20th turn. Adds a reader that keeps a meaningful zero; the shared helper stays in place for resolvers like exploration_slots where a non-positive value really is just invalid.
The suite reached these only through AELFRICE_EXPLORATION, so the env -> kwarg -> TOML -> default chain and the disable contract were asserted by the docs and by nothing else. Each test pins one tier against the tier beneath it, and the zero case asserts the resolved value differs from the default rather than merely being <= 0 -- the weaker form passes against the bug it exists to catch.
…ssion (#1279) fire_idx is per-session -- read_ring_state returns {} on a session mismatch, so the counter restarts each session and never accumulates. Sessions are short: median 2 injecting turns, p90 7. At the specified cadence of 20 the slot reached a firing turn on 8 of 956 turns all-time and 0 of 259 since the #1016-B regime break, so the lane was wired, tested and could never run -- the failure class this issue exists to close. Drops the default to 3. A global 'one turn in twenty' needs a store-level counter and is filed separately.
Fourteen call sites wrote the fire index as a literal 20, so every one of them silently stopped exercising its branch when the default moved, and nothing asserted that a firing turn is reachable from a real session at all. Adds that assertion as a property rather than as an equality on the constant, and fixes a count assertion in the sibling suite that held only because 20 divides 1000 exactly.
7c2b0fb to
1c76517
Compare
Re-review of the pushed branch — both findings closed, verified by mutation. Approving.You pushed four minutes before the operator ruling landed, so the branch answers the finding by a different route than the ruling picked. Working through that below rather than around it. The fixes holdReachability. The one failure is that test, by name. Starting the range at The env Resolver coverage. One thing I changed on your branch
On the ruling, since it says something this branch does notThe operator ruled cross-session counter and explicitly recorded the reachable per-session default as a rejected alternative. That reads as a conflict with what you pushed; I do not think it is one, and here is the reasoning so nobody has to re-derive it:
So: merging this, with the ruling satisfied by #1294 landing before the default is ever flipped on. That ordering is the part worth holding onto — flipping Everything from the first review that I checked and could not fault still stands: store contract matches exactly, ordering safe because Labelling |
robotrocketscience
added
the
ready-to-merge
|
merge-train: blocked 2 review thread(s) are unresolved on these files: src/aelfrice/hook.py, src/aelfrice/retrieval.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
`_substitute_exploration_slots` was the only config-dependent step in `user_prompt_submit` that did not thread `payload_cwd` — the sibling lookups all pass it (`load_user_prompt_submit_config(start=payload_cwd)` and both phantom configs). Its three resolvers therefore walked for `.aelfrice.toml` from the hook process's working directory, so a project that opted in through its own file got the slot only when the hook happened to run from that directory. Test runs the same store, query and ring counter twice differing only in `cwd`; dropping `start=cwd` from any resolver collapses the arms and turns it red.
robotrocketscience
added
the
ready-to-merge
|
merge-train: merged 8c3ccc1 → |
github-actions
Bot
removed
the
ready-to-merge
robotrocketscience
removed
the
attn:review
|
[release:review:Garsecg:2026-08-01T05:19:07Z] |
1 participant
Closes #1279. Parent: #1176 (proposal 5).
What was missing
Two thirds of proposal 5 was already merged and could not reach a pack.
git grep -nE 'aelfrice\.exploration' -- src/onmainreturned nothing:exploration.pywas imported by its own tests and by nothing else insrc/. The pool query, theexploration_eventsledger and the seeded uniform draw were all complete, tested and inert — the category #1162 exists to track. This is the caller, not the mechanism.hook._substitute_exploration_slotssits on theuser_prompt_submitpath between retrieval and render, default OFF behindAELFRICE_EXPLORATION/[retrieval] exploration_enabled, withexploration_cadence(20) andexploration_slots(1).Why the slot exists
Measured on the live 44,586-belief store: 37,489 active unlocked beliefs (84.1%) carry neither a
feedback_historyrow nor aninjection_eventsrow, and only 1,352 (3.0%) have ever been injected at all. Evidence accrues on exposure, so a belief that starts underranked never gets injected, never earns evidence, and stays underranked. The 92,685 feedback rows land on 7,450 beliefs — the feedback is concentrated on the beliefs that were already winning.This is not a ranking change and must not be A/B'd as one. Its outcome is coverage of the never-injected pool over time, countable from
exploration_eventsandinjection_eventswithout a judge or a gold set.Three properties that are load-bearing
Each was a way for this to be worse than useless, and each is pinned by a test that was mutation-verified (delete the guard, the test goes red).
_record_injection_events. Substituting a never-injected belief without recording the exposure would leave the loop exactly as closed as it was, so placement in the call order is the point rather than an implementation detail.Fail-soft end to end: any error in the exploration path — including a raising pool query — leaves the pack exactly as retrieval produced it.
Acceptance
is_exploration_enabled,resolve_exploration_cadence,resolve_exploration_slots). A cadence<= 0disables rather than raising.(session, fire_idx, query, pool)→ same drawn ids.exploration_eventsrow per firing turn, naming the displaced ids and not only the drawn ones.Flipping the default is a separate operator call.
Summary by Sourcery
Wire the exploration slot into UPS retrieval so never-injected beliefs can be substituted into packs under a configurable, fail-soft flag.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes