feat(config): resolve settings from layers, with provenance - #849
Conversation
|
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 the dependency-free
Confidence Score: 4/5The PR does not appear safe to merge until renamed declarations carrying the only default preserve that default on the canonical setting. The current default-seeding loop skips every renamed declaration, so a registry where the old declaration owns the default and its replacement has none resolves both names without the declared value. Files Needing Attention: config/src/resolve.rs Important Files Changed
Reviews (14): Last reviewed commit: "feat(config): resolve settings from laye..." | Re-trigger Greptile |
Instruction counts
2 benchmark(s) above the 1% gate: 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
|
34b7af5 to
7743a25
Compare
|
Four findings on the first review round, all real. The first is the one worth reading — it was a genuine hole in something I called a security property. The scope check skipped every custom source. The fix reframes the question rather than adding a case. What matters is not "was it a file" — a pkl file in a repository is exactly as much a thing a checkout carries as pub enum Trust { Project, Operator, Invocation }Every origin carries one, and Renames were not folded at the merge. A set kept repeats from a single source. Deduplication lived on the merge-two path, so one Collection defaults could not take part in a merge. As a floor applied after the layers, a default only filled in what nothing had set — so a All four mutation-verified. Two of the mutations initially did not apply — AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
f2b4563 to
306b14c
Compare
|
Both bots caught the same thing and both were right: my fix for the rename fold moved the value and the winning origin to the folded Fixed, but the more useful outcome is why it survived my own testing: every assertion I had checked one field at a time — the value landed, the warnings said the right thing — and none checked that the fields agree. So there is now an invariant test: resolve a mix of layers including a renamed key, then for every prop assert that the winning origin is the last contributor and that the two are present or absent together. That fails on the mutation; the old per-field assertions did not. AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
306b14c to
b930fa9
Compare
b930fa9 to
1aaa9bd
Compare
|
Four genuinely new findings in this round — all of them consequences of the previous round's fixes, which is fair. (Several other comments on this commit are re-anchored copies of findings already fixed: the A rename's default was seeded where nothing reads it. My defaults-as-bottom-layer change seeded by each prop's own id, so a default declared on an old name landed under an id every lookup folds away from. An old name is an alias, not a setting — it is skipped now, and the setting that replaced it declares its own default. A deprecated key in a file was folded in silence. The rename warning fired only when the entry still carried the old id, and A post-merge rewrite split the provenance. An empty list could not clear a default. With defaults merging rather than filling in, All four mutation-verified. One took two attempts: the alias-default mutation survived at first because the fixture's alias had no default to seed, so the test could not tell the two behaviours apart. The fixture now has one. AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
1aaa9bd to
57eb621
Compare
|
Two more, both real, and both consequences of the previous round again. An alias was read with its own metadata. An empty string became a one-item list. A list or set given a bare Three mutations, all verified. AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
57eb621 to
a3967d0
Compare
a3967d0 to
49f82ea
Compare
49f82ea to
06b6aab
Compare
|
Both real, and both about the same thing: a warning that names something the user cannot find. The refusal named the folded key. I had fixed the deprecation and rename warnings to say the name that was written and left the refusal saying The refusal blamed a config file. Since the check became one about trust rather than about files, it also refuses a pkl file, a git config or an Three mutations. The type-error one initially survived, because nothing covered that message under an old name; there is a case for it now. AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
06b6aab to
5b5209e
Compare
5b5209e to
83ec93b
Compare
|
Real, and worth fixing for the reason given: a stack overflow is an abort with no message, so one mistyped The chain is now walked rather than recursed, bounded by the number of settings — a chain longer than the registry is a cycle by definition. Verified by mutation in the strongest form available: with the bound removed the cycle test does not fail, it hangs — the run reaches "running 1 test" and never returns. Covered are a two-step cycle, a setting renamed to itself, and a three-link chain that does resolve, so the bound is not simply refusing chains. AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
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 83ec93b. Configure here.
83ec93b to
72af80d
Compare
|
Accurate description, but I am keeping the behaviour, so let me say why rather than quietly not fix it. An old name is an alias. Which value a setting takes is the setting's business, and the replacement declares its own default — so an alias whose default the replacement lacks is a contradictory spec, not a case with an obvious answer. Seeding it would mean a declaration nobody reads any more silently governing the setting that replaced it, which is the class of spooky behaviour the rest of this crate is spent eliminating. The right place to catch it is where the registry is built, not where values are resolved: Worth adding that the case is narrow: a rename in a real registry carries defaults on both declarations — mise's do — and the target's own default wins, which is already what happens. AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
A new crate, `usage-config`: the runtime half of the config feature. The spec declares the settings; `usage-config-build` will turn that declaration into a registry of consts at build time; this crate resolves values against it. No KDL parser and no dependencies at all, because it is what runs in the CLI. Every CLI in the fleet has written this by hand, and every copy has rotted differently — hk declares eighteen `sources.cli` bindings and reads five, pitchfork documents a CLI layer it does not have, fnox's module doc describes a config-file layer that does not exist, mise hand-copies thirteen flags in a forty-nine-line function. That is not carelessness; it is what happens when the declaration of a setting and the code resolving it are two things kept in step by hand. What this PR contains: - `Value` and `Const` — the runtime value and the `const`-constructible default, separate because a generated registry must cost nothing to load. - `Ty` and `Parser` — the declared type as a match rather than a parse, and the named splitters (`list_by_comma` and friends) that are spec vocabulary so another language's runtime can honour them identically. - `Registry`/`PropId` — interned keys, so a merge over a hundred settings never hashes one. Rename chains resolve to their end while reporting the name the user actually wrote. - `Layer`/`LayerCtx` — the interface a CLI writes its own sources against, with `Registry::bindings(kind)` as the mechanism behind hk's git and pkl layers and aube's `.npmrc`. usage knows nothing about git; it just hands back the keys. - `resolve` — one merge, `replace`/`union`/`deep`, and provenance as its output rather than a second pass. hk grew a parallel merge just to answer "where did this come from", and the two could disagree; here `config explain` cannot describe a resolution that did not happen. Two things are deliberately in the merge rather than left to layers. Scope (`global`, `env`) is enforced there because mise calls it a security property, and a check every layer has to remember is one a new layer will forget. And a post-merge rewrite goes through `coerced()`, which re-labels the origin, so a value mise derived from `raw` never claims to have come from a file that never said it. Warnings are returned, never printed: an unknown key, a bad value, a deprecated setting. mise queues these until its logging is up, and a library with an opinion about stderr cannot be used by anything that has its own. 24 tests including the crate-level doc example; eight mutations of the merge semantics — inverted precedence, scope unenforced, global scope over-refusing, union and deep degraded to replace, sets not deduplicating, a rewrite keeping its old origin — each verified to fail without its fix. Not in this PR: file layers (TOML/JSON, find-up), the `explain` renderer, the build-time codegen and the typed `Settings` struct, and the config conformance corpus.
72af80d to
82096dc
Compare

