From 7b8204303f0f18aa9e0de20dc3cadbededdc91e8 Mon Sep 17 00:00:00 2001
From: Evan Krause <evangrayk@meta.com>
Date: Thu, 9 Jan 2025 17:34:20 -0800
Subject: [PATCH] Distinguish EjecaSpawnError from EjecaError

Summary:
There's two error types from Ejeca:
1. a command spawns then exits non-zero
2. a command does not spawn (syscall to spawn fails)

(1) is what we typically think of as an EjecaError, and contains most normal errors.

(2) can happen with ENOENT if `sl` or other commands like `gh` are not on your PATH.

The distinction is important for good error messages. Let's make them separate types to keep these separate.

Reviewed By: quark-zju

Differential Revision: D67995419

fbshipit-source-id: ee8df2d40929277280f564b36ac07e9a1a56c336
---
 addons/isl-server/src/github/queryGraphQL.ts | 10 ++++++----
 addons/isl-server/src/utils.ts               | 15 +++++++++++++--
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/addons/isl-server/src/github/queryGraphQL.ts b/addons/isl-server/src/github/queryGraphQL.ts
index ada195428ca88..134e7ab277fa4 100644
--- a/addons/isl-server/src/github/queryGraphQL.ts
+++ b/addons/isl-server/src/github/queryGraphQL.ts
@@ -6,7 +6,7 @@
  */
 
 import {Internal} from '../Internal';
-import {isEjecaError} from '../utils';
+import {isEjecaError, isEjecaSpawnError} from '../utils';
 import {ejeca} from 'shared/ejeca';
 
 export default async function queryGraphQL<TData, TVariables>(
@@ -52,12 +52,14 @@ 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 (isEjecaSpawnError(error)) {
       if (error.code === 'ENOENT' || error.code === 'EACCES') {
         // `gh` not installed on path
         throw new Error(`GhNotInstalledError: ${(error as Error).stack}`);
-      } else if (error.exitCode === 4) {
+      }
+    } else if (isEjecaError(error)) {
+      // FIXME: we're never setting `code` in ejeca, so this is always false!
+      if (error.exitCode === 4) {
         // `gh` CLI exit code 4 => authentication issue
         throw new Error(`NotAuthenticatedError: ${(error as Error).stack}`);
       }
diff --git a/addons/isl-server/src/utils.ts b/addons/isl-server/src/utils.ts
index dbd8204bb08d2..f3f4960383215 100644
--- a/addons/isl-server/src/utils.ts
+++ b/addons/isl-server/src/utils.ts
@@ -164,11 +164,22 @@ export function parseExecJson<T>(
     });
 }
 
-// FIXME: we're not ever setting `code`!
-export function isEjecaError(s: unknown): s is EjecaError & {code?: string} {
+export type EjecaSpawnError = Error & {code: string; path: string};
+
+/**
+ * True if an Ejeca spawned process exits non-zero or is killed.
+ * @see {EjecaSpawnError} for when a process fails to spawn in the first place (e.g. ENOENT).
+ */
+export function isEjecaError(s: unknown): s is EjecaError {
   return s != null && typeof s === 'object' && 'exitCode' in s;
 }
 
+/** True when Ejeca fails to spawn a process, e.g. ENOENT.
+ * (as opposed to the command spawning, then exiting non-zero) */
+export function isEjecaSpawnError(s: unknown): s is EjecaSpawnError {
+  return s != null && typeof s === 'object' && 'code' in s;
+}
+
 export function fromEntries<V>(entries: Array<[string, V]>): {
   [key: string]: V;
 } {