Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #202 +/- ##
=======================================
Coverage ? 97.75%
=======================================
Files ? 9
Lines ? 401
Branches ? 139
=======================================
Hits ? 392
Misses ? 8
Partials ? 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Use Object.keys instead of for...in, Set instead of array for lookups, and skip parent validation when a subcommand handles the args.
|
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 (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds unknown-option validation and a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "CLI Parser"
participant Runner as "runCommand"
participant Validator as "validateUnknownOptions"
participant Sub as "Subcommand (runCommand)"
User->>CLI: supply raw args
CLI->>Runner: runCommand(cmd, rawArgs)
Runner->>Runner: resolve cmd.meta (cmdMeta)
alt subcommand selected
Runner->>Runner: set handledBySubCommand = true
Runner->>Sub: runCommand(subCmd, subArgs)
Sub-->>Runner: subcommand result
else no subcommand
Runner->>Validator: validateUnknownOptions(argsDef, parsedArgs)
Validator-->>Runner: ok or throws E_UNKNOWN_OPTION
Runner->>Runner: execute command handler
end
Runner-->>CLI: result / exit
CLI-->>User: output / errors
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/command.ts (1)
40-70:⚠️ Potential issue | 🟠 Major
handledBySubCommandsuppresses parent validation too aggressively.Once a subcommand is found, the root command stops validating unknown options for the entire argv. That means
prog --bogus foonever reports--bogus, and a subcommand-only container still throwsE_NO_COMMANDforprog --bogusbefore the new validation runs. Validate only the slice owned by the current command (tokens before the subcommand) before delegating or raisingE_NO_COMMAND.Based on learnings, subcommand lookup taking priority over positionals is a deliberate design convention.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command.ts` around lines 40 - 70, The current flow sets handledBySubCommand true and skips parent option validation entirely when a subcommand is found, which hides unknown options before the subcommand token; change this so the parent only validates the argv slice it owns (tokens before the subcommand). Concretely: compute subCommandArgIndex via opts.rawArgs.findIndex as you already do, then before delegating to runCommand(resolveValue(subCommands[subCommandName])), call validateUnknownOptions on the parent slice (e.g., build cmdArgs/parsedArgs for opts.rawArgs.slice(0, subCommandArgIndex)) unless cmdMeta?.allowUnknownOptions is true; keep the existing unknown-subcommand CLIError path and only skip validation for the delegated slice passed to runCommand. Update usage of handledBySubCommand (or remove it) so it no longer suppresses validation for the parent command.
🧹 Nitpick comments (2)
src/command.ts (1)
4-4: Use an explicit.tsextension for this import.
./validateshould follow the same ESM import convention as the rest of the changedsrc/**/*.tscode.As per coding guidelines,
src/**/*.ts: Use explicit.tsextensions in imports following ESM conventions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command.ts` at line 4, Update the ESM import in src/command.ts to use an explicit .ts extension: replace the current import statement that references "./validate" with one referencing "./validate.ts" so the module loader follows the project's ESM convention (symbol: validateUnknownOptions in src/command.ts).src/validate.ts (1)
1-2: Use explicit.tsextensions in these new imports.
./_utilsand./typesshould be imported as./_utils.tsand./types.tsto match the repo’s ESM import rule.As per coding guidelines,
src/**/*.ts: Use explicit.tsextensions in imports following ESM conventions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/validate.ts` around lines 1 - 2, Update the ESM imports to use explicit .ts extensions: change the import of CLIError and toArray (currently from "./_utils") to "./_utils.ts" and change the type imports ArgsDef and ParsedArgs (currently from "./types") to "./types.ts"; ensure the import specifiers referencing CLIError, toArray, ArgsDef, and ParsedArgs are the only changes so the module resolution follows the repo's ESM rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/main.test.ts`:
- Around line 155-156: The test currently asserts
expect(consolaErrorMock).not.toHaveBeenCalledWith("Unknown option `--foo`")
which only forbids that specific message; change it to
expect(consolaErrorMock).not.toHaveBeenCalled() so the test verifies no error
logs were emitted at all when unknown options are allowed—update the assertion
near the mockRunCommand check (referencing mockRunCommand and consolaErrorMock)
to use not.toHaveBeenCalled().
---
Outside diff comments:
In `@src/command.ts`:
- Around line 40-70: The current flow sets handledBySubCommand true and skips
parent option validation entirely when a subcommand is found, which hides
unknown options before the subcommand token; change this so the parent only
validates the argv slice it owns (tokens before the subcommand). Concretely:
compute subCommandArgIndex via opts.rawArgs.findIndex as you already do, then
before delegating to runCommand(resolveValue(subCommands[subCommandName])), call
validateUnknownOptions on the parent slice (e.g., build cmdArgs/parsedArgs for
opts.rawArgs.slice(0, subCommandArgIndex)) unless cmdMeta?.allowUnknownOptions
is true; keep the existing unknown-subcommand CLIError path and only skip
validation for the delegated slice passed to runCommand. Update usage of
handledBySubCommand (or remove it) so it no longer suppresses validation for the
parent command.
---
Nitpick comments:
In `@src/command.ts`:
- Line 4: Update the ESM import in src/command.ts to use an explicit .ts
extension: replace the current import statement that references "./validate"
with one referencing "./validate.ts" so the module loader follows the project's
ESM convention (symbol: validateUnknownOptions in src/command.ts).
In `@src/validate.ts`:
- Around line 1-2: Update the ESM imports to use explicit .ts extensions: change
the import of CLIError and toArray (currently from "./_utils") to "./_utils.ts"
and change the type imports ArgsDef and ParsedArgs (currently from "./types") to
"./types.ts"; ensure the import specifiers referencing CLIError, toArray,
ArgsDef, and ParsedArgs are the only changes so the module resolution follows
the repo's ESM rule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca5af49f-ad74-406e-896d-af350970c0fd
📒 Files selected for processing (4)
src/command.tssrc/types.tssrc/validate.tstest/main.test.ts
test/main.test.ts
Outdated
| expect(mockRunCommand).toHaveBeenCalledWith(command, { rawArgs }); | ||
| expect(consolaErrorMock).not.toHaveBeenCalledWith("Unknown option `--foo`"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the test file to understand the context
head -n 200 test/main.test.ts | tail -n 100Repository: unjs/citty
Length of output: 2762
🏁 Script executed:
# Get the specific lines mentioned in the review
sed -n '150,160p' test/main.test.tsRepository: unjs/citty
Length of output: 330
🏁 Script executed:
# Look for the test setup to understand consolaErrorMock
sed -n '1,50p' test/main.test.tsRepository: unjs/citty
Length of output: 1344
🏁 Script executed:
# Find the test block containing lines 155-156
sed -n '140,165p' test/main.test.tsRepository: unjs/citty
Length of output: 773
Strengthen this assertion to fully verify no error is logged when unknown options are allowed.
not.toHaveBeenCalledWith("Unknown option --foo") only rejects that specific message; if consola.error is called with any other message, the test still passes. Use not.toHaveBeenCalled() to ensure no errors are logged at all, which better matches the test's intent.
💡 Suggested change
- expect(consolaErrorMock).not.toHaveBeenCalledWith("Unknown option `--foo`");
+ expect(consolaErrorMock).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/main.test.ts` around lines 155 - 156, The test currently asserts
expect(consolaErrorMock).not.toHaveBeenCalledWith("Unknown option `--foo`")
which only forbids that specific message; change it to
expect(consolaErrorMock).not.toHaveBeenCalled() so the test verifies no error
logs were emitted at all when unknown options are allowed—update the assertion
near the mockRunCommand check (referencing mockRunCommand and consolaErrorMock)
to use not.toHaveBeenCalled().
|
I've resolved the conflicts. |
2 participants
resolves #201
This PR adds support for warning about unknown options.
This is enabled by default but can be disabled by setting
meta.allowUnknownOptions: true.Summary by CodeRabbit
Bug Fixes
New Features
Tests