Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR migrates the VIP CLI’s argument parsing from the abandoned args package to a commander-backed compatibility wrapper, while also adding contributor-facing migration/architecture documentation and hardening some dev-env startup flows.
Changes:
- Replace
argswith acommandercompatibility layer insrc/lib/cli/command.js, and adjust root dispatch to await parsing. - Add bounded readiness polling/retry behavior to
dev-envstart/exec flows to reduce Docker/Lando flakiness. - Update dependencies (
package.json,npm-shrinkwrap.json) and add migration/contributor docs (AGENTS.md,COMMANDER-MIGRATION.md,CLAUDE.md).
Reviewed changes
Copilot reviewed 6 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/dev-environment/dev-environment-core.ts | Adds post-start readiness polling + one retry start pass before failing. |
| src/lib/cli/command.js | Replaces args with a commander-based compatibility wrapper, including subcommand spawning. |
| src/bin/vip.js | Awaits cmd.argv() so async parsing/dispatch completes reliably. |
| src/bin/vip-dev-env-exec.js | Adds bounded readiness polling before declaring an env “not running”. |
| package.json | Removes args, adds commander. |
| npm-shrinkwrap.json | Shrinkwrap updates to reflect dependency swap/removals. |
| docs/COMMANDER-MIGRATION.md | Documents migration status, preserved behaviors, and verification steps. |
| CLAUDE.md | Points contributors to AGENTS.md. |
| AGENTS.md | Adds detailed repo/CLI architecture and migration guidance. |
Files not reviewed (1)
- npm-shrinkwrap.json: Language not supported
| const aliasFromRawArgv = argv.slice( 2 ).find( arg => isAlias( arg ) ); | ||
| const subcommandIndex = parsedAlias.argv.findIndex( ( arg, index ) => { | ||
| return index > 1 && arg === subcommand; | ||
| } ); | ||
|
|
||
| let childArgs = subcommandIndex > -1 ? parsedAlias.argv.slice( subcommandIndex + 1 ) : []; | ||
|
|
||
| if ( aliasFromRawArgv ) { |
There was a problem hiding this comment.
executeSubcommand() re-injects an alias by scanning the raw argv after index 1, but this ignores the -- end-of-options convention used by parseEnvAliasFromArgv(). This can accidentally pick up alias-looking tokens intended for passthrough args (after --), duplicating/reordering them when spawning the child command. Consider only searching for/re-injecting an alias when parsedAlias.app is set, and limit the scan to arguments before -- (matching envAlias.ts behavior).
| const aliasFromRawArgv = argv.slice( 2 ).find( arg => isAlias( arg ) ); | |
| const subcommandIndex = parsedAlias.argv.findIndex( ( arg, index ) => { | |
| return index > 1 && arg === subcommand; | |
| } ); | |
| let childArgs = subcommandIndex > -1 ? parsedAlias.argv.slice( subcommandIndex + 1 ) : []; | |
| if ( aliasFromRawArgv ) { | |
| const endOfOptionsIndex = argv.indexOf( '--' ); | |
| const aliasSearchArgs = endOfOptionsIndex === -1 ? argv.slice( 2 ) : argv.slice( 2, endOfOptionsIndex ); | |
| const aliasFromRawArgv = aliasSearchArgs.find( arg => isAlias( arg ) ); | |
| const subcommandIndex = parsedAlias.argv.findIndex( ( arg, index ) => { | |
| return index > 1 && arg === subcommand; | |
| } ); | |
| let childArgs = subcommandIndex > -1 ? parsedAlias.argv.slice( subcommandIndex + 1 ) : []; | |
| if ( parsedAlias && parsedAlias.app && aliasFromRawArgv ) { |
| let isEnvironmentUp = await waitForEnvironmentToBeUp( lando, instancePath ); | ||
| if ( ! isEnvironmentUp ) { | ||
| // A second startup pass helps recover after Docker network auto-cleanup edge cases. | ||
| await landoStart( lando, instancePath ); | ||
| isEnvironmentUp = await waitForEnvironmentToBeUp( lando, instancePath ); | ||
| } |
There was a problem hiding this comment.
startEnvironment() now has bounded readiness polling and a retry landoStart() pass. There are existing unit tests for dev-environment-core, but none cover the new success/retry/failure branches (e.g., isEnvUp false for N attempts then true; or always false leading to UserError). Adding mocked tests around this logic would prevent regressions and make the timing/retry behavior explicit.



Description
This pull request introduces several important updates to the CLI codebase, primarily focused on migrating from the
argslibrary to the more robustcommanderlibrary for command-line argument parsing. It also adds comprehensive documentation for future contributors and updates dependency management files accordingly. These changes modernize the CLI infrastructure and provide clearer guidance for ongoing development.Key changes include:
CLI Parser Migration:
argslibrary with thecommanderlibrary for CLI argument parsing, updating dependencies in bothpackage.jsonandnpm-shrinkwrap.json, and updating imports insrc/lib/cli/command.jsto usecommanderinstead ofargs. [1] [2] [3] [4] [5] [6]argsand its sub-dependencies (camelcase,leven,mri, etc.) fromnpm-shrinkwrap.json. [1] [2] [3] [4]Documentation Improvements:
AGENTS.mdguide for future contributors, covering CLI architecture, traps, migration notes, and best practices for working with the codebase.CLAUDE.mdfile referencingAGENTS.mdfor additional guidance.Dependency Updates:
commanderpackage to version 14.0.3, ensuring compatibility with Node 20+ and updating the shrinkwrap and lock files to reflect the new version and its requirements. [1] [2]These changes lay the groundwork for further CLI improvements and make the codebase easier to maintain and extend.
Pull request checklist
New release checklist
Steps to Test
Outline the steps to test and verify the PR here.
Example:
npm run build./dist/bin/vip-cookies.js nom