browser: optional auto-screenshot after each action - #1135
browser: optional auto-screenshot after each action#1135resumeparseeval wants to merge 2 commits into
Conversation
Capture a screenshot after every state-changing browser action (navigate, click, type, scroll, evaluate, wait, back, forward, refresh) and attach its data-URI to that action's `BrowserResponse`, so chat clients can render a per-step screenshot timeline. Capture happens at the single dispatch chokepoint in `BrowserManager::execute_action`: after a successful action whose response carries no screenshot, `attach_step_screenshot` reuses the existing `screenshot()` + `with_screenshot()` builder. Read-only/meta actions (screenshot, snapshot, get_url, get_title, close) are skipped via `BrowserAction::is_state_changing()`. The capture is best-effort — renderless browser kinds (Obscura/Lightpanda) that cannot screenshot fail silently and the action returns its original response unchanged. Gated by a new `screenshot_each_step` config flag (default true) on `[tools.browser]`, mirrored onto the runtime browser config and threaded through to the manager. With the flag false, behavior is unchanged. No new media/HTTP/persistence code: the chat layer already auto-saves any `BrowserResponse.screenshot` data-URI to the media store, rewrites it to a `media/<key>/<id>.png` ref, and strips it from LLM context.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async fn attach_step_screenshot( | ||
| &self, | ||
| sid: &str, | ||
| response: BrowserResponse, | ||
| sandbox: bool, | ||
| browser: Option<BrowserPreference>, | ||
| ) -> BrowserResponse { | ||
| if response.screenshot.is_some() { | ||
| return response; | ||
| } | ||
|
|
||
| match self | ||
| .screenshot(Some(sid), false, None, sandbox, browser) | ||
| .await | ||
| { | ||
| Ok((_, shot)) => match (shot.screenshot, shot.screenshot_scale) { | ||
| (Some(data), Some(scale)) => response.with_screenshot(data, scale), | ||
| _ => response, | ||
| }, | ||
| Err(e) => { | ||
| debug!( | ||
| session_id = sid, | ||
| error = %e, | ||
| "auto step screenshot failed; returning response without screenshot" | ||
| ); | ||
| response | ||
| }, | ||
| other => other, | ||
| } | ||
| } |
There was a problem hiding this comment.
attach_step_screenshot adds a new observable outcome (success, renderless-browser skip, capture error) but records no counters or durations. Per repo convention (AGENTS.md: "Record metrics at key points (counts, durations, errors)"), at minimum a counter should be incremented for each branch — successful attachment, skipped (response already has screenshot), and failure. Without this, there is no operational visibility into how often the extra CDP round-trip fails or how much it contributes to overall browser tool latency.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Addresses Greptile review on PR moltis-org#1135. - Default `screenshot_each_step` to `false` (was `true`) in both the runtime and TOML-facing config. Defaulting to `true` silently added a CDP screenshot round-trip after every state-changing action for existing installs on upgrade, inflating latency and tool-result payloads with no opt-in. The per-step timeline is now opt-in; behavior is unchanged unless enabled. - Also capture a screenshot when a state-changing action *fails*, not just on success. The page state at the moment of failure (e.g. an unexpected page, a missing element) is often the most useful frame in the timeline. Done in handle_request's error arm, where the user-facing error response is built: best-effort, skipped for dead-connection errors (the session is gone), and bounded by a 15s timeout so a wedged page can't hang the error path. The screenshot rides on the same BrowserResponse.screenshot field, so the chat layer persists it exactly as for successful steps. Docs, template, and the defaults test updated to match.
|
Thanks @greptile-apps — addressed in 8db81f4. P1 (opt-out default): Changed Also added (per maintainer request): capture a screenshot when a state-changing action fails, not just on success — the page state at the moment of failure is often the most useful frame. It's done in Re: the "missing metrics" note — the auto-capture reuses the existing |
|
Both changes look clean — here's a quick read-through of the commit. Default flip ( Failure screenshot path — the guard is well-structured:
One minor thing worth being aware of: the failure screenshot runs after The LGTM. |
|
@greptileai review |
| if want_failure_shot | ||
| && !e.is_connection_error() | ||
| && let Some(sid) = | ||
| request.session_id.as_deref().filter(|s| !s.is_empty()) | ||
| { | ||
| match timeout( | ||
| Duration::from_secs(15), | ||
| self.screenshot(Some(sid), false, None, sandbox, request.browser), | ||
| ) | ||
| .await | ||
| { | ||
| Ok(Ok((_, shot))) => { | ||
| if let (Some(data), Some(scale)) = | ||
| (shot.screenshot, shot.screenshot_scale) | ||
| { | ||
| resp = resp.with_screenshot(data, scale); | ||
| } | ||
| }, | ||
| Ok(Err(err)) => debug!( | ||
| session_id = sid, | ||
| error = %err, | ||
| "failure-state screenshot failed; returning error without screenshot" | ||
| ), | ||
| Err(_) => { | ||
| debug!(session_id = sid, "failure-state screenshot timed out") | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
The failure screenshot runs in the Ok(result) => { Err(e) => { … } } arm, which is outside the timeout(timeout_duration, …) wrapper that bounds execute_action. The inner timeout(Duration::from_secs(15), …) is a separate, unconditional cap — not the remaining slice of request.timeout_ms. A caller who sets timeout_ms = 10_000 and whose action fails at 9 999 ms will wait an additional 15 s (total ≈ 25 s) before receiving the error response.
The fix is to cap the screenshot attempt with the time remaining from the original deadline: timeout_duration.saturating_sub(start.elapsed()), optionally clamped to a safety maximum (e.g. min(remaining, Duration::from_secs(15))).
2 participants
Summary
Automatically captures a screenshot after each state-changing browser action and attaches it to that action's tool result, so chat clients can render a per-step screenshot timeline.
BrowserManager::execute_action(crates/browser/src/manager.rs). After a successful action whoseBrowserResponse.screenshotis stillNone, a newattach_step_screenshothelper reuses the existingscreenshot()+with_screenshot()builder to copy the data-URI andscreenshot_scaleonto the response.Navigateis handled in its early-return path (after the navigate call); all other state-changing actions (Click, Type, Scroll, Evaluate, Wait, Back, Forward, Refresh) are gated by the newBrowserAction::is_state_changing(). Read-only/meta actions (Screenshot, Snapshot, GetUrl, GetTitle, Close) are excluded.supports_screenshots()is false) error on capture — the error is ignored and the original response is returned unchanged. The auto-screenshot never fails the action.screenshot_each_step: boolflag (defaulttrue) on[tools.browser], mirrored onto the runtimemoltis_browser::BrowserConfig+ itsFromimpl, and threaded intoBrowserManager. With the flagfalse, behavior is unchanged.No new media/HTTP/persistence code: the chat layer already auto-saves any
BrowserResponse.screenshotdata-URI to the media store, rewrites it to amedia/<key>/<id>.pngref, and strips it from LLM context. The LLM-context sanitizer andmax_tool_result_bytesare untouched.Per repo conventions, the new config field is also wired into
build_schema_map()(crates/config/src/validate/schema_map.rs), the config template (crates/config/src/template.rs), and the docs (configuration-reference.md,browser-automation.md).Validation
Completed
cargo build -p moltis-browser -p moltis-config -p moltis-toolscargo clippy -p moltis-browser -p moltis-config -p moltis-tools— clean on changed filescargo test -p moltis-browser --lib— newstate_changing_actions_request_step_screenshotsandscreenshot_each_step_defaults_ontests passcargo test -p moltis-config— 347 tests pass (schema-map completeness verified)cargo fmt --check— no genuine diffs in changed filesRemaining
./scripts/local-validate.sh <PR#>(full OS-aware lint/test suite)Manual QA
[tools.browser] screenshot_each_step = true(default), issue anavigate/click/typeaction — the result now includes ascreenshotdata-URI, and the chat client's automation-steps view shows the per-step image.screenshot_each_step = false— results no longer carry an auto-screenshot; behavior matches prior releases.browser = "obscura"/"lightpanda") and run a state-changing action — the action succeeds and returns its normal response with no screenshot and no surfaced error.🤖 Generated with Claude Code