Read the computer's port and timeouts through the fallback the browser limits use - #114
Read the computer's port and timeouts through the fallback the browser limits use#114kevin9327 wants to merge 1 commit into
Conversation
…r limits use CopilotKit#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 CopilotKit#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 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The fix is right and low-risk — a declared-but-empty compose var arrives as
The move: put |
What this changes
agent-computerreadsPORT,NAVIGATION_TIMEOUT_MSandACTION_TIMEOUT_MSwithNumber.parseInt(process.env.X ?? "default", 10). That is the exact shape #96 removed for thebrowser limits: an unset variable declared in a compose file arrives as an empty string, so
??sees
""rather thanundefined, never fires, andNumber.parseInt("")isNaN.ACTION_TIMEOUT_MSis a documented variable (.env.example:173), so a deployment that sets it theway the compose file already sets
COMPUTER_MAX_BROWSERS/COMPUTER_BROWSER_IDLE_MS(${VAR:-})hands Playwright a
NaNtimeout instead of the 10s the default promises — a computer that looksbroken rather than misconfigured, which is precisely the failure #96 wrote up.
The fix reuses #96's own helper:
numberFromEnv(empty, absent, non-numeric and non-positive alltake the fallback) is exported from
profiles.tsand the three values are read through it, so thewhole package treats a bad or empty timeout the same way it already treats a bad or empty browser
cap.
Where it runs
not shared state. The change is that an empty or malformed value now yields the default on
every replica instead of
NaN.changes; the port number is parsed more safely but the listener is the same one.
Boundary and audit
computer process's own timeouts and port.
Proof
agent-computeris not reached by the roottypecheck(that gap is filed as #112), so verified inthe package directly:
bunx tsc --noEmit— clean.bun test tests/number-from-env.test.ts— 5/5. New test covers a positive value (trimmed), unset,the empty string a compose file passes for an unset variable, a non-numeric value, and
zero/negative — each of the last four taking the fallback.
Self-contained:
numberFromEnvalready existed and is unchanged in behaviour; this only exports itand moves three neighbours onto it.