browser: fix macOS Chrome discovery and timeout=/url= prefix-stacking crash#1
Open
dywongcloud wants to merge 1 commit into
Open
Conversation
… crash Two real, live-hit bugs in the native CDP browser verb: 1. find_chrome() only ever checked /usr/bin/google-chrome+chromium (Linux) or Program Files (Windows), with no macOS app-bundle path -- every browser dispatch on a stock macOS install with Chrome only on PATH via nothing (a normal Chrome.app install adds nothing to PATH) failed with "no Chrome found; install Google Chrome or Chromium". Adds the standard system-wide and per-user /Applications/*.app/Contents/MacOS/* candidates (Chrome + Chromium for parity with the existing Linux chromium fallback), purely additive -- existing Linux/Windows/PATH checks are untouched. 2. Stacking a `timeout=<ms>` prefix line in front of `url=` (e.g. `timeout=30000\nurl=http://x/\n<script>`) crashed instantly with `SyntaxError: Unexpected token ':'`, before any navigation ran. Root cause: `timeout=` was documented as a stackable dispatch prefix but nothing in browser.rs ever recognized it -- strip_mode_prefix only knows capture/profile/trace, and parse_body only recognizes url=/a bare http(s):// URL as literally the body's first token. So the entire remaining text, including the real `url=...` line, fell through to be evaluated as raw JS. `url=http://x/` is invalid as a bare JS statement: it tokenizes as the assignment expression `url = http` (the following `//` is lexed as a line comment) immediately followed by a stray `:` left over from `http:`, producing exactly the reported syntax error. Fixed by giving `timeout=` its own prefix-stripper and looping it together with strip_mode_prefix in run() so mode and timeout compose in either order before parse_body ever sees the url=/script remainder -- `timeout=`+`url=` (and `url=`+mode, mode+`timeout=`, all three stacked in any order) now all resolve identically to using any single prefix alone. Verified with new unit tests covering both bugs, including a real end-to-end test that drives actual Chrome over CDP with the exact previously-crashing stacked-prefix body and confirms correct navigation + eval result (`cargo test -p agentplug-host`, 6/6 passing).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two real, live-hit bugs in the native CDP
browserverb (crates/agentplug-host/src/browser.rs):macOS Chrome discovery missing.
find_chrome()only ever checked/usr/bin/google-chrome+chromium (Linux) orProgram Files(Windows) — no macOS app-bundle path existed, so every browser dispatch on a stock macOS install (Chrome not onPATH, which is normal for aChrome.appinstall) failed withno Chrome found; install Google Chrome or Chromium. Adds the standard system-wide and per-user/Applications/*.app/Contents/MacOS/*candidates (Chrome + Chromium, for parity with the existing Linux chromium fallback) as additional candidates only — existing Linux/Windows/PATH checks are untouched.timeout=+url=prefix stacking crash. Stacking atimeout=<ms>prefix line in front ofurl=(e.g.timeout=30000\nurl=http://x/\n<script>) crashed instantly withSyntaxError: Unexpected token ':', before any navigation ran. Root cause:timeout=was a documented stackable dispatch prefix but nothing inbrowser.rsever recognized it —strip_mode_prefixonly knowscapture/profile/trace, andparse_bodyonly recognizesurl=/a barehttp(s)://URL as literally the body's first token. So the entire remaining text, including the realurl=...line, fell through and was evaluated as raw JS.url=http://x/is invalid as a bare JS statement: it tokenizes as the assignment expressionurl = http(the following//is lexed as a line comment) immediately followed by a stray:left over fromhttp:— exactly the reported syntax error. Fixed by givingtimeout=its own prefix-stripper and looping it together withstrip_mode_prefixinrun()so mode and timeout compose in either order beforeparse_bodyever sees theurl=/script remainder.Test plan
cargo build --release— clean compile, zero warningscrates/agentplug-host/src/browser.rscovering: macOS chrome discovery (real filesystem check),timeout=+url=stacking,url=alone (regression), and both stacking orders of mode+timeout+urltimeout=30000\nurl=...\nreturn 1+1;body and confirms correct navigation + eval resultcargo test -p agentplug-host— 6/6 passing, including the live Chrome test🤖 Generated with Claude Code