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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-13 01:59:44 -04:00
commit e0cd94b03e
2 changed files with 15 additions and 3 deletions

View file

@ -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(

View file

@ -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(
() =>