feat(complete): complete config keys and values from the spec - #840
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 |
d71dc80 to
575c469
Compare
575c469 to
1b12aa8
Compare
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
|
Greptile SummaryThe PR adds spec-driven completion for configuration keys and values without invoking subprocesses.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (14): Last reviewed commit: "feat(complete): complete config keys and..." | Re-trigger Greptile |
1b12aa8 to
40230a1
Compare
|
Bugbot's finding is right, and it is the loose end I had left in the doc comment rather than fixed:
Getting this verified took two passes worth reporting. My first test used prefixes matching no file in the test's working directory, so the fallback produced nothing anyway and two of three mutations passed against it — the test could not distinguish the behaviours it was written to pin. The prefix is now AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
40230a1 to
f73f481
Compare
|
Both of Greptile's findings were real; fixed in the amended head. The key scan could pick up a flag's value. My reverse scan over raw words could not tell a positional from the value of a flag, so on A union's boolean depended on member order. Both mutation-verified. The second took two tries to verify honestly: my filtered test run ( One knock-on: threading the parse output through pushed AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
f73f481 to
596b77f
Compare
|
Two more findings from the later round, both real and both fixed. A union that also takes free-form values is not a closed set. A declared A description is one row. Choice help was copied whole into the description column while AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
596b77f to
7a79ead
Compare
7a79ead to
b80f737
Compare
b80f737 to
00b1f03
Compare
00b1f03 to
b00446d
Compare
b00446d to
419e4ea
Compare
419e4ea to
0018df1
Compare
|
Right. The reverse scan I put in to fix the flag-value case had the mirror-image problem: on a variadic value argument, the last positional value is one of the values being typed, so The key comes before the values it is given, so the scan now goes forward and takes the first positional that names a setting (and a variadic's first element, not its last). The example spec gained a variadic AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
0018df1 to
e26d3df
Compare
|
Right, and it is the mirror of the last one — which is the signal that both were guesses. Scanning backwards took a variadic's own value; scanning forwards takes an unrelated positional that happens to name a setting. So the guessing is gone: the key is the argument the spec says holds one, the one declared with AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
e26d3df to
44f1f84
Compare
|
Right, and this is the mutation my own check flagged as surviving last round — I judged it untestable because no CLI had two key arguments, which was a statement about my example spec rather than about the code. The nearest key governs now, the same rule as a variadic's last element, and the example gained a two-key $ mycli config move-to color log_level <TAB> → log_level's choices
$ mycli config move-to log_level color <TAB> → color's true/falseMutation-verified: taking the first key again fails. 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 44f1f84. Configure here.
44f1f84 to
b3ee906
Compare
|
Right. Walking past the nearest key when it names no setting offered a different key's values for a line whose own key is a typo — while a single unknown key correctly offers nothing. Only the nearest is consulted now. Worth recording that my first test for this could not have caught it: I used the prefix AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
Two reserved `complete` types, `config_keys` and `config_values`, so a `config get`/`config set` pair completes from the `config` block with no `run=` and no subprocess. Every CLI in the fleet writes this by hand today as a shell command that asks the binary for its own settings list; the spec already says what the keys are and what each one accepts. `config_keys` offers every prop, dotted keys and all, with its help as the description. `hide` filters here — which is exactly where it belongs, and the difference from the JSON schema: a hidden setting is still settable, so a schema must accept it, but nothing should suggest it. A deprecated setting is still offered, because a config file in the wild names it and a user has to be able to complete what they are removing, with its description saying so. `config_values` looks back along the line for the last word naming a setting, rather than assuming the key sits next to the cursor — `config set --global log_level <TAB>` and `config set log_level --global <TAB>` both work. It offers the setting's choices with their own help, or true/false for a boolean (including `option<bool>` and the boolean side of a union). For anything else it says nothing, which lets the existing file fallback do the obvious thing for a path-valued setting. `complete`'s reference page never documented `type=` at all, so the built-in `file`/`path`/`dir` are written down here too. Four tests over a new `examples/config.usage.kdl`, each mutation-checked. One of those mutations survived at first — the backward scan was never exercised because the flag in the test came *before* the key — so the case with a flag between the key and the cursor was added, which is the one that needs it.
b3ee906 to
748770c
Compare
|
Investigated this one and did not add the guard, because I could not make the situation happen. The concern is that a $ mycli config with-default-key log_level <TAB>
error only failures
warn failures and warnings
info the usual
debug every decision # identical either wayA partial parse does not bind an argument from its default, so the filter changed nothing. Unreachable defensive code in a completion path is worse than the case it defends against — it is the same reason I removed a collision guard from the JSON schema generator earlier in this stack when its mutation proved unreachable. What I did add is that spec and a test asserting the typed key governs. It passes today by construction; it is there so that if a partial parse ever starts filling defaults, this fails rather than silently offering values for a setting nobody wrote. The comment at the call site records the measurement so the next reader does not have to redo it. AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |

Two reserved
completetypes —config_keysandconfig_values— so aconfig get/config setpair completes from the spec'sconfigblock. Fifth and last PR of stack #836 on the spec side; depends on the vocabulary in #835.Every CLI in the fleet writes this by hand today as a
run=that shells out to the binary for its own settings list. The spec already says what the keys are and what each accepts, so the completion needs no subprocess:config_keysEvery prop, dotted keys and all, with its
helpas the description.hidefilters here — which is where it belongs, and the difference from the JSON schema in #839. A hidden setting is still settable, so a schema that rejected it would be wrong; but nothing should suggest it. Same declaration, opposite treatment, for reasons specific to each consumer.A deprecated setting is still offered, with its description saying so. A config file in the wild names it, and a user has to be able to complete the key they are removing.
config_valuesLooks back along the command line for the last word that names a setting, rather than assuming the key sits next to the cursor — where the key sits is the CLI's business:
Offers that setting's
choiceswith their own help, ortrue/falsefor a boolean — includingoption<bool>and the boolean side of abool|stringunion, via the type grammar'ssimplified(). For anything else it says nothing, which lets the existing file fallback do the obvious thing for a path-valued setting rather than reimplementing path completion.Also
complete's reference page never documentedtype=at all, so the built-infile/path/dirare written down alongside the two new ones.Verification
Four tests over a new
examples/config.usage.kdl, each mutation-checked. Worth reporting: the backward-scan mutation survived the first time — my test put the flag before the key, so the word next to the cursor was the key anyway and the scan was never exercised. Rather than accept a passing test, I added the case with a flag between the key and the cursor, which is the one that needs the scan, and re-ran the mutation to confirm it now fails.cargo test --workspace --all-featuresgreen,clippy --all-targets -D warningsclean,cargo fmt, prettier,mise run renderapplied.AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.
Note
Low Risk
Changes are localized to
complete_wordcompletion behavior plus docs/tests; closed-vs-open fallback is the main behavioral shift beyond new config completers.Overview
Adds reserved
completetypesconfig_keysandconfig_valuessoconfig get/setcan complete from the spec’sconfigblock without arun=subprocess.config_keyslists visiblepropkeys (respectinghide, markingdeprecated) with one-line help.config_valuesresolves the active key from partial-parse bindings on the argument wired toconfig_keys(not raw word scans), then offerschoiceshelp, booleans (including unions/options viaholds_a_boolean/accepts_unenumerable_values), or defers to path/file fallback when values aren’t enumerable.Completion plumbing now threads a
Ctx(tera + spec + parse output), and builtins/args return a closed flag so enumerated completers suppress the working-directory file fallback on no match—whilebool|path-style unions stay open so paths still complete.Docs cover built-in
type=values;examples/config.usage.kdland integration tests exercise the edge cases.Reviewed by Cursor Bugbot for commit 748770c. Bugbot is set up for automated code reviews on this repo. Configure here.