feat(browser): ORACLE_BROWSER_MAX_CONCURRENT_TABS env override for the tab-concurrency cap#299
feat(browser): ORACLE_BROWSER_MAX_CONCURRENT_TABS env override for the tab-concurrency cap#299StartupBros wants to merge 2 commits into
Conversation
…e tab-concurrency cap maxConcurrentTabs is configurable via the config file and --browser-max-concurrent-tabs, but unlike its sibling browser knobs (ORACLE_BROWSER_PORT, ORACLE_BROWSER_ALLOW_COOKIE_ERRORS, ORACLE_BROWSER_PROFILE_DIR) it had no env-var fallback. Orchestrators that run oracle across hosts tune per-host env rather than editing config.json; this adds the missing override with the same precedence as the siblings: explicit config > env > default. Invalid or non-positive values are ignored (normalizeMaxConcurrentTabs still guards the final value). Covered in tests/browser/config.test.ts.
|
Codex review: needs maintainer review before merge. Reviewed July 8, 2026, 4:52 AM ET / 08:52 UTC. Summary Reproducibility: not applicable. this is a feature/config-surface PR rather than a bug report with a failing current-main reproduction path. Source inspection confirms current main lacks this env override and the PR adds it in the browser config resolver. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Merge the narrow resolver fallback if maintainers want parity with sibling browser env overrides while preserving config > env > default precedence. Do we have a high-confidence way to reproduce the issue? Not applicable: this is a feature/config-surface PR rather than a bug report with a failing current-main reproduction path. Source inspection confirms current main lacks this env override and the PR adds it in the browser config resolver. Is this the best way to solve the issue? Yes: the resolver fallback is the narrow maintainable place to add this capability because AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 9d71158f6fbb. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8a9c4caab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (originalMaxTabs === undefined) { | ||
| delete process.env.ORACLE_BROWSER_MAX_CONCURRENT_TABS; | ||
| } else { | ||
| process.env.ORACLE_BROWSER_MAX_CONCURRENT_TABS = originalMaxTabs; |
There was a problem hiding this comment.
Clear max-tab env before default assertions
When a developer or CI job already has ORACLE_BROWSER_MAX_CONCURRENT_TABS set (for example 5 to tune local browser runs), this restore block puts that value back after each test but no test setup clears it before cases that assert built-in defaults. As a result, returns defaults when config missing sees the env override and gets maxConcurrentTabs === 5 instead of 3, making this test file environment-dependent; clear the env in a beforeEach or within tests that expect defaults, while still restoring the original value after the suite.
Useful? React with 👍 / 👎.
…vironment Per review (ClawSweeper P3 + Codex P2): a developer/CI export of the new env var leaked into tests asserting built-in defaults. beforeEach now clears it; afterEach still restores the caller's original value. Verified: suite passes with and without a hostile pre-set value.
|
Both review asks addressed in
Thanks for the fast automated review! |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
Gap
maxConcurrentTabsis already configurable via the config file (browser.maxConcurrentTabs) and the--browser-max-concurrent-tabsflag — but unlike its sibling browser knobs it has no environment override. InresolveBrowserConfig,ORACLE_BROWSER_PORT/ORACLE_BROWSER_DEBUG_PORT,ORACLE_BROWSER_ALLOW_COOKIE_ERRORS, andORACLE_BROWSER_PROFILE_DIRall consultprocess.env; the tab cap falls straight through to the default.Change
ORACLE_BROWSER_MAX_CONCURRENT_TABSenv fallback with the same precedence as the siblings: explicit config > env > default.parseMaxConcurrentTabs, mirroringparseDebugPort;normalizeMaxConcurrentTabsstill guards the final value).tests/browser/config.test.tsfollowing the existingORACLE_BROWSER_PROFILE_DIRsave/restore pattern (config-beats-env, env-beats-default,0/garbage ignored).docs/configuration.md, same style as themanualLoginProfileDirenv hint.Why
Orchestrators that drive oracle across hosts (we run it as a PR-review gate, per-host concurrency tuned by an adaptive governor) set per-host env rather than editing
~/.oracle/config.json— this makes the tab cap tunable the same way the other browser knobs already are.vitest run tests/browser/config.test.ts: 6/6 pass;tsc --noEmitclean.(Follow-up in the same area as #285, which you kindly merged — thanks!)
Real behavior proof (added per review)
Patched build, real signed-in remote-Chrome session (
--remote-chrome 127.0.0.1:9222), manual-login profile, user config'sbrowser.maxConcurrentTabskey removed so nothing shadows the env var. Same invocation, only the env var differs:Baseline — no env var (built-in default):
With the override:
(Full logs available; both proof runs later hit a ChatGPT-side "prompt did not appear" send flake unrelated to config resolution — the tab-lease acquisition above happens from the resolved config exactly as in clean runs. Happy to re-capture a full clean transcript on request. Precedence also confirmed live: with
browser.maxConcurrentTabs: 4present in the user config, the same env-var run resolves(4 max)— config beats env, as designed.)Review follow-ups addressed
beforeEachnow clearsORACLE_BROWSER_MAX_CONCURRENT_TABSso a developer/CI export can't leak into the default assertions;afterEachstill restores the caller's original value. Suite verified green with and without a hostile pre-set value (ORACLE_BROWSER_MAX_CONCURRENT_TABS=7 vitest run …→ 6/6).