fix(config): quoted YAML booleans silently coerced to true across 9 config loaders#558
fix(config): quoted YAML booleans silently coerced to true across 9 config loaders#558Tet-9 wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 33 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 (2)
WalkthroughAdds shared YAML boolean coercion and applies it to feature configuration and receipt auto-approval settings. Regression tests cover quoted boolean strings, recognized spellings, whitespace handling, and fallback behavior. ChangesConfiguration boolean coercion
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
github-actions
Bot
added
storage
…onfig loaders
bool(x) on an arbitrary config value is a trap: yaml.safe_load resolves an
unquoted true/false to a real bool, but a mistakenly-quoted "false" stays
a non-empty string, and bool("false") is True. This exact bug was
independently repeated in 9 places:
- compile.py: two_phase
- session_split.py, admission.py, inbox.py, recall.py, capture.py,
volunteer_context.py: enabled
- admission.py: reject_uncited_session_pages (found alongside the enabled
fix, same function)
- proposals.py: auto_approve_on_receipt, read via _review_config() at 4
call sites (2 of which used the bug directly with no bool() at all --
a raw truthy check on the config dict value)
The last one is the one that matters most: auto_approve_on_receipt gates
whether a claim can be durably approved without a human reviewer. A
mistakenly-quoted "false" in config.yaml was silently read as enabled by
every caller, not just the one this PR happened to start from.
- New shared src/vouch/config_coerce.py: coerce_bool(value, default),
fail-soft (bad/unrecognized values degrade to the caller's default,
same posture compile.py's own _coerce() already documents for its
numeric fields) -- as opposed to install_adapter.py's install.yaml
flags, which correctly raise hard, appropriate for a one-time CLI
install but not for config read on every session/render.
- _review_config() (proposals.py) now normalizes auto_approve_on_receipt
once at the read boundary -- the single source of truth every caller
already funnels through -- rather than patching each read site
individually.
- 8 config loaders switched from bool(raw.get(...)) to coerce_bool(...).
- New tests/test_config_coerce.py for the coercer itself, plus a quoted-
false regression test added to every affected module's existing test
file, plus a quoted-true test in test_receipt_auto_approve.py proving
the fix doesn't break the legitimate quoted case.
Not included: openclaw/types.py's CompactParams.force field uses the same
bool(raw.get(...)) shape but parses a JSON wire payload from an external
host (OpenClaw), not YAML config -- JSON has native booleans, so the
quoted-string trap doesn't apply the same way, and it's a different root
cause than the rest of this sweep.
47f8083 to
8b80900
Compare
|
@plind-junior review if you may |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/vouch/config_coerce.py`:
- Line 1: Lowercase the prose sentence starts in the affected docstrings:
`Shared` in `src/vouch/config_coerce.py` lines 1-1, `Parse` in lines 29-30, and
`The` and `Callers` in `src/vouch/proposals.py` lines 480-489; preserve the
remaining wording and behavior.
🪄 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: 0313d580-7924-4d1e-881d-07141d57e47d
📒 Files selected for processing (18)
src/vouch/admission.pysrc/vouch/capture.pysrc/vouch/compile.pysrc/vouch/config_coerce.pysrc/vouch/inbox.pysrc/vouch/proposals.pysrc/vouch/recall.pysrc/vouch/session_split.pysrc/vouch/volunteer_context.pytests/test_admission.pytests/test_capture.pytests/test_compile.pytests/test_config_coerce.pytests/test_inbox.pytests/test_recall.pytests/test_receipt_auto_approve.pytests/test_session_split.pytests/test_volunteer_context.py
|
CodeRabbit review comment: src/vouch/** comments and review notes must be lowercase per path instructions. Fixes 3 capitalized sentence-starts introduced in the previous commit: - config_coerce.py:1 (module docstring) - config_coerce.py: coerce_bool() docstring - proposals.py: _review_config() docstring, two sentence-starts
|
@plind-junior , the remaining to do is all yours to take a look at |
1 participant
what
bool(x)on an arbitrary config value is a trap:yaml.safe_loadresolves an unquotedtrue/falseto a real Python bool, but a mistakenly-quoted"false"in config.yaml stays a non-empty string — andbool("false")isTrue. This exact bug was independently repeated across the codebase in 9 places:compile.py:two_phasesession_split.py,admission.py,inbox.py,recall.py,capture.py,volunteer_context.py:enabledadmission.py:reject_uncited_session_pages(found alongside theenabledfix, same function)proposals.py:auto_approve_on_receipt, read via_review_config()at 4 call sites — 2 of which didn't even wrap it inbool(), just used the raw config value in a truthyifThat last one is the one that actually matters:
auto_approve_on_receiptgates whether a claim can be durably approved with no human reviewer. A mistakenly-quoted"false"in config.yaml was silently read as enabled by every caller, not just the site this PR happened to start from.why
Rather than patch each of the 9 read sites individually with a repeated inline fix, this adds one shared, tested primitive (
coerce_bool) and — for the security-relevant case — normalizes at the single choke point (_review_config()) all 4 callers already funnel through, so the fix can't drift out of sync the way 9 independentbool()calls already had.coerce_bool()is deliberately fail-soft (bad/unrecognized value degrades to the caller's default), matching the posturecompile.py's own_coerce()already documents for its numeric fields. This is a different, and correct, choice frominstall_adapter.py'sinstall.yamlflags, which raise hard — appropriate for a one-time CLI install, wrong for config read on every session/render.Not included:
openclaw/types.py'sCompactParams.forceuses the samebool(raw.get(...))shape, but it parses a JSON wire payload from an external host (OpenClaw), not YAML config. JSON has native booleans, so the quoted-string trap doesn't apply the same way — different root cause, left out of this sweep.invariants held
proposals.py's security-relevant field, which now has one authoritative parse instead of 4 independent (and inconsistent) ones."true"still works correctly (tested explicitly, not just the negative case).tests
tests/test_config_coerce.py: unit tests for the coercer itself (recognized spellings, case-insensitivity, whitespace, fallback behavior)."false"-does-not-enable regression test added to every affected module's existing test file (test_compile.py,test_session_split.py,test_admission.py,test_inbox.py,test_recall.py,test_capture.py,test_volunteer_context.py).test_receipt_auto_approve.py: two regression tests through the real approval path (approve()+auto_approve_receipts()) proving self-approval stays blocked on a quoted-"false"gate, plus one proving quoted-"true"still works.test_cli.py/test_delete.py/test_hot_memory.py, unrelated — verified no reference to any file this PR touches).Summary by CodeRabbit
Bug Fixes
"false"no longer incorrectly enable features (and"true"no longer becomes ambiguous).Tests