feat(argv): route a word that names nothing to the default subcommand - #848
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 |
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 a32d361. Configure here.
Greptile SummaryThe PR implements default-subcommand routing in the compiled argv parser and resolves configured defaults against canonical subcommand names and aliases at compile time.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (7): Last reviewed commit: "fix(argv): route only a word, and let an..." | Re-trigger Greptile |
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
|
Two findings on #848, both real and both confirmed against usage-lib. An unrecognised dash-prefixed token becomes a value when `unknown_flags` is `value`, and it was reaching the routing path as though it were a subcommand word — so `ex --wat` descended into the default command instead of binding where it was typed. usage-lib stops looking for subcommands at an unrecognised flag; verified there, `ex --wat` comes back as commands `["ex"]` with `ROOT_TASK = "--wat"`. Only a token that could have named a command routes now, which also excludes the `--` that a `preserve` argument asks for as a value. (Cursor Bugbot.) `find_subcommand` compared names only, so `default_subcommand "r"` pointing at an alias of `run` failed to compile rather than resolving. usage-lib matches names, aliases and hidden aliases alike, so aliases resolve here too. (Greptile.) Three corpus vectors, all agreeing with usage-lib, and both fixes mutation-checked: with the guard removed the unknown-flag test fails, and with the alias lookup removed the alias test fails to compile. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
a32d361 to
83172c4
Compare
|
Addressed in the latest push. Both findings were real, and both confirmed against usage-lib before fixing.
Three corpus vectors, all agreeing with usage-lib, and both fixes mutation-checked. AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
Two findings on #848, both real and both confirmed against usage-lib. An unrecognised dash-prefixed token becomes a value when `unknown_flags` is `value`, and it was reaching the routing path as though it were a subcommand word — so `ex --wat` descended into the default command instead of binding where it was typed. usage-lib stops looking for subcommands at an unrecognised flag; verified there, `ex --wat` comes back as commands `["ex"]` with `ROOT_TASK = "--wat"`. Only a token that could have named a command routes now, which also excludes the `--` that a `preserve` argument asks for as a value. (Cursor Bugbot.) `find_subcommand` compared names only, so `default_subcommand "r"` pointing at an alias of `run` failed to compile rather than resolving. usage-lib matches names, aliases and hidden aliases alike, so aliases resolve here too. (Greptile.) Three corpus vectors, all agreeing with usage-lib, and both fixes mutation-checked: with the guard removed the unknown-flag test fails, and with the alias lookup removed the alias test fails to compile. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
83172c4 to
af67354
Compare
af67354 to
7ebfff5
Compare
af67354 to
d4da025
Compare
`default_subcommand` was emitted into the spec and ignored by the parser, which the previous commit recorded as a gap rather than a decision. usage-lib routes on it, and this is the rule behind `mise build` meaning `mise run build`. A word naming no subcommand now descends into the named one, and the cursor steps back so the word is examined again against the command it reached — which lets it be that command's argument or one of *its* subcommands, without this deciding which and without two events for one word. The declaring command's own positional does not win, which is what makes the property more than a synonym for an argument, and is mise's shape exactly: the root has `[TASK]` and so does `run`. Taken at most once per parse, as usage-lib does, so a chain of defaults cannot walk a CLI deeper than anything the user typed. The name is resolved by `find_subcommand` during const evaluation, so a name no subcommand answers to is a compile error. The previous commit said the name could not be checked because the variants are another expansion; that is true of the macro and not of const eval, which can search the list the parent already holds. Seven corpus vectors, six agreeing with usage-lib and one recording where it does not: usage-lib holds the single name spec-wide and applies it at whichever command it stands on, so `ex config zzz` descends into `config ls` when an unrelated command happens to have a subcommand of that name. Read here as a property of the root, which is the only place a spec can declare it. `default_subcommand` on a nested `Args` is now refused outright instead of being dropped in silence. Both derives share one parse, so the rule moved to where the context is known — before, a nested struct got whichever check ran first, which named the wrong problem. No measurable cost: 40,370 instructions against 40,472, the branch being reached only by a word that matched no subcommand. Allocations unchanged — 0 bare, 4 bound. One thing this does not buy: `mise build` still fails in the shadow, because mise's spec gives `run` no positional — its arguments are cleared and a mount supplies task names, which usage-argv does not execute. Routing plus mounts is what lets mise delete its hand-rolled dispatch; the gate test now says so rather than asserting the old answer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two findings on #848, both real and both confirmed against usage-lib. An unrecognised dash-prefixed token becomes a value when `unknown_flags` is `value`, and it was reaching the routing path as though it were a subcommand word — so `ex --wat` descended into the default command instead of binding where it was typed. usage-lib stops looking for subcommands at an unrecognised flag; verified there, `ex --wat` comes back as commands `["ex"]` with `ROOT_TASK = "--wat"`. Only a token that could have named a command routes now, which also excludes the `--` that a `preserve` argument asks for as a value. (Cursor Bugbot.) `find_subcommand` compared names only, so `default_subcommand "r"` pointing at an alias of `run` failed to compile rather than resolving. usage-lib matches names, aliases and hidden aliases alike, so aliases resolve here too. (Greptile.) Three corpus vectors, all agreeing with usage-lib, and both fixes mutation-checked: with the guard removed the unknown-flag test fails, and with the alias lookup removed the alias test fails to compile. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d4da025 to
46fff42
Compare
A spec declares one `default_subcommand`, once, at the top — and the parser looked that name up at whichever command it happened to be standing on. So a command with an unrelated subcommand of the same name acquired a default of its own: with `default_subcommand "ls"` declared for the program, `ex config zzz` descended into `config ls` and bound `zzz` there. Nothing declared that, and there is no way to declare it, which is what makes it a bug rather than a feature. Gated on still being at the root, which `out.cmds` already records — it holds just the root until something descends. Found while teaching usage-argv to route on the property: the two disagreed, and the corpus recorded it as a divergence pending a decision. jdx's call was to fix it here. The vector that recorded the difference is now an ordinary agreeing one, and deleting its label was not optional — the reference test checks labels in both directions, so it failed with an instruction to remove it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --- <sub>Stack created with <a href="https://github.com/github/gh-stack">GitHub Stacks CLI</a> • <a href="https://gh.io/stacks-feedback">Give Feedback 💬</a></sub> <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes argv routing for nested commands when a subcommand name matches the root default; behavior is now aligned with the spec and corpus, but it is a user-visible parse change for those edge cases. > > **Overview** > **`default_subcommand` is spec-wide and root-only**, but usage-lib was resolving the declared name against whatever command the parser was on. That let nested commands inherit a default when they merely had a subcommand with the same name (e.g. top-level `default_subcommand "ls"` made `ex config zzz` route into `config ls`). > > The discovery-phase guard now requires **`out.cmds.len() == 1`** in addition to the existing once-per-parse flag, so default routing runs only at the root. **`ex config zzz`** becomes **`unexpected_arg`** instead of silently descending; root-level routing is unchanged. > > The corpus case **`default-is-declared-for-the-root`** drops its divergence label and expects agreement. **PLAN.md** records the fix in usage-lib rather than an open decision. A focused unit test covers nested vs root behavior. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit f1c2af9. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> ## What was happening `default_subcommand` is a property of the `Spec`, declared once at the top — there is no syntax for a per-command one. But the parser looked that single name up against `out.cmd`, whichever command it was standing on. So a command with an unrelated subcommand of the same name silently acquired a default: ``` spec: default_subcommand "ls"; cmd ls; cmd config { cmd ls { arg "[WHAT]" } } before: ["config", "zzz"] -> cmds ["ex", "config", "ls"], args [("WHAT", "zzz")] after: ["config", "zzz"] -> error: unexpected word: zzz ``` Nothing declared that behavior and nothing could have, which is what makes it a bug rather than a feature. Gated on still being at the root, which `out.cmds` already records — it holds just the root until something descends. ## Verification - A usage-lib test asserting both halves: the nested command does *not* route, and the root still does. Mutation-checked — with the guard removed it fails. - All 351 existing usage-lib tests still pass, including the five that already covered `default_subcommand` (explicit selection, same-named child, nested subcommands via mount, and discovery precedence). - The corpus vector that recorded this as a divergence is now an ordinary agreeing vector. **Deleting its label was not optional:** `reference_labels_are_accurate` checks labels in both directions, so it failed with `usage-lib now agrees — delete the label`. That is the mechanism working as intended, and it is also the proof the fix took effect. ## Provenance Found while teaching usage-argv to route on this property (#848): the two implementations disagreed, and rather than guess I recorded it as a divergence pending your decision. You said fix it, so this is that fix. *AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.* Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

