feat: add browser-connection MCP server#4
Merged
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR introduces a complete Rust-based MCP stdio server called ChangesBrowser Connection MCP Implementation
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
README.md (1)
42-42: ⚡ Quick winUpdate comment to reflect the new product path.
Line 42 mentions "MCP Playwright / Hermes" in the context of CDP, which may confuse readers since the README explicitly states that
npx@playwright/mcp`` is not the supported path. Consider rewording to just "CDP (for MCP / Hermes)" or "CDP (for browser-connection MCP server)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 42, Update the header text "CDP (for MCP Playwright / Hermes): http://127.0.0.1:9223" in README.md to remove "Playwright" and clarify the product path — e.g., change it to "CDP (for MCP / Hermes): http://127.0.0.1:9223" or "CDP (for browser-connection MCP server): http://127.0.0.1:9223" so it no longer implies the unsupported `npx `@playwright/mcp`` path; edit the exact header string in the README accordingly.src/cdp.rs (1)
205-235: ⚡ Quick winHardcoded Host header may cause issues with non-localhost endpoints.
Line 217 hardcodes
Host: 127.0.0.1:9222regardless of the actual endpoint. This could cause problems when:
- The CDP endpoint is behind a reverse proxy that routes based on Host header
- The endpoint uses a different port (e.g., 9223 as mentioned in the URL rewriting logic)
Consider deriving the Host header from the actual endpoint URL, or removing it if not strictly necessary.
🔧 Suggested fix
fn curl_json(&self, method: &str, path: &str) -> Result<Value> { let url = format!("{}{}", self.endpoint.trim_end_matches('/'), path); + let host_header = self + .endpoint + .trim() + .strip_prefix("http://") + .or_else(|| self.endpoint.trim().strip_prefix("https://")) + .unwrap_or(&self.endpoint) + .trim_end_matches('/'); + let host_header = format!("Host: {}", host_header); let output = Command::new("curl") .args([ "-sSf", "--connect-timeout", "2", "--max-time", "10", "-X", method, "-H", - "Host: 127.0.0.1:9222", + &host_header, &url, ])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cdp.rs` around lines 205 - 235, The hardcoded "Host: 127.0.0.1:9222" header in curl_json causes incorrect Host routing for non-local endpoints; update curl_json to derive the Host header from self.endpoint (parse self.endpoint into host[:port] and use that in the "-H" arg) or drop the "-H" header entirely if unnecessary, replacing the literal "Host: 127.0.0.1:9222" passed to Command::new("curl") so the args built for the curl invocation use the correct host for the URL variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cdp.rs`:
- Around line 279-314: The send_cdp_command function can hang because
socket.read() may block forever waiting for a matching id; add a
deadline/timeout to break out if no response arrives: after connect(), set a
read timeout on the underlying stream (use socket.get_ref() to access the
TcpStream and call set_read_timeout(Some(timeout))) and also implement an
Instant-based deadline inside the loop (e.g., let deadline = Instant::now() +
timeout; each iteration check if Instant::now() > deadline and return an
Err(anyhow!("timeout waiting for CDP {method} response"))). Keep existing
ping/pong handling and error contexts; use a small sleep or continue to drain
non-id messages until the deadline elapses. Ensure you reference
send_cdp_command, socket.read(), and the id==1 check when making the change.
---
Nitpick comments:
In `@README.md`:
- Line 42: Update the header text "CDP (for MCP Playwright / Hermes):
http://127.0.0.1:9223" in README.md to remove "Playwright" and clarify the
product path — e.g., change it to "CDP (for MCP / Hermes):
http://127.0.0.1:9223" or "CDP (for browser-connection MCP server):
http://127.0.0.1:9223" so it no longer implies the unsupported `npx
`@playwright/mcp`` path; edit the exact header string in the README accordingly.
In `@src/cdp.rs`:
- Around line 205-235: The hardcoded "Host: 127.0.0.1:9222" header in curl_json
causes incorrect Host routing for non-local endpoints; update curl_json to
derive the Host header from self.endpoint (parse self.endpoint into host[:port]
and use that in the "-H" arg) or drop the "-H" header entirely if unnecessary,
replacing the literal "Host: 127.0.0.1:9222" passed to Command::new("curl") so
the args built for the curl invocation use the correct host for the URL
variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a13dcd2f-1ea1-48af-8b07-c86e9d810c51
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlREADME.mdchangelog.d/20260524_141210_browser_connection_mcp.mdsrc/bin/browser-connection.rssrc/cdp.rssrc/lib.rssrc/mcp.rstests/mcp_stdio.rs
Comment on lines
+279
to
+314
| fn send_cdp_command(websocket_url: &str, method: &str, params: Value) -> Result<Value> { | ||
| let (mut socket, _) = connect(websocket_url) | ||
| .with_context(|| format!("failed to connect to CDP websocket {websocket_url}"))?; | ||
| let request = json!({ "id": 1, "method": method, "params": params }); | ||
| socket | ||
| .send(Message::Text(request.to_string())) | ||
| .with_context(|| format!("failed to send CDP command {method}"))?; | ||
|
|
||
| loop { | ||
| let message = socket | ||
| .read() | ||
| .with_context(|| format!("failed to read CDP response for {method}"))?; | ||
| match message { | ||
| Message::Text(text) => { | ||
| let value: Value = serde_json::from_str(text.as_ref()) | ||
| .with_context(|| format!("CDP websocket response for {method} was not JSON"))?; | ||
| if value.get("id").and_then(Value::as_u64) == Some(1) { | ||
| if let Some(error) = value.get("error") { | ||
| return Err(anyhow!("CDP command {method} failed: {error}")); | ||
| } | ||
| return Ok(value.get("result").cloned().unwrap_or(Value::Null)); | ||
| } | ||
| } | ||
| Message::Close(_) => { | ||
| return Err(anyhow!("CDP websocket closed before {method} response")) | ||
| } | ||
| Message::Ping(payload) => { | ||
| socket | ||
| .send(Message::Pong(payload)) | ||
| .context("failed to answer CDP websocket ping")?; | ||
| } | ||
| _ => {} | ||
| } | ||
| std::thread::sleep(Duration::from_millis(1)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing timeout on WebSocket read loop risks indefinite hang.
The loop at lines 287-313 waits for a CDP response with id: 1, but:
- Chrome DevTools Protocol sends many event messages (e.g.,
Network.*,Page.*) without anidfield - If Chrome becomes unresponsive without closing the socket, this loop runs forever
- The
tungstenite::connectandsocket.read()have no configured timeouts
Consider adding a deadline/timeout mechanism to prevent indefinite hangs when Chrome misbehaves.
🛡️ Suggested approach using Instant deadline
+use std::time::Instant;
+
+const CDP_COMMAND_TIMEOUT: Duration = Duration::from_secs(30);
+
fn send_cdp_command(websocket_url: &str, method: &str, params: Value) -> Result<Value> {
let (mut socket, _) = connect(websocket_url)
.with_context(|| format!("failed to connect to CDP websocket {websocket_url}"))?;
let request = json!({ "id": 1, "method": method, "params": params });
socket
.send(Message::Text(request.to_string()))
.with_context(|| format!("failed to send CDP command {method}"))?;
+ let deadline = Instant::now() + CDP_COMMAND_TIMEOUT;
loop {
+ if Instant::now() > deadline {
+ return Err(anyhow!("CDP command {method} timed out after {CDP_COMMAND_TIMEOUT:?}"));
+ }
let message = socket
.read()
.with_context(|| format!("failed to read CDP response for {method}"))?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cdp.rs` around lines 279 - 314, The send_cdp_command function can hang
because socket.read() may block forever waiting for a matching id; add a
deadline/timeout to break out if no response arrives: after connect(), set a
read timeout on the underlying stream (use socket.get_ref() to access the
TcpStream and call set_read_timeout(Some(timeout))) and also implement an
Instant-based deadline inside the loop (e.g., let deadline = Instant::now() +
timeout; each iteration check if Instant::now() > deadline and return an
Err(anyhow!("timeout waiting for CDP {method} response"))). Keep existing
ping/pong handling and error contexts; use a small sleep or continue to drain
non-id messages until the deadline elapses. Ensure you reference
send_cdp_command, socket.read(), and the id==1 check when making the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
browser-connectionRust MCP stdio binary for Codex/Hermes configscommand = "browser-connection"as the client-facing product path and add changelog fragmentVerification
rust-script scripts/check-changelog-fragment.rscargo fmt --checkcargo check --locked --binscargo test --lockedcargo clippy --locked --all-targets -- -D warnings./target/debug/browser-connection --project dg-my-project --no-start-browserNotes
This intentionally replaces external upstream Playwright MCP runtime config with the first-party Rust
browser-connectioncommand while keeping the existingdocker-git-browser-connectionlifecycle binary.