fix(#499): Parse arguments for lazy loaded command objects#500
fix(#499): Parse arguments for lazy loaded command objects#500cameronhunter wants to merge 1 commit intokazupon:mainfrom
Conversation
📝 WalkthroughWalkthroughChanges modify the CLI argument parsing flow to resolve lazy commands earlier, before argument extraction, and to apply level-specific subCommands overrides earlier in the sequence. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 2
🧹 Nitpick comments (1)
packages/gunshi/src/cli/core.ts (1)
139-149:isLazyCommandbranch ingetCommandArgsis now unreachable from its only call site.After the eager-resolution change,
getCommandArgsis always called with a fully resolvedCommand<G>. TheisLazyCommandguard (Line 142) will never be true. You could simplify to only handle theobjectand fallthrough cases, or removeLazyCommand<G>from the parameter union.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gunshi/src/cli/core.ts` around lines 139 - 149, getCommandArgs currently includes an unreachable isLazyCommand<G> branch because callers now always pass a resolved Command<G>; remove LazyCommand<G> from the function signature and delete the isLazyCommand branch so the function simply checks if cmd is an object and returns cmd.args || create<ExtractArgs<G>>(), otherwise returns create<ExtractArgs<G>>(); update the signature to getCommandArgs<G extends GunshiParamsConstraint>(cmd?: Command<G>) to reflect the narrower type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/gunshi/src/cli/core.ts`:
- Around line 90-94: When calling resolveArgs in cli/core.ts, ensure the lazy
command's toKebab hint is preserved by copying it from the original LazyCommand
into resolvedCommand if missing; specifically, after resolveLazyCommand returns,
check resolvedCommand.toKebab and if undefined but the original command (the
LazyCommand passed into resolveLazyCommand) has command.toKebab set (e.g. set
via lazy(loader, { toKebab: true })), assign that value onto
resolvedCommand.toKebab before using resolvedCommand in resolveArgs — this
avoids changing utils.ts and fixes the kebab-casing regression for lazy
commands.
- Around line 74-83: When resolving lazy commands the toKebab flag is not
preserved; update the lazy-resolution code so the resolved Command includes
toKebab copied from the lazy definition: in the baseCommand construction (where
baseCommand is created from the lazy cmd) add toKebab: cmd.toKebab, and in the
needRunResolving/loaded branch (after resolveLazyCommand returns loaded) include
toKebab: loaded.toKebab when building the final resolvedCommand so toKebab flows
through isLazyCommand/resolveLazyCommand paths.
---
Nitpick comments:
In `@packages/gunshi/src/cli/core.ts`:
- Around line 139-149: getCommandArgs currently includes an unreachable
isLazyCommand<G> branch because callers now always pass a resolved Command<G>;
remove LazyCommand<G> from the function signature and delete the isLazyCommand
branch so the function simply checks if cmd is an object and returns cmd.args ||
create<ExtractArgs<G>>(), otherwise returns create<ExtractArgs<G>>(); update the
signature to getCommandArgs<G extends GunshiParamsConstraint>(cmd?: Command<G>)
to reflect the narrower type.
| // override subCommands with level-specific sub-commands for rendering | ||
| if (levelSubCommands) { | ||
| cliOptions.subCommands = levelSubCommands | ||
| } | ||
|
|
||
| // resolve lazy commands before parsing so loader-defined args are available | ||
| const resolvedCommand = isLazyCommand<G>(command) | ||
| ? await resolveLazyCommand<G>(command, name, true) | ||
| : command | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the code context around lines 74-83
cat -n packages/gunshi/src/cli/core.ts | sed -n '65,90p'Repository: kazupon/gunshi
Length of output: 1277
🏁 Script executed:
# Search for playground examples that test lazy commands
find . -type f \( -name "*playground*" -o -name "*example*" -o -name "*demo*" \) | grep -E "\.(ts|js|tsx|jsx)$"Repository: kazupon/gunshi
Length of output: 429
🏁 Script executed:
# Search for lazy command usage patterns in the codebase
rg -n --type=ts 'resolveLazyCommand|isLazyCommand' --max-count=20Repository: kazupon/gunshi
Length of output: 1535
🏁 Script executed:
# Check if there are test files that cover lazy commands
find . -type f -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "lazy" 2>/dev/null | head -10Repository: kazupon/gunshi
Length of output: 217
🏁 Script executed:
# Check the playground test file for lazy command examples
cat -n e2e/playground.spec.ts | head -100Repository: kazupon/gunshi
Length of output: 2289
🏁 Script executed:
# Look at the resolveLazyCommand implementation to understand toKebab handling
cat -n packages/gunshi/src/utils.ts | sed -n '22,70p'Repository: kazupon/gunshi
Length of output: 2200
🏁 Script executed:
# Search for toKebab usage in the codebase to understand if it's affected by lazy resolution
rg -n 'toKebab' --type=ts packages/gunshi/src/Repository: kazupon/gunshi
Length of output: 1497
🏁 Script executed:
# Get the complete resolveLazyCommand implementation
cat -n packages/gunshi/src/utils.ts | sed -n '36,100p'Repository: kazupon/gunshi
Length of output: 2612
🏁 Script executed:
# Check the Command type definition to see what properties should be copied
cat -n packages/gunshi/src/types.ts | sed -n '540,580p'Repository: kazupon/gunshi
Length of output: 1336
🏁 Script executed:
# Check what happens at line 92 where toKebab is used
cat -n packages/gunshi/src/cli/core.ts | sed -n '85,100p'Repository: kazupon/gunshi
Length of output: 712
🏁 Script executed:
# Find how toKebab is handled in the definition
cat -n packages/gunshi/src/definition.ts | sed -n '280,295p'Repository: kazupon/gunshi
Length of output: 789
🏁 Script executed:
# Search for playground examples that use lazy commands
find e2e/fixture -name "config.json" -exec grep -l "lazy" {} \; | xargs dirnameRepository: kazupon/gunshi
Length of output: 127
🏁 Script executed:
# Check if any playground examples have toKebab in their definition
find playground -type f -name "*.ts" | xargs grep -l "toKebab" 2>/dev/nullRepository: kazupon/gunshi
Length of output: 42
🏁 Script executed:
# Look for test cases covering lazy commands with toKebab
grep -r "toKebab.*lazy\|lazy.*toKebab" packages/gunshi/src --type=tsRepository: kazupon/gunshi
Length of output: 178
🏁 Script executed:
# Check the cli.test.ts to see if there are existing tests for lazy commands and toKebab
cat -n packages/gunshi/src/cli.test.ts | sed -n '1095,1145p'Repository: kazupon/gunshi
Length of output: 1390
🏁 Script executed:
# Search for where toKebab is handled in lazy command resolution more carefully
rg -A10 -B5 "resolveLazyCommand" packages/gunshi/src/utils.ts | head -80Repository: kazupon/gunshi
Length of output: 764
🏁 Script executed:
# Check if toKebab is supposed to be copied from the lazy command definition
cat -n packages/gunshi/src/utils.ts | sed -n '36,90p'Repository: kazupon/gunshi
Length of output: 2175
🏁 Script executed:
# Look at what properties are in LazyCommand type
rg -B5 -A20 "type LazyCommand" packages/gunshi/src/types.ts | head -40Repository: kazupon/gunshi
Length of output: 932
🏁 Script executed:
# Check if there are any tests for lazy commands with toKebab
rg -n "lazy.*toKebab|toKebab.*lazy" packages/gunshi/src/cli.test.tsRepository: kazupon/gunshi
Length of output: 40
🏁 Script executed:
# Verify what happens when toKebab is set on a lazy command
cat -n packages/gunshi/src/definition.ts | sed -n '275,310p'Repository: kazupon/gunshi
Length of output: 1326
🏁 Script executed:
# Check if toKebab should be in baseCommand when resolving lazy commands
cat -n packages/gunshi/src/types.ts | sed -n '400,440p'Repository: kazupon/gunshi
Length of output: 1676
🏁 Script executed:
# Look for existing tests with lazy commands to understand expected behavior
rg -B5 -A15 "lazyCommand\|lazy.*toKebab" packages/gunshi/src/definition.test.ts | head -80Repository: kazupon/gunshi
Length of output: 40
🏁 Script executed:
# Verify the exact issue: toKebab is NOT in the baseCommand properties copied from lazy command
cat -n packages/gunshi/src/utils.ts | sed -n '42,56p'Repository: kazupon/gunshi
Length of output: 613
🏁 Script executed:
# Check if toKebab might be copied in the needRunResolving block
cat -n packages/gunshi/src/utils.ts | sed -n '57,79p'Repository: kazupon/gunshi
Length of output: 1105
🏁 Script executed:
# Check definition.test.ts to see if there are tests verifying toKebab on lazy commands
rg -B3 -A10 "toKebab.*true" packages/gunshi/src/definition.test.ts | grep -A10 -B3 "lazy"Repository: kazupon/gunshi
Length of output: 1439
🏁 Script executed:
# Look for any tests that verify lazy command with toKebab works correctly
rg -n "LazyCommand.*toKebab\|toKebab.*LazyCommand" packages/gunshi/src/Repository: kazupon/gunshi
Length of output: 40
🏁 Script executed:
# Check if toKebab should be in the baseCommand or in needRunResolving block
# Let's see if there's a pattern - check if toKebab is handled elsewhere
rg -n "toKebab" packages/gunshi/src/utils.tsRepository: kazupon/gunshi
Length of output: 40
🏁 Script executed:
# Verify the actual test that checks toKebab on lazy commands
cat -n packages/gunshi/src/definition.test.ts | sed -n '290,350p'Repository: kazupon/gunshi
Length of output: 2389
🏁 Script executed:
# Check how non-lazy commands preserve toKebab
rg -B5 -A5 "Object.assign.*create.*Command" packages/gunshi/src/utils.tsRepository: kazupon/gunshi
Length of output: 631
🏁 Script executed:
# Critical: Check if toKebab should be copied in the needRunResolving block from loaded
cat -n packages/gunshi/src/utils.ts | sed -n '57,79p'Repository: kazupon/gunshi
Length of output: 1105
🏁 Script executed:
# Confirm: is there a test that actually exercises resolveLazyCommand and checks toKebab survives?
rg -n "resolveLazyCommand" packages/gunshi/src/cli.test.tsRepository: kazupon/gunshi
Length of output: 40
🏁 Script executed:
# Check if there's a test for lazy commands with toKebab being used in cli function
rg -B10 -A10 "lazy.*toKebab\|toKebab.*lazy" packages/gunshi/src/cli.test.tsRepository: kazupon/gunshi
Length of output: 40
🏁 Script executed:
# Check if there are any actual tests that use lazy commands in cli and verify toKebab works
rg -B5 -A15 "lazy\(" packages/gunshi/src/cli.test.ts | grep -A20 -B5 "toKebab"Repository: kazupon/gunshi
Length of output: 40
🏁 Script executed:
# Search for any test files that test the full cli flow with lazy commands
find packages/gunshi -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "lazy" 2>/dev/nullRepository: kazupon/gunshi
Length of output: 171
🏁 Script executed:
# Check the definition.test.ts to see the full test for lazy with toKebab
cat -n packages/gunshi/src/definition.test.ts | sed -n '270,320p'Repository: kazupon/gunshi
Length of output: 1786
🏁 Script executed:
# Verify the actual behavior by checking what happens when a lazy command with toKebab uses camelCase args
cat -n packages/gunshi/src/definition.test.ts | sed -n '270,316p'Repository: kazupon/gunshi
Length of output: 1655
🏁 Script executed:
# The test above uses '--foo' which matches the arg name exactly.
# Let's check if there's a test that would fail if toKebab didn't work
# Search for tests that verify kebab case argument conversion
rg -B10 -A10 "foo-bar.*toKebab|toKebab.*foo-bar" packages/gunshi/src/Repository: kazupon/gunshi
Length of output: 40
🏁 Script executed:
# Check the actual condition: when resolvedCommand.toKebab is used on line 92
# Verify what value it would have for lazy commands
cat -n packages/gunshi/src/utils.ts | sed -n '36,89p' | grep -A60 "if (isLazyCommand"Repository: kazupon/gunshi
Length of output: 1900
🏁 Script executed:
# Final verification: trace through what toKebab value resolvedCommand would have for a lazy command
# resolveLazyCommand is called with needRunResolving=true at core.ts:81
# Let's confirm that toKebab is missing from both paths in resolveLazyCommand
# Path 1: isLazyCommand(cmd) branch - baseCommand doesn't have toKebab
echo "=== Checking baseCommand properties (should NOT have toKebab) ==="
cat -n packages/gunshi/src/utils.ts | sed -n '43,51p'
echo ""
echo "=== Checking needRunResolving block (should NOT copy toKebab) ==="
cat -n packages/gunshi/src/utils.ts | sed -n '65,72p'Repository: kazupon/gunshi
Length of output: 915
Fix missing toKebab property in lazy command resolution.
When resolving lazy commands, the toKebab property from the lazy command definition must be copied to the resolved Command object. Currently, it's missing from both the baseCommand object (line 43-51) and the needRunResolving block (line 65-72), causing the toKebab feature to break for all lazy commands.
Add toKebab: cmd.toKebab to baseCommand and toKebab: loaded.toKebab to the needRunResolving block to preserve this property through resolution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/gunshi/src/cli/core.ts` around lines 74 - 83, When resolving lazy
commands the toKebab flag is not preserved; update the lazy-resolution code so
the resolved Command includes toKebab copied from the lazy definition: in the
baseCommand construction (where baseCommand is created from the lazy cmd) add
toKebab: cmd.toKebab, and in the needRunResolving/loaded branch (after
resolveLazyCommand returns loaded) include toKebab: loaded.toKebab when building
the final resolvedCommand so toKebab flows through
isLazyCommand/resolveLazyCommand paths.
| const { explicit, values, positionals, rest, error } = resolveArgs(args, tokens, { | ||
| shortGrouping: true, | ||
| toKebab: command.toKebab, | ||
| toKebab: resolvedCommand.toKebab, | ||
| skipPositional | ||
| }) |
There was a problem hiding this comment.
resolvedCommand.toKebab loses the lazy command's toKebab hint — regression.
resolveLazyCommand does not include toKebab in its baseCommand construction, nor does it copy toKebab from the loaded module. Before this change, command.toKebab on the original LazyCommand object correctly reflected the value set via lazy(loader, { toKebab: true }). After this change, resolvedCommand.toKebab is always undefined for lazy commands, silently breaking kebab-casing for any lazy command that specifies toKebab in its definition hint.
Quick fix in cliCore without touching utils.ts:
🐛 Proposed fix to preserve toKebab for lazy commands
const { explicit, values, positionals, rest, error } = resolveArgs(args, tokens, {
shortGrouping: true,
- toKebab: resolvedCommand.toKebab,
+ toKebab: resolvedCommand.toKebab ?? (isLazyCommand<G>(command) ? command.toKebab : undefined),
skipPositional
})A more thorough fix would be to preserve toKebab inside resolveLazyCommand in utils.ts (add toKebab: cmd.toKebab to baseCommand and copy loaded.toKebab in the needRunResolving block), which would also benefit the needRunResolving: false usage path.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { explicit, values, positionals, rest, error } = resolveArgs(args, tokens, { | |
| shortGrouping: true, | |
| toKebab: command.toKebab, | |
| toKebab: resolvedCommand.toKebab, | |
| skipPositional | |
| }) | |
| const { explicit, values, positionals, rest, error } = resolveArgs(args, tokens, { | |
| shortGrouping: true, | |
| toKebab: resolvedCommand.toKebab ?? (isLazyCommand<G>(command) ? command.toKebab : undefined), | |
| skipPositional | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/gunshi/src/cli/core.ts` around lines 90 - 94, When calling
resolveArgs in cli/core.ts, ensure the lazy command's toKebab hint is preserved
by copying it from the original LazyCommand into resolvedCommand if missing;
specifically, after resolveLazyCommand returns, check resolvedCommand.toKebab
and if undefined but the original command (the LazyCommand passed into
resolveLazyCommand) has command.toKebab set (e.g. set via lazy(loader, {
toKebab: true })), assign that value onto resolvedCommand.toKebab before using
resolvedCommand in resolveArgs — this avoids changing utils.ts and fixes the
kebab-casing regression for lazy commands.
@gunshi/bone
@gunshi/combinators
@gunshi/definition
@gunshi/docs
gunshi
@gunshi/plugin
@gunshi/plugin-completion
@gunshi/plugin-dryrun
@gunshi/plugin-global
@gunshi/plugin-i18n
@gunshi/plugin-renderer
@gunshi/resources
@gunshi/shared
commit: |
|
@cameronhunter I've just check your PR! Thanks! |
2 participants
Description
Command line arguments are not being parsed for lazy-loaded sub-commands as described in Alternative Loader Return Types. The arguments are also not showing up in the usage. The issue was that the command wasn't resolved until after the arguments were resolved.
Linked Issues
Summary by CodeRabbit
Bug Fixes
Tests