Options parsed before validating argument

[vck3000]

Hi there,

This is a great project, thank you for maintaining this.

We're in the process of upgrading from v4.0.1 to v7.0.0 with the new features. We have a small problem whereby options are being parsed before the arguments themselves are validated.

We were hoping to be able to continue to listen to command:* to detect bad arguments, as we've currently hooked it up to our own suggestion engine to provide "did you mean" suggestions for the bad argument. Upgrading to v7.0.0 has broken this functionality for us.

E.g.

$ program bad_argument --help

# Outputs the help menu instead of complaining that bad_argument is a bad argument

Another example:

$ program lintt --fix

# Complains that --fix is not a valid option. Instead, we expected `lintt` to be
# detected as a bad argument so we can suggest `lint` to the user.

Minimal reproduction:

import commander from 'commander';

const program = new commander.Command();

program.on('command:*', (arg: string) => {
  console.log(`Detected bad arg: ${arg}`);
});

program.command('lint').action(() => {
  console.log('lint');
});

program.parse();

I dug into the source code a bit and think I found it. It appears to be parsing the options and throwing the error first, before reaching the "command:*" section.

commander.js/index.js

Line 1472 in 034ad9f

this.unknownOption(parsed.unknown[0]);

Is there a solution or workaround to this that I haven't found? Thanks :).

[shadowspawn]

Thanks for clear reproduce steps. I didn't expect the behaviour to change from v4 in these areas, but there have been many parsing improvements. I'll do some digging.

[shadowspawn]

Short version: reproduced, broken from v5. Drat.

Long version

The PR that changed the behaviour is #1149 which shipped in v5.0.0. It was BIG.

There were no explicit unit tests on command:* in v4.0.0. There were some tests added for v5.0.0, but none of them included options. So the change in behaviour was quietly missed. This is a use case that I had not explicitly considered. Interesting situation, that of course the options don't make sense when the command is spelled wrong.

Which check gets called where is a bit fragile.

To some extent I consider command:* as legacy and may not be able to preserve (restore!) all the original behaviours. However, I am interested in supporting "did you mean" suggestions in some way so if can't work out a backwards compatible approach will consider new approaches.

[vck3000]

Thanks for your investigation into this :).

I guess a workaround for me at a library-consumer level could be to simply check whether my first argument (extracted directly from process.argv) exists in the commander object's .commands list before calling parse().

I'm not too familiar with the library source code, but since a misspelt command doesn't need an absent actionHandler(), do you think a similar check could be done just before the unknownOption() call?

[shadowspawn]

That is a potential work-around which is easier at the library-consumer level since you know how you structured your CLI.

In the library, there is a long line of if-else cases following the unknownOption call. I think I'll need to work through them to see which ones should be checking the options. The previous code checked in two different places and I wanted to cover a third situation, and thought I had found the place to check just once before working through the cases. Too simple!

[vck3000]

Haha no worries, sounds great. Thanks :).

[vck3000]

Update: We realised that an error with code commander.unknownCommand is raised by commander when there's an unknown command given in. We've moved from listening to command:* to catching an error with that code now which has also improved the flow of our code :). Do you think this should be documented in the readme somewhere?

[shadowspawn]

The README does mention .exitOverride(). The initial target use cases I had in mind were modifying the runtime and for unit tests. But it is also useful for adding your own extra processing to specific error cases, and that has come up in a few issues as an approach.

[shadowspawn]

I was focused on command:* processing for suggestions, but this issue affects plain programs too. The error for an invalid option is less useful than the error for an unknown command.

You probably realised this when reporting the issue, but I had not included it in my thinking or comments so far!

[shadowspawn]

Opened draft PR #1464

[vck3000]

Here's our consumer-level patch:

import commander from 'commander';

function assertValidArgs(argv: string[], cmd: commander.Command): void {
  const args = argv.slice(2);
  let commands = cmd.commands;

  while (args[0] && !args[0].startsWith('-')) {
    const commandMatch = commands.find((cmd) => cmd.name() === args[0]);
    if (!commandMatch) {
      throw new commander.CommanderError(
        1,
        'commander.unknownCommand',
        `error: unknown command '${args[0]}'. See 'cli --help'.`
      );
    }
    commands = commandMatch.commands;
    args.shift();

    // If there are no more sub-commands, stop early in case
    // of extra args coming in e.g. program create [name]
    // This assumes that a command that asks for user args 
    // doesn't have sub-commands and vice versa.
    if (commands.length === 0) {
      break;
    }
  }
}

const program = new commander.Command();

program.on('command:*', (arg: string) => {
  console.log(`Detected bad arg: ${arg}`);
});

program.command('lint').action(() => {
  console.log('lint');
});

try {
  assertValidArgs(process.argv, program);
  program.parse(process.argv);
} catch (e) {
  console.log(e);
  // Do some nice did-you-mean suggestions here
}

assertValidArgs() will traverse down the nested sub-commands until it runs out of arguments or reaches an option and attempts to mimic commander-thrown exceptions :). Fixes both of the scenarios I listed in the original description.

[shadowspawn]

My PR is aimed at the program lintt --fix case but I haven't attempted to change the program bad_argument --help case as that interacts with missing required (mandatory) options in a tricky way and displaying the help is at least informative. So you may still want your consumer-level patch even after the PR ships.

[shadowspawn]

Reworked error handling in Commander v7.1.0 to better support suggestions, and prefer unknown-command to unknown-option