Repository navigation
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks sound overall. The local-server path (sameMachine uses the persistent partition directly, with per-page file: grants by webContentsId) and the link routing (plain clicks inside [data-pane-row]; the file handler now skips http(s)) fit the existing host/plugin boundary. I could not run it: it all runs in the Electron desktop app, which isn't available here. bun test src/browser passes (205) and tsc --noEmit is clean for gui-plugins and app. The packages' typecheck script fails here with Script not found "check", so I ran tsc directly.
Public API: the plugin SDK, server HTTP API, config and CLI are unchanged. Additions are internal to the built-in browser plugin: a preferences store (links, default on), a BrowserProfile Ipc, a persist:opencode-browser partition and a browser-extensions folder under the app's user data.
Main concern: mirroring the whole profile's cookies into remote servers' partitions (inline comment). Before/after screenshots or a short video of the Settings › Browser page would help, since I could not render it.
Fixes #60313
Fixes #53580
Fixes #55730
Related: #52419, #62989
|
|
||
| if (!shared) { | ||
| handle.network = yield* createBrowserNetwork({ rpc, attachment, location: options.location, partition }) | ||
| // Sign-ins carry over both ways, so a page of a remote server is signed in where the profile is. |
There was a problem hiding this comment.
This copies every cookie in the profile into each remote server's partition, including sign-ins imported from the user's personal Chrome, Safari or Firefox, and syncs changes both ways. Pages there load through that server's tunnel, and the agent on it can drive and browser.evaluate those pages. So any remote server the user attaches to gets the user's full set of personal sign-ins, not just the ones made for the work. Cookies for plain http: sites also cross the tunnel unencrypted. Local servers already had this level of access, but a remote one may belong to someone else. Could remote partitions stay unlinked by default, or ask once per server before mirroring?
…ign Browser settings
There was a problem hiding this comment.
The new commits look sound. I could not run them: they change only the desktop pane and settings, and the desktop app can't run here. bun test src/browser passes (206) and tsc --noEmit is clean for gui-plugins.
Public API is still unchanged. The internal additions are a welcomed field in the browser plugin's preferences store and an optional icon on the BrowserProfile Ipc's sources.
The earlier point about mirroring all profile cookies into remote servers' partitions is not addressed yet; that thread stays open. Nothing new to raise.
|
In addition to having a default, shared browser profile, it would be good for the user to be able to specify the profile in order to work concurrently on things that may compete for browser state. |
|
hmm cool idea @esafak.. I'll explore that |
…tach grant Attaching a desktop browser writes browser:* and skill:browser allows into the session's permissions. Session rules are evaluated after agent rules, so the grant overrode a browser deny from config, an agent definition (for example a "*" deny) or the session itself. Browser tools never called the permission check either, so tool.use policies did not apply. The plugin now records each agent's rules before it appends its own default deny. A browser call is refused when those rules, plus the session's rules without the grant, deny the browser, decided the way core's tool filter decides. The call then runs the permission check with the operation as its resource, so permission.evaluate hooks such as tool.use policies apply. Sessions without a deny behave as before: the grant allows the check, so nothing prompts. Refs #54044, #53294, #56869
A key the schema did not declare was dropped silently: input repair deletes undeclared keys of a closed object, and Effect structs ignore excess keys. A call such as evaluate({ tabID, expression }) then failed with 'script: Missing key' and no hint that expression was the problem. Every browser tool now fails with 'Not a parameter of browser.X. Valid parameters: ...'.
…el return and await
Models passed expression (and code) where evaluate required script; every such call failed. Scripts with a top-level return or await failed with a SyntaxError in the page. The server now folds expression and code into script, and sends a script that does not compile as a classic script but does as an async function body as (async () => { ... })(). Scripts that compile today reach the page unchanged.
…alse browser.wait without condition was 2,138 of 3,099 browser errors across four devices. Models write Playwright-style keys: text with timeout, state, selector, url, time, ms. wait now reads the condition from whichever key is present (none means load), accepts timeout for timeoutMs, shortens waits above 30 s instead of rejecting them, pauses for delayMs, time or ms, and polls for a selector, ref or URL from the server with actions every desktop already supports. A condition that does not hold in time returns met: false with what the page showed instead of throwing. Waits also no longer fail when the document changed since the server last saw the tab.
…rent A child session copies its parent's permissions, so a subagent spawned from an attached session got the browser:* and skill:browser allows but no attachment. It listed the browser tools and the Browser skill, every call failed with [browser.disconnected], and the copied allow outlived the parent's attachment. The plugin now revokes the grant from a child session while the child's prompt is admitted (session prompt hook), before any step of the child can list the tools, so there is no race with its first step. It holds the attachment lock while it checks and writes, and skips a child that has an attachment of its own. A subagent whose agent denies the browser stays denied even when the desktop attaches to it, through the check from the previous commit. Refs #61238
…illed The attach grant is a stored session rule, but its revoke runs only from the plugin's finalizer. After SIGKILL, a crash or a stop timeout, the session kept browser:* and skill:browser allows with no attachment behind them, so the next server listed the browser tools and every call failed with [browser.disconnected]. Each grant is now recorded in plugin storage with the server process that wrote it (host and process ID) and its location, before the grant is written; a successful revoke removes the record. When the plugin starts for a location, and before it registers the browser RPC so no attach can race it, it revokes the grants recorded for that location by a process that no longer runs. Several server processes can share one database (a desktop's server and a manual `opencode serve` both use opencode.db), so a sweep of every session in the location could revoke another server's live grant. The sweep skips records of a running process and of another host, which cannot be checked. Grants written before this change have no record and are left alone; the next attach and detach of their session removes them. The startup sweep before RPC registration, its location scoping and ignoring per-session failures come from #61639. Fixes #61609 Co-authored-by: Chen Yufeiyang <fei76161@gmail.com>
- A session with no desktop no longer says 'Repeating browser actions while disconnected will not help' (#55795): it says the browser runs in the desktop app, to tell the user, and to continue without it until they open the session there. - A closed or foreign tabID lists the session's open tabs (at most ten), so the next call can use one. - A local path or file: URL passed to tabs.open or navigate is named and answered with browser.preview({ path }) or serving it over HTTP; other invalid URLs are named too. - The reconnect advice no longer asks the model to reconnect the desktop, which it cannot do.
…no desktop is connected preview acknowledged dispatch before the pane opened anything, so a path that existed nowhere returned success (#61625). The server now resolves the path the way the Review pane does (workspace-relative or absolute, file:// URLs, :line and #L suffixes, and the one workspace file whose path ends with it) and fails with the resolved path when no file is there or it is a folder. Without a desktop app, preview now says the file was not shown and gives its absolute path for the model to pass to the user, instead of the generic disconnected advice (28 of the 38 disconnected errors on this machine were previews).
The skill and tool descriptions now show evaluate with return and await, wait with its implied conditions, met: false and delayMs, preview as the way to show a local HTML file, and what to do on disconnected and tab_unavailable errors. Optional parameters such as tabs.open focus, frameID and limit had descriptions the model never saw: optional() does not carry annotations to the encoded JSON Schema, so they now sit on the inner schema.
metadata.toolCalls kept only { tool, status, input }. When a program caught an inner call's failure, its message was lost: 30% of browser errors in the recorded sessions have no message, and analysis had to guess the cause from the input. Each failed row now carries error, bounded to 2,000 characters. Interrupted calls stay without one.
A page whose view starts hidden, and the renderer a cross-site navigation starts in a hidden tab, never renders: the backgroundThrottling preference does not wake it. Re-apply it on every commit so hidden tabs keep rendering frames. Viewport and element screenshots now copy the tab's last rendered frame (capturePage with stayHidden), which works while the tab is hidden, instead of refusing unless the Review pane shows it. They wait for a pending main-frame navigation and a fresh frame first, so a capture never shows the page a redirect left. Full-page captures still need the tab on screen and say so. Click and hover no longer force a capturePage after scrolling, which threw UnknownVizError on hidden tabs; they wait for a rendered frame instead. The capture error no longer advises browser.tabs.focus, which did not help.
# Conflicts: # packages/gui-plugins/src/browser/model.ts
… badge and current tab
A minimized window or a hidden app on macOS draws no frames, so capturePage never settled and the tool timed out after 60 s with advice to check the desktop connection. Refuse at once in those states, bound every capture attempt by one 2.5 s deadline, and say the window must be restored. Windows keeps capturing minimized windows.
app.getFileIcon answers by file type on macOS, so every browser showed the generic application icon (Safari a document icon). Draw the bundle's Quick Look thumbnail at its resolved path instead; a failure still falls back to the glyph.
Importing from a Chromium browser on macOS reads its Safe Storage key through /usr/bin/security, so the system prompt names 'security', and its Always Allow would let any process read that key without asking. While the prompt is open, the sign-in offer and Settings now say what macOS asks and why, and to choose Allow, not Always Allow; a denied prompt says the same. Naming OpenCode in the prompt needs an in-process Keychain read, which needs a native module.
With cookie encryption on, Chromium writes no cookie to disk while the system key store refuses its key (a denied Keychain prompt, a locked keyring), so imported sign-ins vanished at the next start without a word. The profile now writes one short-lived cookie at start and looks for it in its database. When it is missing, the profile refuses imports, and Settings and the sign-in offer say that sign-ins cannot be saved and why.
Show calls its children once, and the count was a plain string built there, so a new summary never reached the page until Settings opened again. Render the count as an expression that tracks the summary.
There was a problem hiding this comment.
I could not run the new commits: they all run in the desktop app, which can't run here. bun test src/browser passes (261) and tsc --noEmit is clean for gui-plugins. plugin-browser still has the one preview-path failure on Linux (site\\index.html keeps its backslash in files.ts).
The earlier point about mirroring all profile cookies into remote servers' partitions is not addressed yet; that thread stays open.
New: these commits add about 2,500 lines that rebuild parts of Chrome's extension APIs in the desktop app's main process: chrome.tabs, windows, cookies, contextMenus, webNavigation, notifications, storage.sync, permissions and action/popups, plus a preload script in every frame of the profile. Caller identity comes from the frame's address or the worker's scope, and chrome.cookies checks host permissions, so I found no blocker. Still, this is a large, security-sensitive surface that stands on its own, apart from the profile, link routing and import. With the pull request now at 85 files, a separate pull request for it would be much easier to review and revert.
Public API: unchanged by these commits.
Hona
changed the title
There was a problem hiding this comment.
The new commits look sound. I could not run them: they change only desktop browser behaviour, and the desktop app can't run here. bun test src/browser passes (264) and tsc --noEmit is clean for gui-plugins. plugin-browser still has the one preview-path failure on Linux (site\\index.html keeps its backslash in files.ts).
The earlier point about mirroring all profile cookies into remote servers' partitions is not addressed yet; that thread stays open.
Public API: unchanged by these commits. Nothing new to raise.
There was a problem hiding this comment.
The new commits look sound. The preview-path failure on Linux is fixed: plugin-browser tests now all pass (121), and tsc --noEmit is clean for plugin-browser and gui-plugins. The compatibility-table change is a refactor with no change in behaviour.
The earlier point about mirroring all profile cookies into remote servers' partitions is not addressed yet; that thread stays open.
Public API: unchanged by these commits. Nothing new to raise.
The desktop's in-app browser was a signed-out, in-memory pane per attachment. Agents drove it through tools that failed on one call in five and ignored a configured
browserdeny. After this PR it is one persistent, encrypted profile with imported sign-ins, Chrome extensions and in-pane links. Its agent tools follow the user's permission rules and work on tabs that are not on screen.persist:opencode-browserprofile for the whole appUnknownVizErrorEnableCookieEncryptionoff)demo.mp4
The video shows the original profile, import and extension flow. It was recorded before the extension APIs, tool, permission and macOS work below.
Why
Measured tool-call errors
Browser calls from the session databases of four devices (adais and blade on Windows, air on macOS, vps on Linux with a remote desktop): 14,974 calls, 3,099 errors.
waitwithoutcondition(Playwright-styletext,timeout,state,time)screenshot"needs a visible tab"evaluatewithoutscript(expression,code)[browser.disconnected], no desktop attachedreturnandawaitnow run; other script errors unchangedtoolCalls[].errorfile:)previewclickUnknownVizErrorcapturePage)tab_unavailablewaitcondition not metmet: falseevaluate,tabs.open,preview60 s timeoutswaitand 141evaluateerrors become successful calls, and about 160 more become one precise message.waitgroup, every device was at 6.8 to 9.1%.Community issues
previewfile now fails), browser: sessions with no current attachment get a 'will not help' error, and browser.tabs.list throws it too #55795 (new no-desktop wording; tools still listed).Architecture
flowchart LR click["plain click on an http(s) link"] --> links["Links.open"] links -->|"browser LinkHandler"| tab["pane tab"] links -->|"modifier key or no handler"| system["system browser"] tab --> local{"server on this machine?"} local -->|yes| profile["persist:opencode-browser<br/>cookies, site data, extensions"] local -->|no| remote["per-attachment partition<br/>server tunnel"] remote <-->|"cookie mirror, both ways"| profile profile --> ext["extensions<br/>Electron APIs + profile/extension-api"] ext --> popup["popup over the pane"] agent["agent browser.* call"] --> refuse["refuse: configured deny"] refuse --> lookup["tab and connection lookup"] lookup --> authorize["authorize: permission.assert<br/>ask, tool.use policy, Allow always"] authorize --> gate{"extension page?"} gate -->|yes| refused["refused"] gate -->|no| tabProfile and sign-in import
One profile:
PARTITION = "persist:opencode-browser"inpackages/gui-plugins/src/browser/profile/index.ts. Servers on this machine browse in it directly.Remote servers: keep their own partition, because Chromium sets the proxy per partition.
link()copies every profile cookie into it and mirrors changes both ways. Site storage and extensions stay with the profile.User agent: the profile drops the
Electron/…and app tokens, which sign-in pages such as Google's refuse.Import sources (
profile/sources.ts,chromium.ts,firefox.ts,safari.ts,cookie.ts):cookies.sqlite)Cookies.binarycookies, needs Full Disk Access)cookies.txt, Chrome-API JSON, Playwright storage stateOffer in the pane:
import-prompt.tsxshows a bar (a card on the new tab page) for the most recently used browser until the user imports or clicks "Not now". Settings › Browser is rebuilt around sign-ins, extensions and site data (settings.tsx).Chrome and Edge on Windows: most cookies use app-bound
v20keys that only the browser can unwrap. The import counts them and says to export with a cookie extension instead. A lockedCookiesdatabase (browser still running) is named too.macOS Keychain: the Chromium key is read with
/usr/bin/security, so macOS names "security" in the prompt. While it is open, the app says what it asks and to choose Allow, not Always Allow.Cannot save sign-ins: at start,
cookiesPersistwrites one probe cookie (cookie-check.opencode.invalid), flushes and counts it in a copy of theCookiesdatabase. If it is missing (Keychain key denied), Settings and the offer show a warning and imports are off. Extensions cannot see the probe cookie.macOS icons: browser icons come from the Quick Look thumbnail of the resolved bundle path.
Links
createWebLinks(packages/app/src/runtime/plugin/render.tsx) offers a plain left click on anhttp(s)link inside[data-pane-row]toLinks.open. Modifier clicks,downloadlinks and links in dialogs keep the default.LinkHandler(browser/renderer.tsx, priority 5) takes it when the pane is available for that session and the setting is on. The file plugin no longer claims web links.terminal/terminal.tsx).Extensions
Install: a Chrome Web Store or Edge Add-ons link (CRX3, developer key pinned so the store ID is kept), a copy from a Chromium profile, or an unpacked folder. Loaded with
allowFileAccess: false.Verdict:
profile/compatibility.tsreads the manifest and returnssupported,partial(with the features that may not work) orunsupported. Unsupported only when a background needs an API with events nothing stands in for (readingList,tabGroups,gcm,declarativeContent,debugger,ttsEngine). An unsupported one needs "Add anyway".Results in a Windows dev build:
chrome.storage.syncERR_BLOCKED_BY_CLIENT)Popups:
extension-api/popup.tsanchors a view under the extensions button, sized frompreferred-size-changedwithin 25x25 to 800x600. It closes on blur, Escape, and window blur, move or resize.API modules (
profile/extension-api/): a preload registered for frames and service workers defines the namespaces only inchrome-extension://documents. Main answers only the extension's own pages and worker, and only for namespaces its manifest has permission for.tabs,windows(removerefuses),action/browserAction/pageAction, popup surface,webNavigation,contextMenus(in the tab's right-click menu),cookies(host permissions only),notifications,storage.sync(this device only, pages and workers)permissions(answers from the manifest;requestgrants nothing optional),commands(getAllonly, no shortcuts)sidePanel,identity,history,bookmarks,sessions,downloads,search,fontSettings,privacy.*(stubs.ts)storage.local/session,runtime,scripting,alarms,idle,offscreen,declarativeNetRequest, MV2webRequestnativeMessaging, DevTools panels,storage.syncin content scripts, MV3 workerwebRequestlisteners,userScripts,tabGroupsfile:guard and request blocking: anysession.webRequestlistener makes Electron skip the extensions' request proxy, so blockers saw nothing.protocol.handle("file")does not work either: with an extension listening it never runs and outside files load.profile/file-guard.tsandfile-guard-machine.tsnow register the listener only while afile:document can load something:file:document's loaders are created after its navigation starts, and the listener stays until no frame can use them.file:page is open, extensions do not block requests on other pages.Agent browser tools
Server side in
packages/plugin-browser/src/{rpc,tools,files,machine,skill}.ts. No wire or protocol change: an older desktop works with this server.wait {tabID, text, timeout}condition: Missing keywait {tabID, text, state: "visible"}hiddengivestextGone)wait {tabID, selector, timeout}wait {tabID, timeout}wait {tabID, time}/{ms}timeup to 60 counts as seconds)wait {condition: "text"}that timed outmet: falsewithobservedevaluate {tabID, expression}/{code}script: Missing keyevaluatewith top-levelreturn/awaitSyntaxErrorfind {tabID, query}text: Missing keyquery: Not a parameter of browser.find. Valid parameters: tabID, frameID, text.previewof a missing pathbrowser.preview found no file at <absolute path>previewwith no desktopUnknown parameters: each input is
StructWithRest(Struct(fields), [Record(not a field, always fails)]). The JSON Schema dropsadditionalProperties: false, so core's input repair no longer deletes the guessed key before validation.Descriptions on optional parameters now reach the model's JSON Schema (they were on the type side only).
Code Mode:
packages/core/src/codemode/tool.tsstores a failed inner call's message astoolCalls[].error(up to 2,000 characters), also when the script catches it.Hidden tabs (
gui-plugins/src/browser/chromium.ts,capture.ts,page-machine.ts):did-navigatereappliessetBackgroundThrottling(false), so a renderer that starts hidden still paints.capturePage(rect, { stayHidden: true })after any pending navigation and a fresh frame, under one 2.5 s deadline.click,hoveranddragwait for a double rAF instead of a forcedcapturePage.fullPagekeeps the CDP path and fails with a clear message when the tab is not on screen.undrawable()fails a screenshot at once when the window is minimized or the app is hidden.UnknownVizErrorUnknownVizErrorfocus: falseUnknownVizErrorUnknownVizErrorUnknownVizErrorUnknownVizErrorCells are screenshot / click. Hover matches click after the change. Captures were checked as current after a click, a timer and a redirect; macOS captures are at 2x.
Permissions
The attach grant is a session rule (
browser:* allow) that core evaluates after every agent rule. It still exists; the plugin now decides from the rules the user configured.permission.evaluatehook (connection.ts) selects the rule for the operation from config, the agent definition and the session's own non-grant rules, with core's last-match semantics (selectBrowserRuleinmachine.ts). Deny gives[browser.denied]; ask gives the normal prompt. A read failure fails closed.tool.usepolicies onbrowsernow run, because every call goes throughpermission.assert.save: [operation].packages/core/src/permission.tslets a saved approval answer anaskthat a hook raised; denies are evaluated first and never overridden.prompthook removes the inherited grant from a child with no attachment of its own, before its first prompt is admitted.reconcileStaleGrantsrevokes grants whose process is gone on this host. Another host's or a live process's grants stay. Builds on fix(plugin-browser): revoke stale browser grants on startup #61639 by @feiiiiii5.BrowserPage.executerequiresorigin: "agent" | "user". For agent calls,agentRefusal(policy.ts) refuses navigation to extension URLs and everything except navigate, back, forward and stop on an extension document. Extension frames are dropped from frames, snapshots and contexts, and a call that names an extension frame'sframeIDis refused. Screenshots still show their pixels.Cookie encryption
resetAdHocDarwinSignaturere-signs unsigned macOS builds after the flip, or Apple Silicon refuses to launch them. Signed builds are signed after the fuses anyway.Notes for review
file:guard with extensions installed, the tool-call shapes, hidden tabs, deny / ask / Allow always, and the extension-page refusal, all over the real tool path). macOS dev and packed beta build on Apple Silicon (hidden tabs, links, import offer, Safari import, JSON Formatter, verdicts, fuse Enabled, "cannot save sign-ins"). Linux not tested.v10) on macOS, and in thepersist:partition on any OS.chrome.cookieswith a real extension; context menus with a real right-click.ask(including"*": "ask") now prompts for browser calls while a desktop is attached.waitreturnsmet: falseinstead of throwing; scripts that used the throw as "not found" now continue.askfor every action, not only browser.SecItemCopyMatching, with signing and theRunAsNodefuse off.packages/plugin-browser. It would move thewait,evaluateand schema work and drop the grant, subagent and ledger fixes.Follow-ups
connected: false, when no desktop is attached (browser: sessions with no current attachment get a 'will not help' error, and browser.tabs.list throws it too #55795).storage.syncin content scripts, native messaging, DevTools panels.packages/desktop/script/pack.tswritesapp-update.ymlafter signing, which breaks the local bundle seal.Added by the opencode bot (AI-generated).
Fixes #53580
Related: #52419
Public API: a saved "always" approval now overrides a permission.evaluate hook's ask for every action; browser tools honour configured browser rules and reject unknown parameters; codemode toolCalls gain optional error