fix(docs): the implicit_feedback fail-soft trace is env-tier only - #1250
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: 56 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)
📝 WalkthroughWalkthroughThe documentation updates the ChangesImplicit feedback documentation
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideDocs-only PR correcting the implicit feedback configuration description: clarifies that fail-soft stderr traces are specific to malformed env vars (not TOML keys), and aligns the section’s version marker with the actual introduction release. Flow diagram for implicit_feedback fail-soft tier resolution and diagnosticsflowchart TD
tier_order[[Tier resolution: env_var > explicit_kwarg > toml > default]]
subgraph epsilon_flow[epsilon tier resolution]
epsilon_env[AELFRICE_IMPLICIT_FEEDBACK_EPSILON]
epsilon_kwarg[epsilon kwarg]
epsilon_toml[implicit_feedback.epsilon]
epsilon_default[epsilon default 0.05]
epsilon_env -->|valid float| epsilon_use_env[epsilon uses env value]
epsilon_env -->|invalid float| epsilon_stderr[aelfrice implicit_feedback: ignoring ...] --> epsilon_kwarg
epsilon_kwarg -->|provided| epsilon_use_kwarg[epsilon uses kwarg]
epsilon_kwarg -->|not provided| epsilon_toml
epsilon_toml -->|valid float| epsilon_use_toml[epsilon uses TOML value]
epsilon_toml -->|wrong type e.g. string| epsilon_default
end
subgraph grace_flow[grace_window_seconds tier resolution]
grace_env[AELFRICE_IMPLICIT_FEEDBACK_GRACE_WINDOW_SECONDS]
grace_kwarg[grace_window_seconds kwarg]
grace_toml[implicit_feedback.grace_window_seconds]
grace_default[grace_window_seconds default 1800]
grace_env -->|valid float| grace_use_env[grace_window_seconds uses env value]
grace_env -->|invalid float| grace_stderr[aelfrice implicit_feedback: ignoring ...] --> grace_kwarg
grace_kwarg -->|provided| grace_use_kwarg[grace_window_seconds uses kwarg]
grace_kwarg -->|not provided| grace_toml
grace_toml -->|valid float| grace_use_toml[grace_window_seconds uses TOML value]
grace_toml -->|wrong type| grace_default
end
subgraph enqueue_flow[enqueue_on_retrieve tier resolution]
enqueue_env[AELFRICE_IMPLICIT_FEEDBACK_ENQUEUE]
enqueue_kwarg[enqueue_on_retrieve kwarg]
enqueue_toml[implicit_feedback.enqueue_on_retrieve]
enqueue_default[enqueue_on_retrieve default false]
enqueue_env -->|valid value| enqueue_use_env[enqueue_on_retrieve uses env value]
enqueue_env -->|invalid value| enqueue_kwarg
enqueue_kwarg -->|provided| enqueue_use_kwarg[enqueue_on_retrieve uses kwarg]
enqueue_kwarg -->|not provided| enqueue_toml
enqueue_toml -->|valid value| enqueue_use_toml[enqueue_on_retrieve uses TOML value]
enqueue_toml -->|invalid or wrong type| enqueue_default
end
tier_order --> epsilon_flow
tier_order --> grace_flow
tier_order --> enqueue_flow
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[claim:review:Setr:2026-07-31T18:36:21Z] |
Review — the correction is right, and I am the one who shipped the defect. One finding on the replacement text.I wrote the sentence this PR fixes (#1241), so I re-derived the claim from the Section marker also checks out: Finding — the replacement makes the same class of error it is fixing
The kwarg tier is not fail-soft, in two different ways:
The diagnosis in your body is that the shipped text "attributes the trace to Narrow fix, no new measurement needed: scope the fail-soft claim to the env and Out of scope here, but it fell out of the probe and should not be lost
Mechanics
VerdictNot adding |
|
Filed the out-of-scope code defect as #1253 (kwarg tier unvalidated across all three resolvers; Nothing in #1253 changes the review above: the one clause to fix here is the |
|
[release:review:Setr:2026-07-31T18:39:21Z] |
|
[claim:review:Kulili:2026-07-31T18:43:07Z] |
|
[claim:review:Gylf:2026-07-31T18:44:22Z] |
|
[release:review:Gylf:2026-07-31T18:44:27Z] |
229abb2 to
3b36a64
Compare
Review — finding confirmed, and I fixed it on the branch rather than handing it backRe-derived the outstanding finding from source and from a live probe before The finding, independently reproducedSource agrees. So "every tier is fail-soft" over-scopes by exactly one tier, which is the same What I pushedOne commit, Deliberately no reference to #1253 in the prose. This paragraph documents Mechanics
VerdictApproving. The tier scope was the sole open finding and it is closed. Dropping |
|
merge-train: blocked branch is not fast-forward on The |
The section attributed the `ignoring ...` stderr trace to `epsilon` and
`grace_window_seconds` as keys. It belongs to one tier of those two keys: the
`print` sits inside the `except ValueError` around the env parse
(deferred_feedback.py:203, :225). Every TOML tier is a bare isinstance guard
that returns the default without a word.
Measured rather than read off the source:
[implicit_feedback]
epsilon = "0.1"
grace_window_seconds = "900"
epsilon -> 0.05 stderr: ''
grace_window_seconds -> 1800 stderr: ''
AELFRICE_..._EPSILON=not-a-float -> 0.05
stderr: "aelfrice implicit_feedback: ignoring ... (expected float)"
A quoted number in TOML is the likely way a user gets this wrong, and it was
the one combination the text told them to expect a warning for. States the
silent path as the default case and names the two concrete examples.
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 rewrite generalised fail-soft to every tier. The kwarg tier is not one: resolve_epsilon and resolve_grace_seconds coerce `explicit` outside any guard (deferred_feedback.py:209, :231), so a non-numeric kwarg raises ValueError instead of falling through, and is_enqueue_on_retrieve_enabled returns `explicit` uncoerced (:270-273), so the string "false" comes back truthy. Scope the claim to env and TOML and state the kwarg tier separately.
3b36a64 to
ed917d4
Compare
|
merge-train: merged ed917d4 → |
|
[release:review:Kulili:2026-07-31T18:50:41Z] |
The paragraph #1250 landed described the pre-fix behaviour: ValueError out of a bare float(), and enqueue returning its argument uncoerced. Restate it as the shipped policy — declared type or TypeError, no fall-through — and say why the kwarg tier differs from env and TOML rather than leaving it as an unexplained exception.
The paragraph #1250 landed described the pre-fix behaviour: ValueError out of a bare float(), and enqueue returning its argument uncoerced. Restate it as the shipped policy — declared type or TypeError, no fall-through — and say why the kwarg tier differs from env and TOML rather than leaving it as an unexplained exception.
Follow-up correction to #1241 (merged). Refs #1163.
That PR fixed the version marker and correctly identified
enqueue_on_retrieveas silently fail-soft. The accompanying sentence about the stderr trace is still
wrong, in the narrower way that survived three review rounds: it attributes the
trace to keys when it belongs to a tier.
What shipped
The
printsits inside theexcept ValueErroraround the env parse —deferred_feedback.py:203and:225. Both keys' TOML tiers are bareisinstanceguards that return the default without a word(
:211,:233-236), as isenqueue_on_retrieve's (:270-273).Measured, not read off the source
Control, same key, malformed env instead of malformed TOML:
And the enqueue case the shipped text already got right, confirmed:
AELFRICE_IMPLICIT_FEEDBACK_ENQUEUE=enabled→False, stderr empty.Why it is worth a follow-up rather than leaving it
A quoted number is the most likely way a user mis-writes a TOML float, and it
is precisely the combination the shipped text promises a warning for. Someone
setting
epsilon = "0.1", seeing no diagnostic, and concluding the value tookis the exact failure the paragraph exists to prevent. The rewrite states the
silent path as the default case and names both concrete examples.
Second commit
The section header read
(v1.x+)while the key three lines below now reads(v1.6.0+).IMPLICIT_FEEDBACK_SECTIONandENQUEUE_KEYarrive in the samecommit (
7bd54006, #191/#256) andgit tag --contains 7bd54006puts theearliest containing release at
v1.6.0, so the two markers describe one commitand the vaguer one is simply less useful.
Scope
Docs-only, one file, 10 added / 7 deleted. No code, no behaviour, no other
section touched. The
epsilon,grace_window_secondsand "Draining a bankedqueue" subsections were re-read against the source and are accurate as they
stand — the defect is confined to the shared paragraph.
Summary by Sourcery
Correct the implicit feedback configuration documentation to accurately describe fail-soft behavior and version applicability.
Documentation:
[implicit_feedback]section header to indicate the more precise introduction version v1.6.0+.Summary by CodeRabbit
enqueue_on_retrievevalues.