morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #2265] [MERGED] fix(browser): give the DevToolsActivePort wait one deadline owner #5128

New issue

Closed

opened 2026-10-31 01:04:11 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:04:11 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2265
Author: @NIU-123370
Created: 10/12/2026
Status: ✅ Merged
Merged: 10/13/2026
Merged by: @morluto

Base: main ← Head: fix/browser-devtools-startup-owner

📝 Commits (3)

d2f04b7 Single owner for the Chrome DevToolsActivePort startup deadline
6b1185c Pin the DevToolsActivePort deadline guard to the startup call argument
37faf80 chore(ci): merge current main verification fixes

📊 Changes

4 files changed (+88 additions, -11 deletions)

View changed files

📝 scripts/lib/browser-runtime-fixture-lifecycle.mjs (+5 -2)
📝 scripts/verify-real-browser.mjs (+5 -3)
📝 src/browser/BrowserProcessStartup.ts (+23 -6)
📝 tests/boundary/browser/browserProcessStartup.test.ts (+55 -0)

📄 Description

Summary

One wait — "how long do we give Chrome to publish its DevToolsActivePort" — is still spelled with three different numbers, and the failure that ends it reports only how long it actually took, never the ceiling it hit.

This is a narrowed follow-up to the direction #2265 started with. The maintainer has since taken the larger step: f941968 fix(ci): allow cold Chrome startup in runtime fixtures (2026-10-13 18:17:14Z) moved CDP readiness into the fixture lifecycle and deleted the waitForBrowserDevtoolsPort blocks from both runtime e2e libraries. My first revision (force-pushed-away commit fe8ad28) was built on those two literals, so it was superseded; I dropped that shape rather than re-adding call sites that now live in the lifecycle owner.

What f941968 did not do is collapse the number itself. That is all this PR does now.

Current spellings on `main` (verified at `bb154d1`)

Site	Value
`src/browser/BrowserProcessStartup.ts:56` (helper fallback)	`15_000`
`scripts/lib/browser-runtime-fixture-lifecycle.mjs:53`	literal `60_000`
`scripts/verify-real-browser.mjs:50`	local `const REAL_BROWSER_STARTUP_TIMEOUT_MS = 60_000`

The fallback is the one nobody currently passes through — every remaining caller supplies its own value — so a future caller that omits timeoutMs silently gets a quarter of the tolerance the two verifiers chose. A per-caller literal is also the kind of thing that drifts: this lane failed today at elapsed_ms=20001 / 20002 while the ceiling was a 20-second literal in two places, and the message named only the elapsed time.

Change and scope

src/browser/BrowserProcessStartup.ts exports BROWSER_DEVTOOLS_STARTUP_TIMEOUT_MS = 60_000 as the single owner and uses it as the fallback (replacing 15_000). The value is taken from the maintainer's own f941968 choice; this PR does not argue for a different budget.
scripts/lib/browser-runtime-fixture-lifecycle.mjs and scripts/verify-real-browser.mjs import the owner; the verifier's local constant is removed. Neither lane's effective deadline changes.
BrowserStartupError carries the resolved startupTimeoutMs, and the message includes startup_timeout_ms=<n> between elapsed_ms= and exit_code=, so a startup failure states the ceiling it hit.

Contract and boundary impact

Semantic owner and earliest changed stage: src/browser/BrowserProcessStartup.ts (waitForBrowserDevtoolsPort, startupError, browserStartupMessage).
CLI and MCP/tool-catalog contract: no schema change; no tool, input, or output field; no catalog or docs regeneration.
Provider, bridge, target-format, or platform compatibility: none — the helper is used by verification fixtures, not by product provider admission.
Evidence, artifact, provenance, or reconstruction contract: BrowserStartupError is exported and its message gains one segment. Grep across src, scripts, tests, docs found no consumer destructuring the class outside this module and no assertion or doc quoting the message text; the field is additive for anything reading the typed error.
Process execution, authorization, cleanup, or containment impact: none. Owned-child handling, terminal-state classification (spawn-error / exited / signalled / timeout), and fixture cleanup ordering are unchanged; only the default wait moves (15 s → 60 s), and it is reached solely by callers that omit timeoutMs.
Generated metadata, package, or installation impact: none.

