feat(config): explain why a setting has the value it has - #857
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 SummaryAdds configuration explanation, warning, and listing renderers backed by the existing resolution provenance.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (15): Last reviewed commit: "feat(config): explain why a setting has ..." | Re-trigger Greptile |
| .map(|(kind, key)| format!("{kind} {key}")) | ||
| .collect(); | ||
| let _ = writeln!(out, " also {}", bindings.join(", ")); | ||
| } |
There was a problem hiding this comment.
Bindings label assumes environment section
Low Severity
The bindings line is labeled also and has no leading blank line of its own, so it only reads as a continuation of environment. A setting with git or pkl bindings but no env vars gets a dangling also jammed against the type or help text.
Reviewed by Cursor Bugbot for commit 982b919. Configure here.
982b919 to
3b5aca5
Compare
|
Both real, and both about output that lies about its own shape. A newline in a value broke the record boundary. Both renderers here are line-oriented — one setting per line for a listing, one fact per line for an explanation — and a multi-line string is a perfectly ordinary thing to put in a TOML file. Its continuation read as another setting, or as provenance. Values now render with newlines (and carriage returns, and the backslashes that would be ambiguous) escaped. A setting with bindings and no environment variable had a dangling Three mutations, each verified. The listing test asserts the line count as well as the content, so a value that splits into two records fails rather than merely looking odd. AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
3b5aca5 to
3cfb1dd
Compare
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
Right, and the fix belongs on the message rather than the renderer: a type error named the place in its own text and the The message now says what is wrong and the renderer says where, which also means every warning renders the same way instead of two kinds needing two treatments. Test asserts the place appears exactly once; mutation-verified by putting it back. AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
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
|
a57f88a to
7612b38
Compare
|
Both right, and both the same class as the value fix — applied to the other two things interpolated into a line-oriented output. An origin can carry a newline (a path may contain one), and a warning message quotes the value it rejected, which comes out of a file and can hold anything. Either one splitting its line makes the remainder read as another record; for warnings it hides every warning after it. So the escaping moved off the value and onto the shared helper, and all three go through it. Two things about the verification. My first test covered only one of the two places an origin is rendered — the winner's line and the also-considered list are different branches — and the mutation for the other survived; both paths now carry a newline in the test. And I pushed once with a clippy failure in between (an import left unused by the change), because my gate command let the push run after the check reported. Caught on the next pass and fixed before this push; the branch is clean now. AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
7612b38 to
769addb
Compare
|
Right, and worse than the problem it was solving: Test asserts a Windows path round-trips as written, and the mutation that reintroduces backslash doubling fails. AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
769addb to
8f2465c
Compare
|
Right — same class again, and this time for the string most likely to contain a newline: an adopter's help is a doc comment, and a doc comment with a second paragraph is the ordinary case rather than an exotic one. Help, the deprecation notice, and the joined environment and binding lists all go through the helper now. The test asserts the line count of a whole explanation rather than only the content, so anything splitting a record fails whether or not I thought to look for it. Both mutations verified. That is four rounds on one small renderer, each finding a different string I had not routed through the escaping — which is a fair signal that the helper should have been applied at the point where a line is written rather than at each interpolation. If a fifth turns up I will restructure it that way instead of patching another call site. AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
8f2465c to
95ef048
Compare
95ef048 to
11e4bbb
Compare
11e4bbb to
c0fb90b
Compare
c0fb90b to
5f217e0
Compare
5f217e0 to
2329e95
Compare
2329e95 to
da190e9
Compare
|
Right, and it is the shape I only half-handled. Reading the notice off the replacement printed
Also in this commit: 79 tests. Mutation: restoring the old AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
da190e9 to
c920d2e
Compare
|
One real, one I am going to leave as it is with the reasoning. The padding was aligning a column that does not exist. Real, and worse than trailing The 79 tests. Mutation: restoring the padding fails the new assertion. AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
c920d2e to
ff67378
Compare
|
Right, and my own no-trailing-space assertion from the last round did not catch it — the fixture
The rule is one helper, The same fix lands in #862 for the reader's own errors, since that module quotes values the same way 80 tests. Mutation: returning the value's text unconditionally fails the new test, which covers the 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.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ff67378. Configure here.
hk's `config explain` is the best of its kind in the fleet: it names the winning
source and the exact identifier, and does per-item provenance for lists. It also
costs about two hundred lines and is written against a *second* merge function
that exists only to answer this question — two merges can disagree, and when they
do the explanation describes a resolution that never happened.
Here there is one merge and its provenance is the answer, so this is a renderer
and nothing more. Every adopter gets the command; none of them writes it.
jobs = 8
set by HK_JOBS
type uint
How many jobs to run at once
also considered, lowest precedence first:
the default
hk.toml#jobs
environment HK_JOBS, HK_JOB
also git hk.jobs
Four things the wording is careful about, each of which sends a user to the wrong
place if it is careless:
- A declared default is not something anybody *set*, and a post-merge rewrite is
not either — `default` and `derived` rather than `set by`, so nobody goes
looking for a file that never said it.
- The winner is not repeated among the things it beat.
- Asking after an old name answers about both names, and reads the deprecation
notice from the declaration that *has* one — the old one. Reading it off the
setting that replaced it printed nothing for the only case where it matters.
- The type is shown as the spec spells it (`uint`, `list<string>`) rather than as
the prose an error message uses; a reader searching the docs for "a positive
integer" finds nothing. That is a new `Ty::name`.
Also `warnings`, which pairs each message with the place that caused it, and
`list` for a `config ls` — sorted by key, since a registry's order is the order
somebody wrote a TOML file in, and without hidden settings or old names, which
are documented nowhere and would surface here for the first time.
Plain text on purpose: a CLI that wants JSON has the same `Resolved` and better
taste than a library about what its own output should look like.
Eight tests, five mutations, each verified to fail without its fix.
ff67378 to
59e00a7
Compare
|
Right, and worse than the finding says: read from the text, a one-item list holding the empty Emptiness is now asked of the value, not of its rendering. And the underlying problem is that a list A map keeps its plain form even when a value is empty, because its keys are in the text ( Two mutations: not writing out a list whose items would vanish, and not marking the empty list, each AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |


config explainfor every adopter, as a renderer rather than a reimplementation. Second PR of stack #858; depends on #856 only for sitting on top of it.hk's version is the best of its kind in the fleet — it names the winning source and the exact identifier, and does per-item provenance for lists. It also costs about two hundred lines, and it is written against a second merge function that exists only to answer this question. Two merges can disagree, and when they do the explanation describes a resolution that never happened. Here there is one merge and its provenance is the answer, so this module reads what the merge recorded and formats it.
What the wording is careful about
Each of these sends a user to the wrong place if it is careless:
defaultandderivedrather thanset by. mise'srawimplyingjobs = 1must not read as though a file said so, or the user goes looking for the file.uint,list<string>), not the prose an error message uses. A reader searching the docs for "a positive integer" finds nothing. That is a newTy::name, distinct from the existingdescribe.Also
warnings, pairing each message with the place that caused it, andlistfor aconfig ls— sorted by key, because a registry's order is the order somebody wrote a TOML file in, and excluding hidden settings and old names, which are documented nowhere and would surface here for the first time.Plain text on purpose: a CLI that wants JSON has the same
Resolvedthis reads, and better taste than a library about what its own output should look like.Eight tests, five mutations — the verbs, the winner appearing among the also-considereds, the deprecation source, hidden settings in the listing, and the sort — each verified to fail without its fix.
AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.
Note
Low Risk
User-facing text and warning formatting only; resolution merge logic is unchanged aside from clearer type-error strings.
Overview
Adds shared
config explainrendering on top of existing merge provenance inResolved—no second merge path. Newexplain,warnings, andlistformat plain text for one key, resolution warnings, andconfig ls(sorted keys, hidden/renamed keys omitted).explainprints the winning value withshown, provenance verbs (default/derived/set by), spec types via newTy::name, lower-precedence contributors, env/bindings hints, and deprecation along rename chains.Supporting changes:
one_line/showninvalue.rskeep line-oriented output safe (newlines, empty[]/""/{}); layer type-coercion warnings drop duplicated origin text soexplain::warningscan append(origin)once;Tycoercion errors quote values withshown.Reviewed by Cursor Bugbot for commit 59e00a7. Bugbot is set up for automated code reviews on this repo. Configure here.