fix: handle oclif errors correctly

Author: shetzel

src/SfCommandError.ts

@@ -75,14 +75,13 @@ export class SfCommandError extends SfError {
   ): SfCommandError {
     // SfError.wrap() does most of what we want so start with that.
     const sfError = SfError.wrap(err);
-    const exitCode = computeErrorCode(sfError);
-    sfError.code = 'code' in err ? err.code : exitCode.toString(10);
+    const exitCode = computeErrorCode(err);
     return new this({
       message: sfError.message,
       name: err.name ?? 'Error',
       actions: 'actions' in err ? err.actions : undefined,
       exitCode,
-      code: sfError.code,
+      code: 'code' in err && err.code ? err.code : exitCode.toString(10),
       cause: sfError.cause,
       commandName: 'commandName' in err ? err.commandName : commandName,
       data: 'data' in err ? err.data : undefined,

src/errorHandling.ts

@@ -6,8 +6,7 @@
  */
 
 import { SfError } from '@salesforce/core';
-import { CLIError } from '@oclif/core/errors';
-import { SfCommandError } from './SfCommandError.js';
+import { Errors } from '@oclif/core';
 
 /**
  *
@@ -20,7 +19,7 @@ import { SfCommandError } from './SfCommandError.js';
  * - use the process exitCode
  * - default to 1
  */
-export const computeErrorCode = (e: Error | SfError | SfCommandError): number => {
+export const computeErrorCode = (e: Error | SfError | Errors.CLIError): number => {
   // regardless of the exitCode, we'll set gacks and TypeError to a specific exit code
   if (errorIsGack(e)) {
     return 20;
@@ -66,5 +65,5 @@ export const errorIsTypeError = (error: Error | SfError): boolean =>
   ('cause' in error && error.cause instanceof Error && errorIsTypeError(error.cause));
 
 /** custom typeGuard for handling the fact the SfCommand doesn't know about oclif error structure */
-const isOclifError = <T extends Error | SfError | SfCommandError>(e: T): e is T & CLIError =>
+const isOclifError = <T extends Error | SfError | Errors.CLIError>(e: T): e is T & Errors.CLIError =>
   'oclif' in e ? true : false;

src/sfCommand.ts

@@ -5,7 +5,7 @@
  * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
  */
 import os from 'node:os';
-import { Command, Config, HelpSection, Flags } from '@oclif/core';
+import { Errors, Command, Config, HelpSection, Flags } from '@oclif/core';
 import {
   envVars,
   Messages,
@@ -367,7 +367,7 @@ export abstract class SfCommand<T> extends Command {
   }
 
   // eslint-disable-next-line @typescript-eslint/require-await
-  protected async catch(error: Error | SfError | SfCommand.Error): Promise<never> {
+  protected async catch(error: Error | SfError | Errors.CLIError): Promise<never> {
     // stop any spinners to prevent it from unintentionally swallowing output.
     // If there is an active spinner, it'll say "Error" instead of "Done"
     this.spinner.stop(StandardColors.error('Error'));

test/unit/sfCommand.test.ts

@@ -5,6 +5,7 @@
  * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
  */
 import { Flags } from '@oclif/core';
+import { Errors } from '@oclif/core';
 import { Lifecycle } from '@salesforce/core';
 import { TestContext } from '@salesforce/core/testSetup';
 import { assert, expect } from 'chai';
@@ -70,12 +71,20 @@ class TestCommandErrors extends SfCommand<void> {
       data: errData,
     });
 
+  public static buildOclifError = () => {
+    const err = new Errors.CLIError('Nonexistent flag: --INVALID\nSee more help with --help');
+    err.oclif = { exit: 2 };
+    err.code = undefined;
+    return err;
+  };
+
   // eslint-disable-next-line @typescript-eslint/member-ordering
   public static errors: { [x: string]: Error } = {
     error: new Error('error message'),
     sfError: new SfError('sfError message'),
     fullError: TestCommandErrors.buildFullError(),
     fullSfError: TestCommandErrors.buildFullSfError(),
+    oclifError: TestCommandErrors.buildOclifError(),
   };
 
   // eslint-disable-next-line @typescript-eslint/member-ordering
@@ -239,6 +248,38 @@ describe('error standardization', () => {
 
   afterEach(() => {
     process.removeListener('sfCommandError', sfCommandErrorCb);
+    process.exitCode = undefined;
+  });
+
+  it('should log correct error when command throws an oclif Error', async () => {
+    const logToStderrStub = $$.SANDBOX.stub(SfCommand.prototype, 'logToStderr');
+    try {
+      await TestCommandErrors.run(['--error', 'oclifError']);
+      expect(false, 'error should have been thrown').to.be.true;
+    } catch (e: unknown) {
+      expect(e).to.be.instanceOf(SfCommandError);
+      const err = e as SfCommand.Error;
+
+      // Ensure the error was logged to the console
+      expect(logToStderrStub.callCount).to.equal(1);
+      expect(logToStderrStub.firstCall.firstArg).to.contain(err.message);
+
+      // Ensure the error has expected properties
+      expect(err).to.have.property('actions', undefined);
+      expect(err).to.have.property('exitCode', 2);
+      expect(err).to.have.property('context', 'TestCommandErrors');
+      expect(err).to.have.property('data', undefined);
+      expect(err).to.have.property('cause').and.be.ok;
+      expect(err).to.have.property('code', '2');
+      expect(err).to.have.property('status', 2);
+      expect(err).to.have.property('stack').and.be.ok;
+      expect(err).to.have.property('skipOclifErrorHandling', true);
+      expect(err).to.have.deep.property('oclif', { exit: 2 });
+
+      // Ensure a sfCommandError event was emitted with the expected data
+      expect(sfCommandErrorData[0]).to.equal(err);
+      expect(sfCommandErrorData[1]).to.equal('testcommanderrors');
+    }
   });
 
   it('should log correct error when command throws an Error', async () => {