Skip to content

feat(cli): warn on unrecognized CLI options#1514

Open
sanjanaravikumar-az wants to merge 5 commits into
mainfrom
sanjrkmr/warn-unknown-options
Open

feat(cli): warn on unrecognized CLI options#1514
sanjanaravikumar-az wants to merge 5 commits into
mainfrom
sanjrkmr/warn-unknown-options

Conversation

@sanjanaravikumar-az
Copy link
Copy Markdown
Contributor

@sanjanaravikumar-az sanjanaravikumar-az commented May 14, 2026

Summary

  • Adds a warning when unrecognized CLI options are passed, since yargs currently silently ignores them
  • Handles edge cases: env-injected keys (CDK_* via .env('CDK')), undefined defaults, camelCase variants, boolean negations, negativeAliases

This implements a non-breaking warning approach. Unrecognized options are still processed by yargs as before, but users now get feedback that their flag was not recognized.

Changes

File Change
check-unknown-options.ts New utility that detects unrecognized options
cli.ts Calls findUnknownOptions and emits a warning
check-unknown-options.test.ts 10 tests covering all edge cases

Test plan

  • 10 unit tests passing (known options, aliases, camelCase, negation, env-injected, unknown detection)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

🤖 Generated with Claude Code

Yargs does not enable strict option checking by default, so unknown
flags are silently swallowed. This adds a warning when unrecognized
options are detected, helping users catch typos early.

Handles edge cases:
- Skips undefined-value keys (yargs default placeholders from other commands)
- Skips camelCase variants of known kebab-case options
- Skips boolean negation keys (noFoo for --no-foo)
- Skips negativeAlias keys (e.g. R for --no-rollback)
- Skips keys injected by yargs .env('CDK') from CDK_* environment variables

Closes #928 (partial — related to rix0rrr's strict parsing suggestion)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 14, 2026

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@github-actions github-actions Bot added the p2 label May 14, 2026
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.22%. Comparing base (ae2349b) to head (9d21c60).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1514      +/-   ##
==========================================
+ Coverage   88.10%   88.22%   +0.12%     
==========================================
  Files          75       76       +1     
  Lines       10723    10816      +93     
  Branches     1465     1488      +23     
==========================================
+ Hits         9447     9542      +95     
+ Misses       1248     1246       -2     
  Partials       28       28              
Flag Coverage Δ
suite.unit 88.22% <100.00%> (+0.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

expect(findUnknownOptions(argv)).toEqual([]);
});

test('does not report keys injected by yargs .env("CDK") from environment variables', () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but why?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dug into it and the test was actually wrong. I ran the actual CDK parser and yargs doesn't create noFoo keys from --no-foo flags, it just sets foo: false. The only way a noFoo key ends up in argv is if someone sets CDK_NO_FOO as an env var (via .env('CDK')), which the env prefix check already covers. Removed.

*/
export function findUnknownOptions(argv: any): string[] {
// eslint-disable-next-line @typescript-eslint/no-require-imports
const config = require('../cli-type-registry.json');
Copy link
Copy Markdown
Contributor

@mrgrain mrgrain May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this doesn't change, so can be outside the function as a "global" variable. In practice this doesn't matter much because findUnknownOptions is only called once and require is cached, but you should start to thinking about these kind of things.

The same applies to much of the "setup" code later on: sorting global and command options etc.

A good question to ask is: Which parts of the code are actually dependent on the argv input? Which parts only depend on the static cli-type-registry.json? This will give you an idea how to split it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point, thank you! Moved the registry load and global options set to module scope since they never change. Only the command-specific options are computed inside the function now since those depend on which command was invoked.

Comment on lines +52 to +55
if (argv[key] === undefined) continue;
if (yargsInternals.has(key)) continue;
if (globalOptions.has(key)) continue;
if (commandOptions.has(key)) continue;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be one if

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually removed this entirely since it's the same issue as the first comment. The negation check was solving a problem that doesn't exist with how yargs works.

Comment on lines +65 to +66
if (globalOptions.has(positiveKey) || commandOptions.has(positiveKey) ||
globalOptions.has(positiveKebab) || commandOptions.has(positiveKebab)) continue;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should enforce this as a linter rule, but imho these lines are hard to parse and read. Prefer always using {}.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, changed it

Comment on lines +80 to +82
function camelToKebab(str: string): string {
return str.replace(/[A-Z]/g, (m) => `-${m.toLowerCase()}`);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably happens to work in this case, but is likely not working in the general case. For your code, it would be much easier to go the other way: From kebab to camel.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah okay, switched it

Copy link
Copy Markdown
Contributor

@mrgrain mrgrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small feedback.

sanjanaravikumar-az and others added 2 commits May 15, 2026 12:54
- Move static config and global options set to module scope
- Replace camelToKebab with kebabToCamel (applied at set-construction time)
- Remove unnecessary isNegationOfKnownOption — verified that yargs does
  NOT create "noFoo" keys from --no-foo flags; the only noFoo keys come
  from CDK_NO_FOO env vars which isFromEnvPrefix already handles
- Always use braces for if/continue statements
- Combine the noFoo negation check into isFromEnvPrefix coverage
- Add explanatory comment on env var test

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants