Skip to content

fix(args): enforce required on enum arguments - #279

Open
MFA-G wants to merge 1 commit into
unjs:mainfrom
MFA-G:fix/required-enum-arg
Open

MFA-G wants to merge 1 commit into
unjs:mainfrom
MFA-G:fix/required-enum-arg

Conversation

@MFA-G

@MFA-G MFA-G commented Sep 10, 2026 •

Copy link
Copy Markdown

Problem

required: true is silently ignored for type: "enum" arguments.

parseArgs validates each argument through one if / else if chain, and the generic required check is the last branch:

} else if (arg.type === "enum") {
  // ...only validates the value against `options`
} else if (arg.required && parsedArgsProxy[arg.name] === undefined) {
  throw new CLIError(`Missing required argument: --${arg.name}`, "EARG");
}

An enum argument always matches the enum branch, so it never reaches the required check. Omitting it yields undefined instead of an error:

parseArgs([], { value: { type: "enum", options: ["one", "two"], required: true } });
// -> { value: undefined, _: [] }   (expected: throws EARG)

The inconsistency is visible in the help output too: usage.ts reads arg.required for every non-positional type, so --help renders the argument as (Required) while the parser does not enforce it.

Change

The enum branch now handles the missing-value case itself and throws the same Missing required argument: --<name> error the other types produce. The existing options check moves to the else, so a supplied value is validated exactly as before.

A default still satisfies the requirement — parseRawArgs has already applied parseOptions.default by this point, so parsedArgsProxy[arg.name] is defined. That matches how required + default behave for string arguments today.

Scope

Anyone relying on the current behaviour was declaring required: true and getting nothing for it, so this only turns a silent undefined into the documented error.

Tests

Two cases added to test/args.test.ts:

  • a required enum with no value throws Missing required argument: --value;
  • a required enum with a default parses to the default.

pnpm test passes: 111 tests + 1 expected fail, oxlint/oxfmt clean, tsgo --noEmit clean.

Summary by CodeRabbit

  • Bug Fixes

    • Required enum arguments without a provided value or default now correctly report a missing required argument error.
    • Required enum arguments with valid default values are accepted as expected.
  • Tests

    • Added coverage for required enum arguments with and without default values.

`parseArgs` checks `required` in the final `else if` of the per-argument
chain, but an `enum` argument is caught by the preceding `else if
(arg.type === "enum")` branch and never reaches it. `{ type: "enum",
required: true }` therefore parsed to `undefined` without error, while
`--help` still rendered the argument as `(Required)`, since `usage.ts`
reads `arg.required` for every non-positional type.

Handle the missing value inside the enum branch: throw the same
`Missing required argument: --<name>` error the other types produce.
A default still satisfies the check, because `parseRawArgs` has already
applied it by this point — matching how `required` behaves for strings.
@coderabbitai

coderabbitai Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 67fe03bd-ffb4-4a74-9b5a-6448d10e7e8d

📥 Commits

Reviewing files that changed from the base of the PR and between 3e342fe and 23c3325.

📒 Files selected for processing (2)
  • src/args.ts
  • test/args.test.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

Changes

Enum argument validation

Layer / File(s) Summary
Required enum value handling
src/args.ts, test/args.test.ts
The enum branch throws Missing required argument for undefined required values. Tests verify that defaults satisfy required enum arguments and that missing values throw the expected error.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 23c33

Required enum arguments now consistently reject missing values while allowing defaults, with focused coverage for both behaviors. The change is ready to merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: enforcing required validation for enum arguments.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant