diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 10f7883ed..4a621165f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed the browser tool crashing the whole process (parent session and every subagent) when a CDP world re-acquire failed mid-navigation: the stealth `puppeteer-core` patch called the bare `debugError` logger, which is `undefined` while the `puppeteer:error` debug channel is disabled (the default), turning a transient acquire failure into a fatal `TypeError` unhandled rejection. The patched `FrameManager`/`WebWorker` acquire paths now use `debugCatchError` ([#5296](https://github.com/can1357/oh-my-pi/issues/5296)) + ## [16.4.8] - 2026-07-12 ### Fixed diff --git a/packages/coding-agent/test/tools/browser-stealth-acquire-debugerror.test.ts b/packages/coding-agent/test/tools/browser-stealth-acquire-debugerror.test.ts new file mode 100644 index 000000000..dec0af52e --- /dev/null +++ b/packages/coding-agent/test/tools/browser-stealth-acquire-debugerror.test.ts @@ -0,0 +1,117 @@ +/** + * Regression test for issue #5296: the stealth `puppeteer-core` patch + * (`patches/puppeteer-core@25.3.0.patch`) re-implements world acquisition + * without `Runtime.enable`. Its new catch handlers in `FrameManager` called the + * bare `debugError` logger, which puppeteer leaves `undefined` when the + * `puppeteer:error` debug channel is disabled (the default). A transient CDP + * failure during world re-acquire then threw `TypeError: debugError is not a + * function` from `#doAcquireWorlds`, escaped as an `unhandledRejection`, and the + * postmortem handler killed the whole OMP process (parent session + every + * subagent). + * + * The test drives the real patched `FrameManager` with a `send()` that always + * rejects (a mid-flight CDP failure) and asserts the acquire path emits no + * unhandled `TypeError`. + * + * Real timers are deliberate here (see repo rule ts-no-test-timers): the fatal + * path is the coalescing acquirer's fire-and-forget `void this.#acquireWorlds()` + * retrigger, whose rejection escapes only to the global `unhandledRejection` + * handler — there is no promise or event the test can await, and fake timers + * serialise the two concurrent acquires so the retrigger (and thus the bug) + * never fires. Short real delays let the event loop interleave the acquires the + * way it does in production. + */ + +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import { CdpFrame } from "puppeteer-core/lib/puppeteer/cdp/Frame.js"; +import { FrameManager } from "puppeteer-core/lib/puppeteer/cdp/FrameManager.js"; +import { MAIN_WORLD, PUPPETEER_WORLD } from "puppeteer-core/lib/puppeteer/cdp/IsolatedWorlds.js"; +import { EventEmitter } from "puppeteer-core/lib/puppeteer/common/EventEmitter.js"; +import { TimeoutSettings } from "puppeteer-core/lib/puppeteer/common/TimeoutSettings.js"; +import { debugError } from "puppeteer-core/lib/puppeteer/common/util.js"; + +const ACQUIRE_TIMEOUT_MS = 40; + +// A CDP session double whose every `send` rejects, modelling a navigation that +// tears the target's execution contexts down mid-acquire. +class RejectingSession extends EventEmitter> { + constructor(readonly sessionId: string) { + super(); + } + id(): string { + return this.sessionId; + } + send(): Promise { + return Promise.reject(new Error("mid-flight CDP failure")); + } + target(): unknown { + return { _targetId: "T", type: () => "page" }; + } +} + +function makeFrameManager(session: RejectingSession): FrameManager { + const browser = { isNetworkEnabled: () => false, isIssuesEnabled: () => false, connected: true }; + const page = { browser: () => browser, isClosed: () => false, emit() {}, once() {}, off() {} }; + const timeoutSettings = new TimeoutSettings(); + timeoutSettings.setDefaultTimeout(ACQUIRE_TIMEOUT_MS); + // The patched FrameManager only touches the members exercised here; the + // puppeteer-internal `CdpCDPSession` / `CdpPage` types are far wider than the + // acquire path needs, so the doubles cross the boundary with a cast. + return new FrameManager(session as never, page as never, timeoutSettings); +} + +describe("stealth FrameManager world acquire — issue #5296", () => { + const rejections: unknown[] = []; + const onUnhandled = (reason: unknown) => rejections.push(reason); + + beforeEach(() => { + rejections.length = 0; + process.on("unhandledRejection", onUnhandled); + }); + + afterEach(() => { + process.off("unhandledRejection", onUnhandled); + }); + + it("keeps disabled debugError undefined so bare calls would crash", () => { + // The precondition that makes the bug fatal: with the puppeteer:error + // channel off, the logger the patch used is not callable. + expect(debugError).toBeUndefined(); + }); + + it("does not emit an unhandled TypeError when acquire fails mid-flight", async () => { + const session = new RejectingSession("S1"); + const frameManager = makeFrameManager(session); + const frame = new CdpFrame(frameManager, "F1", undefined, session as never); + frameManager._frameTree.addFrame(frame); + + // Navigation installs the lazy context providers and invalidates the old + // contexts; the async handler must settle before we pull a context. + session.emit("Page.frameNavigated", { + frame: { id: "F1", parentId: undefined, url: "about:blank" }, + type: "Navigation", + }); + await Bun.sleep(20); + + // Concurrent pulls on both worlds force the coalescing acquirer to + // re-run (`void this.#acquireWorlds` in its `finally`), which is the exact + // path where `#doAcquireWorlds`'s catch previously threw a bare + // `debugError(error)`. + const main = frame.worlds[MAIN_WORLD]; + const util = frame.worlds[PUPPETEER_WORLD]; + const results = await Promise.allSettled([main.evaluate(() => 1), util.evaluate(() => 1)]); + + // Let the re-triggered acquire settle and any stray rejection surface. + await Bun.sleep(ACQUIRE_TIMEOUT_MS + 40); + + const typeErrors = rejections.filter( + (reason): reason is TypeError => reason instanceof Error && reason.name === "TypeError", + ); + expect(typeErrors).toHaveLength(0); + expect(rejections).toHaveLength(0); + + // The failure is still observable as an ordinary, recoverable evaluate + // error rather than a silent process death. + expect(results.every(r => r.status === "rejected")).toBe(true); + }); +}); diff --git a/patches/puppeteer-core@25.3.0.patch b/patches/puppeteer-core@25.3.0.patch index bc93ba914..0bf86a5b2 100644 --- a/patches/puppeteer-core@25.3.0.patch +++ b/patches/puppeteer-core@25.3.0.patch @@ -352,7 +352,7 @@ index 2322aa136a47b446e2a7b2c4f0bc751f2fb821d0..2116367ee34bb86948bf956c2afa2f69 + client.send('Page.addScriptToEvaluateOnNewDocument', { + source: `//# sourceURL=${PuppeteerURL.INTERNAL_URL}`, + worldName: UTILITY_WORLD_NAME, -+ }).catch(debugError), + }).catch(debugCatchError), ...(frame ? Array.from(this.#scriptsToEvaluateOnNewDocument.values()) : []).map(script => { @@ -445,7 +445,7 @@ index 2322aa136a47b446e2a7b2c4f0bc751f2fb821d0..2116367ee34bb86948bf956c2afa2f69 + worldName: UTILITY_WORLD_NAME, + grantUniveralAccess: true, + }) -+ .catch(debugError); + .catch(debugCatchError); + const utilityId = iso && typeof iso.executionContextId === 'number' ? iso.executionContextId : undefined; + if (utilityId !== undefined) { + this.#onExecutionContextCreated({ @@ -486,7 +486,7 @@ index 2322aa136a47b446e2a7b2c4f0bc751f2fb821d0..2116367ee34bb86948bf956c2afa2f69 + } + } + catch (error) { -+ debugError(error); + debugCatchError(error); + } + } + // xxx-stealth: resolve a frame's MAIN-world execution context id without @@ -511,7 +511,7 @@ index 2322aa136a47b446e2a7b2c4f0bc751f2fb821d0..2116367ee34bb86948bf956c2afa2f69 + expression: 'globalThis', + serializationOptions: { serialization: 'idOnly' }, + }) -+ .catch(debugError); + .catch(debugCatchError); + return parse(globalThis?.result?.objectId); + } + if (utilityId === undefined) { @@ -523,21 +523,21 @@ index 2322aa136a47b446e2a7b2c4f0bc751f2fb821d0..2116367ee34bb86948bf956c2afa2f69 + contextId: utilityId, + serializationOptions: { serialization: 'idOnly' }, + }) -+ .catch(debugError); + .catch(debugCatchError); + const utilDocObjectId = utilDoc?.result?.objectId; + if (typeof utilDocObjectId !== 'string') { + return undefined; + } + const described = await session + .send('DOM.describeNode', { objectId: utilDocObjectId }) -+ .catch(debugError); + .catch(debugCatchError); + const backendNodeId = described?.node?.backendNodeId; + if (typeof backendNodeId !== 'number') { + return undefined; + } + const mainNode = await session + .send('DOM.resolveNode', { backendNodeId }) -+ .catch(debugError); + .catch(debugCatchError); + return parse(mainNode?.object?.objectId); } async #createIsolatedWorld(session, name) { @@ -605,7 +605,7 @@ index 3d68f887920ded269eb641273a5a13dee235ae1d..dcdd86c8697c0dbd2dd2162c9a739dd9 + this.#world.setContext(new ExecutionContext(client, { id }, this.#world)); + } + }) -+ .catch(debugError); + .catch(debugCatchError); this.#client.once('Inspector.workerScriptLoaded', () => { this.#workerLoaded.resolve(); });