fix: address reviewer comments from PR #315 (snap detection + README)#315
Open
Ayush-Mahadik wants to merge 2 commits intobrowser-use:mainfrom
Open
fix: address reviewer comments from PR #315 (snap detection + README)#315Ayush-Mahadik wants to merge 2 commits intobrowser-use:mainfrom
Ayush-Mahadik wants to merge 2 commits intobrowser-use:mainfrom
Conversation
- README: add Prerequisites section with macOS/Linux Chrome launch commands and a note about Snap Chromium CDP limitations (fixes browser-use#123) - admin.py: add _snap_browser_detected() checking SNAP env var and /snap/ paths in /proc/cmdline; surface in run_doctor() as a FAIL row when snap Chromium is detected (fixes browser-use#191)
There was a problem hiding this comment.
Pull request overview
This PR improves first-time setup and diagnostics for local Chrome connections by documenting the CDP remote-debugging prerequisite and adding a --doctor check for Snap-confined Chromium environments on Linux.
Changes:
- Add a new Prerequisites section to the README describing how to start Chrome with remote debugging and a note about Snap Chromium limitations.
- Introduce
_snap_browser_detected()and surface its result as a newrun_doctor()row (“snap confinement”). - Extend
--doctoroutput to recommend installing non-snap Chrome when Snap confinement is detected.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| README.md | Documents CDP remote-debugging startup and Snap Chromium caveats before the setup prompt. |
| src/browser_harness/admin.py | Adds Snap Chromium detection and reports it in browser-harness --doctor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+674
to
+686
| if os.environ.get("SNAP"): | ||
| return True | ||
| try: | ||
| # Walk /proc looking for chromium/chrome processes with a snap mount | ||
| for pid_dir in Path("/proc").iterdir(): | ||
| if not pid_dir.name.isdigit(): | ||
| continue | ||
| try: | ||
| cmdline = Path(pid_dir / "cmdline").read_text() | ||
| except (FileNotFoundError, PermissionError, OSError): | ||
| continue | ||
| if "/snap/chromium/" in cmdline or "/snap/google-chrome/" in cmdline: | ||
| return True |
Comment on lines
+676
to
+688
| try: | ||
| # Walk /proc looking for chromium/chrome processes with a snap mount | ||
| for pid_dir in Path("/proc").iterdir(): | ||
| if not pid_dir.name.isdigit(): | ||
| continue | ||
| try: | ||
| cmdline = Path(pid_dir / "cmdline").read_text() | ||
| except (FileNotFoundError, PermissionError, OSError): | ||
| continue | ||
| if "/snap/chromium/" in cmdline or "/snap/google-chrome/" in cmdline: | ||
| return True | ||
| except Exception: | ||
| pass |
| cur = _version() | ||
| mode = _install_mode() | ||
| chrome = _chrome_running() | ||
| snap = _snap_browser_detected() |
Comment on lines
+23
to
+33
| Before running browser-harness, Chrome must be running with remote debugging enabled. The harness connects to Chrome via CDP — it will not launch Chrome itself. | ||
|
|
||
| **macOS** | ||
| ```bash | ||
| open -a 'Google Chrome' --args --remote-debugging-port=9222 | ||
| ``` | ||
|
|
||
| **Linux** | ||
| ```bash | ||
| google-chrome --remote-debugging-port=9222 | ||
| ``` |
Comment on lines
+35
to
+36
| Then open `chrome://inspect/#remote-debugging` and enable **Discover network targets** in the Devices tab. | ||
|
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/browser_harness/admin.py">
<violation number="1" location="src/browser_harness/admin.py:674">
P2: This early `SNAP` check misidentifies the harness process as a snap browser and can produce a false FAIL in doctor output.</violation>
<violation number="2" location="src/browser_harness/admin.py:683">
P2: `Path.read_text()` on `/proc/<pid>/cmdline` can raise `UnicodeDecodeError` (a `ValueError`, not `OSError`) if a process has non-UTF-8 bytes in its command line. Since only `FileNotFoundError | PermissionError | OSError` are caught per-PID, the error propagates to the outer `try/except Exception` which aborts the entire scan and returns `False`. Read as bytes instead (`read_bytes()`) or add `ValueError` to the inner except clause.</violation>
<violation number="3" location="src/browser_harness/admin.py:722">
P2: Snap confinement is reported as FAIL but is not included in the doctor exit status, so `--doctor` can exit 0 while signaling an unhealthy browser setup.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
- Fix false positive SNAP env var check in _snap_browser_detected() (SNAP env var identifies the harness process, not the browser) - Add ValueError to exception handling for UnicodeDecodeError on /proc cmdline - Short-circuit snap detection: only scan /proc when chrome is running - Include snap confinement in doctor exit status - Rewrite Prerequisites section: align with install.md (Way 1/Way 2) - Fix Chrome UI toggle: 'Discover network targets' -> 'Allow remote debugging for this browser instance' (matches repo's own docs) - Move snap note to doctor output, update README to point to install.md
Ayush-Mahadik
changed the title
Author
|
Hi @Alezander9 👋 Just pushed the fixes addressing all reviewer comments from Copilot and cubic-dev-ai. Here's what was changed:
Could you please take another look when you get a chance? Happy to make any further adjustments. Thanks! |
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two fixes bundled into one PR:
1. README: Add Chrome Remote Debugging Prerequisite (fixes #123)
New Prerequisites section before the Setup prompt with:
2. admin.py: Add Snap Confinement Detection (fixes #191)
New _snap_browser_detected() helper that checks:
Surfaced as a FAIL row in run_doctor() with:
Files Changed
Tests
63/64 unit tests pass. The single failure (test_cloud_bootstrap_on_headless_server) is pre-existing and unrelated to these changes.
Summary by cubic
Adds a clear Chrome remote‑debugging Prerequisites section (two ways to connect) and updates the doctor to detect and fail on Snap‑confined Chromium, reducing Linux setup issues.
chrome://inspect/#remote-debugging(handles Chrome 144+ allow prompt), or (Way 2) launch with--remote-debugging-port=9222and a non‑default--user-data-dir(e.g./tmp/chrome-harness); recommend settingBU_CDP_URL=http://127.0.0.1:9222._snap_browser_detected()(Linux) that scans/proc/*/cmdlinefor/snap/chromium/or/snap/google-chrome/(noSNAPenv var reliance), scans only when Chrome is running, and tolerates non‑UTF‑8 cmdlines; adds a “snap confinement” row and includes it in the exit status with guidance: “snap Chromium cannot bind CDP port — install Chrome from google.com/chrome instead”.Written for commit a7a7b52. Summary will update on new commits.