From e7009452b49984bb024fb69443aae6360e8a62d2 Mon Sep 17 00:00:00 2001 From: can1357 Date: Tue, 28 Jul 2026 06:40:31 +0200 Subject: [PATCH] feat(coding-agent/tools): simplified browser screenshot persistence and return paths - Remove the per-call `save` option from `tab.screenshot()` to simplify usage. - Update `tab.screenshot()` to return the saved file path as a promise string. - Configure screenshot persistence to use daemon path or custom `browser.screenshotDir`. - Add comprehensive tests verifying temp path return and custom directory saving. --- docs/tools/browser.md | 12 +- packages/coding-agent/CHANGELOG.md | 2 + .../coding-agent/src/prompts/tools/browser.md | 1 + packages/coding-agent/src/tools/browser.ts | 4 +- .../src/tools/browser/cmux/cmux-tab.ts | 23 ++-- .../src/tools/browser/tab-worker.ts | 47 ++------ .../browser-cmux-release-mid-run.test.ts | 107 +++++++++++++++++- .../test/tools/browser-op-tracking.test.ts | 18 --- 8 files changed, 134 insertions(+), 80 deletions(-) diff --git a/docs/tools/browser.md b/docs/tools/browser.md index 402ef3ca7..a5c0e1a59 100644 --- a/docs/tools/browser.md +++ b/docs/tools/browser.md @@ -84,7 +84,7 @@ The tool returns one result per call; no streaming partial output is emitted fro - any other object/array becomes pretty JSON text (`JSON.stringify(value, null, 2)`); a value that is not structured-cloneable is dropped with a debug note. - helper side effects (`read`/`write`/`tree`/...) emit `status` events that surface as compact JSON text. - primitive `display(value)` (string/number/...) and `console.*` flow to the text channel, which the worker forwards as debug logs rather than tool content; `undefined` is ignored. -- `tab.screenshot()` also appends text plus an image content item unless `silent: true`; `details.screenshots` records persisted screenshot metadata `{ dest, mimeType, bytes, width, height }`. +- `tab.screenshot()` returns its saved path and appends text plus an image unless `silent: true`; `details.screenshots` records `{ dest, mimeType, bytes, width, height }`. - `run` `details` includes `action`, `name`, current `browser`/`url` when the tab exists, optional `screenshots`, and `details.result` containing only the concatenated text outputs. Combined run text is capped at the inline byte limit via `enforceInlineByteCap()`; over-cap text is saved as a session artifact (`saveBrowserOutputArtifact()`) and the capped text replaces it in content and `details.result`. ## Flow @@ -125,7 +125,7 @@ The tool returns one result per call; no streaming partial output is emitted fro - `tab.observe({ includeAll?, viewportOnly? })` - `tab.ariaSnapshot(selector?, { depth?, boxes? })` - `tab.ref(id)` - - `tab.screenshot({ selector?, fullPage?, save?, silent? })` + - `tab.screenshot({ selector?, fullPage?, silent? })` - `tab.extract(format = "markdown")` - `tab.click(selector)` - `tab.type(selector, text)` @@ -151,7 +151,7 @@ The tool returns one result per call; no streaming partial output is emitted fro 16a. `tab.ref(id)` resolves a `[ref=eN]` id from the latest `ariaSnapshot()` to a live `ElementHandle` via `resolveAriaRefHandle()` (`page.evaluateHandle` in the main world, walking the document + shadow roots for the matching `_ariaRef`), throwing if no element matches; it accepts a bare `eN` or a prefixed form. For inline selector use, `parseAriaRefSelector()` recognizes only the explicit `aria-ref=eN` / `aria-ref/eN` / `ariaref/eN` forms inside `tab.click/type/fill/waitFor/scrollIntoView` — a bare `eN` is intentionally rejected there so it does not collide with cmux's native observe ids. The cmux backend resolves the same explicit forms through its `aria-ref` `SelectorSpec` kind in `findElement`. 17. `tab.goto()` clears the cached element ids before navigating. Any new `tab.observe()` also clears and rebuilds the cache. 18. `tab.click()` uses a custom retry loop for `text/...` selectors to find an actionable visible match; other selectors use `page.locator(...).click()`. Interactive actions (`click`/`fill`/`type`/`press`/`scroll`/`drag`/`scrollIntoView`/`select`/`uploadFile`) and the `waitFor*` helpers run under a per-op deadline (`min(cellBudget − slack, ceiling)`) threaded into both the puppeteer `signal` and `.setTimeout()`, so a stalled helper aborts the CDP action and rejects with a named `tab. timed out after ms` that leaves cell budget — never the opaque whole-cell timeout. `goto`/`evaluate` stay uncapped. -19. `tab.screenshot()` captures either the whole page or a selector PNG, downsizes a copy for model output, chooses a persistence path, writes the image to disk, records metadata, and optionally emits text + image display entries. +19. `tab.screenshot()` captures the page or selected element as PNG, resizes a model copy, saves under `browser.screenshotDir` or the OS temp directory, returns that path, records metadata, and optionally emits text plus image content. 20. `display()` calls accumulate in an array. After code finishes, the worker posts `{ displays, returnValue, screenshots }`; `BrowserTool.#run()` appends the return value as trailing text content when not `undefined`. 21. `close` releases one tab or all tabs via `releaseTab()` / `releaseAllTabs()`. Each tab aborts pending runs, asks the worker to close, waits up to `750` ms for a `closed` ack, terminates the worker, decrements browser refcount, and disposes the browser handle when refcount reaches zero. @@ -176,9 +176,9 @@ The tool returns one result per call; no streaming partial output is emitted fro - `accept`/`dismiss`: page `dialog` events are handled automatically. - Changing dialog policy on an existing live tab forces tab recreation instead of mutating the worker in place. - **Screenshot persistence** - - `save` provided: persist full-resolution PNG at the resolved cwd-relative or absolute path. - `browser.screenshotDir` session setting set: persist full-resolution PNG under that directory with a timestamped filename. - - Neither set: persist the resized image to a temp-file path under the OS temp dir. + - Unset: persist to a temp-file path under the OS temp dir. + - `tab.screenshot()` returns the saved file path. ## Side Effects - Filesystem @@ -199,7 +199,7 @@ The tool returns one result per call; no streaming partial output is emitted fro - Session state (transcript, memory, jobs, checkpoints, registries) - Browser handles are cached in a process-global `Map` keyed by browser kind in `packages/coding-agent/src/tools/browser/registry.ts`. - Tabs are cached in a process-global `Map` keyed by `name` in `packages/coding-agent/src/tools/browser/tab-supervisor.ts`. - - `run` captures session cwd and optional `browser.screenshotDir` for screenshot/save path resolution. + - `run` captures session cwd and optional `browser.screenshotDir` for screenshot path resolution. - `restartForModeChange()` drops only headless tabs. - User-visible prompts / interactive UI - None beyond normal tool output. Dialog auto-handling is invisible unless it fails and emits debug logs. diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 8796645dc..b7a3aeaf5 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -22,6 +22,8 @@ ### Changed +- `tab.screenshot()` no longer accepts a per-call save path. It saves under `browser.screenshotDir`, or the OS temp directory when unset, and returns the saved path. + - Direct and `xd://` dispatch now share one canonical tool map: `write xd://` executes any enabled top-level or mounted tool, and `read xd://` returns its docs, instead of failing when the name was exposed through the other layer. Mounted names are presentation metadata only, so tool replacement and disconnection cannot leave stale device instances; disabled tools remain unreachable, and both `xd://` and Cursor/top-level fallback execution retain the tool's approval and ACP permission gates. - Session listing now caches parsed headers keyed on file stat identity (mtime + size), so repeated resume-picker opens and startup scans re-read only changed session files - Reduced per-keystroke editor dispatch overhead: keybinding resolution happens once per input chunk and the per-action interception chain is gated behind a single canonical-key set probe diff --git a/packages/coding-agent/src/prompts/tools/browser.md b/packages/coding-agent/src/prompts/tools/browser.md index 76a917b33..cf80a53e1 100644 --- a/packages/coding-agent/src/prompts/tools/browser.md +++ b/packages/coding-agent/src/prompts/tools/browser.md @@ -8,6 +8,7 @@ Drives real Chromium tab; full puppeteer access via JS. - `tab` helpers (drop to raw puppeteer `page` for anything uncovered): Element handles: `tab.ref("e5")` / `tab.id(n)` return a handle you call methods on directly — `(await tab.id(n)).click()`. Handles are NOT selectors: `tab.click`/`type`/`fill`/`waitFor*` take STRING selectors only. Snapshot refs work in any selector slot: `tab.click("e5")` ≡ `tab.click("aria-ref=e5")`. Simple: `tab.goto`, `tab.click`, `tab.type`, `tab.fill`, `tab.press`, `tab.scroll`, `tab.scrollIntoView`, `tab.drag`, `tab.uploadFile`, `tab.select`, `tab.screenshot`, `tab.extract`, `tab.evaluate`. + Screenshots: `tab.screenshot({ selector?, fullPage?, silent? })` saves to `browser.screenshotDir`, or OS temp when unset, then returns the path. It NEVER accepts a path. Waits: `tab.waitFor`, `tab.waitForSelector`, `tab.waitForUrl`, `tab.waitForResponse`, `tab.waitForNavigation`. Snapshots: `tab.observe()` → accessibility tree; `tab.ariaSnapshot()` → ARIA YAML with `[ref=eN]`. diff --git a/packages/coding-agent/src/tools/browser.ts b/packages/coding-agent/src/tools/browser.ts index 5fbd1f45c..92f358fde 100644 --- a/packages/coding-agent/src/tools/browser.ts +++ b/packages/coding-agent/src/tools/browser.ts @@ -163,11 +163,11 @@ export class BrowserTool implements AgentTool { + async screenshot(opts: ScreenshotOptions = {}): Promise { const context = this.#requireRunContext("tab.screenshot()"); // The cmux daemon's `browser.screenshot` captures the surface viewport // only — it has no element-clip or full-page mode, and Bun.Image cannot @@ -518,20 +517,16 @@ export class CmuxTab { excludeWebP: context.session.excludeWebP, }, ); - const explicitPath = opts.save ? resolveToCwd(opts.save, context.session.cwd) : undefined; - const returnedPath = typeof result.path === "string" && result.path.length > 0 ? result.path : undefined; - const saveFullRes = !!(explicitPath || context.session.browserScreenshotDir || returnedPath); + const saveFullRes = !!context.session.browserScreenshotDir; const savedBuffer = saveFullRes ? buffer : Buffer.from(resized.buffer); const savedMimeType = saveFullRes ? captureMime : resized.mimeType; const ext = savedMimeType === "image/webp" ? "webp" : savedMimeType === "image/jpeg" ? "jpg" : "png"; - const dest = - explicitPath ?? - (context.session.browserScreenshotDir - ? path.join( - context.session.browserScreenshotDir, - `screenshot-${new Date().toISOString().replace(/[:.]/g, "-").slice(0, -1)}.${ext}`, - ) - : (returnedPath ?? path.join(os.tmpdir(), `omp-sshots-${Snowflake.next()}.${ext}`))); + const dest = context.session.browserScreenshotDir + ? path.join( + context.session.browserScreenshotDir, + `screenshot-${new Date().toISOString().replace(/[:.]/g, "-").slice(0, -1)}.${ext}`, + ) + : path.join(os.tmpdir(), `omp-sshots-${Snowflake.next()}.${ext}`); await fs.promises.mkdir(path.dirname(dest), { recursive: true }); await Bun.write(dest, savedBuffer); const info: ScreenshotResult = { @@ -556,7 +551,7 @@ export class CmuxTab { context.output.push({ type: "text", text: lines.join("\n") }); context.output.push({ type: "image", data: resized.data, mimeType: resized.mimeType }); } - return info; + return dest; } async waitForUrl(pattern: string | RegExp, opts?: { timeout?: number }): Promise { diff --git a/packages/coding-agent/src/tools/browser/tab-worker.ts b/packages/coding-agent/src/tools/browser/tab-worker.ts index 462c3da78..af2d56d58 100644 --- a/packages/coding-agent/src/tools/browser/tab-worker.ts +++ b/packages/coding-agent/src/tools/browser/tab-worker.ts @@ -11,7 +11,6 @@ import type { ElementHandle, ElementScreenshotOptions, HTTPResponse, - ImageFormat, KeyInput, Page, SerializedAXNode, @@ -217,7 +216,6 @@ export function resolveWaitTimeout(cellTimeoutMs: number, explicit?: number): nu interface ScreenshotOptions { selector?: string; fullPage?: boolean; - save?: string; silent?: boolean; } @@ -233,7 +231,7 @@ interface TabApi { ): Promise; observe(opts?: { includeAll?: boolean; viewportOnly?: boolean }): Promise; ariaSnapshot(selector?: string, opts?: AriaSnapshotOptions): Promise; - screenshot(opts?: ScreenshotOptions): Promise; + screenshot(opts?: ScreenshotOptions): Promise; extract(format?: ReadableFormat): Promise; click(selector: string): Promise; type(selector: string, text: string): Promise; @@ -714,19 +712,6 @@ export function describeScreenshot(opts?: ScreenshotOptions): string { return "tab.screenshot()"; } -/** Map an explicit save path's extension to a puppeteer capture format (default png). */ -export function imageFormatForPath(filePath: string): ImageFormat { - switch (path.extname(filePath).toLowerCase()) { - case ".webp": - return "webp"; - case ".jpg": - case ".jpeg": - return "jpeg"; - default: - return "png"; - } -} - /** Summarize still-running helpers (oldest first) so a cell timeout names what stalled. */ export function describeInflight(inflight: Map): string { const now = Date.now(); @@ -1532,7 +1517,7 @@ export class WorkerCore { screenshots: ScreenshotResult[], signal: AbortSignal | undefined, opts: ScreenshotOptions = {}, - ): Promise { + ): Promise { const page = this.#requirePage(); // Multiple tabs can share one Chromium (sibling headless tabs on a shared // endpoint, cdp/app attach). CDP `Page.captureScreenshot` reads the @@ -1542,12 +1527,8 @@ export class WorkerCore { // already-active or freshly-closed target never fails the capture. await untilAborted(signal, () => page.bringToFront()).catch(() => undefined); const fullPage = opts.selector ? false : (opts.fullPage ?? false); - // An explicit save path picks the full-res capture format: puppeteer encodes - // png/jpeg/webp natively, so `save: "shot.webp"` gets real WebP bytes instead - // of PNG bytes hiding behind a .webp name. Unknown/missing extensions stay PNG. - const explicitPath = opts.save ? resolveToCwd(opts.save, session.cwd) : undefined; - const captureType = explicitPath ? imageFormatForPath(explicitPath) : "png"; - const captureMime = `image/${captureType}` as const; + const captureType = "png"; + const captureMime = "image/png" as const; let buffer: Buffer; if (opts.selector) { const handle = @@ -1581,20 +1562,16 @@ export class WorkerCore { { type: "image", data: buffer.toBase64(), mimeType: captureMime }, { maxWidth: 1024, maxHeight: 1024, maxBytes: 150 * 1024, jpegQuality: 70, excludeWebP: session.excludeWebP }, ); - const saveFullRes = !!(explicitPath || session.browserScreenshotDir); + const saveFullRes = !!session.browserScreenshotDir; const savedBuffer = saveFullRes ? buffer : resized.buffer; const savedMimeType = saveFullRes ? captureMime : resized.mimeType; - // Names must match the bytes we actually write: full-res follows the capture - // format, the resized buffer is whichever of PNG/JPEG/WebP encoded smallest. const ext = savedMimeType === "image/webp" ? "webp" : savedMimeType === "image/jpeg" ? "jpg" : "png"; - const dest = - explicitPath ?? - (session.browserScreenshotDir - ? path.join( - session.browserScreenshotDir, - `screenshot-${new Date().toISOString().replace(/[:.]/g, "-").slice(0, -1)}.${ext}`, - ) - : path.join(os.tmpdir(), `omp-sshots-${Snowflake.next()}.${ext}`)); + const dest = session.browserScreenshotDir + ? path.join( + session.browserScreenshotDir, + `screenshot-${new Date().toISOString().replace(/[:.]/g, "-").slice(0, -1)}.${ext}`, + ) + : path.join(os.tmpdir(), `omp-sshots-${Snowflake.next()}.${ext}`); await fs.promises.mkdir(path.dirname(dest), { recursive: true }); await Bun.write(dest, savedBuffer); const info: ScreenshotResult = { @@ -1616,7 +1593,7 @@ export class WorkerCore { output.push({ type: "text", text: lines.join("\n") }); output.push({ type: "image", data: resized.data, mimeType: resized.mimeType }); } - return info; + return dest; } async #drag(from: DragTarget, to: DragTarget, signal: AbortSignal): Promise { diff --git a/packages/coding-agent/test/tools/browser-cmux-release-mid-run.test.ts b/packages/coding-agent/test/tools/browser-cmux-release-mid-run.test.ts index dbecc8f30..c359827e3 100644 --- a/packages/coding-agent/test/tools/browser-cmux-release-mid-run.test.ts +++ b/packages/coding-agent/test/tools/browser-cmux-release-mid-run.test.ts @@ -29,6 +29,9 @@ */ import { afterEach, describe, expect, it, spyOn, vi } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; import type { CmuxKind } from "@oh-my-pi/pi-coding-agent/tools/browser/cmux/rpc"; import { CmuxSocketClient } from "@oh-my-pi/pi-coding-agent/tools/browser/cmux/socket-client"; import { acquireBrowser } from "@oh-my-pi/pi-coding-agent/tools/browser/registry"; @@ -48,14 +51,13 @@ function makeKind(socketSuffix: string): CmuxKind { }; } -function makeSession(cwd: string): ToolSession { - // Minimal shape: `runInTab` only reads `cwd`, `settings.get("browser.screenshotDir")`, - // and `getActiveModel?.()`. Everything else on `ToolSession` is untouched by the - // tab-supervisor flow we exercise. +function makeSession(cwd: string, screenshotDir?: string): ToolSession { + // Minimal shape: `runInTab` reads `cwd`, `settings.get("browser.screenshotDir")`, + // and `getActiveModel?.()`. Everything else is untouched by this flow. return { cwd, hasUI: false, - settings: { get: () => undefined }, + settings: { get: (key: string) => (key === "browser.screenshotDir" ? screenshotDir : undefined) }, getSessionFile: () => null, } as unknown as ToolSession; } @@ -283,4 +285,99 @@ describe("browser tab-supervisor — cmux tab close mid-run (#4499)", () => { process.removeListener("unhandledRejection", onUnhandled); } }); + + it("ignores the daemon screenshot path when no screenshot directory is configured", async () => { + spyOn(CmuxSocketClient.prototype, "connect").mockResolvedValue(undefined); + spyOn(CmuxSocketClient.prototype, "close").mockImplementation(() => undefined); + spyOn(CmuxSocketClient.prototype, "request").mockImplementation( + async (method: string): Promise> => { + switch (method) { + case "browser.open_split": + return { surface_id: "surface-screenshot", url: "about:blank" }; + case "browser.url.get": + return { url: "about:blank" }; + case "browser.snapshot": + return { page: { html: "" } }; + case "browser.eval": + return { value: "" }; + case "browser.screenshot": + return { + path: "/workspace/screenshots/daemon-owned.png", + png_base64: + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNk+M/wHwAF/gL+4z8ZAAAAAElFTkSuQmCC", + }; + default: + return {}; + } + }, + ); + + const browser = await acquireBrowser(makeKind("screenshot-temp"), { cwd: "/tmp" }); + await acquireTab("screenshot-temp", browser, { + timeoutMs: 5_000, + ownerSessionId: "session-screenshot-temp", + }); + + const result = await runInTab("screenshot-temp", { + code: "return await tab.screenshot({ silent: true });", + timeoutMs: 5_000, + session: makeSession("/tmp"), + }); + const savedPath = result.returnValue; + expect(typeof savedPath).toBe("string"); + if (typeof savedPath !== "string") throw new Error("tab.screenshot() did not return a path"); + expect(path.dirname(savedPath)).toBe(os.tmpdir()); + expect(savedPath).not.toBe("/workspace/screenshots/daemon-owned.png"); + expect(await Bun.file(savedPath).exists()).toBe(true); + await fs.rm(savedPath); + }); + + it("saves screenshots under the configured screenshot directory", async () => { + spyOn(CmuxSocketClient.prototype, "connect").mockResolvedValue(undefined); + spyOn(CmuxSocketClient.prototype, "close").mockImplementation(() => undefined); + spyOn(CmuxSocketClient.prototype, "request").mockImplementation( + async (method: string): Promise> => { + switch (method) { + case "browser.open_split": + return { surface_id: "surface-screenshot-configured", url: "about:blank" }; + case "browser.url.get": + return { url: "about:blank" }; + case "browser.snapshot": + return { page: { html: "" } }; + case "browser.eval": + return { value: "" }; + case "browser.screenshot": + return { + path: "/workspace/screenshots/daemon-owned.png", + png_base64: + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNk+M/wHwAF/gL+4z8ZAAAAAElFTkSuQmCC", + }; + default: + return {}; + } + }, + ); + + const screenshotDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-cmux-screenshot-")); + try { + const browser = await acquireBrowser(makeKind("screenshot-configured"), { cwd: "/tmp" }); + await acquireTab("screenshot-configured", browser, { + timeoutMs: 5_000, + ownerSessionId: "session-screenshot-configured", + }); + + const result = await runInTab("screenshot-configured", { + code: "return await tab.screenshot({ silent: true });", + timeoutMs: 5_000, + session: makeSession("/tmp", screenshotDir), + }); + const savedPath = result.returnValue; + expect(typeof savedPath).toBe("string"); + if (typeof savedPath !== "string") throw new Error("tab.screenshot() did not return a path"); + expect(path.dirname(savedPath)).toBe(screenshotDir); + expect(await Bun.file(savedPath).exists()).toBe(true); + } finally { + await fs.rm(screenshotDir, { recursive: true, force: true }); + } + }); }); diff --git a/packages/coding-agent/test/tools/browser-op-tracking.test.ts b/packages/coding-agent/test/tools/browser-op-tracking.test.ts index 9371f6381..a1cc08e2e 100644 --- a/packages/coding-agent/test/tools/browser-op-tracking.test.ts +++ b/packages/coding-agent/test/tools/browser-op-tracking.test.ts @@ -3,7 +3,6 @@ import { describeInflight, describeScreenshot, type InflightOp, - imageFormatForPath, } from "@oh-my-pi/pi-coding-agent/tools/browser/tab-worker"; describe("browser op tracking — timeout diagnostics", () => { @@ -37,20 +36,3 @@ describe("browser op tracking — timeout diagnostics", () => { expect(describeInflight(new Map())).toBe(""); }); }); - -describe("imageFormatForPath — explicit save capture format", () => { - it("maps the save path's extension to the matching capture format", () => { - expect(imageFormatForPath("/tmp/shot.webp")).toBe("webp"); - expect(imageFormatForPath("/tmp/shot.WEBP")).toBe("webp"); - expect(imageFormatForPath("/tmp/shot.jpg")).toBe("jpeg"); - expect(imageFormatForPath("/tmp/shot.jpeg")).toBe("jpeg"); - expect(imageFormatForPath("/tmp/shot.png")).toBe("png"); - }); - - it("falls back to png for unknown or missing extensions", () => { - expect(imageFormatForPath("/tmp/shot")).toBe("png"); - expect(imageFormatForPath("/tmp/shot.txt")).toBe("png"); - // A dotted directory must not leak its "extension" into an extensionless basename. - expect(imageFormatForPath("/tmp/v1.2/shot")).toBe("png"); - }); -});