From c6af28d403a3b2e78b3feb3703e3b5c990762643 Mon Sep 17 00:00:00 2001 From: kevin9327 Date: Sat, 22 Aug 2026 06:40:07 +0900 Subject: [PATCH 1/2] Read the computer's port and timeouts through the fallback the browser limits use #96 found that an unset variable declared in a compose file arrives as an empty string, so `Number.parseInt(process.env.X ?? "default")` never falls back: `??` sees "" rather than undefined, and the parse is NaN. It moved the browser limits onto `numberFromEnv`, which treats empty, absent, non-numeric and non-positive alike as "not set". The three values alongside them in `agent-computer/src/index.ts` kept the raw parse: `PORT`, `NAVIGATION_TIMEOUT_MS` and `ACTION_TIMEOUT_MS`. `ACTION_TIMEOUT_MS` is a documented variable (`.env.example:173`), so a deployment that sets it the way the compose file already sets the browser limits gets a NaN timeout, Playwright waiting on NaN instead of the 10s the default promises, a computer that looks broken rather than misconfigured. Export `numberFromEnv` and read the three through it. Adds a test for the helper, including the empty-string case that is the whole point. This is in agent-computer, which the root typecheck does not reach (that gap is #112). Verified in the package: `bunx tsc --noEmit` clean, and `bun test tests/number-from-env.test.ts` is 5/5. Co-Authored-By: Claude Opus 4.8 --- agent-computer/src/index.ts | 14 +++----- agent-computer/src/profiles.ts | 9 ++++- agent-computer/tests/number-from-env.test.ts | 38 ++++++++++++++++++++ 3 files changed, 50 insertions(+), 11 deletions(-) create mode 100644 agent-computer/tests/number-from-env.test.ts diff --git a/agent-computer/src/index.ts b/agent-computer/src/index.ts index 98a6c5f7..9441f7e0 100644 --- a/agent-computer/src/index.ts +++ b/agent-computer/src/index.ts @@ -12,7 +12,7 @@ import { TAKE_CONTROL_FIRST, } from "./control"; import { identity } from "./identity"; -import { createProfiles, VIEWPORT } from "./profiles"; +import { createProfiles, numberFromEnv, VIEWPORT } from "./profiles"; import { type InputMessage, type Screencast, @@ -70,11 +70,8 @@ if (!COMPUTER_TOKEN) { process.exit(1); } -const PORT = Number.parseInt(process.env.PORT ?? "4100", 10); -const NAVIGATION_TIMEOUT_MS = Number.parseInt( - process.env.NAVIGATION_TIMEOUT_MS ?? "30000", - 10, -); +const PORT = numberFromEnv("PORT", 4100); +const NAVIGATION_TIMEOUT_MS = numberFromEnv("NAVIGATION_TIMEOUT_MS", 30000); /** * How long one action waits for its element. @@ -83,10 +80,7 @@ const NAVIGATION_TIMEOUT_MS = Number.parseInt( * behaviour we want, but a ref that no longer resolves would otherwise hang for the full navigation * timeout before saying so, and the person is sitting watching a screen that is not changing. */ -const ACTION_TIMEOUT_MS = Number.parseInt( - process.env.ACTION_TIMEOUT_MS ?? "10000", - 10, -); +const ACTION_TIMEOUT_MS = numberFromEnv("ACTION_TIMEOUT_MS", 10000); /** * How much page text a navigation hands back. diff --git a/agent-computer/src/profiles.ts b/agent-computer/src/profiles.ts index b0b2e714..0d69a921 100644 --- a/agent-computer/src/profiles.ts +++ b/agent-computer/src/profiles.ts @@ -152,7 +152,14 @@ async function closeAndWait(context: BrowserContext): Promise { * and every browser would be closed the moment it opened. Anything that is not a positive number * falls back, because "I typed this wrong" and "I did not set it" both mean the default. */ -function numberFromEnv(name: string, fallback: number): number { +/** + * A positive number from the environment, or the fallback. + * + * `Number.parseInt(process.env.X ?? "default")` is not enough: an unset variable declared in a + * compose file arrives as an empty string rather than as absent, so `??` never fires and the parse + * yields `NaN`. Empty, absent, non-numeric and non-positive all mean "not set" and take the fallback. + */ +export function numberFromEnv(name: string, fallback: number): number { const raw = process.env[name]?.trim(); if (!raw) return fallback; const value = Number(raw); diff --git a/agent-computer/tests/number-from-env.test.ts b/agent-computer/tests/number-from-env.test.ts new file mode 100644 index 00000000..43f0d381 --- /dev/null +++ b/agent-computer/tests/number-from-env.test.ts @@ -0,0 +1,38 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { numberFromEnv } from "../src/profiles"; + +const NAME = "OPENBOT_TEST_NUMBER_FROM_ENV"; + +afterEach(() => { + delete process.env[NAME]; +}); + +describe("numberFromEnv", () => { + test("takes a positive number, trimming surrounding whitespace", () => { + process.env[NAME] = " 5000 "; + expect(numberFromEnv(NAME, 10000)).toBe(5000); + }); + + test("falls back when the variable is unset", () => { + expect(numberFromEnv(NAME, 10000)).toBe(10000); + }); + + test("falls back on the empty string a compose file passes for an unset variable", () => { + // The bug this guards: `Number.parseInt(process.env.X ?? "default")` sees "" here, not undefined, + // so `??` never fires and the parse is NaN. An empty value means "not set" and takes the fallback. + process.env[NAME] = ""; + expect(numberFromEnv(NAME, 10000)).toBe(10000); + }); + + test("falls back on a non-numeric value", () => { + process.env[NAME] = "soon"; + expect(numberFromEnv(NAME, 10000)).toBe(10000); + }); + + test("falls back on zero and negatives, so a bad timeout is never enforced", () => { + process.env[NAME] = "0"; + expect(numberFromEnv(NAME, 10000)).toBe(10000); + process.env[NAME] = "-5"; + expect(numberFromEnv(NAME, 10000)).toBe(10000); + }); +}); From 05bdcb610932dfdfe240571e75ba8b2ae6d928b1 Mon Sep 17 00:00:00 2001 From: kevin9327 Date: Sat, 22 Aug 2026 23:26:46 +0900 Subject: [PATCH 2/2] Move numberFromEnv to a playwright-free module so the test runs in CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test imported `numberFromEnv` from `profiles.ts`, which hard-imports `playwright` at module load. CI installs only the root workspace, not `agent-computer`, so that import throws `Cannot find package 'playwright'`, drops the file's tests, and fails the `tests` and `verify` jobs. `numberFromEnv` moves to its own `agent-computer/src/env.ts`, which imports nothing; `profiles.ts` re-exports it so `index.ts` and the browser-limit constants are untouched, and the test imports it from `./env`. Verified with `agent-computer/node_modules` moved aside — a runner without playwright — where `bun test` now passes 5/5 instead of failing to import. Co-Authored-By: Claude Opus 4.8 --- agent-computer/src/env.ts | 16 ++++++++++++ agent-computer/src/profiles.ts | 27 ++++---------------- agent-computer/tests/number-from-env.test.ts | 2 +- 3 files changed, 22 insertions(+), 23 deletions(-) create mode 100644 agent-computer/src/env.ts diff --git a/agent-computer/src/env.ts b/agent-computer/src/env.ts new file mode 100644 index 00000000..2cd93262 --- /dev/null +++ b/agent-computer/src/env.ts @@ -0,0 +1,16 @@ +/** + * A positive number from the environment, or the fallback. + * + * `Number.parseInt(process.env.X ?? "default")` is not enough: an unset variable declared in a + * compose file arrives as an empty string rather than as absent, so `??` never fires and the parse + * yields `NaN`. Empty, absent, non-numeric and non-positive all mean "not set" and take the fallback. + * + * Its own module, free of the `playwright` import `profiles.ts` carries, so a test can reach it + * without loading a browser driver that is not installed where the tests run. + */ +export function numberFromEnv(name: string, fallback: number): number { + const raw = process.env[name]?.trim(); + if (!raw) return fallback; + const value = Number(raw); + return Number.isFinite(value) && value > 0 ? value : fallback; +} diff --git a/agent-computer/src/profiles.ts b/agent-computer/src/profiles.ts index 0d69a921..420931ee 100644 --- a/agent-computer/src/profiles.ts +++ b/agent-computer/src/profiles.ts @@ -39,6 +39,11 @@ import { type BrowserContext, chromium, type Page } from "playwright"; import { profileDirectoryFor } from "./bot-id"; import { chooseEvictions, chooseIdle } from "./browser-eviction"; import { egressFor, egressLabel } from "./egress"; +import { numberFromEnv } from "./env"; + +// Re-exported so callers that already import it from here do not change, while the test imports it +// from the playwright-free `./env` instead of pulling this module's browser driver in with it. +export { numberFromEnv }; /** The viewport, which is what a person's click coordinates are relative to. */ export const VIEWPORT = { width: 1280, height: 800 }; @@ -144,28 +149,6 @@ async function closeAndWait(context: BrowserContext): Promise { await new Promise((resolve) => setTimeout(resolve, CLOSE_SETTLE_MS)); } -/** - * A number an operator set, or the default. - * - * `Number("")` is zero, and an unset variable in a compose file arrives as an empty string rather - * than as absent. Read with `??` alone, an operator who had not set the cap would get a cap of zero - * and every browser would be closed the moment it opened. Anything that is not a positive number - * falls back, because "I typed this wrong" and "I did not set it" both mean the default. - */ -/** - * A positive number from the environment, or the fallback. - * - * `Number.parseInt(process.env.X ?? "default")` is not enough: an unset variable declared in a - * compose file arrives as an empty string rather than as absent, so `??` never fires and the parse - * yields `NaN`. Empty, absent, non-numeric and non-positive all mean "not set" and take the fallback. - */ -export function numberFromEnv(name: string, fallback: number): number { - const raw = process.env[name]?.trim(); - if (!raw) return fallback; - const value = Number(raw); - return Number.isFinite(value) && value > 0 ? value : fallback; -} - /** * How many browsers one computer holds at once. * diff --git a/agent-computer/tests/number-from-env.test.ts b/agent-computer/tests/number-from-env.test.ts index 43f0d381..6233aa0f 100644 --- a/agent-computer/tests/number-from-env.test.ts +++ b/agent-computer/tests/number-from-env.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, test } from "bun:test"; -import { numberFromEnv } from "../src/profiles"; +import { numberFromEnv } from "../src/env"; const NAME = "OPENBOT_TEST_NUMBER_FROM_ENV";