Merge pull request #351 from NianJiuZst/codex/fix-download-effect-state

fix(download): preserve dispatched effects when the trigger fails

.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:

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 = <T = Record<string, unknown>>(
+  method: string,
+  params?: object,
+  sessionId?: string,
+) => Promise<T>;
+
+async function browser(
+  run: (send: Send, cdp: ChromiumCdp, sessionId: () => string) => Promise<void>,
+) {
+  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 = '<button id="export" style="width:200px;height:100px">Export</button>';
+        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);
+});

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();
+    }
+  });
+});

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,