refactor: rewrite browser tools — 12 flat tools, CDP accessibility API, secure credential entry#352
Open
refactor: rewrite browser tools — 12 flat tools, CDP accessibility API, secure credential entry#352
Conversation
…ols with index-based ARIA tree Replace the single BrowserTool with 12 separate tools (browser_launch, browser_navigate, browser_snapshot, browser_click, browser_type, browser_press_key, browser_screenshot, browser_evaluate, browser_tab_open, browser_tab_list, browser_tab_close, browser_close) that share a BrowserContext with Arc<Mutex<BrowserState>>. Port extract_dom.js from browser-use-rs to build an ARIA tree with numeric indices and CSS selectors. browser_snapshot returns YAML-like output where interactive elements are tagged [index=N]. Click/type/press_key accept an index that resolves to a CSS selector at call time, eliminating stale CDP node ID issues. Add snapshot caching on BrowserState, invalidated on mutations. Update worker prompt and capability fragments for the new tool names and workflow.
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:
WalkthroughRefactors browser tooling into a suite of granular browser_* tools, replaces element-ref interactions with ARIA-indexed DOM snapshots and selectors, centralizes shared browser state registration and URL safety checks, and adds a browser-side DOM extraction script. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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)
src/tools/browser.rs (1)
1670-1673: Consider removing the unusedcontextparameter.The
contextparameter is explicitly suppressed as unused. If it's not needed for the function's logic, consider removing it from the signature to avoid the workaround.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 1670 - 1673, Remove the unused parameter by deleting `context` from the function signature and removing the suppression line `let _ = context;`; then update all call sites and any trait/closure/type signatures that expect the extra `context` parameter (e.g., the navigate tool pattern) so they match the new signature, ensuring types still align with surrounding code and removing the need for the unused-variable workaround.
🤖 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/tools/browser_extract_dom.js`:
- Around line 156-161: The selector built for finding a <label> by the input's
id can break if element.id contains quotes or other special chars; update the
code that builds the selector (the document.querySelector call that uses
'label[for="' + id + '"]') to escape the id using CSS.escape (or a small escape
fallback) before interpolating, so labels are correctly matched for element.id
values; ensure you reference element.id and replace the raw usage in the
document.querySelector('label[for="' + id + '"]') expression with the escaped
value.
- Around line 651-684: The buildSelector function currently interpolates
element.id and className parts directly, producing invalid selectors when
identifiers contain special characters; update buildSelector to escape IDs and
class tokens using CSS.escape (falling back to a small polyfill if CSS.escape is
unavailable) whenever you prepend '#' for id or '.' for class (i.e., inside the
early return that returns '#'+element.id and where selector += '.' +
classes[0]); ensure the nth-child logic and tag name usage remain unchanged but
only use escaped identifier strings in the assembled selector.
In `@src/tools/browser.rs`:
- Around line 633-639: The code currently discards the Result from
std::fs::remove_file(&lock_file) with let _ = ..., so change it to explicitly
handle the Result and log failures; locate the block guarded by
persistent_profile and the lock_file variable in src/tools/browser.rs and
replace the discard with a match or if let Err(e) =
std::fs::remove_file(&lock_file) that emits a tracing::warn! or tracing::error!
including the lock_file path (use %lock_file.display()) and the error (e) so the
removal remains best-effort but failures are recorded.
---
Nitpick comments:
In `@src/tools/browser.rs`:
- Around line 1670-1673: Remove the unused parameter by deleting `context` from
the function signature and removing the suppression line `let _ = context;`;
then update all call sites and any trait/closure/type signatures that expect the
extra `context` parameter (e.g., the navigate tool pattern) so they match the
new signature, ensuring types still align with surrounding code and removing the
need for the unused-variable workaround.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 00c6ce62-3488-4810-9da8-6319c0a191e6
📒 Files selected for processing (6)
prompts/en/fragments/worker_capabilities.md.j2prompts/en/tools/browser_description.md.j2prompts/en/worker.md.j2src/tools.rssrc/tools/browser.rssrc/tools/browser_extract_dom.js
…h Rust struct The JS returned camelCase `iframeIndices` but DomSnapshot expected snake_case `iframe_indices`, causing deserialization to fail with 'missing field iframe_indices' on every browser_snapshot call. This forced workers to fall back to browser_evaluate for all interaction. Also soften worker prompt guidance on browser_evaluate — suggest index-based tools as preferred but keep evaluate as a valid fallback.
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
… trimmed element roles Three fixes for browser tool reliability: 1. CSS selector generation: look for nearest ancestor with an ID and anchor from there (max 6 levels). Verify uniqueness via querySelectorAll before returning. Falls back to full path from body. Replaces the old strategy that built deep fragile nth-child chains from body every time. 2. Page readiness: add wait_for_page_ready() that polls readyState + body.children.length after navigate and click. Gives SPAs up to 3s to hydrate before snapshot extraction runs. Fixes empty snapshots on client-rendered pages like railway.com. 3. Trim interactiveRoles: remove structural/landmark roles (generic, list, listitem, heading, region, navigation, banner, contentinfo, article, form, search, main, complementary, tabpanel, tree, grid, row, status, progressbar, alert) from the indexed set. Only truly interactive elements (button, link, textbox, checkbox, etc.) get indices now. Reduces snapshot noise from ~139 to ~20-30 elements on typical pages.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
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)
39-82:⚠️ Potential issue | 🔴 CriticalBlock hostnames that resolve to local/private addresses.
This only rejects literal IPs and three hard-coded metadata hosts.
http://localhost:...and any DNS name that resolves to127.0.0.1/RFC1918 still pass, so the SSRF boundary is bypassable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 39 - 82, validate_url currently only blocks literal IPs and a few hard-coded hostnames; to prevent SSRF you must also resolve hostnames and deny navigation if any resolved A/AAAA address is private/loopback. After the existing host extraction in validate_url, perform a DNS lookup for the hostname (handling bracketed IPv6 by reusing the existing stripped logic) and iterate all resolved IpAddr results; for each resolution call is_blocked_ip(ip) and return a BrowserError if any address is blocked. Ensure you follow existing behavior for the special metadata hosts, handle both IPv4 and IPv6 records, and use a resolver with a reasonable timeout to avoid hanging the caller (e.g. async DNS resolver or std lookup_host wrapped in a timeout).
🤖 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/tools/browser_extract_dom.js`:
- Around line 429-434: The code currently sets visible = isElementVisibleForAria
|| isElementVisible(element), which reintroduces elements that are aria-hidden
but have layout; change the logic so ARIA-hidden elements are never included by
using a conjunction: set visible to require not aria-hidden and layout-visible
(e.g., visible = isElementVisibleForAria && isElementVisible(element)),
referencing isElementHiddenForAria and isElementVisible and the visible variable
so the subsequent "if (!visible) return;" correctly skips ARIA-hidden nodes.
- Around line 485-488: The traversal currently descends into element.shadowRoot
inside visit, producing selectors for shadow-root-contained nodes that
buildSelector (which relies on parentElement and parent.children) cannot resolve
via document.querySelector; stop descending into shadow roots so published
indices only reference nodes reachable from the main document. In practice,
remove or skip the loop that iterates element.shadowRoot.firstChild in visit
(and the similar block at the other location) so shadow DOM is treated as
opaque, ensuring buildSelector produces selectors resolvable by
document.querySelector.
In `@src/tools/browser.rs`:
- Around line 555-560: When snapshot.error is Some we must not treat the result
as a successful empty snapshot; change the code in the browser snapshot handling
(the block that currently does `if let Some(ref error) = snapshot.error {
tracing::warn!(%error, "DOM extraction JS reported an error"); } state.snapshot
= Some(snapshot); Ok(state.snapshot.as_ref().expect("just stored"))`) to return
an Err (or propagate a failure) containing the extraction error instead of
storing and returning the snapshot; update the same pattern found likewise in
the other occurrence (the second DOM extraction handler) so that
`snapshot.error` triggers an error return and the bad snapshot is not cached in
`state.snapshot`.
- Around line 1408-1415: The code removes the Page from state.pages before
awaiting page.close(), so if page.close() fails the tab is lost from the map and
active_target may point to a missing page; change the flow so you obtain the
Page (e.g., via get/clone or by temporarily taking an Arc/handle without
removing it), call page.close().await first and only upon success remove/drain
the entry from state.pages and update active_target accordingly; ensure error
paths return the BrowserError without mutating state.pages (and apply the same
change to the similar block handling page close around the close/open swap that
mirrors this logic).
- Around line 805-810: The double navigation occurs because
get_or_create_page(&self.context, &mut state, Some(&args.url)) may call
new_page(url) which already navigates, and browser_navigate then unconditionally
calls page.goto(&args.url); change the flow so you only call page.goto when the
page returned was not already navigated: modify get_or_create_page/new_page to
either (a) return a tuple (Page, bool) or a Page wrapper indicating whether
initial navigation occurred, or (b) add an overload/getter that does not
auto-navigate; then update the browser_navigate call site to check that flag and
skip page.goto(&args.url) when the page was created with the initial URL. Ensure
references to get_or_create_page, new_page, and page.goto are adjusted
accordingly.
- Around line 999-1006: The current JS always assigns el.value = {text_json}, so
when args.clear is false it still overwrites; update the JS assembly in
browser.rs (the clear_js/js construction that references selector_json and
text_json) to conditionally insert/append instead of replacing: if args.clear is
true keep el.value = {text_json}, otherwise preserve existing content by
inserting at the cursor (use el.selectionStart/selectionEnd to splice: el.value
= el.value.slice(0, el.selectionStart) + {text_json} +
el.value.slice(el.selectionEnd) and update selection/caret) or at minimum append
with el.value = el.value + {text_json}; ensure selector_json and text_json are
used and maintain focus behavior.
- Around line 311-314: The current truncation uses raw byte-slicing like
&trimmed[..200] which can panic on multi-byte UTF-8; replace these with a
char-boundary-safe truncation (e.g., implement a helper like
truncate_to_char_boundary(s: &str, max_chars: usize) -> String that uses
s.chars().take(max_chars).collect() or uses char_indices to cut at a valid
boundary) and use it when building display from trimmed (and the other
occurrence referenced). Ensure any out-of-range or invalid input returns a
proper Err variant/results from the enclosing function instead of panicking so
tool errors are returned as structured Results rather than hard failures.
---
Outside diff comments:
In `@src/tools/browser.rs`:
- Around line 39-82: validate_url currently only blocks literal IPs and a few
hard-coded hostnames; to prevent SSRF you must also resolve hostnames and deny
navigation if any resolved A/AAAA address is private/loopback. After the
existing host extraction in validate_url, perform a DNS lookup for the hostname
(handling bracketed IPv6 by reusing the existing stripped logic) and iterate all
resolved IpAddr results; for each resolution call is_blocked_ip(ip) and return a
BrowserError if any address is blocked. Ensure you follow existing behavior for
the special metadata hosts, handle both IPv4 and IPv6 records, and use a
resolver with a reasonable timeout to avoid hanging the caller (e.g. async DNS
resolver or std lookup_host wrapped in a timeout).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a38d0ba8-59f2-485e-96c7-f90e06075795
📒 Files selected for processing (3)
prompts/en/worker.md.j2src/tools/browser.rssrc/tools/browser_extract_dom.js
Comment on lines
+805
to
810
| // Get or create the active page | ||
| let page = get_or_create_page(&self.context, &mut state, Some(&args.url)).await?; | ||
|
|
||
| page.goto(&url) | ||
| page.goto(&args.url) | ||
| .await | ||
| .map_err(|error| BrowserError::new(format!("navigation failed: {error}")))?; |
Contributor
There was a problem hiding this comment.
The first navigation can fire twice.
If there is no active page, get_or_create_page(..., Some(&args.url)) opens new_page(url) and browser_navigate immediately calls goto(url) again. That duplicates the initial request and can repeat page side effects.
Suggested diff
- let page = get_or_create_page(&self.context, &mut state, Some(&args.url)).await?;
+ let page = get_or_create_page(&self.context, &mut state, None).await?;Also applies to: 1620-1624
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/browser.rs` around lines 805 - 810, The double navigation occurs
because get_or_create_page(&self.context, &mut state, Some(&args.url)) may call
new_page(url) which already navigates, and browser_navigate then unconditionally
calls page.goto(&args.url); change the flow so you only call page.goto when the
page returned was not already navigated: modify get_or_create_page/new_page to
either (a) return a tuple (Page, bool) or a Page wrapper indicating whether
initial navigation occurred, or (b) add an overload/getter that does not
auto-navigate; then update the browser_navigate call site to check that flag and
skip page.goto(&args.url) when the page was created with the initial URL. Ensure
references to get_or_create_page, new_page, and page.goto are adjusted
accordingly.
Comment on lines
+1408
to
+1415
| let page = state | ||
| .pages | ||
| .remove(&id) | ||
| .ok_or_else(|| BrowserError::new(format!("no tab with target_id '{id}'")))?; | ||
|
|
||
| page.close() | ||
| .await | ||
| .map_err(|error| BrowserError::new(format!("failed to close tab: {error}")))?; |
Contributor
There was a problem hiding this comment.
Don't drop tab bookkeeping before close() succeeds.
Both close paths remove/drain state.pages before awaiting page.close(). If the CDP close call fails, the tab can remain open but disappear from the in-memory map, and active_target can keep pointing at a page that no longer exists in state.pages.
Also applies to: 1464-1487
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/browser.rs` around lines 1408 - 1415, The code removes the Page
from state.pages before awaiting page.close(), so if page.close() fails the tab
is lost from the map and active_target may point to a missing page; change the
flow so you obtain the Page (e.g., via get/clone or by temporarily taking an
Arc/handle without removing it), call page.close().await first and only upon
success remove/drain the entry from state.pages and update active_target
accordingly; ensure error paths return the BrowserError without mutating
state.pages (and apply the same change to the similar block handling page close
around the close/open swap that mirrors this logic).
browser_click and browser_type now accept either `index` (from snapshot) or `selector` (CSS selector like "#login_field") to target elements. This lets the LLM use a known selector directly instead of relying on snapshot indices that may be stale or point to the wrong element after page transitions. browser_type also validates that the target element is an INPUT, TEXTAREA, SELECT, or contentEditable element before typing, returning a clear error if it's a button or div instead of silently 'succeeding'. Updated worker prompt to document both targeting modes.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (13)
src/tools/browser_extract_dom.js (3)
429-434:⚠️ Potential issue | 🟠 MajorARIA-hidden nodes can be reintroduced into the snapshot.
The OR logic at line 431 allows elements with
aria-hidden="true"orvisibility:hiddento be included whenever they have layout dimensions. This breaks the ARIA snapshot contract — indices may be assigned to elements users cannot perceive.🛡️ Proposed fix — use AND to require both conditions
// Check visibility const isElementVisibleForAria = !isElementHiddenForAria(element); - let visible = isElementVisibleForAria || isElementVisible(element); + let visible = isElementVisibleForAria && isElementVisible(element); // Skip if not visible for ARIA if (!visible) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser_extract_dom.js` around lines 429 - 434, The current visibility check uses OR which lets aria-hidden elements with layout dimensions pass; update the logic so an element is considered visible only if it is both not hidden for ARIA and has layout visibility: change the computed "visible" to require isElementVisibleForAria && isElementVisible(element) (referencing isElementHiddenForAria, isElementVisible, and the visible variable) so elements with aria-hidden="true" are excluded from the ARIA snapshot.
156-161:⚠️ Potential issue | 🟡 MinorUnescaped ID in CSS attribute selector may cause incorrect matches.
The label lookup at line 159 uses raw
idwithout escaping. Ifelement.idcontains a double quote or other special characters, the selector will be malformed.🛡️ Proposed fix
if (id) { - const label = document.querySelector('label[for="' + id + '"]'); + const escapedId = (window.CSS && CSS.escape) ? CSS.escape(id) : id; + const label = document.querySelector('label[for="' + escapedId + '"]'); if (label) return label.textContent || ''; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser_extract_dom.js` around lines 156 - 161, The label lookup uses the raw element.id in a CSS attribute selector which can break if the id contains quotes or special characters; update the lookup in the branch handling INPUT/TEXTAREA/SELECT (the code referencing element.tagName and id) to escape the id before building the selector (use CSS.escape or an equivalent escaping helper) so document.querySelector('label[for="..."]') receives a safe, escaped identifier and cannot be malformed by special characters.
485-490:⚠️ Potential issue | 🟠 MajorShadow-root elements get indices that cannot be resolved by
document.querySelector().The traversal descends into
element.shadowRootand assigns indices to elements inside. However,buildSelector()builds paths usingparentElementwhich stops at shadow boundaries. Whenbrowser.rscallsdocument.querySelector(selector), the lookup fails because shadow DOM is encapsulated.Consider either:
- Skip shadow root traversal entirely (treat shadow DOM as opaque)
- Store a separate resolution path that explicitly traverses shadow hosts
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser_extract_dom.js` around lines 485 - 490, The traversal currently descends into element.shadowRoot inside visit(), producing selectors buildSelector() cannot resolve via document.querySelector() because shadow DOM is encapsulated; either stop descending into shadow roots (remove or guard the element.shadowRoot loop in visit() and treat shadow DOM as opaque so indices and parentElement-based paths remain resolvable), or persist an alternate resolution path on the aria node that records shadow boundaries (e.g., store a shadow-aware path or a sequence of {hostSelector, childIndex} entries while visiting) and update browser.rs to resolve selectors by traversing shadowHost -> shadowRoot explicitly instead of using document.querySelector(); pick one approach and apply consistently to visit(), buildSelector(), and the resolution logic in browser.rs.src/tools/browser.rs (10)
664-670:⚠️ Potential issue | 🟡 MinorSilently discarded error when removing stale lock file.
Per coding guidelines, errors should not be silently discarded with
let _ =. While the lock file removal is best-effort, the error should at least be logged.🛡️ Proposed fix
if persistent_profile { let lock_file = user_data_dir.join("SingletonLock"); if lock_file.exists() { tracing::debug!(path = %lock_file.display(), "removing stale Chrome SingletonLock"); - let _ = std::fs::remove_file(&lock_file); + if let Err(error) = std::fs::remove_file(&lock_file) { + tracing::debug!(path = %lock_file.display(), %error, "failed to remove stale SingletonLock"); + } } }Based on learnings: "Never silently discard errors; no
let _ =on Results; handle, log, or propagate 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 664 - 670, The removal of the stale Chrome SingletonLock currently discards any error with `let _ = std::fs::remove_file(&lock_file);`; change this to handle and log failures instead: replace the silent discard with an explicit check (e.g., `if let Err(e) = std::fs::remove_file(&lock_file)` or `match`) and emit a tracing log (tracing::warn! or tracing::debug!) that includes the `lock_file.display()` and the error `e` so failures are recorded; keep the operation best-effort (do not propagate) but do not ignore the Result from `remove_file`.
1089-1093:⚠️ Potential issue | 🔴 CriticalUTF-8 byte slicing can panic on multi-byte characters.
Same issue as in
render_child—&args.text[..50]will panic if byte 50 is mid-codepoint.🛡️ Proposed fix
- let display_text = if args.text.len() > 50 { - format!("{}...", &args.text[..50]) - } else { - args.text.clone() - }; + let display_text = { + let mut chars = args.text.chars(); + let prefix: String = chars.by_ref().take(50).collect(); + if chars.next().is_some() { + format!("{prefix}...") + } else { + prefix + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 1089 - 1093, The code currently slices args.text by bytes (using &args.text[..50]) which can panic on multi-byte UTF-8 characters; update the logic in the display_text construction (and mirror the same pattern used in render_child) to operate on character count instead of byte offsets: check args.text.chars().count() > 50 and build the truncated string by taking the first 50 chars (e.g., via chars().take(50).collect()) then append "..." so you never slice mid-codepoint and avoid panics.
598-603:⚠️ Potential issue | 🟠 MajorDOM extraction failures are logged but not surfaced to the caller.
When
snapshot.erroris present, the code logs a warning but still caches and returns the snapshot. This makes extraction failures indistinguishable from a genuinely empty page and poisons the cache.🛡️ Proposed fix — return error instead of caching bad snapshot
let snapshot: DomSnapshot = serde_json::from_value(json_value) .map_err(|error| BrowserError::new(format!("failed to parse DOM snapshot: {error}")))?; if let Some(ref error) = snapshot.error { - tracing::warn!(%error, "DOM extraction JS reported an error"); + return Err(BrowserError::new(format!("DOM extraction failed: {error}"))); } state.snapshot = Some(snapshot); Ok(state.snapshot.as_ref().expect("just stored"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 598 - 603, The code currently logs DOM extraction failures (snapshot.error) but still sets state.snapshot and returns it, which caches bad snapshots; update the logic in the function handling snapshot creation so that if snapshot.error.is_some() you do NOT assign state.snapshot and instead return an Err containing the extraction error (or map snapshot.error into your function's error type) so callers see the failure; keep the tracing::warn! call if desired but ensure state.snapshot = Some(snapshot) only happens when snapshot.error.is_none(), referencing snapshot.error and state.snapshot to find where to change the flow.
1262-1290:⚠️ Potential issue | 🟠 MajorSnapshot not invalidated after JavaScript evaluation.
browser_evaluatecan execute arbitrary JS that mutates the DOM, but it leaves the cached snapshot intact. Subsequent index-based interactions may operate on stale selectors.🛡️ Proposed fix
async fn call(&self, args: Self::Args) -> Result<Self::Output, Self::Error> { if !self.context.config.evaluate_enabled { return Err(BrowserError::new( "JavaScript evaluation is disabled in browser config (set evaluate_enabled = true)", )); } - let state = self.context.state.lock().await; + let mut state = self.context.state.lock().await; let page = self.context.require_active_page(&state)?; let result = page .evaluate(args.script) .await .map_err(|error| BrowserError::new(format!("evaluate failed: {error}")))?; let value = result.value().cloned(); + state.invalidate_snapshot(); Ok(BrowserOutput {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 1262 - 1290, The JavaScript evaluation path in call(...) doesn't invalidate the cached snapshot, so after page.evaluate(...) succeeds you must clear the page's cached snapshot/state to avoid stale DOM usage; locate the active page via self.context.require_active_page(&state) (or directly via self.context.state) and reset its snapshot/cache (e.g., set the page.snapshot or cached_snapshot field to None or call the page.invalidate_snapshot()/state.invalidate_snapshot_for_page(page_id) helper if one exists) before returning the BrowserOutput so subsequent index-based interactions use a fresh snapshot.
849-858:⚠️ Potential issue | 🟠 MajorFirst navigation can fire twice.
If no active page exists,
get_or_create_page(..., Some(&args.url))callsbrowser.new_page(target_url)which navigates to the URL. Then line 851 immediately callspage.goto(&args.url)again. This duplicates the initial request and can repeat page side effects.🛡️ Proposed fix — don't pass URL to get_or_create_page
- let page = get_or_create_page(&self.context, &mut state, Some(&args.url)).await?; + let page = get_or_create_page(&self.context, &mut state, None).await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 849 - 858, The code double-navigates because get_or_create_page(&self.context, &mut state, Some(&args.url)) calls browser.new_page(target_url) and then page.goto(&args.url) runs again; change the call to get_or_create_page to pass None for the URL so the helper won't pre-navigate (i.e., call get_or_create_page(&self.context, &mut state, None) or otherwise ensure browser.new_page is not given args.url), keep the single explicit page.goto(&args.url) afterward, and leave wait_for_page_ready(page).await as-is; update references to get_or_create_page, browser.new_page, and page.goto accordingly.
354-358:⚠️ Potential issue | 🔴 CriticalUTF-8 byte slicing can panic on multi-byte characters.
&trimmed[..200]will panic at runtime if byte 200 falls within a multi-byte UTF-8 character. Per coding guidelines, tool implementations must return errors as structured results, not panics.🛡️ Proposed fix — truncate by chars
- let display = if trimmed.len() > 200 { - format!("{}...", &trimmed[..200]) - } else { - trimmed.to_string() - }; + let display = { + let mut chars = trimmed.chars(); + let prefix: String = chars.by_ref().take(200).collect(); + if chars.next().is_some() { + format!("{prefix}...") + } else { + prefix + } + };Based on learnings: "Tool implementations must return errors as structured results, not panics".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 354 - 358, The current byte-slice truncation (&trimmed[..200]) can panic on multi-byte UTF-8 characters; change the display creation to safely truncate by characters: compute whether trimmed.chars().count() > 200, then build a safe truncated string using trimmed.chars().take(200).collect::<String>() and append "..." when truncated. Replace the existing display logic (the display variable and its branches) to use this char-based approach so no byte-slicing/panic occurs and behavior remains deterministic.
1461-1476:⚠️ Potential issue | 🟠 MajorTab bookkeeping updated before
close()succeeds.The page is removed from
state.pagesat line 1463 beforepage.close().awaitis called. If the CDP close call fails, the tab remains open but disappears from the in-memory map, andactive_targetmay point at a page that no longer exists instate.pages.🛡️ Proposed fix — close first, then update bookkeeping
async fn call(&self, args: Self::Args) -> Result<Self::Output, Self::Error> { let mut state = self.context.state.lock().await; let id = args .target_id .or_else(|| state.active_target.clone()) .ok_or_else(|| BrowserError::new("no active tab to close"))?; - let page = state - .pages - .remove(&id) - .ok_or_else(|| BrowserError::new(format!("no tab with target_id '{id}'")))?; + let page = state + .pages + .get(&id) + .ok_or_else(|| BrowserError::new(format!("no tab with target_id '{id}'")))?; page.close() .await .map_err(|error| BrowserError::new(format!("failed to close tab: {error}")))?; + state.pages.remove(&id); + if state.active_target.as_ref() == Some(&id) { state.active_target = state.pages.keys().next().cloned(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 1461 - 1476, The code currently removes the Page from state.pages before calling page.close(), which can leave the tab open in CDP but missing from the in-memory map if close() fails; instead, first fetch the page without removing it (e.g., via state.pages.get or similar), call page.close().await and return the BrowserError on failure, and only after close succeeds remove the page from state.pages and update state.active_target (if it was the closed id) to the next key; finally call state.invalidate_snapshot() and return success. Reference the page.close().await call and the state.pages.remove(&id) / state.active_target update so you replace the remove-before-close behavior with close-then-remove bookkeeping.
1134-1142:⚠️ Potential issue | 🟠 MajorSnapshot not invalidated after key press.
Key presses like Enter or Tab can mutate the DOM or change focus state. The cached snapshot should be invalidated so subsequent index interactions don't use stale selectors.
🛡️ Proposed fix
async fn call(&self, args: Self::Args) -> Result<Self::Output, Self::Error> { - let state = self.context.state.lock().await; + let mut state = self.context.state.lock().await; let page = self.context.require_active_page(&state)?; dispatch_key_press(page, &args.key).await?; + state.invalidate_snapshot(); Ok(BrowserOutput::success(format!( "Pressed key '{}'", args.key ))) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 1134 - 1142, The key-press handler currently calls dispatch_key_press(...) but doesn't invalidate the cached DOM snapshot, so subsequent index/selector operations may use stale state; after the await dispatch_key_press(page, &args.key)? call in the async fn call (the method using self.context.require_active_page and returning BrowserOutput::success), add a call to clear the cached snapshot on the browser context (for example self.context.invalidate_snapshot().await or state.clear_snapshot())—if no such API exists, implement an invalidate_snapshot/clear_snapshot method on the Context (or on the locked state) that drops/refreshes the cached snapshot and await it here so the snapshot is invalidated immediately after the key press.
1516-1523:⚠️ Potential issue | 🟠 Major
CloseTabspolicy drains pages before closing them.Similar to
browser_tab_close, theCloseTabspolicy at line 1519 drains all pages from the map before iterating and callingpage.close(). If some closes fail, bookkeeping is already corrupted.🛡️ Proposed fix — collect pages, close, then drain on success
ClosePolicy::CloseTabs => { - let pages_to_close: Vec<(String, chromiumoxide::Page)> = { - let mut state = self.context.state.lock().await; - let pages = state.pages.drain().collect(); - state.active_target = None; - state.invalidate_snapshot(); - pages - }; + let mut state = self.context.state.lock().await; + let page_ids: Vec<String> = state.pages.keys().cloned().collect(); let mut close_errors = Vec::new(); - for (id, page) in pages_to_close { - if let Err(error) = page.close().await { - close_errors.push(format!("{id}: {error}")); + for id in &page_ids { + if let Some(page) = state.pages.get(id) { + if let Err(error) = page.close().await { + close_errors.push(format!("{id}: {error}")); + } } } if !close_errors.is_empty() { // ... error handling } + state.pages.clear(); + state.active_target = None; + state.invalidate_snapshot();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 1516 - 1523, The ClosePolicy::CloseTabs branch currently drains state.pages before attempting page.close(), corrupting bookkeeping if any close fails; instead, snapshot the pages while holding the lock (e.g., collect identifiers and clones/references of the pages into a temporary Vec), then release the lock and iterate calling page.close() on the snapshot, and only after successful closes reacquire the lock to drain/remove those entries and set state.active_target = None and call state.invalidate_snapshot(); reference ClosePolicy::CloseTabs, state.pages, state.active_target, state.invalidate_snapshot(), and page.close() when making this change.
1052-1067:⚠️ Potential issue | 🟠 Major
clear: falsestill overwrites the existing value.When
args.clearis false,clear_jsis empty, but line 1059 unconditionally assignsel.value = {text_json}. For prefilled inputs,clear: falseshould append or insert at cursor position rather than replacing field contents.🛡️ Proposed fix — insert at cursor when not clearing
let text_json = serde_json::to_string(&args.text).unwrap_or_default(); - let clear_js = if args.clear { "el.value = '';" } else { "" }; + let value_assignment = if args.clear { + format!("el.value = {text_json};") + } else { + // Insert at cursor position (or append if no selection) + format!( + "const start = el.selectionStart || el.value.length; \ + const end = el.selectionEnd || el.value.length; \ + el.value = el.value.slice(0, start) + {text_json} + el.value.slice(end); \ + el.selectionStart = el.selectionEnd = start + {text_json}.length;" + ) + }; let js = format!( r#"(() => {{ const el = document.querySelector({selector_json}); if (!el) return JSON.stringify({{success: false, error: 'element not found'}}); el.focus(); - {clear_js} - el.value = {text_json}; + {value_assignment} el.dispatchEvent(new Event('input', {{bubbles: true}}));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 1052 - 1067, The current JS unconditionally replaces the element value; change it so when args.clear is false it inserts at the cursor (or appends if no cursor) instead of overwriting. Replace the simple clear_js/assignment logic with a conditional JS block that: if args.clear is true, sets el.value = {text_json}; else compute let start = el.selectionStart ?? el.value.length, end = el.selectionEnd ?? start, then set el.value = el.value.slice(0, start) + {text_json} + el.value.slice(end), and set el.selectionStart = el.selectionEnd = start + ({text_json}.length) to place the caret after the inserted text; keep el.focus() and the input/change event dispatching using the existing selector_json and text_json identifiers. Ensure you update the variable used in format! (js string) rather than leaving the old unconditional el.value assignment.
🤖 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/tools/browser_extract_dom.js`:
- Around line 429-434: The current visibility check uses OR which lets
aria-hidden elements with layout dimensions pass; update the logic so an element
is considered visible only if it is both not hidden for ARIA and has layout
visibility: change the computed "visible" to require isElementVisibleForAria &&
isElementVisible(element) (referencing isElementHiddenForAria, isElementVisible,
and the visible variable) so elements with aria-hidden="true" are excluded from
the ARIA snapshot.
- Around line 156-161: The label lookup uses the raw element.id in a CSS
attribute selector which can break if the id contains quotes or special
characters; update the lookup in the branch handling INPUT/TEXTAREA/SELECT (the
code referencing element.tagName and id) to escape the id before building the
selector (use CSS.escape or an equivalent escaping helper) so
document.querySelector('label[for="..."]') receives a safe, escaped identifier
and cannot be malformed by special characters.
- Around line 485-490: The traversal currently descends into element.shadowRoot
inside visit(), producing selectors buildSelector() cannot resolve via
document.querySelector() because shadow DOM is encapsulated; either stop
descending into shadow roots (remove or guard the element.shadowRoot loop in
visit() and treat shadow DOM as opaque so indices and parentElement-based paths
remain resolvable), or persist an alternate resolution path on the aria node
that records shadow boundaries (e.g., store a shadow-aware path or a sequence of
{hostSelector, childIndex} entries while visiting) and update browser.rs to
resolve selectors by traversing shadowHost -> shadowRoot explicitly instead of
using document.querySelector(); pick one approach and apply consistently to
visit(), buildSelector(), and the resolution logic in browser.rs.
In `@src/tools/browser.rs`:
- Around line 664-670: The removal of the stale Chrome SingletonLock currently
discards any error with `let _ = std::fs::remove_file(&lock_file);`; change this
to handle and log failures instead: replace the silent discard with an explicit
check (e.g., `if let Err(e) = std::fs::remove_file(&lock_file)` or `match`) and
emit a tracing log (tracing::warn! or tracing::debug!) that includes the
`lock_file.display()` and the error `e` so failures are recorded; keep the
operation best-effort (do not propagate) but do not ignore the Result from
`remove_file`.
- Around line 1089-1093: The code currently slices args.text by bytes (using
&args.text[..50]) which can panic on multi-byte UTF-8 characters; update the
logic in the display_text construction (and mirror the same pattern used in
render_child) to operate on character count instead of byte offsets: check
args.text.chars().count() > 50 and build the truncated string by taking the
first 50 chars (e.g., via chars().take(50).collect()) then append "..." so you
never slice mid-codepoint and avoid panics.
- Around line 598-603: The code currently logs DOM extraction failures
(snapshot.error) but still sets state.snapshot and returns it, which caches bad
snapshots; update the logic in the function handling snapshot creation so that
if snapshot.error.is_some() you do NOT assign state.snapshot and instead return
an Err containing the extraction error (or map snapshot.error into your
function's error type) so callers see the failure; keep the tracing::warn! call
if desired but ensure state.snapshot = Some(snapshot) only happens when
snapshot.error.is_none(), referencing snapshot.error and state.snapshot to find
where to change the flow.
- Around line 1262-1290: The JavaScript evaluation path in call(...) doesn't
invalidate the cached snapshot, so after page.evaluate(...) succeeds you must
clear the page's cached snapshot/state to avoid stale DOM usage; locate the
active page via self.context.require_active_page(&state) (or directly via
self.context.state) and reset its snapshot/cache (e.g., set the page.snapshot or
cached_snapshot field to None or call the
page.invalidate_snapshot()/state.invalidate_snapshot_for_page(page_id) helper if
one exists) before returning the BrowserOutput so subsequent index-based
interactions use a fresh snapshot.
- Around line 849-858: The code double-navigates because
get_or_create_page(&self.context, &mut state, Some(&args.url)) calls
browser.new_page(target_url) and then page.goto(&args.url) runs again; change
the call to get_or_create_page to pass None for the URL so the helper won't
pre-navigate (i.e., call get_or_create_page(&self.context, &mut state, None) or
otherwise ensure browser.new_page is not given args.url), keep the single
explicit page.goto(&args.url) afterward, and leave
wait_for_page_ready(page).await as-is; update references to get_or_create_page,
browser.new_page, and page.goto accordingly.
- Around line 354-358: The current byte-slice truncation (&trimmed[..200]) can
panic on multi-byte UTF-8 characters; change the display creation to safely
truncate by characters: compute whether trimmed.chars().count() > 200, then
build a safe truncated string using
trimmed.chars().take(200).collect::<String>() and append "..." when truncated.
Replace the existing display logic (the display variable and its branches) to
use this char-based approach so no byte-slicing/panic occurs and behavior
remains deterministic.
- Around line 1461-1476: The code currently removes the Page from state.pages
before calling page.close(), which can leave the tab open in CDP but missing
from the in-memory map if close() fails; instead, first fetch the page without
removing it (e.g., via state.pages.get or similar), call page.close().await and
return the BrowserError on failure, and only after close succeeds remove the
page from state.pages and update state.active_target (if it was the closed id)
to the next key; finally call state.invalidate_snapshot() and return success.
Reference the page.close().await call and the state.pages.remove(&id) /
state.active_target update so you replace the remove-before-close behavior with
close-then-remove bookkeeping.
- Around line 1134-1142: The key-press handler currently calls
dispatch_key_press(...) but doesn't invalidate the cached DOM snapshot, so
subsequent index/selector operations may use stale state; after the await
dispatch_key_press(page, &args.key)? call in the async fn call (the method using
self.context.require_active_page and returning BrowserOutput::success), add a
call to clear the cached snapshot on the browser context (for example
self.context.invalidate_snapshot().await or state.clear_snapshot())—if no such
API exists, implement an invalidate_snapshot/clear_snapshot method on the
Context (or on the locked state) that drops/refreshes the cached snapshot and
await it here so the snapshot is invalidated immediately after the key press.
- Around line 1516-1523: The ClosePolicy::CloseTabs branch currently drains
state.pages before attempting page.close(), corrupting bookkeeping if any close
fails; instead, snapshot the pages while holding the lock (e.g., collect
identifiers and clones/references of the pages into a temporary Vec), then
release the lock and iterate calling page.close() on the snapshot, and only
after successful closes reacquire the lock to drain/remove those entries and set
state.active_target = None and call state.invalidate_snapshot(); reference
ClosePolicy::CloseTabs, state.pages, state.active_target,
state.invalidate_snapshot(), and page.close() when making this change.
- Around line 1052-1067: The current JS unconditionally replaces the element
value; change it so when args.clear is false it inserts at the cursor (or
appends if no cursor) instead of overwriting. Replace the simple
clear_js/assignment logic with a conditional JS block that: if args.clear is
true, sets el.value = {text_json}; else compute let start = el.selectionStart ??
el.value.length, end = el.selectionEnd ?? start, then set el.value =
el.value.slice(0, start) + {text_json} + el.value.slice(end), and set
el.selectionStart = el.selectionEnd = start + ({text_json}.length) to place the
caret after the inserted text; keep el.focus() and the input/change event
dispatching using the existing selector_json and text_json identifiers. Ensure
you update the variable used in format! (js string) rather than leaving the old
unconditional el.value assignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9c76510c-8de0-4faa-9be4-53b62403e77a
📒 Files selected for processing (2)
src/tools/browser.rssrc/tools/browser_extract_dom.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/tools/browser.rs (2)
52-80:⚠️ Potential issue | 🔴 CriticalBlock hostname-based SSRF, not just literal IPs.
This only rejects hosts that already parse as
IpAddr, sohttp://localhost,*.localhost, and any DNS name resolving to RFC1918/loopback/link-local space still pass the check. Because the browser follows redirects after this preflight, an external URL can also bounce into an internal target. If this is meant to be SSRF protection, resolve the hostname to all A/AAAA records and reject any non-public result, plus apply the same policy to redirects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 52 - 80, The current checks only reject literal IpAddr hosts; change the logic where you use parsed.host_str() and is_blocked_ip to also resolve the hostname to its A/AAAA records (and treat names like "localhost" and wildcarded names) and validate every resolved IP with is_blocked_ip, returning BrowserError::new(...) if any resolved address is blocked; also keep the existing bracketed-IPv6 handling but treat bracketed names by resolving the inner name if not a literal IP. Finally, ensure the same resolution+IP-check is applied to any redirect target in the navigation flow so redirects cannot lead to private/loopback addresses.
1620-1642:⚠️ Potential issue | 🟠 MajorPreserve browser state until
close()actually succeeds.This path clears
browser,pages, andactive_targetbefore awaitingbrowser.close(). If the close call fails, the process can still be running but the handle is gone, so the worker cannot retry, reconnect, or clean up the leaked browser anymore. Only clear the in-memory state after a successful close, or restore it on error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 1620 - 1642, The code is clearing state.browser, state._handler_task, state.user_data_dir, state.persistent_profile, state.pages, and state.active_target before awaiting browser.close(), which loses the handle if close() fails; change the logic in the block around state.lock().await so you only take (remove) the browser handle (and related fields) from state after close() succeeds or, on error, restore them back into state: i.e., acquire the browser Option into a local variable without mutating state (or clone/peek it), call browser.close().await while state still holds the authoritative values, and only after Ok(()) clear state.pages, active_target, user_data_dir, persistent_profile and drop state.browser; if browser.close() returns Err, do not clear state (or explicitly put the original browser and handler_task back into state) and propagate the error via BrowserError::new(message); ensure you still abort handler_task safely (abort after successful close or restore it on failure).
🤖 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/tools/browser.rs`:
- Around line 148-157: The readiness probe currently swallows all CDP/runtime
errors by calling .await.ok() and turning every failure into false; update the
code around page.evaluate (the expression assigning ready) to stop using .ok()
and instead handle the Result properly: propagate or return the error (or map
and log terminal errors) from the readiness check, distinguish transient
navigation/runtime errors (retry or treat as not-ready) from terminal errors
(log and propagate as a tool error), and remove the final unwrap_or(false) that
conceals failures so that page.evaluate failures surface as structured errors
the caller can act on.
---
Outside diff comments:
In `@src/tools/browser.rs`:
- Around line 52-80: The current checks only reject literal IpAddr hosts; change
the logic where you use parsed.host_str() and is_blocked_ip to also resolve the
hostname to its A/AAAA records (and treat names like "localhost" and wildcarded
names) and validate every resolved IP with is_blocked_ip, returning
BrowserError::new(...) if any resolved address is blocked; also keep the
existing bracketed-IPv6 handling but treat bracketed names by resolving the
inner name if not a literal IP. Finally, ensure the same resolution+IP-check is
applied to any redirect target in the navigation flow so redirects cannot lead
to private/loopback addresses.
- Around line 1620-1642: The code is clearing state.browser,
state._handler_task, state.user_data_dir, state.persistent_profile, state.pages,
and state.active_target before awaiting browser.close(), which loses the handle
if close() fails; change the logic in the block around state.lock().await so you
only take (remove) the browser handle (and related fields) from state after
close() succeeds or, on error, restore them back into state: i.e., acquire the
browser Option into a local variable without mutating state (or clone/peek it),
call browser.close().await while state still holds the authoritative values, and
only after Ok(()) clear state.pages, active_target, user_data_dir,
persistent_profile and drop state.browser; if browser.close() returns Err, do
not clear state (or explicitly put the original browser and handler_task back
into state) and propagate the error via BrowserError::new(message); ensure you
still abort handler_task safely (abort after successful close or restore it on
failure).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3c65376a-0da4-4ce8-9843-9af95e724aab
📒 Files selected for processing (2)
prompts/en/worker.md.j2src/tools/browser.rs
Move JSON.stringify inside the try/catch block so stringify errors (circular refs, size limits) are caught and reported. Add stack traces to the JS error fallback. Surface extraction errors to the LLM as tool errors instead of silently returning 0 elements. Add debug-level tracing of the raw CDP result type and size. This is diagnostic — we need to see why railway.com returns 0 elements despite having 92 interactive elements in the DOM.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (10)
src/tools/browser_extract_dom.js (2)
156-161:⚠️ Potential issue | 🟡 MinorLabel selector still uses unescaped ID.
Although
buildSelectorwas updated to useCSS.escape, thegetElementAccessibleNamefunction still uses rawelement.idin thelabel[for="..."]query. IDs containing quotes or special characters will break this selector.🛡️ Proposed fix to escape the ID
if (element.tagName === 'INPUT' || element.tagName === 'TEXTAREA' || element.tagName === 'SELECT') { const id = element.id; if (id) { - const label = document.querySelector('label[for="' + id + '"]'); + const escapedId = (window.CSS && CSS.escape) ? CSS.escape(id) : id; + const label = document.querySelector('label[for="' + escapedId + '"]'); if (label) return label.textContent || ''; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser_extract_dom.js` around lines 156 - 161, The label query in getElementAccessibleName uses raw element.id in document.querySelector('label[for="..."]'), which can break for IDs with quotes/special chars; update getElementAccessibleName to escape the id using CSS.escape(element.id) before building the attribute selector (i.e., use label[for=" + CSS.escape(id) + "] or equivalent) and keep the existing guard that skips when id is falsy so the selector is only used for non-empty ids.
429-434:⚠️ Potential issue | 🟡 MinorVisibility check logic may still include ARIA-hidden elements.
The condition
let visible = isElementVisibleForAria || isElementVisible(element)uses OR logic, which means an element that isaria-hidden="true"(whereisElementVisibleForAriareturnsfalse) could still be included if it has layout (bounding box > 0). This contradicts the ARIA snapshot contract wherearia-hiddenelements should be excluded.The
isElementHiddenForAriafunction correctly returnstrueforaria-hidden="true"elements, soisElementVisibleForAriawould befalse. However, the OR withisElementVisible(element)re-admits these elements if they have a visible bounding box.🛡️ Proposed fix to use AND logic
// Check visibility const isElementVisibleForAria = !isElementHiddenForAria(element); - let visible = isElementVisibleForAria || isElementVisible(element); + // Element must pass both ARIA visibility AND have layout + let visible = isElementVisibleForAria && isElementVisible(element); // Skip if not visible for ARIA if (!visible) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser_extract_dom.js` around lines 429 - 434, The visibility check currently uses OR which re-includes aria-hidden elements; update the logic around isElementHiddenForAria/isElementVisibleForAria and isElementVisible so that an element is considered visible only if it is visible for ARIA AND has a visible bounding box. Specifically change the computation of visible (and any subsequent checks that use it) to require both isElementVisibleForAria and isElementVisible, ensuring aria-hidden elements (as detected by isElementHiddenForAria) are excluded from the ARIA snapshot.src/tools/browser.rs (8)
700-706:⚠️ Potential issue | 🟡 MinorSilently discarded error when removing stale lock file.
Per coding guidelines, errors should not be silently discarded with
let _ =. While the lock file removal is best-effort, the error should at least be logged.🛡️ Proposed fix to log the error
if persistent_profile { let lock_file = user_data_dir.join("SingletonLock"); if lock_file.exists() { tracing::debug!(path = %lock_file.display(), "removing stale Chrome SingletonLock"); - let _ = std::fs::remove_file(&lock_file); + if let Err(error) = std::fs::remove_file(&lock_file) { + tracing::debug!(path = %lock_file.display(), %error, "failed to remove stale SingletonLock"); + } } }Based on learnings: "Never silently discard errors; no
let _ =on Results; handle, log, or propagate 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 700 - 706, The current persistent_profile block silently discards errors when calling std::fs::remove_file(&lock_file) using let _ = — change this to explicitly handle and log failures: replace the `let _ = std::fs::remove_file(&lock_file);` with an error-aware branch (e.g., if let Err(e) = std::fs::remove_file(&lock_file) { tracing::warn!(error=%e, path=%lock_file.display(), "failed to remove stale Chrome SingletonLock"); }) so the removal remains best-effort but any error is recorded; reference the existing lock_file variable and persistent_profile conditional when making the change.
1190-1191:⚠️ Potential issue | 🟠 MajorSame UTF-8 slicing issue in browser_type.
&args.text[..50]can panic if the text contains multi-byte UTF-8 characters and the 50th byte falls within a character.🛡️ Proposed fix
- let display_text = if args.text.len() > 50 { - format!("{}...", &args.text[..50]) - } else { - args.text.clone() + let display_text = { + let mut chars = args.text.chars(); + let prefix: String = chars.by_ref().take(50).collect(); + if chars.next().is_some() { + format!("{prefix}...") + } else { + prefix + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 1190 - 1191, The code slices UTF-8 string bytes with &args.text[..50] (in the display_text construction), which can panic on multi-byte characters; change the truncation to operate on characters instead, e.g. build the prefix with args.text.chars().take(50).collect::<String>() and use that in the format! call so display_text = if args.text.chars().count() > 50 { format!("{}...", args.text.chars().take(50).collect::<String>()) } else { args.text.clone() }, updating the code paths that reference display_text (and keeping the variable names: args.text and display_text).
1138-1168:⚠️ Potential issue | 🟠 Major
clear: falsestill overwrites existing value.When
args.clearisfalse, the code skipsel.value = ''but the next lineel.value = {text_json}still replaces the entire field contents. For prefilled inputs,clear: falseshould append/insert text rather than replacing it.🛡️ Proposed fix to append when clear is false
if (editable) {{ {clear_editable} - el.textContent = {text_json}; + if ({clear}) {{ + el.textContent = {text_json}; + }} else {{ + el.textContent += {text_json}; + }} }} else {{ {clear_js} - el.value = {text_json}; + if ({clear}) {{ + el.value = {text_json}; + }} else {{ + // Insert at cursor position or append + const start = el.selectionStart ?? el.value.length; + const end = el.selectionEnd ?? el.value.length; + el.value = el.value.slice(0, start) + {text_json} + el.value.slice(end); + el.selectionStart = el.selectionEnd = start + {text_json}.length; + }} }}This requires also passing the
clearboolean to the JS template:+ clear = args.clear,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 1138 - 1168, The JS injected into the `js` string always assigns `el.value = {text_json}` (and `el.textContent = {text_json}` for contentEditable) which replaces existing content even when `args.clear` is false; update the template generation so it knows the `clear` boolean and conditionally appends instead of overwriting: pass `args.clear` into the JS template and change the branches that set the element value (`el.textContent` and `el.value`) to use concatenation (e.g., `el.value += {text_json}` / `el.textContent += {text_json}`) when clear is false, and only use the emptying lines (`el.value = ''` or `el.textContent = ''`) followed by assignment when clear is true; adjust the `clear_editable`, `clear_js`, `text_json`/`selector_json` template placeholders used to build the `js` string so they reflect this conditional behavior.
367-368:⚠️ Potential issue | 🟠 MajorUTF-8 slicing can panic on multi-byte characters.
&trimmed[..200]uses byte indexing. Iftrimmedcontains multi-byte UTF-8 characters and the 200th byte falls within a character, this will panic at runtime. Per coding guidelines, tool implementations must return errors as structured results, not panics.🛡️ Proposed fix using char-boundary-safe truncation
let trimmed = text.trim(); if !trimmed.is_empty() { let indent = " ".repeat(depth); // Truncate long text for LLM context efficiency - let display = if trimmed.len() > 200 { - format!("{}...", &trimmed[..200]) - } else { - trimmed.to_string() + let display = { + let mut chars = trimmed.chars(); + let prefix: String = chars.by_ref().take(200).collect(); + if chars.next().is_some() { + format!("{prefix}...") + } else { + prefix + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 367 - 368, The code slices trimmed with &trimmed[..200] which can panic on multi-byte UTF-8 boundaries; change the truncation in the display assignment to a char-boundary-safe approach (e.g., take the first 200 characters using trimmed.chars().take(200).collect() or use char_indices to find a safe byte index) so it never panics and still produces a truncated string with "..." appended; update the logic around the display variable in src/tools/browser.rs (referring to trimmed and display) to return an Err result on failure paths rather than allowing any panic.
1234-1242:⚠️ Potential issue | 🟡 MinorKey presses may mutate DOM but snapshot is not invalidated.
browser_press_keydispatches keyboard events (Enter, Tab, Escape) that can trigger form submissions, navigation, or focus changes — all of which can mutate the DOM. However, the snapshot is not invalidated, leaving stale indices for subsequent interactions.🛡️ Proposed fix to invalidate snapshot after key press
async fn call(&self, args: Self::Args) -> Result<Self::Output, Self::Error> { - let state = self.context.state.lock().await; + let mut state = self.context.state.lock().await; let page = self.context.require_active_page(&state)?; dispatch_key_press(page, &args.key).await?; + state.invalidate_snapshot(); Ok(BrowserOutput::success(format!( "Pressed key '{}'", args.key ))) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 1234 - 1242, The call handler in async fn call (which acquires self.context.state, uses self.context.require_active_page and calls dispatch_key_press) can cause DOM changes but doesn't update the stored snapshot; after dispatch_key_press completes you must invalidate the cached snapshot/state so subsequent actions use a fresh DOM; add a call to the context snapshot-invalidation helper (e.g. a method on self.context such as invalidate_snapshot/clear_snapshot or similar that accepts the locked state) immediately after dispatch_key_press and before returning BrowserOutput::success so indices won't be stale.
1561-1568:⚠️ Potential issue | 🟡 MinorTab bookkeeping removed before close() succeeds.
The page is removed from
state.pages(line 1563) beforepage.close().awaitis called. If the CDP close fails, the tab remains open but disappears from the in-memory map, causingactive_targetto potentially point to a missing page.🛡️ Proposed fix — close before removing from map
async fn call(&self, args: Self::Args) -> Result<Self::Output, Self::Error> { let mut state = self.context.state.lock().await; let id = args .target_id .or_else(|| state.active_target.clone()) .ok_or_else(|| BrowserError::new("no active tab to close"))?; - let page = state - .pages - .remove(&id) - .ok_or_else(|| BrowserError::new(format!("no tab with target_id '{id}'")))?; + let page = state + .pages + .get(&id) + .ok_or_else(|| BrowserError::new(format!("no tab with target_id '{id}'")))?; page.close() .await .map_err(|error| BrowserError::new(format!("failed to close tab: {error}")))?; + state.pages.remove(&id); + if state.active_target.as_ref() == Some(&id) { state.active_target = state.pages.keys().next().cloned(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 1561 - 1568, The code removes the Page from state.pages before calling page.close(), which can leave an open tab untracked if close() fails; change the order so you call page.close().await first and only remove the entry from state.pages (or update active_target) after close succeeds. Concretely, obtain a mutable reference or temporarily take ownership without deleting (e.g., use entry API or clone/peek the Page), await page.close().await and map any error to BrowserError as before, and only then remove the page from state.pages (and clear/update active_target) to ensure bookkeeping stays consistent on failures.
884-889:⚠️ Potential issue | 🟠 MajorDouble navigation when creating a new page.
When there's no active page,
get_or_create_page(..., Some(&args.url))callsbrowser.new_page(target_url)which navigates to the URL, thenbrowser_navigateimmediately callspage.goto(&args.url)again. This duplicates the initial request and can cause side effects (analytics, form submissions) to fire twice.🛡️ Proposed fix — don't pass URL to get_or_create_page
- let page = get_or_create_page(&self.context, &mut state, Some(&args.url)).await?; + let page = get_or_create_page(&self.context, &mut state, None).await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 884 - 889, The code double-navigates because get_or_create_page(..., Some(&args.url)) causes browser.new_page(target_url) to navigate, then browser_navigate calls page.goto(&args.url) again; fix by calling get_or_create_page(&self.context, &mut state, None).await to avoid implicit navigation in browser.new_page, then keep the explicit page.goto(&args.url).await (with the existing BrowserError handling) so navigation only happens once; update the call site that currently passes Some(&args.url) to pass None and ensure get_or_create_page/new_page behavior remains unchanged.
161-170:⚠️ Potential issue | 🟡 MinorReadiness probe swallows all CDP/runtime errors.
The
.ok()call at line 167 converts every CDP/runtime error intoNone, making broken browser sessions indistinguishable from "page not ready yet". Per coding guidelines, errors should be handled or logged, not silently discarded. Consider at minimum logging failures before treating them as "not ready".🛡️ Proposed fix to log errors
let ready = page .evaluate( "(document.readyState === 'complete' && \ document.body && document.body.children.length > 0)", ) .await - .ok() + .map_err(|error| { + tracing::trace!(%error, "page readiness check failed"); + error + }) + .ok() .and_then(|result| result.value().cloned()) .and_then(|value| value.as_bool()) .unwrap_or(false);Based on learnings: "Never silently discard errors; no
let _ =on Results; handle, log, or propagate 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 161 - 170, The readiness probe currently swallows all CDP/runtime errors because the Result from page.evaluate(...).await is converted to None via .ok(); update the logic around the let ready = ... block (the page.evaluate call and subsequent .ok().and_then(...) chain) to explicitly handle the Result instead of .ok(): match or use map_err/log on the Err to record failures (include the evaluation expression "(document.readyState === 'complete' ...)" and any error details) and then fall back to false only after logging; ensure the variable name ready and the existing value.as_bool() extraction are preserved when unwrapping the successful path.
🧹 Nitpick comments (1)
src/tools/browser.rs (1)
1783-1785: Suppressed unused variable warning withlet _ =.While this particular usage (
let _ = context;) is for suppressing an unused variable warning (not discarding aResult), it's still a code smell. Consider using_contextas the parameter name instead to suppress the warning idiomatically.♻️ Proposed refactor
async fn get_or_create_page<'a>( - context: &BrowserContext, + _context: &BrowserContext, state: &'a mut BrowserState, url: Option<&str>, ) -> Result<&'a chromiumoxide::Page, BrowserError> { // ... - // Suppress the "unused variable" warning — we need `context` for the type - // signature to match the pattern used by the navigate tool. - let _ = context; - Ok(&state.pages[&target_id]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/browser.rs` around lines 1783 - 1785, The parameter `context` is currently unused and suppressed via `let _ = context;`; change the function signature to rename the parameter to `_context` (or `_context: Type`) so the compiler will treat it as intentionally unused and remove the `let _ = context;` suppression; update the function containing `context` (the navigate tool handler / function where `let _ = context;` appears) to use the new `_context` parameter name and delete the redundant suppression line.
🤖 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/tools/browser_extract_dom.js`:
- Around line 774-775: The file ends with an IIFE closure shown as "})())"
missing a terminating semicolon; update the IIFE's final token to include a
semicolon (e.g., change "})())" to "})());" or otherwise append ";" after the
final closing parenthesis so the immediately-invoked function expression (IIFE)
is explicitly terminated.
---
Duplicate comments:
In `@src/tools/browser_extract_dom.js`:
- Around line 156-161: The label query in getElementAccessibleName uses raw
element.id in document.querySelector('label[for="..."]'), which can break for
IDs with quotes/special chars; update getElementAccessibleName to escape the id
using CSS.escape(element.id) before building the attribute selector (i.e., use
label[for=" + CSS.escape(id) + "] or equivalent) and keep the existing guard
that skips when id is falsy so the selector is only used for non-empty ids.
- Around line 429-434: The visibility check currently uses OR which re-includes
aria-hidden elements; update the logic around
isElementHiddenForAria/isElementVisibleForAria and isElementVisible so that an
element is considered visible only if it is visible for ARIA AND has a visible
bounding box. Specifically change the computation of visible (and any subsequent
checks that use it) to require both isElementVisibleForAria and
isElementVisible, ensuring aria-hidden elements (as detected by
isElementHiddenForAria) are excluded from the ARIA snapshot.
In `@src/tools/browser.rs`:
- Around line 700-706: The current persistent_profile block silently discards
errors when calling std::fs::remove_file(&lock_file) using let _ = — change this
to explicitly handle and log failures: replace the `let _ =
std::fs::remove_file(&lock_file);` with an error-aware branch (e.g., if let
Err(e) = std::fs::remove_file(&lock_file) { tracing::warn!(error=%e,
path=%lock_file.display(), "failed to remove stale Chrome SingletonLock"); }) so
the removal remains best-effort but any error is recorded; reference the
existing lock_file variable and persistent_profile conditional when making the
change.
- Around line 1190-1191: The code slices UTF-8 string bytes with
&args.text[..50] (in the display_text construction), which can panic on
multi-byte characters; change the truncation to operate on characters instead,
e.g. build the prefix with args.text.chars().take(50).collect::<String>() and
use that in the format! call so display_text = if args.text.chars().count() > 50
{ format!("{}...", args.text.chars().take(50).collect::<String>()) } else {
args.text.clone() }, updating the code paths that reference display_text (and
keeping the variable names: args.text and display_text).
- Around line 1138-1168: The JS injected into the `js` string always assigns
`el.value = {text_json}` (and `el.textContent = {text_json}` for
contentEditable) which replaces existing content even when `args.clear` is
false; update the template generation so it knows the `clear` boolean and
conditionally appends instead of overwriting: pass `args.clear` into the JS
template and change the branches that set the element value (`el.textContent`
and `el.value`) to use concatenation (e.g., `el.value += {text_json}` /
`el.textContent += {text_json}`) when clear is false, and only use the emptying
lines (`el.value = ''` or `el.textContent = ''`) followed by assignment when
clear is true; adjust the `clear_editable`, `clear_js`,
`text_json`/`selector_json` template placeholders used to build the `js` string
so they reflect this conditional behavior.
- Around line 367-368: The code slices trimmed with &trimmed[..200] which can
panic on multi-byte UTF-8 boundaries; change the truncation in the display
assignment to a char-boundary-safe approach (e.g., take the first 200 characters
using trimmed.chars().take(200).collect() or use char_indices to find a safe
byte index) so it never panics and still produces a truncated string with "..."
appended; update the logic around the display variable in src/tools/browser.rs
(referring to trimmed and display) to return an Err result on failure paths
rather than allowing any panic.
- Around line 1234-1242: The call handler in async fn call (which acquires
self.context.state, uses self.context.require_active_page and calls
dispatch_key_press) can cause DOM changes but doesn't update the stored
snapshot; after dispatch_key_press completes you must invalidate the cached
snapshot/state so subsequent actions use a fresh DOM; add a call to the context
snapshot-invalidation helper (e.g. a method on self.context such as
invalidate_snapshot/clear_snapshot or similar that accepts the locked state)
immediately after dispatch_key_press and before returning BrowserOutput::success
so indices won't be stale.
- Around line 1561-1568: The code removes the Page from state.pages before
calling page.close(), which can leave an open tab untracked if close() fails;
change the order so you call page.close().await first and only remove the entry
from state.pages (or update active_target) after close succeeds. Concretely,
obtain a mutable reference or temporarily take ownership without deleting (e.g.,
use entry API or clone/peek the Page), await page.close().await and map any
error to BrowserError as before, and only then remove the page from state.pages
(and clear/update active_target) to ensure bookkeeping stays consistent on
failures.
- Around line 884-889: The code double-navigates because get_or_create_page(...,
Some(&args.url)) causes browser.new_page(target_url) to navigate, then
browser_navigate calls page.goto(&args.url) again; fix by calling
get_or_create_page(&self.context, &mut state, None).await to avoid implicit
navigation in browser.new_page, then keep the explicit
page.goto(&args.url).await (with the existing BrowserError handling) so
navigation only happens once; update the call site that currently passes
Some(&args.url) to pass None and ensure get_or_create_page/new_page behavior
remains unchanged.
- Around line 161-170: The readiness probe currently swallows all CDP/runtime
errors because the Result from page.evaluate(...).await is converted to None via
.ok(); update the logic around the let ready = ... block (the page.evaluate call
and subsequent .ok().and_then(...) chain) to explicitly handle the Result
instead of .ok(): match or use map_err/log on the Err to record failures
(include the evaluation expression "(document.readyState === 'complete' ...)"
and any error details) and then fall back to false only after logging; ensure
the variable name ready and the existing value.as_bool() extraction are
preserved when unwrapping the successful path.
---
Nitpick comments:
In `@src/tools/browser.rs`:
- Around line 1783-1785: The parameter `context` is currently unused and
suppressed via `let _ = context;`; change the function signature to rename the
parameter to `_context` (or `_context: Type`) so the compiler will treat it as
intentionally unused and remove the `let _ = context;` suppression; update the
function containing `context` (the navigate tool handler / function where `let _
= context;` appears) to use the new `_context` parameter name and delete the
redundant suppression line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0897d3e-e689-4b0f-9ed1-58ca57848984
📒 Files selected for processing (2)
src/tools/browser.rssrc/tools/browser_extract_dom.js
Switch from injected browser_extract_dom.js to Chrome's native Accessibility.getFullAXTree CDP call. The JS injection approach failed silently on complex SPA pages (railway.com returned 0 elements despite 92 interactive elements being present). Key changes: - Extract snapshot via page.execute(GetFullAxTreeParams) instead of page.evaluate(EXTRACT_DOM_JS) - Reconstruct tree from flat Vec<AxNode> using parent_id/child_ids - Walk through ignored AX nodes transparently to reach non-ignored interactive descendants (fixes the 0-element bug) - Store BackendNodeId per indexed element instead of CSS selectors - Resolve clicks via DOM.getBoxModel + Input.dispatchMouseEvent instead of fragile CSS selector queries - Resolve typing via DOM.resolveNode + Runtime.callFunctionOn - Delete browser_extract_dom.js (775 lines of JS removed)
jamiepine
changed the title
Workers using models with extended thinking could exit the agent loop with a Reasoning-only response (no Text, no ToolCall). The nudge gate in should_nudge_tool_usage only checked for Text and ToolCall variants, so a Reasoning-only response slipped through without triggering a nudge. The worker reached Done with an empty result string, producing no RESULT block for the channel to relay. Two mechanical guards: 1. should_nudge_tool_usage now treats responses with no Text and no ToolCall (e.g. reasoning-only) as nudge-worthy, sending the worker back to actually do the work. 2. Empty-result safety net in worker.rs: if the initial task loop produces an empty/whitespace result, the worker fails instead of reaching Done. Also strengthened prompt language around premature outcome signaling in worker.md.j2, set_status tool description, and parameter docs.
… transcript UI Tool nudge retries now track consecutive text-only responses instead of total nudges per prompt lifetime. Successful tool calls reset the counter via on_tool_result, so long-running workers (e.g. browser exploration with 12+ tool calls) no longer get killed after a brief narration blip. Add WorkerText process event to stream model reasoning text between tool calls to the dashboard in real-time. Extract ToolCall component with tool-specific renderers (browser, shell, file) and paired call/result matching. Remove duplicate status text injection from worker transcripts.
…s; unify live/transcript rendering
Parse structured shell results (stdout, stderr, exit_code) into distinct sections instead of dumping raw JSON. Non-zero exit codes show a red badge, stderr gets its own labeled section with appropriate coloring. Also split the generic file renderer into file_read/file_write/file_edit/file_list with tailored displays (diff-style old/new for edits, line range info for reads). Error detection now catches success:false, non-zero exit_code, and 'Toolset error:' prefixes so failed tools render with a red X instead of a green checkmark.
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
Completely rewrite the browser automation system. Replace the single
BrowserToolaction enum with 12 flat tools, replace JS DOM injection with Chrome's native CDP Accessibility API, and add secure credential entry via the secret store.Key Changes
12 flat tools replace single action enum
browser_launch,browser_navigate,browser_snapshot,browser_click,browser_type,browser_press_key,browser_screenshot,browser_evaluate,browser_tab_open,browser_tab_list,browser_tab_close,browser_close— each with its own struct, args, and tool definition. LLMs no longer need to pick an action variant and fill the right subset of fields.CDP Accessibility API replaces JS injection
browser_snapshotnow callsAccessibility.getFullAXTreevia CDP instead of injecting 775 lines of JavaScript. The JS injection approach failed silently on complex SPA pages (railway.com returned 0 elements despite 92 interactive elements being present). The CDP approach uses Chrome's own computed accessibility tree, which works reliably on all pages.The flat
Vec<AxNode>is reconstructed into a tree usingparent_id/child_ids. Ignored AX nodes (structural DOM wrappers) are walked through transparently to reach their non-ignored interactive descendants — this was the critical fix that made the tree actually populate.BackendNodeId replaces CSS selectors
Element interaction uses
BackendNodeId(stored per indexed element) instead of fragile generated CSS selectors. Clicks resolve viaDOM.getBoxModel→ center coordinates →Input.dispatchMouseEvent. Typing resolves viaDOM.resolveNode→Runtime.callFunctionOn. The CSS selector fallback path is retained for theselectorparameter.Secure credential entry
browser_typeaccepts asecretparameter that resolves credentials from the secret store server-side. The actual password/token value never appears in tool arguments, output, or LLM context. This replaces the pattern of workers shelling out toecho $PASSWORDand passing values in plain text.Snapshot caching
AxSnapshotis cached onBrowserStateand invalidated on mutations (navigate, click, tab switch). Repeatedbrowser_snapshotcalls without intervening mutations return the cached result.Consecutive nudge tracking fixes long-running worker kills
The tool nudge retry budget now tracks consecutive text-only responses instead of total nudges per prompt lifetime. Whenever a tool call completes successfully,
on_tool_resultresets thenudge_attemptscounter to zero. This fixes a bug where long-running browser workers (e.g. 12+ tool calls exploring Railway settings) would get killed by the nudge system after a single narration blip, because earlier nudge attempts had already exhausted the retry budget. The invariant is now "2 consecutive text-only responses = stalled", not "2 total text-only responses ever".WorkerText event + transcript UI refactor
ProcessEvent::WorkerTextstreams model reasoning text between tool calls to the dashboard in real-timeToolCallcomponent with tool-specific renderers (browser, shell, file, set_status) and paired call/result matching viacall_idset_statusalready appears as a paired tool_started/tool_completed eventFiles Changed
src/tools/browser.rsBrowserContextwith optionalSecretsStore,AxSnapshot/SnapshotNodetypes built from CDPAxNode, YAML rendering,BackendNodeId-based element resolution, securesecretparameter onbrowser_type.src/tools/browser_extract_dom.jsgetFullAXTree.src/tools.rssrc/tools/set_status.rsStatusKinddocs.src/hooks/spacebot.rsnudge_attemptsto atomic on hook, reset on tool completion. WorkerText event emission. Clippy fixes (collapsible if, clone on Copy). 2 new unit tests.src/lib.rsProcessEvent::WorkerTextvariant.src/agent/channel_history.rsWorkerTextfrom channel event matching.src/agent/cortex.rsWorkerTextin cortex signal mapping.src/api/state.rsWorkerTextto SSE broadcast.src/api/system.rs"worker_text"SSE event type.docs/design-docs/tool-nudging.mdinterface/src/api/client.tsWorkerTextEventtype.interface/src/components/ToolCall.tsxinterface/src/hooks/useLiveContext.tsxworker_textevents, remove duplicate status injection.interface/src/routes/AgentWorkers.tsxToolCallcomponent.prompts/en/worker.md.j2prompts/en/tools/browser_description.md.j2secretparameter.prompts/en/tools/set_status_description.md.j2docs/content/docs/(features)/browser.mdxdocs/content/docs/(configuration)/secrets.mdxTesting
just gate-prgreen