Evidence and regression coverage

tests/boundary/browser/browserProcessStartup.test.ts: 5 passed (3 pre-existing, 2 added).
- "reports bounded timeout diagnostics" now also asserts an explicit override is honoured and echoed (startupTimeoutMs: 20, startup_timeout_ms=20);
- "names the owner's deadline when a caller omits one" — omitted timeoutMs yields startupTimeoutMs === BROWSER_DEVTOOLS_STARTUP_TIMEOUT_MS;
- "keeps one owner for the DevToolsActivePort deadline" — pins the owner at 60,000 and reads both remaining call sites, requiring each file to contain exactly one waitForBrowserDevtoolsPort call whose single timeoutMs argument is BROWSER_DEVTOOLS_STARTUP_TIMEOUT_MS. The assertion inspects the argument of each startup call rather than whole-file text, so an unrelated numeric timeoutMs elsewhere in a caller stays green and a call that swaps in a different constant goes red.
The lifecycle owner's own guard added in f941968 still passes next to this change: tests/boundary/browser/runtimeFixtureCleanup.test.ts 2 passed (7 passed across both files).
Mutation checks (each arm applied, the focused file re-run, then reverted; diff -q against backups confirmed all files restored, and the restored baseline re-run is 5 passed):

Arm	Observed
owner lowered to `20_000`	1 failed — the owner-pinning guard
helper fallback restored to a literal `15_000`	1 failed — "names the owner's deadline when a caller omits one"
message loses the `startup_timeout_ms=` segment	2 failed — both message assertions
`verify-real-browser.mjs` re-hardcodes its number	1 failed — the source guard
fixture lifecycle re-hardcodes `20_000`	1 failed — the source guard

Real-lane runs with local Google Chrome on macOS arm64 (Node 24.18.0):
- verify:browser:runtime → exit 0, 15 s;
- verify:browser (the real-browser verifier, one of the two call sites changed here) → exit 0, 79 s, against a control run of unmodified main in the same worktree that passed in 78 s;
- flagged, not explained: the very first verify:browser attempt on my tree exited 1 in 17 s with "code":"cleanup_incomplete", Analysis operation timed out after 1000ms: capture_browser_scenario. I could not reproduce it (0 further failures) and nothing in this diff shortens that 1000 ms cleanup budget, so I am recording it as an unattributed cold-host observation rather than claiming it away; the hosted browser / real-browser lane is the arbiter.
Remaining proof gaps: the value 60,000 is inherited from f941968 and is not re-justified here; hosted Linux/Windows behaviour is unmeasured locally; verify-browser-source-maps.mjs no longer participates in this wait at all after f941968, so it is out of scope rather than untested.

Validation performed

npm run check:fast (turbo typecheck + lint) on the rebased head: 2/2 successful.
./node_modules/.bin/oxfmt --check on the four changed files: correct format.
npx vitest run tests/boundary/browser/browserProcessStartup.test.ts tests/boundary/browser/runtimeFixtureCleanup.test.ts: 7 passed.
npm run build then the two real-lane runs above.
Rebased onto bb154d1; no commit in a2d8b94..bb154d1 touches these four files.
Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Compatibility, safety, and release

Breaking changes or migration steps: none for callers. Anything matching on the exact BrowserStartupError message string would see the added startup_timeout_ms= segment.
Real Hopper/Ghidra, browser, or OS coverage: real Google Chrome on macOS arm64 for both verify:browser and verify:browser:runtime. No Hopper/Ghidra involvement.
Package or release metadata impact: none.
Security, privacy, process, or containment review: none needed; no new process, path, or privilege, and stderr excerpting is unchanged.

Review checklist

This PR addresses a concrete problem or an agreed enhancement.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

