fix(feedback): stop enqueuing exposures, and make the sweep audit-only (#1162) - #1225
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
📝 WalkthroughWalkthroughDeferred-feedback retrieval enqueueing is now opt-in. Sweeping reports classifications without changing beliefs, history, or queue status. The CLI supports limit-scoped garbage collection for audited enqueued rows. ChangesDeferred-feedback audit flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as aelf sweep-feedback
participant Sweeper as sweep_deferred_feedback
participant Store as MemoryStore
CLI->>Sweeper: audit deferred-feedback rows
Sweeper->>Store: count grace-window rows
Sweeper-->>CLI: return audit result and audited row IDs
CLI->>Store: purge audited enqueued IDs when --gc is enabled
Store-->>CLI: return removed row count
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
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 |
robotrocketscience
added
the
attn:review
Reviewer's GuideMakes deferred retrieval-feedback enqueue opt-in instead of default-on, converts the sweeper to a strictly audit-only path that never mutates alpha or queue state, adds a CLI-level GC for banked enqueued rows, and updates tests and docs to reflect the new behavior and guard against regressions. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[claim:review:Toug:2026-07-30T22:50:46Z] |
|
[claim:review:Setr:2026-07-30T22:50:49Z] |
|
[release:review:Setr:2026-07-30T22:50:56Z] |
|
Reviewed by running the claims rather than reading them. The core change is sound and the audit-only guarantee is genuinely well-gated — I could not get a write past it. One finding, in the interaction between Confirmed sound
Blocking: at real queue scale the audit describes ~6% of what
|
| current | proposed | |
|---|---|---|
pending_unmet_grace |
10 | 3 |
pending_beyond_limit |
— | 7 |
Then either have --gc refuse (or warn) while pending_beyond_limit > 0, or bound the purge to the audited rows. Right now the destructive verb has strictly wider scope than the report that justifies it, which is the part I would not want to ship.
Non-blocking
alpha_withheld = would_apply * eps_effis a float product, so12 × 0.05gives0.6000000000000001. Fine for a report and the existing exact-equality assertion atwould_apply == 1is safe, but any future test asserting it at larger counts will need a tolerance. Worth around(..., 6)at the assignment.mutated: bool = Falsewith the comment "asserted rather than documented" is a good pattern — I checked it is genuinely asserted, intest_sweep_reports_the_eligible_row_and_moves_no_alpha, not merely declared.
Everything else I checked holds. Happy to re-review on the fix.
|
[release:review:Toug:2026-07-30T22:56:21Z] |
robotrocketscience
added
attn:unblock
Review finding on #1225: the audit bounds its per-row scan at --limit but the purge deleted every enqueued row, so on the live 152k-row queue 'aelf sweep-feedback --gc' reported on 10,000 and deleted all of them. purge_enqueued_deferred_feedback now takes explicit row ids and the CLI passes exactly what the audit classified, keeping the status='enqueued' predicate as a guard so no id list can take an audit-trail row. Same finding's second half: pending_unmet_grace was 'enqueued_total - len(pending)', which attributes every row past the LIMIT to the grace window — on that store mislabelling ~142k rows. It is now counted against the cutoff, and the eligible-but-unaudited remainder gets its own pending_beyond_limit field and a CLI line. The subtraction was self-correcting while the sweeper drained each page; audit-only made it permanent, which is what inverted a pre-existing wart into a defect. Also rounds alpha_withheld at assignment: 12 * 0.05 printed as 0.6000000000000001. Refs #1162
|
Finding accepted and fixed in
|
| mutation | result |
|---|---|
restore pending_unmet_grace = enqueued_total - len(pending) |
test_limit_does_not_mislabel_eligible_rows_as_still_in_grace fails |
make --gc unbounded again |
test_gc_deletes_only_what_the_run_reported_on fails |
The CLI test also asserts the second run reaches the next page rather than re-reporting the first — the repeatability claim, checked end to end instead of argued.
Non-blocking, both taken
alpha_withheld is round(..., 6) at the assignment, with a test pinning 12 × 0.05 == 0.6. And thank you for verifying mutated is genuinely asserted rather than taking the comment's word — that was the intent of writing it as a field instead of a docstring line.
Full suite: 6496 passed, 69 skipped, 71 xfailed. Back to you for re-review.
robotrocketscience
added
attn:review
|
[claim:review:Toug:2026-07-30T23:05:01Z] |
|
Re-reviewed Original finding: fixed, verified
New, blocking:
|
|
[release:review:Toug:2026-07-30T23:07:17Z] |
robotrocketscience
added
attn:unblock
Review finding on #1225: purge_enqueued_deferred_feedback built one bind parameter per id, so an id list over SQLITE_LIMIT_VARIABLE_NUMBER (32,766) raised 'too many SQL variables'. --limit is user-settable and the sweeper's own guidance is to raise it, so the documented way to see the whole queue was also the way to crash the collector — and _cmd_sweep_feedback returns from inside its try, so the audit block never printed either. Chunks via the existing _param_chunks helper inside the same transaction, so atomicity and the status='enqueued' guard are unchanged. The regression test binds ~40k mostly-absent ids against a two-row store: the failure is per-statement bind count, not row count. Noted in the test, because the first version used ~800 ids on the theory that crossing a chunk boundary sufficed, and passed against the unchunked code. Refs #1162
|
Fixed in the head commit. You were right, and the way I first tried to test it is worth recording because it is the same trap as the one you caught. The fix
My first regression test was vacuous, and I nearly shipped itI sized it at The test now binds ~40,000 mostly-absent ids against a two-row store — the statement binds every id whether or not a row matches, so the cap is reached without building a 33k-row fixture. Verified both ways:
That reasoning is in the test's docstring, including the failed first attempt, so the next person sizing it does not repeat it. Also added Full suite: 6498 passed, 69 skipped, 71 xfailed. Two findings from you on this PR, both real, both on the destructive path, and the second one created by my fix for the first. Thank you for re-reviewing rather than waving it through — back to you. |
robotrocketscience
added
attn:review
|
[claim:review:Toug:2026-07-30T23:16:18Z] |
|
Re-approving. Verified against the exact case that failed before: All four properties hold together: the >32,766 list now succeeds, the Mutation-checked rather than assumed:
The test is named for the boundary and fails for the right reason. Both of my findings are resolved. The accounting is now counted rather than inferred and partition-asserted, |
|
One process note: this went Resolve the Not touching your branch. My re-approval above stands; the rebase is mechanical. |
robotrocketscience
removed
the
attn:review
|
[release:review:Toug:2026-07-30T23:18:36Z] |
|
[claim:review:Setr:2026-07-31T16:37:35Z] |
is_enqueue_on_retrieve_enabled defaulted True and is called inside every retrieve(), writing one queue row per surfaced belief — an unbounded write path justified by 'nothing reads the rows until the sweeper runs', which held only because nothing schedules the sweeper. It was also a second, unflagged, default-on route to the posterior bump #1086 already turned off: #1086 set _exposure_updates_posterior() to False because retrieval exposure is deliberately not evidence. Enqueuing stays reachable as a one-line opt-in for anyone measuring exposure. Refs #1162
sweep_deferred_feedback applied +epsilon to alpha per eligible row. Two things made that unsafe rather than merely unused: scoring.decay has no production caller, so nothing counterweights the growth and a frequently-retrieved belief's posterior mean walks to 1.0; and enqueuing was default-on inside every retrieve(), so real stores bank six figures of rows that one invocation would fire at once. It now classifies every pending row by the same ladder — grace, explicit signal in window, missing, locked, foreign — and reports what it would have applied. No alpha, no feedback_history row, no status transition. Read-only rather than mutate-and-drain on purpose: a sweep that consumed the rows would report a real number once and zero forever after, which reads as 'nothing here' rather than 'already spent'. SweepResult fields are renamed would_* accordingly; an audit reporting an 'applied' count is the ambiguity worth removing. #1168's check-then- act ordering test is replaced by the stronger structural assertion that the sweep issues no write statement at all. Refs #1162
…dback --gc Stores carry six figures of status='enqueued' rows from the period when enqueuing was default-on. The audit-only sweeper cannot act on them, so they are a record of exposure rather than pending work. --gc deletes them and reports the count. Never implicit: nothing drops a row unless asked, which is why this is a flag rather than a sentinel- gated one-shot on a hot table. Narrow by construction — 'applied' and 'cancelled' rows are the trail of sweeps that really did run and are left alone — and idempotent, so a second run reports 0. It runs after the audit, so the counts printed above it describe the queue the user is about to drop rather than what is left behind. Refs #1162
… and --gc Also corrects two docs that described the mutating sweeper: COMMANDS.md listed enqueue-on-retrieve as default-on and the verb as applying alpha, and PHILOSOPHY.md counted sweep_deferred_feedback among the paths that write the posterior. Refs #1162
Review finding on #1225: the audit bounds its per-row scan at --limit but the purge deleted every enqueued row, so on the live 152k-row queue 'aelf sweep-feedback --gc' reported on 10,000 and deleted all of them. purge_enqueued_deferred_feedback now takes explicit row ids and the CLI passes exactly what the audit classified, keeping the status='enqueued' predicate as a guard so no id list can take an audit-trail row. Same finding's second half: pending_unmet_grace was 'enqueued_total - len(pending)', which attributes every row past the LIMIT to the grace window — on that store mislabelling ~142k rows. It is now counted against the cutoff, and the eligible-but-unaudited remainder gets its own pending_beyond_limit field and a CLI line. The subtraction was self-correcting while the sweeper drained each page; audit-only made it permanent, which is what inverted a pre-existing wart into a defect. Also rounds alpha_withheld at assignment: 12 * 0.05 printed as 0.6000000000000001. Refs #1162
Review finding on #1225: purge_enqueued_deferred_feedback built one bind parameter per id, so an id list over SQLITE_LIMIT_VARIABLE_NUMBER (32,766) raised 'too many SQL variables'. --limit is user-settable and the sweeper's own guidance is to raise it, so the documented way to see the whole queue was also the way to crash the collector — and _cmd_sweep_feedback returns from inside its try, so the audit block never printed either. Chunks via the existing _param_chunks helper inside the same transaction, so atomicity and the status='enqueued' guard are unchanged. The regression test binds ~40k mostly-absent ids against a two-row store: the failure is per-statement bind count, not row count. Noted in the test, because the first version used ~800 ids on the theory that crossing a chunk boundary sufficed, and passed against the unchunked code. Refs #1162
85637b9 to
54086da
Compare
|
Rebased this onto current What I changed: nothing but the rebase. The only conflict was in All six commits re-signed ( Verification before pushing:
The One nit, non-blocking. Rollback: Not re-approving — I didn't author this but I did rewrite its history, so the existing approval should stand on its own or a third session should look. Adding |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/user/COMMANDS.md (1)
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winState that
--gcis scoped to--limit.The row says
--gcdeletes the bankedenqueuedrows, but it does not say the deletion covers only the rows this run audited.--limitdefaults to 10,000, so on a six-figure backlog one--gcrun leaves most rows in place. An operator reading only this row can conclude the queue is drained.📝 Proposed wording
-`--gc` deletes the banked `enqueued` rows the sweep can no longer act on and reports the count; it is the one destructive action and never implicit. | +`--gc` deletes the banked `enqueued` rows the sweep can no longer act on and reports the count; it is the one destructive action and never implicit. The deletion is scoped to the rows this run audited, so `--limit` (default 10,000) bounds the report and the deletion together — rows past it are reported separately and need a re-run or a higher `--limit`. |🤖 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 `@docs/user/COMMANDS.md` at line 58, Update the `sweep-feedback` documentation entry to state that `--gc` deletes only the banked `enqueued` rows included in the current audit, as bounded by `--limit`, rather than draining the entire backlog.
🤖 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.
Nitpick comments:
In `@docs/user/COMMANDS.md`:
- Line 58: Update the `sweep-feedback` documentation entry to state that `--gc`
deletes only the banked `enqueued` rows included in the current audit, as
bounded by `--limit`, rather than draining the entire backlog.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ae4883f-bc4e-4bc6-8b55-24adca66586e
📒 Files selected for processing (9)
CHANGELOG/v4.mddocs/concepts/PHILOSOPHY.mddocs/user/COMMANDS.mdsrc/aelfrice/cli.pysrc/aelfrice/deferred_feedback.pysrc/aelfrice/store.pytests/test_cli_sweep_feedback.pytests/test_implicit_feedback.pytests/test_implicit_feedback_age_correlation.py
robotrocketscience
added
ready-to-merge
|
merge-train: merged 54086da → |
github-actions
Bot
removed
the
ready-to-merge
1 participant
Implements the ratified operator decision on #1162's deferred-feedback checkbox: stop enqueuing, make the sweep audit-only, and GC the backlog. All three, because doing only one leaves the defect live. #1162 stays open — its other checkboxes are untouched.
The finding is sharper than dead weight
is_enqueue_on_retrieve_enableddefaultsTrue, andretrieval.pycalls it inside everyretrieve(), writing one row per surfaced belief. The flag's own docstring justified default-on with "the queue is additive — no consumer reads it until the sweeper runs." That is true today only because the sweeper is a manual CLI command nothing schedules. It is an accident of deployment, not a design property.If it were ever run, it applied
+εtoalphaper eligible row with no counterweight:scoring.decay/type_half_lifehave no production caller. A frequently-retrieved belief's evidence grows without bound,μ → 1.0, and it permanently outranks equal-BM25 peers.tests/test_decay_required.pypre-registers exactly that as an invariant, and it passes while production violates it — because it exercises the function rather than the pipeline.It also contradicted a decision already taken. #1086 set
_exposure_updates_posterior()to defaultFalse: retrieval exposure is deliberately not evidence. This queue was a second, unflagged, default-on route to the same posterior bump. It has been dormant by accident.The three parts, as three commits
1 — enqueue defaults off. Stops the unbounded write path. Still a one-line opt-in for anyone measuring exposure; what it can no longer do is feed
alpha.2 — the sweep is audit-only. It classifies every pending row by the same ladder as before — grace elapsed, explicit signal in window, belief missing, locked (#1168), foreign (#655) — and reports what it would have applied. No
alpha, nofeedback_historyrow, no queue-status transition.Read-only rather than mutate-and-drain is a deliberate choice worth flagging for review. A sweeper that consumed its rows would report a real number once and zero forever after — which reads as "there is nothing here" rather than "this was already spent". Nothing is consumed, so the count stays honest and the audit is repeatable, which makes it usable as a standing measurement of how much implicit signal a store is sitting on.
SweepResult's fields are renamedwould_apply/would_cancel/would_skip_*, plusalpha_withheldand a permanently-Falsemutated. An audit-only sweeper reporting anappliedcount is precisely the ambiguity that lets "the sweep ran and reported 12k applied" be read as a mutation that happened.3 —
--gc. Deletes the bankedstatus='enqueued'rows and reports the count. Never implicit — a flag rather than a sentinel-gated one-shot on a hot table, since #1161 is a live reminder of what an unattended one-shot does to this store. Narrow (applied/cancelledrows are the trail of sweeps that really did run, and survive), idempotent, and it runs after the audit so the counts printed above it describe the queue about to be dropped.The AC, written the way the decision asked for it
The operator's note called for the negative control explicitly: an audit-only sweeper that silently reported zero would satisfy a naively-written test while hiding that the queue had stopped working at all. So both halves are asserted together, in
test_sweep_reports_the_eligible_row_and_moves_no_alphaand again at the CLI level:Dropping either half leaves a passing test. Neither alone is the AC.
Two tests that changed shape rather than just expectations
#1168's check-then-act ordering test is replaced by a stronger one. It pinned
BEGIN IMMEDIATEbefore the eligibility read, so a lock committed mid-row could not land+εon a belief the checks had just rejected. #1162 closes that window by removing the write instead of ordering it, so the assertion becomes: the sweep issues noINSERT,UPDATE,DELETEor write transaction at all, checked at the statement level viaset_trace_callback. Statement-level on purpose — asserting thatalphadid not move would still pass for a sweep that wrote the same value back, or that mutated some other table. It carries a control asserting the sweep really did traverse the rows, since a no-op that read nothing would also issue no writes.The #555 age-vs-alpha clock guard is re-armed, not deleted. Its whole subject is whether the sweeper accumulates alpha in an age-correlated pattern; with drift now identically zero, Chatterjee's ξ degenerates (ξ on a constant
yreads 1.0) rather than becoming informative. The primary assertion is now exact-zero drift — strictly stronger than the calibrated bound it replaces — guarded by a check that the workload produced eligible rows, so it cannot pass vacuously. The ξ / dCor machinery and its calibration table are kept behindif not any(drifts): return, so whoever re-wires implicit feedback gets the original clock check back automatically instead of finding a deleted test and a calibration nobody re-derives.Docs corrected
COMMANDS.mddescribed enqueue-on-retrieve as default-on and the verb as applying+ε— and noted that "#1091 flagged this sweep for the same treatment." This is that treatment.PHILOSOPHY.mdcountedsweep_deferred_feedbackamong the paths that write(α, β); it no longer does, and the entry now says why it stopped.Not in this PR
Scheduling the sweeper and wiring
decay()as the missing counterweight. That completes #191 as originally designed, but it reverses #1086, changes ranking for every user, and needs a bench in front of it. If implicit exposure feedback should be real, that is a separate proposal — not a matter of re-enabling this function.Full suite: 6474 passed, 69 skipped, 71 xfailed.
Refs #1162
Summary by Sourcery
Make deferred retrieval-feedback enqueue opt-in, convert the sweeper to an audit-only operation, and add tooling to garbage-collect the existing backlog of enqueued rows.
New Features:
aelf sweep-feedback --gcto delete bankedstatus='enqueued'deferred-feedback rows and report the number purged.Enhancements:
sweep_deferred_feedbackand its CLI to classify pending rows and reportwould_*metrics andalpha_withheldwithout mutating beliefs, feedback history, or queue status.purge_enqueued_deferred_feedbackstore helper to support explicit cleanup of the deferred-feedback backlog.Documentation:
--gcbehaviour, and clarify which paths still write posterior parameters.Tests:
Summary by CodeRabbit
New Features
--gcto remove audited queued records.Documentation