feat(reconcile): validate preference values against the trait at plan time#1794
Conversation
… time A file entry could set a value the server rejects (an empty value, or a value outside a checkbox or select trait's options). It passed validation and showed a set in the plan, then failed at apply, breaking rule 2 (a plan never contains a change that cannot apply). The diff now rebuilds each trait from the describe-preferences response and runs its own validator over the desired value, and over a trait default before a reset, so an unappliable change fails the plan with a clear message. Reuses core/preference validators, so plan-time and server-side checks cannot drift.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 (4)
📝 WalkthroughWalkthroughPreference reconciliation now loads complete trait definitions, validates requested values and reset defaults during planning, uses trait defaults for comparisons, and documents upfront validation behavior. ChangesPreference trait validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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 |
Coverage Report for CI Build 29988651525Warning No base build found for commit Coverage: 46.241%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
![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
🧹 Nitpick comments (1)
internal/reconcile/preference_reconciler.go (1)
134-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrait-conversion branches largely untested — likely the source of Coveralls' 17 uncovered lines.
inputTypeFromPB's switch (Textarea/Select/Combobox/Multiselect/Number) and theInputOptionsloop intraitFromPBaren't exercised by any test:platformTraits()/platformTraitPBinpreference_reconciler_test.goonly build checkbox-style traits withInputHints, so these branches are only implicitly covered by the default/empty-options path. A wrong mapping here would silently select the wrong validator without any test catching it.Consider adding one or two trait fixtures (e.g. a select trait with
InputOptions, a number/textarea trait) toplatformTraits()or a dedicated test to exercise the remaining switch cases and the options loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a9b2c03-f24e-4257-96ce-7b039c07e5a5
📒 Files selected for processing (5)
docs/content/docs/reconcile.mdxinternal/reconcile/preference.gointernal/reconcile/preference_reconciler.gointernal/reconcile/preference_reconciler_test.gointernal/reconcile/preference_test.go
The first cut validated every listed value, which broke the export round trip (rule 5): a value stored under an older or looser trait definition is exported verbatim, and re-reconciling it was rejected even though it equals the server's own value and plans no op. Validate only a value that differs from the one in effect, so a planned set is still appliable but a no-op set is left alone. Also treat a stored empty value as unset in both the reset loop and export, so it is not reset and not exported as an empty value the plan would reject. Adds tests for the stale-value round trip, the stored-empty cases, and value validation over the wire (exercising the proto-to-core trait mapping, not just the text path).
2 participants
What
Makes the Preference kind check a value against its trait during the plan, so a value the server would reject fails the plan up front instead of failing later at apply.
Why
Rule 2 of RFC 0001 says a plan never contains a change that cannot apply. A preference entry could set a value the server rejects: an empty value, or a value outside a checkbox or select trait's options. It passed validation and showed a
setin the dry-run plan, then failed at apply. That was the one gap that kept Preference at Partial on the rules audit.How
fetchTraitsrebuilds eachcore/preference.Traitfrom the describe-preferences response (input type, hints, options, default), keyed by name.core/preference, so the plan-time check matches the server's at the same version.Testing
go build,go vet,go test ./internal/reconcile/... ./core/preference/...,gofmt, andgolangci-lintall pass.Known limitations
allowed_scopes) is not expressible over the wire: the protoPreferenceTraitcarries noallowed_scopesfield, so the plan cannot pre-check a scope mismatch and that one case still surfaces at apply. Platform traits are global today, so this is a misconfiguration edge.core/preferencevalidators, so they agree at the same version. A CLI built against a different server release could disagree if the validator rules changed between versions.Status
Closes the last Rule 2 gap for the Preference kind on the rules-v2 audit, without weakening the Rule 5 export round trip. Targets
main.