Repository navigation
fix(parse): let a bundle contain a supplied short - #1175
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 |
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
|
e649dbf to
a1fb474
Compare
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 a1fb474. Configure here.
| let is_bundle = | ||
| word.starts_with("--") || short_bundle_is_known(&out.available_flags, &word); | ||
| let is_bundle = word.starts_with("--") | ||
| || short_bundle_is_known(spec, &out.cmds, &out.available_flags, &word); |
There was a problem hiding this comment.
Bundled help/version skips past subcommands
High Severity
Phase 1 now treats a short bundle containing a supplied -h or -V as a known flag and keeps scanning for subcommands. A following subcommand is selected before Phase 2 peels the token, so ex -vh run answers with run's help instead of the root's, and ex -vV run fails the root-only version check and surfaces a stray word instead of the version. Whole-token -h/-V and first-letter forms like -hv/-Vv still stop the scan, so the same argv disagrees with itself and with one-pass parsers.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a1fb474. Configure here.
a1fb474 to
0c02861
Compare
`-vh` was refused as an unknown word wherever `-h` was not declared, because `short_bundle_is_known` asked only the declared flags and a token holding an unrecognized letter is not a bundle at all. But `-h` *is* recognized — the parser supplies it, as it supplies `-V` on a root that declares a version — so the token was a bundle and the rule was reading it against the wrong set. usage-argv and usage-go both resolve the letter through the same lookup that finds a declared short, and clap prints help for `-vh` too. usage-lib is the implementation the corpus measures the others against, and it was the one that disagreed — the same shape as the `--version` divergence in the commit below, and unseen for the same reason: the binding corpus has no vocabulary for an invocation that prints and exits, so nothing measures these. The letter is answered wherever it sits, so `-hv` asks for help as surely as `-vh` does; neither reached the whole-token spellings that handled `-h` alone. Always the short response — `-h` is short help however many letters share its token, and `-V` the concise version — with the long forms left to the long spellings. `-?` stays a whole-token spelling rather than a letter, and `-V` keeps its root-only rule. A spec that declares the letter keeps it: nothing is supplied where the CLI spent it, so a `-h` meaning `--host` still reads `-vhlocal` as its own. And a letter nothing supplies still refuses the whole bundle, which is the rule this must not weaken: `-az` sets nothing. The grammar now says so, in the section that states the bundle rule. It said nothing about supplied letters at all, which is what left three implementations agreeing by coincidence and one disagreeing without anything noticing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0c02861 to
13b278a
Compare


Follow-up to the
-vhquestion raised in review on #1168. Stacked on that branch, because the-Vhalf needs the supplied version flag that lands there; the diff here is one commit.The bug
-vhwas refused as an unknown word wherever-hwas not declared:short_bundle_is_knownasks whether every letter names a declared flag, on the grammar's rule that a token containing an unrecognized letter is not a bundle at all. But-his recognized — the parser supplies it, as it supplies-Von a root that declares a version — so the token was a bundle and the check was reading it against the wrong set. The peeling machinery below it was always fine:-vapplies and pushes-hback, which the whole-token spelling then answers. It simply never got there.Who else was already right
-vhunexpected word: -vhBoth other implementations resolve the letter through the same lookup that finds a declared short (
find_shortinargv/src/lib.rs,findShortingo/argv/parser.go, each falling back to a suppliedHELP_SHORT/VERSION_SHORT). usage-lib is the implementation the corpus measures the others against, and it was the one that disagreed — the same shape as the--versiondivergence in the commit below it, and unseen for the same reason: the binding corpus has no vocabulary for an invocation that prints and exits, so nothing measures these at all.What changed
short_bundle_is_knowncounts a supplied-hor-Vas a recognized letter, under exactly the conditionsis_help_argandis_version_argalready state — asked one letter at a time, because a bundle is read one letter at a time.-hvasks for help as surely as-vhdoes. Neither reached the whole-token spellings that handled-halone.-his short help however many letters share its token, and-Vthe concise version. The long forms belong to the long spellings.-?stays a whole-token spelling rather than a letter anyone bundles, and-Vkeeps its root-only rule —demo run -vVis still refused.Two rules deliberately unweakened, both tested:
-hmeaning--hoststill reads-vhlocalas a bundle and its value.-azsets nothing on the way to discovering thatznames nothing, which is what the corpus pins inshort-bundle-unknown-letter-applies-nothing.disable_help_flagtakes the letter back out, and that is tested too.The grammar
docs/spec/argv.mdsaid nothing about supplied letters anywhere — which is what left three implementations agreeing by coincidence and one disagreeing without anything noticing. The section that states the bundle rule now states this one beside it.Observed, not fixed
While testing value-taking shorts:
-jhon a spec declaring-j <n>errors withInvalid flag --jobs: requires an argumentin usage-lib, while usage-argv bindsjobs = "h"— which is what the grammar's own table says (-j8takes the rest of the token). Present onmain, unchanged by this PR, and a different mechanism: the pushed-back remainder is refused as a pending value for looking flag-like. Worth its own change.Testing
Six cases in
lib/src/parse.rscovering both letters, both orders, the short-page rule, the root-only rule for-V, a declared letter winning,disable_help_flag, and the unchanged-az.cargo test --all --all-featurespasses (1,990), clippy and prettier clean.🤖 Generated with Claude Code
Note
Medium Risk
Changes argv binding for help/version shorts, which is user-facing CLI behavior, but the rules are narrow and covered by tests.
Overview
Treats parser-supplied
-hand-Vas recognized short letters so tokens like-vhand-hvare real bundles that print help or version, matching usage-argv, usage-go, and clap.short_bundle_is_knownnow counts those letters under the same conditions asis_help_arg/is_version_arg. The letter is answered wherever it sits, always as the short help page or concise version. A declared-hstill wins (-vhlocalas--host), unknown letters still refuse the whole token (-az), anddisable_help_flag/ root-only-Vstay as they were.The argv grammar now states this beside the bundle rule.
Reviewed by Cursor Bugbot for commit 13b278a. Bugbot is set up for automated code reviews on this repo. Configure here.