fix(browser): improve browser control disabled/error hints by zhangjunmengyang · Pull Request #34864 · openclaw/openclaw

zhangjunmengyang · 2026-03-04T18:42:24Z

Summary

Problem: When the browser tool can’t talk to the browser control service, the current messaging can over-suggest restarting the gateway.
Why it matters: During intermittent browser-control failures this can contribute to disruptive restart loops; additionally, when browser control is disabled in config the error is currently misleading (presented as a reachability failure).
What changed: Improve error classification + operator guidance for browser fetch failures; add unit tests to lock behavior.
What did NOT change (scope boundary): No changes to browser control service implementation/enablement logic; no changes to routing/dispatcher behavior beyond messaging.

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Closes #
Related #

User-visible / Behavior Changes

When browser control is disabled, errors now clearly state it’s a config/enablement issue (not “can’t reach service”).
For local browser-tool failures, operator hint is less disruptive: check enablement/running first, restart gateway only if needed.

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: N/A

Repro + Verification

Environment

OS: macOS (local dev)
Runtime/container: N/A
Model/provider: N/A
Integration/channel (if any): N/A
Relevant config (redacted): browser control disabled scenario

Steps

Call fetchBrowserJson("/") while browser control is disabled.
Call fetchBrowserJson("http://127.0.0.1:18888/", { timeoutMs: 5 }) with an aborting timeout.

Expected

Disabled browser control surfaces a clear “browser control is disabled” config message.
Timeout path includes the model-facing “Do NOT retry the browser tool” guidance.

Actual

Matches expected (covered by unit tests below).

Evidence

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

Local unit test:

bunx vitest run --config vitest.unit.config.ts src/browser/client-fetch.error-hints.test.ts

Human Verification (required)

Verified scenarios:
- Browser control disabled produces config-specific error (not reachability wrapper)
- Absolute URL timeout produces non-retry guidance
Edge cases checked:
- Local vs absolute URL error-hint paths
What I did not verify:
- Full end-to-end browser tool execution against a live browser control service

Compatibility / Migration

Backward compatible? (Yes)
Config/env changes? (No)
Migration needed? (No)

Failure Recovery (if this breaks)

How to disable/revert this change quickly:
- Revert the PR / commit(s) affecting src/browser/client-fetch.ts
Known bad symptoms reviewers should watch for:
- Browser tool failures no longer include useful operator guidance
- Error messages regress to misleading “can’t reach service” for disabled config

Risks and Mitigations

Risk: Error-message wording changes could break brittle string-matching in downstream tooling.
- Mitigation: This is user-facing guidance; behavior is covered with unit tests and can be adjusted if a stable contract is required.

CI note

Current CI failure on this PR appears unrelated to code changes: oven-sh/setup-bun@v2 attempts to download bun-v1.3.9+cf6cdbbba and receives HTTP 404.

greptile-apps · 2026-03-04T18:44:25Z

Greptile Summary

This PR improves the browser control error-hint UX in two ways: it avoids the misleading "Can't reach the service" wrapper when browser control is simply disabled in config, and it softens the operator hint to check enablement/running status before recommending a full gateway restart (reducing the risk of restart loops on intermittent failures). A small unit test is added to lock both behaviors.

src/browser/client-fetch.ts: New early-return in enhanceBrowserFetchError intercepts "browser control disabled" errors and surfaces a dedicated config-issue message. The operatorHint for local URLs is also updated to lead with a less disruptive check before the restart suggestion.
src/browser/client-fetch.error-hints.test.ts: New test file covering the disabled-config path and absolute-URL timeout hints. The abort-mock helper correctly simulates a never-resolving fetch that rejects on AbortSignal firing.
Maintainability: The detection at line 118 relies on string-matching against the literal thrown at line 185; a shared constant or dedicated error subclass would make this coupling explicit and prevent silent regressions if either string is changed independently.
Test coverage gap: The "disabled" test verifies the new message is present but does not assert the absence of the old "Can't reach" wrapper, so a regression reintroducing that wrapper would still pass.

Confidence Score: 5/5

This PR is safe to merge; changes are confined to error-message text and early-return logic with no behavioral impact on the happy path.
Both changed files are limited in scope: one rewrites an error-formatting helper and the other adds unit tests. The happy-path code path (fetchBrowserJson when the service is reachable) is completely unaffected. The only risks are minor maintainability concerns (fragile string matching) and a small test coverage gap — neither of which would cause a runtime regression.
No files require special attention; the two style concerns flagged are non-blocking.

_{Last reviewed commit: c9424f3}

greptile-apps · 2026-03-04T18:44:29Z

src/browser/client-fetch.ts

+  if (msgLower.includes("browser control disabled")) {
+    return new Error(
+      `OpenClaw browser control is disabled. Enable it in config before using the browser tool. ${modelHint}`,
+    );
+  }


Fragile string-based detection couples two locations

The detection here (msgLower.includes("browser control disabled")) is tightly coupled to the literal string thrown at line 185 (new Error("browser control disabled")). If either string drifts independently (e.g. one is updated to "browser control is disabled" to match the user-facing message), the special-case branch will silently stop firing and fall through to the generic "Can't reach" wrapper — precisely the misleading path this PR is trying to avoid.

Using a shared constant or a dedicated error subclass (similar to BrowserServiceError) would make the coupling explicit and type-safe:

