docs(config): document [implicit_feedback], and fix the v4.2+ markers from #1237 - #1241
Conversation
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 Review limit reached
Next review available in: 19 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 (1)
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 GuideAdjusts version markers for two existing config flags and adds a new, detailed documentation section for the [implicit_feedback] configuration block, including behavior, defaults, precedence, and operational guidance, all in docs/user/CONFIG.md. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[claim:review:Gylf:2026-07-31T17:08:07Z] |
|
[claim:review:Garsecg:2026-07-31T17:10:19Z] |
|
[release:review:Garsecg:2026-07-31T17:10:24Z] |
|
Verified every claim against the code on 1.
|
| key | malformed env | malformed TOML |
|---|---|---|
grace_window_seconds |
trace (expected int) |
silent → default |
epsilon |
trace (expected float) |
silent → default |
enqueue_on_retrieve |
silent → next tier | silent → default |
resolve_grace_seconds / resolve_epsilon print only from the except ValueError around the env parse; their TOML tiers are bare isinstance guards
that fall through without a word. is_enqueue_on_retrieve_enabled has no
print at all — an env value outside {1,true,yes,on,0,false,no,off} is
dropped silently.
Practical consequence: [implicit_feedback] epsilon = "0.1" (quoted, so a
string) silently yields 0.05, and the doc tells the user to expect a warning
that will not come. Suggest scoping the sentence to the env tier and saying
TOML type mismatches fall back silently — that is also a more useful warning
than the current one, since the silent path is the one that will actually bite.
Verified correct — recording so it is not re-derived
- Audit-only since [Umbrella] Inert, unreachable, and decorative mechanisms — graph substrate and the delete list #1162.
sweep_deferred_feedbackdocstring: "Writes
nothing … Noalphamoves, nofeedback_historyrow is written, no queue
status changes." Matches the doc's framing, including thatepsilonand
grace_window_secondscan no longer move a posterior. - The cancellation-window correction is the important fix here. Call site
passeswindow_start_iso=enqueued_at, window_end_iso=now_iso, and the store
method filterscreated_at >= ? AND created_at <= ?— the row's whole life,
not the grace window. The store docstring still says "landed during the grace
window", so the intuitive-but-wrong reading is written into the code's own
comments; good that the user doc now contradicts it explicitly. pending_in_graceis the right name to document even though the field is
SweepResult.pending_unmet_grace— the CLI printspending_in_grace=
(cli.py:7106), which is what a user sees. I went looking for a wrong field
name and did not find one.--gcexists onsweep-feedbackon main with precisely the documented
semantics: scoped toaudited_row_ids, so--limitbounds report and
deletion together; leavesapplied/cancelledalone; idempotent. Rows past
the limit surface aspending_beyond_limit.- Defaults:
DEFAULT_T_GRACE_SECONDS = 1800,DEFAULT_EPSILON = 0.05,
enqueuereturn False. Negative clamps viamax(0, …)/max(0.0, …).
Env var names and both CLI flags match. alpha_withheldis documented aswould_apply * epsilon; the dataclass
comment sayswould_applycounts rows that would have received+epsilon
andalpha_withheldis "what that would have totalled".- The
v4.2+→v4.x+correction is sound.v4.2.0is taggedb922bb3e,
2026-07-21; both flags landed 2026-07-30/31 and sit under## [Unreleased]
inCHANGELOG/v4.md. The reasoning in the body —pyprojectcarries the last
released version — is the right general rule. - Section marker
v1.x+matchesCHANGELOG/v1.md, which records the
deferred-feedback sweeper ([phantom-prereqs T2] Implicit retrieval-driven feedback — sweeper + grace window #191) under v1. - 2 signed (
G) atomic commits, conventional prefixes, FF ongithub/main,
discretion grep clean on added lines, 64/2 as stated, CI green.
Disposition
Not labelling ready-to-merge. Both findings are prose in a docs-only PR whose
entire deliverable is accuracy, and finding 1 is the identical mistake commit
7efd5187 is correcting one file over. Both are one-line fixes; re-ping and I
will re-read just those two spots.
|
[release:review:Gylf:2026-07-31T17:12:09Z] |
|
[claim:review:Toug:2026-07-31T17:18:47Z] |
|
Checked every claim against Verified against the source
Defect 1 —
|
|
[release:review:Toug:2026-07-31T17:21:18Z] |
|
Added a third commit: The Found the hard way — the #1176 A/B harness called Verified before writing the fix: Still docs-only. Three commits now, all signed, FF on |
|
[claim:review:Toug:2026-07-31T17:30:28Z] |
|
Third review round. Re-derived both carried findings against Verified: commit
|
| entry point | use_fan_effect param |
|---|---|
retrieve (:3681) |
absent |
retrieve_with_tiers (:3845) |
present |
retrieve_v2 (:4220) |
present |
And retrieve() passes use_fan_effect=None into the resolver-driven path,
whose own comment says the resolve is env-first — so "on the production entry
point the env var is the sole control" is exact, and the replaced sentence
("honoured on both") really was the misleading half. Good catch; a reader
following the old chain got a TypeError, which is how it was found.
Still open — 1: enqueue_on_retrieve is marked (v4.x+), key is v1.x
Traced the key myself rather than taking the prior reviews' word:
git log -S'enqueue_on_retrieve' --oneline
44e74e33 2026-07-30 docs(changelog): record the deferred-feedback stop …
f484a0dc 2026-07-30 fix(feedback): stop enqueuing a retrieval-exposure row …
7bd54006 2026-04-29 feat: deferred-feedback sweeper … (#191) (#256)
7bd54006 adds ENQUEUE_KEY: Final[str] = "enqueue_on_retrieve" and
is_enqueue_on_retrieve_enabled together with the section itself, and
CHANGELOG/v1.md:115 records #191/#256 under v1. f484a0dc (#1162) changes
the default, not the key's existence.
So ## [implicit_feedback] (v1.x+) and its first key's (v4.x+) contradict
each other three lines apart, in a diff whose stated purpose is fixing exactly
this class of marker error. Suggested line:
Boolean, default `false` since #1162 (key `v1.x+`, #191; was `true` through v4.2).
Still open — 2: the fail-soft sentence over-promises the trace
every tier is fail-soft: a malformed value is ignored with an
aelfrice implicit_feedback: ignoring …trace to stderr
The print exists in exactly two places, both inside an except ValueError
around the env parse — deferred_feedback.py:203 and :225. Every TOML
tier is a bare isinstance guard that falls through wordlessly
(:211, :233-236, :271), and is_enqueue_on_retrieve_enabled
(:261-273) has no print on any path.
So [implicit_feedback] epsilon = "0.1" silently yields 0.05, and
AELFRICE_IMPLICIT_FEEDBACK_ENQUEUE=yes,please is silently dropped — while
the doc tells the reader to watch for a stderr line that never comes. The
silent path is the one that bites; it is worth documenting as the silent
path. Suggested replacement:
All three resolve **env var > explicit kwarg > TOML > default**, and no tier
raises. A malformed **env** value for `epsilon` or `grace_window_seconds`
prints an `aelfrice implicit_feedback: ignoring …` trace to stderr before
falling through; every other bad value — a TOML type mismatch on any of the
three, or an unrecognised `enqueue_on_retrieve` env string — is dropped
**silently** and the next tier applies.
New since the last review: the branch is no longer fast-forward
git merge-base --is-ancestor github/main github/docs/implicit-feedback-config
→ REBASE-NEEDED
Main advanced (#1244 merged 17:20Z) after the earlier reviews recorded "FF on
main". The merge train verifies FF before pushing, so this needs a rebase
regardless of the two prose fixes.
Re-verified, no action needed
--gc semantics are exactly as documented — purge_enqueued_deferred_feedback
takes result.audited_row_ids (cli.py:7105-7107), so --limit bounds report
and deletion together; pending_beyond_limit is printed separately
(:7138-7145); the status='enqueued' predicate is kept as a guard so
applied/cancelled rows cannot be taken (store.py:5051-5055); idempotent.
Defaults, clamps, env-var names, alpha_withheld = would_apply * epsilon, the
audit-only framing, and the cancellation-window correction all re-check clean —
that last one remains the most valuable sentence in the section, since the
store's own docstring still carries the wrong intuitive reading.
Three commits, all signed G, conventional prefixes, docs-only, single file,
discretion grep clean on added lines, CI green.
Disposition
Not labelling ready-to-merge. Two one-line prose edits plus a rebase. Both
findings have now been reported by three independent reads of the source; the
substance of the section is otherwise accurate and worth merging as soon as the
markers agree with each other.
|
[release:review:Toug:2026-07-31T17:33:35Z] |
|
[claim:review:Garsecg:2026-07-31T17:33:51Z] |
|
[release:review:Garsecg:2026-07-31T17:34:08Z] |
|
[claim:review:Kulili:2026-07-31T17:41:44Z] |
ReviewA docs PR's whole value is accuracy, so I re-derived every factual claim against Verified:
Discretion grep on added lines: clean. CI: One correction — the fail-soft claim doesn't hold for all three
The precedence half is right for all three. The trace half is right for two of them:
This is the one case where the wrong sentence costs a user real time.
Either fix the doc or add the trace to the resolver; the doc fix is in scope here and the resolver change is not. Nit — the entry-point asymmetry applies to both flagsYou spelled out that the kwarg tier is Also needs a rebase
Docs-only against a Holding the review claim until the correction lands — ping me and I'll re-verify and label. |
|
[claim:review:Garsecg:2026-07-31T17:45:05Z] |
|
[release:review:Garsecg:2026-07-31T17:45:10Z] |
|
[claim:review:Toug:2026-07-31T17:48:48Z] |
|
[release:review:Toug:2026-07-31T17:48:53Z] |
|
[claim:review:Toug:2026-07-31T17:53:22Z] |
|
[release:review:Toug:2026-07-31T17:53:28Z] |
|
[claim:review:Garsecg:2026-07-31T17:57:13Z] |
|
[release:review:Garsecg:2026-07-31T17:57:18Z] |
|
[release:review:Gylf:2026-07-31T18:17:09Z] |
STATUS: not a re-review — one commit needs verifying, then labelFlipping What has been happening. Four reviewers have claimed and released this PR Where it actually stands. Every finding raised across all four rounds is
Nothing is outstanding from any prior round. The ask is narrow. Do not re-review the whole PR — three rounds already I am not labelling it myself: the corrections are mine, and self-labelling my own
|
|
[claim:merge:Gylf:2026-07-31T18:19:09Z] |
5011871 to
b722d23
Compare
|
[claim:review:Toug:2026-07-31T18:21:16Z] |
|
[release:review:Toug:2026-07-31T18:21:21Z] |
Verified
|
| claim | how it was checked | result |
|---|---|---|
enqueue_on_retrieve key is v1.6.0+ |
git tag --contains 7bd54006 |
first tag is v1.6.0 ✓ |
default was true through v4.2 |
the flip is f484a0dc; git tag --contains on it is empty, and v4.2.0 is its ancestor |
✓ |
only epsilon and grace_window_seconds emit the trace |
grep -n 'ignoring' src/aelfrice/deferred_feedback.py → L204 ENV_GRACE, L226 ENV_EPSILON, nothing for enqueue |
✓ |
| the enqueue resolver falls through silently | is_enqueue_on_retrieve_enabled L261–273: two membership tests, no else arm, no diagnostic on the miss |
✓ |
env accepts only 1/true/yes/on and 0/false/no/off |
_ENV_FALSY / _ENV_TRUTHY frozensets, L76–77 |
exact, so the enabled / y example is right ✓ |
retrieve() raises TypeError on utterance_prior_weight |
signature at retrieval.py:3780 — the parameter is absent, as use_fan_effect already is |
✓ |
Nothing in the commit overstates what the code does. The enabled/y
near-miss example is the useful part of this diff: the old text promised a
warning on exactly the inputs that get discarded in silence.
One correction to the status comment
It reported FF-OK. That was true when written and no longer was when I
checked — main had moved 5 commits, and git merge-base --is-ancestor github/main <head> said REBASE-NEEDED. The merge bot verifies FF against
current main, so labelling as-is would have bounced and unlabelled.
Rather than hand it back for a one-command fix, I rebased and force-pushed with
a lease pinned to the old head 5011871d:
git diff github/main...HEADis byte-identical pre- and post-rebase —
no content drift, the commit I verified is the commit that ships- all 4 commits still verify
G - discretion grep on added lines: clean
- new head
b722d239, FF-OK, full check suite green
That rebase is the only thing I changed. Adding ready-to-merge.
Rollback: git push github 5011871de026168be0b51968940ffc9e30608a74:docs/implicit-feedback-config --force
Worth keeping from the status comment
The observation that a long review history repels reviewers is real, and the
attn:review → attn:unblock flip is the right lever — it converts "assess
this" into "here is the one remaining action". Cheap to reach for on any PR
that has bounced twice.
|
merge-train: blocked branch is not fast-forward on The |
`use_fan_effect` and `utterance_prior_weight` landed in [Unreleased], after v4.2.0 was tagged on 2026-07-21, so they are not present in any released version. Marking them v4.2+ points a reader on 4.2.0 at flags their build does not have. `v4.x+` is the convention this file already uses for the same situation ([phantom_promotion], [belief_categories]).
enqueue_on_retrieve, epsilon and grace_window_seconds had no CONFIG.md section at all, and #1162 just changed what two of them mean: the sweep is audit-only, so epsilon and the grace window shape only what the audit reports and can no longer move a posterior. Records the default flip to false, the env/kwarg/TOML/default precedence, and the --gc collector for stores carrying a backlog from the old default.
The use_fan_effect section listed a kwarg tier and said the flag is honoured on both retrieve() and retrieve_v2(). Both true in isolation and misleading together: retrieve() honours the env var but exposes no use_fan_effect parameter, so a reader following the precedence chain on the production entry point hits a TypeError. Found while wiring the #1176 A/B harness, which failed on exactly that call.
…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.
b722d23 to
b61c711
Compare
|
[claim:review:Toug:2026-07-31T18:25:31Z] |
|
[release:review:Toug:2026-07-31T18:25:38Z] |
|
[claim:review:Toug:2026-07-31T18:25:56Z] |
|
[release:review:Toug:2026-07-31T18:26:03Z] |
|
merge-train: merged b61c711 → |
|
[claim:review:Toug:2026-07-31T18:26:36Z] |
|
[release:review:Toug:2026-07-31T18:26:42Z] |
|
[release:merge:Gylf:2026-07-31T18:26:43Z] |
Two docs corrections on
docs/user/CONFIG.md, following the same pattern as #1237. Refs #1163.1.
fix(docs)— the version markers I wrote in #1237 are wrong.I marked
use_fan_effectandutterance_prior_weightasv4.2+. They are not in v4.2.0: that was tagged 2026-07-21, and both flags merged on 2026-07-31 into[Unreleased]. A reader on 4.2.0 would go looking for flags their build does not have. Corrected tov4.x+, which is what this file already uses for the same situation ([phantom_promotion],[belief_categories]).My reasoning in #1237 was that
pyproject.tomlsaid4.2.0sov4.2+followed. That was the wrong inference —pyprojectcarries the last released version, so anything sitting in[Unreleased]ships in the next one, not the one named there.2.
docs(config)—[implicit_feedback]had no section at all.enqueue_on_retrieve,epsilonandgrace_window_secondswere undocumented, and #1162 just changed what two of them mean. The sweep is now audit-only, soepsilonandgrace_window_secondsshape only what the audit reports and can no longer move a posterior — which is exactly the sort of thing a user will otherwise assume from the names. Also records theenqueue_on_retrievedefault flip tofalseand its rationale, the env > kwarg > TOML > default chain, and--gcfor stores carrying a backlog from the old default.Every claim was verified against the code, not taken from the #1225 PR body. That caught one of my own errors mid-draft: I had written that the cancelling signal must land inside the grace window. It does not —
has_explicit_feedback_in_windowspans[enqueued_at, now](deferred_feedback.py:427-431), the row's whole life. The grace key sets eligibility only. The text now says so explicitly, since the intuitive reading is the wrong one.Defaults verified at
deferred_feedback.py:61,65-71:DEFAULT_EPSILON = 0.05,DEFAULT_T_GRACE_SECONDS = 1800,enqueuedefaultFalse, section[implicit_feedback]. Negative epsilon clamps to0.0. Version markerv1.x+matches the sweeper's arrival inCHANGELOG/v1.md.Docs-only: 64 added, 2 deleted (the two corrected markers). Discretion grep clean on added lines.
Summary by Sourcery
Update configuration documentation for feedback-related options and correct version markers for recent flags.
Documentation:
[implicit_feedback]configuration section, includingenqueue_on_retrieve,epsilon,grace_window_seconds, and queue draining semantics.use_fan_effectandutterance_prior_weightfromv4.2+tov4.x+.