Subcommand destroys ordering of options

[laurentseal]

Commander.js version: 2.13.0

The feature "subcommand in dedicated binary" destroys option ordering for boolean longopts. The bug is that, for unknown options, commander always assumes they have an argument. Boolean longopts don't have an argument, so commander assumes the next option to be an argument to the boolean option. Unknown options get sorted to the end of the argument list, so option ordering is nonsensical after the aforementioned wrong assumption has taken place.

Example:

$ ls
somecommand              somecommand-subcommand1

Contents of somecommand:

program.command('subcommand1', 'execute subcommand1').

Triggering the bug:
$ ./somecommand subcommand1 --bool-option subcommand2 --subcommand2-option subcommand2-argument

should result in

./somecommand-subcommand1 --bool-option subcommand2 --subcommand2-option subcommand2-argument

being called but commander executes

./somecommand-subcommand1 subcommand2-argument --bool-option subcommand2

This is a serious bug because the usage output cannot be made correct for the user, it is confusing the user and it results in the program not working without user feedback in combination with bug #961 .
It also could lead to random logic being executed if the options are implemented in a specific manner in the subcommand.

[MatthewRalston]

Seconded. I'm having a similar issue with 'git-style subcommands'. I have to admit it's only happening for one of my subcommands. For example, I'm using a --verbose flag to enable debug level logging in a subcommand. Node v12.2.0, commander 2.20.0

Expectation

I expect that the process.argv from the 'git-style subcommand' is identical in order to process.argv from the main command

Observation

I observe a reordering of the process.argv array that interferes with positional argument parsing AND doesn't throw an error about non-flag arguments being passed/parsed. Perhaps this is not important, but the order of all flags/positional arguments is rearranged. Also, this is not being handled by unknownOptions=false which would normally throw an error if --foo is passed.

Steps taken to debug

Only affects one of my subcommands, 'probability' (sub2) which is correctly spelled in both, and no similar command or alias exists. Also, rearrangement of the command declarations in mycommand had no effect.

bin/mycommand sub1 --verbose positionalArg1 # works fine
bin/mycommand sub2 --verbose positionalArg1 positionalArg2 #Causes the process.argv order to be rearranged 

To find where the rearrangement is being done, I did the following

mycommand

program.description(...)
   .version(...)
   .command('sub1 <positionalArg1> <positionalArg2>', 'sub1 does distance')
   .command('sub2 <positionalArg1> <positionalArg2>', 'sub2 does probability')

console.log(process.argv)
program.parse(process.argv)

mycommand-sub1

program.description(...)
   .arguments('<positionalArg1> <positionalArg2>', 'these do something')
   .option('-v, --verbose', 'enable verbosity')
console.log(process.argv)
program.parse(process.argv)

>bin/mycommand sub2 --verbose positionalArg1 positionalArg2
[
   'path/to/node/v12.2.0/bin/node'
   'path/to/bin/mycommand'
   'sub2'
   '--verbose'
   'positionalArg1'
   'positionalArg2'
]
[
   'path/to/node/v12.2.0/bin/node'
   'path/to/bin/mycommand-sub2'
   'positionalArg2'
   '--verbose'
   'positionalArg1'
]

As you can see, the order is rearranged substantially from the original.

Best guess as to where in the codebase?

The commander.Command.prototype.normalize function is invoked here. From inspecting in the REPL, the normalize function returns the expected order. This result is passed to parseOptions. This too seems to work as expected, and returns an object with an args attribute that looks as expected with known options removed. BUT all options and some positional arguments are considered unknown in the base executable. You'll see why this matters in a second. Then the args are assigned to the instance and unknowns are passed to parseArgs, and eventually the first positional argument is correctly assumed to be the name of a subcommand and executeSubCommand is called, passing in the original argv, the parsed args (which are so far correct), and the unknown options. It's worth noting that parseOptions returns some positional arguments in the unknown attributes if some options are not declared!! THIS MATTERS because an option (and therefore an unknown positional argument) can be 'unknown' in the main executable and then be concatenated to the end of the positional args! This rearranges all argv passed to the child process with spawn.

Idea for a quick fix?

I understand that the difference between argv and rawArgv is the only big issue here. You'd need to do the following to transform rawArgv into the desired argv:

1. remove the subcommand from the args list, since it's replaced with the concatenation (e.g. mycommand sub1 becomes mycommand-sub1) and this is already done AFAICT unfortunately with the args array passed in, but its ultimately concatenated with the unknowns. You could .slice(2) the rawArgv here, but you'll need to worry about:
2. Omit any options that pertained to the top-level executable.

[shadowspawn]

Thanks for the detail @laurentseal and @MatthewRalston.

Related open issues: #508
Related open PR: #734

(As I noted in #734, I am surprised things are potentially this broken, which makes me wonder if there are some calling patterns which are ok.)

[shadowspawn]

I am going to close this in favour of #508, and add a note there about the good diagnosis and reproduction steps here.

Thank you for your contributions.

[shadowspawn]

FYI, opened a PR to preserve argument order: #1138

[shadowspawn]

The argument order is fixed in the published pre-release of Commander v5.0.0 (#1163)