From 2247b0cd8d5bd3b1384cd7311e2ba3ec329abbc3 Mon Sep 17 00:00:00 2001 From: Ryan Ciehanski Date: Sat, 26 Sep 2026 23:07:58 -0500 Subject: [PATCH] feat(cli): require Node 20.19+, reject unknown flags, exit non-zero on rejection --- .github/workflows/test.yml | 6 +- README.md | 2 + package.json | 4 +- script/cli.ts | 12 +++- script/command-parser.ts | 89 ++++++++++++++++++++---- script/node-version-check.ts | 13 ++++ test/command-parser.ts | 127 +++++++++++++++++++++++++++++++++++ 7 files changed, 236 insertions(+), 17 deletions(-) create mode 100644 script/node-version-check.ts create mode 100644 test/command-parser.ts diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f4a3ec8..e46591b 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -11,12 +11,16 @@ on: jobs: test: runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + node-version: ["20.19.0", "22.12.0", "24"] steps: - uses: actions/checkout@v6 - uses: actions/setup-node@v6 with: - node-version: "22" + node-version: ${{ matrix.node-version }} cache: "npm" - run: npm ci diff --git a/README.md b/README.md index e428fc7..bcf9469 100644 --- a/README.md +++ b/README.md @@ -19,6 +19,8 @@ To install and run the DeployPulse CLI, follow these steps: 3. Build the CLI by running `npm run build`. 4. Install CLI globally by running `npm install -g`. +Before upgrading a CI pipeline to 1.2.0, note two changes: an unknown flag is now an error rather than silently ignored, and a rejected command exits 1 rather than 0. A pipeline that passed on a misspelled flag will now fail. + ## Getting started 1. Create a [DeployPulse account](https://deploypulse.io/register). diff --git a/package.json b/package.json index 43bfd91..d4c139d 100644 --- a/package.json +++ b/package.json @@ -11,7 +11,7 @@ "prettier": "prettier --write \"./**/*.ts\"", "lint": "npx eslint ./script/**/*.ts", "lint:fix": "npx eslint ./script/**/*.ts --fix", - "test": "mocha --require ts-node/register ./test/cli.ts ./test/hash-utils.ts ./test/acquisition-sdk.ts ./test/management-sdk.ts --reporter mochawesome --exit" + "test": "mocha --require ts-node/register ./test/cli.ts ./test/hash-utils.ts ./test/acquisition-sdk.ts ./test/management-sdk.ts ./test/command-parser.ts --reporter mochawesome --exit" }, "bin": { "dpctl": "./bin/script/cli.js" @@ -37,7 +37,7 @@ "access": "public" }, "engines": { - "node": ">=18" + "node": "^20.19.0 || >=22.12.0" }, "dependencies": { "chalk": "^4.1.2", diff --git a/script/cli.ts b/script/cli.ts index c143d57..810b1fa 100644 --- a/script/cli.ts +++ b/script/cli.ts @@ -3,6 +3,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +import "./node-version-check"; import * as parser from "./command-parser"; import * as execute from "./command-executor"; import * as chalk from "chalk"; @@ -11,7 +12,16 @@ function run() { const command = parser.createCommand(); if (!command) { - parser.showHelp(/*showRootDescription*/ false); + // Print if the command is unrecognized, then provide command help info + const args = process.argv.slice(2).filter((a) => !a.startsWith("-")); + if (args.length && !parser.failureReported()) { + console.error(chalk.red(`[Error] Unrecognized command: ${args.join(" ")}`)); + } + parser.showHelp(/*showRootDescription*/ !args.length); + // A bare `dpctl` is someone asking for help, not a failed command: it keeps 1.0.0's exit 0 so a + // Dockerfile smoke check or a `set -e` script does not fail on it. Anything actually typed and + // rejected still exits 1. + if (args.length) process.exitCode = 1; return; } diff --git a/script/command-parser.ts b/script/command-parser.ts index 0cdaa08..029a06a 100644 --- a/script/command-parser.ts +++ b/script/command-parser.ts @@ -18,8 +18,14 @@ const USAGE_PREFIX = "Usage: dpctl"; let isValidCommandCategory = false; // Commands are the verb following the command category (e.g.: "add" in "app add"). let isValidCommand = false; +let lastFailMessage: string | undefined; let wasHelpShown = false; +/** True when yargs already printed why the arguments were rejected, so callers need not add their own. */ +export function failureReported(): boolean { + return !!lastFailMessage; +} + export function showHelp(showRootDescription?: boolean): void { if (!wasHelpShown) { if (showRootDescription) { @@ -121,7 +127,22 @@ function addCommonConfiguration(yargs: yargs.Argv): void { yargs .wrap(/*columnLimit*/ null) .string("_") // Interpret non-hyphenated arguments as strings (e.g. an app version of '1.10'). - .fail((msg: string) => showHelp()); // Suppress the default error message. + // strictOptions, NOT strict: unknown flags become errors instead of being silently dropped, which is + // what let `--deployment Production` ship releases to Staging. Full .strict() also validates + // positionals, and this parser declares only positional counts, so every command would fail with + // "Unknown arguments: MyApp, ios". + .strictOptions() + // Keep yargs' actual complaint ("Unknown argument: deployment"); a bare showHelp() throws away the + // one line that says what was wrong. + .fail((msg: string) => { + // yargs runs this handler once per nesting level (root, category, subcommand, ...), so the + // same message arrives several times for a single mistake. Print each one once. + if (msg && msg !== lastFailMessage) { + lastFailMessage = msg; + console.error(chalk.red(`[Error] ${msg}`)); + } + showHelp(); + }); } function appList(commandName: string, yargs: yargs.Argv): void { @@ -543,7 +564,6 @@ yargs }) .option("rollout", { alias: "r", - default: null, demand: false, description: "Percentage of users this release should be immediately available to. This attribute can only be increased from the current value.", @@ -557,7 +577,10 @@ yargs type: "string", }) .check((argv: any, aliases: { [aliases: string]: string }): any => { - return isValidRollout(argv); + if (!isValidRollout(argv)) { + throw new Error("--rollout must be a whole percentage from 1 to 100, e.g. 25 or 25%"); + } + return true; }); addCommonConfiguration(yargs); @@ -628,7 +651,10 @@ yargs type: "string", }) .check((argv: any, aliases: { [aliases: string]: string }): any => { - return isValidRollout(argv); + if (!isValidRollout(argv)) { + throw new Error("--rollout must be a whole percentage from 1 to 100, e.g. 25 or 25%"); + } + return true; }); addCommonConfiguration(yargs); @@ -650,7 +676,9 @@ yargs 'Releases the "./platforms/ios/www" folder and all its contents to the "MyApp" app\'s "Production" deployment, targeting the 1.0.3 binary version and rolling out to about 20% of the users' ) .option("deploymentName", { - alias: "d", + // "deployment" is an alias because the docs used it for a long time while yargs silently + // swallowed it and released to Staging. All three spellings, so no pipeline breaks on strict mode. + alias: ["d", "deployment"], default: "Staging", demand: false, description: "Deployment to release the update to", @@ -692,14 +720,22 @@ yargs type: "string", }) .option("privateKey", { - alias: ["private-key", "k"], + // `privateKeyPath` / `private-key-path` are what upstream code-push called this and what older + // docs still show; accepted so a migrated script does not hard-fail under strictOptions. + alias: ["private-key", "privateKeyPath", "private-key-path", "k"], default: null, demand: false, - description: "RSA private key for code signing — either a file path (./private.pem) or inline PEM content", + description: "RSA private key for code signing: either a file path (./private.pem) or inline PEM content", type: "string", }) .check((argv: any, aliases: { [aliases: string]: string }): any => { - return checkValidReleaseOptions(argv); + if (!isValidRollout(argv)) { + throw new Error("--rollout must be a whole percentage from 1 to 100, e.g. 25 or 25%"); + } + if (!argv["deploymentName"]) { + throw new Error("--deploymentName needs a value, e.g. --deploymentName Staging"); + } + return true; }); addCommonConfiguration(yargs); @@ -729,7 +765,9 @@ yargs type: "string", }) .option("deploymentName", { - alias: "d", + // "deployment" is an alias because the docs used it for a long time while yargs silently + // swallowed it and released to Staging. All three spellings, so no pipeline breaks on strict mode. + alias: ["d", "deployment"], default: "Staging", demand: false, description: "Deployment to release the update to", @@ -828,14 +866,22 @@ yargs type: "string", }) .option("privateKey", { - alias: ["private-key", "k"], + // `privateKeyPath` / `private-key-path` are what upstream code-push called this and what older + // docs still show; accepted so a migrated script does not hard-fail under strictOptions. + alias: ["private-key", "privateKeyPath", "private-key-path", "k"], default: null, demand: false, - description: "RSA private key for code signing — either a file path (./private.pem) or inline PEM content", + description: "RSA private key for code signing: either a file path (./private.pem) or inline PEM content", type: "string", }) .check((argv: any, aliases: { [aliases: string]: string }): any => { - return checkValidReleaseOptions(argv); + if (!isValidRollout(argv)) { + throw new Error("--rollout must be a whole percentage from 1 to 100, e.g. 25 or 25%"); + } + if (!argv["deploymentName"]) { + throw new Error("--deploymentName needs a value, e.g. --deploymentName Staging"); + } + return true; }); addCommonConfiguration(yargs); @@ -884,10 +930,27 @@ yargs .example("whoami", "Display the account info for the current login session"); addCommonConfiguration(yargs); }) + // Without this yargs falls back to $0 and prints the entry file, so every subcommand listing read + // "cli.js app add" instead of "dpctl app add". + .scriptName("dpctl") .alias("v", "version") + // -h is help, the way it is in every other CLI. Declared explicitly so no option can claim it later. + .alias("h", "help") .version(packageJson.version) .wrap(/*columnLimit*/ null) - .fail((msg: string) => showHelp(/*showRootDescription*/ true)).argv; // Suppress the default error message. + // Same treatment as the per-command handler above: say what was wrong, then show help without the + // banner. This used to print the banner and throw `msg` away, so `dpctl --badflag` answered a typo with + // six lines of ASCII art and no explanation. The greeting for a bare `dpctl` comes from cli.ts, not here. + .fail((msg: string) => { + // A bare `dpctl` also lands here (yargs demands a command), and that is not a mistake to report: it + // gets the greeting. Anything else typed something wrong and wants the reason, not the banner. + const typedSomething = process.argv.slice(2).length > 0; + if (typedSomething && msg && msg !== lastFailMessage) { + lastFailMessage = msg; + console.error(chalk.red(`[Error] ${msg}`)); + } + showHelp(/*showRootDescription*/ !typedSomething); + }).argv; export function createCommand(): cli.ICommand { let cmd: cli.ICommand; diff --git a/script/node-version-check.ts b/script/node-version-check.ts new file mode 100644 index 0000000..7668d1b --- /dev/null +++ b/script/node-version-check.ts @@ -0,0 +1,13 @@ +// Imported first by cli.ts. parse-duration 2.x is ESM-only (earlier versions have a ReDoS advisory), and +// require()ing an ES module needs Node 20.19 or 22.12. Below that it throws ERR_REQUIRE_ESM before dpctl can +// print anything, and "engines" only makes npm warn. + +const [major, minor] = process.versions.node.split(".").map(Number); +const supported = major > 22 || (major === 22 && minor >= 12) || (major === 20 && minor >= 19); + +if (!supported) { + console.error(`[Error] dpctl needs Node.js 20.19 or later, or 22.12 or later. You are running Node.js ${process.versions.node}.`); + process.exit(1); +} + +export {}; diff --git a/test/command-parser.ts b/test/command-parser.ts new file mode 100644 index 0000000..0457e94 --- /dev/null +++ b/test/command-parser.ts @@ -0,0 +1,127 @@ +// Contract tests for the command line itself: exit codes and which flags are accepted. +// +// These run the real CLI in a child process rather than calling createCommand(), because what is being +// guarded here IS the process-level behaviour: the exit code a CI script branches on, and whether yargs +// accepts a flag at all. A unit call cannot observe either, and yargs keeps module state between parses. +// +// Every run gets an empty HOME and no DEPLOYPULSE_ACCESS_KEY, so there is no session file and the CLI +// stops at "You are not currently logged in" instead of doing real work against a real account. + +import * as assert from "assert"; +import * as fs from "fs"; +import * as os from "os"; +import * as path from "path"; +import { spawnSync } from "child_process"; + +const CLI_PATH = path.join(__dirname, "..", "script", "cli.ts"); + +interface Run { + status: number; + stdout: string; + stderr: string; + output: string; +} + +let sandboxHome: string; + +function run(...args: string[]): Run { + const result = spawnSync(process.execPath, ["-r", "ts-node/register", CLI_PATH, ...args], { + encoding: "utf8", + env: { + ...process.env, + HOME: sandboxHome, + LOCALAPPDATA: sandboxHome, + DEPLOYPULSE_ACCESS_KEY: "", + DEPLOYPULSE_ORG_ID: "", + TS_NODE_TRANSPILE_ONLY: "1", + // chalk would otherwise wrap the strings being matched in escape codes. + FORCE_COLOR: "0", + }, + }); + const stdout = result.stdout || ""; + const stderr = result.stderr || ""; + return { status: result.status, stdout, stderr, output: stdout + stderr }; +} + +describe("command line", function () { + // Each run boots ts-node, so this suite is seconds rather than milliseconds. + this.timeout(60000); + + before(() => { + sandboxHome = fs.mkdtempSync(path.join(os.tmpdir(), "dpctl-test-home-")); + }); + + after(() => { + fs.rmSync(sandboxHome, { recursive: true, force: true }); + }); + + // --------------------------------------------------------------------------- + // Exit codes. `dpctl` on its own is someone asking for help; anything typed and rejected is an error. + // --------------------------------------------------------------------------- + + it("a bare dpctl prints help and exits 0", () => { + const result = run(); + assert.strictEqual(result.status, 0, "a Dockerfile smoke check or a `set -e` script must not fail on this"); + assert.ok(/Usage: dpctl/.test(result.output), result.output.slice(0, 400)); + }); + + it("help is branded dpctl, not the entry file name", () => { + // yargs falls back to $0 without scriptName, which printed "cli.js app add" in every listing. + const result = run("app", "--help"); + assert.ok(/dpctl app add/.test(result.output), result.output.slice(0, 400)); + assert.ok(!/cli\.js/.test(result.output), `the entry file leaked into help: ${result.output.slice(0, 400)}`); + }); + + it("an unrecognized command exits 1 and says which one", () => { + const result = run("nonsense-command"); + assert.strictEqual(result.status, 1); + assert.ok(/Unrecognized command: nonsense-command/.test(result.output), result.output.slice(0, 400)); + }); + + // --------------------------------------------------------------------------- + // -h is help, everywhere, and reserved so no option can claim it later. + // --------------------------------------------------------------------------- + + it("-h prints help and exits 0, on a subcommand as well as the root", () => { + const result = run("release-react", "-h"); + assert.strictEqual(result.status, 0, "-h must not be read as an argument to the command"); + assert.ok(/Usage: dpctl release-react/.test(result.output), result.output.slice(0, 400)); + assert.ok(/-h, --help/.test(result.output), "-h should be listed as the help alias"); + }); + + // --------------------------------------------------------------------------- + // strictOptions rejects an unknown flag before anything runs, so "was it accepted?" is answered by + // whether the run got as far as the auth check. Every spelling below appears in older docs or in + // scripts migrated from upstream code-push, so all of them have to survive. + // --------------------------------------------------------------------------- + + const NOT_LOGGED_IN = /not currently logged in/; + const UNKNOWN_ARGUMENT = /Unknown argument/i; + + ["--private-key", "--private-key-path", "--privateKeyPath", "-k"].forEach((flag: string) => { + it(`release-react accepts ${flag}`, () => { + const result = run("release-react", "myapp", "ios", flag, "./private.pem"); + assert.ok(!UNKNOWN_ARGUMENT.test(result.output), `${flag} was rejected by strictOptions: ${result.output.slice(0, 300)}`); + // Reaching the auth check proves the parse succeeded and nothing was released. + assert.ok(NOT_LOGGED_IN.test(result.output), result.output.slice(0, 300)); + }); + }); + + it("an actually unknown option is still rejected", () => { + // The guard above is only meaningful if strictOptions still bites. + const result = run("release-react", "myapp", "ios", "--not-a-real-flag", "x"); + assert.notStrictEqual(result.status, 0); + assert.ok(UNKNOWN_ARGUMENT.test(result.output), result.output.slice(0, 300)); + }); + + it("--deployment is accepted as an alias for --deploymentName", () => { + const result = run("release-react", "myapp", "ios", "--deployment", "Production"); + assert.ok(!UNKNOWN_ARGUMENT.test(result.output), result.output.slice(0, 300)); + assert.ok(NOT_LOGGED_IN.test(result.output), result.output.slice(0, 300)); + }); +}); + +// --------------------------------------------------------------------------- +// Two dependencies of the destructive-command path, kept honest here because both broke silently once. +// --------------------------------------------------------------------------- +