Repository: oclif/core
Pull request #861 (2023-11-09 14:44:18)

fix: add exit codes to different flag validation errors
@W-14408666@

adds different exit codes (3-7) for different flag/args validation errors

mshanemc: QA notes:  

build oclif/core and `yarn link` into the local CLI (from installer) `/Users/shane.mclaughlin/.local/share/sf/client/current`

run a command, then `echo $?` to see exitCode.
`sf version --foo` => 2
saying "no" to did-you-mean after entering a non-existent command => 127
`sf org create sandbox` (missing flag) => 1
`sf org list --no-prompt` (missing dependsOn) => 1
 
I *think* these are thrown from "inside" a command instead of during parse/validate
`sf update --version 0.0.0` => 1
`sf org display -o bad` => 1

---
and then we go customize the exitCodes prop 


`"nonExistentFlag": 404,` ❓   sf version --foo => 148 [I don't know how it gets to 148 from 404.  I think it's modulo 255?
`"nonExistentFlag": 8,` ✅  sf version --foo => 8 
`"nonExistentFlag": 33,` ✅  sf version --foo => 33

`"failedFlagValidation": 5,` `sf org list --no-prompt` (missing dependsOn) => 1 ❌ should have been 5.
[I tried a bunch of things and could never get `failedFlagValidation` to fire from `dependsOn`]
mdonnalley: > `"nonExistentFlag": 404,` ❓ sf version --foo => 148 [I don't know how it gets to 148 from 404. I think it's modulo 255?

Turns out that exit codes beyond 255 aren't supported: https://www.shellscript.sh/exitcodes.html
mdonnalley: > "failedFlagValidation": 5, sf org list --no-prompt (missing dependsOn) => 1 ❌ should have been 5.
[I tried a bunch of things and could never get failedFlagValidation to fire from dependsOn]

This is an issue with any command extending SfCommand. I'll have to fix that in a separate PR

Patch:
diff --git a/src/config/cache.ts b/src/cache.ts
similarity index 76%
rename from src/config/cache.ts
rename to src/cache.ts
index c91bc57..92041ca 100644
--- a/src/config/cache.ts
+++ b/src/cache.ts
@@ -1,7 +1,8 @@
-import {Plugin} from '../interfaces'
+import {PJSON, Plugin} from './interfaces'
 
 type CacheContents = {
   rootPlugin: Plugin
+  exitCodes: PJSON.Plugin['oclif']['exitCodes']
 }
 
 type ValueOf<T> = T[keyof T]
@@ -20,6 +21,7 @@ export default class Cache extends Map<keyof CacheContents, ValueOf<CacheContent
   }
 
   public get(key: 'rootPlugin'): Plugin | undefined
+  public get(key: 'exitCodes'): PJSON.Plugin['oclif']['exitCodes'] | undefined
   public get(key: keyof CacheContents): ValueOf<CacheContents> | undefined {
     return super.get(key)
   }
diff --git a/src/config/config.ts b/src/config/config.ts
index 9cf3ede..601f09d 100644
--- a/src/config/config.ts
+++ b/src/config/config.ts
@@ -4,6 +4,7 @@ import {arch, userInfo as osUserInfo, release, tmpdir, type} from 'node:os'
 import {join, resolve, sep} from 'node:path'
 import {URL, fileURLToPath} from 'node:url'
 
+import Cache from '../cache'
 import {ux} from '../cli-ux'
 import {parseTheme} from '../cli-ux/theme'
 import {Command} from '../command'
@@ -19,7 +20,6 @@ import {settings} from '../settings'
 import {requireJson, safeReadJson} from '../util/fs'
 import {getHomeDir, getPlatform} from '../util/os'
 import {compact, isProd} from '../util/util'
-import Cache from './cache'
 import PluginLoader from './plugin-loader'
 import {tsPath} from './ts-node'
 import {Debug, collectUsableIds, getCommandIdPermutations} from './util'
@@ -290,7 +290,9 @@ export class Config implements IConfig {
 
     // Cache the root plugin so that we can reference it later when determining if
     // we should skip ts-node registration for an ESM plugin.
-    Cache.getInstance().set('rootPlugin', this.rootPlugin)
+    const cache = Cache.getInstance()
+    cache.set('rootPlugin', this.rootPlugin)
+    cache.set('exitCodes', this.rootPlugin.pjson.oclif.exitCodes ?? {})
 
     this.root = this.rootPlugin.root
     this.pjson = this.rootPlugin.pjson
diff --git a/src/config/ts-node.ts b/src/config/ts-node.ts
index 2e9f19c..04b4f90 100644
--- a/src/config/ts-node.ts
+++ b/src/config/ts-node.ts
@@ -1,12 +1,12 @@
 import {join, relative as pathRelative, sep} from 'node:path'
 import * as TSNode from 'ts-node'
 
+import Cache from '../cache'
 import {memoizedWarn} from '../errors'
 import {Plugin, TSConfig} from '../interfaces'
 import {settings} from '../settings'
 import {existsSync, readTSConfig} from '../util/fs'
 import {isProd} from '../util/util'
-import Cache from './cache'
 import {Debug} from './util'
 // eslint-disable-next-line new-cap
 const debug = Debug('ts-node')
diff --git a/src/errors/errors/cli.ts b/src/errors/errors/cli.ts
index 06717d8..9016411 100644
--- a/src/errors/errors/cli.ts
+++ b/src/errors/errors/cli.ts
@@ -3,6 +3,7 @@ import cs from 'clean-stack'
 import indent from 'indent-string'
 import wrap from 'wrap-ansi'
 
+import Cache from '../../cache'
 import {OclifError, PrettyPrintableError} from '../../interfaces/errors'
 import {errtermwidth} from '../../screen'
 import {config} from '../config'
@@ -16,7 +17,7 @@ export function addOclifExitCode(error: Record<string, any>, options?: {exit?: f
     ;(error as unknown as OclifError).oclif = {}
   }
 
-  error.oclif.exit = options?.exit === undefined ? 2 : options.exit
+  error.oclif.exit = options?.exit === undefined ? Cache.getInstance().get('exitCodes')?.default ?? 2 : options.exit
   return error as OclifError
 }
 
diff --git a/src/interfaces/parser.ts b/src/interfaces/parser.ts
index d6899df..9fff88b 100644
--- a/src/interfaces/parser.ts
+++ b/src/interfaces/parser.ts
@@ -9,6 +9,7 @@ export type CLIParseErrorOptions = {
     input?: ParserInput
     output?: ParserOutput
   }
+  exit?: number
 }
 
 export type OutputArgs<T extends ParserInput['args']> = {[P in keyof T]: any}
diff --git a/src/interfaces/pjson.ts b/src/interfaces/pjson.ts
index 3f82f5f..883f7bc 100644
--- a/src/interfaces/pjson.ts
+++ b/src/interfaces/pjson.ts
@@ -25,6 +25,15 @@ export namespace PJSON {
       default?: string
       description?: string
       devPlugins?: string[]
+      exitCodes?: {
+        default?: number
+        failedFlagParsing?: number
+        failedFlagValidation?: number
+        invalidArgsSpec?: number
+        nonExistentFlag?: number
+        requiredArgs?: number
+        unexpectedArgs?: number
+      }
       flexibleTaxonomy?: boolean
       helpClass?: string
       helpOptions?: HelpOptions
diff --git a/src/parser/errors.ts b/src/parser/errors.ts
index 67d6344..5174a94 100644
--- a/src/parser/errors.ts
+++ b/src/parser/errors.ts
@@ -1,11 +1,10 @@
 import chalk from 'chalk'
 
+import Cache from '../cache'
 import {renderList} from '../cli-ux/list'
 import {CLIError} from '../errors'
-import {Flag, OptionFlag} from '../interfaces'
-import {Arg, ArgInput, CLIParseErrorOptions} from '../interfaces/parser'
+import {Arg, ArgInput, CLIParseErrorOptions, OptionFlag} from '../interfaces/parser'
 import {uniq} from '../util/util'
-import {flagUsages} from './help'
 
 export {CLIError} from '../errors'
 
@@ -21,7 +20,7 @@ export class CLIParseError extends CLIError {
 
   constructor(options: CLIParseErrorOptions & {message: string}) {
     options.message += '\nSee more help with --help'
-    super(options.message)
+    super(options.message, {exit: options.exit})
     this.parse = options.parse
   }
 }
@@ -29,7 +28,7 @@ export class CLIParseError extends CLIError {
 export class InvalidArgsSpecError extends CLIParseError {
   public args: ArgInput
 
-  constructor({args, parse}: CLIParseErrorOptions & {args: ArgInput}) {
+  constructor({args, exit, parse}: CLIParseErrorOptions & {args: ArgInput}) {
     let message = 'Invalid argument spec'
     const namedArgs = Object.values(args).filter((a) => a.name)
     if (namedArgs.length > 0) {
@@ -41,7 +40,7 @@ export class InvalidArgsSpecError extends CLIParseError {
       message += `:\n${list}`
     }
 
-    super({message, parse})
+    super({exit: Cache.getInstance().get('exitCodes')?.invalidArgsSpec ?? exit, message, parse})
     this.args = args
   }
 }
@@ -51,6 +50,7 @@ export class RequiredArgsError extends CLIParseError {
 
   constructor({
     args,
+    exit,
     flagsWithMultiple,
     parse,
   }: CLIParseErrorOptions & {args: Arg<any>[]; flagsWithMultiple?: string[]}) {
@@ -71,28 +71,17 @@ export class RequiredArgsError extends CLIParseError {
       message += '\nAlternatively, you can use "--" to signify the end of the flags and the beginning of arguments.'
     }
 
-    super({message, parse})
+    super({exit: Cache.getInstance().get('exitCodes')?.requiredArgs ?? exit, message, parse})
     this.args = args
   }
 }
 
-export class RequiredFlagError extends CLIParseError {
-  public flag: Flag<any>
-
-  constructor({flag, parse}: CLIParseErrorOptions & {flag: Flag<any>}) {
-    const usage = renderList(flagUsages([flag], {displayRequired: false}))
-    const message = `Missing required flag:\n${usage}`
-    super({message, parse})
-    this.flag = flag
-  }
-}
-
 export class UnexpectedArgsError extends CLIParseError {
   public args: unknown[]
 
-  constructor({args, parse}: CLIParseErrorOptions & {args: unknown[]}) {
+  constructor({args, exit, parse}: CLIParseErrorOptions & {args: unknown[]}) {
     const message = `Unexpected argument${args.length === 1 ? '' : 's'}: ${args.join(', ')}`
-    super({message, parse})
+    super({exit: Cache.getInstance().get('exitCodes')?.unexpectedArgs ?? exit, message, parse})
     this.args = args
   }
 }
@@ -100,9 +89,9 @@ export class UnexpectedArgsError extends CLIParseError {
 export class NonExistentFlagsError extends CLIParseError {
   public flags: string[]
 
-  constructor({flags, parse}: CLIParseErrorOptions & {flags: string[]}) {
+  constructor({exit, flags, parse}: CLIParseErrorOptions & {flags: string[]}) {
     const message = `Nonexistent flag${flags.length === 1 ? '' : 's'}: ${flags.join(', ')}`
-    super({message, parse})
+    super({exit: Cache.getInstance().get('exitCodes')?.nonExistentFlag ?? exit, message, parse})
     this.flags = flags
   }
 }
@@ -122,11 +111,21 @@ export class ArgInvalidOptionError extends CLIParseError {
 }
 
 export class FailedFlagValidationError extends CLIParseError {
-  constructor({failed, parse}: CLIParseErrorOptions & {failed: Validation[]}) {
+  constructor({exit, failed, parse}: CLIParseErrorOptions & {failed: Validation[]}) {
     const reasons = failed.map((r) => r.reason)
     const deduped = uniq(reasons)
     const errString = deduped.length === 1 ? 'error' : 'errors'
     const message = `The following ${errString} occurred:\n  ${chalk.dim(deduped.join('\n  '))}`
-    super({message, parse})
+    super({exit: Cache.getInstance().get('exitCodes')?.failedFlagValidation ?? exit, message, parse})
+  }
+}
+
+export class FailedFlagParsingError extends CLIParseError {
+  constructor({flag, message}: {flag: string; message: string}) {
+    super({
+      exit: Cache.getInstance().get('exitCodes')?.failedFlagParsing,
+      message: `Parsing --${flag} \n\t${message}`,
+      parse: {},
+    })
   }
 }
diff --git a/src/parser/parse.ts b/src/parser/parse.ts
index 3faf942..ba273ea 100644
--- a/src/parser/parse.ts
+++ b/src/parser/parse.ts
@@ -19,7 +19,7 @@ import {
   ParsingToken,
 } from '../interfaces/parser'
 import {isTruthy, last, pickBy} from '../util/util'
-import {ArgInvalidOptionError, CLIError, FlagInvalidOptionError} from './errors'
+import {ArgInvalidOptionError, CLIError, FailedFlagParsingError, FlagInvalidOptionError} from './errors'
 
 let debug: any
 try {
@@ -341,8 +341,7 @@ export class Parser<
 
         return await flag.parse(input, ctx, flag)
       } catch (error: any) {
-        error.message = `Parsing --${flag.name} \n\t${error.message}\nSee more help with --help`
-        throw error
+        throw new FailedFlagParsingError({flag: flag.name, message: error.message})
       }
     }
 
diff --git a/src/parser/validate.ts b/src/parser/validate.ts
index 2a72fee..d2caee7 100644
--- a/src/parser/validate.ts
+++ b/src/parser/validate.ts
@@ -14,13 +14,19 @@ export async function validate(parse: {input: ParserInput; output: ParserOutput}
 
   function validateArgs() {
     if (parse.output.nonExistentFlags?.length > 0) {
-      throw new NonExistentFlagsError({flags: parse.output.nonExistentFlags, parse})
+      throw new NonExistentFlagsError({
+        flags: parse.output.nonExistentFlags,
+        parse,
+      })
     }
 
     const maxArgs = Object.keys(parse.input.args).length
     if (parse.input.strict && parse.output.argv.length > maxArgs) {
       const extras = parse.output.argv.slice(maxArgs)
-      throw new UnexpectedArgsError({args: extras, parse})
+      throw new UnexpectedArgsError({
+        args: extras,
+        parse,
+      })
     }
 
     const missingRequiredArgs: Arg<any>[] = []
@@ -32,7 +38,10 @@ export async function validate(parse: {input: ParserInput; output: ParserOutput}
       } else if (hasOptional) {
         // (required arg) check whether an optional has occurred before
         // optionals should follow required, not before
-        throw new InvalidArgsSpecError({args: parse.input.args, parse})
+        throw new InvalidArgsSpecError({
+          args: parse.input.args,
+          parse,
+        })
       }
 
       if (arg.required && !parse.output.args[name] && parse.output.args[name] !== 0) {
@@ -45,7 +54,11 @@ export async function validate(parse: {input: ParserInput; output: ParserOutput}
         .filter(([_, flagDef]) => flagDef.type === 'option' && Boolean(flagDef.multiple))
         .map(([name]) => name)
 
-      throw new RequiredArgsError({args: missingRequiredArgs, flagsWithMultiple, parse})
+      throw new RequiredArgsError({
+        args: missingRequiredArgs,
+        flagsWithMultiple,
+        parse,
+      })
     }
   }
 
@@ -76,7 +89,11 @@ export async function validate(parse: {input: ParserInput; output: ParserOutput}
     const results = await Promise.all(promises)
 
     const failed = results.filter((r) => r.status === 'failed')
-    if (failed.length > 0) throw new FailedFlagValidationError({failed, parse})
+    if (failed.length > 0)
+      throw new FailedFlagValidationError({
+        failed,
+        parse,
+      })
   }
 
   async function resolveFlags(flags: FlagRelationship[]): Promise<Record<string, unknown>> {


Test patch:
diff --git a/test/parser/error-codes.test.ts b/test/parser/error-codes.test.ts
new file mode 100644
index 0000000..7d3b83d
--- /dev/null
+++ b/test/parser/error-codes.test.ts
@@ -0,0 +1,123 @@
+import {expect} from 'chai'
+import {join, resolve} from 'node:path'
+import {SinonSandbox, createSandbox} from 'sinon'
+
+import {Config} from '../../src'
+import Cache from '../../src/cache'
+import {CLIError} from '../../src/errors'
+
+function hasExitCode(error: unknown, expectedCode: number): void {
+  if (error instanceof Error && error.message === 'Expected command to fail but it passed') {
+    throw error
+  }
+
+  if (error instanceof CLIError) {
+    expect(error.oclif.exit).to.equal(expectedCode)
+  } else {
+    expect.fail('Expected CLIError')
+  }
+}
+
+type AsyncFunction = (...args: unknown[]) => Promise<unknown>
+
+async function runCommand(fn: AsyncFunction, expectedCode: number): Promise<void> {
+  try {
+    await fn()
+    expect.fail('Expected command to fail but it passed')
+  } catch (error) {
+    hasExitCode(error, expectedCode)
+  }
+}
+
+describe('configurable error codes', () => {
+  let sandbox: SinonSandbox
+  let config: Config
+
+  const defaultExitCode = 2
+  const exitCodes = {
+    failedFlagParsing: 101,
+    failedFlagValidation: 102,
+    invalidArgsSpec: 103,
+    nonExistentFlag: 104,
+    requiredArgs: 105,
+    unexpectedArgs: 106,
+  }
+
+  beforeEach(async () => {
+    sandbox = createSandbox()
+    config = await Config.load(resolve(__dirname, join('fixtures', 'test-plugin')))
+  })
+
+  afterEach(() => {
+    sandbox.restore()
+  })
+
+  describe('failedFlagParsing', () => {
+    it('should use default exit code for failed flag parsing', async () => {
+      await runCommand(() => config.runCommand('test', ['--flag1', '100', '--flag2', 'arg1']), defaultExitCode)
+    })
+
+    it('should use configured exit code for failed flag parsing', async () => {
+      sandbox.stub(Cache.prototype, 'get').withArgs('exitCodes').returns(exitCodes)
+      await runCommand(
+        () => config.runCommand('test', ['--flag1', '100', '--flag2', 'arg1']),
+        exitCodes.failedFlagParsing,
+      )
+    })
+  })
+
+  describe('failedFlagValidation', () => {
+    it('should use default exit code for failed flag parsing', async () => {
+      await runCommand(() => config.runCommand('test', ['--flag2', 'arg1']), defaultExitCode)
+    })
+
+    it('should use configured exit code for failed flag validation', async () => {
+      sandbox.stub(Cache.prototype, 'get').withArgs('exitCodes').returns(exitCodes)
+      await runCommand(() => config.runCommand('test', ['--flag2', 'arg1']), exitCodes.failedFlagValidation)
+    })
+  })
+
+  describe('invalidArgsSpec', () => {
+    it('should use default exit code for failed flag parsing', async () => {
+      await runCommand(() => config.runCommand('invalid', ['arg1', 'arg2']), defaultExitCode)
+    })
+
+    it('should use configured exit code for invalid args spec', async () => {
+      sandbox.stub(Cache.prototype, 'get').withArgs('exitCodes').returns(exitCodes)
+      await runCommand(() => config.runCommand('invalid', ['arg1', 'arg2']), exitCodes.invalidArgsSpec)
+    })
+  })
+
+  describe('nonExistentFlag', () => {
+    it('should use default exit code for failed flag parsing', async () => {
+      await runCommand(() => config.runCommand('test', ['--DOES_NOT_EXIST', 'arg1']), defaultExitCode)
+    })
+
+    it('should use configured exit code for failed flag validation', async () => {
+      sandbox.stub(Cache.prototype, 'get').withArgs('exitCodes').returns(exitCodes)
+      await runCommand(() => config.runCommand('test', ['--DOES_NOT_EXIST', 'arg1']), exitCodes.nonExistentFlag)
+    })
+  })
+
+  describe('requiredArgs', () => {
+    it('should use default exit code for failed flag parsing', async () => {
+      await runCommand(() => config.runCommand('test', ['--flag1', '1']), defaultExitCode)
+    })
+
+    it('should use configured exit code for failed flag validation', async () => {
+      sandbox.stub(Cache.prototype, 'get').withArgs('exitCodes').returns(exitCodes)
+      await runCommand(() => config.runCommand('test', ['--flag1', '1']), exitCodes.requiredArgs)
+    })
+  })
+
+  describe('unexpectedArgs', () => {
+    it('should use default exit code for failed flag parsing', async () => {
+      await runCommand(() => config.runCommand('test', ['arg1', 'arg2', 'arg3']), defaultExitCode)
+    })
+
+    it('should use configured exit code for failed flag validation', async () => {
+      sandbox.stub(Cache.prototype, 'get').withArgs('exitCodes').returns(exitCodes)
+      await runCommand(() => config.runCommand('test', ['arg1', 'arg2', 'arg3']), exitCodes.unexpectedArgs)
+    })
+  })
+})
diff --git a/test/parser/fixtures/test-plugin/package.json b/test/parser/fixtures/test-plugin/package.json
new file mode 100644
index 0000000..2eb5d4c
--- /dev/null
+++ b/test/parser/fixtures/test-plugin/package.json
@@ -0,0 +1,8 @@
+{
+  "name": "test-plugin",
+  "private": true,
+  "files": [],
+  "oclif": {
+    "commands": "./lib/commands"
+  }
+}
diff --git a/test/parser/fixtures/test-plugin/src/commands/invalid.ts b/test/parser/fixtures/test-plugin/src/commands/invalid.ts
new file mode 100644
index 0000000..a87b226
--- /dev/null
+++ b/test/parser/fixtures/test-plugin/src/commands/invalid.ts
@@ -0,0 +1,28 @@
+import {Args, Command, Flags} from '../../../../../../src'
+
+export default class Invalid extends Command {
+  public static readonly args = {
+    arg1: Args.string({
+      required: false,
+    }),
+    arg2: Args.string({
+      required: true,
+    }),
+  }
+
+  public static readonly flags = {
+    flag1: Flags.integer({
+      min: 1,
+      max: 10,
+      required: true,
+    }),
+    flag2: Flags.boolean({
+      dependsOn: ['flag1'],
+    }),
+  }
+
+  public async run(): Promise<unknown> {
+    const {args, flags} = await this.parse(Invalid)
+    return {args, flags}
+  }
+}
diff --git a/test/parser/fixtures/test-plugin/src/commands/test.ts b/test/parser/fixtures/test-plugin/src/commands/test.ts
new file mode 100644
index 0000000..aa83912
--- /dev/null
+++ b/test/parser/fixtures/test-plugin/src/commands/test.ts
@@ -0,0 +1,25 @@
+import {Args, Command, Flags} from '../../../../../../src'
+
+export default class Test extends Command {
+  public static readonly args = {
+    arg1: Args.string({
+      required: true,
+    }),
+  }
+
+  public static readonly flags = {
+    flag1: Flags.integer({
+      min: 1,
+      max: 10,
+      required: true,
+    }),
+    flag2: Flags.boolean({
+      dependsOn: ['flag1'],
+    }),
+  }
+
+  public async run(): Promise<unknown> {
+    const {args, flags} = await this.parse(Test)
+    return {args, flags}
+  }
+}
diff --git a/test/parser/fixtures/test-plugin/tsconfig.json b/test/parser/fixtures/test-plugin/tsconfig.json
new file mode 100644
index 0000000..8d00831
--- /dev/null
+++ b/test/parser/fixtures/test-plugin/tsconfig.json
@@ -0,0 +1,7 @@
+{
+  "compilerOptions": {
+    "outDir": "./lib",
+    "rootDirs": ["./src"]
+  },
+  "include": ["./src/**/*"]
+}