fix: subcommand resolution incorrectly consumes flag values#231
Merged
fix: subcommand resolution incorrectly consumes flag values#231
Conversation
When a parent command defines both subCommands and string/enum args,
the subcommand resolver treats flag values as subcommand names. For
example, `--name Citty build` fails because "Citty" is the first
non-dash token and gets interpreted as a subcommand name.
Replace the naive `findIndex(arg => !arg.startsWith("-"))` heuristic
with a helper that uses the parent command's arg definitions to skip
over value tokens consumed by string/enum flags.
Closes unjs#133
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces the previous non-flag heuristic with a new internal helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/command.ts`:
- Around line 21-30: The loop that scans rawArgs (used by runCommand() /
resolveSubCommand()) currently returns the first bare token unconditionally,
causing ambiguity when a parent command defines positional args; update the
logic so that when encountering a bare token it first checks the current
command's subCommands map for that token and only treat it as a subcommand if
present; if the current command declares positional parameters (e.g.,
command.positional or command.positionals length > 0) and the token is not a
known subcommand, treat it as a positional (do not return its index as a
subcommand) or else throw a clear validation error when a command defines both
positionals and subCommands and a token could match both, ensuring valuedFlags
and the existing skip-i logic remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 707119f8-a441-44c9-aed5-adaab6ea9d8d
📒 Files selected for processing (2)
src/command.tstest/main.test.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #231 +/- ##
=======================================
Coverage ? 96.95%
=======================================
Files ? 7
Lines ? 328
Branches ? 123
=======================================
Hits ? 318
Misses ? 9
Partials ? 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/command.ts (1)
92-118: Add explicit regression tests for--end-of-options semantics.
findSubCommandIndexintentionally stops scanning at--; adding tests for this behavior will lock expected dispatch semantics and prevent future regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command.ts` around lines 92 - 118, Add explicit regression tests for the end-of-options semantics in findSubCommandIndex: write unit tests that call findSubCommandIndex with rawArgs arrays that include "--" (e.g. ["--", "subcmd"], flags before then "--", and flags with values before "--") and assert it returns -1 and does not treat tokens after "--" as flags; also add tests showing that valuedFlags handling (the valuedFlags Set built from ArgsDef) still skips flag values before "--" but stops scanning once "--" is encountered. Reference the function name findSubCommandIndex, the rawArgs parameter, and the valuedFlags behavior when creating the tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/command.ts`:
- Around line 92-118: Add explicit regression tests for the end-of-options
semantics in findSubCommandIndex: write unit tests that call findSubCommandIndex
with rawArgs arrays that include "--" (e.g. ["--", "subcmd"], flags before then
"--", and flags with values before "--") and assert it returns -1 and does not
treat tokens after "--" as flags; also add tests showing that valuedFlags
handling (the valuedFlags Set built from ArgsDef) still skips flag values before
"--" but stops scanning once "--" is encountered. Reference the function name
findSubCommandIndex, the rawArgs parameter, and the valuedFlags behavior when
creating the tests.
Replace Set-based precomputation with direct iteration and camelCase normalization. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 15, 2026
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
2 participants
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.
Summary
findSubCommandIndex()that uses arg definitions to skip flag values--name Citty buildwould treat"Citty"as the subcommand name instead of"build"Problem
When a parent command defines
argswithtype: "string"ortype: "enum"alongsidesubCommands, the subcommand resolver usesrawArgs.findIndex(arg => !arg.startsWith("-"))to find the subcommand token. This picks up the flag's value (e.g."Citty"from--name Citty) instead of the actual subcommand name.Closes #133
Solution
Added
findSubCommandIndex()helper insrc/command.tsthat:rawArgs, skipping value tokens consumed by those flagsUpdated both
runCommand()andresolveSubCommand()to use this helper.Test plan
--name Citty build=syntax:--name=Citty build-n Citty build--env prod build--verbose buildresolveSubCommandwith parent string argsSummary by CodeRabbit
Bug Fixes
Tests