feat(app): add embedded-browser automation tools for the agent (#1186) - #1212
feat(app): add embedded-browser automation tools for the agent (#1186)#1212Astro-Han wants to merge 2 commits into
Conversation
Give the in-process agent six tools to drive the PawWork embedded browser: browser_navigate, browser_screenshot, browser_extract, browser_wait, browser_click, browser_type. They act on the live, logged-in WebContentsView the user sees, not a separate headless fetch. Mechanism: a BrowserBridge registrable port lives in the opencode server (exported from node.ts). The Electron main process — where the WebContentsView controllers live — injects a controller-backed implementation at startup (registerBrowserAutomationBridge, right after the in-process server spawns). Because the server is imported in-process by Electron main, a tool calling the bridge is a direct in-process call, not IPC. This is the inverse of the usual main -> server direction. - opencode: BrowserBridge port + 6 Tool.define tools; tools gated to the desktop client in the registry; new `browser` permission key (a Rule, so it can be scoped per target) defaulting to allow, matching the local-self-use threat model (the browser the user opened for the agent shouldn't be walled off behind per-call prompts). - desktop-electron: controller automation methods (capturePage, page scripts via executeJavaScript, synthetic input via sendInputEvent) built on pure, unit-tested script builders; ipc/browser.ts hoists the per-window controller map to module scope and exposes resolveAutomationController (focused / sole window, else a typed error). - ui: a shared browser tool card plus icon/title/subtitle entries in the toolInfo map, so a call reads as "Open Page — example.com" instead of the generic "Called browser_navigate". Verification: opencode tool unit tests (mock bridge), desktop-electron logic + options tests, ui tool-info + tool-contract tests, `bun run typecheck` across the workspace, `bun run snap browser-tools`, and a `bun run dev:desktop` boot confirming the bridge registers and the app reaches ready without error.
|
Warning Review limit reached
More reviews will be available in 45 minutes and 8 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds a complete embedded-browser automation system. Desktop Electron exposes browser controller methods (navigate, screenshot, extract, wait, click, type) that execute JavaScript and input events, wired through a bridge contract to six new opencode tools, gated by permissions, and rendered in the UI with i18n support. ChangesBrowser Automation System
Sequence Diagram(s)sequenceDiagram
participant Agent as opencode Agent
participant Tool as BrowserNavigate Tool
participant Bridge as BrowserBridge Registry
participant Controller as BrowserViewController
participant Renderer as WebView Renderer
Agent->>Tool: execute(url)
Tool->>Tool: validateURL + checkAvailable
Tool->>Tool: ctx.ask(permission)
Tool->>Bridge: Bridge.navigate(url)
Bridge->>Controller: navigateAndReport(url)
Controller->>Renderer: executeJavaScript(page.url)
Renderer->>Controller: {url, title}
Controller->>Bridge: {url, title}
Bridge->>Tool: result
Tool->>Agent: {output, metadata}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
github-actions
Bot
added
app
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/desktop-electron/src/main/browser/automation-bridge.ts, packages/desktop-electron/src/main/browser/controller.ts, packages/desktop-electron/src/main/browser/logic.test.ts, packages/desktop-electron/src/main/browser/logic.ts, packages/desktop-electron/src/main/env.d.ts, packages/desktop-electron/src/main/index.ts, packages/desktop-electron/src/main/ipc/browser.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces embedded-browser automation tools (navigate, screenshot, extract, wait, click, and type) to the PawWork desktop application, implementing an in-process BrowserBridge to connect the agent tools with Electron's WebContentsView controllers. The review feedback highlights several critical robustness improvements for the Electron main process. These include filtering out hidden or devtools windows when resolving the active window, adding defensive guards to prevent crashes if the WebContents is destroyed during asynchronous operations, and ensuring that synchronous errors in the automation bridge are properly converted into promise rejections.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export function resolveAutomationController(): BrowserViewController { | ||
| const focused = BrowserWindow.getFocusedWindow() | ||
| if (focused && !focused.isDestroyed()) return ensureControllerForWindow(focused) | ||
| const windows = BrowserWindow.getAllWindows().filter((win) => !win.isDestroyed()) |
There was a problem hiding this comment.
BrowserWindow.getAllWindows() returns all active browser windows, including hidden background windows, utility windows, or undocked devtools windows. If any such window exists (which is very common in Electron apps), windows.length === 1 will evaluate to false when the main window is unfocused, causing the agent to throw a NoBrowserWindowError even if there is only one user-visible application window.
To ensure the agent can reliably automate the browser even when the application is in the background, filter the windows to only include visible, non-devtools windows.
const windows = BrowserWindow.getAllWindows().filter((win) => {
if (win.isDestroyed() || !win.isVisible()) return false
const url = win.webContents.getURL()
if (url.startsWith("devtools://") || url.startsWith("chrome-devtools://")) return false
return true
})| async navigateAndReport(input: string): Promise<{ url: string; title: string }> { | ||
| const url = parseNavigable(input) | ||
| if (!url) throw new Error("URL must start with http:// or https://") | ||
| await this.loadInternal(url) | ||
| return { url: this.wc.getURL(), title: this.wc.getTitle() } | ||
| } |
There was a problem hiding this comment.
If the window or webContents is destroyed before or during the asynchronous navigation, calling this.wc.getURL() or this.wc.getTitle() will throw a fatal "Object has been destroyed" exception.
Adding defensive guards before and after the asynchronous loadInternal call ensures that we handle destruction gracefully.
async navigateAndReport(input: string): Promise<{ url: string; title: string }> {
if (this.destroyed || this.wc.isDestroyed()) throw new Error("Browser view is destroyed")
const url = parseNavigable(input)
if (!url) throw new Error("URL must start with http:// or https://")
await this.loadInternal(url)
if (this.destroyed || this.wc.isDestroyed()) throw new Error("Browser view was destroyed during navigation")
return { url: this.wc.getURL(), title: this.wc.getTitle() }
}| async captureScreenshot(): Promise<{ mime: string; base64: string; width: number; height: number }> { | ||
| const image = await this.wc.capturePage() | ||
| const size = image.getSize() | ||
| return { mime: "image/png", base64: image.toPNG().toString("base64"), width: size.width, height: size.height } | ||
| } |
There was a problem hiding this comment.
If the window or webContents is destroyed before calling capturePage, calling this.wc.capturePage() will throw a fatal "Object has been destroyed" exception.
Adding a defensive guard at the start of the method ensures that we handle destruction gracefully.
| async captureScreenshot(): Promise<{ mime: string; base64: string; width: number; height: number }> { | |
| const image = await this.wc.capturePage() | |
| const size = image.getSize() | |
| return { mime: "image/png", base64: image.toPNG().toString("base64"), width: size.width, height: size.height } | |
| } | |
| async captureScreenshot(): Promise<{ mime: string; base64: string; width: number; height: number }> { | |
| if (this.destroyed || this.wc.isDestroyed()) throw new Error("Browser view is destroyed") | |
| const image = await this.wc.capturePage() | |
| const size = image.getSize() | |
| return { mime: "image/png", base64: image.toPNG().toString("base64"), width: size.width, height: size.height } | |
| } |
| async extractText( | ||
| selector: string | undefined, | ||
| maxChars: number, | ||
| ): Promise<{ url: string; title: string; text: string; truncated: boolean }> { | ||
| const raw = await this.wc.executeJavaScript(buildExtractScript(selector), true) | ||
| const text = typeof raw === "string" ? raw : "" | ||
| const truncated = text.length > maxChars | ||
| return { | ||
| url: this.wc.getURL(), | ||
| title: this.wc.getTitle(), | ||
| text: truncated ? text.slice(0, maxChars) : text, | ||
| truncated, | ||
| } | ||
| } |
There was a problem hiding this comment.
If the webContents is destroyed during the asynchronous executeJavaScript call, calling this.wc.getURL() or this.wc.getTitle() will throw a fatal "Object has been destroyed" exception.
Adding defensive guards before and after the asynchronous call ensures that we handle destruction gracefully.
async extractText(
selector: string | undefined,
maxChars: number,
): Promise<{ url: string; title: string; text: string; truncated: boolean }> {
if (this.destroyed || this.wc.isDestroyed()) throw new Error("Browser view is destroyed")
const raw = await this.wc.executeJavaScript(buildExtractScript(selector), true)
if (this.destroyed || this.wc.isDestroyed()) throw new Error("Browser view was destroyed during text extraction")
const text = typeof raw === "string" ? raw : ""
const truncated = text.length > maxChars
return {
url: this.wc.getURL(),
title: this.wc.getTitle(),
text: truncated ? text.slice(0, maxChars) : text,
truncated,
}
}| async clickSelector(selector: string): Promise<{ matched: boolean; x: number; y: number }> { | ||
| const rect = await this.wc.executeJavaScript(buildClickRectScript(selector), true) | ||
| const point = clickPointFromRect(rect) | ||
| if (!point) return { matched: false, x: 0, y: 0 } | ||
| this.wc.sendInputEvent({ type: "mouseDown", x: point.x, y: point.y, button: "left", clickCount: 1 }) | ||
| this.wc.sendInputEvent({ type: "mouseUp", x: point.x, y: point.y, button: "left", clickCount: 1 }) | ||
| return { matched: true, x: point.x, y: point.y } | ||
| } |
There was a problem hiding this comment.
If the webContents is destroyed before or during the asynchronous executeJavaScript call, calling sendInputEvent will throw a fatal exception.
Adding defensive guards before and after the asynchronous call ensures that we handle destruction gracefully.
async clickSelector(selector: string): Promise<{ matched: boolean; x: number; y: number }> {
if (this.destroyed || this.wc.isDestroyed()) throw new Error("Browser view is destroyed")
const rect = await this.wc.executeJavaScript(buildClickRectScript(selector), true)
if (this.destroyed || this.wc.isDestroyed()) throw new Error("Browser view was destroyed during click")
const point = clickPointFromRect(rect)
if (!point) return { matched: false, x: 0, y: 0 }
this.wc.sendInputEvent({ type: "mouseDown", x: point.x, y: point.y, button: "left", clickCount: 1 })
this.wc.sendInputEvent({ type: "mouseUp", x: point.x, y: point.y, button: "left", clickCount: 1 })
return { matched: true, x: point.x, y: point.y }
}| async typeText( | ||
| selector: string | undefined, | ||
| text: string, | ||
| submit: boolean, | ||
| ): Promise<{ matched: boolean; submitted: boolean }> { | ||
| if (selector) { | ||
| const focused = await this.wc.executeJavaScript(buildFocusScript(selector), true).catch(() => false) | ||
| if (focused !== true) return { matched: false, submitted: false } | ||
| } | ||
| for (const char of text) this.wc.sendInputEvent({ type: "char", keyCode: char }) | ||
| if (submit) { | ||
| this.wc.sendInputEvent({ type: "keyDown", keyCode: "Return" }) | ||
| this.wc.sendInputEvent({ type: "keyUp", keyCode: "Return" }) | ||
| } | ||
| return { matched: true, submitted: submit } | ||
| } |
There was a problem hiding this comment.
If the webContents is destroyed before or during the asynchronous executeJavaScript call, calling sendInputEvent will throw a fatal exception.
Adding defensive guards before and after the asynchronous call ensures that we handle destruction gracefully.
async typeText(
selector: string | undefined,
text: string,
submit: boolean,
): Promise<{ matched: boolean; submitted: boolean }> {
if (this.destroyed || this.wc.isDestroyed()) throw new Error("Browser view is destroyed")
if (selector) {
const focused = await this.wc.executeJavaScript(buildFocusScript(selector), true).catch(() => false)
if (focused !== true) return { matched: false, submitted: false }
}
if (this.destroyed || this.wc.isDestroyed()) throw new Error("Browser view was destroyed during typing")
for (const char of text) this.wc.sendInputEvent({ type: "char", keyCode: char })
if (submit) {
this.wc.sendInputEvent({ type: "keyDown", keyCode: "Return" })
this.wc.sendInputEvent({ type: "keyUp", keyCode: "Return" })
}
return { matched: true, submitted: submit }
}| navigate: ({ url }) => resolveAutomationController().navigateAndReport(url), | ||
| screenshot: () => resolveAutomationController().captureScreenshot(), | ||
| extract: ({ selector, maxChars }) => resolveAutomationController().extractText(selector, maxChars), | ||
| waitFor: ({ selector, text, timeoutMs }) => resolveAutomationController().waitFor(selector, text, timeoutMs), | ||
| click: ({ selector }) => resolveAutomationController().clickSelector(selector), | ||
| type: ({ selector, text, submit }) => resolveAutomationController().typeText(selector, text, submit), |
There was a problem hiding this comment.
If resolveAutomationController() throws a synchronous error (such as NoBrowserWindowError), calling these bridge methods will throw synchronously instead of returning a rejected promise. This can bypass promise-based error handling or cause unexpected synchronous exceptions in the caller.
Marking the bridge implementation methods as async ensures that any synchronous exceptions thrown during window resolution are safely converted into standard promise rejections.
| navigate: ({ url }) => resolveAutomationController().navigateAndReport(url), | |
| screenshot: () => resolveAutomationController().captureScreenshot(), | |
| extract: ({ selector, maxChars }) => resolveAutomationController().extractText(selector, maxChars), | |
| waitFor: ({ selector, text, timeoutMs }) => resolveAutomationController().waitFor(selector, text, timeoutMs), | |
| click: ({ selector }) => resolveAutomationController().clickSelector(selector), | |
| type: ({ selector, text, submit }) => resolveAutomationController().typeText(selector, text, submit), | |
| navigate: async ({ url }) => resolveAutomationController().navigateAndReport(url), | |
| screenshot: async () => resolveAutomationController().captureScreenshot(), | |
| extract: async ({ selector, maxChars }) => resolveAutomationController().extractText(selector, maxChars), | |
| waitFor: async ({ selector, text, timeoutMs }) => resolveAutomationController().waitFor(selector, text, timeoutMs), | |
| click: async ({ selector }) => resolveAutomationController().clickSelector(selector), | |
| type: async ({ selector, text, submit }) => resolveAutomationController().typeText(selector, text, submit), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/e2e/snap/browser-tools.snap.ts`:
- Line 9: The fixturePath produced by fileURLToPath may contain Windows
backslashes which break the dynamic browser import using `/@fs/${fixturePath}`;
before constructing that import URL, normalize fixturePath to POSIX separators
(e.g., replace backslashes with forward slashes or use path.posix behavior) and
use the normalized value in the import string. Update the code around the
fixturePath variable and where `/@fs/${fixturePath}` is used (the dynamic import
in the test) so the browser always receives a forward-slash URL independent of
platform.
In `@packages/desktop-electron/src/main/browser/controller.ts`:
- Around line 182-187: navigateAndReport currently calls loadInternal which
swallows loadURL rejections, causing navigateAndReport to return stale
wc.getURL()/getTitle(); change loadInternal (the helper that calls
webContents.loadURL) to propagate/rethrow any errors from loadURL (do not catch
or swallow them) or return a failing Promise so that await
this.loadInternal(url) in navigateAndReport will throw on navigation failure;
ensure navigateAndReport does not catch that error and only returns { url:
this.wc.getURL(), title: this.wc.getTitle() } when loadInternal completes
successfully, so browser_navigate consumers receive real failures instead of
stale metadata.
In `@packages/opencode/src/tool/browser/tools.ts`:
- Around line 230-233: The no-match message currently interpolates
params.selector and shows "undefined" when selector is omitted; update the
output for the false branch (the object property that uses result.matched) to
conditionally render the selector: use params.selector ? `No element matched
${params.selector}.` : `No element matched.` so that when params.selector is
absent the user sees a clean "No element matched." message (refer to the object
fields using result.matched and params.selector in tools.ts).
In `@packages/opencode/test/tool/browser.test.ts`:
- Around line 1-159: The tests use raw bun:test and a custom exec(...) that runs
Effects with Effect.runPromise; migrate to the project test harness by
importing/creating const it = testEffect(...) near the top, replace
describe/test with the harness (use it instead of test), and convert each test
body to return an Effect via Effect.gen(function* () { ... }) rather than
awaiting exec(...); inside those Effects call the existing exec(...) helper but
remove Effect.runPromise (have exec return an Effect instead) or adapt exec to
return an Effect so tests yield it; update imports to include testEffect and
ensure BrowserBridge.unregister() remains in afterEach. Reference: exec,
BrowserBridge, BrowserNavigateTool, BrowserScreenshotTool, BrowserExtractTool,
BrowserWaitTool, BrowserClickTool, BrowserTypeTool.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 943e1cd0-bb2f-4e65-8704-318a404e8268
📒 Files selected for processing (25)
packages/app/e2e/snap/browser-tools.snap.tspackages/app/e2e/snap/fixtures/browser-tools-fixture.tsxpackages/desktop-electron/src/main/browser/automation-bridge.tspackages/desktop-electron/src/main/browser/controller.tspackages/desktop-electron/src/main/browser/logic.test.tspackages/desktop-electron/src/main/browser/logic.tspackages/desktop-electron/src/main/env.d.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/ipc/browser.tspackages/opencode/src/agent/agent.tspackages/opencode/src/config/permission.tspackages/opencode/src/node.tspackages/opencode/src/tool/browser/bridge.tspackages/opencode/src/tool/browser/tools.tspackages/opencode/src/tool/registry.tspackages/opencode/test/tool/browser.test.tspackages/ui/src/components/message-part/tools/browser.tsxpackages/ui/src/components/message-part/tools/index.tspackages/ui/src/components/tool-contract.test.tspackages/ui/src/components/tool-contract.tspackages/ui/src/components/tool-info.test.tspackages/ui/src/components/tool-info.tspackages/ui/src/i18n/en.tspackages/ui/src/i18n/zh.tspackages/ui/src/i18n/zht.ts
| test.use({ viewport: { width: 520, height: 420 }, deviceScaleFactor: 2 }) | ||
|
|
||
| const LANGUAGE_KEY = "pawwork.global.dat:language" | ||
| const fixturePath = fileURLToPath(new URL("./fixtures/browser-tools-fixture.tsx", import.meta.url)) |
There was a problem hiding this comment.
Normalize the /@fs fixture path to POSIX separators before browser import.
fileURLToPath(...) returns backslashes on Windows, and passing that raw value into /@fs/${fixturePath} can break the dynamic import URL in Line 36. Normalize the path first so this snapshot test stays cross-platform.
Suggested patch
-const fixturePath = fileURLToPath(new URL("./fixtures/browser-tools-fixture.tsx", import.meta.url))
+const fixturePath = fileURLToPath(new URL("./fixtures/browser-tools-fixture.tsx", import.meta.url)).replaceAll("\\", "/")
...
- await page.evaluate(async (path) => {
+ await page.evaluate(async (path) => {
const mod = await import(path)
mod.mountBrowserToolsFixture(document.body)
}, `/@fs/${fixturePath}`)Also applies to: 33-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/app/e2e/snap/browser-tools.snap.ts` at line 9, The fixturePath
produced by fileURLToPath may contain Windows backslashes which break the
dynamic browser import using `/@fs/${fixturePath}`; before constructing that
import URL, normalize fixturePath to POSIX separators (e.g., replace backslashes
with forward slashes or use path.posix behavior) and use the normalized value in
the import string. Update the code around the fixturePath variable and where
`/@fs/${fixturePath}` is used (the dynamic import in the test) so the browser
always receives a forward-slash URL independent of platform.
| async navigateAndReport(input: string): Promise<{ url: string; title: string }> { | ||
| const url = parseNavigable(input) | ||
| if (!url) throw new Error("URL must start with http:// or https://") | ||
| await this.loadInternal(url) | ||
| return { url: this.wc.getURL(), title: this.wc.getTitle() } | ||
| } |
There was a problem hiding this comment.
Propagate navigation failures instead of reporting stale page metadata.
navigateAndReport on Line 185 uses loadInternal, which swallows loadURL rejections. On failed loads, Line 186 can return the previous page URL/title, and downstream browser_navigate output becomes incorrect.
Suggested fix
async navigateAndReport(input: string): Promise<{ url: string; title: string }> {
const url = parseNavigable(input)
if (!url) throw new Error("URL must start with http:// or https://")
- await this.loadInternal(url)
+ await this.wc.loadURL(url)
return { url: this.wc.getURL(), title: this.wc.getTitle() }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async navigateAndReport(input: string): Promise<{ url: string; title: string }> { | |
| const url = parseNavigable(input) | |
| if (!url) throw new Error("URL must start with http:// or https://") | |
| await this.loadInternal(url) | |
| return { url: this.wc.getURL(), title: this.wc.getTitle() } | |
| } | |
| async navigateAndReport(input: string): Promise<{ url: string; title: string }> { | |
| const url = parseNavigable(input) | |
| if (!url) throw new Error("URL must start with http:// or https://") | |
| await this.wc.loadURL(url) | |
| return { url: this.wc.getURL(), title: this.wc.getTitle() } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/desktop-electron/src/main/browser/controller.ts` around lines 182 -
187, navigateAndReport currently calls loadInternal which swallows loadURL
rejections, causing navigateAndReport to return stale wc.getURL()/getTitle();
change loadInternal (the helper that calls webContents.loadURL) to
propagate/rethrow any errors from loadURL (do not catch or swallow them) or
return a failing Promise so that await this.loadInternal(url) in
navigateAndReport will throw on navigation failure; ensure navigateAndReport
does not catch that error and only returns { url: this.wc.getURL(), title:
this.wc.getTitle() } when loadInternal completes successfully, so
browser_navigate consumers receive real failures instead of stale metadata.
| title: result.matched ? "Typed" : "No match", | ||
| output: result.matched | ||
| ? `Typed ${params.text.length} character(s)${params.selector ? ` into ${params.selector}` : ""}${result.submitted ? " and submitted." : "."}` | ||
| : `No element matched ${params.selector}.`, |
There was a problem hiding this comment.
Handle missing selector in browser_type no-match output.
Line 233 renders undefined when selector is omitted (No element matched undefined.), which is incorrect user-visible output.
💡 Suggested fix
return {
title: result.matched ? "Typed" : "No match",
output: result.matched
? `Typed ${params.text.length} character(s)${params.selector ? ` into ${params.selector}` : ""}${result.submitted ? " and submitted." : "."}`
- : `No element matched ${params.selector}.`,
+ : params.selector
+ ? `No element matched ${params.selector}.`
+ : "No focused element matched.",
metadata: { matched: result.matched, submitted: result.submitted, selector: params.selector },
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/src/tool/browser/tools.ts` around lines 230 - 233, The
no-match message currently interpolates params.selector and shows "undefined"
when selector is omitted; update the output for the false branch (the object
property that uses result.matched) to conditionally render the selector: use
params.selector ? `No element matched ${params.selector}.` : `No element
matched.` so that when params.selector is absent the user sees a clean "No
element matched." message (refer to the object fields using result.matched and
params.selector in tools.ts).
| import { afterEach, describe, expect, test } from "bun:test" | ||
| import path from "path" | ||
| import { Effect, Layer } from "effect" | ||
| import { Agent } from "../../src/agent/agent" | ||
| import { Truncate } from "../../src/tool/truncate" | ||
| import { Instance } from "../../src/project/instance" | ||
| import { BrowserBridge } from "../../src/tool/browser/bridge" | ||
| import { | ||
| BrowserClickTool, | ||
| BrowserExtractTool, | ||
| BrowserNavigateTool, | ||
| BrowserScreenshotTool, | ||
| BrowserTypeTool, | ||
| BrowserWaitTool, | ||
| } from "../../src/tool/browser/tools" | ||
| import { MessageID, SessionID } from "../../src/session/schema" | ||
|
|
||
| const projectRoot = path.join(import.meta.dir, "../..") | ||
|
|
||
| const ctx = { | ||
| sessionID: SessionID.make("ses_test"), | ||
| messageID: MessageID.make("message"), | ||
| callID: "", | ||
| agent: "build", | ||
| abort: AbortSignal.any([]), | ||
| messages: [], | ||
| metadata: () => Effect.void, | ||
| ask: () => Effect.void, | ||
| } | ||
|
|
||
| // A bridge stub that records the last input so each test can assert what the tool | ||
| // forwarded, then returns a canned result. Tests register the slice they need. | ||
| function stubBridge(overrides: Partial<BrowserBridge.Impl>): { calls: Record<string, unknown> } { | ||
| const calls: Record<string, unknown> = {} | ||
| const record = | ||
| <K extends keyof BrowserBridge.Impl>(key: K, fn: BrowserBridge.Impl[K]): BrowserBridge.Impl[K] => | ||
| ((input: never) => { | ||
| calls[key] = input | ||
| return (fn as (i: never) => unknown)(input) | ||
| }) as BrowserBridge.Impl[K] | ||
| const base: BrowserBridge.Impl = { | ||
| navigate: async () => ({ url: "about:blank", title: "" }), | ||
| screenshot: async () => ({ mime: "image/png", base64: "", width: 0, height: 0 }), | ||
| extract: async () => ({ url: "about:blank", title: "", text: "", truncated: false }), | ||
| waitFor: async () => ({ found: false, waitedMs: 0, reason: "timeout" }), | ||
| click: async () => ({ matched: false, x: 0, y: 0 }), | ||
| type: async () => ({ matched: false, submitted: false }), | ||
| } | ||
| const merged = { ...base, ...overrides } as BrowserBridge.Impl | ||
| const wrapped: BrowserBridge.Impl = { | ||
| navigate: record("navigate", merged.navigate), | ||
| screenshot: record("screenshot", merged.screenshot), | ||
| extract: record("extract", merged.extract), | ||
| waitFor: record("waitFor", merged.waitFor), | ||
| click: record("click", merged.click), | ||
| type: record("type", merged.type), | ||
| } | ||
| BrowserBridge.register(wrapped) | ||
| return { calls } | ||
| } | ||
|
|
||
| function exec<P>(tool: typeof BrowserNavigateTool | any, args: P) { | ||
| return Instance.provide({ | ||
| directory: projectRoot, | ||
| fn: () => | ||
| tool.pipe( | ||
| Effect.flatMap((info: any) => info.init()), | ||
| Effect.flatMap((t: any) => t.execute(args, ctx)), | ||
| Effect.provide(Layer.mergeAll(Truncate.defaultLayer, Agent.defaultLayer)), | ||
| Effect.runPromise, | ||
| ), | ||
| }) | ||
| } | ||
|
|
||
| afterEach(() => BrowserBridge.unregister()) | ||
|
|
||
| describe("tool.browser", () => { | ||
| test("is unavailable when no implementation is registered", async () => { | ||
| expect(BrowserBridge.available()).toBe(false) | ||
| await expect(exec(BrowserScreenshotTool, {})).rejects.toThrow() | ||
| }) | ||
|
|
||
| test("navigate forwards the url and reports the landed page", async () => { | ||
| const { calls } = stubBridge({ navigate: async ({ url }) => ({ url: `${url}/`, title: "Example" }) }) | ||
| const result = await exec(BrowserNavigateTool, { url: "https://example.com" }) | ||
| expect(calls.navigate).toEqual({ url: "https://example.com" }) | ||
| expect(result.output).toContain("https://example.com/") | ||
| expect(result.output).toContain("Example") | ||
| expect(result.metadata).toMatchObject({ url: "https://example.com/", pageTitle: "Example" }) | ||
| }) | ||
|
|
||
| test("navigate rejects a non-web url before touching the bridge", async () => { | ||
| stubBridge({}) | ||
| await expect(exec(BrowserNavigateTool, { url: "file:///etc/passwd" })).rejects.toThrow() | ||
| }) | ||
|
|
||
| test("screenshot returns a base64 png file attachment", async () => { | ||
| stubBridge({ screenshot: async () => ({ mime: "image/png", base64: "QUJD", width: 800, height: 600 }) }) | ||
| const result = await exec(BrowserScreenshotTool, {}) | ||
| expect(result.output).toContain("800") | ||
| expect(result.output).toContain("600") | ||
| expect(result.attachments?.length).toBe(1) | ||
| expect(result.attachments?.[0].type).toBe("file") | ||
| expect(result.attachments?.[0].mime).toBe("image/png") | ||
| expect(result.attachments?.[0].url).toBe("data:image/png;base64,QUJD") | ||
| expect(result.attachments?.[0]).not.toHaveProperty("id") | ||
| expect(result.attachments?.[0]).not.toHaveProperty("sessionID") | ||
| }) | ||
|
|
||
| test("extract clamps maxChars and surfaces the truncated flag", async () => { | ||
| const { calls } = stubBridge({ | ||
| extract: async ({ maxChars }) => ({ | ||
| url: "https://a/", | ||
| title: "A", | ||
| text: "body text", | ||
| truncated: maxChars < 100, | ||
| }), | ||
| }) | ||
| const result = await exec(BrowserExtractTool, { maxChars: 5 }) | ||
| expect(calls.extract).toMatchObject({ maxChars: 5 }) | ||
| expect(result.output).toBe("body text") | ||
| expect(result.metadata).toMatchObject({ truncated: true }) | ||
| }) | ||
|
|
||
| test("wait requires a selector or text", async () => { | ||
| stubBridge({}) | ||
| await expect(exec(BrowserWaitTool, {})).rejects.toThrow() | ||
| }) | ||
|
|
||
| test("wait reports a satisfied selector", async () => { | ||
| stubBridge({ waitFor: async () => ({ found: true, waitedMs: 120, reason: "selector" }) }) | ||
| const result = await exec(BrowserWaitTool, { selector: ".ready" }) | ||
| expect(result.title).toBe("Wait satisfied") | ||
| expect(result.metadata).toMatchObject({ found: true, reason: "selector" }) | ||
| }) | ||
|
|
||
| test("click reports a match with coordinates", async () => { | ||
| const { calls } = stubBridge({ click: async () => ({ matched: true, x: 12, y: 34 }) }) | ||
| const result = await exec(BrowserClickTool, { selector: "#go" }) | ||
| expect(calls.click).toEqual({ selector: "#go" }) | ||
| expect(result.output).toContain("(12, 34)") | ||
| expect(result.metadata).toMatchObject({ matched: true }) | ||
| }) | ||
|
|
||
| test("click reports no match", async () => { | ||
| stubBridge({ click: async () => ({ matched: false, x: 0, y: 0 }) }) | ||
| const result = await exec(BrowserClickTool, { selector: ".missing" }) | ||
| expect(result.title).toBe("No match") | ||
| expect(result.metadata).toMatchObject({ matched: false }) | ||
| }) | ||
|
|
||
| test("type forwards text and submit and reports submission", async () => { | ||
| const { calls } = stubBridge({ type: async () => ({ matched: true, submitted: true }) }) | ||
| const result = await exec(BrowserTypeTool, { selector: "#q", text: "hello", submit: true }) | ||
| expect(calls.type).toEqual({ selector: "#q", text: "hello", submit: true }) | ||
| expect(result.output).toContain("submitted") | ||
| expect(result.metadata).toMatchObject({ matched: true, submitted: true }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Use the repo’s testEffect(...) harness for these Effect-based tests.
This file currently uses raw bun:test + a custom exec(...) runtime path; for packages/opencode/test/**/*.test.{ts,tsx} the project standard is const it = testEffect(...) with test bodies in Effect.gen(function* () { ... }). Please migrate this file to that pattern to stay aligned with the test harness contract.
As per coding guidelines, “Define const it = testEffect(...) near the top of the test file and keep the test body inside Effect.gen(function* () { ... }).”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/test/tool/browser.test.ts` around lines 1 - 159, The tests
use raw bun:test and a custom exec(...) that runs Effects with
Effect.runPromise; migrate to the project test harness by importing/creating
const it = testEffect(...) near the top, replace describe/test with the harness
(use it instead of test), and convert each test body to return an Effect via
Effect.gen(function* () { ... }) rather than awaiting exec(...); inside those
Effects call the existing exec(...) helper but remove Effect.runPromise (have
exec return an Effect instead) or adapt exec to return an Effect so tests yield
it; update imports to include testEffect and ensure BrowserBridge.unregister()
remains in afterEach. Reference: exec, BrowserBridge, BrowserNavigateTool,
BrowserScreenshotTool, BrowserExtractTool, BrowserWaitTool, BrowserClickTool,
BrowserTypeTool.
Source: Coding guidelines
…1186) Address two P2 findings from Codex review of the browser automation tools: - Wildcard `browser: deny` now hides all six browser tools from the model. The model-facing list filters via Permission.disabled(), which maps a tool id to a permission key; the browser tools' ids (browser_navigate, ...) didn't match the shared `browser` key, so a deny left them listed but failing at execution. Map them to `browser` the same way edit tools map to `edit`. - browser_click scrolls with behavior "instant" so a page's CSS scroll-behavior: smooth can't leave getBoundingClientRect reading a stale rect and clicking off-viewport while still reporting matched: true. Tests: permission disabled mapping (wildcard deny hides, allow / scoped deny do not) and a click-script assertion for the instant scroll.
|
Closing per maintainer decision. The in-process, selector-based tool design here is solid code but a generation behind current browser-agent practice: the model authors raw CSS selectors with no observe/snapshot step and no self-verifying action results. Rather than ship it and redo it later, we are changing direction: drive the embedded WebContentsView through a CDP endpoint and reuse opencli existing CDP bridge, which already supports an Electron/explicit endpoint and needs no Chrome extension. The BrowserBridge port, permission wiring, and UI scaffolding are kept for the redo. Tracking under #1186. |
1 participant
Summary
Adds six tools that let the in-process agent drive the PawWork embedded browser:
browser_navigate,browser_screenshot,browser_extract,browser_wait,browser_click,browser_type. They act on the live, logged-inWebContentsViewthe user already sees — not a separate headless fetch.The mechanism is an in-process bridge, the inverse of the usual main → server direction:
BrowserBridgeregistrable port lives in the opencode server (exported fromnode.ts). The tools call it.WebContentsViewcontrollers — injects a controller-backed implementation at startup (registerBrowserAutomationBridge, right after the in-process server spawns). Because the server is imported in-process by Electron main, a tool calling the bridge is a direct in-process call, not IPC.Layer breakdown:
BrowserBridgeport + 6Tool.definetools; tools gated to thedesktopclient in the registry; newbrowserpermission key (aRule, so it can be scoped per target) defaulting to allow.capturePage, page reads viaexecuteJavaScript, synthetic input viasendInputEvent) built on pure, unit-tested script builders;ipc/browser.tshoists the per-window controller map to module scope and exposesresolveAutomationController(focused / sole window, else a typed error).toolInfomap, so a call reads as "Open Page — example.com" instead of the generic "Called browser_navigate".Why
Issue #1186 PR2. PR1 shipped the embedded browser panel (login, navigation, partition). This PR makes that browser actionable by the agent so it can read and interact with pages the user is signed into. Default-allow matches the agreed local-self-use threat model: the browser the user opened for the agent should not be walled off behind a per-call prompt. The bridge is shaped as a registrable port so the OpenCLI / token-gated CDP endpoint can plug into the same seam in a later phase without reworking the tools.
Related Issue
#1186
Human Review Status
PendingReview Focus
opencode/src/tool/browser/bridge.ts,desktop-electron/.../browser/automation-bridge.ts, thenode.tsexport, and the hand-maintainedenv.d.tsdeclaration staying in sync with the real port.ipc/browser.ts: hoisting the controller map to module scope andresolveAutomationController's window selection (focused → sole → typed error).browserpermission inagent.tsand the newbrowserkey inconfig/permission.ts— confirm this matches the intended threat model.controller.ts(clickPointFromRectcenter +sendInputEvent; char-by-char typing + Return on submit).Risk Notes
browserpermission key, defaultallow. Intentional (local-self-use threat model). Users can still scope it per target since it is aRule.OPENCODE_CLIENT === "desktop", and the bridge implementation is registered only by the desktop main process; cli/app/headless never expose them.capturePage/executeJavaScript/sendInputEvent). Verified on macOS viadev:desktop; Windows not separately exercised in this PR.capturePageon aWebContentsViewthat has never been painted (panel never shown) may return a blank or last-committed frame; screenshots are reliable once the panel is visible.ci-smoke-cdp), and a full agent-loop E2E land with the later phase that adds that endpoint. Called out so the missing E2E is visible, not silent.How To Verify
Codex review: two P2 findings, both fixed in follow-up commits — (1) a wildcard
browser: denynow hides all six tools from the model (thePermission.disabledid→key mapping), and (2)browser_clickscrolls withbehavior: "instant"so a smooth-scroll page can't leave it clicking a stale rect.Screenshots or Recordings
bun run snap browser-toolsrenders the six tool cards (grid atdocs/design/preview/screenshots/browser-tools.png, reviewed). Each shows the localized title and its target subtitle: 打开网页 —https://news.ycombinator.com/, 网页截图, 提取文本 —main article, 等待 —.results, 点击 —button[type=submit], 输入 —input[name=q]. The leading "browser" family icon is supplied by the trow summary viatoolIcon()and pinned intool-info.test.ts.Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.