Browser stack quick wins: argv hardening, resize/typing/loading perf, iframe failures, agent docs - #774
Conversation
Deploying mouseterm with
|
| Latest commit: |
dd92ba8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://eda94d93.mouseterm.pages.dev |
| Branch Preview URL: | https://browser-quick-wins.mouseterm.pages.dev |
dormouse-bot
left a comment
There was a problem hiding this comment.
This is feedback on a draft, not a merge verdict. Mark the PR ready when you want the full review. I've put the findings inline. The biggest one is a URL-bar regression. normalizeNavUrl in lib/src/components/wall/browser-url.ts still passes file://, about:, data: and view-source: through unchanged, and the controller's navigate sends them as ['open', url]. parseWebviewCommand now refuses those, so typing about:blank or file:///tmp/report.html closes the URL editor and does nothing. The only trace is a console.warn. The host refusal is right. The UI should either show the refusal or stop producing URLs the host won't accept.
The IFRAME_HTTP_ONLY rule in the Iframe Renderer section of docs/specs/dor-browser.md says every entry names dor ab open <url>. Only surface.iframe does. The Display modal shows the reason without the command, the Wall's swap refusal only calls console.warn, and the tool-surface swap returns silently. Also, openContextPort in Wall.tsx (terminal-context port, mode === 'iframe') creates or updates an iframe pane with no iframeRefusal check, so an https port entry falls through to the scheme panel error. That entry isn't in the table either. Either narrow the rule or bring these entries in line with it.
2cd4377 to
8e341fb
Compare
|
Re the body findings on the draft review: B1 (URL-bar regression). The URL editor now refuses any scheme other than http(s). It uses B2 (IFRAME_HTTP_ONLY drift). The table now lists what each entry actually does:
The swap refusal is now one check at the top of Branch rebased onto playwright-browser 28f0aa0; fixes in cce497d, spawn timeout on captures in 8e341fb. |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
dormouse-bot
left a comment
There was a problem hiding this comment.
The fixes for both draft-review findings hold up: the URL editor now refuses visibly, and the entry table matches the code. There are four inline findings, and one more outside the diff:
The Display modal and the swap it triggers read different URLs. AgentBrowserScreenModal decides whether iframe is allowed from chrome.url, but onSwapRenderMode in Wall.tsx checks browserUrlFromParams(params) || chrome().url. Because rememberRestorableUrl now keeps only http(s) URLs, params.url can hold an earlier page than the one on screen. Example: an agent opens https://github.com/x and then file:///tmp/report.html. The modal sees file: and offers iframe; Apply checks the https URL and refuses, and the only trace is a console.warn. When the swap is allowed instead, it frames the earlier page. Having both read the same source (chrome URL first, as the modal does) would keep them in agreement.
8e341fb to
10f4eba
Compare
|
Re the modal/swap URL mismatch in the review body: fixed in 10f4eba (rebased onto playwright-browser 5edc46b). The Display modal and the swap now judge the same URL: the page on screen (the chrome URL) first, then Tests:
|
|
Argos caught a real regression here. Every Production isn't affected.
The full Argos story run passes in Chromium and WebKit (588/588). The branch is also rebased onto playwright-browser 1e16cf1, #773's context-panel fit fix. |
60df23e to
a631b5c
Compare
…line The host's `command()` matched only `args[0]` against a verb allowlist, then ran `--session <session> ...args`. agent-browser reads launch options anywhere on its command line (verified against 0.31.1: `open about:blank --executable-path /x` tries to launch /x), so an allowlisted verb from the webview could carry `--executable-path`, `--args`, `--extension`, `--init-script`, `--profile`, `--state` or `--proxy` past the `binaryPath` gate. The allowlist also passed `close --all`, `tab new <url>`, and `screenshot <path>` (an image written over any user-writable file). `command()` now accepts exactly one argv shape per verb (`WEBVIEW_COMMANDS`): the controller's chrome, tab and Display actions, `get cdp-url`, and a bare `close`. Sessions must not be option- or path-shaped on every entry point (a session names `<socket dir>/<session>.pid`, whose pid a relaunch SIGTERMs), and `open`/pop-out/pop-in launch URLs must be absolute. The webview-side `AGENT_BROWSER_ALLOWED_SUBCOMMANDS` constant is gone; the host owns the table. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe
`onWindowResize` issued `set viewport` on every window `resize` event while sync was engaged — about 60 per second per synced pane during a window or VS Code sash drag, one host spawn each — although the pane's ResizeObserver already delivers the same size change debounced by 200ms. The window listener exists only for display-scale changes, which resize nothing; it now re-syncs only when devicePixelRatio differs from the last issued viewport, and still refreshes the cached pane size and screen snapshot on every event. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe
… input `send()` opened the provisional (stream-paint) window only for `input_mouse`, so each keystroke's echo waited a whole crisp capture round trip (~120ms plus up to ~180ms of loop pacing in a burst) while a hover repainted from the stream in ~50ms. Every input message now opens the window — keys, pasted text replayed as keys, and the host-routed select-all/copy/cut chords — and the crisp capture still sharpens the frame 250ms after the last input. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe
…open `agent-browser open` holds the daemon's queue until the page loads (up to 25s). A crisp capture issued meanwhile waits behind it, and once the pane had a frame the stream only pulsed the loop, so the canvas stayed on the previous page for the whole load. The 8s watchdog then freed the slot and spawned another `screenshot` into the same queue — stacking blocked CLI processes and, on VS Code, racing the session's one capture file (the first reply's unlink could delete the second capture). - The screenshot loop reports `captureOverdue()` once a capture has been out for twice the average round trip (at least 400ms), and the controller paints stream frames provisionally meanwhile; the existing stale guards keep the crisp shot owed until `open` returns. - The loop never re-issues a capture while the host call is unresolved; the watchdog only warns. An overdue round trip is clamped before it enters the pacing average, so the next slow load is overdue just as soon. - The agent-browser host keeps one capture per session in flight and joins a concurrent request to it, which also covers the webview adapters re-asking after their own reply timeouts (VS Code 10s, standalone 30s). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe
Four defects in the iframe proxy's HTML path, each reproduced: - `streamHtml` relabelled every HTML response `text/html; charset=utf-8`, overriding the upstream header and any `<meta charset>`, so Shift_JIS or windows-1252 pages mis-decoded. The upstream `content-type` now passes through as sent. - An upstream that compresses without being asked had the shim prepended to its gzip bytes with `content-encoding` kept (a blank frame with ERR_CONTENT_DECODING_FAILED). Only an identity-encoded, ASCII-compatible body is instrumented; a compressed or UTF-16 one passes through. - A valid document with neither `</head>` nor `<body>` got the shim before `<!doctype html>`, switching it to quirks mode. The fallback now inserts after the leading doctype/`<html>`/`<head>`/`<meta charset>` tags. - `Accept-Encoding` was deleted on every request, so a remote upstream's scripts and styles came back uncompressed. Only document loads (by `Sec-Fetch-Dest`, or none sent) ask for identity now. The head scan also decodes and searches each chunk once plus a short carry, instead of re-decoding the whole buffered prefix on every chunk. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe
Pages that fail in the iframe renderer failed silently or at a dead end, and the pane never offered the swap to agent-browser that sat one call away. - A proxied frame `load` with no shim `location` report within 1s marks the document uninstrumented (it left the proxy, was refused framing, or its grant was swept), and a slim banner offers Reload and Open in agent-browser. Only a report naming the proxy origin counts, so a clicked off-proxy link does not. - The panel's proxy errors (https://, link-local, unreachable) get an Open in agent-browser button where the host can launch one, keeping `dor ab open` as fallback text; the two scheme messages now say the same thing. - The new-tab prompt opens an https:// URL as an agent-browser pane on a proxy host, bound to its launch like a render swap and closed if the launch fails, instead of another iframe that would refuse it. - `surface.iframe` refuses an https:// target on a host with the proxy, naming `dor ab open <url>`, so `dor iframe https://...` no longer prints "created" and dead-ends. `dor iframe --help` and the `dor split` example follow. - The Display modal lists that the embed keeps no logins/cookies and disables it, with the reason, for an https:// page on a proxying host. - The proxy's served error page follows the system color scheme, and only a loopback upstream is asked about its dev server. The optional `cookies-dropped` shim notice is left out: most server-rendered frameworks set a session or CSRF cookie on every response, so the banner could not tell a dropped login from routine traffic. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe
The agent-browser host no longer keeps a subcommand allowlist; its webview channel accepts fixed argv shapes (docs/specs/dor-browser.md → Agent-Browser Host Capabilities). The sweep for the retired term missed this line-wrapped mention. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe
…efusal With two automation providers the agent-facing text still read as if there were one, and nothing said when to reach for `dor pw` or `dor iframe`. - `dor/skill.md` gains a "Which browser command" rule after the two hard rules (`dor ab` by default, `dor pw` for a Playwright user/project or a `pw-*` browser, `dor iframe` only to show a human a local http page) and the `render_mode` → `--surface` pairing; the pw section leads with "launch once with `open`, then navigate with `goto`", since Playwright's `open` restarts the browser. Stale single-provider lines are fixed: the `--json` exception list, the `--workspace` command list, and where browser keys live. - `dor pw --help` leads its examples with `open` once then `goto`, and says the first command, not the first launch, pins the cwd. - `dor ab --help` no longer promises one session is always exactly one surface (dor-browser.md → Managed identity says it is not an invariant), and points a Playwright browser at `dor pw --surface`. - Provider-mismatch refusals append the command that works: `— drive it with dor pw --surface surface:4`, or for an iframe, `dor ab open <its url>`. - Automated-browser placeholders address their own pane (`dor ab --surface surface:N open <url>`) rather than a bare `dor ab open` that drives a different browser, and stop printing internal session names. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe
`onOpenBrowserPane` created the eager, session-less pane before checking for `agentBrowserOpen`, so a caller on a host without it would leave a pane waiting on a launch that never starts. The IframePanel offers the option only where the host can launch, so this guards the Wall action itself. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe
As a type predicate on an already-`string[]` parameter, `isWebviewCommand` narrowed `args` to `never` in the refusal branch, which the VS Code extension's typecheck (which compiles the shared host) rejects. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe
The agent-browser host's `WEBVIEW_COMMANDS` table and the Playwright host's own re-parse of the same argv had already drifted: agent-browser accepted a URL of any scheme (file:, javascript:) and any DPR, Playwright http(s) and a DPR of at most 10, and only Playwright took `tab list`. `parseWebviewCommand` in browser-host-shared.ts now turns the webview's argv (the wire stays `string[]`) into a typed `WebviewCommand`. The agent-browser host rebuilds its argv from the parsed value; the Playwright host switches on it, before connecting. Launch and navigation URLs are http(s) only on both hosts (`isBrowsableUrl`), which tightens agent-browser: a URL-bar entry or relaunch target of another scheme is refused or relaunches at about:blank. The two session-name checks sit side by side there, as does the capture format normalization three sites repeated. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe
Cleanup after review (quality only, except the three tightenings noted): - One iframe refusal: `iframeRefusal(url)` in browser-url.ts and one `IFRAME_HTTP_ONLY` wording in iframe-proxy-types.ts serve the proxy, `surface.iframe`, the panel, the Display modal and the new-tab prompt. The Wall's automation→iframe swap now refuses an https page too (before, only the modal's disabled radio stopped it), and `onOpenBrowserPane` picks the renderer itself instead of taking one. The iframe remedy always says `dor ab open <url>`. - One launch-then-bind helper, `bindLaunch`, for the new-tab agent-browser pane and the pane context menu's connect, through `browserPlatform`. - A paint made only because a capture is overdue no longer supersedes it, and the pulses of that wait leave no shot owed: the held capture is drawn when `open` releases it, instead of being discarded and taken again. - VS Code waits 30s, not 10s, for an agent-browser command, edit or capture reply, past the CLI's 25s action timeout, so the webview never re-asks while the extension host is still working (Tauri already waited 30s). - DPR changes come from a `(resolution)` media query re-armed on each change, replacing the per-frame window `resize` listener and its forced layout; the pane observer publishes size changes itself. `dprMatch` joins `dimsMatch`. - One loopback-hostname predicate, `isLoopbackHostname` in lib/src/lib/ ip-literal.ts with the IPv4-mapped normalization the SSRF guard uses, for both the proxy's error pages and the browser URL helpers. - IframePanel reuses `modalActionButton` and phosphor `XIcon`, and opens agent-browser through its own `setRenderMode`. - Prose: one mechanism comment per story at the owning code; the https rule is one entry-point table in dor-browser.md with pointers elsewhere; the skill routes `pw-*` once. The stall timer becomes a `stalled` log field, the screenshot format normalization is shared, `HEAD_MARKER` reuses `instrumentHtml`'s regexes, and the provisional-paint tests share a fixture. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe
…edges - B1: the URL editor refuses a non-http(s) address with a warning under the field (a new `AnchoredWarning`, which the rename warning now wraps) instead of closing silently while the host drops it. - B2: the https refusal table in dor-browser.md now states what each entry actually shows; one hoisted check refuses a swap to `iframe` for tools and plain panes alike. Terminal-context port rows are always http:// by construction (`listenerUrlsByPort`), so they are not an entry. - T1: the controller remembers and relaunches at http(s) URLs only (`isBrowsableUrl`, now in platform/browser-automation.ts for both realms), so a `file:`/`data:` tab leaves the last http(s) page as the one restored; the header still shows such a page. - T2: `oneCapture` joins no capture from before a close or relaunch, nor one out past 30s, and a replacement capture gets a fresh file. - T3: an overdue capture clears the loop's owed shot only when it is drawn, so a failed or timed-out one still sharpens the pane. - T4: the uninstrumented-load check waits for the frame's first shim report, so a proxied image, PDF or JSON document framed from the start is not flagged; the banner no longer asserts a cause. - T5: proxied responses carry `Vary: Sec-Fetch-Dest`, since the encoding asked of the upstream depends on it. - T6: the spec names the `(resolution)` media query, not the window resize. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe
`spawnAndCapture` now takes a `timeoutMs`, so the screenshot spawn is killed past 30s — beyond the CLI's 25s action timeout a capture can queue behind a page-loading `open`. A hung capture can therefore no longer pin `oneCapture` for its session, which replaces the 30s join cap the previous commit used. Close and relaunch still evict pending captures and give the next a fresh file. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe
…rts, UTF-16 by BOM - The Display modal and the swap to `iframe` now judge the same URL, the page on screen (chrome URL, then params.url, which keeps only http(s)). `iframeRefusal` also refuses anything but http(s), so a `file:` page disables the option, and Apply is disabled if the page changed under a chosen iframe. - T1: the shim tags its `pageshow`/`DOMContentLoaded` location reports `loaded: true` (a field on the existing kind), and only those vouch for a loaded document, so a clicked link's report from the page being left no longer hides a shim-less page that loads right after. - T2: a UTF-16 body is recognized by every WHATWG label and by a byte-order mark, and passes through uninstrumented; the fallback insertion point stays behind a UTF-8 BOM. - T3: an overdue capture no longer clears the shot its wait's pulses owe — when in the round trip a slow capture was taken is unknown. It is still drawn on arrival: the loop now drops a decoded shot only when a newer one has drawn, not when a newer one merely started. - T4: the spec says one capture per session and format. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe
The modal now reads the chrome channel through useSyncExternalStore, and the story's hand-rolled controller built a new chrome object per read, so every AgentBrowserScreenModal story looped its render. The registry, the only production controller, already returns its stored objects. State that contract on ScreenController, pin it in the registry test, and give the Wall test's spied controller stable objects too. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe
a631b5c to
dd92ba8
Compare
Small, independent fixes to the browser stack from the deep dive on #773. Stacked on #773.
Security
opencarry agent-browser launch options such as--executable-pathor--args, verified against agent-browser 0.31.1.parseWebviewCommandinlib/src/host/browser-host-shared.tsnow parses the command once for both provider hosts into a typed command. Launch and navigation URLs must be http(s).screenshot <path>,close --allandtab neware no longer accepted.Performance
set viewportper event per pane (≈60/s during a drag). Size changes go through the pane's existing 200 ms debounce. A(resolution: Ndppx)media query catches DPR changes.openno longer freezes the pane on the previous page.Iframe
surface.iframerefuses https on proxy hosts, where it previously answered "created" and dead-ended.Accept-Encodingonly on document loads;Agents
dor/skill.mdsays which browser command to use (ab/pw/iframe), and thatpw-*panes are driven withdor pw --surface.openrestarts the browser;gotonavigates.dor ab --surface surface:N open <url>) instead of printing internal session names.Testing
🤖 Generated with Claude Code
https://claude.ai/code/session_01NCV5Uq6FeZbBfzfpk3vrpe