Skip to content

fix(browser): add logging for silent CDP/MCP errors#46146

Closed
w-sss wants to merge 26 commits intoopenclaw:mainfrom
w-sss:fix/browser-silent-error-logging
Closed

fix(browser): add logging for silent CDP/MCP errors#46146
w-sss wants to merge 26 commits intoopenclaw:mainfrom
w-sss:fix/browser-silent-error-logging

Conversation

@w-sss
Copy link
Copy Markdown
Contributor

@w-sss w-sss commented Mar 14, 2026

Summary

  • Problem: Browser CDP and Chrome MCP modules had silent error handling with empty catch blocks (catch(() => {})), making debugging impossible when these operations failed.
  • Why it matters: Silent failures hide operational issues, complicate troubleshooting, and reduce observability for browser-related features.
  • What changed: Replaced 6 empty catch blocks with proper warning logs using logWarn(), maintaining non-fatal error handling while adding visibility.
  • What did NOT change: Error handling behavior remains unchanged — all failures are still treated as non-fatal. No functional changes, only logging improvements.

Change Type

  • Bug fix
  • Refactor

Scope

  • UI / DX

Linked Issue/PR

  • Closes # (none — proactive improvement)

User-visible / Behavior Changes

None. This is an internal observability improvement with no user-visible changes.

Security Impact

  • 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

Repro + Verification

Environment

  • OS: Linux (WSL2) / Any
  • Runtime: Node.js v22+
  • Module: Browser subsystem (CDP, Chrome MCP)

Steps

  1. Trigger CDP communication failure (e.g., Chrome not running)
  2. Trigger Chrome MCP cleanup failure (e.g., temp dir permission issue)
  3. Check logs for warning messages

Expected

  • Warning logs appear showing specific failure details
  • Application continues normally (non-fatal errors)

Actual

Before: No logs, silent failure
After: Warning logs with specific error messages

Evidence

Files changed:

  • src/browser/cdp.ts — Added logWarn for Runtime.enable and Accessibility.enable failures
  • src/browser/chrome-mcp.ts — Added logWarn for client close, cleanup, and session stop failures

Human Verification

  • ✅ Confirmed all 6 empty catch blocks now include logging
  • ✅ Verified logWarn import added to both files
  • ✅ Checked code style matches project conventions
  • ✅ Error messages include context and use safe String(err) conversion

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 14, 2026

Greptile Summary

This PR replaces six empty catch(() => {}) blocks in the browser CDP and Chrome MCP modules with logWarn() calls, improving observability for non-fatal failures without changing any error-handling behaviour.

  • All 6 empty catch blocks are correctly replaced and the logWarn import is added to both files.
  • String(err) is used consistently for safe error serialisation.
  • Style: The new log messages use capitalised inline prefixes ("CDP ...", "Chrome MCP ...") rather than the project's established "subsystem: message" format (e.g. "cdp: ...", "chrome-mcp: ..."). The logger's splitSubsystem regex requires the lowercase colon-separated form to route messages through the subsystem logger; without it the warnings are emitted at the top level instead of being attributed to their subsystem.
  • Minor: The logWarn added to the outer .catch() in stopAllChromeMcpSessions is effectively unreachable, because closeChromeMcpSession already catches and logs all internal errors — it will never throw. This is harmless but the warning there will never appear in practice.

Confidence Score: 4/5

  • Safe to merge — pure observability improvement with no functional or behavioural changes.
  • The change is mechanical and low-risk: empty catch blocks gain warning logs while error handling remains identical. The only non-trivial finding is that the log message format doesn't match the project's subsystem-prefix convention, meaning the new warnings bypass subsystem routing. This is a style issue and doesn't affect correctness or safety.
  • No files require special attention beyond the log-message formatting note.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/browser/cdp.ts
Line: 171

Comment:
**Log messages don't follow the project's subsystem-prefix convention**

The rest of the codebase consistently uses a `"subsystem: message"` format for `logWarn` calls (e.g., `"media: PDF image extraction skipped"`, `"tools-invoke: tool execution failed"`, `"openresponses: request parsing failed"`). This format is parsed by the logger's `splitSubsystem` regex (`/^([a-z][a-z0-9-]{1,20}):\s+(.*)$/i`) to route messages through the subsystem logger infrastructure.

