Update EjecaError to be a custom type with custom toString

Summary:
We were creating `EjecaError`s via `new Error` and a type cast, and assigning extra properties. But this means `.toString` would not print the additional fields like exit code and stdout.

This makes it hard to debug issues, as we don't see stdout/stderr in the logs (though users should still see in their output UI).

This error format might be visible in the UI for certain errors, though I think that's fine?

I also noticed the EjecaError type is missing `code`, but some callers still check that. We should fix that too (not in this diff)

Reviewed By: muirdm

Differential Revision: D67994455

fbshipit-source-id: f7b2933187c17800037716609120f76417f2cdb6

Author: evangrayk

addons/isl-server/src/github/queryGraphQL.ts

@@ -53,6 +53,7 @@ export default async function queryGraphQL<TData, TVariables>(
     return json.data;
   } catch (error: unknown) {
     if (isEjecaError(error)) {
+      // FIXME: we're never setting `code` in ejeca, so this is always false!
       if (error.code === 'ENOENT' || error.code === 'EACCES') {
         // `gh` not installed on path
         throw new Error(`GhNotInstalledError: ${(error as Error).stack}`);

addons/isl-server/src/utils.ts

@@ -164,6 +164,7 @@ export function parseExecJson<T>(
     });
 }
 
+// FIXME: we're not ever setting `code`!
 export function isEjecaError(s: unknown): s is EjecaError & {code?: string} {
   return s != null && typeof s === 'object' && 'exitCode' in s;
 }

addons/isl/integrationTests/ejeca.test.tsx

@@ -6,11 +6,12 @@
  */
 
 import type {EjecaChildProcess} from 'shared/ejeca';
+
 import {ejeca} from 'shared/ejeca';
 
 describe('test running binaries', () => {
   it('we can get both streams and awaitables', async () => {
-    let spawned = ejeca('node', ['-e', "console.log('uno') ; console.error('dos')"]);
+    const spawned = ejeca('node', ['-e', "console.log('uno') ; console.error('dos')"]);
     let streamOut = '';
     let streamErr = '';
     spawned.stdout?.on('data', data => {
@@ -33,11 +34,8 @@ describe('test running binaries', () => {
 
   it('when erroring out the command name is present', async () => {
     const spawned = ejeca('node', ['-', 'foo("bar")'], {input: 'babar'});
-    await expect(spawned).rejects.toThrow(
-      expect.objectContaining({
-        escapedCommand: 'node "-" "foo(\\"bar\\")"',
-        message: 'Command `node "-" "foo(\\"bar\\")"` exited with non-zero status',
-      }),
+    await expect(spawned).rejects.toThrowErrorMatchingInlineSnapshot(
+      `"Command \`node "-" "foo(\\"bar\\")"\` exited with non-zero status with exit code 1"`,
     );
   });
 
@@ -120,7 +118,7 @@ console.log("Goodbye");
     killArgs: Parameters<EjecaChildProcess['kill']> = [],
     expectedSignal?: string,
   ) => {
-    let spawned = ejeca('node', ['-', ...pythonArgs], {
+    const spawned = ejeca('node', ['-', ...pythonArgs], {
       input: sighandlerScript,
     });
     setTimeout(() => spawned.kill(...killArgs), 1000);

addons/shared/ejeca.ts

@@ -8,6 +8,7 @@
 import type {ChildProcess, IOType, SpawnOptions, Serializable} from 'node:child_process';
 import type {Stream} from 'node:stream';
 
+import {truncate} from './utils';
 import getStream from 'get-stream';
 import {spawn} from 'node:child_process';
 import {Readable} from 'node:stream';
@@ -171,8 +172,6 @@ export interface EjecaReturn {
   killed: boolean;
 }
 
-export interface EjecaError extends Error, EjecaReturn {}
-
 interface EjecaChildPromise {
   catch<ResultType = never>(
     onRejected?: (reason: EjecaError) => ResultType | PromiseLike<ResultType>,
@@ -244,7 +243,7 @@ function getSpawnedPromise(
     values => {
       const [{exitCode, signal}, stdout, stderr] = values;
       const stripfinalNl = options?.stripFinalNewline ?? true;
-      const ret = {
+      const ret: EjecaReturn = {
         exitCode,
         signal,
         stdout: maybeStripFinalNewline(stdout, stripfinalNl),
@@ -253,23 +252,41 @@ function getSpawnedPromise(
         escapedCommand,
       };
       if (exitCode !== 0 || signal != undefined) {
-        const err = new Error(
-          `Command \`${ret.escapedCommand.slice(0, 50)}\` ` +
-            (signal != null ? 'was killed' : 'exited with non-zero status'),
-        ) as unknown as EjecaError;
-        err.exitCode = ret.exitCode;
-        err.signal = ret.signal;
-        err.stdout = ret.stdout;
-        err.stderr = ret.stderr;
-        err.killed = ret.killed;
-        err.escapedCommand = ret.escapedCommand;
-        throw err;
+        throw new EjecaError(ret);
       }
       return ret;
     },
   );
 }
 
+export class EjecaError extends Error implements EjecaReturn {
+  escapedCommand: string;
+  exitCode: number;
+  signal?: string;
+  stdout: string;
+  stderr: string;
+  killed: boolean;
+
+  constructor(info: EjecaReturn) {
+    const message =
+      `Command \`${truncate(info.escapedCommand, 50)}\` ` +
+      (info.signal != null ? 'was killed' : 'exited with non-zero status') +
+      (info.signal != null ? ` with signal ${info.signal}` : ` with exit code ${info.exitCode}`);
+    super(message);
+
+    this.exitCode = info.exitCode;
+    this.signal = info.signal;
+    this.stdout = info.stdout;
+    this.stderr = info.stderr;
+    this.killed = info.killed;
+    this.escapedCommand = info.escapedCommand;
+  }
+
+  toString() {
+    return `${this.message}\n${JSON.stringify(this, undefined, 2)}\n`;
+  }
+}
+
 function getStreamPromise(origStream: Stream | null): Promise<string> {
   const stream = origStream ?? new Readable({read() {}});
   return getStream(stream, {encoding: 'utf8'});