feat(config): hold a value to the choices its setting declares - #868
Conversation
A spec's `choice` nodes reached the docs, the JSON schema and completions, and nothing that resolved a value. So a CLI documenting three allowed values took a fourth in silence and failed later, somewhere that could not say why — the declaration and the behaviour drifting apart, which is the thing this crate exists to stop. The check sits beside the type check, in both paths a value can arrive by, and costs a setting without choices nothing. A collection is checked item by item, because choices on a `list<string>` mean each item is one of them — the rule `usage g json-schema` already follows, putting the enum on every value position rather than on the container — and the *item* is what the message quotes, since naming the whole list leaves the user to work out which of five items is wrong. A refused value costs its own key, like a value of the wrong type: a bad line in a system-wide file must not stop a CLI from starting for every user on the machine. And `explain` now lists what a setting will take, because somebody reading an explanation after a refusal has already scrolled past the warning.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Greptile SummaryThe PR adds runtime enforcement and explanation output for configuration choices.
Confidence Score: 4/5The generated-registry path must populate the new choices field before merging, otherwise spec-declared restrictions remain unenforced. Both ingestion paths correctly consult Files Needing Attention: config/src/registry.rs and config-build/src/emit.rs Important Files Changed
Reviews (1): Last reviewed commit: "feat(config): hold a value to the choice..." | Re-trigger Greptile |
| /// Empty means anything the type allows. Declared in the spec as `choice` nodes, where they | ||
| /// already reach the docs, the JSON schema and completions — and, until this, nothing that | ||
| /// *resolved* a value, so a CLI documenting three allowed values accepted a fourth in silence. | ||
| pub choices: &'static [Const], |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e78b52b. Configure here.
| parse: None, | ||
| envs: &[], | ||
| bindings: &[], | ||
| choices: &[], |
There was a problem hiding this comment.
Choices never reach generated registries
High Severity
PropMeta now carries choices and the layer paths refuse values that miss them, but usage-config-build still never writes the field — it stays the empty slice from PropMeta::new. Spec choice nodes therefore still never reach resolution for any generated registry, so a CLI documenting three allowed values continues to accept a fourth in silence.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e78b52b. Configure here.
Instruction counts
No instruction-count regression above 1%. Only instruction counts gate. Wall clock is shown for context — on identical hardware it moves 4-20% run to run. Measured by tak — instruction-counted CLI benchmarks, stored in this repository's git notes. Shadow comparisonParsing
|
|
Both bots found the same gap, and it is real — but it is the next commit rather than a defect here: They are stacked, so this never reaches a release without it. Split because they are two different AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
…869) The step between #868's enforcement and the spec that declares it: `choice` nodes become `Const`s in the generated registry, so the values a CLI documents are the values it accepts. The end-to-end test runs a config file through the *generated* registry and watches the merge refuse `stash = "svn"` while the declared default stands — the three places that used to disagree (docs, schema, resolution) now come from one declaration. **A default the choices do not allow is refused here.** At run time it is seeded as the bottom layer and then goes through the same check as every other value, so it would be a warning on every run of a shipped binary — for a mistake only the author of the spec can fix: ``` `stash` defaults to `svn`, which is not one of the values it allows: git, none ``` A list default is held to them item by item, the way the values themselves are. Two mutations: not emitting the choices (caught by the regenerate-and-diff test, which is how any emitter change is caught here), and checking only the scalar default and not the list one. Still deliberately out: generating an *enum* per choice-bearing setting. It needs variant naming, a `FromValue` impl and its own error, and none of that is needed for the values to be enforced — `String` plus a refusal at the boundary is the smaller thing that makes the declaration true. *AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.* <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches config resolution, codegen defaults, and value display/coercion across three crates; mistakes could change which values are accepted or what defaults ship, though coverage is heavy. > > **Overview** > **Choices in the generated registry** — `config-build` now emits `choices` on each `PropMeta` and depends on `usage-config` so validation uses the same `Ty::coerce` the runtime uses. > > **Build-time spec validation** — The generator rejects defaults and choices the declared type cannot read, defaults outside the allowed set (including list defaults checked per-item or as a whole for scalar/`any` types), and emits defaults via `coerced_const` so seeded values match what resolution expects (e.g. `default "yes"` on `list<bool>` becomes bool literals). > > **Runtime choice matching** — `PropMeta::refuses` walks collections using the **declared** type (not value shape), and `allows` coerces each choice before comparing. `Const::matches` falls back to matching `display()` text so cross-shaped literals (e.g. `choice 4` vs string `"4"`, `choice "yes"` vs bool) agree with coercion. > > **Shared float rendering** — `usage-config` and `usage-lib` format whole-number floats with a decimal point (`1.0` vs `1`) so default/choice comparisons and error messages stay consistent across spec parse, codegen, and merge warnings. > > **Tests** — Fixture `log_format`, golden output, end-to-end resolve for invalid `stash`, and broad refusal tests in `config-build/tests/refusals.rs`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 964d72b. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Configuration settings now expose their permitted values. * Added the `log_format` setting with supported values `1` and `2`. * Compatible scalar values are recognized across textual and numeric representations. * **Bug Fixes** * Invalid scalar and list defaults are rejected with clear messages showing invalid and allowed values. * Invalid setting values fall back to the effective default and generate a warning. * Configuration validation messages now format values more clearly, including whole-number decimals. <!-- end of auto-generated comment: release notes by coderabbit.ai -->


A spec's
choicenodes reach the docs, the JSON schema and completions — and, until this, nothingthat resolved a value.
PropMetahad no choices at all, so a CLI documenting three allowed valuestook a fourth in silence and failed later, somewhere that could not say why. That is the declaration
and the behaviour drifting apart, which is the thing this crate exists to stop.
Where the check goes. Beside the type check, in both paths a value can arrive by — text through a
parser, and a structured value out of a file. Checking one and not the other is how a rule ends up
applying to the environment and not to the file beside it, so there is a test for each.
Collections are checked item by item. Choices on a
list<string>mean each item is one of them —the rule
usage g json-schemaalready follows, putting the enum on every value position rather thanon the container — and the item is what the message quotes, since naming the whole list leaves
the user to work out which of five is wrong.
A refused value costs its own key, exactly like a value of the wrong type: a bad line in a
system-wide file must not stop a CLI from starting for every user on the machine.
Const::matchescompares without building theValuea choice stands for: this runs once perdeclared choice for every value supplied, and a setting with choices is usually a string, where the
comparison would otherwise allocate a copy to throw away. A setting with no choices returns
immediately.
explainnow lists what a setting will take — somebody reading an explanation after a refusal hasalready scrolled past the warning.
Verification
107 tests in the crate. Four mutations, each killing the right test: the text path unchecked, the
structured path unchecked, list items unchecked, and an empty choice list refusing everything (which
takes 30 tests with it, since it makes every ordinary setting unsettable).
Next in the stack: the generator emitting these from the spec, and refusing a declared default that
is not among them — an authoring mistake that would otherwise be a warning on every run of a shipped
binary.
AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.