fix(spec): apply default_subcommand only at the root - #850
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 |
8384add to
85f943d
Compare
85f943d to
363dbd7
Compare
Greptile SummaryThe PR restricts
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (4): Last reviewed commit: "fix(spec): apply default_subcommand only..." | 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
|
85f943d to
6bd131f
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>
6bd131f to
f1c2af9
Compare
A spec declares one
default_subcommand, once, at the top — and the parser looked that nameup 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 zzzdescended intoconfig lsand boundzzzthere.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.cmdsalready records — it holds just the rootuntil 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
Stack created with GitHub Stacks CLI • Give Feedback 💬
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_subcommandis 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-leveldefault_subcommand "ls"madeex config zzzroute intoconfig ls).The discovery-phase guard now requires
out.cmds.len() == 1in addition to the existing once-per-parse flag, so default routing runs only at the root.ex config zzzbecomesunexpected_arginstead of silently descending; root-level routing is unchanged.The corpus case
default-is-declared-for-the-rootdrops 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.Reviewed by Cursor Bugbot for commit f1c2af9. Bugbot is set up for automated code reviews on this repo. Configure here.
What was happening
default_subcommandis a property of theSpec, declared once at the top — there is nosyntax 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 subcommandof the same name silently acquired a default:
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.cmdsalready records — it holdsjust the root until something descends.
Verification
still does. Mutation-checked — with the guard removed it fails.
default_subcommand(explicit selection, same-named child, nested subcommands via mount,and discovery precedence).
Deleting its label was not optional:
reference_labels_are_accuratechecks labels inboth directions, so it failed with
usage-lib now agrees — delete the label. That is themechanism 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.