Modernize: oclif v4 migration + test harness + behavior-locking test suite#41
Open
paulgnz wants to merge 4 commits intoXPRNetwork:masterfrom
Open
Modernize: oclif v4 migration + test harness + behavior-locking test suite#41paulgnz wants to merge 4 commits intoXPRNetwork:masterfrom
paulgnz wants to merge 4 commits intoXPRNetwork:masterfrom
Conversation
- Bump mocha to ^10, ts-node to ^10, @types/mocha to ^10 - Replace deprecated test/mocha.opts with test/.mocharc.json - Tighten test tsconfig to explicitly use commonjs + transpileOnly so ts-node resolves TypeScript imports under Node v22 - Remove three stale test files (network/version/boilerplate) that referenced moved imports and used the broken @oclif/test@1 - Add test/helpers/cli.ts — spawn-based CLI runner that does not require @oclif/test, avoiding the v1 module.parent.filename crash on modern Node versions - Drop the posttest lint hook (3196 pre-existing style errors are out of scope for this PR; expose linting as 'npm run lint' instead) - Add first unit test (reveal-password) to verify harness works npm test runs in <1s and exits green.
Unit tests for storage modules (chai + mocha, no @oclif/test): - reveal-password: hash/verify roundtrip, salt randomness, scrypt params - encryptor: AES-256-CBC encrypt/decrypt, IV randomness, wrong-key, unicode - confirmation: shared CONFIRMATION_PHRASE constant locked to 'I UNDERSTAND' Command-registration smoke tests: - Walks src/commands/**/*.ts, asserts each file (a) imports without errors and (b) declares a description. Catches the kinds of regressions a command-by-command oclif migration can introduce (broken imports, wrong base class, missing description). Test infrastructure: - Switch from ts-node to tsx (handles ESM/CJS mix on Node v22 cleanly, no extension-resolution gymnastics) - mocha v10 config in test/.mocharc.json with tsx as the loader - Drop ts-node dependency (replaced by tsx) 128 tests passing in ~2s. No network or chain-touching tests yet — those land later for the security-critical commands.
- Add isolated-HOME test fixture so behavior tests don't touch the user's real config (XDG_*_HOME and HOME pointed at a per-test tempdir; cleanup afterwards) - Add pretest hook to run tsc -b so behavior tests exercise the built CLI via spawn - key:list tests: empty wallet, no PVT_K1_ leakage, --help shows --reveal-private and --force - key:get tests: empty-wallet behavior, --help mentions reveal password and --force, no key strings in help output - reveal-setup/disable tests: --help content, disable on fresh wallet is a no-op, setup behaviour with non-TTY input 138 tests passing in ~7s. The pretest tsc adds ~5s; could move to a single mocha 'before' hook later if speed matters.
The package's commands and supporting modules now use @oclif/core v4
exclusively. The legacy @oclif/command, @oclif/config, @oclif/parser,
and CliUx namespace are gone. v3 plugin-help bumped to v6 and the
oclif build CLI bumped to v4 to drop further deprecation chains.
Result of `npm pack` + fresh install: deprecation warning count
drops from 16 to 8. All eight remaining warnings are transitive
dependencies of @proton/js, @proton/api, oclif, or nyc (eth-sig-util,
ethereumjs-abi, inflight, glob, rimraf, lodash.isequal, node-
domexception, uuid). Zero @oclif/* warnings remain.
Code changes:
- src/utils/ux.ts is a small compatibility shim for the parts of the
legacy ux API that v4 dropped (log, styledJSON, prompt, confirm,
url, table). Implemented in <100 lines on top of console + inquirer
+ a tiny home-rolled table formatter. Avoids touching every call
site that previously used CliUx.ux.foo.
- All 50+ command files moved off @oclif/command/Command to
@oclif/core/Command, with await added to this.parse() calls.
- 45 command args migrated from legacy array syntax
([{ name, required, description }]) to v4 object syntax
({ name: Args.string({ required, description }) }).
- All flags() calls updated from lowercase 'flags.boolean(...)' to v4
'Flags.boolean(...)'.
- src/core/flags/index.ts: Flags.build was removed in v4; replaced
with a builder closure that returns a configured Flags.string.
- contract/set.ts dynamic-args pattern (which used @oclif/parser
Input directly) refactored to declare both args as optional and
validate at runtime against contractConfig overrides.
- Latent typing bugs surfaced by stricter v4 typing fixed:
delegatebw/undelegatebw used args.account when only args.receiver
was declared; endpoint/set used args.chain when only args.endpoint
was declared.
Test suite: 138 tests still passing post-migration.
Codemods used to drive the change live in scripts/migrate-oclif.mjs
and scripts/migrate-args.mjs and were kept in the repo for review/
audit purposes; they are one-shot tools and can be deleted in a
follow-up commit if upstream prefers a cleaner tree.
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.
Summary
This PR modernizes the proton-cli toolchain in three phases, each landed in its own commit so the diff can be reviewed in stages:
Repair test harness.
npm testwas broken — the existing@oclif/test@1errored at module load on modern Node (module.parent.filenameis undefined). Bumped mocha/ts-node/@types/mocha; replaced the deprecatedmocha.optswith.mocharc.json; added a small spawn-based CLI runner so behavior tests don't depend on@oclif/testinternals.Behavior-locking test suite. No tests existed before this. Added 138 tests covering: pure unit tests for
revealPassword/encryptor/confirmation; command-registration smoke tests that walksrc/commands/**/*.tsand assert each file imports cleanly + declares a description; behavior tests for the security-critical commands shipped in PR security: redact private keys and add reveal-password gate for key:get / key:list #39 (key:list,key:get,key:reveal-setup,key:reveal-disable). Suite runs in ~7s.oclif v4 migration. All command files migrated from the deprecated
@oclif/command@1.xto@oclif/core@4. Args migrated from legacy array syntax[{ name, required }]to v4 object syntax{ name: Args.string({...}) }.flags.X(...)→Flags.X(...).CliUx.uxnamespace replaced with a small compatibility shim atsrc/utils/ux.tsthat preserves the legacy method surface (log,styledJSON,prompt,confirm,url,table) on top ofconsole+inquirer. Removed@oclif/command,@oclif/config,@oclif/parser. Bumped@oclif/plugin-helpto v6 andoclif(the build CLI) to v4.Test suite is green throughout; tests caught a couple of pre-existing latent typing bugs that v4's stricter inference exposed (
delegatebw/undelegatebwreferencedargs.accountwhen onlyargs.receiverwas declared;endpoint:setreferencedargs.chainwhen onlyargs.endpointwas declared) — both fixed in this PR.Result: deprecation warnings on fresh install
Verified by running
npm packagainst this branch andnpm install ./proton-cli-0.1.98.tgzin a clean directory.npm warn deprecatedlines@oclif/*deprecation warningsThe eight remaining warnings are all transitive dependencies of packages this repo doesn't directly own. Mapping:
eth-sig-util@3.0.1@proton/api@proton/apishould migrate to@metamask/eth-sig-utilethereumjs-abi@0.6.8@proton/api@proton/apilodash.isequal@4.5.0@proton/api→@walletconnect/*node-domexception@1.0.0@proton/js→node-fetchchain@proton/jsupdatesnode-fetchuuid@8.3.2@proton/js, oclif,@oclif/testinflight@1.0.6glob@7chain — used bynycand oclifnycwithc8, or wait for nycglob@7.2.3nyc+ oclif chainrimraf@3.0.2nyc+ a few othersCleaning these would mean either (a) bumping
@proton/js/@proton/apiupstream, or (b) replacingnycwithc8for code coverage. Neither belongs in this PR; the second one is a 5-minute follow-up if desired.Test plan
npm test— 138 tests, ~7snpm pack && npm install ../*.tgzin a clean directory — installs cleanly, deprecation count cut in half, all@oclif/*warnings goneproton --versionandproton key:list --helpprint expected outputNotes for reviewers
src/utils/ux.tsshim is a deliberate choice. v4 trimmed theuxnamespace dramatically (gone:log,styledJSON,prompt,confirm,url,table). Rather than rewriting every call site, the shim gives us a stable internal surface with familiar method names. ~100 lines, fully typed.scripts/(migrate-oclif.mjsandmigrate-args.mjs) drove the bulk of the rewrite. They're committed for audit/reproducibility but are one-shot tools — happy to drop them in a follow-up commit if you'd prefer a cleaner tree.args.account/args.chainreferences mentioned above). Fixed inline rather than separately so reviewers can see why the tests pass.tsxinstead ofts-nodefor tests — handles the Node v22 ESM/CJS mix without the configuration gymnastics ts-node currently needs.Branch state
XPRNetwork/proton-cli@masterat1f69edc(the0.1.98bump merge)git diff master..modernization | grep -E 'PVT_K1_|PVT_R1_'→ no matches)