fix(browser): keep static helper seams cold by vincentkoc · Pull Request #59471 · openclaw/openclaw

vincentkoc · 2026-04-02T05:52:42Z

Summary

Problem: browser health/config helpers were still mixed into the guarded browser-runtime facade, so plain host inspection like Chrome MCP readiness depended on browser runtime activation.
Why it matters: explicit browser-disable setups and passive audit/doctor paths should stay cold and safe instead of accidentally tripping the runtime boundary.
What changed: added a cold browser-host-inspection seam, re-exported those helpers directly from browser-runtime, and moved core maintenance/config callers onto browser-config / browser-maintenance seams.
What did NOT change (scope boundary): no change to browser runtime operations, activation rules, or plugin config semantics.

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Closes #
Related #
This PR fixes a bug or regression

Root Cause / Regression History (if applicable)

Root cause: browser-runtime still bundled static host-inspection exports with activated runtime exports, and core callers were importing the mixed facade for config/cleanup paths.
Missing detection / guardrail: we had import-boundary tests for some browser helpers, but not the Chrome MCP readiness helpers or the narrowed core call sites.
Prior context (git blame, prior PR, issue, or refactor if known): this is follow-on hardening after the facade activation guard PR.
Why this regressed now: once browser runtime was activation-guarded, any helper left on the mixed facade became a latent activation dependency.
If unknown, what was ruled out: N/A.

Regression Test Plan (if applicable)

Coverage level that should have caught this:
- Unit test
- Seam / integration test
- End-to-end test
- Existing coverage already sufficient
Target test or file: src/plugin-activation-boundary.test.ts, src/commands/doctor-browser.test.ts, gateway mutation/reset tests.
Scenario the test should lock in: browser doctor/config/cleanup imports stay cold and callable without loading the bundled browser runtime.
Why this is the smallest reliable guardrail: the failure mode is import/load policy, so seam tests around the facade boundary catch it directly.
Existing test that already covers this (if any): prior boundary tests covered browser config and cleanup helpers, but not doctor/browser host inspection.
If no new test is added, why not: N/A.

User-visible / Behavior Changes

Browser doctor readiness, browser config inspection, and browser-backed cleanup paths no longer depend on browser runtime activation just to read static host/config data.

Diagram (if applicable)

Before:
[doctor/audit/reset path] -> [browser-runtime facade] -> [activation guard risk]

After:
[doctor/audit/reset path] -> [browser-config/browser-host-inspection/browser-maintenance] -> [no bundled runtime load]

Security Impact (required)

New permissions/capabilities? (No)
Secrets/tokens handling changed? (No)
New/changed network calls? (No)
Command/tool execution surface changed? (No)
Data access scope changed? (No)
If any Yes, explain risk + mitigation:

Repro + Verification

Environment

OS: macOS
Runtime/container: Node 25 / pnpm 10
Model/provider: N/A
Integration/channel (if any): browser
Relevant config (redacted): default config plus explicit browser-disabled boundary cases in tests

Steps

Import browser doctor/config/cleanup surfaces.
Exercise static browser helper paths without activating the browser plugin runtime.
Verify no bundled browser runtime load occurs.

Expected

Static helpers and cleanup paths stay cold and callable.

Actual

Verified locally after the patch.

Evidence

Failing test/log before + passing after
Trace/log snippets
Screenshot/recording
Perf numbers (if relevant)

Human Verification (required)

Verified scenarios: pnpm test -- src/commands/doctor-browser.test.ts src/plugin-activation-boundary.test.ts src/gateway/server-methods/agents-mutate.test.ts src/gateway/server.sessions.gateway-server-sessions-a.test.ts, pnpm check, pnpm build.
Edge cases checked: browser helper import under inactive runtime, browser cleanup no-op path, gateway session reset/agent delete mocks after seam narrowing.
What you did not verify: full pnpm test.

Review Conversations

I replied to or resolved every bot review conversation I addressed in this PR.
I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

Backward compatible? (Yes)
Config/env changes? (No)
Migration needed? (No)
If yes, exact upgrade steps:

Risks and Mitigations

Risk: copied browser host-inspection logic could drift from the browser plugin implementation.
- Mitigation: keep the copied seam minimal, covered by boundary/doctor tests, and limited to static Google Chrome detection/version parsing.

greptile-apps · 2026-04-02T07:56:02Z

Greptile Summary

This PR introduces a cold browser-host-inspection seam by extracting static Chrome detection helpers (resolveGoogleChromeExecutableForPlatform, readBrowserVersion, parseBrowserMajorVersion) out of the activation-guarded browser-runtime facade and into a new standalone module. Several core callers (doctor-browser, security/audit, gateway/session-reset-service, agents.ts, node-host/runner) are re-pointed to the appropriate narrow seam (browser-config, browser-maintenance, or browser-host-inspection), ensuring passive audit/doctor/cleanup paths cannot accidentally trip the browser runtime activation guard.

Key changes:

New seam src/plugin-sdk/browser-host-inspection.ts implements Chrome executable discovery and version parsing as pure static helpers (no facade dispatch, no runtime activation).
browser-runtime.ts re-exports the three helpers directly from the new seam (dropping the loadFacadeModule() wrappers for them) while all activation-dependent exports remain on the facade.
Call sites updated across agents/sandbox/browser.ts, agents/sandbox/context.ts, security/audit*.ts, gateway/session-reset-service.ts, gateway/server-methods/agents.ts, and node-host/runner.ts.
Tests in plugin-activation-boundary.test.ts now explicitly assert the three new helpers remain callable without loading the bundled browser plugin; agents-mutate.test.ts and server.sessions.gateway-server-sessions-a.test.ts mock targets are updated to browser-maintenance.js.
scripts/lib/plugin-sdk-facades.mjs registers the three helpers to the new browser-host-inspection.js facade file.