_{🔄 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/2265 **Author:** [@NIU-123370](https://github.com/NIU-123370) **Created:** 10/12/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/browser-devtools-startup-owner` --- ### 📝 Commits (3) - [`d2f04b7`](https://github.com/morluto/rea/commit/d2f04b775646cfdbcde932f9b43c79f452cef86b) Single owner for the Chrome DevToolsActivePort startup deadline - [`6b1185c`](https://github.com/morluto/rea/commit/6b1185c39cb0b9c312aee0a12d77b5e91b0f3000) Pin the DevToolsActivePort deadline guard to the startup call argument - [`37faf80`](https://github.com/morluto/rea/commit/37faf802281533184896cc2e9b58dc34e823016b) chore(ci): merge current main verification fixes ### 📊 Changes **4 files changed** (+88 additions, -11 deletions) <details> <summary>View changed files</summary> 📝 `scripts/lib/browser-runtime-fixture-lifecycle.mjs` (+5 -2) 📝 `scripts/verify-real-browser.mjs` (+5 -3) 📝 `src/browser/BrowserProcessStartup.ts` (+23 -6) 📝 `tests/boundary/browser/browserProcessStartup.test.ts` (+55 -0) </details> ### 📄 Description ## Summary One wait — "how long do we give Chrome to publish its `DevToolsActivePort`" — is still spelled with three different numbers, and the failure that ends it reports only how long it actually took, never the ceiling it hit. This is a narrowed follow-up to the direction #2265 started with. **The maintainer has since taken the larger step**: `f941968 fix(ci): allow cold Chrome startup in runtime fixtures` (2026-10-13 18:17:14Z) moved CDP readiness into the fixture lifecycle and deleted the `waitForBrowserDevtoolsPort` blocks from both runtime e2e libraries. My first revision (force-pushed-away commit `fe8ad28`) was built on those two literals, so it was superseded; I dropped that shape rather than re-adding call sites that now live in the lifecycle owner. What `f941968` did not do is collapse the number itself. That is all this PR does now. ## Current spellings on `main` (verified at `bb154d1`) | Site | Value | | --- | --- | | `src/browser/BrowserProcessStartup.ts:56` (helper fallback) | `15_000` | | `scripts/lib/browser-runtime-fixture-lifecycle.mjs:53` | literal `60_000` | | `scripts/verify-real-browser.mjs:50` | local `const REAL_BROWSER_STARTUP_TIMEOUT_MS = 60_000` | The fallback is the one nobody currently passes through — every remaining caller supplies its own value — so a future caller that omits `timeoutMs` silently gets a quarter of the tolerance the two verifiers chose. A per-caller literal is also the kind of thing that drifts: this lane failed today at `elapsed_ms=20001` / `20002` while the ceiling was a 20-second literal in two places, and the message named only the elapsed time. ## Change and scope - `src/browser/BrowserProcessStartup.ts` exports `BROWSER_DEVTOOLS_STARTUP_TIMEOUT_MS = 60_000` as the single owner and uses it as the fallback (replacing `15_000`). The value is taken from the maintainer's own `f941968` choice; this PR does not argue for a different budget. - `scripts/lib/browser-runtime-fixture-lifecycle.mjs` and `scripts/verify-real-browser.mjs` import the owner; the verifier's local constant is removed. Neither lane's effective deadline changes. - `BrowserStartupError` carries the resolved `startupTimeoutMs`, and the message includes `startup_timeout_ms=<n>` between `elapsed_ms=` and `exit_code=`, so a startup failure states the ceiling it hit. ## Contract and boundary impact - Semantic owner and earliest changed stage: `src/browser/BrowserProcessStartup.ts` (`waitForBrowserDevtoolsPort`, `startupError`, `browserStartupMessage`). - CLI and MCP/tool-catalog contract: no schema change; no tool, input, or output field; no catalog or docs regeneration. - Provider, bridge, target-format, or platform compatibility: none — the helper is used by verification fixtures, not by product provider admission. - Evidence, artifact, provenance, or reconstruction contract: `BrowserStartupError` is exported and its `message` gains one segment. Grep across `src`, `scripts`, `tests`, `docs` found no consumer destructuring the class outside this module and no assertion or doc quoting the message text; the field is additive for anything reading the typed error. - Process execution, authorization, cleanup, or containment impact: none. Owned-child handling, terminal-state classification (`spawn-error` / `exited` / `signalled` / `timeout`), and fixture cleanup ordering are unchanged; only the *default* wait moves (15 s → 60 s), and it is reached solely by callers that omit `timeoutMs`. - Generated metadata, package, or installation impact: none. ## Evidence and regression coverage - `tests/boundary/browser/browserProcessStartup.test.ts`: 5 passed (3 pre-existing, 2 added). - "reports bounded timeout diagnostics" now also asserts an explicit override is honoured and echoed (`startupTimeoutMs: 20`, `startup_timeout_ms=20`); - "names the owner's deadline when a caller omits one" — omitted `timeoutMs` yields `startupTimeoutMs === BROWSER_DEVTOOLS_STARTUP_TIMEOUT_MS`; - "keeps one owner for the DevToolsActivePort deadline" — pins the owner at 60,000 and reads both remaining call sites, requiring each file to contain exactly one `waitForBrowserDevtoolsPort` call whose single `timeoutMs` argument is `BROWSER_DEVTOOLS_STARTUP_TIMEOUT_MS`. The assertion inspects the argument of each startup call rather than whole-file text, so an unrelated numeric `timeoutMs` elsewhere in a caller stays green and a call that swaps in a different constant goes red. - The lifecycle owner's own guard added in `f941968` still passes next to this change: `tests/boundary/browser/runtimeFixtureCleanup.test.ts` 2 passed (7 passed across both files). - Mutation checks (each arm applied, the focused file re-run, then reverted; `diff -q` against backups confirmed all files restored, and the restored baseline re-run is 5 passed): | Arm | Observed | | --- | --- | | owner lowered to `20_000` | 1 failed — the owner-pinning guard | | helper fallback restored to a literal `15_000` | 1 failed — "names the owner's deadline when a caller omits one" | | message loses the `startup_timeout_ms=` segment | 2 failed — both message assertions | | `verify-real-browser.mjs` re-hardcodes its number | 1 failed — the source guard | | fixture lifecycle re-hardcodes `20_000` | 1 failed — the source guard | - Real-lane runs with local Google Chrome on macOS arm64 (Node 24.18.0): - `verify:browser:runtime` → exit 0, 15 s; - `verify:browser` (the real-browser verifier, one of the two call sites changed here) → **exit 0, 79 s**, against a control run of unmodified `main` in the same worktree that passed in 78 s; - **flagged, not explained**: the very first `verify:browser` attempt on my tree exited 1 in 17 s with `"code":"cleanup_incomplete"`, `Analysis operation timed out after 1000ms: capture_browser_scenario`. I could not reproduce it (0 further failures) and nothing in this diff shortens that 1000 ms cleanup budget, so I am recording it as an unattributed cold-host observation rather than claiming it away; the hosted `browser / real-browser` lane is the arbiter. - Remaining proof gaps: the value 60,000 is inherited from `f941968` and is not re-justified here; hosted Linux/Windows behaviour is unmeasured locally; `verify-browser-source-maps.mjs` no longer participates in this wait at all after `f941968`, so it is out of scope rather than untested. ## Validation performed - `npm run check:fast` (turbo typecheck + lint) on the rebased head: 2/2 successful. - `./node_modules/.bin/oxfmt --check` on the four changed files: correct format. - `npx vitest run tests/boundary/browser/browserProcessStartup.test.ts tests/boundary/browser/runtimeFixtureCleanup.test.ts`: 7 passed. - `npm run build` then the two real-lane runs above. - Rebased onto `bb154d1`; no commit in `a2d8b94..bb154d1` touches these four files. - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Compatibility, safety, and release - Breaking changes or migration steps: none for callers. Anything matching on the exact `BrowserStartupError` message string would see the added `startup_timeout_ms=` segment. - Real Hopper/Ghidra, browser, or OS coverage: real Google Chrome on macOS arm64 for both `verify:browser` and `verify:browser:runtime`. No Hopper/Ghidra involvement. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none needed; no new process, path, or privilege, and stderr excerpting is unchanged. ## Review checklist - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:04:11 +00:00

closed this issue
added the
pull-request
label

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#5128

No description provided.

Rows
Columns