feat: deferred-feedback sweeper — implicit retrieval-driven posterior signal (#191) - #256
Conversation
robotrocketscience
added
review-Gylf
|
Review: code looks solid — schema additive, transactional per-row writes with BEGIN IMMEDIATE, fail-soft retrieval hook, comprehensive tests, all required checks green. Blocker: GitHub reports Two minor nits worth a look while you're rebasing — neither is a merge blocker:
Dropping |
|
Review: blocked — needs rebase before this is reviewable. This branch is
These aren't intentional reverts — the diff is showing the gap because the branch was forked from a much older Action requested: rebase onto current The deferred-feedback work itself looks substantial ( Dropping |
|
[claim:review:Kulili:2026-04-29T01:30:01Z] |
|
[release:review:Kulili:2026-04-29T01:31:07Z] |
Disposition: rebase needed; no current ownerCI is green on this branch's last push (all checks passing as of 2026-04-28 23:10), but This PR has been open ~24h with no follow-up after the conflict appeared. Either:
I'm not picking up authorship here — the cleanest path is for whoever pushed the original 6 commits to drive the rebase, since they hold the design context for the grace-window / retrieval-exposure schema choices that won't survive a blind merge. Flagging only. Underlying issue #191 remains open and the substrate-cascade ratification (2026-04-29) kept the v2.0 ship intent intact, so the work itself isn't blocked at the spec level. |
|
[claim:review:Setr:2026-04-29T02:13:46Z] |
|
[release:review:Setr:2026-04-29T02:14:01Z] |
|
[claim:review:Kulili:2026-04-29T02:21:32Z] |
|
[release:review:Kulili:2026-04-29T02:21:48Z] |
## Summary Single-doc index for the open-issue tree. Sorts everything in `gh issue list --state open` into five waves with explicit deps, soft-deps, and cross-cutting hazards. Goal: prevent the rework patterns that already bit us (#293's tests dying on #283's UNIQUE constraint, #256 stuck on author rebase after multiple PRs landed on `store.py`, etc.). Five waves: - **Wave 0** — close-out. Seven stale items that should walk to zero before new work starts (#223, #254, #281 re-scope, #286 → tracker, #287 dup, #280, #288 implementation tail). - **Wave 1** — rebuild redesign (#288, #289, #290, #291). All four spec memos in flight; ratify all four before opening any implementation PR. Implementations must sequence (`context_rebuilder.py` is a merge-conflict spot). - **Wave 2** — phantom-prereqs T1→T2→T3 (#191/#256 stuck on rebase; #192 blocks on T2). - **Wave 3** — v2.0 substrate decision tree. #196 gates everything posterior-related. Bench-gated items (#197/#198/#199/#201/#229) wait on #288's harness producing precision/recall numbers. - **Wave 4** — v2.x materialization (#262 → #264 → #265). Sequential, must not parallelise. - **Wave 5** — long-tail retrieval / research (#154, #153). Hold; #154 refactor would rework #289–#291. Plus five cross-cutting hazards on the wall: `context_rebuilder.py` and `store.py` as merge-conflict spots; the #283 UNIQUE constraint test pattern; calibration-data bench-gate; substrate ratification before #290 implementation. ## Decision asks Bottom of the doc — operator stamping queue (5 items) that unblocks Wave 1 implementation in sequence. ## Test plan - [x] All open issues from `gh issue list` accounted for - [x] All in-flight PRs cross-referenced in the spec / impl index tables - [x] Discretion grep clean - [ ] Operator review: confirm wave assignments + hazards list
|
[claim:review:Setr:2026-04-29T03:08:50Z] |
|
Branch is 21+ commits behind Review: not yet — claim released. |
|
[release:review:Setr:2026-04-29T03:09:50Z] |
|
[claim:review:Setr:2026-04-29T04:53:56Z] |
|
Review pass: code + tests + discretion clean, all checks green, content LGTM. Blocked on merge conflict against main — needs rebase before squash-merge. Releasing review claim. |
|
[release:review:Setr:2026-04-29T04:55:05Z] |
|
[claim:review:Kulili:2026-04-29T04:59:00Z] |
The sweeper module that processes deferred_feedback_queue rows whose grace window has elapsed. Cancellation gate queries feedback_history for any non-retrieval source within the row's [enqueued_at, now] window — catches both explicit user feedback and contradiction-tiebreaker resolutions in one shot. Per-row writes share an explicit BEGIN IMMEDIATE / COMMIT transaction so a crash mid-row leaves the queue row 'enqueued' and the alpha unchanged. Re-running the sweeper is a no-op for already-applied / already-cancelled rows. Config: T_grace and epsilon are env > kwarg > .aelfrice.toml > default (1800 s, 0.05). enqueue_on_retrieve gating defaults to True since the queue is additive — no consumer reads from it until the sweeper is invoked. Retrieval-side wiring is the next commit; CLI subcommand follows after that.
…#191) Wires retrieve()'s output into deferred_feedback_queue. Deferred import keeps the retrieval module from depending on the sweeper at import time. Default-on gate respects the implicit_feedback config section; opt-out is one TOML line. Fail-soft: any DB exception during enqueue is logged to stderr and swallowed — retrieve() must never fail because of an additive side-effect. The 30-test retrieval suite passes byte-for-byte; the hook is purely additive on the return path.
Drives sweep_deferred_feedback from the CLI. Defaults are cron-safe: exits 0 even on internal exceptions (logged to stderr) so a wedged DB doesn't break a scheduled job. --strict flips that for operators who want failure visibility. Auto-discovered by _known_cli_subcommands() (no registry to update). Flags expose the standard env > kwarg > TOML > default precedence the rest of aelfrice uses.
33 tests across two files: tests/test_implicit_feedback.py — schema sanity, retrieve() enqueue (with env-off + empty-query + fail-soft variants), apply path, grace-window skip, cancellation by explicit feedback or contradiction-tiebreaker, audit source distinctness, idempotency (sweep x 2 = sweep x 1), partial-progress resume via --limit, config resolution (env / kwarg / TOML / default for grace + epsilon), propagate=False on locked neighbours, FK-cascade on belief delete. tests/test_cli_sweep_feedback.py — subcommand registered, empty-queue exit, end-to-end apply with --grace-seconds 0, --strict flag flips error exit code. Add sweep-feedback to HIDDEN_SUBCOMMANDS in test_slash_commands.py (scripting / hook entry point; no user-facing slash file).
aad2c47 to
021241a
Compare
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 selected for processing (7)
✨ 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. Review rate limit: 0/1 reviews remaining, refill in 42 minutes and 5 seconds.Comment |
|
[release:review:Kulili:2026-04-29T16:13:19Z] |
## Summary - New workflow `.github/workflows/auto-rebase-open-prs.yml` fires on push to `main` and on `workflow_dispatch`. - For each open PR with `base=main`, fetches the head branch, attempts `git rebase origin/main`, force-pushes if clean. - Conflicts get a `merge-conflict` label and a comment with the local-recovery one-liner. ## Why Three concurrent sessions opening PRs against main means every merge invalidates ~2 in-flight PRs. Reviewer hits "BLOCKED — needs rebase" and bounces. This eliminates the manual rebase step for the common (clean) case and makes real conflicts surface as a flag. ## Resolves Refs #256 review thrash (5+ claim/release cycles before merge). ## Test plan - [ ] Merge this PR. Watch the next merge-to-main; verify any other open PRs auto-rebase. - [ ] Land a deliberately conflicting branch; verify it gets the `merge-conflict` label + comment. ## Summary by Sourcery CI: - Add a GitHub Actions workflow that rebases all open PRs targeting main onto the updated main branch, force-pushes clean rebases, and labels/comment flags PRs with merge conflicts. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Adds automated rebasing of open PRs onto main to keep branches up to date; skips PRs already current. * When rebasing succeeds, updates branches automatically; when conflicts occur, labels PRs as "merge-conflict" and posts guidance on how to resolve and update the PR. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
sweep_deferred_feedback writes alpha directly rather than going through apply_feedback. That stays — it owns its own per-row BEGIN IMMEDIATE and its queue-status bookkeeping — but it was also skipping the two invariants that endpoint enforces. A retrieval-driven +epsilon landed on user locks, which LIMITATIONS.md and PRIVACY.md both promise cannot happen, and on federated beliefs that are read-only through the local DB (#655). Both cases now drain the queue row instead of applying it, so an ineligible belief cannot accumulate a backlog that all lands at once if it later becomes eligible. New skipped_locked / skipped_foreign counters on SweepResult report it. Scope note: this does not gate the sweeper on the #1086 exposure-updates-posterior flag. The implicit lane predates #1086 (#191/#256) and is a materially different mechanism — a much smaller epsilon, behind a grace window that any explicit correction cancels — not the immediate exposure-as-endorsement bump #1086 removed. Whether the lane should exist at all is an operator call, not a bug fix.
sweep_deferred_feedback writes alpha directly rather than going through apply_feedback. That stays — it owns its own per-row BEGIN IMMEDIATE and its queue-status bookkeeping — but it was also skipping the two invariants that endpoint enforces. A retrieval-driven +epsilon landed on user locks, which LIMITATIONS.md and PRIVACY.md both promise cannot happen, and on federated beliefs that are read-only through the local DB (#655). Both cases now drain the queue row instead of applying it, so an ineligible belief cannot accumulate a backlog that all lands at once if it later becomes eligible. New skipped_locked / skipped_foreign counters on SweepResult report it. Scope note: this does not gate the sweeper on the #1086 exposure-updates-posterior flag. The implicit lane predates #1086 (#191/#256) and is a materially different mechanism — a much smaller epsilon, behind a grace window that any explicit correction cancels — not the immediate exposure-as-endorsement bump #1086 removed. Whether the lane should exist at all is an operator call, not a bug fix.
sweep_deferred_feedback writes alpha directly rather than going through apply_feedback. That stays — it owns its own per-row BEGIN IMMEDIATE and its queue-status bookkeeping — but it was also skipping the two invariants that endpoint enforces. A retrieval-driven +epsilon landed on user locks, which LIMITATIONS.md and PRIVACY.md both promise cannot happen, and on federated beliefs that are read-only through the local DB (#655). Both cases now drain the queue row instead of applying it, so an ineligible belief cannot accumulate a backlog that all lands at once if it later becomes eligible. New skipped_locked / skipped_foreign counters on SweepResult report it. Scope note: this does not gate the sweeper on the #1086 exposure-updates-posterior flag. The implicit lane predates #1086 (#191/#256) and is a materially different mechanism — a much smaller epsilon, behind a grace window that any explicit correction cancels — not the immediate exposure-as-endorsement bump #1086 removed. Whether the lane should exist at all is an operator call, not a bug fix.
sweep_deferred_feedback writes alpha directly rather than going through apply_feedback. That stays — it owns its own per-row BEGIN IMMEDIATE and its queue-status bookkeeping — but it was also skipping the two invariants that endpoint enforces. A retrieval-driven +epsilon landed on user locks, which LIMITATIONS.md and PRIVACY.md both promise cannot happen, and on federated beliefs that are read-only through the local DB (#655). Both cases now drain the queue row instead of applying it, so an ineligible belief cannot accumulate a backlog that all lands at once if it later becomes eligible. New skipped_locked / skipped_foreign counters on SweepResult report it. Scope note: this does not gate the sweeper on the #1086 exposure-updates-posterior flag. The implicit lane predates #1086 (#191/#256) and is a materially different mechanism — a much smaller epsilon, behind a grace window that any explicit correction cancels — not the immediate exposure-as-endorsement bump #1086 removed. Whether the lane should exist at all is an operator call, not a bug fix.
…ail-soft Three corrections carried from review, all re-derived against main first. The `enqueue_on_retrieve` key was marked `(v4.x+)` three lines below a section header reading `(v1.x+)`. `ENQUEUE_KEY` and `is_enqueue_on_retrieve_enabled` both land in 7bd5400 (#191/#256), first tagged v1.6.0; #1162 changed only the default. A reader on v2 or v3 would conclude the key does not exist in their build, when theirs is exactly the store that has been banking rows under the old default-true. The fail-soft sentence claimed all three tiers emit an `implicit_feedback: ignoring ...` trace. Only epsilon and grace_window_seconds do. `is_enqueue_on_retrieve_enabled` tests membership in the truthy/falsy sets and falls through with no diagnostic, so `...ENQUEUE=enabled` and `=y` resolve false in silence while the doc promised a warning that never comes. `utterance_prior_weight` has the same entry-point asymmetry already spelled out for `use_fan_effect`: the kwarg tier is retrieve_v2 / retrieve_with_tiers only, and `retrieve()` raises TypeError. Confirmed by calling it.
…ail-soft Three corrections carried from review, all re-derived against main first. The `enqueue_on_retrieve` key was marked `(v4.x+)` three lines below a section header reading `(v1.x+)`. `ENQUEUE_KEY` and `is_enqueue_on_retrieve_enabled` both land in 7bd5400 (#191/#256), first tagged v1.6.0; #1162 changed only the default. A reader on v2 or v3 would conclude the key does not exist in their build, when theirs is exactly the store that has been banking rows under the old default-true. The fail-soft sentence claimed all three tiers emit an `implicit_feedback: ignoring ...` trace. Only epsilon and grace_window_seconds do. `is_enqueue_on_retrieve_enabled` tests membership in the truthy/falsy sets and falls through with no diagnostic, so `...ENQUEUE=enabled` and `=y` resolve false in silence while the doc promised a warning that never comes. `utterance_prior_weight` has the same entry-point asymmetry already spelled out for `use_fan_effect`: the kwarg tier is retrieve_v2 / retrieve_with_tiers only, and `retrieve()` raises TypeError. Confirmed by calling it.
…ail-soft Three corrections carried from review, all re-derived against main first. The `enqueue_on_retrieve` key was marked `(v4.x+)` three lines below a section header reading `(v1.x+)`. `ENQUEUE_KEY` and `is_enqueue_on_retrieve_enabled` both land in 7bd5400 (#191/#256), first tagged v1.6.0; #1162 changed only the default. A reader on v2 or v3 would conclude the key does not exist in their build, when theirs is exactly the store that has been banking rows under the old default-true. The fail-soft sentence claimed all three tiers emit an `implicit_feedback: ignoring ...` trace. Only epsilon and grace_window_seconds do. `is_enqueue_on_retrieve_enabled` tests membership in the truthy/falsy sets and falls through with no diagnostic, so `...ENQUEUE=enabled` and `=y` resolve false in silence while the doc promised a warning that never comes. `utterance_prior_weight` has the same entry-point asymmetry already spelled out for `use_fan_effect`: the kwarg tier is retrieve_v2 / retrieve_with_tiers only, and `retrieve()` raises TypeError. Confirmed by calling it.
The header said v1.x+ while the key three lines below now says v1.6.0+. Both describe the same commit -- IMPLICIT_FEEDBACK_SECTION and ENQUEUE_KEY arrive together in 7bd5400 (#191/#256), and `git tag --contains` puts its earliest release at v1.6.0 -- so the vaguer of the two markers is just less useful.
The header said v1.x+ while the key three lines below now says v1.6.0+. Both describe the same commit -- IMPLICIT_FEEDBACK_SECTION and ENQUEUE_KEY arrive together in 7bd5400 (#191/#256), and `git tag --contains` puts its earliest release at v1.6.0 -- so the vaguer of the two markers is just less useful.
2 participants
Closes #191. Track 2 of the phantom-prereqs campaign (#189).
Today
apply_feedbackis invoked rarely — anything downstream that wants to use feedback as a posterior-update signal is starved. Meanwhileretrieve()fires constantly. This PR turns retrieval exposure into a small, deferred posterior signal: every surfaced belief enqueues aretrieval_exposurerow; a CLI sweeper applies+epsilonto its alpha after the grace window elapses, unless an explicit correction or contradiction event landed on the same belief in that window.Hard depends on #190 (T1 belief_corroborations) — shipped today. T3 (#192) lands after.
What lands
store.py): additivedeferred_feedback_queue(id, belief_id, enqueued_at, event_type, applied_at, status)with FK to beliefs (ON DELETE CASCADE) + indexes on(status, enqueued_at)andbelief_id.store.py):enqueue_deferred_feedback,list_pending_deferred_feedback,has_explicit_feedback_in_window,count_deferred_feedback_by_status.deferred_feedback.py):enqueue_retrieval_exposures+sweep_deferred_feedback. Per-row processing wraps the alpha update + audit insert + status update in a single explicitBEGIN IMMEDIATE / COMMITtransaction, so a crash mid-row leaves the queue rowenqueuedand the alpha unchanged.retrieval.py): post-retrieve()enqueues one row per surfaced belief. Default-on; opt-out via[implicit_feedback] enqueue_on_retrieve = false. Fail-soft — any DB error is logged to stderr but never breaks retrieval.cli.py):aelf sweep-feedback [--grace-seconds N] [--epsilon F] [--limit N] [--strict]. Cron-safe by default (exits 0 on internal exceptions);--strictflips that.T_graceandepsilonresolve env > kwarg >.aelfrice.toml [implicit_feedback]> defaults (1800 s, 0.05). Same pattern as the rest of aelfrice.Cancellation contract
A pending row is cancelled (no alpha change) if any
feedback_historyrow exists for the same belief whosesourceis notretrieval_driven_feedbackand whosecreated_atis in[enqueued_at, now]. This single check covers both contracts the spec calls out:feedback_historyrows with a distinctivecontradiction_tiebreaker:source prefix).Acceptance
deferred_feedback_queuetable; additive migration.retrieve()post-hook enqueues one row per surfaced belief withevent_type='retrieval_exposure'.aelf sweep-feedbacksubcommand processes the queue with configurableT_grace+epsilon.+epsilonexactly once per row in the no-contradiction path; transitions tostatus='applied'.status='cancelled'.feedback_historyrecordsretrieval_driven_feedbackdistinguishably from user-driven sources.enqueued; transactional row writes).T_graceandepsilonconfigurable via env / kwarg / TOML; defaults documented in the module docstring.Tests
tests/test_implicit_feedback.py— 29 tests across schema, retrieve enqueue (env-off, empty-query, fail-soft), apply/cancel paths, audit distinctness, idempotency, partial-progress resume via--limit, config resolution for both knobs, FK-cascade on belief delete, and propagate=False on locked neighbours (implicit signals must not pressure user-locked contradictors).tests/test_cli_sweep_feedback.py— 4 tests: subcommand registered, empty-queue exit, end-to-end apply with--grace-seconds 0,--strictflag flips error exit code.Out of scope
epsilonfrom corpus measurement — defaults ship; re-tune is a separate evaluation issue.