// top of file const BROWSER_CONTROL_DISABLED_MSG = "browser control disabled" as const; // line 185 throw new Error(BROWSER_CONTROL_DISABLED_MSG); // line 118 if (msgLower.includes(BROWSER_CONTROL_DISABLED_MSG)) { ... }

Or, even more robust — a subclass like BrowserControlDisabledError extends Error that can be caught by instanceof (just as BrowserServiceError is handled at line 261).

Prompt To Fix With AI

This is a comment left during a code review. Path: src/browser/client-fetch.ts Line: 118-122 Comment: **Fragile string-based detection couples two locations** The detection here (`msgLower.includes("browser control disabled")`) is tightly coupled to the literal string thrown at line 185 (`new Error("browser control disabled")`). If either string drifts independently (e.g. one is updated to `"browser control is disabled"` to match the user-facing message), the special-case branch will silently stop firing and fall through to the generic "Can't reach" wrapper — precisely the misleading path this PR is trying to avoid. Using a shared constant or a dedicated error subclass (similar to `BrowserServiceError`) would make the coupling explicit and type-safe: ```typescript // top of file const BROWSER_CONTROL_DISABLED_MSG = "browser control disabled" as const; // line 185 throw new Error(BROWSER_CONTROL_DISABLED_MSG); // line 118 if (msgLower.includes(BROWSER_CONTROL_DISABLED_MSG)) { ... } ``` Or, even more robust — a subclass like `BrowserControlDisabledError extends Error` that can be caught by `instanceof` (just as `BrowserServiceError` is handled at line 261). How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-03-04T18:44:30Z

src/browser/client-fetch.error-hints.test.ts

+  it("surfaces 'browser control disabled' as a config issue (not reachability)", async () => {
+    await expect(fetchBrowserJson("/")).rejects.toThrow(/browser control is disabled/i);
+  });


Test doesn't assert absence of the old "Can't reach" wrapper

The PR description explicitly calls out eliminating the misleading "Can't reach the ... service" wrapper for the disabled case, but the test only asserts the presence of the new message — it never verifies the old wrapper is gone. A future regression that re-introduces the wrapper (e.g. the msgLower.includes check stops matching) would still pass this test.

Consider adding a negative assertion:

Suggested change

it("surfaces 'browser control disabled' as a config issue (not reachability)", async () => {

await expect(fetchBrowserJson("/")).rejects.toThrow(/browser control is disabled/i);

});

await expect(fetchBrowserJson("/")).rejects.toThrow(/browser control is disabled/i);

await expect(fetchBrowserJson("/")).rejects.not.toThrow(/can't reach/i);

Prompt To Fix With AI

This is a comment left during a code review. Path: src/browser/client-fetch.error-hints.test.ts Line: 42-44 Comment: **Test doesn't assert absence of the old "Can't reach" wrapper** The PR description explicitly calls out eliminating the misleading "Can't reach the ... service" wrapper for the disabled case, but the test only asserts the presence of the new message — it never verifies the old wrapper is gone. A future regression that re-introduces the wrapper (e.g. the `msgLower.includes` check stops matching) would still pass this test. Consider adding a negative assertion: ```suggestion await expect(fetchBrowserJson("/")).rejects.toThrow(/browser control is disabled/i); await expect(fetchBrowserJson("/")).rejects.not.toThrow(/can't reach/i); ``` How can I resolve this? If you propose a fix, please make it concise.

wirelessjoe · 2026-03-08T23:49:01Z

This is a great UX improvement. I've been bitten by the misleading "can't reach service" error when browser control was simply disabled.

One thought: for Browserbase users, it might also be helpful to hint at rate limit scenarios (e.g., "max concurrent sessions reached") separately from "service unreachable." We hit 429s from Browserbase that surface as generic browser failures.

Not blocking this PR, just a potential follow-up. Happy to open a separate issue if useful.

openclaw-barnacle bot added the size: S label Mar 4, 2026

greptile-apps bot reviewed Mar 4, 2026

View reviewed changes

fix(browser): clarify browser control disabled error; test error hints

bcd7584

zhangjunmengyang force-pushed the fix/browser-client-fetch-hints branch from c9424f3 to bcd7584 Compare March 5, 2026 01:43

wirelessjoe mentioned this pull request Mar 9, 2026

[Feature]: Surface Browserbase rate limit errors (429) with actionable hints #40390

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(browser): improve browser control disabled/error hints#34864

fix(browser): improve browser control disabled/error hints#34864
zhangjunmengyang wants to merge 1 commit intoopenclaw:mainfrom
zhangjunmengyang:fix/browser-client-fetch-hints

zhangjunmengyang commented Mar 4, 2026 •

edited

Loading

Uh oh!

greptile-apps bot commented Mar 4, 2026

Uh oh!

greptile-apps bot Mar 4, 2026

Uh oh!

greptile-apps bot Mar 4, 2026

Uh oh!

wirelessjoe commented Mar 8, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

zhangjunmengyang commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

User-visible / Behavior Changes

Security Impact (required)

Repro + Verification

Environment

Steps

Expected

Actual

Evidence

Human Verification (required)

Compatibility / Migration

Failure Recovery (if this breaks)

Risks and Mitigations

CI note

Uh oh!

greptile-apps bot commented Mar 4, 2026

Greptile Summary

Confidence Score: 5/5

Uh oh!

greptile-apps bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

wirelessjoe commented Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zhangjunmengyang commented Mar 4, 2026 •

edited

Loading

wirelessjoe commented Mar 8, 2026 •

edited

Loading