fix(browser): add logging for silent CDP/MCP errors#46146
fix(browser): add logging for silent CDP/MCP errors#46146w-sss wants to merge 26 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR replaces six empty
Confidence Score: 4/5
Prompt To Fix All With AIThis 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 |
There was a problem hiding this comment.
💡 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".
bae0c2e to
f00f681
Compare
There was a problem hiding this comment.
💡 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".
There was a problem hiding this comment.
💡 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".
There was a problem hiding this comment.
💡 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".
2c97788 to
7971283
Compare
There was a problem hiding this comment.
💡 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".
There was a problem hiding this comment.
💡 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".
There was a problem hiding this comment.
💡 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".
…ore italic stripping
- 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
- 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
There was a problem hiding this comment.
💡 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".
| const shouldSkipTextFallback = | ||
| ttsMode === "all" || | ||
| ttsSucceeded || | ||
| (params.shouldRouteToOriginating && (routedCounts.block > 0 || routedCounts.final > 0)); | ||
| if (!shouldSkipTextFallback && accumulatedBlockText.trim()) { |
There was a problem hiding this comment.
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 👍 / 👎.
- 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
- 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
6364973 to
6101b41
Compare
There was a problem hiding this comment.
💡 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".
| <<<<<<< HEAD:extensions/line/src/markdown-to-line.ts | ||
| ======= |
There was a problem hiding this comment.
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 👍 / 👎.
| <<<<<<< HEAD | ||
| await client.close().catch(() => {}); |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
💡 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".
| <<<<<<< HEAD:extensions/line/src/markdown-to-line.ts | ||
| ======= |
There was a problem hiding this comment.
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 👍 / 👎.
| <<<<<<< HEAD | ||
| await client.close().catch(() => {}); |
There was a problem hiding this comment.
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 👍 / 👎.
📊 CI 状态说明当前失败: actionlint, build-smoke, check, channels, extensions, Windows tests, line 原因分析:
共同失败模式:
建议: 可以安全合并,CI 失败是仓库已有问题。 |
🔧 CI 修复进展已创建 PR #53245 修复仓库依赖问题:
状态: 等待维护者审核合并 合并后本 PR 的 CI 将自动变绿。 |
Summary
catch(() => {})), making debugging impossible when these operations failed.logWarn(), maintaining non-fatal error handling while adding visibility.Change Type
Scope
Linked Issue/PR
User-visible / Behavior Changes
None. This is an internal observability improvement with no user-visible changes.
Security Impact
Repro + Verification
Environment
Steps
Expected
Actual
Before: No logs, silent failure
After: Warning logs with specific error messages
Evidence
Files changed:
src/browser/cdp.ts— AddedlogWarnforRuntime.enableandAccessibility.enablefailuressrc/browser/chrome-mcp.ts— AddedlogWarnfor client close, cleanup, and session stop failuresHuman Verification
logWarnimport added to both filesString(err)conversionReview Conversations