pr 14#15
Conversation
skulidropek
commented
Jun 24, 2026
skulidropek
commented
- Initial commit with task details
- feat: add personal browser targets
- Revert "Initial commit with task details"
- Add browser share relay and Edge extension artifact
Agent Plan UpdateBranch: 1. PlanSource: codex - Captured: 2026-06-24T09:46:40.255Z Personal Browser via MCP + noVNCSummary
Key Changes
Host Setup
google-chrome \
--remote-debugging-address=0.0.0.0 \
--remote-debugging-port=9222 \
--user-data-dir="$HOME/.browser-connection-personal" \
--no-first-run
x11vnc -display :0 -rfbport 5900 -listen 0.0.0.0 -nopw -forever -shared
[mcp_servers.playwright]
command = "browser-connection"
args = [
"--project", "dg-rust-browser-connection-pr-14",
"--network", "container:dg-rust-browser-connection-pr-14",
"--browser", "personal=http://host.docker.internal:9222",
"--browser-vnc", "personal=host.docker.internal:5900",
"--active-browser", "managed",
]Test Plan
Assumptions
2. PlanSource: codex - Captured: 2026-06-24T09:48:16.361Z Codex PlanSteps
3. PlanSource: codex - Captured: 2026-06-24T10:13:46.893Z Codex PlanSteps
4. PlanSource: codex - Captured: 2026-06-24T10:21:03.251Z Codex PlanSteps
5. PlanSource: codex - Captured: 2026-06-24T10:38:17.658Z Link-Based Remote Edge SharingSummary
Key Changes
Test Plan
Assumptions
|
AI Session BackupCommit: 25fac54
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds named browser targets with runtime switching, a ChangesNamed browser targets, shared relay, and control panel
Sequence Diagram(s)sequenceDiagram
participant MCPClient as MCP Client
participant MCPServer as browser-connection
participant ControlPanel as Local Control Panel
participant RelayServer as browser-connection-relay
participant EdgeExtension as Edge Share Extension
participant SharedClient as SharedBrowserClient
MCPClient->>MCPServer: tools/call browser_select
MCPServer->>MCPServer: register active browser
ControlPanel->>MCPServer: POST /api/select
MCPServer->>MCPServer: update active browser
EdgeExtension->>RelayServer: connect browser session
SharedClient->>RelayServer: connect agent session
MCPClient->>MCPServer: tools/call browser_navigate
MCPServer->>SharedClient: dispatch browser command
SharedClient->>RelayServer: JSON command
RelayServer->>EdgeExtension: forward command
EdgeExtension-->>RelayServer: JSON response
RelayServer-->>MCPServer: forward response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)src/bin/target_resolution/mod.rs┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.19][ERROR]: unable to find a config; path src/bin/rbc.rs┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.18][ERROR]: unable to find a config; path extension/edge-share/service_worker.js┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.28][ERROR]: unable to find a config; path
🔧 markdownlint-cli2 (0.22.1)README.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) changelog.d/20260625_000000_rbc_cli.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) extension/edge-share/README.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 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 `@extension/edge-share/README.md`:
- Around line 100-102: Clarify the manifest permissions guidance in the Edge
share README: the relay connection is not controlled by <all_urls>, so update
the wording around the extension’s permissions to distinguish page-level access
from relay WebSocket access. In the section describing production restrictions
for the edge-share extension, reference the relay connectivity behavior
explicitly and instruct readers to narrow
content_security_policy.extension_pages.connect-src to the known relay origins
rather than implying that <all_urls> governs relay access.
In `@extension/edge-share/service_worker.js`:
- Around line 437-466: The screenshot fallback in the `screenshot` handler
returns a different payload shape than the CDP path, so make the `catch` branch
match the success branch used in `sendCdp`/`Page.captureScreenshot`. In
`service_worker.js`, update the fallback that uses
`chrome.tabs.captureVisibleTab` to normalize the returned Data URL into the same
base64 field used by the main path (and keep the existing `mimeType`, `tab`,
`fallback`, and `warning` fields consistent), so callers always receive the
documented screenshot result shape regardless of which capture path succeeds.
- Around line 820-836: The normalizeRelayUrl helper currently accepts any http
URL, which should be restricted to loopback-only development use. Update
normalizeRelayUrl so it still allows https everywhere, but rejects non-loopback
http: relay URLs unless the hostname is localhost, 127.0.0.1, or ::1; keep the
existing validation and cleanup behavior in this function while enforcing the
scheme/host check before returning the normalized href.
- Around line 482-510: getTargetTabId() only updates the in-memory
state.activeTabId, so the selected tab is lost when the MV3 service worker
suspends. Update the selection paths in getTargetTabId() to persist the chosen
tab ID whenever it changes (for example, alongside the state.activeTabId
assignment), and restore it before using the fallback active-tab query so the
“last selected tab” behavior survives worker restarts.
- Around line 558-565: The attach error handling in chrome.debugger.attach is
incorrectly caching tabs even when attach failed, which causes sendCdp() to skip
retrying later. Update the try/catch around chrome.debugger.attach so
attachedTabs.add(tabId) only happens after a successful attach, and in the
“Another debugger is already attached” branch do not add the tab to
attachedTabs; instead leave it uncached so the next sendCdp() call can attempt
attach again.
In `@src/bin/browser-connection-relay.rs`:
- Around line 13-15: The `bind` argument in `browser-connection-relay` currently
shows a public plaintext address as a normal example, which can imply unsafe
defaults; update the `bind` field documentation and/or validation so that
non-loopback binds like public addresses are clearly marked as requiring TLS or
authenticated proxying, or require an explicit opt-in for insecure public binds.
Use the `bind` CLI argument and its surrounding `#[arg(...)]` metadata in
`browser-connection-relay` to add the warning or guard without presenting
`0.0.0.0:8765` as a safe default example.
In `@src/browser_target.rs`:
- Around line 345-361: The upsert_browser_share_url helper currently adds a
share_url onto an existing NamedBrowserEndpoint even when that endpoint already
has a cdp_endpoint, which allows a browser target to end up with both transport
modes at once. Update upsert_browser_share_url in src/browser_target.rs so
share-based and CDP-based routing are mutually exclusive: either prevent
attaching share_url to an existing CDP endpoint, or explicitly clear the
conflicting transport fields when setting share_url. Make sure the behavior
aligns with the dispatch logic in src/mcp.rs where share_url takes precedence.
- Around line 266-280: The normalize_share_url function currently only trims and
checks the scheme, which lets invalid relay share links slip through until
SharedBrowserClient is used. Update normalize_share_url to fully parse and
validate the share URL against the relay contract so malformed CLI/env/config
values fail early during startup or selection. Use the existing
normalize_share_url helper in src/browser_target.rs to enforce the expected
share-link structure, not just http/https/ws/wss, and return an error with the
parsed validation result when the link does not match.
- Around line 239-250: Update the endpoint normalization logic in the browser
target parsing path so IPv6 literals keep their brackets: the current host
cleanup in the endpoint handling code strips brackets from any host, which turns
inputs like [::1]:5900 into an ambiguous result. Adjust the parsing/formatting
in the endpoint normalization flow so host/port extraction still validates the
port, but the final normalized value preserves brackets when the host is an IPv6
address, while leaving regular hostnames and IPv4 unchanged.
In `@src/browser.rs`:
- Around line 128-136: The proxy reuse logic in BrowserTargetDisplayRuntime
currently only checks the VNC endpoint label, so it can incorrectly reuse a
running container even when the effective network mode has changed. Update the
reuse condition in novnc_proxy_matches and the related
BrowserTargetDisplayRuntime path so it also compares the target’s network
configuration before returning the existing container, ensuring a recreated
container is used whenever the network mode differs.
In `@src/mcp.rs`:
- Around line 242-252: Define and enforce a clear precedence for browser targets
that have both cdp_endpoint and share_url, since active_share_url() currently
wins implicitly and can override CDP routing. Update handle_tool_call() and
browser_endpoint_kind() so the dispatch path and reported kind match the chosen
rule, either by rejecting mixed targets during config handling or by making one
field explicitly take priority and reflecting that consistently across these
helpers.
In `@src/mcp/control_panel.rs`:
- Around line 148-158: The localhost control API is too permissive because
route_request currently allows GET for the state-changing /api/share and
/api/select endpoints, and the control panel exposes wildcard CORS. Restrict
these handlers to POST only in route_request, and update the response
headers/CORS behavior used by the embedded panel so it does not advertise
Access-Control-Allow-Origin: *; keep same-origin access for the
control_panel_html flow while preventing cross-site requests from mutating
McpRuntime via register_share_response and select_browser_response.
In `@src/shared_browser.rs`:
- Around line 193-236: The shared browser client in `SharedBrowser::call` is
swallowing relay registration failures by treating a closed socket as a generic
“relay closed” error. Update the relay handshake/registration path so failures
from `RelayEndpoint::parse` and `state.register` are surfaced back to the caller
as a structured error frame, or fail the connection before entering the read
loop. Make sure the `call` method reports the actual registration error instead
of only returning the close message, and apply the same fix to the other
affected relay path referenced in the review.
- Around line 451-453: The relay path in shared_browser’s connection handling is
busy-waiting: the short read timeout plus the timeout-retry loop in the
read/forward logic keeps idle browser/agent sessions spinning. Update the stream
handling around the setup and the retry loop in shared_browser so it blocks or
uses an evented wait instead of immediately retrying on read timeout, and make
sure the forwarding flow no longer treats idle timeouts as a fast poll. Focus on
the connection setup and the retry logic in the same relay functions that
configure the stream and process reads.
🪄 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: 94d71daa-27db-4ae0-abaa-7b1d4e78656e
⛔ Files ignored due to path filters (1)
artifacts/edge-share-extension.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (22)
Cargo.tomlREADME.mdartifacts/README.mdchangelog.d/20260624_071400_personal_browser_targets.mdextension/edge-share/README.mdextension/edge-share/manifest.jsonextension/edge-share/popup.htmlextension/edge-share/popup.jsextension/edge-share/service_worker.jssrc/bin/browser-connection-relay.rssrc/bin/browser-connection.rssrc/browser.rssrc/browser/assets.rssrc/browser/tests.rssrc/browser_target.rssrc/lib.rssrc/mcp.rssrc/mcp/control_panel.rssrc/mcp/tests.rssrc/shared_browser.rstests/mcp_stdio.rstests/shared_browser_relay.rs
| - The extension uses broad host permissions so it can connect to arbitrary relay | ||
| hosts during development. A production build should narrow this to known relay | ||
| origins. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
# Inspect the README section and related extension manifest/settings
git ls-files extension/edge-share
printf '\n--- README excerpt ---\n'
sed -n '80,130p' extension/edge-share/README.md
printf '\n--- manifest files containing connect-src / host_permissions / all_urls ---\n'
rg -n '"host_permissions"|"connect-src"|<all_urls>|websocket|relay' extension/edge-share -SRepository: ProverCoderAI/rust-browser-connection
Length of output: 6001
🏁 Script executed:
# Read the relevant manifest and any docs that define extension network permissions
fd -a 'manifest.json|manifest.*.json|README.md' extension/edge-shareRepository: ProverCoderAI/rust-browser-connection
Length of output: 279
🏁 Script executed:
# Map the extension files if the previous searches are inconclusive
ast-grep outline extension/edge-share/README.md --view expandedRepository: ProverCoderAI/rust-browser-connection
Length of output: 220
🏁 Script executed:
sed -n '1,120p' extension/edge-share/manifest.jsonRepository: ProverCoderAI/rust-browser-connection
Length of output: 810
🏁 Script executed:
rg -n '"content_security_policy"|connect-src|host_permissions|permissions' extension/edge-share/manifest.json extension/edge-share/service_worker.js extension/edge-share/README.md -n -SRepository: ProverCoderAI/rust-browser-connection
Length of output: 633
🏁 Script executed:
rg -n 'chrome\.scripting|executeScript|fetch\(|XMLHttpRequest|WebSocket|tabs\.|debugger|host_permissions' extension/edge-share/service_worker.js extension/edge-share/manifest.json -SRepository: ProverCoderAI/rust-browser-connection
Length of output: 2668
🏁 Script executed:
sed -n '1,120p' extension/edge-share/manifest.json
printf '\n--- service_worker relay / page-access calls ---\n'
sed -n '1,260p' extension/edge-share/service_worker.jsRepository: ProverCoderAI/rust-browser-connection
Length of output: 6856
🏁 Script executed:
sed -n '1,120p' extension/edge-share/manifest.json
printf '\n--- service_worker relay / page-access calls ---\n'
sed -n '1,260p' extension/edge-share/service_worker.jsRepository: ProverCoderAI/rust-browser-connection
Length of output: 6856
Clarify <all_urls> vs relay access. <all_urls> is for page-level access (debugger, scripting, tabs), not the relay WebSocket. If the goal is to limit relay connectivity, narrow content_security_policy.extension_pages.connect-src to the known relay origins instead.
🤖 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 `@extension/edge-share/README.md` around lines 100 - 102, Clarify the manifest
permissions guidance in the Edge share README: the relay connection is not
controlled by <all_urls>, so update the wording around the extension’s
permissions to distinguish page-level access from relay WebSocket access. In the
section describing production restrictions for the edge-share extension,
reference the relay connectivity behavior explicitly and instruct readers to
narrow content_security_policy.extension_pages.connect-src to the known relay
origins rather than implying that <all_urls> governs relay access.
| try { | ||
| await sendCdp(tabId, "Page.enable", {}); | ||
| const result = await sendCdp(tabId, "Page.captureScreenshot", { | ||
| format, | ||
| quality, | ||
| fromSurface: true, | ||
| captureBeyondViewport: params.fullPage === true | ||
| }); | ||
|
|
||
| return { | ||
| tab: await currentTabSummary(tabId), | ||
| mimeType: `image/${format}`, | ||
| data: result.data | ||
| }; | ||
| } catch (error) { | ||
| const tab = await chromeCall(chrome.tabs.get, chrome.tabs, tabId); | ||
| await chromeCall(chrome.tabs.update, chrome.tabs, tabId, { active: true }); | ||
| const dataUrl = await chromeCall(chrome.tabs.captureVisibleTab, chrome.tabs, tab.windowId, { | ||
| format, | ||
| quality | ||
| }); | ||
|
|
||
| return { | ||
| tab: tabSummary(tab), | ||
| mimeType: `image/${format}`, | ||
| dataUrl, | ||
| fallback: "tabs.captureVisibleTab", | ||
| warning: errorMessage(error) | ||
| }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the screenshot payload shape consistent on fallback.
Line 449 returns raw base64 in data, but Line 462 switches to a full Data URL in dataUrl. That makes the protocol branch-dependent for pages where the CDP capture path is rejected, even though the README documents a base64 screenshot result.
Proposed fix
} catch (error) {
const tab = await chromeCall(chrome.tabs.get, chrome.tabs, tabId);
await chromeCall(chrome.tabs.update, chrome.tabs, tabId, { active: true });
const dataUrl = await chromeCall(chrome.tabs.captureVisibleTab, chrome.tabs, tab.windowId, {
format,
quality
});
+ const match = /^data:([^;]+);base64,(.*)$/.exec(dataUrl);
+ if (!match) {
+ throw new Error("Unexpected screenshot payload from captureVisibleTab");
+ }
return {
tab: tabSummary(tab),
- mimeType: `image/${format}`,
- dataUrl,
+ mimeType: match[1],
+ data: match[2],
fallback: "tabs.captureVisibleTab",
warning: errorMessage(error)
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| await sendCdp(tabId, "Page.enable", {}); | |
| const result = await sendCdp(tabId, "Page.captureScreenshot", { | |
| format, | |
| quality, | |
| fromSurface: true, | |
| captureBeyondViewport: params.fullPage === true | |
| }); | |
| return { | |
| tab: await currentTabSummary(tabId), | |
| mimeType: `image/${format}`, | |
| data: result.data | |
| }; | |
| } catch (error) { | |
| const tab = await chromeCall(chrome.tabs.get, chrome.tabs, tabId); | |
| await chromeCall(chrome.tabs.update, chrome.tabs, tabId, { active: true }); | |
| const dataUrl = await chromeCall(chrome.tabs.captureVisibleTab, chrome.tabs, tab.windowId, { | |
| format, | |
| quality | |
| }); | |
| return { | |
| tab: tabSummary(tab), | |
| mimeType: `image/${format}`, | |
| dataUrl, | |
| fallback: "tabs.captureVisibleTab", | |
| warning: errorMessage(error) | |
| }; | |
| } | |
| try { | |
| await sendCdp(tabId, "Page.enable", {}); | |
| const result = await sendCdp(tabId, "Page.captureScreenshot", { | |
| format, | |
| quality, | |
| fromSurface: true, | |
| captureBeyondViewport: params.fullPage === true | |
| }); | |
| return { | |
| tab: await currentTabSummary(tabId), | |
| mimeType: `image/${format}`, | |
| data: result.data | |
| }; | |
| } catch (error) { | |
| const tab = await chromeCall(chrome.tabs.get, chrome.tabs, tabId); | |
| await chromeCall(chrome.tabs.update, chrome.tabs, tabId, { active: true }); | |
| const dataUrl = await chromeCall(chrome.tabs.captureVisibleTab, chrome.tabs, tab.windowId, { | |
| format, | |
| quality | |
| }); | |
| const match = /^data:([^;]+);base64,(.*)$/.exec(dataUrl); | |
| if (!match) { | |
| throw new Error("Unexpected screenshot payload from captureVisibleTab"); | |
| } | |
| return { | |
| tab: tabSummary(tab), | |
| mimeType: match[1], | |
| data: match[2], | |
| fallback: "tabs.captureVisibleTab", | |
| warning: errorMessage(error) | |
| }; | |
| } |
🤖 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 `@extension/edge-share/service_worker.js` around lines 437 - 466, The
screenshot fallback in the `screenshot` handler returns a different payload
shape than the CDP path, so make the `catch` branch match the success branch
used in `sendCdp`/`Page.captureScreenshot`. In `service_worker.js`, update the
fallback that uses `chrome.tabs.captureVisibleTab` to normalize the returned
Data URL into the same base64 field used by the main path (and keep the existing
`mimeType`, `tab`, `fallback`, and `warning` fields consistent), so callers
always receive the documented screenshot result shape regardless of which
capture path succeeds.
| async function getTargetTabId(params) { | ||
| if (Number.isInteger(params.tabId)) { | ||
| state.activeTabId = params.tabId; | ||
| return params.tabId; | ||
| } | ||
|
|
||
| if (Number.isInteger(state.activeTabId)) { | ||
| try { | ||
| await chromeCall(chrome.tabs.get, chrome.tabs, state.activeTabId); | ||
| return state.activeTabId; | ||
| } catch (_error) { | ||
| state.activeTabId = null; | ||
| } | ||
| } | ||
|
|
||
| const activeTabs = await chromeCall(chrome.tabs.query, chrome.tabs, { | ||
| active: true, | ||
| lastFocusedWindow: true | ||
| }); | ||
|
|
||
| if (activeTabs.length > 0 && Number.isInteger(activeTabs[0].id)) { | ||
| state.activeTabId = activeTabs[0].id; | ||
| return activeTabs[0].id; | ||
| } | ||
|
|
||
| const tabs = await chromeCall(chrome.tabs.query, chrome.tabs, { active: true }); | ||
| if (tabs.length > 0 && Number.isInteger(tabs[0].id)) { | ||
| state.activeTabId = tabs[0].id; | ||
| return tabs[0].id; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Persist activeTabId when selection changes.
getTargetTabId() only mutates in-memory state. In an MV3 service worker that means the documented "last selected tab" behavior is lost after suspension, and the next command can silently target whatever tab is currently active instead.
Proposed fix
async function getTargetTabId(params) {
if (Number.isInteger(params.tabId)) {
- state.activeTabId = params.tabId;
+ await persistState({ activeTabId: params.tabId });
return params.tabId;
}
@@
if (activeTabs.length > 0 && Number.isInteger(activeTabs[0].id)) {
- state.activeTabId = activeTabs[0].id;
+ await persistState({ activeTabId: activeTabs[0].id });
return activeTabs[0].id;
}
@@
if (tabs.length > 0 && Number.isInteger(tabs[0].id)) {
- state.activeTabId = tabs[0].id;
+ await persistState({ activeTabId: tabs[0].id });
return tabs[0].id;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function getTargetTabId(params) { | |
| if (Number.isInteger(params.tabId)) { | |
| state.activeTabId = params.tabId; | |
| return params.tabId; | |
| } | |
| if (Number.isInteger(state.activeTabId)) { | |
| try { | |
| await chromeCall(chrome.tabs.get, chrome.tabs, state.activeTabId); | |
| return state.activeTabId; | |
| } catch (_error) { | |
| state.activeTabId = null; | |
| } | |
| } | |
| const activeTabs = await chromeCall(chrome.tabs.query, chrome.tabs, { | |
| active: true, | |
| lastFocusedWindow: true | |
| }); | |
| if (activeTabs.length > 0 && Number.isInteger(activeTabs[0].id)) { | |
| state.activeTabId = activeTabs[0].id; | |
| return activeTabs[0].id; | |
| } | |
| const tabs = await chromeCall(chrome.tabs.query, chrome.tabs, { active: true }); | |
| if (tabs.length > 0 && Number.isInteger(tabs[0].id)) { | |
| state.activeTabId = tabs[0].id; | |
| return tabs[0].id; | |
| async function getTargetTabId(params) { | |
| if (Number.isInteger(params.tabId)) { | |
| await persistState({ activeTabId: params.tabId }); | |
| return params.tabId; | |
| } | |
| if (Number.isInteger(state.activeTabId)) { | |
| try { | |
| await chromeCall(chrome.tabs.get, chrome.tabs, state.activeTabId); | |
| return state.activeTabId; | |
| } catch (_error) { | |
| state.activeTabId = null; | |
| } | |
| } | |
| const activeTabs = await chromeCall(chrome.tabs.query, chrome.tabs, { | |
| active: true, | |
| lastFocusedWindow: true | |
| }); | |
| if (activeTabs.length > 0 && Number.isInteger(activeTabs[0].id)) { | |
| await persistState({ activeTabId: activeTabs[0].id }); | |
| return activeTabs[0].id; | |
| } | |
| const tabs = await chromeCall(chrome.tabs.query, chrome.tabs, { active: true }); | |
| if (tabs.length > 0 && Number.isInteger(tabs[0].id)) { | |
| await persistState({ activeTabId: tabs[0].id }); | |
| return tabs[0].id; | |
| } |
🤖 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 `@extension/edge-share/service_worker.js` around lines 482 - 510,
getTargetTabId() only updates the in-memory state.activeTabId, so the selected
tab is lost when the MV3 service worker suspends. Update the selection paths in
getTargetTabId() to persist the chosen tab ID whenever it changes (for example,
alongside the state.activeTabId assignment), and restore it before using the
fallback active-tab query so the “last selected tab” behavior survives worker
restarts.
| try { | ||
| await chromeCall(chrome.debugger.attach, chrome.debugger, { tabId }, "1.3"); | ||
| attachedTabs.add(tabId); | ||
| } catch (error) { | ||
| if (String(errorMessage(error)).includes("Another debugger is already attached")) { | ||
| attachedTabs.add(tabId); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant file and surrounding logic.
git ls-files 'extension/edge-share/service_worker.js'
ast-grep outline extension/edge-share/service_worker.js --view expanded
# Show the relevant lines and nearby helpers.
sed -n '520,640p' extension/edge-share/service_worker.js
# Find all references to attachedTabs and debugger attach/send paths.
rg -n 'attachedTabs|chrome\.debugger\.attach|chrome\.debugger\.sendCommand|sendCdp\(' extension/edge-share/service_worker.jsRepository: ProverCoderAI/rust-browser-connection
Length of output: 6525
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the initialization and any debugger detach event handling.
sed -n '1,80p' extension/edge-share/service_worker.js
# Search for onDetach / detachment-related cleanup.
rg -n 'onDetach|detach|attachedTabs\.delete|attachedTabs\.clear|debugger' extension/edge-share/service_worker.jsRepository: ProverCoderAI/rust-browser-connection
Length of output: 2839
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect initialization and debugger detach handling.
sed -n '1,80p' extension/edge-share/service_worker.js
# Find all cleanup paths that may clear attachedTabs.
rg -n 'onDetach|attachedTabs\.delete|attachedTabs\.clear|chrome\.debugger|detachAllDebuggers' extension/edge-share/service_worker.jsRepository: ProverCoderAI/rust-browser-connection
Length of output: 2687
🌐 Web query:
chrome.debugger.onDetach event semantics another debugger already attached attach failure attachedTabs cache chrome extension debugger
💡 Result:
The chrome.debugger.onDetach event is triggered when the Chrome browser terminates the debugging session for a specific target (such as a tab) [1]. This termination typically occurs due to one of two main scenarios: the target (tab) is being closed, or the user has manually invoked the Chrome DevTools for the already-attached tab [1]. Regarding attachment conflicts: 1. Exclusivity: Chrome supports only one active debugger attachment per target at any given time [2]. If your extension attempts to call chrome.debugger.attach on a target that is already being debugged by another process or another instance of the developer tools, the operation will fail [2]. 2. Error Handling: When an attachment attempt fails—including when another debugger is already attached—the promise returned by chrome.debugger.attach will be rejected [3][1]. If using the older callback-based syntax, chrome.extension.lastError will be set to the corresponding error message [4][3]. 3. Handling Detachment: Because onDetach signals the end of your debugging session, it is standard practice to use this event to clean up any internal state, stop associated monitoring, or update the user interface to reflect that the extension is no longer attached to that specific tab [1]. To avoid issues, always ensure your extension handles the onDetach event gracefully and checks the current attachment status if necessary before attempting to attach. Note that there is no public API to "force" the detachment of an existing debugger from another process; you must wait for the existing session to terminate or handle the error resulting from the conflict [2][3].
Citations:
- 1: https://developer.chrome.com/docs/extensions/mv2/reference/debugger
- 2: https://forum.ui.vision/t/error-another-debugger-is-already-attached-to-the-tab-with-id-357/1273
- 3: https://developer.chrome.com/docs/extensions/reference/api/debugger
- 4: https://groups.google.com/g/google-chrome-developer-tools/c/eGufuLb0QZk
🌐 Web query:
chrome.debugger.onDetach event semantics another debugger already attached attach failure attachedTabs cache chrome extension debugger
💡 Result:
In the Chrome Debugger API, the chrome.debugger.onDetach event is fired when a debugging session for a specific tab or target is terminated [1]. Understanding the semantics of this event and the behavior of attachment failures is essential for maintaining a stable extension [2][3]. Key Semantics and Failure Modes: 1. Detachment Behavior: The onDetach event provides a reason for the termination [1]. Common reasons include the tab being closed, the user invoking Chrome DevTools on the same tab (as Chrome only allows one primary debugger connection at a time), or internal browser events [1][3]. When a session is detached, the extension must handle cleanup to prevent errors in subsequent commands, such as those attempting to use a stale connection [2][3]. 2. "Another Debugger Already Attached": Chrome generally permits only one debugger to be attached to a given target at a time [1]. If an extension attempts to attach to a tab that is already being debugged—either by another extension, an internal process, or the user via the F12 DevTools—the chrome.debugger.attach method will fail [1][4]. 3. Handling Attachment Failures: - Race Conditions: Extensions often encounter race conditions when attempting to attach to a new tab immediately after creation [5]. It is recommended to wait for the tab to commit (or navigate to a stable state) before calling attach [5]. - Persistent Monitoring: To handle unexpected detachments (e.g., when a user opens DevTools or a page navigates to a privileged chrome:// URL), many developers implement a retry mechanism [3]. By listening to onDetach, an extension can log the event and, if appropriate, attempt to re-establish the connection after a short delay [3]. - Troubleshooting: If "Cannot attach to this target" errors persist, ensure no other extensions are interfering and check that your extension has the necessary manifest permissions [4]. In some cases, the debugger may detach automatically due to internal browser navigation constraints [2][3]. For robust implementation, always check chrome.runtime.lastError when calling attach or sendCommand to gracefully handle and log connection failures [5][2][3].
Citations:
- 1: https://developer.chrome.com/docs/extensions/mv2/reference/debugger
- 2: https://stackoverflow.com/questions/68526570/while-using-chrome-debugger-api-it-closes-automatically-and-says-target-closed-b
- 3: Extension Error from running Lighthouse on a github page GoogleChrome/lighthouse#5798
- 4: https://community.latenode.com/t/debugger-issue-in-chrome-extension-for-google-docs-tabs/8407
- 5: https://groups.google.com/a/chromium.org/g/chromium-extensions/c/avq_G6bVmaE
🌐 Web query:
chrome.debugger.onDetach event semantics another debugger already attached attach failure attachedTabs cache chrome extension debugger
💡 Result:
The chrome.debugger.onDetach event is fired when a debugging session is terminated by the browser [1]. This occurs primarily in two scenarios: when the target tab or extension is closed, or when the user opens the Chrome DevTools window for the same tab, which force-detaches any existing debugger [1]. Regarding attachment failures and the error message Another debugger is already attached to the tab with id, note the following: 1. Exclusivity: Chrome only allows a single debugger instance to be attached to a specific target (e.g., a tab) at any given time [1]. If your extension attempts to call chrome.debugger.attach on a tab that is already being debugged—either by another extension or by a user-opened DevTools window—the attachment operation will fail [2][3]. 2. Error Handling: When attach fails due to an existing debugger or other reasons, the promise returned by the method will be rejected (or the callback will receive an error if not using Promises), and chrome.runtime.lastError will contain the specific error message [1]. 3. Handling Detachment: Developers should listen for the onDetach event to clean up local state or inform the user that debugging has ended [1]. Because the browser may detach the extension's debugger without notice (e.g., when a user opens the DevTools UI), relying on onDetach is essential for maintaining a synchronized state [1]. 4. Prevention and Recovery: If your extension encounters this error, it is standard practice to verify if the tab is already being debugged or to implement logic to gracefully handle the rejection [3]. You cannot force-attach if another entity (like the built-in DevTools) is already holding the debugging lock [1]. Always ensure your background script is robust enough to handle unexpected detachment and subsequent re-attachment attempts [3].
Citations:
- 1: https://developer.chrome.com/docs/extensions/mv2/reference/debugger
- 2: https://forum.ui.vision/t/error-another-debugger-is-already-attached-to-the-tab-with-id-357/1273
- 3: Dart Debug Extension background error: 'Another debugger is already attached to the tab with id: _' dart-lang/webdev#615
Do not cache tabs after a failed attach
If chrome.debugger.attach returns Another debugger is already attached, this tab still isn’t attached. Adding it to attachedTabs makes sendCdp() skip the next attach attempt, so the tab stays unusable until this cache is cleared.
🤖 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 `@extension/edge-share/service_worker.js` around lines 558 - 565, The attach
error handling in chrome.debugger.attach is incorrectly caching tabs even when
attach failed, which causes sendCdp() to skip retrying later. Update the
try/catch around chrome.debugger.attach so attachedTabs.add(tabId) only happens
after a successful attach, and in the “Another debugger is already attached”
branch do not add the tab to attachedTabs; instead leave it uncached so the next
sendCdp() call can attempt attach again.
| function normalizeRelayUrl(input) { | ||
| const raw = String(input || "").trim(); | ||
| if (!raw) { | ||
| throw new Error("Relay URL is required"); | ||
| } | ||
|
|
||
| const withScheme = /^[a-z][a-z0-9+.-]*:\/\//i.test(raw) ? raw : `https://${raw}`; | ||
| const url = new URL(withScheme); | ||
|
|
||
| if (url.protocol !== "http:" && url.protocol !== "https:") { | ||
| throw new Error("Relay URL must use http or https"); | ||
| } | ||
|
|
||
| url.search = ""; | ||
| url.hash = ""; | ||
| url.pathname = url.pathname.replace(/\/+$/, ""); | ||
| return url.href.replace(/\/$/, ""); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject cleartext relay URLs outside local development.
Accepting arbitrary http: relays means the browser token, agent token, and live control traffic can all be observed or tampered with on the network. This should be limited to loopback development URLs and require https: everywhere else.
Proposed fix
function normalizeRelayUrl(input) {
@@
const url = new URL(withScheme);
- if (url.protocol !== "http:" && url.protocol !== "https:") {
+ if (url.protocol !== "http:" && url.protocol !== "https:") {
throw new Error("Relay URL must use http or https");
}
+ const isLoopback =
+ url.hostname === "127.0.0.1" ||
+ url.hostname === "localhost" ||
+ url.hostname === "::1" ||
+ url.hostname === "[::1]";
+ if (url.protocol === "http:" && !isLoopback) {
+ throw new Error("Remote relay URLs must use https");
+ }
url.search = "";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function normalizeRelayUrl(input) { | |
| const raw = String(input || "").trim(); | |
| if (!raw) { | |
| throw new Error("Relay URL is required"); | |
| } | |
| const withScheme = /^[a-z][a-z0-9+.-]*:\/\//i.test(raw) ? raw : `https://${raw}`; | |
| const url = new URL(withScheme); | |
| if (url.protocol !== "http:" && url.protocol !== "https:") { | |
| throw new Error("Relay URL must use http or https"); | |
| } | |
| url.search = ""; | |
| url.hash = ""; | |
| url.pathname = url.pathname.replace(/\/+$/, ""); | |
| return url.href.replace(/\/$/, ""); | |
| function normalizeRelayUrl(input) { | |
| const raw = String(input || "").trim(); | |
| if (!raw) { | |
| throw new Error("Relay URL is required"); | |
| } | |
| const withScheme = /^[a-z][a-z0-9+.-]*:\/\//i.test(raw) ? raw : `https://${raw}`; | |
| const url = new URL(withScheme); | |
| if (url.protocol !== "http:" && url.protocol !== "https:") { | |
| throw new Error("Relay URL must use http or https"); | |
| } | |
| const isLoopback = | |
| url.hostname === "127.0.0.1" || | |
| url.hostname === "localhost" || | |
| url.hostname === "::1" || | |
| url.hostname === "[::1]"; | |
| if (url.protocol === "http:" && !isLoopback) { | |
| throw new Error("Remote relay URLs must use https"); | |
| } | |
| url.search = ""; | |
| url.hash = ""; | |
| url.pathname = url.pathname.replace(/\/+$/, ""); | |
| return url.href.replace(/\/$/, ""); |
🤖 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 `@extension/edge-share/service_worker.js` around lines 820 - 836, The
normalizeRelayUrl helper currently accepts any http URL, which should be
restricted to loopback-only development use. Update normalizeRelayUrl so it
still allows https everywhere, but rejects non-loopback http: relay URLs unless
the hostname is localhost, 127.0.0.1, or ::1; keep the existing validation and
cleanup behavior in this function while enforcing the scheme/host check before
returning the normalized href.
| let state = inspect_container_state(&spec.container_name)?; | ||
| match state.as_deref() { | ||
| Some("running") if novnc_proxy_matches(spec)? => { | ||
| let novnc_url = wait_for_novnc_proxy(spec)?; | ||
| return Ok(BrowserTargetDisplayRuntime { | ||
| container_name: spec.container_name.clone(), | ||
| vnc_endpoint: spec.vnc_endpoint.clone(), | ||
| novnc_url, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Include network configuration in the proxy reuse check.
Reuse currently keys only on the VNC endpoint label. If the target's network mode changes between runs, this path will keep the old container even though it is attached to the wrong network, and the readiness probe just times out against that stale proxy. Compare the effective network mode too before deciding not to recreate.
Also applies to: 342-347
🤖 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/browser.rs` around lines 128 - 136, The proxy reuse logic in
BrowserTargetDisplayRuntime currently only checks the VNC endpoint label, so it
can incorrectly reuse a running container even when the effective network mode
has changed. Update the reuse condition in novnc_proxy_matches and the related
BrowserTargetDisplayRuntime path so it also compares the target’s network
configuration before returning the existing container, ensuring a recreated
container is used whenever the network mode differs.
| fn call(&self, command: &str, params: Value) -> Result<Value> { | ||
| let websocket_url = agent_ws_url_from_share_url(&self.share_url)?; | ||
| let (mut socket, _) = connect(&websocket_url).with_context(|| { | ||
| format!("failed to connect to shared browser relay {websocket_url}") | ||
| })?; | ||
| let request = json!({ "id": 1, "command": command, "params": params }); | ||
| socket | ||
| .send(Message::Text(request.to_string())) | ||
| .with_context(|| format!("failed to send shared browser command {command}"))?; | ||
|
|
||
| loop { | ||
| match socket | ||
| .read() | ||
| .with_context(|| format!("failed to read shared browser response for {command}"))? | ||
| { | ||
| Message::Text(text) => { | ||
| let value: Value = serde_json::from_str(&text).with_context(|| { | ||
| format!("shared browser response for {command} was not JSON") | ||
| })?; | ||
| if value.get("id").and_then(Value::as_i64) != Some(1) { | ||
| continue; | ||
| } | ||
| if value.get("ok").and_then(Value::as_bool) == Some(false) { | ||
| let error = value | ||
| .get("error") | ||
| .and_then(Value::as_str) | ||
| .unwrap_or("shared browser command failed"); | ||
| return Err(anyhow!("{error}")); | ||
| } | ||
| return Ok(value.get("result").cloned().unwrap_or(Value::Null)); | ||
| } | ||
| Message::Ping(payload) => { | ||
| socket | ||
| .send(Message::Pong(payload)) | ||
| .context("failed to answer shared browser relay ping")?; | ||
| } | ||
| Message::Close(_) => { | ||
| return Err(anyhow!( | ||
| "shared browser relay closed before {command} response" | ||
| )) | ||
| } | ||
| _ => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Relay registration failures are hidden from the agent.
Here the socket handshake succeeds before RelayEndpoint::parse/state.register, and any failure is then dropped by the client thread. The caller only gets shared browser relay closed before ... response, so bad share links and expired sessions become almost impossible to diagnose through the MCP path. Please return a structured error frame (or reject earlier) instead of silently closing.
Also applies to: 458-476
🤖 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/shared_browser.rs` around lines 193 - 236, The shared browser client in
`SharedBrowser::call` is swallowing relay registration failures by treating a
closed socket as a generic “relay closed” error. Update the relay
handshake/registration path so failures from `RelayEndpoint::parse` and
`state.register` are surfaced back to the caller as a structured error frame, or
fail the connection before entering the read loop. Make sure the `call` method
reports the actual registration error instead of only returning the close
message, and apply the same fix to the other affected relay path referenced in
the review.
| .set_read_timeout(Some(Duration::from_millis(100))) | ||
| .ok(); | ||
| stream.set_write_timeout(Some(Duration::from_secs(5))).ok(); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Idle relay connections spin forever.
Line 451 turns every client thread into a 100ms polling loop, and Lines 520-521 immediately retry on timeout. With MAX_SESSIONS at 1024, idle browser/agent pairs can create thousands of runnable threads and tens of thousands of wakeups per second. Please switch this path to a blocking/evented design so idle connections do not burn CPU.
Also applies to: 497-525
🤖 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/shared_browser.rs` around lines 451 - 453, The relay path in
shared_browser’s connection handling is busy-waiting: the short read timeout
plus the timeout-retry loop in the read/forward logic keeps idle browser/agent
sessions spinning. Update the stream handling around the setup and the retry
loop in shared_browser so it blocks or uses an evented wait instead of
immediately retrying on read timeout, and make sure the forwarding flow no
longer treats idle timeouts as a fast poll. Focus on the connection setup and
the retry logic in the same relay functions that configure the stream and
process reads.
Agent Plan UpdateBranch: 1. Planning DecisionSource: codex - Captured: 2026-06-25T06:00:16Z Questions:
Answer: |
Agent Plan UpdateBranch: 1. PlanSource: codex - Captured: 2026-06-25T06:29:16.269Z noVNC-Control Panel Edge ConnectSummaryДа, это можно сделать на уровне текущего noVNC/control panel без обязательного обновления docker-git: пользователь открывает wrapper-панель noVNC в своём Edge, панель автоматически вызывает расширение, расширение показывает approve, после approve Edge подключается к relay, а панель регистрирует Key Changes
Test Plan
Assumptions
2. PlanSource: codex - Captured: 2026-06-25T06:39:25.841Z Codex PlanSteps
3. PlanSource: codex - Captured: 2026-06-25T06:46:20.879Z Codex PlanSteps
4. PlanSource: codex - Captured: 2026-06-25T06:59:49.744Z Codex PlanSteps
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@extension/edge-share/connect.js`:
- Around line 57-67: The reject() flow in connect.js closes the popup in a
finally block, which can hide failures and close the UI even if sendMessage({
type: "reject_platform_connect", requestId }) fails. Update reject() so
window.close() runs only after sendMessage succeeds, and let any error from
sendMessage surface instead of being swallowed; keep the busy state handling
around the async call.
In `@extension/edge-share/service_worker.js`:
- Around line 117-122: The shared session is losing the requesting tab identity
because `handlePlatformRequest()` captures `sender.tab.id` but
`approvePlatformConnect()` and `startShare()` do not carry it through and reset
`activeTabId` to null. Update the session payload flow so the tab id captured in
`handlePlatformRequest()` is passed into `approvePlatformConnect()` and
preserved by `startShare()` when initializing the shared state, using the
`activeTabId` field instead of clearing it.
In `@src/mcp/control_panel.rs`:
- Around line 203-206: Reject non-loopback Host values before serving the
token-bearing panel or API routes. In the request handling flow around match on
request.method and request_path, add a Host validation step that only allows
loopback/local hosts before calling control_panel_response or
browser_inventory_response, and return a rejection for any other host to prevent
DNS-rebinding/origin-confusion exposure.
- Around line 56-62: The 3-second read/write timeouts are being applied before
the relay WebSocket path in control_panel.rs, which can prematurely disconnect
idle BrowserShareRelay sessions. Update the logic around
is_relay_websocket_request and BrowserShareRelay::handle_stream so the timeout
is either cleared/reset before handing off the stream or only applied on the
HTTP request path, keeping the relay WebSocket connection free of the short
timeout.
- Around line 420-425: The control token generator in generate_control_token
currently reads from /dev/urandom, which is Linux-only and breaks startup on
Windows. Replace the File::open("/dev/urandom").read_exact(...) path with an
OS-agnostic entropy source such as getrandom::getrandom, keeping the existing
context error handling and token hex-encoding logic in generate_control_token.
Also add the getrandom dependency to Cargo.toml so the control panel works
across the full CI matrix.
🪄 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: d391d060-0a7f-4744-8ca7-53dc4f955964
⛔ Files ignored due to path filters (2)
artifacts/edge-share-extension.tar.gzis excluded by!**/*.gzartifacts/edge-share-extension.zipis excluded by!**/*.zip
📒 Files selected for processing (13)
README.mdartifacts/README.mdextension/edge-share/README.mdextension/edge-share/connect.htmlextension/edge-share/connect.jsextension/edge-share/content_script.jsextension/edge-share/manifest.jsonextension/edge-share/popup.jsextension/edge-share/provider.jsextension/edge-share/service_worker.jssrc/mcp/control_panel.rssrc/shared_browser.rstests/mcp_stdio.rs
✅ Files skipped from review due to trivial changes (4)
- artifacts/README.md
- extension/edge-share/connect.html
- extension/edge-share/README.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- extension/edge-share/manifest.json
- extension/edge-share/popup.js
- src/shared_browser.rs
| async function reject() { | ||
| setBusy(true); | ||
|
|
||
| try { | ||
| await sendMessage({ | ||
| type: "reject_platform_connect", | ||
| requestId | ||
| }); | ||
| } finally { | ||
| window.close(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep the popup open until rejection succeeds.
finally { window.close(); } hides any rejection failure and can leave the connect request pending even though the UI disappears. Only close after a successful reject_platform_connect, and surface the error if the message fails.
Suggested fix
async function reject() {
setBusy(true);
try {
await sendMessage({
type: "reject_platform_connect",
requestId
});
- } finally {
window.close();
+ } catch (error) {
+ showError(error);
+ setBusy(false);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function reject() { | |
| setBusy(true); | |
| try { | |
| await sendMessage({ | |
| type: "reject_platform_connect", | |
| requestId | |
| }); | |
| } finally { | |
| window.close(); | |
| } | |
| async function reject() { | |
| setBusy(true); | |
| try { | |
| await sendMessage({ | |
| type: "reject_platform_connect", | |
| requestId | |
| }); | |
| window.close(); | |
| } catch (error) { | |
| showError(error); | |
| setBusy(false); | |
| } | |
| } |
🧰 Tools
🪛 ast-grep (0.44.0)
[error] 57-57: React's useState should not be directly called
Context: setBusy(true)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🤖 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 `@extension/edge-share/connect.js` around lines 57 - 67, The reject() flow in
connect.js closes the popup in a finally block, which can hide failures and
close the UI even if sendMessage({ type: "reject_platform_connect", requestId })
fails. Update reject() so window.close() runs only after sendMessage succeeds,
and let any error from sendMessage surface instead of being swallowed; keep the
busy state handling around the async call.
| stream.set_read_timeout(Some(Duration::from_secs(3))).ok(); | ||
| stream.set_write_timeout(Some(Duration::from_secs(3))).ok(); | ||
|
|
||
| if is_relay_websocket_request(&stream)? { | ||
| return relay | ||
| .handle_stream(stream) | ||
| .context("failed to handle embedded browser share relay client"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t carry the 3-second HTTP timeout into relay WebSockets.
Line 56 sets timeouts before Line 59 hands relay requests to BrowserShareRelay; an idle shared-browser session can fail after 3 seconds. Clear/reset the timeouts before handle_stream, or only apply them to the HTTP request path.
Suggested fix
if is_relay_websocket_request(&stream)? {
+ stream.set_read_timeout(None).ok();
+ stream.set_write_timeout(None).ok();
return relay
.handle_stream(stream)
.context("failed to handle embedded browser share relay client");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| stream.set_read_timeout(Some(Duration::from_secs(3))).ok(); | |
| stream.set_write_timeout(Some(Duration::from_secs(3))).ok(); | |
| if is_relay_websocket_request(&stream)? { | |
| return relay | |
| .handle_stream(stream) | |
| .context("failed to handle embedded browser share relay client"); | |
| stream.set_read_timeout(Some(Duration::from_secs(3))).ok(); | |
| stream.set_write_timeout(Some(Duration::from_secs(3))).ok(); | |
| if is_relay_websocket_request(&stream)? { | |
| stream.set_read_timeout(None).ok(); | |
| stream.set_write_timeout(None).ok(); | |
| return relay | |
| .handle_stream(stream) | |
| .context("failed to handle embedded browser share relay client"); |
🤖 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/control_panel.rs` around lines 56 - 62, The 3-second read/write
timeouts are being applied before the relay WebSocket path in control_panel.rs,
which can prematurely disconnect idle BrowserShareRelay sessions. Update the
logic around is_relay_websocket_request and BrowserShareRelay::handle_stream so
the timeout is either cleared/reset before handing off the stream or only
applied on the HTTP request path, keeping the relay WebSocket connection free of
the short timeout.
| match (request.method.as_str(), request_path(&request.target)) { | ||
| ("OPTIONS", _) => empty_response(204, "No Content"), | ||
| ("GET", "/") | ("GET", "/index.html") => control_panel_response(runtime, control_token), | ||
| ("GET", "/api/browsers") => browser_inventory_response(runtime), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate Host before serving the token-bearing panel.
GET / returns HTML containing controlToken, but routing currently accepts any Host value. Reject non-loopback hosts before serving the panel or APIs to reduce DNS-rebinding/origin-confusion exposure.
Suggested fix
fn route_request(
runtime: Arc<Mutex<McpRuntime>>,
control_token: &str,
request: &HttpRequest,
) -> HttpResponse {
+ if !has_valid_control_host(request) {
+ return json_response(403, "Forbidden", json!({ "error": "invalid host" }));
+ }
+
match (request.method.as_str(), request_path(&request.target)) {+fn has_valid_control_host(request: &HttpRequest) -> bool {
+ let Some(host) = request.header("host") else {
+ return false;
+ };
+ let host = host
+ .trim()
+ .rsplit_once(':')
+ .map(|(name, _)| name)
+ .unwrap_or(host.trim());
+ matches!(host, "127.0.0.1" | "localhost")
+}
+
impl HttpRequest {Also applies to: 238-243
🤖 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/control_panel.rs` around lines 203 - 206, Reject non-loopback Host
values before serving the token-bearing panel or API routes. In the request
handling flow around match on request.method and request_path, add a Host
validation step that only allows loopback/local hosts before calling
control_panel_response or browser_inventory_response, and return a rejection for
any other host to prevent DNS-rebinding/origin-confusion exposure.
| fn generate_control_token() -> Result<String> { | ||
| let mut bytes = [0_u8; 32]; | ||
| File::open("/dev/urandom") | ||
| .context("failed to open /dev/urandom for control panel token")? | ||
| .read_exact(&mut bytes) | ||
| .context("failed to read control panel token bytes")?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check declared platform expectations and whether a portable RNG dependency already exists.
fd -i 'Cargo.toml|README.md|.*\.ya?ml|rust-toolchain.*' -t f . \
| xargs -r rg -n -i 'windows|msvc|edge|getrandom|rand|/dev/urandom'Repository: ProverCoderAI/rust-browser-connection
Length of output: 3519
🏁 Script executed:
cat Cargo.tomlRepository: ProverCoderAI/rust-browser-connection
Length of output: 1116
🏁 Script executed:
head -n 50 src/mcp/control_panel.rsRepository: ProverCoderAI/rust-browser-connection
Length of output: 1892
🏁 Script executed:
sed -n '410,435p' src/mcp/control_panel.rsRepository: ProverCoderAI/rust-browser-connection
Length of output: 959
🏁
Length of output: 25
🏁 Script executed:
fn generate_control_token() -> Result<String> {
let mut bytes = [0_u8; 32];
// getrandom::getrandom(&mut bytes)?; // Requires adding getrandom to Cargo.toml
std::fs::File::open("/dev/urandom") // Fails on Windows
.context("failed to open /dev/urandom for control panel token")?
.read_exact(&mut bytes)
.context("failed to read control panel token bytes")?;
// ...
}Repository: src/mcp/control_panel.rs
Length of output: 2056
Use a cross-platform entropy source for the control token
The current implementation uses File::open("/dev/urandom"), which does not exist on Windows and will cause the control panel to fail at startup. Since your CI matrix includes windows-latest, you must replace this with an OS-agnostic RNG, such as the getrandom crate.
Add getrandom = "0.2" to your Cargo.toml dependencies and update the function as follows:
// src/mcp/control_panel.rs
fn generate_control_token() -> Result<String> {
let mut bytes = [0_u8; 32];
getrandom::getrandom(&mut bytes)
.context("failed to read control panel token bytes")?;
let mut token = String::with_capacity(bytes.len() * 2);
for byte in bytes {
write!(&mut token, "{byte:02x}").expect("writing to a String cannot fail");
}
Ok(token)
}🤖 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/control_panel.rs` around lines 420 - 425, The control token generator
in generate_control_token currently reads from /dev/urandom, which is Linux-only
and breaks startup on Windows. Replace the
File::open("/dev/urandom").read_exact(...) path with an OS-agnostic entropy
source such as getrandom::getrandom, keeping the existing context error handling
and token hex-encoding logic in generate_control_token. Also add the getrandom
dependency to Cargo.toml so the control panel works across the full CI matrix.
Agent Plan UpdateBranch: 1. PlanSource: codex - Captured: 2026-06-25T07:00:59.727Z Codex PlanSteps
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
extension/edge-share/service_worker.js (1)
271-283: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard close handlers against stale sockets.
connectRelay()can close an old socket and immediately assign a new one; if the old socket’scloseevent fires after the replacement opens, Line 272 can clear the new connection’s keepalive timer and Line 273 can null the active socket. Capture the socket instance and ignore lifecycle events from stale sockets.Proposed fix
- socket = new WebSocket(wsUrl); + const currentSocket = new WebSocket(wsUrl); + socket = currentSocket; @@ - socket.addEventListener("open", () => { + currentSocket.addEventListener("open", () => { + if (socket !== currentSocket) return; @@ - socket.addEventListener("message", (event) => { + currentSocket.addEventListener("message", (event) => { + if (socket !== currentSocket) return; @@ - socket.addEventListener("close", (event) => { + currentSocket.addEventListener("close", (event) => { + if (socket !== currentSocket) return; clearKeepAliveTimer(); socket = null; @@ - socket.addEventListener("error", () => { + currentSocket.addEventListener("error", () => { + if (socket !== currentSocket) return; clearKeepAliveTimer();🤖 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 `@extension/edge-share/service_worker.js` around lines 271 - 283, The socket close handler in connectRelay() needs to ignore stale close events from previously replaced sockets. Capture the current socket instance when registering the close listener, then in the handler verify it still matches the active socket before calling clearKeepAliveTimer(), setting socket to null, or scheduling reconnect/persisting disconnected state. Use the connectRelay() socket setup and its close event callback to apply the guard so only the live connection can affect lifecycle state.src/mcp/control_panel.rs (1)
203-207: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire the control token for browser inventory.
/api/browsersis unauthenticated, but inventory includesshareUrland endpoint metadata. Gate it like the other panel APIs and call it throughapiFetch().Proposed fix
- ("GET", "/api/browsers") => browser_inventory_response(runtime), + ("GET", "/api/browsers") => { + if !has_valid_control_token(request, control_token) { + return json_response( + 403, + "Forbidden", + json!({ "error": "invalid control token" }), + ); + } + browser_inventory_response(runtime) + }, @@ - const response = await fetch("/api/browsers", { cache: "no-store" }); + const response = await apiFetch("/api/browsers", { cache: "no-store" });Also applies to: 738-740
🤖 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/control_panel.rs` around lines 203 - 207, The browser inventory endpoint is currently exposed without authentication, which leaks `shareUrl` and endpoint metadata. Update the request routing in `control_panel_response` so the "/api/browsers" branch is gated by the control token like the other panel APIs, and ensure the client side calls it through `apiFetch()` instead of a raw fetch. Use the existing `apiFetch` and `browser_inventory_response` symbols to locate the affected flow and keep the auth handling consistent with the other control panel endpoints.
🤖 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 273-290: The shared-browser I/O is being performed while the
runtime mutex is held, which can block MCP and control panel requests. Refactor
browser_activity and refresh_active_shared_tabs, and also the activate_tab flow
referenced in the review, so they only read the share URL or tab state under
runtime lock, then drop the lock before calling
SharedBrowserClient::new(...).list_tabs() or activate_tab(), and finally re-lock
only to record the resulting activity/error state.
In `@src/mcp/activity.rs`:
- Around line 78-85: Activity events are still recording user-entered text from
browser_type, so redact typed input before it reaches the event payload. Update
the activity event construction in the event-writing logic around
push_front/json! and the related code path that also mirrors this payload to
ensure arguments are sanitized for browser_type, not just token/share URL keys.
Use the existing sanitize_value flow as the central place to strip or mask typed
text before storing anything exposed through /api/activity.
- Around line 30-38: The activity snapshot is returning every retained
screenshot with full data URLs, which makes each poll unnecessarily large.
Update snapshot() in ActivityState to stop cloning the full screenshots history
and instead return only the latest full image plus lightweight metadata or
thumbnails for prior entries. Use the existing latestScreenshot and screenshots
fields as the place to reshape the payload so /api/activity stays small while
preserving recent image access.
In `@src/mcp/tools.rs`:
- Around line 141-144: The browser_type branch in the tool dispatch is trimming
the typed text by using required_str for the text argument, which strips
intentional whitespace. Update the browser_type handling in src/mcp/tools.rs so
selector/URL-style inputs still use required_str, but the text parameter is read
with a raw non-empty helper that preserves leading and trailing spaces; apply
the same change anywhere else the browser_type text path is handled in the
referenced dispatch cases.
---
Outside diff comments:
In `@extension/edge-share/service_worker.js`:
- Around line 271-283: The socket close handler in connectRelay() needs to
ignore stale close events from previously replaced sockets. Capture the current
socket instance when registering the close listener, then in the handler verify
it still matches the active socket before calling clearKeepAliveTimer(), setting
socket to null, or scheduling reconnect/persisting disconnected state. Use the
connectRelay() socket setup and its close event callback to apply the guard so
only the live connection can affect lifecycle state.
In `@src/mcp/control_panel.rs`:
- Around line 203-207: The browser inventory endpoint is currently exposed
without authentication, which leaks `shareUrl` and endpoint metadata. Update the
request routing in `control_panel_response` so the "/api/browsers" branch is
gated by the control token like the other panel APIs, and ensure the client side
calls it through `apiFetch()` instead of a raw fetch. Use the existing
`apiFetch` and `browser_inventory_response` symbols to locate the affected flow
and keep the auth handling consistent with the other control panel endpoints.
🪄 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: 463d038e-8e43-43a7-9848-664483deeb9b
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockartifacts/edge-share-extension.tar.gzis excluded by!**/*.gzartifacts/edge-share-extension.zipis excluded by!**/*.zip
📒 Files selected for processing (7)
Cargo.tomlextension/edge-share/service_worker.jssrc/mcp.rssrc/mcp/activity.rssrc/mcp/control_panel.rssrc/mcp/tools.rssrc/shared_browser.rs
| fn browser_activity(&mut self, refresh: bool) -> Value { | ||
| if refresh { | ||
| self.refresh_active_shared_tabs(); | ||
| } | ||
| self.activity | ||
| .snapshot(&self.active_browser, &self.active_browser_mode()) | ||
| } | ||
|
|
||
| fn refresh_active_shared_tabs(&mut self) { | ||
| let Some(share_url) = self.active_share_url() else { | ||
| self.activity.clear_tabs(); | ||
| return; | ||
| }; | ||
| match SharedBrowserClient::new(share_url).list_tabs() { | ||
| Ok(tabs) => self.activity.record_tabs(tabs), | ||
| Err(error) => self.activity.record_tab_error(error), | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Don’t run shared-browser network calls while holding the runtime mutex.
list_tabs() and activate_tab() can block on relay/browser I/O, and the control panel invokes these through runtime.lock(), stalling MCP tool calls and panel requests behind a slow or missing extension. Snapshot the share URL under lock, drop the lock for relay I/O, then re-lock only to record activity.
Also applies to: 319-331
🤖 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 273 - 290, The shared-browser I/O is being performed
while the runtime mutex is held, which can block MCP and control panel requests.
Refactor browser_activity and refresh_active_shared_tabs, and also the
activate_tab flow referenced in the review, so they only read the share URL or
tab state under runtime lock, then drop the lock before calling
SharedBrowserClient::new(...).list_tabs() or activate_tab(), and finally re-lock
only to record the resulting activity/error state.
| pub(super) fn snapshot(&self, active_browser: &str, mode: &str) -> Value { | ||
| json!({ | ||
| "activeBrowser": active_browser, | ||
| "mode": mode, | ||
| "tabs": self.latest_tabs.clone().unwrap_or(Value::Null), | ||
| "tabsError": self.latest_tab_error, | ||
| "screenshots": self.screenshots.iter().cloned().collect::<Vec<_>>(), | ||
| "latestScreenshot": self.screenshots.front().cloned().unwrap_or(Value::Null), | ||
| "events": self.events.iter().cloned().collect::<Vec<_>>(), |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Avoid returning every full screenshot in each activity poll.
snapshot() clones all retained screenshots including full dataUrls; with 24 screenshots and 8 MiB relay messages, /api/activity can serialize hundreds of MiB repeatedly. Return only the latest full image plus metadata/thumbnails for history.
Also applies to: 94-112
🤖 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/activity.rs` around lines 30 - 38, The activity snapshot is returning
every retained screenshot with full data URLs, which makes each poll
unnecessarily large. Update snapshot() in ActivityState to stop cloning the full
screenshots history and instead return only the latest full image plus
lightweight metadata or thumbnails for prior entries. Use the existing
latestScreenshot and screenshots fields as the place to reshape the payload so
/api/activity stays small while preserving recent image access.
| self.events.push_front(json!({ | ||
| "id": event_id, | ||
| "at": started_at_ms, | ||
| "browser": browser, | ||
| "mode": mode, | ||
| "tool": tool, | ||
| "arguments": sanitize_value(arguments), | ||
| "ok": ok, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact typed text before writing activity events.
browser_type arguments include user-entered text, but only token/share URL keys are redacted. This can expose passwords or other PII through /api/activity.
Proposed fix
- "arguments": sanitize_value(arguments),
+ "arguments": sanitize_arguments(tool, arguments),
@@
+fn sanitize_arguments(tool: &str, arguments: &Value) -> Value {
+ if tool == "browser_type" {
+ let mut sanitized = sanitize_value(arguments);
+ if let Value::Object(object) = &mut sanitized {
+ object.insert("text".to_string(), json!("[redacted]"));
+ }
+ return sanitized;
+ }
+ sanitize_value(arguments)
+}
+
fn sanitize_value(value: &Value) -> Value {Also applies to: 130-139
🤖 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/activity.rs` around lines 78 - 85, Activity events are still
recording user-entered text from browser_type, so redact typed input before it
reaches the event payload. Update the activity event construction in the
event-writing logic around push_front/json! and the related code path that also
mirrors this payload to ensure arguments are sanitized for browser_type, not
just token/share URL keys. Use the existing sanitize_value flow as the central
place to strip or mask typed text before storing anything exposed through
/api/activity.
| "browser_type" => client.type_text( | ||
| required_str(arguments, "selector")?, | ||
| required_str(arguments, "text")?, | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve whitespace for browser_type text.
required_str() trims every string argument, so typing " value " sends "value". Keep trimming for selectors/URLs, but use a raw non-empty helper for the text argument.
Proposed fix
- required_str(arguments, "text")?,
+ required_raw_str(arguments, "text")?,
@@
- required_str(arguments, "text")?,
+ required_raw_str(arguments, "text")?,
@@
fn required_str<'a>(arguments: &'a Value, name: &str) -> Result<&'a str> {
@@
}
+
+fn required_raw_str<'a>(arguments: &'a Value, name: &str) -> Result<&'a str> {
+ arguments
+ .get(name)
+ .and_then(Value::as_str)
+ .filter(|value| !value.is_empty())
+ .ok_or_else(|| anyhow!("argument `{name}` is required"))
+}Also applies to: 168-170, 188-194
🤖 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/tools.rs` around lines 141 - 144, The browser_type branch in the tool
dispatch is trimming the typed text by using required_str for the text argument,
which strips intentional whitespace. Update the browser_type handling in
src/mcp/tools.rs so selector/URL-style inputs still use required_str, but the
text parameter is read with a raw non-empty helper that preserves leading and
trailing spaces; apply the same change anywhere else the browser_type text path
is handled in the referenced dispatch cases.
Agent Plan UpdateBranch: 1. PlanSource: codex - Captured: 2026-06-25T09:03:55.634Z CLI-First Browser Control Without MCPSummaryСделать отдельный CLI-слой для управления браузером без MCP: агент будет вызывать обычные команды и JS-скрипты, а MCP останется только совместимым wrapper поверх того же action dispatcher. Основной новый binary: Key Changes
Skill
Test Plan
Assumptions
|
Agent Plan UpdateBranch: 1. PlanSource: codex - Captured: 2026-06-25T09:43:45.866Z Add Real Playwright Runner To
|
Agent Plan UpdateBranch: 1. PlanSource: codex - Captured: 2026-06-25T10:20:18.297Z Browser Pool Defaults For
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
tests/cli_status.rs (1)
67-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the validation-before-resolution path.
Because this test passes
--cdp-url, it bypasses control-panel resolution and would not catchevaldoing network work before argument validation. Add a variant withBROWSER_CONNECTION_CONTROL_URLset and no--cdp-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 `@tests/cli_status.rs` around lines 67 - 75, The current eval validation test only covers the --cdp-url path, so it misses the validation-before-resolution behavior. Update rbc_eval_requires_expression_or_file_before_network in cli_status.rs to add a case that sets BROWSER_CONNECTION_CONTROL_URL without passing --cdp-url and still asserts the same eval error, so the eval command is verified before any control-panel/network resolution happens.
🤖 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/bin/playwright/mod.rs`:
- Around line 286-292: The temporary wrapper file creation in
write_temp_playwright_wrapper is vulnerable because fs::write can follow
existing paths and does not enforce private permissions. Update this function to
create the file atomically with create_new, set restrictive 0600 permissions
before writing the Playwright source, and keep the existing temp name
generation/context handling so the wrapper file is only created if it does not
already exist.
- Around line 97-171: The stderr pipe in run_node_shared_playwright_wrapper is
only read after the stdout loop finishes, which can deadlock if the Node process
writes enough error output; change the bridge handling so stderr is drained
concurrently while reading stdout. Update the logic around Command::new("node"),
the stdout loop, and read_stderr so Node’s stderr is consumed in parallel (for
example with a separate reader thread or nonblocking drain) and any stderr text
is still collected for the final error/status handling.
In `@src/bin/rbc.rs`:
- Around line 167-170: Validate the tool command before any target resolution in
run_tool_cli. Move the tool parsing/validation using tool_command(&cli.command)
and browser_command(&tool)? ahead of resolve_target(cli)? so invalid local input
is rejected first and does not get masked by control panel or network failures.
In `@src/bin/target_resolution/mod.rs`:
- Around line 75-78: The control-panel fetch in target resolution can hang
indefinitely because the curl invocation has no timeout. Update the request path
in the target resolution logic around the curl Command::new("curl") call to
enforce a bounded timeout, and handle timeout failures with a clear error so a
stale BROWSER_CONNECTION_CONTROL_URL or stalled local port does not block rbc
forever.
In `@src/browser_actions.rs`:
- Around line 87-90: The `browser_type` handling in `BrowserCommand::TypeText`
is trimming user text via `required_str()`, which changes the exact content and
rejects whitespace-only input; switch this command to read the raw string
argument without trimming so the text is preserved exactly. Apply the same fix
anywhere else in the browser action parsing path that uses the same text-input
logic, including the other referenced `browser_type` cases.
- Around line 32-40: `safe_label()` is still leaking sensitive URL components
for both `Self::Cdp` and `Self::Shared`, so update it to return only sanitized
labels. In `browser_actions::BrowserTarget::safe_label`, parse the
endpoint/share URL and redact userinfo, query strings, and any token-bearing
fragment or relay-auth params (including `agent`) before formatting the label.
Preserve the existing `cdp:` and `shared:` prefixes, but ensure the returned
string never includes credentials, raw tokens, or full URLs.
---
Nitpick comments:
In `@tests/cli_status.rs`:
- Around line 67-75: The current eval validation test only covers the --cdp-url
path, so it misses the validation-before-resolution behavior. Update
rbc_eval_requires_expression_or_file_before_network in cli_status.rs to add a
case that sets BROWSER_CONNECTION_CONTROL_URL without passing --cdp-url and
still asserts the same eval error, so the eval command is verified before any
control-panel/network resolution happens.
🪄 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: 7b39b9c2-c291-495b-a54e-ee4a07b6e20a
⛔ Files ignored due to path filters (2)
artifacts/edge-share-extension.tar.gzis excluded by!**/*.gzartifacts/edge-share-extension.zipis excluded by!**/*.zip
📒 Files selected for processing (17)
README.mdchangelog.d/20260625_000000_rbc_cli.mdextension/edge-share/README.mdextension/edge-share/service_worker.jsskills/browser-cli/SKILL.mdskills/browser-cli/agents/openai.yamlsrc/bin/playwright/mod.rssrc/bin/rbc.rssrc/bin/target_resolution/mod.rssrc/browser_actions.rssrc/lib.rssrc/mcp/activity.rssrc/mcp/control_panel.rssrc/mcp/tools.rssrc/shared_browser.rstests/cli_status.rstests/shared_browser_relay.rs
✅ Files skipped from review due to trivial changes (3)
- changelog.d/20260625_000000_rbc_cli.md
- skills/browser-cli/agents/openai.yaml
- skills/browser-cli/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (6)
- extension/edge-share/README.md
- src/mcp/activity.rs
- src/mcp/control_panel.rs
- src/lib.rs
- extension/edge-share/service_worker.js
- src/shared_browser.rs
| fn run_node_shared_playwright_wrapper(share_url: &str, wrapper_path: &Path) -> Result<String> { | ||
| let mut child = Command::new("node") | ||
| .arg(wrapper_path) | ||
| .stdin(Stdio::piped()) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()) | ||
| .spawn() | ||
| .context("failed to run node for shared-extension rbc pw")?; | ||
| let mut stdin = child | ||
| .stdin | ||
| .take() | ||
| .ok_or_else(|| anyhow!("failed to open node stdin for shared-extension rbc pw"))?; | ||
| let stdout = child | ||
| .stdout | ||
| .take() | ||
| .ok_or_else(|| anyhow!("failed to open node stdout for shared-extension rbc pw"))?; | ||
| let mut stderr = child | ||
| .stderr | ||
| .take() | ||
| .ok_or_else(|| anyhow!("failed to open node stderr for shared-extension rbc pw"))?; | ||
| let client = SharedBrowserClient::new(share_url); | ||
| let mut reader = BufReader::new(stdout); | ||
| let mut line = String::new(); | ||
| let mut final_result = None; | ||
| let mut final_error = None; | ||
| let mut plain_output = String::new(); | ||
|
|
||
| loop { | ||
| line.clear(); | ||
| let bytes = reader | ||
| .read_line(&mut line) | ||
| .context("failed to read shared-extension rbc pw bridge output")?; | ||
| if bytes == 0 { | ||
| break; | ||
| } | ||
| let text = line.trim_end_matches(['\r', '\n']); | ||
| if text.is_empty() { | ||
| continue; | ||
| } | ||
|
|
||
| let Ok(message) = serde_json::from_str::<Value>(text) else { | ||
| plain_output.push_str(text); | ||
| plain_output.push('\n'); | ||
| continue; | ||
| }; | ||
| match message.get("type").and_then(Value::as_str) { | ||
| Some("rbc-command") => { | ||
| let response = shared_bridge_response(&client, &message); | ||
| writeln!(stdin, "{}", serde_json::to_string(&response)?) | ||
| .context("failed to write shared-extension rbc pw bridge response")?; | ||
| } | ||
| Some("rbc-result") => { | ||
| final_result = Some(message.get("result").cloned().unwrap_or(Value::Null)); | ||
| break; | ||
| } | ||
| Some("rbc-error") => { | ||
| let error = message | ||
| .get("error") | ||
| .and_then(Value::as_str) | ||
| .unwrap_or("shared-extension rbc pw failed"); | ||
| final_error = Some(error.to_string()); | ||
| break; | ||
| } | ||
| _ => { | ||
| plain_output.push_str(text); | ||
| plain_output.push('\n'); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| drop(stdin); | ||
| let status = child | ||
| .wait() | ||
| .context("failed to wait for shared-extension rbc pw")?; | ||
| let stderr_text = read_stderr(&mut stderr)?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Drain Node stderr while reading bridge stdout.
This reads stdout line-by-line while stderr remains piped until after wait(). If the user script writes enough stderr, Node can block on the full stderr pipe while Rust waits forever for the next stdout line.
Suggested direction
- let mut stderr = child
+ let mut stderr = child
.stderr
.take()
.ok_or_else(|| anyhow!("failed to open node stderr for shared-extension rbc pw"))?;
+ let stderr_reader = std::thread::spawn(move || {
+ let mut text = String::new();
+ stderr.read_to_string(&mut text).map(|_| text)
+ });
@@
- let stderr_text = read_stderr(&mut stderr)?;
+ let stderr_text = stderr_reader
+ .join()
+ .map_err(|_| anyhow!("shared-extension rbc pw stderr reader panicked"))?
+ .context("failed to read shared-extension rbc pw stderr")?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn run_node_shared_playwright_wrapper(share_url: &str, wrapper_path: &Path) -> Result<String> { | |
| let mut child = Command::new("node") | |
| .arg(wrapper_path) | |
| .stdin(Stdio::piped()) | |
| .stdout(Stdio::piped()) | |
| .stderr(Stdio::piped()) | |
| .spawn() | |
| .context("failed to run node for shared-extension rbc pw")?; | |
| let mut stdin = child | |
| .stdin | |
| .take() | |
| .ok_or_else(|| anyhow!("failed to open node stdin for shared-extension rbc pw"))?; | |
| let stdout = child | |
| .stdout | |
| .take() | |
| .ok_or_else(|| anyhow!("failed to open node stdout for shared-extension rbc pw"))?; | |
| let mut stderr = child | |
| .stderr | |
| .take() | |
| .ok_or_else(|| anyhow!("failed to open node stderr for shared-extension rbc pw"))?; | |
| let client = SharedBrowserClient::new(share_url); | |
| let mut reader = BufReader::new(stdout); | |
| let mut line = String::new(); | |
| let mut final_result = None; | |
| let mut final_error = None; | |
| let mut plain_output = String::new(); | |
| loop { | |
| line.clear(); | |
| let bytes = reader | |
| .read_line(&mut line) | |
| .context("failed to read shared-extension rbc pw bridge output")?; | |
| if bytes == 0 { | |
| break; | |
| } | |
| let text = line.trim_end_matches(['\r', '\n']); | |
| if text.is_empty() { | |
| continue; | |
| } | |
| let Ok(message) = serde_json::from_str::<Value>(text) else { | |
| plain_output.push_str(text); | |
| plain_output.push('\n'); | |
| continue; | |
| }; | |
| match message.get("type").and_then(Value::as_str) { | |
| Some("rbc-command") => { | |
| let response = shared_bridge_response(&client, &message); | |
| writeln!(stdin, "{}", serde_json::to_string(&response)?) | |
| .context("failed to write shared-extension rbc pw bridge response")?; | |
| } | |
| Some("rbc-result") => { | |
| final_result = Some(message.get("result").cloned().unwrap_or(Value::Null)); | |
| break; | |
| } | |
| Some("rbc-error") => { | |
| let error = message | |
| .get("error") | |
| .and_then(Value::as_str) | |
| .unwrap_or("shared-extension rbc pw failed"); | |
| final_error = Some(error.to_string()); | |
| break; | |
| } | |
| _ => { | |
| plain_output.push_str(text); | |
| plain_output.push('\n'); | |
| } | |
| } | |
| } | |
| drop(stdin); | |
| let status = child | |
| .wait() | |
| .context("failed to wait for shared-extension rbc pw")?; | |
| let stderr_text = read_stderr(&mut stderr)?; | |
| fn run_node_shared_playwright_wrapper(share_url: &str, wrapper_path: &Path) -> Result<String> { | |
| let mut child = Command::new("node") | |
| .arg(wrapper_path) | |
| .stdin(Stdio::piped()) | |
| .stdout(Stdio::piped()) | |
| .stderr(Stdio::piped()) | |
| .spawn() | |
| .context("failed to run node for shared-extension rbc pw")?; | |
| let mut stdin = child | |
| .stdin | |
| .take() | |
| .ok_or_else(|| anyhow!("failed to open node stdin for shared-extension rbc pw"))?; | |
| let stdout = child | |
| .stdout | |
| .take() | |
| .ok_or_else(|| anyhow!("failed to open node stdout for shared-extension rbc pw"))?; | |
| let mut stderr = child | |
| .stderr | |
| .take() | |
| .ok_or_else(|| anyhow!("failed to open node stderr for shared-extension rbc pw"))?; | |
| let stderr_reader = std::thread::spawn(move || { | |
| let mut text = String::new(); | |
| stderr.read_to_string(&mut text).map(|_| text) | |
| }); | |
| let client = SharedBrowserClient::new(share_url); | |
| let mut reader = BufReader::new(stdout); | |
| let mut line = String::new(); | |
| let mut final_result = None; | |
| let mut final_error = None; | |
| let mut plain_output = String::new(); | |
| loop { | |
| line.clear(); | |
| let bytes = reader | |
| .read_line(&mut line) | |
| .context("failed to read shared-extension rbc pw bridge output")?; | |
| if bytes == 0 { | |
| break; | |
| } | |
| let text = line.trim_end_matches(['\r', '\n']); | |
| if text.is_empty() { | |
| continue; | |
| } | |
| let Ok(message) = serde_json::from_str::<Value>(text) else { | |
| plain_output.push_str(text); | |
| plain_output.push('\n'); | |
| continue; | |
| }; | |
| match message.get("type").and_then(Value::as_str) { | |
| Some("rbc-command") => { | |
| let response = shared_bridge_response(&client, &message); | |
| writeln!(stdin, "{}", serde_json::to_string(&response)?) | |
| .context("failed to write shared-extension rbc pw bridge response")?; | |
| } | |
| Some("rbc-result") => { | |
| final_result = Some(message.get("result").cloned().unwrap_or(Value::Null)); | |
| break; | |
| } | |
| Some("rbc-error") => { | |
| let error = message | |
| .get("error") | |
| .and_then(Value::as_str) | |
| .unwrap_or("shared-extension rbc pw failed"); | |
| final_error = Some(error.to_string()); | |
| break; | |
| } | |
| _ => { | |
| plain_output.push_str(text); | |
| plain_output.push('\n'); | |
| } | |
| } | |
| } | |
| drop(stdin); | |
| let status = child | |
| .wait() | |
| .context("failed to wait for shared-extension rbc pw")?; | |
| let stderr_text = stderr_reader | |
| .join() | |
| .map_err(|_| anyhow!("shared-extension rbc pw stderr reader panicked"))? | |
| .context("failed to read shared-extension rbc pw stderr")?; |
🤖 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/bin/playwright/mod.rs` around lines 97 - 171, The stderr pipe in
run_node_shared_playwright_wrapper is only read after the stdout loop finishes,
which can deadlock if the Node process writes enough error output; change the
bridge handling so stderr is drained concurrently while reading stdout. Update
the logic around Command::new("node"), the stdout loop, and read_stderr so
Node’s stderr is consumed in parallel (for example with a separate reader thread
or nonblocking drain) and any stderr text is still collected for the final
error/status handling.
| fn write_temp_playwright_wrapper(source: &str) -> Result<PathBuf> { | ||
| let path = std::env::temp_dir().join(format!( | ||
| "rbc-playwright-{}-{}.js", | ||
| std::process::id(), | ||
| unix_ms() | ||
| )); | ||
| fs::write(&path, source).with_context(|| format!("failed to write {}", path.display()))?; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Create wrapper files atomically with private permissions.
fs::write creates a predictable temp-path file and follows pre-existing paths. Since the wrapper contains user Playwright code, use create_new and 0600 permissions to avoid temp-dir disclosure/symlink risks.
Suggested fix
fn write_temp_playwright_wrapper(source: &str) -> Result<PathBuf> {
let path = std::env::temp_dir().join(format!(
"rbc-playwright-{}-{}.js",
std::process::id(),
unix_ms()
));
- fs::write(&path, source).with_context(|| format!("failed to write {}", path.display()))?;
+ let mut options = fs::OpenOptions::new();
+ options.write(true).create_new(true);
+ #[cfg(unix)]
+ {
+ use std::os::unix::fs::OpenOptionsExt;
+ options.mode(0o600);
+ }
+ let mut file = options
+ .open(&path)
+ .with_context(|| format!("failed to create {}", path.display()))?;
+ file.write_all(source.as_bytes())
+ .with_context(|| format!("failed to write {}", path.display()))?;
Ok(path)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn write_temp_playwright_wrapper(source: &str) -> Result<PathBuf> { | |
| let path = std::env::temp_dir().join(format!( | |
| "rbc-playwright-{}-{}.js", | |
| std::process::id(), | |
| unix_ms() | |
| )); | |
| fs::write(&path, source).with_context(|| format!("failed to write {}", path.display()))?; | |
| fn write_temp_playwright_wrapper(source: &str) -> Result<PathBuf> { | |
| let path = std::env::temp_dir().join(format!( | |
| "rbc-playwright-{}-{}.js", | |
| std::process::id(), | |
| unix_ms() | |
| )); | |
| let mut options = fs::OpenOptions::new(); | |
| options.write(true).create_new(true); | |
| #[cfg(unix)] | |
| { | |
| use std::os::unix::fs::OpenOptionsExt; | |
| options.mode(0o600); | |
| } | |
| let mut file = options | |
| .open(&path) | |
| .with_context(|| format!("failed to create {}", path.display()))?; | |
| file.write_all(source.as_bytes()) | |
| .with_context(|| format!("failed to write {}", path.display()))?; | |
| Ok(path) | |
| } |
🤖 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/bin/playwright/mod.rs` around lines 286 - 292, The temporary wrapper file
creation in write_temp_playwright_wrapper is vulnerable because fs::write can
follow existing paths and does not enforce private permissions. Update this
function to create the file atomically with create_new, set restrictive 0600
permissions before writing the Playwright source, and keep the existing temp
name generation/context handling so the wrapper file is only created if it does
not already exist.
| fn run_tool_cli(cli: &Cli) -> Result<()> { | ||
| let target = resolve_target(cli)?; | ||
| let tool = tool_command(&cli.command); | ||
| let command = browser_command(&tool)?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate tool input before resolving a browser target.
resolve_target(cli)? can hit the control panel before browser_command(&tool)? rejects invalid input like bare eval, so local validation can be masked by network/control-panel failures.
Suggested fix
fn run_tool_cli(cli: &Cli) -> Result<()> {
- let target = resolve_target(cli)?;
let tool = tool_command(&cli.command);
let command = browser_command(&tool)?;
+ let target = resolve_target(cli)?;
let started = Instant::now();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn run_tool_cli(cli: &Cli) -> Result<()> { | |
| let target = resolve_target(cli)?; | |
| let tool = tool_command(&cli.command); | |
| let command = browser_command(&tool)?; | |
| fn run_tool_cli(cli: &Cli) -> Result<()> { | |
| let tool = tool_command(&cli.command); | |
| let command = browser_command(&tool)?; | |
| let target = resolve_target(cli)?; |
🤖 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/bin/rbc.rs` around lines 167 - 170, Validate the tool command before any
target resolution in run_tool_cli. Move the tool parsing/validation using
tool_command(&cli.command) and browser_command(&tool)? ahead of
resolve_target(cli)? so invalid local input is rejected first and does not get
masked by control panel or network failures.
| let output = Command::new("curl") | ||
| .args(["-fsS", url.as_str()]) | ||
| .output() | ||
| .with_context(|| format!("failed to run curl for {url}"))?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add timeouts to control-panel inventory requests.
curl has no timeout here; a stale BROWSER_CONNECTION_CONTROL_URL or stalled local control-panel port can hang rbc indefinitely during target resolution.
Suggested fix
let output = Command::new("curl")
- .args(["-fsS", url.as_str()])
+ .args(["-fsS", "--connect-timeout", "2", "--max-time", "5", url.as_str()])
.output()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let output = Command::new("curl") | |
| .args(["-fsS", url.as_str()]) | |
| .output() | |
| .with_context(|| format!("failed to run curl for {url}"))?; | |
| let output = Command::new("curl") | |
| .args(["-fsS", "--connect-timeout", "2", "--max-time", "5", url.as_str()]) | |
| .output() | |
| .with_context(|| format!("failed to run curl for {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/bin/target_resolution/mod.rs` around lines 75 - 78, The control-panel
fetch in target resolution can hang indefinitely because the curl invocation has
no timeout. Update the request path in the target resolution logic around the
curl Command::new("curl") call to enforce a bounded timeout, and handle timeout
failures with a clear error so a stale BROWSER_CONNECTION_CONTROL_URL or stalled
local port does not block rbc forever.
| pub fn safe_label(&self) -> String { | ||
| match self { | ||
| Self::Cdp { endpoint } => format!("cdp:{endpoint}"), | ||
| Self::Shared { share_url } => { | ||
| let redacted = share_url | ||
| .split_once("#agent=") | ||
| .map(|(base, _)| format!("{base}#agent=<redacted>")) | ||
| .unwrap_or_else(|| "<shared-browser-url>".to_string()); | ||
| format!("shared:{redacted}") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact credentials and token-bearing URL parts in safe_label.
safe_label() still emits full CDP endpoints and the shared URL base; URLs like https://user:pass@host/..., ...?token=..., or other relay-auth query params would be printed by rbc JSON output via target.safe_label().
🛡️ Suggested direction
- Self::Cdp { endpoint } => format!("cdp:{endpoint}"),
- Self::Shared { share_url } => {
- let redacted = share_url
- .split_once("`#agent`=")
- .map(|(base, _)| format!("{base}`#agent`=<redacted>"))
- .unwrap_or_else(|| "<shared-browser-url>".to_string());
- format!("shared:{redacted}")
- }
+ Self::Cdp { endpoint } => format!("cdp:{}", redact_url_label(endpoint)),
+ Self::Shared { share_url } => format!("shared:{}", redact_url_label(share_url)),Redact URL userinfo, query strings, and known sensitive fragment params such as agent.
🤖 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/browser_actions.rs` around lines 32 - 40, `safe_label()` is still leaking
sensitive URL components for both `Self::Cdp` and `Self::Shared`, so update it
to return only sanitized labels. In
`browser_actions::BrowserTarget::safe_label`, parse the endpoint/share URL and
redact userinfo, query strings, and any token-bearing fragment or relay-auth
params (including `agent`) before formatting the label. Preserve the existing
`cdp:` and `shared:` prefixes, but ensure the returned string never includes
credentials, raw tokens, or full URLs.
| "browser_type" => Ok(BrowserCommand::TypeText { | ||
| selector: required_str(arguments, "selector")?.to_string(), | ||
| text: required_str(arguments, "text")?.to_string(), | ||
| }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve browser_type text exactly.
required_str() trims every string argument, so browser_type silently changes text like " hello " to "hello" and rejects whitespace-only input.
🐛 Proposed fix
"browser_type" => Ok(BrowserCommand::TypeText {
selector: required_str(arguments, "selector")?.to_string(),
- text: required_str(arguments, "text")?.to_string(),
+ text: required_raw_str(arguments, "text")?.to_string(),
}), fn required_str<'a>(arguments: &'a Value, name: &str) -> Result<&'a str> {
arguments
.get(name)
.and_then(Value::as_str)
.map(str::trim)
.filter(|value| !value.is_empty())
.ok_or_else(|| anyhow!("argument `{name}` is required"))
}
+
+fn required_raw_str<'a>(arguments: &'a Value, name: &str) -> Result<&'a str> {
+ arguments
+ .get(name)
+ .and_then(Value::as_str)
+ .filter(|value| !value.is_empty())
+ .ok_or_else(|| anyhow!("argument `{name}` is required"))
+}Also applies to: 152-158
🤖 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/browser_actions.rs` around lines 87 - 90, The `browser_type` handling in
`BrowserCommand::TypeText` is trimming user text via `required_str()`, which
changes the exact content and rejects whitespace-only input; switch this command
to read the raw string argument without trimming so the text is preserved
exactly. Apply the same fix anywhere else in the browser action parsing path
that uses the same text-input logic, including the other referenced
`browser_type` cases.
2 participants
