Support external browser containers by connecting via CDP#152
Open
hotzen wants to merge 3 commits intospacedriveapp:mainfrom
Open
Support external browser containers by connecting via CDP#152hotzen wants to merge 3 commits intospacedriveapp:mainfrom
hotzen wants to merge 3 commits intospacedriveapp:mainfrom
Conversation
Member
|
Woah! Love this |
hotzen
force-pushed
the
external-browser
branch
2 times, most recently
from
e1a2929 to
85ff033
Compare
Contributor
Author
|
next: testing |
hotzen
changed the title
hotzen
force-pushed
the
external-browser
branch
from
5cbd34c to
b190811
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds external-browser support: a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
generally works. issues with screenshots vs. docker though, need to fix: |
hotzen
force-pushed
the
external-browser
branch
4 times, most recently
from
644a3fc to
f93a192
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/docker.md (1)
152-249: Clear and comprehensive external browser documentation.The documentation covers the key deployment scenarios well: host+Docker, both-in-Docker with shared networks, and per-agent dedicated sandboxes. The
connect_urlformat options and lifecycle notes are helpful.One consideration: the PR comments mention an issue with screenshot file access ("ACCESS DENIED: Path is outside the workspace") when using external Docker browsers. If this is a known limitation, it may be worth documenting that screenshot paths need to be in a shared/mounted volume accessible to both containers, or noting any workspace configuration required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docker.md` around lines 152 - 249, Add a short note under the "External Browser" / "Per-agent dedicated sandboxes" section explaining the documented "ACCESS DENIED: Path is outside the workspace" issue: state that screenshots and any file paths referenced by the browser container must live in a volume or bind mount shared between the Spacebot container/process and the chromedp/headless-shell container, and that workspace/config paths must be inside that shared volume; mention this applies when using external browsers via connect_url and when running chromedp/headless-shell in Docker so users know to mount the same volume for both containers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config.rs`:
- Line 4848: When merging connect_url defaults/overrides (the expression using
b.connect_url.or_else(|| base.connect_url.clone())), normalize both
b.connect_url and base.connect_url by trimming whitespace and treating empty
strings as None before merging so that empty/" " values don't win over a valid
fallback; update the merge logic to map and filter each Option<String> (or
equivalent type) to None if trimmed().is_empty() and only then perform
or_else/or logic for connect_url (apply same normalization where connect_url is
handled at the other location referenced by lines ~5046-5048).
- Around line 5837-5852: The tracing warnings print the raw external URL via the
local `url` bound from `config.connect_url`, which may leak credentials; update
the two `tracing::warn!` calls that reference `connect_url = url` to instead log
a redacted or safe representation (e.g., host and path without userinfo/query,
or a masked query/userinfo) by deriving a safe string from `config.connect_url`
before the warns (create a small helper or inline logic to parse and strip
userinfo/query) and use that redacted value in both `tracing::warn!` invocations
so no raw tokens are emitted.
- Around line 918-919: BrowserConfig's derived Debug can leak credentials via
the connect_url field; implement a custom Debug for the BrowserConfig struct (or
manually implement Debug for the enclosing Config/DefaultsConfig if they format
BrowserConfig directly) that prints all fields as before but replaces
connect_url's value with a redacted placeholder (e.g., Some("<redacted>") or
"<redacted>"/None) so the real URL is never emitted; locate the BrowserConfig
struct and its connect_url field and replace the derive(Debug) behavior with
this custom Debug implementation.
---
Nitpick comments:
In `@docs/docker.md`:
- Around line 152-249: Add a short note under the "External Browser" /
"Per-agent dedicated sandboxes" section explaining the documented "ACCESS
DENIED: Path is outside the workspace" issue: state that screenshots and any
file paths referenced by the browser container must live in a volume or bind
mount shared between the Spacebot container/process and the
chromedp/headless-shell container, and that workspace/config paths must be
inside that shared volume; mention this applies when using external browsers via
connect_url and when running chromedp/headless-shell in Docker so users know to
mount the same volume for both containers.
| .map(PathBuf::from) | ||
| .or_else(|| base.screenshot_dir.clone()), | ||
| chrome_cache_dir: chrome_cache_dir.clone(), | ||
| connect_url: b.connect_url.or_else(|| base.connect_url.clone()), |
Contributor
There was a problem hiding this comment.
Normalize connect_url before merging defaults/overrides.
Line 4848 and Line 5046-5048 preserve empty/whitespace values as Some, which can force a failing external-connect path instead of a clean fallback.
🧹 Proposed fix
+fn normalize_connect_url(value: Option<String>) -> Option<String> {
+ value
+ .map(|url| url.trim().to_string())
+ .filter(|url| !url.is_empty())
+}
@@
- connect_url: b.connect_url.or_else(|| base.connect_url.clone()),
+ connect_url: normalize_connect_url(b.connect_url)
+ .or_else(|| base.connect_url.clone()),
@@
- connect_url: b
- .connect_url
- .or_else(|| defaults.browser.connect_url.clone()),
+ connect_url: normalize_connect_url(b.connect_url)
+ .or_else(|| defaults.browser.connect_url.clone()),Also applies to: 5046-5048
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config.rs` at line 4848, When merging connect_url defaults/overrides (the
expression using b.connect_url.or_else(|| base.connect_url.clone())), normalize
both b.connect_url and base.connect_url by trimming whitespace and treating
empty strings as None before merging so that empty/" " values don't win over a
valid fallback; update the merge logic to map and filter each Option<String> (or
equivalent type) to None if trimmed().is_empty() and only then perform
or_else/or logic for connect_url (apply same normalization where connect_url is
handled at the other location referenced by lines ~5046-5048).
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
|
|
||
| // Guard against a concurrent launch that won the race. | ||
| if state.browser.is_some() { | ||
| drop(browser); |
Contributor
There was a problem hiding this comment.
If the external CDP connection drops, launch can still short-circuit on state.browser.is_some(), which makes the recovery message ("reconnect with launch") hard to follow. Consider treating connected && !connection_alive as "not running" and resetting state before returning success.
Suggested change
| drop(browser); | |
| { | |
| let mut state = self.state.lock().await; | |
| if state.browser.is_some() { | |
| if state.connected && !state.connection_alive.load(Ordering::Acquire) { | |
| self.reset_state(&mut state).await; | |
| } else { | |
| return Ok(BrowserOutput::success("Browser already running")); | |
| } | |
| } | |
| } |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/config.rs (3)
918-919:⚠️ Potential issue | 🟠 MajorRedact
connect_urlfromDebugoutput.
BrowserConfigstill derivesDebug, so credential-bearing CDP URLs can leak via config/runtime debug logs.🔐 Proposed fix
-#[derive(Debug, Clone)] +#[derive(Clone)] pub struct BrowserConfig { @@ pub chrome_cache_dir: PathBuf, } + +impl std::fmt::Debug for BrowserConfig { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter + .debug_struct("BrowserConfig") + .field("enabled", &self.enabled) + .field("headless", &self.headless) + .field("evaluate_enabled", &self.evaluate_enabled) + .field("executable_path", &self.executable_path) + .field("screenshot_dir", &self.screenshot_dir) + .field("connect_url", &self.connect_url.as_ref().map(|_| "[REDACTED]")) + .field("chrome_cache_dir", &self.chrome_cache_dir) + .finish() + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 918 - 919, BrowserConfig's derived Debug can leak credentials via the connect_url field; implement a custom Debug for BrowserConfig (or manually implement fmt::Debug) that prints all fields normally but replaces the connect_url value with a redacted marker (e.g., "<redacted>" or None) so the Option<String> connect_url is never shown; locate the BrowserConfig struct and change its Debug impl to redact the connect_url field instead of deriving Debug.
4848-4848:⚠️ Potential issue | 🟡 MinorNormalize
connect_urlbefore fallback merging.Whitespace/empty values currently remain
Some(""), which can suppress a valid inherited URL and force a failing external-connect path.🧹 Proposed fix
+fn normalize_connect_url(value: Option<String>) -> Option<String> { + value + .map(|connect_url| connect_url.trim().to_string()) + .filter(|connect_url| !connect_url.is_empty()) +} @@ - connect_url: b.connect_url.or_else(|| base.connect_url.clone()), + connect_url: normalize_connect_url(b.connect_url) + .or_else(|| normalize_connect_url(base.connect_url.clone())), @@ - connect_url: b - .connect_url - .or_else(|| defaults.browser.connect_url.clone()), + connect_url: normalize_connect_url(b.connect_url) + .or_else(|| { + normalize_connect_url(defaults.browser.connect_url.clone()) + }),Also applies to: 5046-5048
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` at line 4848, The connect_url merge currently uses b.connect_url.or_else(|| base.connect_url.clone()) which preserves Some("") and prevents inheriting base values; normalize both b.connect_url and base.connect_url by trimming and treating empty/whitespace-only strings as None before performing the or_else merge (apply the same normalization where connect_url is merged at the other location around lines 5046-5048); update the merge expression around the connect_url field (referencing b, base, and connect_url) to first map/and_then through a trim-and-empty-check so only non-empty strings are considered Some when falling back to base.
5837-5852:⚠️ Potential issue | 🟠 MajorDo not log raw
connect_urlvalues.
tracing::warn!currently emits the raw external URL, which can expose credentials/tokens in logs.🔒 Proposed fix
fn warn_browser_config(context: &str, config: &BrowserConfig) { - let Some(url) = config.connect_url.as_deref().filter(|u| !u.is_empty()) else { + let Some(_url) = config.connect_url.as_deref().filter(|u| !u.is_empty()) else { return; }; if config.executable_path.is_some() { tracing::warn!( context, - connect_url = url, "connect_url is set; executable_path has no effect" ); } if !config.headless { tracing::warn!( context, - connect_url = url, "connect_url is set; headless flag has no effect" ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 5837 - 5852, The warnings currently pass the raw connect_url into tracing::warn! (see uses of config.connect_url / url and the tracing::warn! calls for executable_path and headless), which can leak sensitive tokens; change these logs to avoid emitting the full URL by either removing the connect_url field from the structured context or replacing it with a redacted value (e.g., "REDACTED" or a masked/boolean indicator like connect_url_set = true) and update the two tracing::warn! invocations accordingly so they only indicate that a connect URL is present without logging the raw url string.
🧹 Nitpick comments (2)
docs/docker.md (2)
220-232: Consider briefly explainingshm_sizevalues.The example uses different
shm_sizevalues (512mb for browser-main, 1gb for browser-research) without explanation. A brief comment about how to size shared memory for browser containers would help users optimize their deployments (e.g., based on expected tab count, memory-intensive operations, etc.).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docker.md` around lines 220 - 232, Add a short explanatory note near the browser container examples describing what shm_size controls and guidance for choosing values: explain that shm_size sets the container's /dev/shm (shared memory) which affects headless browser stability and performance, recommend sizing based on expected concurrent tabs and memory-heavy operations (e.g., 512mb for lightweight single-tab use, 1gb+ for multiple tabs or heavy rendering), and suggest monitoring and increasing if you observe crashes or OOMs; reference the shown services browser-main and browser-research so readers understand why different values are used.
152-249: Consider adding troubleshooting section for volume mounting issues.According to the PR comments, there's a reported issue with screenshots when using Docker: "ACCESS DENIED: Path is outside the workspace" for screenshot file paths. This suggests potential volume mounting or path resolution problems when using external browser containers that users are likely to encounter.
Consider adding a troubleshooting subsection addressing common issues like:
- Screenshot/download path resolution when browser runs in a separate container
- Volume mounting requirements for shared file access between containers
- Workspace path configuration for external browsers
This would help users avoid the issue that was encountered during testing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docker.md` around lines 152 - 249, Add a short "Troubleshooting: volume & path issues" subsection under the "External Browser" section explaining how screenshot/download paths must be accessible to both the Spacebot process and the browser container, how to set connect_url correctly for container-to-container routing, and common fixes: bind-mount the same host volume into both containers (so screenshot paths are not "outside the workspace"), ensure workspace path configuration matches the mounted path inside the browser, and verify the browser container can reach /json/version or the direct WebSocket URL; mention the "ACCESS DENIED: Path is outside the workspace" symptom and suggest validating mounts and permissions as the first debugging step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docker.md`:
- Around line 161-189: Add a security note immediately after the docker-compose
ports example (the block showing ports: - "127.0.0.1:9222:9222") warning that
the Chrome DevTools Protocol (port 9222) provides full browser control without
authentication and must not be exposed publicly (e.g., avoid 0.0.0.0:9222:9222);
recommend binding to 127.0.0.1 or using Docker networks/container-to-container
communication and mention the connect_url = "http://localhost:9222" config as an
example of localhost-only usage.
---
Duplicate comments:
In `@src/config.rs`:
- Around line 918-919: BrowserConfig's derived Debug can leak credentials via
the connect_url field; implement a custom Debug for BrowserConfig (or manually
implement fmt::Debug) that prints all fields normally but replaces the
connect_url value with a redacted marker (e.g., "<redacted>" or None) so the
Option<String> connect_url is never shown; locate the BrowserConfig struct and
change its Debug impl to redact the connect_url field instead of deriving Debug.
- Line 4848: The connect_url merge currently uses b.connect_url.or_else(||
base.connect_url.clone()) which preserves Some("") and prevents inheriting base
values; normalize both b.connect_url and base.connect_url by trimming and
treating empty/whitespace-only strings as None before performing the or_else
merge (apply the same normalization where connect_url is merged at the other
location around lines 5046-5048); update the merge expression around the
connect_url field (referencing b, base, and connect_url) to first map/and_then
through a trim-and-empty-check so only non-empty strings are considered Some
when falling back to base.
- Around line 5837-5852: The warnings currently pass the raw connect_url into
tracing::warn! (see uses of config.connect_url / url and the tracing::warn!
calls for executable_path and headless), which can leak sensitive tokens; change
these logs to avoid emitting the full URL by either removing the connect_url
field from the structured context or replacing it with a redacted value (e.g.,
"REDACTED" or a masked/boolean indicator like connect_url_set = true) and update
the two tracing::warn! invocations accordingly so they only indicate that a
connect URL is present without logging the raw url string.
---
Nitpick comments:
In `@docs/docker.md`:
- Around line 220-232: Add a short explanatory note near the browser container
examples describing what shm_size controls and guidance for choosing values:
explain that shm_size sets the container's /dev/shm (shared memory) which
affects headless browser stability and performance, recommend sizing based on
expected concurrent tabs and memory-heavy operations (e.g., 512mb for
lightweight single-tab use, 1gb+ for multiple tabs or heavy rendering), and
suggest monitoring and increasing if you observe crashes or OOMs; reference the
shown services browser-main and browser-research so readers understand why
different values are used.
- Around line 152-249: Add a short "Troubleshooting: volume & path issues"
subsection under the "External Browser" section explaining how
screenshot/download paths must be accessible to both the Spacebot process and
the browser container, how to set connect_url correctly for
container-to-container routing, and common fixes: bind-mount the same host
volume into both containers (so screenshot paths are not "outside the
workspace"), ensure workspace path configuration matches the mounted path inside
the browser, and verify the browser container can reach /json/version or the
direct WebSocket URL; mention the "ACCESS DENIED: Path is outside the workspace"
symptom and suggest validating mounts and permissions as the first debugging
step.
Comment on lines
+158
to
+160
| Workers spawned by the same agent share one Chrome process (each gets its own tab). A | ||
| Workers pointing at the same `connect_url` share one Chrome process (each gets its own tab). A | ||
| Chrome crash kills all tabs connected to that browser. |
Contributor
There was a problem hiding this comment.
Looks like a paste/duplication here (extra leading A + repeated sentence). Suggest:
Suggested change
| Workers spawned by the same agent share one Chrome process (each gets its own tab). A | |
| Workers pointing at the same `connect_url` share one Chrome process (each gets its own tab). A | |
| Chrome crash kills all tabs connected to that browser. | |
| Workers pointing at the same `connect_url` share one Chrome process (each gets its own tab). A Chrome crash kills all tabs connected to that browser. |
Comment on lines
+5840
to
+5853
| if config.executable_path.is_some() { | ||
| tracing::warn!( | ||
| context, | ||
| connect_url = url, | ||
| "connect_url is set; executable_path has no effect" | ||
| ); | ||
| } | ||
| if !config.headless { | ||
| tracing::warn!( | ||
| context, | ||
| connect_url = url, | ||
| "connect_url is set; headless flag has no effect" | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Minor nit: logging the full connect_url here can leak credentials if someone uses ws://user:pass@... or query tokens. I'd rather log a boolean (or a redacted host).
Suggested change
| if config.executable_path.is_some() { | |
| tracing::warn!( | |
| context, | |
| connect_url = url, | |
| "connect_url is set; executable_path has no effect" | |
| ); | |
| } | |
| if !config.headless { | |
| tracing::warn!( | |
| context, | |
| connect_url = url, | |
| "connect_url is set; headless flag has no effect" | |
| ); | |
| } | |
| if config.executable_path.is_some() { | |
| tracing::warn!( | |
| context, | |
| connect_url_set = true, | |
| "connect_url is set; executable_path has no effect" | |
| ); | |
| } | |
| if !config.headless { | |
| tracing::warn!( | |
| context, | |
| connect_url_set = true, | |
| "connect_url is set; headless flag has no effect" | |
| ); | |
| } |
|
|
||
| tracing::info!(connect_url, "connecting to external browser"); | ||
|
|
||
| let (browser, mut handler) = Browser::connect(connect_url).await.map_err(|error| { |
Contributor
There was a problem hiding this comment.
Docs mention allowing http://host:9222 by auto-discovering the WebSocket URL via /json/version. If chromiumoxide::Browser::connect doesn't already do that, it might be worth resolving webSocketDebuggerUrl here (or tightening the docs to only claim ws://... support).
Comment on lines
+1144
to
+1147
| if state.connected && !state.connection_alive.load(Ordering::Acquire) { | ||
| return Err(BrowserError::new( | ||
| "external browser connection lost — reconnect with launch", | ||
| )); |
Contributor
There was a problem hiding this comment.
Minor: right now launch will still report "already running" if the external WebSocket dropped, so the "reconnect with launch" instruction is a bit misleading. Maybe point folks at close + launch (and keep the message consistent in require_active_page too).
Suggested change
| if state.connected && !state.connection_alive.load(Ordering::Acquire) { | |
| return Err(BrowserError::new( | |
| "external browser connection lost — reconnect with launch", | |
| )); | |
| if state.connected && !state.connection_alive.load(Ordering::Acquire) { | |
| return Err(BrowserError::new( | |
| "external browser connection lost — call close, then launch to reconnect", | |
| )); | |
| } |
Co-authored-by: tembo[bot] <208362400+tembo[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tools/browser.rs (1)
615-621:⚠️ Potential issue | 🟡 MinorDon’t silently ignore temp-dir cleanup errors on launch race.
let _ = std::fs::remove_dir_all(&user_data_dir);drops a non-channel error silently.Suggested fix
- let _ = std::fs::remove_dir_all(&user_data_dir); + if let Err(error) = std::fs::remove_dir_all(&user_data_dir) { + tracing::debug!( + path = %user_data_dir.display(), + %error, + "failed to clean up raced browser user data dir" + ); + }As per coding guidelines: "Don't silently discard errors; use
let _ =only on channel sends where the receiver may be dropped; handle, log, or propagate all other errors".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 615 - 621, The temp-dir cleanup error is being silently ignored in the early-return branch when state.browser.is_some() — replace the `let _ = std::fs::remove_dir_all(&user_data_dir);` idiom with explicit error handling: attempt to remove_dir_all(user_data_dir) and on Err(e) log a warning or error (e.g., via tracing::warn! or process logger) including the user_data_dir and the error; keep the existing drop(browser), handler_task.abort(), and return of BrowserOutput::success("Browser already running") otherwise. Ensure the change touches the same block that references state.browser, user_data_dir, handler_task.abort(), and BrowserOutput::success.
♻️ Duplicate comments (5)
src/config.rs (1)
4848-4848:⚠️ Potential issue | 🟡 MinorNormalize
connect_urlbefore merge and warning checks.Line 4848 and Line 5046 currently keep whitespace-only values as
Some(...), which can force an invalid external-connect path instead of a clean fallback. Line 5837 has the same issue for warning gating.🧹 Proposed fix
+fn normalize_connect_url(value: Option<String>) -> Option<String> { + value + .map(|url| url.trim().to_string()) + .filter(|url| !url.is_empty()) +} @@ - connect_url: b.connect_url.or_else(|| base.connect_url.clone()), + connect_url: normalize_connect_url(b.connect_url) + .or_else(|| normalize_connect_url(base.connect_url.clone())), @@ - connect_url: b - .connect_url - .or_else(|| defaults.browser.connect_url.clone()), + connect_url: normalize_connect_url(b.connect_url) + .or_else(|| normalize_connect_url(defaults.browser.connect_url.clone())), @@ - let Some(url) = config.connect_url.as_deref().filter(|u| !u.is_empty()) else { + let Some(url) = config + .connect_url + .as_deref() + .map(str::trim) + .filter(|url| !url.is_empty()) + else { return; };Also applies to: 5046-5048, 5837-5838
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` at line 4848, Normalize connect_url by trimming and converting whitespace-only strings to None before merging and before the warning checks: replace uses like connect_url: b.connect_url.or_else(|| base.connect_url.clone()) with a normalized value (e.g., map the Option<String> through trim-and-empty->None) so that whitespace-only Some("") does not override a valid fallback, and apply the same normalization where warning gating reads connect_url; operate on the Option<String> (connect_url) values rather than downstream logic to ensure both merge and warning checks receive cleaned Options.docs/docker.md (2)
158-160:⚠️ Potential issue | 🟡 MinorRemove the duplicated sentence in the worker-sharing note.
This paragraph currently repeats itself and introduces conflicting phrasing (
same agentvssame connect_url). Keep only theconnect_url-based wording.Suggested doc fix
-Workers spawned by the same agent share one Chrome process (each gets its own tab). A Workers pointing at the same `connect_url` share one Chrome process (each gets its own tab). A Chrome crash kills all tabs connected to that browser.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docker.md` around lines 158 - 160, Remove the duplicated sentence in the worker-sharing note and keep only the connect_url phrasing: delete the line "Workers spawned by the same agent share one Chrome process (each gets its own tab)." so the paragraph reads with the single sentence "Workers pointing at the same `connect_url` share one Chrome process (each gets its own tab). A Chrome crash kills all tabs connected to that browser.".
167-190:⚠️ Potential issue | 🟠 MajorAdd an explicit CDP security warning after the host-port example.
The example is safe (
127.0.0.1), but readers still need a hard warning that CDP has no auth and must never be publicly exposed.Suggested doc note
services: browser: image: chromedp/headless-shell:latest @@ restart: unless-stopped
+> Security: CDP on port
9222provides full browser control and has no authentication.
+> Never expose it publicly (e.g., avoid0.0.0.0:9222:9222).
+> Use localhost binding (127.0.0.1) or private container networks.Test whether the browser is reachable from the host:
</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/docker.mdaround lines 167 - 190, Add an explicit security warning
immediately after the host-port curl example in the browser docker-compose
snippet: insert a short note saying that CDP on port 9222 provides full browser
control, has no authentication, must never be exposed publicly (avoid
0.0.0.0:9222:9222), and recommend using localhost binding (127.0.0.1) or private
container networks; place this warning directly after the curl test and before
the config toml section so readers see it before configuring connect_url.</details> </blockquote></details> <details> <summary>src/tools/browser.rs (2)</summary><blockquote> `539-543`: _⚠️ Potential issue_ | _🟠 Major_ **Verify `Browser::connect` supports the documented `http://host:9222` format.** Docs claim HTTP endpoint auto-discovery, but this code passes `connect_url` directly to `Browser::connect`. Please confirm the crate behavior for the pinned version and align docs/implementation if needed. ```web For the chromiumoxide version used by this repository, does `Browser::connect(...)` accept an HTTP DevTools endpoint like `http://localhost:9222` (auto-discovering `webSocketDebuggerUrl`), or does it require a `ws://.../devtools/browser/<id>` URL?
511-531:⚠️ Potential issue | 🟠 Major
launchstill can’t recover from a dropped external connection.If the external WebSocket drops,
state.browsermay remain set, solaunchreturns “already running” instead of reconnecting. This conflicts with the recovery hint added elsewhere.Suggested fix
- { - let state = self.state.lock().await; - if state.browser.is_some() { - return Ok(BrowserOutput::success("Browser already running")); - } - } + { + let mut state = self.state.lock().await; + if state.browser.is_some() { + if state.connected && !state.connection_alive.load(Ordering::Acquire) { + self.reset_state(&mut state).await; + } else { + return Ok(BrowserOutput::success("Browser already running")); + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 511 - 531, The early return in handle_launch wrongly assumes state.browser being Some means a healthy connection; instead, before returning "Browser already running" check the actual connection liveness and clear/reset state.browser if the external WebSocket has dropped so connect_external can retry. Concretely: in handle_launch, after acquiring state via self.state.lock().await and finding state.browser.is_some(), call the browser liveness check (or expose one on the browser handle, e.g., an is_connected/is_alive method or attempt a lightweight ping) and if it reports disconnected, set state.browser = None and drop the lock so the function proceeds to call self.connect_external().await; otherwise keep the existing early Ok(BrowserOutput::success("Browser already running")). Ensure connect_external and launch_local behavior is unchanged.
🧹 Nitpick comments (1)
src/config.rs (1)
5574-5580: Add smoke tests forconnect_urlmerge edge cases.A compact config smoke test here would lock in
agent > defaultsbehavior and empty/whitespace handling to prevent regressions.Based on learnings: In
src/config.rstests, prefer smoke tests that do not panic and avoid adding test-only dependencies solely to assert tracing warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/tools/browser.rs`:
- Around line 615-621: The temp-dir cleanup error is being silently ignored in
the early-return branch when state.browser.is_some() — replace the `let _ =
std::fs::remove_dir_all(&user_data_dir);` idiom with explicit error handling:
attempt to remove_dir_all(user_data_dir) and on Err(e) log a warning or error
(e.g., via tracing::warn! or process logger) including the user_data_dir and the
error; keep the existing drop(browser), handler_task.abort(), and return of
BrowserOutput::success("Browser already running") otherwise. Ensure the change
touches the same block that references state.browser, user_data_dir,
handler_task.abort(), and BrowserOutput::success.
---
Duplicate comments:
In `@docs/docker.md`:
- Around line 158-160: Remove the duplicated sentence in the worker-sharing note
and keep only the connect_url phrasing: delete the line "Workers spawned by the
same agent share one Chrome process (each gets its own tab)." so the paragraph
reads with the single sentence "Workers pointing at the same `connect_url` share
one Chrome process (each gets its own tab). A Chrome crash kills all tabs
connected to that browser.".
- Around line 167-190: Add an explicit security warning immediately after the
host-port curl example in the browser docker-compose snippet: insert a short
note saying that CDP on port 9222 provides full browser control, has no
authentication, must never be exposed publicly (avoid 0.0.0.0:9222:9222), and
recommend using localhost binding (127.0.0.1) or private container networks;
place this warning directly after the curl test and before the config toml
section so readers see it before configuring connect_url.
In `@src/config.rs`:
- Line 4848: Normalize connect_url by trimming and converting whitespace-only
strings to None before merging and before the warning checks: replace uses like
connect_url: b.connect_url.or_else(|| base.connect_url.clone()) with a
normalized value (e.g., map the Option<String> through trim-and-empty->None) so
that whitespace-only Some("") does not override a valid fallback, and apply the
same normalization where warning gating reads connect_url; operate on the
Option<String> (connect_url) values rather than downstream logic to ensure both
merge and warning checks receive cleaned Options.
In `@src/tools/browser.rs`:
- Around line 511-531: The early return in handle_launch wrongly assumes
state.browser being Some means a healthy connection; instead, before returning
"Browser already running" check the actual connection liveness and clear/reset
state.browser if the external WebSocket has dropped so connect_external can
retry. Concretely: in handle_launch, after acquiring state via
self.state.lock().await and finding state.browser.is_some(), call the browser
liveness check (or expose one on the browser handle, e.g., an
is_connected/is_alive method or attempt a lightweight ping) and if it reports
disconnected, set state.browser = None and drop the lock so the function
proceeds to call self.connect_external().await; otherwise keep the existing
early Ok(BrowserOutput::success("Browser already running")). Ensure
connect_external and launch_local behavior is unchanged.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/config.rs (1)
4903-4903:⚠️ Potential issue | 🟡 MinorNormalize
connect_url(trim + empty-to-None) before merge and warning checks.Whitespace-only values currently survive as
Some, so they can override valid fallbacks and force an invalid external-connect path.🧹 Proposed fix
+fn normalize_connect_url(value: Option<String>) -> Option<String> { + value + .map(|url| url.trim().to_string()) + .filter(|url| !url.is_empty()) +} @@ - connect_url: b.connect_url.or_else(|| base.connect_url.clone()), + connect_url: normalize_connect_url(b.connect_url) + .or_else(|| normalize_connect_url(base.connect_url.clone())), @@ - connect_url: b - .connect_url - .or_else(|| defaults.browser.connect_url.clone()), + connect_url: normalize_connect_url(b.connect_url).or_else(|| { + normalize_connect_url(defaults.browser.connect_url.clone()) + }), @@ - let Some(url) = config.connect_url.as_deref().filter(|u| !u.is_empty()) else { + let Some(url) = config + .connect_url + .as_deref() + .map(str::trim) + .filter(|url| !url.is_empty()) + else { return; };Also applies to: 5101-5103, 5912-5913
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` at line 4903, Normalize the connect_url by trimming whitespace and converting empty strings to None before any merge or warning logic: ensure the temporary/parsed value (e.g., b.connect_url) is replaced with a normalized Option (trimmed string -> Some only if non-empty) and then use that normalized value in the merge expression (the line building connect_url that uses b.connect_url.or_else(|| base.connect_url.clone())) and in the other two similar merge sites; apply the same normalization wherever connect_url is inspected for warnings so whitespace-only values cannot override base.connect_url.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/config.rs`:
- Line 4903: Normalize the connect_url by trimming whitespace and converting
empty strings to None before any merge or warning logic: ensure the
temporary/parsed value (e.g., b.connect_url) is replaced with a normalized
Option (trimmed string -> Some only if non-empty) and then use that normalized
value in the merge expression (the line building connect_url that uses
b.connect_url.or_else(|| base.connect_url.clone())) and in the other two similar
merge sites; apply the same normalization wherever connect_url is inspected for
warnings so whitespace-only values cannot override base.connect_url.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add connect_url to BrowserConfig so agents can connect to an external chromedp/headless-shell container via CDP instead of launching a local Chrome process.
Config (src/config.rs)
Docs (docs/docker.md)