fix(clap): say what clap would do with an unknown flag - #899
Conversation
Warning Review limit reached
Next review available in:6 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
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 makes clap-derived specs explicitly serialize clap-like unknown-flag handling at root and subcommand levels.
Confidence Score: 4/5The PR does not yet appear safe to merge because per-argument clap forwarding settings still relax unknown-flag handling for an entire generated command. A command containing any hyphen-enabled or trailing positional receives Files Needing Attention: lib/src/spec/cmd.rs Important Files Changed
Reviews (3): Last reviewed commit: "fix(clap): say what clap would do with a..." | Re-trigger Greptile |
Uh oh!
There was an error while loading. Please reload this page.
| let forwards = cmd.is_allow_external_subcommands_set() | ||
| || cmd | ||
| .get_arguments() | ||
| .any(|arg| arg.is_allow_hyphen_values_set() || arg.is_trailing_var_arg_set()); |
There was a problem hiding this comment.
Trailing arg heuristic too broad
Medium Severity
The clap-to-spec conversion treats any trailing_var_arg as forwarding and sets unknown_flags to value. Clap still rejects unknown dash-words unless allow_hyphen_values is also set, so commands that only use a trailing var arg become looser than clap and miss the typo errors this change aims to restore.
Reviewed by Cursor Bugbot for commit c0daf25. 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
|
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 fabea27. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
A spec generated from clap said nothing about `unknown_flags`, so every command it described fell back to this spec's default — offer the token to the positionals — while clap itself rejects one. mise's spec has 211 commands and not one of them said it, which is why `mise use --globa` is an error under clap and a *tool named `--globa`* under a usage parser. The two defaults differ for a reason and both are right where they are: a spec also describes wrappers, where a dash-word is data in transit. But a CLI generated from clap is not one of those — clap already decided, and dropping the decision quietly loosened the grammar. Which commands forward is clap's own knowledge: an argument that takes hyphen values or a trailing var-arg, or a command that accepts external subcommands. Regenerating mise against this says `error` on 205 commands and `value` on five — `run`, `watch`, `asdf`, `tool-stub`, and `tasks run` — which is the audit, and it agrees with the five files in mise that set those clap attributes. The usage CLI's own spec is a smaller check of the same rule: `bash`, `exec`, `fish` and `complete-word` forward; `generate` does not. The root's own answer had nowhere to go and was being dropped. A spec says this at the top level, which is the field a reader puts it back into, so that is where it goes — mise's root forwards (its implicit task arguments take hyphen values), so it stays permissive, while a CLI whose root does not now gets the strict reading it asked for. What this buys, checked against the regenerated spec: `mise use --globa` now says "Invalid flag `--globa`: no such flag" instead of parsing, and the same for `mise current --forc` and `mise prune --dry-ru`. The forwarding commands are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>


Stack created with GitHub Stacks CLI • Give Feedback 💬
Note
Medium Risk
Changes default parsing semantics for specs generated from clap (stricter unknown-flag handling for most commands), which fixes real CLI bugs but could affect any consumer that relied on the old permissive default.
Overview
Clap → usage spec export now sets
unknown_flagson every command (and on the root spec) instead of leaving it unset, so usage-based parsers match clap’s behavior for unrecognized dash-words.Commands that reject unknown flags get
unknown_flags=error. Commands that forward hyphenated tokens getunknown_flags=value, inferred from clap when external subcommands are allowed or any arg hasallow_hyphen_values/trailing_var_arg.Regenerated
usage.usage.kdl,commands.json, and test snapshots reflect the new metadata. Unit tests cover the inference rules and KDL round-trip serialization.Reviewed by Cursor Bugbot for commit 9fa97a8. Bugbot is set up for automated code reviews on this repo. Configure here.