Confidence Score: 4/5

Safe to merge; changes are narrowly scoped import-boundary refactors with comprehensive boundary tests, no runtime behaviour changes.
The logic is correct and the seam split is clean. The three minor observations (regex anchoring, beta/unstable kind classification, dropped mock in the sessions test) are low-severity style/maintenance concerns that do not affect correctness or security in current usage. The author's self-noted drift risk between the copied seam and the plugin implementation is the main long-term maintenance concern, but it is explicitly acknowledged and mitigated by the new boundary tests.
src/plugin-sdk/browser-host-inspection.ts (new file — primary logic to review); src/gateway/server.sessions.gateway-server-sessions-a.test.ts (dropped movePathToTrash mock worth verifying is truly unreachable from reset paths).

Prompt To Fix All With AI

This is a comment left during a code review.
Path: src/plugin-sdk/browser-host-inspection.ts
Line: 9

Comment:
**Regex may match minor version digits before the major**

`/(\d+)(?:\.\d+){0,3}/` has no word boundary or start-of-segment anchor. For a version string like `"Chromium 3.0/1.2.3"`, the first match is `"3"` (major = 3) rather than the intended major part. In practice, Chrome version strings are `"Google Chrome 144.0.7534.0"` so this causes no bug today, but a small anchor would make the intent explicit and protect against unexpected formats:

```suggestion
const CHROME_VERSION_RE = /(\d+)(?:\.\d+){0,3}/;
```

Consider using `/\b(\d+)(?:\.\d+){0,3}/` or, if you always want the last big number before the dot-quad, a leading word boundary `\b`. This is low-risk given real Chrome output, but worth noting for future-proofing.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/plugin-sdk/browser-host-inspection.ts
Line: 64-71

Comment:
**Beta/unstable channels classified as `"chrome"` kind, not `"canary"`**

The `findFirstChromeExecutable` helper classifies a candidate as `"canary"` only if its path contains `"sxs"` or `"canary"` (case-insensitive). Linux paths `/usr/bin/google-chrome-beta` and `/usr/bin/google-chrome-unstable` contain neither token, so both are returned with `kind: "chrome"`. Depending on how callers use `BrowserExecutable.kind`, this may cause beta/unstable Chrome to be treated as stable. If `kind` is only consumed for display purposes this is harmless, but if any downstream code gates behaviour on `kind === "chrome"` assuming stability, it could silently include pre-release builds.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/gateway/server.sessions.gateway-server-sessions-a.test.ts
Line: 168-172

Comment:
**`movePathToTrash` mock silently dropped**

The previous mock of `browser-runtime.js` included `movePathToTrash: vi.fn(async () => {})`. The new mock of `browser-maintenance.js` omits it. If any code path reachable from these tests (e.g., via `session-reset-service.ts` calling agents cleanup) ever calls `movePathToTrash`, the real implementation would execute against the filesystem instead of being intercepted. Worth confirming that no agent-deletion path can be triggered from the reset scenarios covered by this suite, or explicitly re-adding the no-op mock as a safety net.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "Merge branch 'main' into fix/facade-main..." | Re-trigger Greptile}

* fix(browser): keep static helper seams cold * fix(browser): narrow sandbox helper facade imports * fix(browser): harden host inspection helpers

vincentkoc self-assigned this Apr 2, 2026

openclaw-barnacle Bot added gateway Gateway runtime scripts Repository scripts commands Command implementations agents Agent runtime and tooling size: S maintainer Maintainer-authored PR labels Apr 2, 2026

vincentkoc added 2 commits April 2, 2026 15:31

fix(browser): keep static helper seams cold

6d3edb7

fix(browser): narrow sandbox helper facade imports

c3431d9

vincentkoc force-pushed the fix/facade-maintenance-classes branch from 4a38817 to c3431d9 Compare April 2, 2026 06:34

vincentkoc marked this pull request as ready for review April 2, 2026 07:53

vincentkoc requested a review from a team as a code owner April 2, 2026 07:53

Merge branch 'main' into fix/facade-maintenance-classes

79e9756

greptile-apps Bot reviewed Apr 2, 2026

View reviewed changes

Comment thread src/plugin-sdk/browser-host-inspection.ts Outdated

Comment thread src/plugin-sdk/browser-host-inspection.ts

Comment thread src/gateway/server.sessions.gateway-server-sessions-a.test.ts

fix(browser): harden host inspection helpers

8e2af90

openclaw-barnacle Bot added size: M and removed size: S labels Apr 2, 2026

vincentkoc merged commit 08962b6 into main Apr 2, 2026
43 checks passed

vincentkoc deleted the fix/facade-maintenance-classes branch April 2, 2026 08:12

github-actions Bot mentioned this pull request Apr 2, 2026

📡 Upstream Digest — 2026-04-02 08:47 UTC curtismercier/openclaw-mods#441

Open

ngutman pushed a commit that referenced this pull request Apr 3, 2026

fix(browser): keep static helper seams cold (#59471)

2f28931

* fix(browser): keep static helper seams cold * fix(browser): narrow sandbox helper facade imports * fix(browser): harden host inspection helpers

clawsweeper Bot mentioned this pull request Jun 27, 2026

Control UI: Pinned context panel for user-defined persistent facts #46812

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(browser): keep static helper seams cold#59471

fix(browser): keep static helper seams cold#59471
vincentkoc merged 4 commits into
mainfrom
fix/facade-maintenance-classes

vincentkoc commented Apr 2, 2026

Uh oh!

greptile-apps Bot commented Apr 2, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant