fix(policy,adapters,plugins): typed errors for every user-authored TOML read (#440, #473) - #688
Conversation
…fields (#440) Generalize the four [limits]-only readers into section-aware `_typed_int` / `_typed_float` / `_typed_bool` / `_typed_str(d, where, key, default)` and add `_typed_str_tuple` for the argv-shaped arrays, then route every remaining bare `int()` in `loads()` through them: verify.stream_capture_kb, the four sweep counters, scm.max_parallel, scm.failed_diff_max_mb, and the two cleanup retention knobs. `_opt_grace`/`_opt_nudges` return None for "inherit" so they cannot take a default — they carry the same guard inline. All ~17 [limits] call sites pass where="limits", so every message #587 pinned stays byte-identical; scm.preserve_keep folds into `_typed_int`, which is the guard it already hand-rolled. The arrays had two failure modes, not one: a bare TOML string is iterable, so `commands = "pytest"` exploded per character into six one-character commands that read as applied configuration, while a scalar raised a bare TypeError out of `loads`. Both now raise PolicyError naming section.key, and unset stays distinct from empty (extra_args None = inherit the profile's flags). Every new gate is ablation-verified singly; the pairs redden disjoint row sets. The validate carry (#474) asserts on the machine_json document, not rc — with the conversion reverted the backstop returns 1 too, and what actually bites is stdout carrying no document at all.
…dropped _snapshot_extra_args is now the one place carrying the pre-fix shape. Its input is a json round-tripped asdict(Policy), already validated on the way in, and adapter_policy_from_snapshot wraps it in except Exception: return None because it feeds display surfaces that must never crash — so a PolicyError here would only be swallowed, at the cost of blanking the display.
…nstead of crashing (#473)
|
Warning Review limit reached
Next review available in: 40 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughPolicy parsing now rejects wrong-typed TOML values with contextual ChangesConfiguration and resource error handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change routes invalid user-authored TOML through typed errors while preserving valid configuration behavior, and no actionable merge-blocking risk remains after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66d6e2e849
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - **A policy field of the wrong TOML type now raises `PolicyError` naming `section.key` | ||
| (#440).** `loads()` coerced with bare `int()`/`float()`/`bool()`/`str()` outside the |
There was a problem hiding this comment.
Condense these additions to terse, scannable release-note bullets. The two new entries span 36 lines and include implementation history, exception-flow analysis, and exhaustive field lists, making the Unreleased section difficult to scan and directly violating the repository's changelog convention.
AGENTS.md reference: AGENTS.md:L68-L70
Useful? React with 👍 / 👎.
Cut reviewer-facing rationale from the two Unreleased entries: the rejected-alternative note on why widening the conversion funnel could not have caught the read fault, the per-handler exception tracing, and the 15-section enumeration. Every user-visible fact is kept -- the symptoms, the two silent coercions, both arms of the read guard, and the compatibility notes.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@coderabbitai review |
|
#473's repro and contract are both the validate command, which never reaches the plugin loader, so the plugin half of this entry was attributed to an issue that does not cover it.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
The problem
Three separate parsers read user-authored TOML —
policy.loads,adapters/profile.load_profiles,and
plugins/loader— and each has a typed domain error (PolicyError,ProfileError,PluginError) that every consumer keys its fault handling on. But a file whose content waswrong escaped as a raw
ValueError/TypeError/UnicodeDecodeErrorinstead, walkingstraight past the handlers written to survive exactly that:
cli.py's_configure_mux—except (PolicyError, OSError), and it runs before dispatch onevery subcommand, so one bad character traced back at you from an unrelated command.
tui/app.pyandtui/screens/dashboard.py— the TUI died at construction rather than fallingback to defaults and letting the operator open the settings editor to fix the file.
tui/settings.py—except (PolicyError, PluginError), the degrade that renders the messageinto the settings screen's error line.
cmd_validate's role loop — the raw escape hitmain's bare-except backstop, which prints oneerror:line and no document, breakingmachine.py's one-object--jsoncontract.Two of the policy faults never announced themselves at all:
notify.desktop = "false"is a truthystring, so
bool()turned the feature on;verify.commands = "pytest"iterated a bare stringinto six one-character commands that read back as applied configuration.
What changed, phase by phase
d2e728c6,025d57ca[limits]-only helpers generalized to section-aware_typed_int/_typed_float/_typed_bool/_typed_str(d, where, key, default)(+_typed_str_tuple); every remaining rawint()/float()inloads()routed through them;_opt_grace/_opt_nudgesgiven inline type guards; array shape guards onverify.commandsand bothextra_args.cda395d2bool()/str()routed through the typed helpers — 16 boolean fields, 22 string fields, theplugins.enabledentries, and the deprecated-[engine]fold. Allowlist fields keep their allowlist and blame the type first.bcddfed9_read_profile_text(entry, source)at bothload_profilesread sites (packaged + overlay), convertingUnicodeDecodeError/OSErrorintoProfileErrornaming the file.66d6e2e8_read_manifest_text(toml, source)at bothplugins/loaderread sites (builtin + project); CHANGELOG.[limits]messages are byte-identical throughout — all call sites passwhere="limits". Validpolicies parse to identical
Policyobjects: this changes only what happens to invalid values.usage_grace_s = 30is still a legal float, and an unsetextra_argsstill means "inherit theprofile's flags" where
[]means "none". The lenient snapshot readers (_snapshot_extra_args,adapter_policy_from_snapshot) are deliberately untouched — they parse the json-round-trippedRunState.policy_snapshotand must never crash;025d57cadocuments why.plugins/manifest.pyneeded no work: its value-coercion funnel already exists and its messagesare pinned by tests. The live plugin gap was the unguarded read, not the parse.
Repros, re-measured on this branch
[scm] max_parallel = "x"ValueErrorPolicyError: scm.max_parallel must be an integer: got 'x'[cleanup] run_retention = "x"ValueErrorPolicyError: cleanup.run_retention must be an integer: got 'x'[limits] dev_contract_nudge = "false"[notify] desktop = "false"PolicyError: notify.desktop must be a boolean: got 'false'validate --project proj --jsonerror:line, empty stdout, rc 1adapter.profilefinding naming the file; stderr emptyplugin.tomlUnicodeDecodeErrorthrough the TUI settings degradePluginError: plugin <path>: not valid UTF-8: …, settings surface degradesVerification
uv run pytest -q -n logical— 6397 passed, 53 skipped (6225 before phase 1; +172 cases).uv run pyright— 0 errors.trunk fmt+trunk check --all(258 files) — clean.cpbackup withan md5 check rather than
git checkout. Where a fix had two axes, both were measured and theredden-sets are disjoint — which is what proves neither half stands in for the other:
UnicodeDecodeErrorarm reddens 1, theOSErrorarm 2.scm.keep_failed) to a barebool()withboth helpers intact reddens exactly 3 rows while 62 sibling rows stay green.
machine_jsondocument, never on rc alone:main'sbare-except backstop also returns 1, so an rc-only assertion is green with the fix reverted.
Closes #440. Closes #473. Closes #689.
#689 was filed for the plugin-manifest leg after review: #473's repro and contract are both
the
validatecommand, which never reaches the plugin loader, so that leg had no issue of itsown even though it is the same fault at the third and last parser reading user-authored TOML.
Filed rather than split out — the guard is 37 lines in
loader.pymirroring the profile guard,and shipping two of three parsers fixed would leave a known crash on
main.Context, no status change intended: #587 (the earlier
[limits]leg this extends), #474(a duplicate of #440, contributing the validate-document carry test), #278 (where the
truthy-string hazard was recorded).
Summary by CodeRabbit