diff --git a/packages/redrob/test/cli/run/run-process.test.ts b/packages/redrob/test/cli/run/run-process.test.ts index cb318b5fea..f72728da03 100644 --- a/packages/redrob/test/cli/run/run-process.test.ts +++ b/packages/redrob/test/cli/run/run-process.test.ts @@ -6,7 +6,7 @@ import { describe, expect } from "bun:test" import { Effect } from "effect" import { reply } from "../../lib/llm-server" -import { cliIt } from "../../lib/cli-process" +import { cliIt, slowPlatform } from "../../lib/cli-process" describe("redrob run (non-interactive subprocess)", () => { // Happy path: prompt completes, output reaches stdout, process exits 0. @@ -73,10 +73,12 @@ describe("redrob run (non-interactive subprocess)", () => { Effect.gen(function* () { const result = yield* redrob.run("say hi", { model: "test/nonexistent-model", - timeoutMs: 15_000, + timeoutMs: slowPlatform(15_000), }) expect(result.exitCode).not.toBe(0) - expect(result.durationMs).toBeLessThan(15_000) + /* Scaled WITH the bound: the claim is that it exits promptly rather than being killed, and a + duration assertion pinned to a base figure while the bound moves tests the wrong thing. */ + expect(result.durationMs).toBeLessThan(slowPlatform(15_000)) }), 60_000, ) @@ -96,7 +98,7 @@ describe("redrob run (non-interactive subprocess)", () => { ) yield* llm.fail("upstream provider exploded mid-stream") yield* llm.text("recovered") - const result = yield* redrob.run("trigger midstream error", { timeoutMs: 30_000 }) + const result = yield* redrob.run("trigger midstream error", { timeoutMs: slowPlatform(30_000) }) expect(result.exitCode).toBe(0) expect(result.stdout).toBe("partial response\nrecovered\n") expect(result.stderr).not.toContain("upstream provider exploded mid-stream") diff --git a/packages/redrob/test/lib/cli-process.ts b/packages/redrob/test/lib/cli-process.ts index e9d141e2d7..80537c6d0e 100644 --- a/packages/redrob/test/lib/cli-process.ts +++ b/packages/redrob/test/lib/cli-process.ts @@ -63,6 +63,16 @@ const CHILD_TIMEOUT_MS = 30_000 * SLOW_PLATFORM_FACTOR const READY_TIMEOUT_MS = 15_000 * SLOW_PLATFORM_FACTOR const TEST_TIMEOUT_MS = CHILD_TIMEOUT_MS * 2 +/** + * A test's own bound, scaled for the slow platform. + * + * For the call sites that pass an explicit `timeoutMs` rather than taking the default -- those bypass the + * allowance entirely, which is how the first version of this fix left the very test that was flaking still + * pinned to thirty seconds. Write `slowPlatform(30_000)` and the intent stays readable while the platform + * correction is applied for you. + */ +export const slowPlatform = (ms: number) => ms * SLOW_PLATFORM_FACTOR + export const testModelID = "test/test-model" // Wrap a Bun subprocess pipe (or any ReadableStream) as a Stream. @@ -572,9 +582,16 @@ export const cliIt = { (process.platform === "win32" ? test : test.concurrent)( name, () => Effect.runPromise(Effect.scoped(withCliFixture(body))), - // Bun's timeout includes spawn-gate wait, so it must stay above the child timeout or a queued - // concurrent CLI test expires while holding no permit. Derived from it rather than restated, which - // is what let the two drift apart. - opts ?? TEST_TIMEOUT_MS, + /* + A caller's own number is a FLOOR, not a ceiling. + + Bun's timeout includes spawn-gate wait, so it must stay above the child timeout or a queued test + expires while holding no permit -- and that failure reads as a slow command rather than as a + mis-tuned harness. Dozens of call sites pass `60_000`, which was comfortably above the old fixed + 30s child bound and is BELOW the Windows one, so honouring them literally would have reintroduced + exactly the drift these constants exist to prevent. Clamped here rather than edited at every call + site: the invariant then holds by construction instead of by everyone remembering it. + */ + typeof opts === "number" ? Math.max(opts, TEST_TIMEOUT_MS) : (opts ?? TEST_TIMEOUT_MS), ), }