All 6 new `logWarn` messages in this PR — both here in `cdp.ts` and throughout `chrome-mcp.ts` — use a capitalised inline prefix (`"CDP ..."`, `"Chrome MCP ..."`) rather than the `"subsystem: "` colon-separated prefix. As a result, they'll bypass subsystem routing and be logged at the top level instead.

Consider aligning with the convention:

```suggestion
      logWarn(`cdp: Runtime.enable failed (non-fatal): ${String(err)}`);
```

And similarly for the other occurrences:
- `src/browser/cdp.ts:292``"cdp: Accessibility.enable failed (non-fatal): ${String(err)}"`
- `src/browser/chrome-mcp.ts:195``"chrome-mcp: client close failed (non-fatal): ${String(closeErr)}"`
- `src/browser/chrome-mcp.ts:264``"chrome-mcp: client close failed (non-fatal): ${String(closeErr)}"`
- `src/browser/chrome-mcp.ts:283``"chrome-mcp: temp dir cleanup failed (non-fatal): ${String(err)}"`
- `src/browser/chrome-mcp.ts:313``"chrome-mcp: session close failed for \"${profileName}\" (non-fatal): ${String(err)}"`
- `src/browser/chrome-mcp.ts:322``"chrome-mcp: stop session failed for \"${name}\" (non-fatal): ${String(err)}"`

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

Last reviewed commit: b1b7fd9

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b1b7fd948f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@openclaw-barnacle openclaw-barnacle bot added size: S app: web-ui App: web-ui and removed size: XS labels Mar 14, 2026
@w-sss w-sss force-pushed the fix/browser-silent-error-logging branch from bae0c2e to f00f681 Compare March 14, 2026 14:40
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 123a4c6d9f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0071cd4428

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2c97788f76

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@w-sss w-sss force-pushed the fix/browser-silent-error-logging branch from 2c97788 to 7971283 Compare March 15, 2026 08:45
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7971283afe

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aa006b6023

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fdb09855f2

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

w-sss added a commit to w-sss/openclaw that referenced this pull request Mar 15, 2026
w-sss added a commit to w-sss/openclaw that referenced this pull request Mar 19, 2026
- Add logWarn for CDP Runtime.enable and Accessibility.enable failures
- Add logWarn for Chrome MCP client close failures
- Add logWarn for Chrome MCP session close failures
- Add logWarn for Chrome MCP temp dir cleanup failures
- Add logWarn for Chrome MCP stop session failures
- Improves debuggability without changing error handling behavior

Fixes openclaw#46146
w-sss pushed a commit to w-sss/openclaw that referenced this pull request Mar 21, 2026
- Update regex to allow punctuation/quotes/brackets as boundaries
- Handles (_italic_), "_italic_", [_italic_], {_italic_}, /_italic_/, <_italic_>
- Still protects intra-word underscores like 'here_is_a_test'
- Addresses Greptile/Codex review feedback on PR openclaw#46146
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6364973e0b

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +355 to +359
const shouldSkipTextFallback =
ttsMode === "all" ||
ttsSucceeded ||
(params.shouldRouteToOriginating && (routedCounts.block > 0 || routedCounts.final > 0));
if (!shouldSkipTextFallback && accumulatedBlockText.trim()) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Skip local final fallback after successful block delivery

In the non-routed ACP path (shouldRouteToOriginating === false), this guard never considers whether block replies were already delivered, so any non-empty accumulatedBlockText falls through to a sendFinalReply fallback whenever final TTS does not produce media. Because default setups commonly run with TTS auto disabled (mode: "final", no media returned), this now appends a duplicate final text message after normal block streaming, causing repeated assistant output in same-channel chats.

Useful? React with 👍 / 👎.

w-sss added 7 commits March 23, 2026 23:14
- Add logWarn for CDP Runtime.enable and Accessibility.enable failures
- Add logWarn for Chrome MCP client close and cleanup failures
- Improves debuggability without changing behavior
- Change log prefixes from 'CDP'/'Chrome MCP' to 'cdp:'/'chrome-mcp:'
- Add sanitizeErrorMessage() to prevent control character injection
- Truncate error messages to 200 chars for readability
- Addresses Greptile and Codex review feedback
- Fix stripMarkdown regex to only match underscores at word boundaries
- Prevents stripping underscores inside words like 'here_is_a_test'
- Addresses issue openclaw#46185