The runtime half of the config feature: a new
usage-configcrate that resolves a CLI's settings from its layers. Sixth PR of stack #836; depends on the vocabulary in #835 only conceptually — this crate has no dependencies at all, not even onusage-lib.That is the central design decision. The spec declares the settings;
usage-config-build(a later PR) reads theconfigblock at build time and emits a registry of consts; this crate resolves against it. A CLI therefore ships a resolver, not a spec parser, and a registry costs nothing to load.Why this exists
Every CLI in the fleet has written this by hand, and every copy has rotted differently:
sources.clibindings and reads 5 of them; itsvalidate.enumis declared and never read; its docs claim 7 layers and its code has 5.--helpthat does not exist, and has 5 settings that are declared, generated, live in code, and unreachable fromsettings get/set.ifs, scans--offlineout of raw argv, and types Duration as"number"in one generator and"string"in another.None of that is carelessness. It is what happens when the declaration of a setting and the code that resolves it are two separate things kept in step by hand.
What is in it
valueValue(owned, runtime) andConst(const-constructible default) — two types because a generated registry must have no initializer to runtylist_by_commaand friends)registryPropId-interned keys, so a merge over 100 settings never hashes one; rename chains resolve to their endlayerresolvereplace/union/deep— with provenance as its outputProvenance is not a second pass. hk grew a parallel merge function purely to answer "where did this come from", and the two could disagree — so
hk config explaincould describe a resolution that never happened. Here the origin is recorded as the value is chosen, so that class of bug is unreachable.Origincarries the identifier, not just the kind: "from the environment" is not something a user can act on andHK_JOBSis.Custom sources are the normal case, not an escape hatch.
Registry::bindings("git")hands a layer the settings bound to its own kind and the key each has there. usage knows nothing about git, pkl or.npmrc; hk's git layer becomes about twenty lines rather than a second resolution system.Two things live in the merge rather than in layers. Scope (
global,env) is enforced there because mise calls it a security property, and a check every layer has to remember is one a new layer will forget. And a post-merge rewrite goes throughcoerced(), which re-labels the origin — so a value mise derived fromrawnever claims to have come from a file that never said it. That is how a user ends up editing a file with nothing to do with what they are seeing.Nothing prints. Unknown keys, bad values and deprecated settings all come back as warnings. mise queues these until its logging is up, and a library with an opinion about stderr cannot be used by anything that has its own.
Verification
24 tests, including the crate-level doc example so the documented usage is compiled and run. Eight mutations of the merge semantics, each verified to fail without its fix: inverted precedence, scope unenforced,
globalover-refusing the user's own file,unionanddeepeach degraded toreplace, a set that stops deduplicating, and a rewritten value that keeps its old origin. Two of those mutations initially did not apply — the search text had been reflowed bycargo fmt— which is only visible if you assert the match before mutating; the run that reported "ok" was measuring nothing.cargo test --workspace --all-featuresgreen,clippy --all-targets -D warningsclean with noallowanywhere (the one clippy complaint,Origin::defaultshadowing the trait method, is nowOrigin::declared_default— anOriginhas no sensible zero, since every one names a real place).Not in this PR
File layers (TOML/JSON, find-up, trust), the
explainrenderer,usage-config-buildand the typedSettingsstruct, and the config conformance corpus. Each is its own change on top of this one.AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.
Note
Medium Risk
New foundational config resolution with scope/trust rules and extensive merge edge cases; security-sensitive behavior is centralized but will underpin future CLI integration.
Overview
Adds a new
usage-configworkspace crate (zero runtime dependencies) that resolves CLI settings from ordered layers against a build-timeRegistryofPropMetaconsts—no KDL or spec parsing in the hot path.resolveperforms a single merge with fixed precedence (higher layers win), recording provenance (Originwith concrete identifiers likeHK_JOBS) alongside values. Merge modesreplace,union, anddeepare supported; declared defaults seed the lowest contributor so union/deep semantics stay correct.Scope(global,env) is enforced centrally viaTruston origins (including custom sources like git/pkl), not per layer.Layer+LayerCtxgive CLIs a shared path for parsing raw strings with specTy/Parser, unknown keys and bad values as warnings, and rename/deprecation handling.Registry::bindingslets custom layers map external keys without usage knowing the format.Post-merge rewrites go through
Resolved::coercedso explain-style tooling stays honest. WorkspaceCargo.toml/ lockfile register the new member.Reviewed by Cursor Bugbot for commit 82096dc. Bugbot is set up for automated code reviews on this repo. Configure here.