[PR #3515] [MERGED] fix(browser): reject overflowing scenario timers #5795

New issue

Closed

opened 2026-10-31 01:09:29 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:09:29 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3515
Author: @sagarithm
Created: 10/14/2026
Status: ✅ Merged
Merged: 10/20/2026
Merged by: @gensokyo-dev

Base: main ← Head: fix/browser-scenario-timeout-overflow-3392

📝 Commits (2)

a1acef8 fix(browser): enforce signed 32-bit integer limits on scenario timeouts (#3392)
d1c71f4 fix(browser): qualify timer bounds across CLI and MCP

📊 Changes

4 files changed (+141 additions, -17 deletions)

View changed files

📝 docs/browser-scenario-contract.md (+5 -0)
📝 src/domain/browserScenario.test.ts (+57 -0)
📝 src/domain/browserScenarioValues.ts (+5 -0)
📝 tests/boundary/mcp/browserScenarioInputDiagnosticsMcp.test.ts (+74 -17)

📄 Description

Summary

Fixes #3392. Browser scenarios admitted 2_147_483_648 ms even though the underlying timers cannot represent it. On pinned main cf5194f68, real Chromium completed that wait in 24 ms and failed an already-visible button deadline in 42 ms, both with timer-overflow warnings.

The shared scenario contract now rejects action timeout_ms and wait duration_ms above 2,147,483,647 ms before browser launch. The maximum representable value and omitted action deadlines remain valid. One private constant owns the physical limit; CLI, MCP and the provider consume the same contract. No new cache, provider authority or execution path is introduced.

The weak bare-throw assertions are replaced by precise Zod issue code/maximum/path checks, positive boundary/omission checks, and real CLI/stdio-MCP validation in full and compact schema profiles. Compact intentionally elides nested action constraints from its advertised projection; the canonical server schema still rejects them. CLI errors retain structured paths/bounds. MCP's existing SDK error normalization retains field/limit text and recovery guidance but can flatten structured paths; this PR preserves that existing projection.

Executed validation

Linux x64, Node 24.18.0, npm 11.16.0, Playwright 1.58.2, Chrome for Testing 156.0.8078.4:

12 distinct targeted tests pass, including named SDK schemas, precise invalid-input errors, continued MCP usability and valid maximum/omitted deadlines. Full npm run check passes after the final typed test update. docs:check passes, including 5 consistency tests.
Integration with newer main 57e76599c (local merge tree e2573fb6f477c2d3bebc42a2c828235108ae9c73): the same 12 tests pass, and real Chromium MCP again completes both tasks, exports Evidence and confirms cleanup (18.757 s total, 18.484 s CPU, 1,296.7 MiB peak).
Public boundary regression fails on exact main because its advertised schema accepts the oversized wait; candidate rejects it. The CLI regression uses an unavailable executable to distinguish validation from browser startup errors.
Real public CLI, seven cases on each revision: two overflow inputs, 100-ms wait, 1,000-ms deadline, maximum deadline, omitted deadline and maximum wait followed by SIGINT. Candidate overflow cases produce invalid_request, zero browser launches and zero page requests. The maximum wait remains pending for over one second, then returns cancelled partial observations with confirmed browser/profile cleanup. This does not claim completion of a multi-day wait.
Real public MCP SDK 2026-07-28, both schema profiles: two identical completed three-action tasks per run, each opening a fresh owned browser/profile; verify returned parameters, all steps, cleanup and full Evidence export. The second call warms server imports, not an analysis-result cache. The loopback page is an authored minimal fixture, not a claim about a complex production website.

All raw captures, schemas and profiler receipts are retained locally; only compact summaries reach the agent. Original macOS/Node 26 behavior was not separately exercised. An initial compact-profile probe incorrectly assumed nested constraints were advertised; it is retained as a failed harness attempt and excluded from completed-task comparisons.

Performance and context

One CPU, 8-GiB hard / 7-GiB early family memory bounds, no swap. Complete SDK tasks include catalog discovery, two successful captures, Evidence export and close:

Profile / revision	Total elapsed	Family CPU	Peak family memory	First / repeat call
Compact / main	18.647 s	18.397 s	1,293.2 MiB	9.205 / 1.169 s
Compact / candidate	38.563 s	38.104 s	1,197.4 MiB	19.174 / 2.378 s
Full / main	43.549 s	38.636 s	1,303.3 MiB	21.052 / 2.153 s
Full / candidate	25.314 s	22.812 s	1,303.1 MiB	11.668 / 1.429 s
Compact / candidate, reverse-order repeat	19.452 s	19.262 s	1,208.4 MiB	10.099 / 1.211 s
Compact / main, reverse-order repeat	21.577 s	21.219 s	1,150.8 MiB	11.352 / 1.253 s

OS caches and shared-host load are uncontrolled. The opposite timing changes between profiles establish no general speedup or stable latency effect; slower samples remain included. CLI qualification bundles cost 35.063 / 22.892 s, CPU 25.724 / 20.363 s, peak 883.0 / 546.1 MiB (main/candidate). Those bundles include intentionally different overflow outcomes, so their totals are not equivalent healthy-workflow speed comparisons. All completed controls confirm cleanup and no OOM.

cl100k_base estimates: selected input schema 4,924 / 4,908 tokens in full mode and 344 / 344 in compact mode. A name/description/inputSchema-only catalog proxy is 194,414 / 194,398 full and 44,607 / 44,607 compact. URL-only capture text is about 1,801–1,814 tokens. Captured post-connect JSON messages, including output schemas and export, total 872,343 / 872,368 full and 722,369 / 722,348 compact. These are serialized-protocol/proxy estimates, not actual model context or usage; initialization outside capture hooks and downstream prompts are unmeasured.

CI/review assessment and authorized bypass

Latest head d1c71f41: Codex review completes with no findings. The original Greptile test-oracle thread is fixed and resolved with source/public-boundary/native verification. Its latest-head run now completes with 5/5. GitHub required fork-workflow approval, which was supplied; current classification, readiness and title jobs remain queued without a runner. No latest-head CI test result is claimed.

The task owner explicitly authorized bypass when the PR is independently qualified. The decision uses completed full checks, documentation, 12 targeted tests, 12 newer-main integration tests, real CLI regression/controls and real MCP workflows in both schema profiles. Pending CI results remain visible and will be assessed when available. Main advanced from the integrated 57e76599c to 55cce6fc1 with only an unrelated CLI test update; production workflow code is unchanged.

Post-merge CI assessment: 34 latest-head checks succeeded; shard 5 failed only jsonOutputOwnership.test.ts / preserves cancellation when staging cleanup also fails at writer.close(), with unexpected staging entries. Exact filtered controls reproduce the same failure on pre-change main cf5194f68 and candidate d1c71f41; the test and staging-cleanup producers are outside this diff. Other shards were cancelled after that failure. This is an existing main failure, not a passing full CI claim; no unrelated repair is included.

Author readiness

Human owner: @gensokyo-dev

The task owner explicitly delegated validation, readiness declarations and merge to this agent. These declarations are supplied under that authorization; no separate new manual human review by the original author is asserted.

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

Before merge

Existing scope and authority preserved; no targets, credentials, dependencies or local reports in the diff.
Owning contract, documentation, regression oracles and real public workflows verified.
Available latest-head CI/review state and actionable findings assessed; remaining queued work and bypass authorization recorded above.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/3515 **Author:** [@sagarithm](https://github.com/sagarithm) **Created:** 10/14/2026 **Status:** ✅ Merged **Merged:** 10/20/2026 **Merged by:** [@gensokyo-dev](https://github.com/gensokyo-dev) **Base:** `main` ← **Head:** `fix/browser-scenario-timeout-overflow-3392` --- ### 📝 Commits (2) - [`a1acef8`](https://github.com/morluto/rea/commit/a1acef84e32c3c45358d7af4f512063a61ec09af) fix(browser): enforce signed 32-bit integer limits on scenario timeouts (#3392) - [`d1c71f4`](https://github.com/morluto/rea/commit/d1c71f41b80ae98b89e09fa75e59ac447f265682) fix(browser): qualify timer bounds across CLI and MCP ### 📊 Changes **4 files changed** (+141 additions, -17 deletions) <details> <summary>View changed files</summary> 📝 `docs/browser-scenario-contract.md` (+5 -0) 📝 `src/domain/browserScenario.test.ts` (+57 -0) 📝 `src/domain/browserScenarioValues.ts` (+5 -0) 📝 `tests/boundary/mcp/browserScenarioInputDiagnosticsMcp.test.ts` (+74 -17) </details> ### 📄 Description ## Summary Fixes #3392. Browser scenarios admitted `2_147_483_648` ms even though the underlying timers cannot represent it. On pinned main `cf5194f68`, real Chromium completed that wait in **24 ms** and failed an already-visible button deadline in **42 ms**, both with timer-overflow warnings. The shared scenario contract now rejects action `timeout_ms` and wait `duration_ms` above **2,147,483,647 ms** before browser launch. The maximum representable value and omitted action deadlines remain valid. One private constant owns the physical limit; CLI, MCP and the provider consume the same contract. No new cache, provider authority or execution path is introduced. The weak bare-throw assertions are replaced by precise Zod issue code/maximum/path checks, positive boundary/omission checks, and real CLI/stdio-MCP validation in **full and compact** schema profiles. Compact intentionally elides nested action constraints from its advertised projection; the canonical server schema still rejects them. CLI errors retain structured paths/bounds. MCP's existing SDK error normalization retains field/limit text and recovery guidance but can flatten structured paths; this PR preserves that existing projection. ## Executed validation Linux x64, Node **24.18.0**, npm **11.16.0**, Playwright **1.58.2**, Chrome for Testing **156.0.8078.4**: - **12 distinct targeted tests pass**, including named SDK schemas, precise invalid-input errors, continued MCP usability and valid maximum/omitted deadlines. Full `npm run check` passes after the final typed test update. `docs:check` passes, including **5 consistency tests**. - Integration with newer main **57e76599c** (local merge tree `e2573fb6f477c2d3bebc42a2c828235108ae9c73`): the same **12 tests pass**, and real Chromium MCP again completes both tasks, exports Evidence and confirms cleanup (**18.757 s** total, **18.484 s** CPU, **1,296.7 MiB** peak). - Public boundary regression fails on exact main because its advertised schema accepts the oversized wait; candidate rejects it. The CLI regression uses an unavailable executable to distinguish validation from browser startup errors. - Real public CLI, **seven cases on each revision**: two overflow inputs, 100-ms wait, 1,000-ms deadline, maximum deadline, omitted deadline and maximum wait followed by SIGINT. Candidate overflow cases produce `invalid_request`, **zero browser launches and zero page requests**. The maximum wait remains pending for over one second, then returns cancelled partial observations with confirmed browser/profile cleanup. This does not claim completion of a multi-day wait. - Real public MCP SDK **2026-07-28**, both schema profiles: two identical completed three-action tasks per run, each opening a fresh owned browser/profile; verify returned parameters, all steps, cleanup and full Evidence export. The second call warms server imports, not an analysis-result cache. The loopback page is an authored minimal fixture, not a claim about a complex production website. All raw captures, schemas and profiler receipts are retained locally; only compact summaries reach the agent. Original macOS/Node 26 behavior was not separately exercised. An initial compact-profile probe incorrectly assumed nested constraints were advertised; it is retained as a failed harness attempt and excluded from completed-task comparisons. ## Performance and context One CPU, 8-GiB hard / 7-GiB early family memory bounds, no swap. Complete SDK tasks include catalog discovery, two successful captures, Evidence export and close: | Profile / revision | Total elapsed | Family CPU | Peak family memory | First / repeat call | | --- | ---: | ---: | ---: | ---: | | Compact / main | 18.647 s | 18.397 s | 1,293.2 MiB | 9.205 / 1.169 s | | Compact / candidate | 38.563 s | 38.104 s | 1,197.4 MiB | 19.174 / 2.378 s | | Full / main | 43.549 s | 38.636 s | 1,303.3 MiB | 21.052 / 2.153 s | | Full / candidate | 25.314 s | 22.812 s | 1,303.1 MiB | 11.668 / 1.429 s | | Compact / candidate, reverse-order repeat | 19.452 s | 19.262 s | 1,208.4 MiB | 10.099 / 1.211 s | | Compact / main, reverse-order repeat | 21.577 s | 21.219 s | 1,150.8 MiB | 11.352 / 1.253 s | OS caches and shared-host load are uncontrolled. The opposite timing changes between profiles establish no general speedup or stable latency effect; slower samples remain included. CLI qualification bundles cost **35.063 / 22.892 s**, CPU **25.724 / 20.363 s**, peak **883.0 / 546.1 MiB** (main/candidate). Those bundles include intentionally different overflow outcomes, so their totals are not equivalent healthy-workflow speed comparisons. All completed controls confirm cleanup and no OOM. `cl100k_base` estimates: selected input schema **4,924 / 4,908 tokens** in full mode and **344 / 344** in compact mode. A name/description/inputSchema-only catalog proxy is **194,414 / 194,398** full and **44,607 / 44,607** compact. URL-only capture text is about **1,801–1,814 tokens**. Captured post-connect JSON messages, including output schemas and export, total **872,343 / 872,368** full and **722,369 / 722,348** compact. These are serialized-protocol/proxy estimates, not actual model context or usage; initialization outside capture hooks and downstream prompts are unmeasured. ## CI/review assessment and authorized bypass Latest head `d1c71f41`: Codex review completes with **no findings**. The original Greptile test-oracle thread is fixed and resolved with source/public-boundary/native verification. Its latest-head run now completes with **5/5**. GitHub required fork-workflow approval, which was supplied; current classification, readiness and title jobs remain queued without a runner. No latest-head CI test result is claimed. The task owner explicitly authorized bypass when the PR is independently qualified. The decision uses completed full checks, documentation, 12 targeted tests, 12 newer-main integration tests, real CLI regression/controls and real MCP workflows in both schema profiles. Pending CI results remain visible and will be assessed when available. Main advanced from the integrated `57e76599c` to `55cce6fc1` with only an unrelated CLI test update; production workflow code is unchanged. Post-merge CI assessment: 34 latest-head checks succeeded; shard 5 failed only `jsonOutputOwnership.test.ts` / `preserves cancellation when staging cleanup also fails` at `writer.close()`, with unexpected staging entries. Exact filtered controls reproduce the same failure on pre-change main `cf5194f68` and candidate `d1c71f41`; the test and staging-cleanup producers are outside this diff. Other shards were cancelled after that failure. This is an existing main failure, not a passing full CI claim; no unrelated repair is included. ## Author readiness Human owner: @gensokyo-dev The task owner explicitly delegated validation, readiness declarations and merge to this agent. These declarations are supplied under that authorization; no separate new manual human review by the original author is asserted. - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] Existing scope and authority preserved; no targets, credentials, dependencies or local reports in the diff. - [x] Owning contract, documentation, regression oracles and real public workflows verified. - [x] Available latest-head CI/review state and actionable findings assessed; remaining queued work and bypass authorization recorded above. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:09:29 +00:00