feat(cli): require Node 20.19+, reject unknown flags, exit non-zero on rejection

.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

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).

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",

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;
   }
 

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;

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 {};

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.
+// ---------------------------------------------------------------------------
+