fix(cli): exit code 1 on unknown commands, sub-commands and flags (#5235

)

* test(cli): test specs to describe the expected behavior

* fix(cli): exit with code 1 on unknown commands, sub-commands and flags

Handle `--help` flag separately.

core/src/cli/cli.ts

@@ -12,7 +12,7 @@ import chalk from "chalk"
 import { pathExists } from "fs-extra"
 import { getBuiltinCommands } from "../commands/commands"
 import { shutdown, getPackageVersion, getCloudDistributionName } from "../util/util"
-import { Command, CommandResult, CommandGroup, BuiltinArgs } from "../commands/base"
+import { Command, CommandResult, BuiltinArgs, CommandGroup } from "../commands/base"
 import { PluginError, toGardenError, GardenError } from "../exceptions"
 import { Garden, GardenOpts, makeDummyGarden } from "../garden"
 import { getRootLogger, getTerminalWriterType, LogLevel, parseLogLevel, RootLogger } from "../logger/logger"
@@ -51,6 +51,7 @@ import { uuidv4 } from "../util/random"
 import { withSessionContext } from "../util/open-telemetry/context"
 import { wrapActiveSpan } from "../util/open-telemetry/spans"
 import { JsonFileWriter } from "../logger/writers/json-file-writer"
+import minimist from "minimist"
 
 export interface RunOutput {
   argv: any
@@ -67,6 +68,10 @@ export interface GardenCliParams {
   cloudApiFactory?: CloudApiFactory
 }
 
+function hasHelpFlag(argv: minimist.ParsedArgs) {
+  return argv.h || argv.help
+}
+
 // TODO: this is used in more contexts now, should rename to GardenCommandRunner or something like that
 @Profile()
 export class GardenCli {
@@ -482,32 +487,65 @@ ${renderCommands(commands)}
 
     // If we still haven't found a valid command, print help
     if (!command) {
-      const exitCode = argv._.length === 0 || argv._[0] === "help" ? 0 : 1
+      const exitCode = argv._.length === 0 && hasHelpFlag(argv) ? 0 : 1
       return done(exitCode, await this.renderHelp(log, workingDir))
     }
 
     // Parse the arguments again with the Command set, to fully validate, and to ensure boolean options are
-    // handled correctly
+    // handled correctly.
     argv = parseCliArgs({ stringArgs: args, command, cli: true })
 
     // Slice command name from the positional args
     argv._ = argv._.slice(command.getPath().length)
 
-    // Handle -h, --help, and subcommand listings
-    if (argv.h || argv.help || command instanceof CommandGroup) {
-      // Try to show specific help for given subcommand
+    // Handle -h and --help flags, those are always valid and should return exit code 0
+    if (hasHelpFlag(argv)) {
+      // Handle subcommand listings
       if (command instanceof CommandGroup) {
+        const subCommandName = rest[0]
+        if (subCommandName === undefined) {
+          // Exit code 0 if sub-command is not specified and --help flag is passed, e.g. garden get --help
+          return done(0, command.renderHelp())
+        }
+
+        // Try to show specific help for given subcommand
         for (const subCommand of command.subCommands) {
           const sub = new subCommand()
           if (sub.name === rest[0]) {
             return done(0, sub.renderHelp())
           }
         }
-        // If not found, falls through to general command help below
+
+        // If sub-command was not found, then the sub-command name is incorrect.
+        // Falls through to general command help and exit code 1.
+        return done(1, command.renderHelp())
       }
+
       return done(0, command.renderHelp())
     }
 
+    // Handle incomplete subcommand listings.
+    // A complete sub-command won't be recognized as CommandGroup.
+    if (command instanceof CommandGroup) {
+      const subCommandName = rest[0]
+      if (subCommandName === undefined) {
+        // exit code 1 if sub-command is missing
+        return done(1, command.renderHelp())
+      }
+
+      // Try to show specific help for given subcommand
+      for (const subCommand of command.subCommands) {
+        const sub = new subCommand()
+        if (sub.name === rest[0]) {
+          return done(1, sub.renderHelp())
+        }
+      }
+
+      // If sub-command was not found, then the sub-command name is incorrect.
+      // Falls through to general command help and exit code 1.
+      return done(1, command.renderHelp())
+    }
+
     let parsedArgs: BuiltinArgs & ParameterValues<ParameterObject>
     let parsedOpts: ParameterValues<GlobalOptions & ParameterObject>
 

core/test/unit/src/cli/cli.ts

@@ -56,7 +56,7 @@ describe("cli", () => {
     it("aborts with help text if no positional argument is provided", async () => {
       const { code, consoleOutput } = await cli.run({ args: [], exitOnError: false })
 
-      expect(code).to.equal(0)
+      expect(code).to.equal(1)
       expect(consoleOutput).to.equal(await cli.renderHelp(log, "/"))
     })
 
@@ -187,6 +187,126 @@ describe("cli", () => {
       })
     })
 
+    context("exit codes", () => {
+      const cwd = getDataDir("test-project-a")
+
+      context("garden binary", () => {
+        it("garden exits with code 1 on no flags", async () => {
+          const res = await cli.run({ args: [], exitOnError: false, cwd })
+
+          expect(res.code).to.equal(1)
+        })
+
+        it("garden exits with code 0 on --help flag", async () => {
+          const res = await cli.run({ args: ["--help"], exitOnError: false, cwd })
+
+          expect(res.code).to.equal(0)
+        })
+
+        it("garden exits with code 1 on unrecognized flag", async () => {
+          const res = await cli.run({ args: ["--i-am-groot"], exitOnError: false, cwd })
+
+          expect(res.code).to.equal(1)
+          // TODO: this requires more complicated chnages in the args parsing flow,
+          //  let's do it in a separate PR
+          // expect(stripAnsi(res.consoleOutput!).toLowerCase()).to.contain("unrecognized option flag")
+        })
+      })
+
+      context("garden command without sub-commands", () => {
+        it("garden exits with code 0 on recognized command", async () => {
+          const res = await cli.run({ args: ["validate"], exitOnError: false, cwd })
+
+          expect(res.code).to.equal(0)
+        })
+
+        it("garden exits with code 0 on recognized command with --help flag", async () => {
+          const res = await cli.run({ args: ["validate", "--help"], exitOnError: false, cwd })
+
+          expect(res.code).to.equal(0)
+        })
+
+        it("garden exits with code 1 on recognized command with unrecognized flag", async () => {
+          const res = await cli.run({ args: ["validate", "--i-am-groot"], exitOnError: false, cwd })
+
+          expect(res.code).to.equal(1)
+          expect(stripAnsi(res.consoleOutput!).toLowerCase()).to.contain("unrecognized option flag")
+        })
+
+        it("garden exits with code 1 on unrecognized command", async () => {
+          const res = await cli.run({ args: ["baby-groot"], exitOnError: false, cwd })
+
+          expect(res.code).to.equal(1)
+        })
+
+        it("garden exits with code 1 on unrecognized command with --help flag", async () => {
+          const res = await cli.run({ args: ["baby-groot", "--help"], exitOnError: false, cwd })
+
+          expect(res.code).to.equal(1)
+        })
+
+        it("garden exits with code 1 on unrecognized command with unrecognized flag", async () => {
+          const res = await cli.run({ args: ["baby-groot", "--i-am-groot"], exitOnError: false, cwd })
+
+          expect(res.code).to.equal(1)
+        })
+      })
+
+      // Command with sub-commands is always a recognized command, so here we test only flags.
+      context("garden command with sub-commands", () => {
+        it("garden exits with code 0 on incomplete sub-command with --help flag", async () => {
+          const res = await cli.run({ args: ["get", "--help"], exitOnError: false, cwd })
+
+          expect(res.code).to.equal(0)
+        })
+
+        it("garden exits with code 1 on incomplete sub-command with unrecognized flag", async () => {
+          const res = await cli.run({ args: ["get", "--i-am-groot"], exitOnError: false, cwd })
+
+          expect(res.code).to.equal(1)
+        })
+      })
+
+      context("garden sub-command", () => {
+        it("garden exits with code 0 on recognized sub-command", async () => {
+          const res = await cli.run({ args: ["get", "actions"], exitOnError: false, cwd })
+
+          expect(res.code).to.equal(0)
+        })
+
+        it("garden exits with code 0 on recognized sub-command with --help flag", async () => {
+          const res = await cli.run({ args: ["get", "actions", "--help"], exitOnError: false, cwd })
+
+          expect(res.code).to.equal(0)
+        })
+
+        it("garden exits with code 1 on recognized sub-command with unrecognized flag", async () => {
+          const res = await cli.run({ args: ["get", "actions", "--i-am-groot"], exitOnError: false, cwd })
+
+          expect(res.code).to.equal(1)
+          expect(stripAnsi(res.consoleOutput!).toLowerCase()).to.contain("unrecognized option flag")
+        })
+
+        it("garden exits with code 1 on unrecognized sub-command", async () => {
+          const res = await cli.run({ args: ["get", "baby-groot"], exitOnError: false, cwd })
+
+          expect(res.code).to.equal(1)
+        })
+
+        it("garden exits with code 1 on unrecognized sub-command with --help flag", async () => {
+          const res = await cli.run({ args: ["get", "baby-groot", "--help"], exitOnError: false, cwd })
+
+          expect(res.code).to.equal(1)
+        })
+
+        it("garden exits with code 1 on unrecognized sub-command with unrecognized flag", async () => {
+          const res = await cli.run({ args: ["get", "baby-groot", "--i-am-groot"], exitOnError: false, cwd })
+
+          expect(res.code).to.equal(1)
+        })
+      })
+    })
+
     context("test logger initialization", () => {
       const envLoggerType = process.env.GARDEN_LOGGER_TYPE
       const envLogLevel = process.env.GARDEN_LOG_LEVEL
@@ -238,7 +358,7 @@ describe("cli", () => {
       const cmd = new UtilCommand()
       const { code, consoleOutput } = await cli.run({ args: ["util"], exitOnError: false })
 
-      expect(code).to.equal(0)
+      expect(code).to.equal(1)
       expect(consoleOutput).to.equal(cmd.renderHelp())
     })
 
@@ -247,7 +367,7 @@ describe("cli", () => {
       const secrets = new cmd.subCommands[0]()
       const { code, consoleOutput } = await cli.run({ args: ["cloud", "secrets"], exitOnError: false })
 
-      expect(code).to.equal(0)
+      expect(code).to.equal(1)
       expect(consoleOutput).to.equal(secrets.renderHelp())
     })