feat: add personal browser targets#14
Conversation
Adding .gitkeep for PR creation (default mode). This file will be removed when the task is complete. Issue: ProverCoderAI#13
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds named browser endpoint support ( ChangesPersonal browser targets and runtime switching
Sequence Diagram(s)sequenceDiagram
participant Agent as MCP Agent
participant Server as McpRuntime
participant BrowserTarget as browser_target helpers
participant CDP as CDP Browser
Agent->>Server: browser_select(name="personal", cdp_endpoint="http://…:9444")
Server->>BrowserTarget: normalize_cdp_endpoint("http://…:9444")
BrowserTarget-->>Server: normalized endpoint
Server->>BrowserTarget: upsert_browser_endpoint(endpoints, personal)
Server->>Server: active_browser = "personal"
Server-->>Agent: OK
Agent->>Server: browser_list()
Server->>Server: browser_inventory()
Server-->>Agent: {active: "personal", browsers: [{name: "personal", active: true, …}]}
Agent->>Server: browser_navigate(url="…")
Server->>Server: cdp_endpoint() → lookup "personal" in browser_endpoints
Server->>CDP: navigate via CDP
CDP-->>Server: result
Server-->>Agent: navigation result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
konard
changed the title
Working session summaryImplemented and pushed PR 14: #14 Added named MCP browser targets so agents can attach to a personal CDP browser and switch at runtime with Verified locally:
PR is marked ready. Latest GitHub Actions run for commit This summary was automatically extracted from the AI working session output. |
🤖 Solution Draft LogThis log file contains the complete execution trace of the AI solution draft process. 💰 Cost estimation:
📊 Context and tokens usage:
Total: (292.6K + 6.2M cached) input tokens, 36.2K output tokens, $10.715979 cost 🤖 Models used:
📎 Log file uploaded as Repository (46375KB)Now working session is ended, feel free to review and add any feedback on the solution draft. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/mcp.rs`:
- Around line 134-147: McpRuntime::new currently accepts any
config.active_browser value and can mark an unavailable browser as active,
creating an inconsistent startup state. Add validation in McpRuntime::new (or
the browser-selection helper it uses) to verify the requested target exists in
the available browser set, including rejecting explicit when
has_explicit_cdp_endpoint(&config) is false and any unknown custom name. If the
selection is invalid, fail initialization immediately instead of building
McpRuntime with active_browser set to a missing entry.
- Around line 317-325: The explicit CDP endpoint lookup is swallowing
normalization failures and turning a malformed value into None, which can
incorrectly fall back to the managed browser path. Update
explicit_cdp_endpoint() to propagate the normalize_cdp_endpoint() error instead
of calling ok(), and adjust has_explicit_cdp_endpoint() and any callers in the
McpServerConfig flow to handle/report the invalid --cdp-endpoint or env value
rather than treating it as absent. If you prefer central validation, enforce it
when constructing McpServerConfig so invalid endpoints are rejected once.
🪄 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: aec2bd6d-c049-4615-8e30-639a7a9317d4
📒 Files selected for processing (8)
.gitkeepREADME.mdchangelog.d/20260624_071400_personal_browser_targets.mdsrc/bin/browser-connection.rssrc/browser_target.rssrc/lib.rssrc/mcp.rstests/mcp_stdio.rs
| fn new(config: McpServerConfig) -> Self { | ||
| let active_browser = config.active_browser.clone().unwrap_or_else(|| { | ||
| if has_explicit_cdp_endpoint(&config) { | ||
| EXPLICIT_BROWSER_NAME.to_string() | ||
| } else { | ||
| MANAGED_BROWSER_NAME.to_string() | ||
| } | ||
| }); | ||
|
|
||
| Self { | ||
| config, | ||
| cdp_endpoint: None, | ||
| managed_cdp_endpoint: None, | ||
| active_browser, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Reject invalid startup browser selections.
If config.active_browser names a target that is not actually available (for example explicit without cdp_endpoint, or a typoed custom name), McpRuntime::new still marks it active. That leaves browser_list reporting an active browser that is missing from browsers, and the first browser tool fails only later. Please validate the chosen target during runtime construction and fail initialization instead of creating this inconsistent state.
🤖 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/mcp.rs` around lines 134 - 147, McpRuntime::new currently accepts any
config.active_browser value and can mark an unavailable browser as active,
creating an inconsistent startup state. Add validation in McpRuntime::new (or
the browser-selection helper it uses) to verify the requested target exists in
the available browser set, including rejecting explicit when
has_explicit_cdp_endpoint(&config) is false and any unknown custom name. If the
selection is invalid, fail initialization immediately instead of building
McpRuntime with active_browser set to a missing entry.
| fn explicit_cdp_endpoint(config: &McpServerConfig) -> Option<String> { | ||
| config | ||
| .cdp_endpoint | ||
| .as_deref() | ||
| .and_then(|endpoint| normalize_cdp_endpoint(endpoint).ok()) | ||
| } | ||
|
|
||
| fn has_explicit_cdp_endpoint(config: &McpServerConfig) -> bool { | ||
| explicit_cdp_endpoint(config).is_some() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not silently ignore malformed explicit CDP endpoints.
explicit_cdp_endpoint() turns normalization failures into None, so a bad --cdp-endpoint / env value quietly falls back to the managed browser path instead of rejecting the configuration. That can send commands to the wrong browser. Propagate the normalization error here (or validate once when building McpServerConfig) rather than swallowing it.
🤖 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/mcp.rs` around lines 317 - 325, The explicit CDP endpoint lookup is
swallowing normalization failures and turning a malformed value into None, which
can incorrectly fall back to the managed browser path. Update
explicit_cdp_endpoint() to propagate the normalize_cdp_endpoint() error instead
of calling ok(), and adjust has_explicit_cdp_endpoint() and any callers in the
McpServerConfig flow to handle/report the invalid --cdp-endpoint or env value
rather than treating it as absent. If you prefer central validation, enforce it
when constructing McpServerConfig so invalid endpoints are rejected once.
✅ Ready to mergeThis pull request is now ready to be merged:
Monitored by hive-mind with --auto-restart-until-mergeable flag |
This reverts commit 1566bac.
1 participant
Summary
Reproduction
Before this change, browser-connection could only use the Docker-managed browser or one startup --cdp-endpoint. There was no MCP tool to register a personal desktop browser or switch back and forth without restarting the MCP server.
Verification
Fixes #13