default_subcommandwas emitted into the spec and ignored by the parser, which theprevious commit recorded as a gap rather than a decision. usage-lib routes on it, and
this is the rule behind
mise buildmeaningmise run build.A word naming no subcommand now descends into the named one, and the cursor steps back so
the word is examined again against the command it reached — which lets it be that
command's argument or one of its subcommands, without this deciding which and without
two events for one word. The declaring command's own positional does not win, which is
what makes the property more than a synonym for an argument, and is mise's shape exactly:
the root has
[TASK]and so doesrun.Taken at most once per parse, as usage-lib does, so a chain of defaults cannot walk a CLI
deeper than anything the user typed.
The name is resolved by
find_subcommandduring const evaluation, so a name nosubcommand answers to is a compile error. The previous commit said the name could not be
checked because the variants are another expansion; that is true of the macro and not of
const eval, which can search the list the parent already holds.
Seven corpus vectors, six agreeing with usage-lib and one recording where it does not:
usage-lib holds the single name spec-wide and applies it at whichever command it stands
on, so
ex config zzzdescends intoconfig lswhen an unrelated command happens tohave a subcommand of that name. Read here as a property of the root, which is the only
place a spec can declare it.
default_subcommandon a nestedArgsis now refused outright instead of being droppedin silence. Both derives share one parse, so the rule moved to where the context is
known — before, a nested struct got whichever check ran first, which named the wrong
problem.
No measurable cost: 40,370 instructions against 40,472, the branch being reached only by
a word that matched no subcommand. Allocations unchanged — 0 bare, 4 bound.
One thing this does not buy:
mise buildstill fails in the shadow, because mise's specgives
runno positional — its arguments are cleared and a mount supplies task names,which usage-argv does not execute. Routing plus mounts is what lets mise delete its
hand-rolled dispatch; the gate test now says so rather than asserting the old answer.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Stack created with GitHub Stacks CLI • Give Feedback 💬
Note
Medium Risk
Changes core subcommand routing and argv binding semantics (default descent, once-per-parse latch, token rewind), which can alter how bare words and edge cases parse across CLIs that declare
default_subcommand.Overview
Implements
default_subcommandin the hot-path parser so a bare word likebuildcan mean “descend intorunand bind there,” matching usage-lib and mise’smise build→mise run buildshape.Commandgainsdefault_subcommandplusfind_subcommand(const-time name/alias lookup, compile error if the name is missing). Inword(), after a failed subcommand match, the parser may descend once per parse (default_taken), rewind the argv cursor, and re-read the same token under the default command. Routing is skipped for flag-like tokens, after--, and when a positional already filled; unknown flags still bind at the current command.Derive and conformance wire the root-level spec property into static tables; nested
default_subcommandonArgsis rejected. Corpus adds09-default-subcommand.json(including one recorded usage-lib divergence: root-only vs spec-wide default). Gate shadow now expectsmise buildto route intorunthen fail withUnexpectedArguntil mounts exist.Reviewed by Cursor Bugbot for commit 46fff42. Bugbot is set up for automated code reviews on this repo. Configure here.
The decision behind this
Investigating
flattenturned up that five of mise's ten#[clap(flatten)]sites are asubcommand field beside a flatten of the same type the
Lsvariant holds —config,tasks,settings,deps,toolall hand-rolling "with no subcommand, behave asls".Which raised the question of what
default_subcommandactually means, and the answer was that#842 got it wrong: usage-lib routes on it while parsing.
Even though the root declares its own
arg "[TASK]". So the property is a binding rule, and#842's test asserting otherwise was corrected in its own commit before this one.
Cost
No measurable cost. The branch is reached only by a word that already failed a subcommand
lookup, and the ±100 is code layout rather than work done — cachegrind is deterministic and
gave the same figure on repeated runs.
The name is now checked at compile time
#842 said "which command it names cannot be checked here — the enum's variants are in another
expansion". True of the macro, not of const evaluation:
find_subcommandsearches the list theparent already holds, so
A wrong name is a compile error rather than a spec property nothing validates.
What this does not buy
mise buildstill does not work end to end in the shadow, and it is worth being plain aboutwhy: mise's spec gives
runno positional at all —src/cli/usage.rsclears them and addsmount run="mise tasks --usage", so task names are meant to come from running that.usage-argv does not execute mounts, so after descending there is nothing to bind to. Routing
plus mounts is what would let mise delete its hand-rolled dispatch; this is half of it. The
gate test now records that instead of asserting the old answer.
Verification
referencelabel is an assertion the reference test enforces in both directions.reached command, the once-per-parse latch, a flag before the word, and nothing routing past
--.four.
rather than a copy.
One divergence, and a question
default-is-declared-for-the-rootrecords it: usage-lib holds the single name for the wholespec and applies it at whichever command it is standing on, so with
default_subcommand "ls"declared at the top,ex config zzzdescends intoconfig ls— an unrelated commandacquiring a default because a name happened to match one level down.
This parser reads it as a property of the root, which is the only place a spec can declare one.
That looks like the intended meaning and usage-lib's looks accidental — but it is your call,
and if you agree it is a small fix in
lib/src/parse.rs. The corpus label makes the differencefail loudly the moment it changes either way.
AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.