fix(cli): fail when excess positional arguments are provided#6619
fix(cli): fail when excess positional arguments are provided#6619wasim-builds wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: adee9f1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 28 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe CLI adds an ChangesCLI excess argument validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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
🧹 Nitpick comments (1)
packages/effect/src/unstable/cli/internal/command.ts (1)
294-299: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression tests for excess-argument validation.
Cover commands with no positional parameters, fixed parameters at maximum cardinality, multiple leftovers, and variadic parameters that should still consume all arguments. Assert both the
ValidationErrortag and rendered message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/effect/src/unstable/cli/internal/command.ts` around lines 294 - 299, Add regression tests for the excess-argument branch in the command argument validation flow, covering no positional parameters, fixed parameters at maximum cardinality, multiple leftover arguments, and variadic parameters consuming all arguments. Assert each failure has the ValidationError tag and the rendered “Excess arguments” message, while variadic commands succeed without an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/effect/src/unstable/cli/CliError.ts`:
- Line 96: Add ValidationError to the NonShowHelpErrors union so ShowHelp.errors
includes validation failures in its help-rendering schema. Preserve the existing
error members and type structure.
---
Nitpick comments:
In `@packages/effect/src/unstable/cli/internal/command.ts`:
- Around line 294-299: Add regression tests for the excess-argument branch in
the command argument validation flow, covering no positional parameters, fixed
parameters at maximum cardinality, multiple leftover arguments, and variadic
parameters consuming all arguments. Assert each failure has the ValidationError
tag and the rendered “Excess arguments” message, while variadic commands succeed
without an error.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fe339596-05e1-4654-b776-071fa809b1a8
📒 Files selected for processing (2)
packages/effect/src/unstable/cli/CliError.tspackages/effect/src/unstable/cli/internal/command.ts
| | UnknownSubcommand | ||
| | ShowHelp | ||
| | UserError | ||
| | ValidationError |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -A25 -B5 'NonShowHelpErrors|ShowHelp|ValidationError' \
packages/effect/src/unstable/cli/CliError.tsRepository: Effect-TS/effect
Length of output: 5432
Add ValidationError to NonShowHelpErrors. CliError now includes ValidationError, but ShowHelp.errors still uses NonShowHelpErrors, which currently excludes it. That leaves validation failures out of the help-rendering schema.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/effect/src/unstable/cli/CliError.ts` at line 96, Add ValidationError
to the NonShowHelpErrors union so ShowHelp.errors includes validation failures
in its help-rendering schema. Preserve the existing error members and type
structure.
github-project-automation
Bot
moved this from Discussion Ongoing
to Waiting on Author
in PR Backlog
IMax153
left a comment
IMax153
left a comment
There was a problem hiding this comment.
This PR will also need a changeset - you can generate one with pnpm changeset. It can just be a patch.
|
Thanks for the review! I've renamed |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/effect/src/unstable/cli/internal/command.ts (1)
294-297: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression tests for excess positional arguments.
Cover commands with no positional parameters, fixed-cardinality parameters already at their maximum, and multiple leftover arguments. Assert the resulting
UnrecognizedArgumenterror and message so this validation behavior remains protected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/effect/src/unstable/cli/internal/command.ts` around lines 294 - 297, Add regression tests for the excess-argument validation in the command execution flow around the currentArguments check: cover commands with no positional parameters, fixed-cardinality positional parameters already at their maximum, and multiple leftover arguments. Assert that each case produces an UnrecognizedArgument error with the expected “Excess arguments” message, including all remaining arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/effect/src/unstable/cli/internal/command.ts`:
- Around line 294-297: Add regression tests for the excess-argument validation
in the command execution flow around the currentArguments check: cover commands
with no positional parameters, fixed-cardinality positional parameters already
at their maximum, and multiple leftover arguments. Assert that each case
produces an UnrecognizedArgument error with the expected “Excess arguments”
message, including all remaining arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b3dec45a-69b3-4d2d-a0e4-a4ca64151dd7
📒 Files selected for processing (3)
.changeset/unrecognized-arguments.mdpackages/effect/src/unstable/cli/CliError.tspackages/effect/src/unstable/cli/internal/command.ts
|
@wasim-builds thank you! We also need a series of regression tests as described by the CodeRabbit review. |
|
Alright, since CodeRabbit kept nagging for them, I went ahead and added the full suite of regression tests in |
|
By the way, I've really been enjoying diving into the Effect codebase! My goal is to get more involved in open source to help maintain projects and learn from great engineering teams. Is there a process or pathway for becoming a member of the Effect-TS GitHub organization? I'd love to continue helping out where I can! |
|
@wasim-builds - I apologize, I realized there was a pre-existing PR here: #6561 to fix the issue. I'm going to give precedence to the PR that was opened earlier - plus it also addresses a few more edge cases. Please let me know if it addresses your issues, and if not feel free to re-open a PR / issue. |
That's wonderful! I'm glad to hear the Effect codebase has been a source of inspiration :) We always welcome contributions from the community! The organization access is intentionally quite limited for security reasons, but always happy to review PRs and collaborate, either here or in Discord! |
2 participants
Fixes #6594
This throws a ValidationError when there are unconsumed positional arguments after parsing all parameters. Previously, excess positionals were silently discarded.
Summary by CodeRabbit
UnrecognizedArgumentCLI error type for invalid command-line input.Excess arguments: ...).