Let the resume subcommand receive the options it declares - #328
Open
beeman wants to merge 1 commit into
Open
Conversation
`resume` redeclares options the root command also owns — `--keypair`, `--verbose`, `--portal-url`, `--rpc-url`, `--api-key-stdin`, `--local-dev`, `--skip-self-update`. Commander gives the root command first refusal on the whole argv unless positional option parsing is enabled, so it consumed all of them and the subcommand was left with only the flags the root does not declare. `runResumeAction` then read `options.keypair` off that stripped set and threw, which made every invocation of the documented usage fail with "`--keypair` is required" while the keypair sat in the command line:
$ dapp-store resume --release-id <id> --keypair ~/publisher.json
`--keypair` is required.
Parsing `resume --keypair /tmp/x.json --session-id abc --verbose` showed where they went: the subcommand saw `{apiKeyEnv, sessionId}` while the root held `{keypair, verbose}`. `resume` could not complete under any argument order, so the recovery path for a partially completed publication was unreachable.
`enablePositionalOptions()` assigns each option to the command it was typed after, which fixes the documented form. Options typed before the subcommand still land on the root, so `withRootOptionFallbacks` reads them back from there rather than reporting a flag as missing when it is plainly present — both orders now work.
Verified against the staging portal: `dapp-store resume --release-id <id> --keypair <path>` reaches "Loading existing publication session" and reports what the portal says about that session, with the flags in either position.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
dapp-store resumecannot run. Every invocation of its own documented usage fails on a flag that is right there in the command line:resumeredeclares seven options the root command also owns —--keypair,--verbose,--portal-url,--rpc-url,--api-key-stdin,--local-dev,--skip-self-update. Commander gives the root command first refusal on the whole argv unless positional option parsing is enabled, so the root consumed all seven and the subcommand was left with only the flags the root does not declare. Parsingresume --keypair /tmp/x.json --session-id abc --verbose --portal-url https://x.testshows exactly where they went:Only
--session-idand--release-id— the two options the root does not declare — reached the subcommand.runResumeActionreadsoptions.keypairoff that stripped set, sovalidateResumeArgsthrew before any work started, under every argument order. That made the recovery path for a partially completed publication unreachable, which matters precisely when a publish has already half-run and starting a fresh one is the wrong move.The fix
enablePositionalOptions()assigns each option to the command it was typed after, which fixes the documented form.Options typed before the subcommand still legitimately land on the root — someone who read these flags off the root help writes
dapp-store --keypair <path> resume --release-id <id>— sowithRootOptionFallbacksreads them back from there. Reporting a flag as missing when it is plainly present is the worse failure, and both orders now work.--api-key-envneeds care since it defaults on both commands: an untouched subcommand value is indistinguishable from an unset one, so the root is consulted only while the subcommand still holds the default.Verification
Against the staging portal, with the flags in either position,
resumenow runs the real workflow instead of dying on argument validation:That last line is the portal's answer for a release whose session is gone, which is the correct response for the id I tested with — the point is that it gets there at all.
Three tests, all parse-level since that is where the bug was:
resumereceives the options it shares with the root command — fails withoutenablePositionalOptions()resumefalls back to options typed before the subcommand — fails withoutwithRootOptionFallbacksThe third one caught something worth knowing: Commander retains option values on both the root and the subcommand across parses, so a test that parses repeatedly leaks flags into the next case. Harmless in production, where
main()parses argv exactly once per process, but the suite now clears both commands inbeforeEach.pnpm run buildandpackages/clitests pass: 60 tests, 9 suites.Independent of #327 and branched from
main; the two can land in either order.