fix(cli): register own flags on commands that have sub-commands - #10622
Draft
luvkapur wants to merge 2 commits into
Draft
fix(cli): register own flags on commands that have sub-commands#10622luvkapur wants to merge 2 commits into
luvkapur wants to merge 2 commits into
Conversation
Running a command that has sub-commands with any of its own flags failed as an unknown argument and printed help instead, e.g. `bit schema <pattern> --json`, `bit lane -d`, `bit lane --not-merged`. The flags were also missing from the command's `--help` output. `parseCommandWithSubCommands` has to override the yargs builder to register the sub-commands, which discards the adapter's builder — the one that declares the command's own flags, positionals and examples. Delegate to it instead of re-implementing part of it. Introduced in #4359 (Commander -> Yargs), where the override dropped the parent's options. It stayed invisible because the adapter's handler reads every key off argv, so unregistered flags still reached the command, until #4429 added `yargs.strict()` two weeks later and they became unknown arguments. #5585 later patched the same override for the global flags only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Registering the parent's flags made them yargs globals, which propagate into sub-command contexts. Where a sub-command declares the same flag name with a different arity the parent's definition shadowed it, so `bit lane remove <lane> --remote --silent --force` failed with "Not enough arguments following: remote" - the parent's `--remote <scope-name>` takes a value while `lane remove --remote` is a boolean. Register a command's own flags with `global: false` when it has sub-commands. Sub-commands never inherited these flags before, since they were not registered at all, so this restores their previous behavior. The global options (log, safe-mode, token, pager) stay global. Caught by e2e lane-export-skip-main-history, which passes locally with this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Running a command that has sub-commands with any of its own flags failed as an unknown argument and printed help instead of running:
Same for
bit lane -d,bit lane --merged,bit lane --not-merged,bit capsuleflags, and so on. The flags were also missing from the command's--helpoutput.Cause
CLIParser.parseCommandWithSubCommandsmust override the yargsbuilderin order to register the sub-commands. That override discardsYargsAdapter.builder, which is what declares the command's own flags, positionals and examples — so underyargs.strict()those flags are unknown arguments.The fix delegates to the adapter's builder instead of re-implementing part of it. That also removes the need for the explicit
getGlobalOptionscall, since the adapter's builder already includes the global options.Scoping the flags to the parent (second commit)
The first commit registered the parent's flags as yargs globals (
.option()defaults toglobal: true), which propagate into sub-command contexts. Where a sub-command declares the same flag name with a different arity, the parent's definition shadowed it:LaneCmdhas--remote <remote-scope-name>(takes a value) whileLaneRemoveCmdhas a boolean--remote. CI'slane-export-skip-main-historye2e caught this.A command's own flags are now registered with
global: falsewhen it has sub-commands. Sub-commands never inherited these flags before — they were not registered at all — so this restores their previous behaviour exactly, while the parent gains its own. The global options (log,safe-mode,token,pager) stay global, which is howbit lane list --log=errorkeeps working.Why
-hkept workingWorth stating, since it makes the bug look narrower than it is.
--help/-his registered inconfigureGlobalFlags()on the root parser with yargs' defaultglobal: true, so it propagates into every command context and never passes through the per-command builder that the override discards.--log,--safe-modeand--tokensurvive for the same reason (re-added inside the override by #5585).On the unfixed build,
bit lane -hshows only those four global flags — the wholeOptionsgroup with-d/--details,-j/--json,-r/--remote,--merged,--not-mergedis absent — whilebit lane -dfails withUnknown argument: d.That globality is the mechanism is confirmed by the counter-example:
versionis registered with an explicitglobal: false, andbit lane --versionis rejected as unknown whilebit --versionworks at top level.-halso escapes strict validation becausesetHelpMiddlewareis registered withapplyBeforeValidation = true, so it prints and exits before validation runs.When it broke
df255d772, 2021-06-02, Commander → Yargs) introduced the override that drops the parent's options. It was latent:YargsAdapter.handlerbuilds its flags from every key onargv, so an unregistered flag still reached the command.5bef11e93, 2021-06-16) addedyargs.strict(), which turned it into a hard failure. This is the commit the breakage becomes visible in, two weeks after the migration.6e70e34b2, 2022-03-22) hit the same override for global flags and patched only those.I confirmed the latency empirically: with
strict()commented out on unfixedmaster,bit schema --jsonreturns valid JSON.Tests
New
cli-parser.spec.ts, 6 cases: the parent's own flags (long form and alias), a parent flag that takes a value, sub-command routing with its own flags, global flags on the parent, and a parent/sub-command flag-name collision with mismatched arity (the CI failure above, reproduced as a unit test).Verified the spec fails only for the right reasons — against unfixed code the two parent-flag cases fail while sub-command routing and global flags still pass.
The spec puts the logger in daemon mode, because a parse failure otherwise calls
process.exit(1)and would kill the whole mocha run instead of reporting a failing test.Verification
npm run lintclean. Afterbit compile teambit.harmony/cli, against the real CLI:bit schema "ui/tooltip" --json→ valid JSONbit lane -d,bit lane --not-merged→ run instead of printing helpbit lane --helpnow lists-d, --details,-j, --json,--merged,--not-mergedbit lane list,bit capsule list,bit aspect list,bit app list,bit envs list,bit schema diff --helpbit lane --remote some-scope→ the parent's value-taking flag still worksbit lane list --log=error→ global flags still reach sub-commandsbit lane remove <lane> --remote --silent --force→ no longer a parse errorbit lane --definitely-not-a-flag→Unknown argumentsThe e2e that caught the sub-command regression,
lane-export-skip-main-history(forked-from lane deleted upstream), passes locally: 2 passing.Note
bit lane --jsonnow parses the flag and reaches the command, which then reportscommand "lane" doesn't implement "json" method—LaneCmddeclares ajsonflag but only implementsreport. That is a separate pre-existing gap inLaneCmd, left alone here.🤖 Generated with Claude Code