Skip to content

fix(cli): report mistyped subcommand before options - #504

Open
vianmangal wants to merge 6 commits into
elastic:mainfrom
vianmangal:agent/fix-unknown-subcommand-error
Open

fix(cli): report mistyped subcommand before options#504
vianmangal wants to merge 6 commits into
elastic:mainfrom
vianmangal:agent/fix-unknown-subcommand-error

Conversation

@vianmangal

Copy link
Copy Markdown

Summary

  • defer group-level unknown option validation until Commander resolves a child command
  • report a mistyped subcommand before any options that follow it
  • preserve unknown-option errors for valid leaf commands and groups without a subcommand

Why

Command groups validated unknown options before their default action could identify an invalid subcommand. As a result, elastic stack es serch --index my-index reported --index as unknown instead of identifying the serch typo.

Testing

  • npm test
  • npm run test:lint
  • npm run test:spdx
  • npm run test:notice
  • manually verified mistyped-subcommand, valid-command/invalid-option, and group/invalid-option cases

Closes #501

@vianmangal
vianmangal marked this pull request as ready for review August 6, 2026 11:34
Comment thread src/factory-core.ts Outdated
Comment thread test/cli.test.ts Outdated

@margaretjgu margaretjgu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See inline comments.

@margaretjgu

Copy link
Copy Markdown
Member

/ai-review

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

The logic looks correct and the tests cover the key cases. One edge case worth flagging:

-- with nothing after it (this.args = ['--'])

When the user types elastic stack es --, firstArg is '--' and unknownCommand is this.args[1] which is undefined. The code falls through to the final else { group.help() } branch — showing help silently. That's probably fine, but it's worth a comment since it's a subtle case that could surprise someone later.

Minor redundancy in the condition chain

The else if (unknownCommand != null) guard is always true when reached (because firstArg !== null and firstArg === '--' is the only way unknownCommand could be undefined, and that case only triggers when this.args[1] is undefined). The structure is a bit hard to follow as a result. A small simplification:

if (firstArg == null) {
  group.help()
} else if (firstArg !== '--' && firstArg.startsWith('-')) {
  group.error(`unknown option '${firstArg}'`)
} else {
  const cmd = firstArg === '--' ? this.args[1] : firstArg
  if (cmd != null) {
    group.error(`unknown command: ${cmd}`)
  } else {
    group.help() // bare '--' with nothing after
  }
}

Not a bug, just clarity.

Test boilerplate

Each test creates and tears down its own temp dir + config file with identical content. This works but repeats ~10 lines per test. A beforeEach/afterEach or a shared helper would reduce noise significantly. Not a functionality issue.

Nothing else is wrong.

@vianmangal

vianmangal commented Aug 7, 2026

Copy link
Copy Markdown
Author

should i fix this or not since its not a functionality issue @margaretjgu

@margaretjgu

Copy link
Copy Markdown
Member

Please address the condition simplification. Even though it doesn't affect functionality, cleaner and more readable code should be enforced for maintainability :) we want to avoid additional tech debt

@vianmangal
vianmangal requested a review from margaretjgu August 7, 2026 20:14
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.

fix(cli): mistyped subcommand reports the flag as unknown instead of the command

3 participants