diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index ad5f374bd..fcbfa030d 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -7,6 +7,7 @@ - Fixed credential-shaped tokens (GitHub/GitLab/OpenAI/Anthropic key patterns) being redacted from outbound provider requests even with `secrets.enabled` off; the pattern redaction now follows the `secrets.enabled` ("Hide Secrets") setting like the secret obfuscator. - Fixed Ctrl-clicking a wrapped OAuth authorization URL opening only the clicked row's truncated fragment by preserving the complete hyperlink target on every rendered row. - Fixed used-only absolute usage amounts across output surfaces: CLI now renders `$123.45 used`; the TUI shows a neutral, width-bounded amount instead of a pending/dotted/account-count placeholder; and ACP preserves `123.45 usd used` while suppressing duplicate window suffixes such as `— extra`. ([#5575](https://github.com/can1357/oh-my-pi/issues/5575)) +- Fixed the `browser` tool's `open` action ignoring the requested `timeout` during browser acquisition (CDP discovery/connect ran to its own fixed wait), and orphaning a freshly-created browser on abort/timeout before tab publication. The requested timeout now bounds the whole open lifecycle, and one explicit registry lease is held across tab acquisition so rollback disposes exactly the failed open — a concurrent open of a different tab name on the same browser can no longer dispose the browser out from under it. ([#6365](https://github.com/can1357/oh-my-pi/issues/6365)) ## [17.0.9] - 2026-07-23 diff --git a/packages/coding-agent/src/tools/browser.ts b/packages/coding-agent/src/tools/browser.ts index 362d24cd6..5fbd1f45c 100644 --- a/packages/coding-agent/src/tools/browser.ts +++ b/packages/coding-agent/src/tools/browser.ts @@ -7,9 +7,24 @@ import type { ToolSession } from "../sdk"; import { enforceInlineByteCap } from "../session/streaming-output"; import { truncateForPrompt } from "./approval"; import { resolveCmuxKind } from "./browser/cmux/rpc"; -import { acquireBrowser, type BrowserHandle, type BrowserKind, type BrowserKindTag } from "./browser/registry"; +import { + acquireBrowser, + type BrowserHandle, + type BrowserKind, + type BrowserKindTag, + holdBrowser, + releaseBrowser, +} from "./browser/registry"; import type { Observation, ScreenshotResult } from "./browser/tab-protocol"; -import { acquireTab, dropHeadlessTabs, getTab, releaseAllTabs, releaseTab, runInTab } from "./browser/tab-supervisor"; +import { + type AcquireTabResult, + acquireTab, + dropHeadlessTabs, + getTab, + releaseAllTabs, + releaseTab, + runInTab, +} from "./browser/tab-supervisor"; import type { OutputMeta } from "./output-meta"; import { resolveToCwd } from "./path-utils"; import { ToolAbortError, ToolError, throwIfAborted } from "./tool-errors"; @@ -232,52 +247,84 @@ export class BrowserTool implements AgentTool - acquireBrowser(kind, { - cwd: this.session.cwd, - viewport: params.viewport - ? { - width: params.viewport.width, - height: params.viewport.height, - deviceScaleFactor: params.viewport.scale, - } - : undefined, - appArgs: params.app?.args, - signal, - }), - ); + // The requested timeout must cover the *entire* open — browser + // acquisition (CDP discovery/connect), queued tab acquisition, worker + // creation, and navigation — not only `acquireTab`. Compose one deadline + // from the caller signal and `params.timeout` and thread it through both + // stages so a stalled acquisition rejects at the requested boundary. + const timeoutSignal = AbortSignal.timeout(timeoutMs); + const openSignal = signal ? AbortSignal.any([signal, timeoutSignal]) : timeoutSignal; + try { + const browser = await untilAborted(openSignal, () => + acquireBrowser(kind, { + cwd: this.session.cwd, + viewport: params.viewport + ? { + width: params.viewport.width, + height: params.viewport.height, + deviceScaleFactor: params.viewport.scale, + } + : undefined, + appArgs: params.app?.args, + signal: openSignal, + }), + ); - const result = await untilAborted(signal, () => - acquireTab(name, browser, { - url: params.url, - waitUntil: params.wait_until, - viewport: params.viewport - ? { - width: params.viewport.width, - height: params.viewport.height, - deviceScaleFactor: params.viewport.scale, - } - : undefined, - target: params.app?.target, - timeoutMs, - dialogs: params.dialogs, - signal, - ownerSessionId: this.session.getSessionId?.() ?? undefined, - }), - ); - const tab = result.tab; - const url = tab.info.url; - const title = tab.info.title ?? ""; - details.url = url; - details.viewport = tab.info.viewport; - const verb = result.created ? "Opened" : "Reused"; - const lines = [ - `${verb} tab ${JSON.stringify(name)} on ${describeBrowser(browser)}`, - `URL: ${url}`, - title ? `Title: ${title}` : null, - ].filter((l): l is string => typeof l === "string"); - details.result = lines.join("\n"); - return toolResult(details).text(lines.join("\n")).done(); + // Hold one open-acquisition lease across the whole tab acquisition. + // A freshly-created browser sits in the registry at refCount 0 until a + // tab takes a hold; without this lease an abort/timeout mid-acquisition + // (or a sibling open of a different tab name on the same browser that + // fails) could dispose it out from under this operation. The lease is + // released exactly once — the success and failure paths are mutually + // exclusive — transferring ownership to the published tab on success or + // rolling the fresh browser back on failure. + holdBrowser(browser); + let result: AcquireTabResult; + try { + result = await untilAborted(openSignal, () => + acquireTab(name, browser, { + url: params.url, + waitUntil: params.wait_until, + viewport: params.viewport + ? { + width: params.viewport.width, + height: params.viewport.height, + deviceScaleFactor: params.viewport.scale, + } + : undefined, + target: params.app?.target, + timeoutMs, + dialogs: params.dialogs, + signal: openSignal, + ownerSessionId: this.session.getSessionId?.() ?? undefined, + }), + ); + } catch (error) { + await releaseBrowser(browser, { kill: false }); + throw error; + } + await releaseBrowser(browser, { kill: false }); + + const tab = result.tab; + const url = tab.info.url; + const title = tab.info.title ?? ""; + details.url = url; + details.viewport = tab.info.viewport; + const verb = result.created ? "Opened" : "Reused"; + const lines = [ + `${verb} tab ${JSON.stringify(name)} on ${describeBrowser(browser)}`, + `URL: ${url}`, + title ? `Title: ${title}` : null, + ].filter((l): l is string => typeof l === "string"); + details.result = lines.join("\n"); + return toolResult(details).text(lines.join("\n")).done(); + } catch (error) { + // Caller cancellation stays a ToolAbortError; the requested timeout + // becomes a timeout ToolError; anything else passes through unchanged. + if (signal?.aborted) throw error instanceof ToolAbortError ? error : new ToolAbortError(); + if (timeoutSignal.aborted) throw new ToolError(`Browser open timed out after ${timeoutMs}ms`); + throw error; + } } async #close( diff --git a/packages/coding-agent/src/tools/browser/tab-supervisor.ts b/packages/coding-agent/src/tools/browser/tab-supervisor.ts index 3a1cc875e..dd9ff45af 100644 --- a/packages/coding-agent/src/tools/browser/tab-supervisor.ts +++ b/packages/coding-agent/src/tools/browser/tab-supervisor.ts @@ -286,13 +286,16 @@ async function acquireTabImpl( } } - // If the caller aborted while we were spawning/initializing the worker, - // tear the freshly-built worker down before publishing the tab so the - // browser refCount (which `holdBrowser` below would take) never grows for - // a tab nobody is waiting for. + // If the caller aborted while we were spawning/initializing the worker, tear + // the freshly-built worker down before publishing the tab so the browser + // refCount (which `holdBrowser` below would take) never grows for a tab + // nobody is waiting for. Mirror the error paths' `refCount === 0` release so + // a fresh browser held by nothing but this aborted open is not orphaned in + // the registry; a browser still leased/held elsewhere (refCount > 0) is left + // for its owner to release. if (opts.signal?.aborted) { await worker.terminate().catch(() => undefined); - if (tempHold) await releaseBrowser(browser, { kill: false }).catch(() => undefined); + if (tempHold || browser.refCount === 0) await releaseBrowser(browser, { kill: false }).catch(() => undefined); throw new ToolAbortError("Browser tab open aborted"); } diff --git a/packages/coding-agent/test/tools/browser-open-lease.test.ts b/packages/coding-agent/test/tools/browser-open-lease.test.ts new file mode 100644 index 000000000..334e38bf9 --- /dev/null +++ b/packages/coding-agent/test/tools/browser-open-lease.test.ts @@ -0,0 +1,211 @@ +/** + * Regression tests for issue #6365: `BrowserTool.#open` must apply the + * requested `timeout` to the *entire* open lifecycle (browser acquisition + + * tab acquisition), and must hold one explicit browser lease across tab + * acquisition so a refCount:0 browser is never orphaned by an abort/timeout + * nor disposed out from under a concurrent open of a different tab name. + * + * The tool resolves the cmux backend (`CMUX_SOCKET_PATH` + settings), so + * `CmuxSocketClient.prototype` is spied and no real socket / Chromium is used. + */ + +import { afterEach, beforeEach, describe, expect, it, spyOn, vi } from "bun:test"; +import { BrowserTool } from "@oh-my-pi/pi-coding-agent/tools/browser"; +import { CmuxSocketClient } from "@oh-my-pi/pi-coding-agent/tools/browser/cmux/socket-client"; +import { getBrowsersMapForTest } from "@oh-my-pi/pi-coding-agent/tools/browser/registry"; +import { getTabsMapForTest, releaseTab } from "@oh-my-pi/pi-coding-agent/tools/browser/tab-supervisor"; +import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools/index"; +import { ToolAbortError, ToolError } from "@oh-my-pi/pi-coding-agent/tools/tool-errors"; + +function makeSession(): ToolSession { + return { + cwd: "/tmp", + hasUI: false, + settings: { + get: (key: string) => (key === "browser.cmux" ? true : key === "tools.maxTimeout" ? 0 : undefined), + }, + getSessionId: () => "session-open-lease", + } as unknown as ToolSession; +} + +async function drainAllTabs(): Promise { + for (const name of [...getTabsMapForTest().keys()]) { + await releaseTab(name, { kill: false }).catch(() => undefined); + } +} + +let prevSocketPath: string | undefined; + +beforeEach(() => { + prevSocketPath = process.env.CMUX_SOCKET_PATH; + // Unique per test so the module-global browsers map (keyed by socket path) + // never carries a handle across tests. + process.env.CMUX_SOCKET_PATH = `/tmp/omp-open-lease-${process.pid}-${Math.random().toString(36).slice(2)}.sock`; +}); + +afterEach(async () => { + vi.useRealTimers(); + await drainAllTabs().catch(() => undefined); + vi.restoreAllMocks(); + if (prevSocketPath === undefined) delete process.env.CMUX_SOCKET_PATH; + else process.env.CMUX_SOCKET_PATH = prevSocketPath; +}); + +describe("browser open — requested timeout bounds the whole acquisition (#6365)", () => { + it("rejects with a timeout ToolError when browser acquisition stays pending past the deadline", async () => { + vi.useFakeTimers(); + const connectGate = Promise.withResolvers(); + spyOn(CmuxSocketClient.prototype, "connect").mockImplementation(async () => { + await connectGate.promise; + }); + const closeSpy = spyOn(CmuxSocketClient.prototype, "close").mockImplementation(() => undefined); + + const tool = new BrowserTool(makeSession()); + const open = tool.execute("call-timeout", { action: "open", name: "late", timeout: 1 }); + const settled = open.then( + () => ({ ok: true as const }), + (err: unknown) => ({ ok: false as const, err }), + ); + + // The requested 1s deadline elapses while `acquireBrowser` is still + // blocked on the (never-resolving) socket connect. Bun's fake timers fire + // `AbortSignal.timeout` synchronously on advance; awaiting `settled` below + // flushes the rejection. + vi.advanceTimersByTime(1000); + + const outcome = await settled; + expect(outcome.ok).toBe(false); + if (outcome.ok) throw new Error("unreachable"); + // The requested action timeout surfaces as a timeout ToolError — never a + // ToolAbortError (that is reserved for caller cancellation). + expect(outcome.err).toBeInstanceOf(ToolError); + expect(outcome.err).not.toBeInstanceOf(ToolAbortError); + expect((outcome.err as Error).message).toMatch(/timed out/i); + + // Let the orphan launch resolve; the aborted deadline must dispose it so + // no refCount:0 browser survives in the registry. + connectGate.resolve(); + for (let i = 0; i < 20; i++) await Promise.resolve(); + expect(closeSpy).toHaveBeenCalledTimes(1); + expect(getBrowsersMapForTest().size).toBe(0); + }); +}); + +describe("browser open — caller cancellation rolls back the fresh browser (#6365)", () => { + it("aborting before tab publication rejects with ToolAbortError and leaves both maps empty", async () => { + spyOn(CmuxSocketClient.prototype, "connect").mockResolvedValue(undefined); + const closeSpy = spyOn(CmuxSocketClient.prototype, "close").mockImplementation(() => undefined); + const openSplitGate = Promise.withResolvers(); + const surfaceClosed: string[] = []; + spyOn(CmuxSocketClient.prototype, "request").mockImplementation( + async (method: string, params: Record): Promise> => { + if (method === "browser.open_split") { + await openSplitGate.promise; + return { surface_id: "surface-abort", url: "about:blank" }; + } + if (method === "surface.close") { + surfaceClosed.push(String(params.surface_id)); + return {}; + } + return {}; + }, + ); + + const tool = new BrowserTool(makeSession()); + const controller = new AbortController(); + const open = tool.execute("call-abort", { action: "open", name: "fresh", timeout: 30 }, controller.signal); + const settled = open.then( + () => ({ ok: true as const }), + (err: unknown) => ({ ok: false as const, err }), + ); + + // Browser acquisition has resolved; tab acquisition is parked in + // `open_split`. Cancel here — before any tab is published. + await Promise.resolve(); + controller.abort(); + + const outcome = await settled; + expect(outcome.ok).toBe(false); + if (outcome.ok) throw new Error("unreachable"); + expect(outcome.err).toBeInstanceOf(ToolAbortError); + + // The open-acquisition lease rollback disposes the fresh browser exactly + // once and leaves nothing owned solely by the failed open. + expect(getTabsMapForTest().has("fresh")).toBe(false); + expect(getBrowsersMapForTest().size).toBe(0); + expect(closeSpy).toHaveBeenCalledTimes(1); + + // Let the orphaned acquisition unwind so it does not leak past the test. + openSplitGate.resolve(); + await Promise.resolve(); + }); +}); + +describe("browser open — concurrent different-name acquisitions each own a lease (#6365)", () => { + it("aborting one open releases only its lease; the survivor keeps the browser and one tab", async () => { + spyOn(CmuxSocketClient.prototype, "connect").mockResolvedValue(undefined); + const closeSpy = spyOn(CmuxSocketClient.prototype, "close").mockImplementation(() => undefined); + const openGate = Promise.withResolvers(); + let splitCount = 0; + const aEntered = Promise.withResolvers(); + const bEntered = Promise.withResolvers(); + const surfaceClosed: string[] = []; + spyOn(CmuxSocketClient.prototype, "request").mockImplementation( + async (method: string, params: Record): Promise> => { + if (method === "browser.open_split") { + const id = `surface-${++splitCount}`; + (splitCount === 1 ? aEntered : bEntered).resolve(); + await openGate.promise; + return { surface_id: id, url: "about:blank" }; + } + if (method === "surface.close") { + surfaceClosed.push(String(params.surface_id)); + return {}; + } + return {}; + }, + ); + + const tool = new BrowserTool(makeSession()); + + // Open A first and wait until it is parked inside `open_split` — proof it + // acquired the shared browser and took its open-acquisition lease. + const controllerA = new AbortController(); + const openA = tool.execute("call-a", { action: "open", name: "tab-a", timeout: 30 }, controllerA.signal); + const settledA = openA.then( + () => ({ ok: true as const }), + (err: unknown) => ({ ok: false as const, err }), + ); + await aEntered.promise; + expect(getBrowsersMapForTest().size).toBe(1); + + // Open B against the SAME browser (different tab name). It reuses the + // registry handle and takes its own lease; both are now parked. + const openB = tool.execute("call-b", { action: "open", name: "tab-b", timeout: 30 }); + await bEntered.promise; + + // Abort A while both are queued; releasing A's lease must not dispose the + // browser B still needs. + controllerA.abort(); + const outcomeA = await settledA; + expect(outcomeA.ok).toBe(false); + if (outcomeA.ok) throw new Error("unreachable"); + expect(outcomeA.err).toBeInstanceOf(ToolAbortError); + + // Release the gate so B publishes its tab. + openGate.resolve(); + const resultB = await openB; + expect(resultB.content.some(part => part.type === "text" && /Opened tab "tab-b"/.test(part.text ?? ""))).toBe( + true, + ); + + // B's browser survived A's rollback: still present, never closed, exactly + // one published tab. A's rollback closed only its own orphan surface. + expect(getBrowsersMapForTest().size).toBe(1); + expect(closeSpy).not.toHaveBeenCalled(); + expect(getTabsMapForTest().has("tab-b")).toBe(true); + expect(getTabsMapForTest().has("tab-a")).toBe(false); + expect(getTabsMapForTest().size).toBe(1); + expect(surfaceClosed).toEqual(["surface-1"]); + }); +});