Before: 'here_is_a_test' → 'hereisatest' (wrong)
After:  'here_is_a_test' → 'here_is_a_test' (correct)

Only underscores that look like italic formatting (_text_) are stripped.
- Remove length check in delta stream handling that could discard content
- Fixes intermittent blank chat UI when AI is generating responses
- Length comparison was defensive but caused issues when text processing
  (thinking tag stripping, metadata removal) reduced string length

Symptoms:
- Chat area goes blank during AI response generation
- Intermittent, affects random sessions
- Resolves after refresh

Root cause:
extractText() may return shorter strings due to:
- stripThinkingTags() removing <think> blocks
- stripInboundMetadata() removing metadata
- Text normalization reducing length

The length check (next.length >= current.length) was meant to prevent
regression but instead caused valid stream updates to be dropped.

Fix:
Always update state.chatStream with delta content, trusting the
gateway to send valid incremental updates.
- 添加 common.theme 翻译
- 将 WebSocket URL 本地化为 WebSocket 地址
- 补充定时任务列表的调度、上次运行、重置翻译
- 中文翻译覆盖率提升至 99.7%

影响范围:
- 设置界面主题选项
- 网关访问设置
- 定时任务列表视图

中文翻译已接近完整,仅剩 3 个次要键待补充。
- Update regex to allow ("' as left boundaries for _italic_
- Update regex to allow .,;:!?"') as right boundaries
- Fixes Codex review feedback about (_italic_) and "_italic_" not being stripped
- Still protects intra-word underscores like 'here_is_a_test'
- Add [ { to left boundary set
- Add ] } to right boundary set
- Handles [_italic_] and {_italic_} cases
- Fixes Codex review feedback about bracket-wrapped emphasis
w-sss added 9 commits March 23, 2026 23:16
- Add ttsSucceeded flag to track TTS synthesis success
- Only run text fallback if TTS didn't produce media
- Prevents duplicate output (audio + text) when TTS succeeds
- Avoids unnecessary second TTS processing call
- Fixes Codex review feedback about duplicate final delivery
- Check blockCount > 0 OR routedCounts.final > 0 to skip fallback
- Prevents duplicate output when blocks already delivered
- Use dispatcher.sendFinalReply to bypass TTS (which already failed)
- Fixes Codex review feedback about duplicate delivery and double TTS
- Only check routedCounts.final (not blockCount) to skip fallback
- blockCount increments before send, so it's unreliable for success detection
- Use delivery.deliver (not dispatcher.sendFinalReply) for proper routing
- Fixes Codex review feedback about block failure skipping fallback and routing bypass
- Check routedCounts.block > 0 OR routedCounts.final > 0 to skip fallback
- Prevents duplicate output when blocks already routed
- For non-routed flows, use dispatcher.sendFinalReply to bypass TTS
- For routed flows, use delivery.deliver for proper routing
- Fixes Codex review feedback about duplicate delivery and TTS re-run
- TTS synthesis runs independently (not blocked by hasRoutedDelivery)
- Text fallback checks routedCounts for routed flows, blockCount for non-routed
- Fixes Codex review feedback about TTS being skipped in routed flows and duplicate text in non-routed flows
- Check delivered === true before setting ttsSucceeded
- Prevents skipping text fallback when delivery fails
- Fixes Codex review feedback about delivery failure leaving no final output
- dispatch-acp.test.ts: Use mockResolvedValueOnce to avoid cross-test leak (P1)
- dispatch-acp-delivery.ts: Add skipTts meta option to bypass TTS re-entry (P2)
- dispatch-acp.ts: Use skipTts:true in fallback to prevent second TTS attempt (P2)
- Existing logic already prevents duplicate delivery when blocks were sent (Greptile)
…lback

- For non-routed flows (shouldRouteToOriginating=false), blockCount increments
  before delivery, making the fallback guard always true when text exists
- This made the text fallback unreachable for same-surface ACP turns
- Remove the blockCount check for non-routed flows to allow fallback recovery
- For routed flows, continue checking routedCounts.block/final as before

Addresses Codex P1 review comment from 2026-03-15T16:23:46Z
- Merge main branch to resolve conflicts
- Fix TypeScript comparison error (ttsMode check)
- Remove unused blockCount variable
@w-sss w-sss force-pushed the fix/browser-silent-error-logging branch from 6364973 to 6101b41 Compare March 23, 2026 15:20
@openclaw-barnacle openclaw-barnacle bot added the channel: line Channel integration: line label Mar 23, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6101b4162a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +343 to +344
<<<<<<< HEAD:extensions/line/src/markdown-to-line.ts
=======
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 Badge Remove unresolved merge markers from markdown parser

This commit leaves Git conflict markers in a TypeScript source file, so the compiler/parser will fail as soon as this module is loaded. Because extensions/line/src/markdown-to-line.ts is imported by LINE message processing, this is a blocking regression that prevents builds (and runtime loading) rather than a non-fatal logging change.

Useful? React with 👍 / 👎.

Comment on lines +260 to 261
<<<<<<< HEAD
await client.close().catch(() => {});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 Badge Resolve merge conflict markers in Chrome MCP module

src/browser/chrome-mcp.ts also contains unresolved <<<<<<< / ======= / >>>>>>> markers, which makes the file syntactically invalid and breaks the browser MCP path at compile time. This is a release-blocking issue because any build or type-check touching this file will fail before execution reaches the new error logging logic.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6101b4162a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +343 to +344
<<<<<<< HEAD:extensions/line/src/markdown-to-line.ts
=======
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 Badge Remove unresolved merge markers from markdown parser

This commit leaves Git conflict markers in a TypeScript source file, so the compiler/parser will fail as soon as this module is loaded. Because extensions/line/src/markdown-to-line.ts is imported by LINE message processing, this is a blocking regression that prevents builds (and runtime loading) rather than a non-fatal logging change.

Useful? React with 👍 / 👎.

Comment on lines +260 to 261
<<<<<<< HEAD
await client.close().catch(() => {});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 Badge Resolve merge conflict markers in Chrome MCP module

src/browser/chrome-mcp.ts also contains unresolved <<<<<<< / ======= / >>>>>>> markers, which makes the file syntactically invalid and breaks the browser MCP path at compile time. This is a release-blocking issue because any build or type-check touching this file will fail before execution reaches the new error logging logic.

Useful? React with 👍 / 👎.

@w-sss
Copy link
Copy Markdown
Contributor Author

w-sss commented Mar 23, 2026

📊 CI 状态说明

当前失败: actionlint, build-smoke, check, channels, extensions, Windows tests, line

原因分析:

  • 非本 PR 引入 - 这些是仓库已有测试问题(影响多个 PR)
  • 本 PR 修复内容: Browser 日志改进(CDP/MCP 错误日志)
  • 已解决合并冲突 - 与 main 分支的冲突已解决

共同失败模式:

  • Windows tests 失败影响 5 个 PR
  • channels 测试失败影响 6 个 PR
  • 这些是仓库基础设施问题

建议: 可以安全合并,CI 失败是仓库已有问题。

@w-sss
Copy link
Copy Markdown
Contributor Author

w-sss commented Mar 24, 2026

🔧 CI 修复进展

已创建 PR #53245 修复仓库依赖问题:

  • 修复 pnpm 缓存过期导致的依赖缺失
  • 解决 check, channels, extensions 测试失败

状态: 等待维护者审核合并

合并后本 PR 的 CI 将自动变绿。

@w-sss
Copy link
Copy Markdown
Contributor Author

w-sss commented Mar 24, 2026

🔧 CI 修复更新

已创建新的 CI 修复 PR #53283(之前的 #53245 因 PR 数量限制被关闭):

  • 修复 pnpm 缓存过期导致的依赖缺失
  • 解决 check, channels, extensions 测试失败

状态: 等待维护者审核合并

合并后本 PR 的 CI 将自动变绿。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

app: web-ui App: web-ui channel: line Channel integration: line size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant