From e0cd94b03e06eb469c69e8cc8ad378c4a054e186 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 13 Sep 2026 01:59:44 -0400 Subject: [PATCH] fix: address Copilot review round 1 (#2333) A runner that threw ETIMEDOUT landed in the catch and read as unavailable, bypassing the split the previous commit added. The thrown error is now normalized onto the `{ error }` path so the timeout check covers both shapes; a test pins the thrown case. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp Signed-off-by: cliffhall --- scripts/lib/pty.mjs | 9 ++++++--- scripts/lib/pty.test.mjs | 9 +++++++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/scripts/lib/pty.mjs b/scripts/lib/pty.mjs index df816e97..74be1485 100644 --- a/scripts/lib/pty.mjs +++ b/scripts/lib/pty.mjs @@ -165,9 +165,12 @@ export function probeScriptVersion( let r; try { r = runner("script", ["--version"]); - } catch { - // A runner that throws outright (injected, or a future spawn shape). - return { available: false, output: "" }; + } catch (err) { + // A runner that throws outright (injected, or a future spawn shape) is + // normalized onto the same path as one that returns `{ error }`, so the + // timeout check below applies to both shapes — a thrown ETIMEDOUT must not + // slip back into "unavailable" (Copilot, #2333). + r = { error: err }; } if (r?.error?.code === "ETIMEDOUT") { throw new Error( diff --git a/scripts/lib/pty.test.mjs b/scripts/lib/pty.test.mjs index dad61a9d..3f4d4fc0 100644 --- a/scripts/lib/pty.test.mjs +++ b/scripts/lib/pty.test.mjs @@ -189,6 +189,15 @@ test("probeScriptVersion throws on a timed-out probe instead of reporting no scr probeScriptVersion(() => ({ error: timedOut, stdout: "", stderr: "" })), new RegExp(`did not exit within ${SCRIPT_PROBE_TIMEOUT_MS}ms`), ); + // A runner that THROWS the same error is normalized onto the same path — the + // `catch` must not quietly turn a thrown ETIMEDOUT back into "unavailable". + assert.throws( + () => + probeScriptVersion(() => { + throw timedOut; + }), + /did not exit within/, + ); // And it reaches the caller: resolvePtyWrapper must not turn it into a skip. assert.throws( () =>