docs(config): write down what a resolution does - #882
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 a normative configuration-resolution specification, links it from the documentation and corpus README, and expands conformance coverage. The rename-chain addition still leaves one previously identified normative behavior uncovered.
Confidence Score: 4/5The PR is not yet safe to merge because the attempted corpus-coverage fix still permits implementations to omit required intermediate rename-chain deprecation warnings. The reply shown without an author says the missing rename-chain vector was added, but the concrete counterexample remains: the only new chain has no intermediate deprecation metadata and therefore cannot enforce the normative warning behavior. Files Needing Attention: corpus/config/04-renames.kdl, docs/spec/resolution.md Important Files Changed
Reviews (3): Last reviewed commit: "docs(config): say which rules the corpus..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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
|
c71a106 to
14479f7
Compare
The `config` block reference says how a CLI *declares* settings. What happens to them was written down nowhere: every rule about precedence, coercion, scope, renames and warnings existed only as the merge's implementation and the corpus that pins it — so the corpus README had to say "these vectors are the definition, which is the wrong way round". Now it is prose first and vectors second, the way the argv grammar and its corpus sit together, and the page is normative: an implementation in Go or TypeScript has something to read before it starts. Includes the things a reader would otherwise have to infer from a test: that a declared default is the bottom layer rather than a floor, that a `union` list keeps duplicates and a `set` drops them, that an empty value is a value, that a refused value costs its own key, and that scope asks how far a place is trusted rather than what kind of place it is.
…e rest "Every rule on this page has vectors" was not true. Five boolean spellings, the `set_by_comma` parser and a chain of two renames had none, so an implementation could pass every vector and still read `y` as false. Those now have vectors. Two rules still have none, both because a vector says what a resolution *produces* and neither of these is one: `list_by_os_path_separator` splits on a character that depends on the machine, and a corpus whose answers differ by platform stops being the definition of correct; `not-read` is a layer's choice between two of its own variables, which a vector describing what a layer supplies cannot express. Said on the page rather than left to a reader to discover by diffing.
14479f7 to
216d124
Compare
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
| setting "jobs" type="uint" default=1 | ||
| setting "concurrency" type="uint" renamed-to="jobs" | ||
| setting "threads" type="uint" renamed-to="concurrency" |
There was a problem hiding this comment.
When an intermediate alias carries the first deprecation notice, this vector tests only terminal resolution and a renamed warning. The corpus therefore accepts implementations that omit the required intermediate-chain deprecated warning, including the current resolution path that checks only the exact written alias.
Knowledge Base Used: Configuration registries and layered resolution
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Correct at the time, and closed by #893, which merged alongside this one — the vector you are describing was what found the bug.
On main now:
vector "a-notice-anywhere-along-a-chain-is-reported" {
setting "jobs" type="uint" default=1
setting "concurrency" type="uint" renamed-to="jobs" deprecated="Use jobs instead."
setting "threads" type="uint" renamed-to="concurrency"
layer "file" id="hk.toml" { value "threads" "8" }
expect { value "jobs" 8; warning "deprecated"; warning "renamed" }
}And you had the resolution path right too: it checked the exact written alias while explain walked the chain, so config explain threads told a user to stop using a key that running the CLI said nothing about. There is one walker now, Registry::deprecation, called by both.
AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.
…hain (#893) A bug found by writing the corpus vector for a rule [#882](#882) already stated on the page. `explain` walked the chain of renames looking for a deprecation notice — `a` renamed to `b`, and `b` the one carrying the notice that says to use `c` — with a comment explaining why. The merge read the notice off the declaration the user wrote. So: ``` $ hk config explain threads threads = 8 deprecated: Use jobs instead. $ hk check # nothing at all ``` The quieter of the two is the one a CLI actually surfaces, so the user is told to stop using the key only if they happen to ask about it. Same shape as most of this stack's findings: one rule, two implementations. Fixed by moving the walker onto `Registry` and having both callers ask it, rather than teaching the second one to agree. The message still names the key that was written — `threads is deprecated: Use jobs instead.` — because that is the line in the file they would go and edit, not the one the notice happens to be attached to. ## Verification A test at the merge and the corpus vector that found it. Mutating the walk back to "read the declaration written" fails both. *AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.* <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Targeted behavior fix in config resolution and explain output; covered by new tests and corpus vectors, with no auth or security surface. > > **Overview** > **Unifies deprecation discovery** so `config explain` and config merge behave the same when a notice sits on an intermediate rename (e.g. `threads` → `concurrency` → `jobs`, with deprecation on `concurrency`). > > The rename-chain walker moves from a private helper in `explain` to **`Registry::deprecation`**. **`explain`** and **`resolve`** both call it. Merge no longer reads `deprecated` only off the declaration for the key the user wrote, which meant CLIs could stay silent while `config explain` still showed a deprecation. > > Warnings still use the **written** key (`threads is deprecated: …`) so messages point at the line in the file users would edit. > > Adds a merge unit test and corpus vector **`a-notice-anywhere-along-a-chain-is-reported`** in `04-renames.kdl`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit a09162b. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 participant
The
configblock reference says how a CLI declaressettings. What happens to them was written down nowhere — precedence, coercion, scope, renames,
warnings existed only as the merge's implementation and the corpus that pins it. Which is why the
corpus README I wrote in #875 had to say, in as many words, "these vectors are the definition, which
is the wrong way round".
Now it is prose first and vectors second, the way
/spec/argvand the argv corpus sit together, andthe page is normative rather than descriptive: an implementation in Go or TypeScript has something to
read before it starts, instead of reverse-engineering 46 JSON vectors.
It states the things a reader would otherwise have to infer from a test:
unionlist with a default and one file gets both
unionconcatenates, so alistkeeps duplicates and asetdrops them, because that is whatdistinguishes the two
HK_EXCLUDE=is how a default gets turned offis as much a thing a repository carries as a TOML file
MISE_JOBS,--jobs,./hk.toml) and never the kind, because a user whowants a different answer needs to know what to unset
The corpus README now points at it instead of standing in for it, and it is in the sidebar under the
argv grammar.
AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.
Note
Low Risk
Documentation and conformance vectors only; no resolver or library code changes in the diff.
Overview
Adds a normative config resolution spec at
docs/spec/resolution.md(precedence, merging, types, scope, renames, warnings, corpus relationship) and links it from the VitePress sidebar next to the argv grammar.Updates
corpus/config/README.mdso vectors are explicitly secondary to that prose. Corpus gains coverage for multi-hop rename chains (threads→concurrency→jobs), expanded boolean env spellings (y,on,n, empty → false), andset_by_commaonset<string>with deduplication.Reviewed by Cursor Bugbot for commit 216d124. Bugbot is set up for automated code reviews on this repo. Configure here.