secrets env needs -- to be passed twice to parse the command

[aryanjassal]

Specification

When running the secrets env command in the tests, we only need to pass -- once, and that will delimit the commands and the vault names.

However, this is not the case when running the program on the CLI, as we need to write -- twice to get it to work. For example, the following will work in the tests but not in CLI.

# In jest
[aryanj@matrix-34xx:~]$ polykey secrets env vault -- echo 'hi'
TESTING='top secret'
hi
 
# In shell
[aryanj@matrix-34xx:~]$ polykey secrets env vault -- echo 'hi'
ErrorPolykeyRemote: Remote error from RPC call
  ...
  cause: ErrorVaultsVaultUndefined: Vault does not exist
 
[aryanj@matrix-34xx:~]$ polykey secrets env vault -- -- echo 'hi'
TESTING='top secret'
hi

It is difficult to see which vault was the offender for commands containing multiple vaults, like secrets commands. For those commands at least, the ErrorVaultsVaultUndefined should also return the name of the offending vault. For example, a command like this can be difficult to find the invalid vault.

[aryanj@matrix-34xx:~]$ polykey secrets cat test.vault:file1 anothr.vault:file2
ErrorPolykeyRemote: Remote error from RPC call
  ...
  cause: ErrorVaultsVaultUndefined: Vault does not exist

Without reading the command properly, the user won't know that the offending vault was a typo in another.

Additional context

• Needing program.enablePositionalArguments enabled to use command.passThroughOptions, but enabling positional arguments breaks the commands: Polykey-CLI#325 and Polykey-CLI#329 (comment)
• Upstream discussion on a solution without needing to enable positional arguments: commander.js#2281

Tasks

1. Allow -- once in the CLI to run commands as opposed to twice
2. Provide feedback by listing which vault was incorrect

[linear]

ENG-466 `secrets env` needs `--` to be passed twice to parse the command

[aryanjassal]

When printing process.argv, we can see that -- is left to be parsed by the program. All the output is obtained when running this command.

[aryanj@matrix-34xx:~] polykey secrets env vault:SECRET -- echo 'hi'

[
  '/home/aryanj/Projects/polykey-cli/node_modules/.bin/ts-node',
  '/home/aryanj/Projects/polykey-cli/src/polykey.ts',
  'secrets',
  'env',
  'vault:another.txt',
  '--',
  'echo',
  'hi'
]

However, when printing the captured values by commander in the parser, we get the following as the vault paths.

[
  vault:SECRET
  echo
  hi
]

This means that the arguments are captured properly, but commander is swallowing the singular -- on the CLI for some reason, and not in tests.

I'd say the easiest solution would be to simply use commander for argument validation, but use the arguments present in process.argv for actual parsing and processing, but that might have hidden pitfalls, so more research needs to be done before a solution like this can be finalised.

[CMCDragonkai]

Specification

When running the secrets env command in the tests, we only need to pass -- once, and that will delimit the commands and the vault names.

However, this is not the case when running the program on the CLI, as we need to write -- twice to get it to work. For example, the following will work in the tests but not in CLI.

# In jest
[aryanj@matrix-34xx:~]$ polykey secrets env vault -- echo 'hi'
TESTING='top secret'
hi

# In shell
[aryanj@matrix-34xx:~]$ polykey secrets env vault -- echo 'hi'
ErrorPolykeyRemote: Remote error from RPC call
  ...
  cause: ErrorVaultsVaultUndefined: Vault does not exist

[aryanj@matrix-34xx:~]$ polykey secrets env vault -- -- echo 'hi'
TESTING='top secret'
hi

It is difficult to see which vault was the offender for commands containing multiple vaults, like secrets commands. For those commands at least, the ErrorVaultsVaultUndefined should also return the name of the offending vault. For example, a command like this can be difficult to find the invalid vault.

[aryanj@matrix-34xx:~]$ polykey secrets cat test.vault:file1 anothr.vault:file2
ErrorPolykeyRemote: Remote error from RPC call
  ...
  cause: ErrorVaultsVaultUndefined: Vault does not exist

Without reading the command properly, the user won't know that the offending vault was a typo in another.

Additional context

Tasks

1. Allow -- once in the CLI to run commands as opposed to twice
2. Provide feedback by listing which vault was incorrect

Um echo is not a program. So how is this working? Can you verify this? Check if env Unix command runs shell commands or runs programs? Because if it runs programs, then echo ... should be an error because it's not a valid program, and we don't want to hard code the shell. But if env does run shell commands then check if it is controlled by $SHELL.

[aryanjassal]

You are right in assuming that env runs programs and not commands. After a little investigation, I have realised that echo on my system is installed on the system and also provided by the shell.

[aryanj@matrix-34xx:~]$ which echo
echo: shell built-in command

[aryanj@matrix-34xx:~]$ which env
/run/current-system/sw/bin/env

[aryanj@matrix-34xx:~]$ env which echo
/run/current-system/sw/bin/echo

[aryanj@matrix-34xx:~]$ /run/current-system/sw/bin/echo testing
testing

[aryanj@matrix-34xx:~]$ env echo testing
testing

So, we won't need to handle running shell commands with env, as it only runs programs, but echo seems to be kind of special, as it exists on both the shell and the system.

Other bash-inbuilt commands like alias don't work, as expected.

[aryanj@matrix-34xx:~]$ which alias
alias: shell built-in command

[aryanj@matrix-34xx:~]$ env alias
env: 'alias': No such file or directory

Sources

https://superuser.com/a/1014120

[aryanjassal]

Polykey-CLI#329 (comment) has seen some discussion to the underlying reason and how to fix it. Basically, we need to enable this.enablePositionalArguments() globally (it needs to be enabled globally), and then this.passThroughOptions() will work as intended.

However, enabling the option breaks the command pass-through. This means that, if the --format has been defined on the root level, that is where the option needs to be for it to actually work, otherwise it is just skipped.

This needs more work and investigation, so I'm leaving this as work for another PR.

[aryanjassal]

Upstream discussion in commander.js#2281 (comment) has a solution. They recommend us to use process.argv to identify the index of the first --, then parse everything after as commands instead of vault names. This has been done inside the action, and most likely, we would need to stop relying on the parser to parse this, and instead do this parsing from within the action itself.

This does end up with the issue of commander.ParserError not printing the help menu and exiting. As such, I need to discuss ways to approach this with @tegefaulkes.

[tegefaulkes]

Seems like there's no easy clean way to do this. We're going to have to mimic what the parser does when the parser error is thrown. When manually parsing the args with this method. If we run into a parser error then we can just manually print the help text and exit with the error we'd normally expect if the parser was handling it.