From 7efe13a1bcd4590dca451e4e11d3384c909be1fd Mon Sep 17 00:00:00 2001 From: NianJiuZst <180004567+NianJiuZst@users.noreply.github.com> Date: Sun, 27 Sep 2026 01:48:29 +0800 Subject: [PATCH] fix(download): preserve dispatched effects when the trigger fails --- .github/workflows/ci.yml | 6 + .../download-effect-state.browser.test.ts | 139 ++++++++++++++++++ .../__tests__/download-effect-state.test.ts | 35 +++++ apps/extension/src/tools/download-capture.ts | 13 +- 4 files changed, 191 insertions(+), 2 deletions(-) create mode 100644 apps/extension/src/tools/__tests__/download-effect-state.browser.test.ts create mode 100644 apps/extension/src/tools/__tests__/download-effect-state.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index af909851..d6938cb8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -203,6 +203,12 @@ jobs: - name: Prepare extension types run: pnpm --filter @browser-skill/extension exec wxt prepare + - name: Run download-effect-state browser regression + working-directory: apps/extension + env: + BSK_CLICK_CHROME: google-chrome + run: pnpm exec vitest run src/tools/__tests__/download-effect-state.browser.test.ts + - name: Run browser input and debugging regressions working-directory: apps/extension env: diff --git a/apps/extension/src/tools/__tests__/download-effect-state.browser.test.ts b/apps/extension/src/tools/__tests__/download-effect-state.browser.test.ts new file mode 100644 index 00000000..d460b66c --- /dev/null +++ b/apps/extension/src/tools/__tests__/download-effect-state.browser.test.ts @@ -0,0 +1,139 @@ +// @vitest-environment node +// Opt in with BSK_CLICK_CHROME; the harness owns an isolated browser/profile. +import { describe, expect, it } from "vitest"; +import { type CdpDebuggerApi, ChromiumCdp } from "@/browser-driver/chromium-cdp"; +import { SessionManager } from "@/session-manager/manager"; +import { handleDownload } from "../download"; +import type { CdpRunner } from "../shared"; + +type Send = >( + method: string, + params?: object, + sessionId?: string, +) => Promise; + +async function browser( + run: (send: Send, cdp: ChromiumCdp, sessionId: () => string) => Promise, +) { + const { withChrome } = await import( + new URL( + "../../../../../evals/browser/cases/regression/snapshot-coordinates/chrome.mjs", + import.meta.url, + ).href + ); + await withChrome( + { + executable: process.env.BSK_CLICK_CHROME, + deviceScale: 1, + zoom: 1, + startupTimeout: 30_000, + }, + async (send: Send) => { + const { targetId } = await send<{ targetId: string }>("Target.createTarget", { + url: "about:blank", + }); + let activeSession = ""; + const api: CdpDebuggerApi = { + attach: async () => { + const reply = await send<{ sessionId: string }>("Target.attachToTarget", { + targetId, + flatten: true, + }); + activeSession = reply.sessionId; + }, + detach: async () => { + await send("Target.detachFromTarget", { sessionId: activeSession }); + activeSession = ""; + }, + sendCommand: async (_target, method, params) => send(method, params, activeSession), + onEvent: { addListener() {}, removeListener() {} } as unknown as CdpDebuggerApi["onEvent"], + onDetach: { + addListener() {}, + removeListener() {}, + } as unknown as CdpDebuggerApi["onDetach"], + }; + const cdp = new ChromiumCdp(api); + try { + await run(send, cdp, () => activeSession); + } finally { + await cdp.detach(7); + } + }, + ); +} + +function manager() { + return new SessionManager({ + agentWindow: { + create: async () => ({ windowId: 100, initialTabIds: [7] }), + remove: async () => {}, + ensureActiveTab: async () => 7, + }, + }); +} +const tab = { id: 7, windowId: 100, active: true, url: "about:blank" } as chrome.tabs.Tab; +const tabsApi = { get: async () => tab, query: async () => [tab] }; + +describe.skipIf(!process.env.BSK_CLICK_CHROME)("download effect metadata", () => { + it("a cancelled export whose click reached the page reports an unknown effect", async () => { + await browser(async (_send, cdp) => { + const sessions = manager(); + const ctx = await sessions.start("download-effect"); + await cdp.send(7, "Runtime.evaluate", { + expression: ` + document.body.innerHTML = ''; + window.startedExports = 0; + document.querySelector('#export').onclick = () => { window.startedExports++; }; + `, + }); + const ac = new AbortController(); + const wrapped: CdpRunner = { + send: (async (tabId: number, method: string, params?: object) => { + const reply = await cdp.send(tabId, method, params); + if ( + method === "Input.dispatchMouseEvent" && + (params as { type?: string })?.type === "mouseReleased" + ) + ac.abort(); + return reply; + }) as CdpRunner["send"], + onEvent: () => ({ dispose() {} }), + }; + const event = () => ({ addListener() {}, removeListener() {} }); + const downloads = { + onCreated: event(), + onChanged: event(), + onDeterminingFilename: event(), + search: async () => [], + cancel: async () => {}, + removeFile: async () => {}, + }; + const result = await handleDownload( + sessions, + { + session_id: ctx.sessionId, + tab_id: 7, + selector: "#export", + browser_relative_dir: "BrowserSkill/audit", + timeout_ms: 2000, + }, + { + cdp: wrapped, + tabsApi, + downloads, + signal: ac.signal, + navigationTargets: { onCreatedNavigationTarget: event() }, + }, + ); + const startedExports = ( + await cdp.send<{ result: { value: number } }>(7, "Runtime.evaluate", { + expression: "startedExports", + returnByValue: true, + }) + ).result.value; + console.log("DOWNLOAD_EFFECT_PROOF", JSON.stringify({ startedExports, result })); + expect(startedExports).toBe(1); + expect(result).toMatchObject({ data: { effect_state: "unknown" } }); + }); + }, 40_000); +}); diff --git a/apps/extension/src/tools/__tests__/download-effect-state.test.ts b/apps/extension/src/tools/__tests__/download-effect-state.test.ts new file mode 100644 index 00000000..fffc97ec --- /dev/null +++ b/apps/extension/src/tools/__tests__/download-effect-state.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, it, vi } from "vitest"; +import { captureBrowserDownload, type DownloadsApi } from "../download-capture"; +import type { CdpRunner } from "../shared"; + +describe("download trigger effect state", () => { + it.each(["returned", "thrown"])("distinguishes pre/post-dispatch %s failures", async (kind) => { + for (const dispatched of [false, true]) { + const event = () => ({ addListener: vi.fn(), removeListener: vi.fn() }); + const downloads: DownloadsApi = { + onCreated: event(), + onChanged: event(), + onDeterminingFilename: event(), + search: vi.fn(async () => []), + cancel: vi.fn(async () => {}), + removeFile: vi.fn(async () => {}), + }; + const cdp: CdpRunner = { send: vi.fn(), onEvent: () => ({ dispose: vi.fn() }) }; + const result = await captureBrowserDownload({ + cdp, + downloads, + target: { tabId: 4 }, + browserRelativeDir: "BrowserSkill/effect", + timeoutMs: 1_000, + trigger: async (markDispatched) => { + if (dispatched) markDispatched(); + if (kind === "thrown") throw new Error("trigger failed"); + return { code: "cancelled", message: "click aborted", data: { effect_state: "none" } }; + }, + }); + expect(result).toMatchObject({ data: { effect_state: dispatched ? "unknown" : "none" } }); + expect(downloads.cancel).not.toHaveBeenCalled(); + expect(downloads.removeFile).not.toHaveBeenCalled(); + } + }); +}); diff --git a/apps/extension/src/tools/download-capture.ts b/apps/extension/src/tools/download-capture.ts index f6d32bd1..c8ee1797 100644 --- a/apps/extension/src/tools/download-capture.ts +++ b/apps/extension/src/tools/download-capture.ts @@ -355,8 +355,13 @@ export async function captureBrowserDownload( }); if (isRpcError(triggered)) { void completion.catch(() => undefined); + // No download event yet does not undo a click already delivered to the page. const effect: TransferEffectState = - capturedId !== undefined ? "committed" : intent || popupUrls.size > 0 ? "unknown" : "none"; + capturedId !== undefined + ? "committed" + : dispatched || intent || popupUrls.size > 0 + ? "unknown" + : "none"; failureResult = { ...triggered, data: { ...triggered.data, effect_state: effect, phase: "trigger" }, @@ -369,7 +374,11 @@ export async function captureBrowserDownload( return { click, item }; } catch (err) { const effect: TransferEffectState = - capturedId !== undefined ? "committed" : click ? "unknown" : "none"; + capturedId !== undefined + ? "committed" + : dispatched || click || intent || popupUrls.size > 0 + ? "unknown" + : "none"; failureResult = captureError( err instanceof Error ? err.message : String(err), effect,