Revert "Browser: scope nested batch failures in switch"

This reverts commit aaeb348bb7.

src/browser/pw-tools-core.interactions.batch.test.ts

@@ -1,131 +0,0 @@
-import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest";
-
-let page: { evaluate: ReturnType<typeof vi.fn> } | null = null;
-
-const getPageForTargetId = vi.fn(async () => {
-  if (!page) {
-    throw new Error("test: page not set");
-  }
-  return page;
-});
-const ensurePageState = vi.fn(() => {});
-const forceDisconnectPlaywrightForTarget = vi.fn(async () => {});
-const refLocator = vi.fn(() => {
-  throw new Error("test: refLocator should not be called");
-});
-const restoreRoleRefsForTarget = vi.fn(() => {});
-
-const closePageViaPlaywright = vi.fn(async () => {});
-const resizeViewportViaPlaywright = vi.fn(async () => {});
-
-vi.mock("./pw-session.js", () => ({
-  ensurePageState,
-  forceDisconnectPlaywrightForTarget,
-  getPageForTargetId,
-  refLocator,
-  restoreRoleRefsForTarget,
-}));
-
-vi.mock("./pw-tools-core.snapshot.js", () => ({
-  closePageViaPlaywright,
-  resizeViewportViaPlaywright,
-}));
-
-let batchViaPlaywright: typeof import("./pw-tools-core.interactions.js").batchViaPlaywright;
-
-describe("batchViaPlaywright", () => {
-  beforeAll(async () => {
-    ({ batchViaPlaywright } = await import("./pw-tools-core.interactions.js"));
-  });
-
-  beforeEach(() => {
-    vi.clearAllMocks();
-    page = {
-      evaluate: vi.fn(async () => "ok"),
-    };
-  });
-
-  it("propagates evaluate timeouts through batched execution", async () => {
-    const result = await batchViaPlaywright({
-      cdpUrl: "http://127.0.0.1:9222",
-      targetId: "tab-1",
-      evaluateEnabled: true,
-      actions: [{ kind: "evaluate", fn: "() => 1", timeoutMs: 5000 }],
-    });
-
-    expect(result).toEqual({ results: [{ ok: true }] });
-    expect(page?.evaluate).toHaveBeenCalledWith(
-      expect.any(Function),
-      expect.objectContaining({
-        fnBody: "() => 1",
-        timeoutMs: 4500,
-      }),
-    );
-  });
-
-  it("supports resize and close inside a batch", async () => {
-    const result = await batchViaPlaywright({
-      cdpUrl: "http://127.0.0.1:9222",
-      targetId: "tab-1",
-      actions: [{ kind: "resize", width: 800, height: 600 }, { kind: "close" }],
-    });
-
-    expect(result).toEqual({ results: [{ ok: true }, { ok: true }] });
-    expect(resizeViewportViaPlaywright).toHaveBeenCalledWith({
-      cdpUrl: "http://127.0.0.1:9222",
-      targetId: "tab-1",
-      width: 800,
-      height: 600,
-    });
-    expect(closePageViaPlaywright).toHaveBeenCalledWith({
-      cdpUrl: "http://127.0.0.1:9222",
-      targetId: "tab-1",
-    });
-  });
-
-  it("propagates nested batch failures to the parent batch result", async () => {
-    const result = await batchViaPlaywright({
-      cdpUrl: "http://127.0.0.1:9222",
-      targetId: "tab-1",
-      actions: [
-        {
-          kind: "batch",
-          actions: [{ kind: "evaluate", fn: "() => 1" }],
-        },
-      ],
-    });
-
-    expect(result).toEqual({
-      results: [
-        { ok: false, error: "act:evaluate is disabled by config (browser.evaluateEnabled=false)" },
-      ],
-    });
-  });
-
-  it("includes all nested batch failures when stopOnError is false", async () => {
-    const result = await batchViaPlaywright({
-      cdpUrl: "http://127.0.0.1:9222",
-      targetId: "tab-1",
-      actions: [
-        {
-          kind: "batch",
-          stopOnError: false,
-          actions: [
-            { kind: "evaluate", fn: "() => 1" },
-            { kind: "evaluate", fn: "() => 2" },
-          ],
-        },
-      ],
-    });
-
-    expect(result).toEqual({
-      results: [
-        {
-          ok: false,
-          error:
-            "act:evaluate is disabled by config (browser.evaluateEnabled=false); act:evaluate is disabled by config (browser.evaluateEnabled=false)",
-        },
-      ],
-    });
-  });
-});

src/browser/pw-tools-core.interactions.ts

@@ -1,4 +1,4 @@
-import type { BrowserActRequest, BrowserFormField } from "./client-actions-core.js";
+import type { BrowserFormField } from "./client-actions-core.js";
 import { DEFAULT_FILL_FIELD_TYPE } from "./form-fields.js";
 import { DEFAULT_UPLOAD_DIR, resolveStrictExistingPathsWithinRoot } from "./paths.js";
 import {
@@ -8,32 +8,12 @@ import {
   refLocator,
   restoreRoleRefsForTarget,
 } from "./pw-session.js";
-import {
+import { normalizeTimeoutMs, requireRef, toAIFriendlyError } from "./pw-tools-core.shared.js";
-  normalizeTimeoutMs,
-  requireRef,
-  requireRefOrSelector,
-  toAIFriendlyError,
-} from "./pw-tools-core.shared.js";
-import { closePageViaPlaywright, resizeViewportViaPlaywright } from "./pw-tools-core.snapshot.js";
 
 type TargetOpts = {
   cdpUrl: string;
   targetId?: string;
 };
-const MAX_CLICK_DELAY_MS = 5_000;
-const MAX_WAIT_TIME_MS = 30_000;
-const MAX_BATCH_ACTIONS = 100;
-
-function resolveBoundedDelayMs(value: number | undefined, label: string, maxMs: number): number {
-  const normalized = Math.floor(value ?? 0);
-  if (!Number.isFinite(normalized) || normalized < 0) {
-    throw new Error(`${label} must be >= 0`);
-  }
-  if (normalized > maxMs) {
-    throw new Error(`${label} exceeds maximum of ${maxMs}ms`);
-  }
-  return normalized;
-}
 
 async function getRestoredPageForTarget(opts: TargetOpts) {
   const page = await getPageForTargetId(opts);
@@ -79,27 +59,17 @@ export async function highlightViaPlaywright(opts: {
 export async function clickViaPlaywright(opts: {
   cdpUrl: string;
   targetId?: string;
-  ref?: string;
+  ref: string;
-  selector?: string;
   doubleClick?: boolean;
   button?: "left" | "right" | "middle";
   modifiers?: Array<"Alt" | "Control" | "ControlOrMeta" | "Meta" | "Shift">;
-  delayMs?: number;
   timeoutMs?: number;
 }): Promise<void> {
-  const resolved = requireRefOrSelector(opts.ref, opts.selector);
   const page = await getRestoredPageForTarget(opts);
-  const label = resolved.ref ?? resolved.selector!;
+  const ref = requireRef(opts.ref);
-  const locator = resolved.ref
+  const locator = refLocator(page, ref);
-    ? refLocator(page, requireRef(resolved.ref))
-    : page.locator(resolved.selector);
   const timeout = resolveInteractionTimeoutMs(opts.timeoutMs);
   try {
-    const delayMs = resolveBoundedDelayMs(opts.delayMs, "click delayMs", MAX_CLICK_DELAY_MS);
-    if (delayMs > 0) {
-      await locator.hover({ timeout });
-      await new Promise((r) => setTimeout(r, delayMs));
-    }
     if (opts.doubleClick) {
       await locator.dblclick({
         timeout,
@@ -114,84 +84,67 @@ export async function clickViaPlaywright(opts: {
       });
     }
   } catch (err) {
-    throw toAIFriendlyError(err, label);
+    throw toAIFriendlyError(err, ref);
   }
 }
 
 export async function hoverViaPlaywright(opts: {
   cdpUrl: string;
   targetId?: string;
-  ref?: string;
+  ref: string;
-  selector?: string;
   timeoutMs?: number;
 }): Promise<void> {
-  const resolved = requireRefOrSelector(opts.ref, opts.selector);
+  const ref = requireRef(opts.ref);
   const page = await getRestoredPageForTarget(opts);
-  const label = resolved.ref ?? resolved.selector!;
-  const locator = resolved.ref
-    ? refLocator(page, requireRef(resolved.ref))
-    : page.locator(resolved.selector);
   try {
-    await locator.hover({
+    await refLocator(page, ref).hover({
       timeout: resolveInteractionTimeoutMs(opts.timeoutMs),
     });
   } catch (err) {
-    throw toAIFriendlyError(err, label);
+    throw toAIFriendlyError(err, ref);
   }
 }
 
 export async function dragViaPlaywright(opts: {
   cdpUrl: string;
   targetId?: string;
-  startRef?: string;
+  startRef: string;
-  startSelector?: string;
+  endRef: string;
-  endRef?: string;
-  endSelector?: string;
   timeoutMs?: number;
 }): Promise<void> {
-  const resolvedStart = requireRefOrSelector(opts.startRef, opts.startSelector);
+  const startRef = requireRef(opts.startRef);
-  const resolvedEnd = requireRefOrSelector(opts.endRef, opts.endSelector);
+  const endRef = requireRef(opts.endRef);
+  if (!startRef || !endRef) {
+    throw new Error("startRef and endRef are required");
+  }
   const page = await getRestoredPageForTarget(opts);
-  const startLocator = resolvedStart.ref
-    ? refLocator(page, requireRef(resolvedStart.ref))
-    : page.locator(resolvedStart.selector);
-  const endLocator = resolvedEnd.ref
-    ? refLocator(page, requireRef(resolvedEnd.ref))
-    : page.locator(resolvedEnd.selector);
-  const startLabel = resolvedStart.ref ?? resolvedStart.selector!;
-  const endLabel = resolvedEnd.ref ?? resolvedEnd.selector!;
   try {
-    await startLocator.dragTo(endLocator, {
+    await refLocator(page, startRef).dragTo(refLocator(page, endRef), {
       timeout: resolveInteractionTimeoutMs(opts.timeoutMs),
     });
   } catch (err) {
-    throw toAIFriendlyError(err, `${startLabel} -> ${endLabel}`);
+    throw toAIFriendlyError(err, `${startRef} -> ${endRef}`);
   }
 }
 
 export async function selectOptionViaPlaywright(opts: {
   cdpUrl: string;
   targetId?: string;
-  ref?: string;
+  ref: string;
-  selector?: string;
   values: string[];
   timeoutMs?: number;
 }): Promise<void> {
-  const resolved = requireRefOrSelector(opts.ref, opts.selector);
+  const ref = requireRef(opts.ref);
   if (!opts.values?.length) {
     throw new Error("values are required");
   }
   const page = await getRestoredPageForTarget(opts);
-  const label = resolved.ref ?? resolved.selector!;
-  const locator = resolved.ref
-    ? refLocator(page, requireRef(resolved.ref))
-    : page.locator(resolved.selector);
   try {
-    await locator.selectOption(opts.values, {
+    await refLocator(page, ref).selectOption(opts.values, {
       timeout: resolveInteractionTimeoutMs(opts.timeoutMs),
     });
   } catch (err) {
-    throw toAIFriendlyError(err, label);
+    throw toAIFriendlyError(err, ref);
   }
 }
 
@@ -215,20 +168,16 @@ export async function pressKeyViaPlaywright(opts: {
 export async function typeViaPlaywright(opts: {
   cdpUrl: string;
   targetId?: string;
-  ref?: string;
+  ref: string;
-  selector?: string;
   text: string;
   submit?: boolean;
   slowly?: boolean;
   timeoutMs?: number;
 }): Promise<void> {
-  const resolved = requireRefOrSelector(opts.ref, opts.selector);
   const text = String(opts.text ?? "");
   const page = await getRestoredPageForTarget(opts);
-  const label = resolved.ref ?? resolved.selector!;
+  const ref = requireRef(opts.ref);
-  const locator = resolved.ref
+  const locator = refLocator(page, ref);
-    ? refLocator(page, requireRef(resolved.ref))
-    : page.locator(resolved.selector);
   const timeout = resolveInteractionTimeoutMs(opts.timeoutMs);
   try {
     if (opts.slowly) {
@@ -241,7 +190,7 @@ export async function typeViaPlaywright(opts: {
       await locator.press("Enter", { timeout });
     }
   } catch (err) {
-    throw toAIFriendlyError(err, label);
+    throw toAIFriendlyError(err, ref);
   }
 }
 
@@ -418,22 +367,18 @@ export async function evaluateViaPlaywright(opts: {
 export async function scrollIntoViewViaPlaywright(opts: {
   cdpUrl: string;
   targetId?: string;
-  ref?: string;
+  ref: string;
-  selector?: string;
   timeoutMs?: number;
 }): Promise<void> {
-  const resolved = requireRefOrSelector(opts.ref, opts.selector);
   const page = await getRestoredPageForTarget(opts);
   const timeout = normalizeTimeoutMs(opts.timeoutMs, 20_000);
 
-  const label = resolved.ref ?? resolved.selector!;
+  const ref = requireRef(opts.ref);
-  const locator = resolved.ref
+  const locator = refLocator(page, ref);
-    ? refLocator(page, requireRef(resolved.ref))
-    : page.locator(resolved.selector);
   try {
     await locator.scrollIntoViewIfNeeded({ timeout });
   } catch (err) {
-    throw toAIFriendlyError(err, label);
+    throw toAIFriendlyError(err, ref);
   }
 }
 
@@ -454,7 +399,7 @@ export async function waitForViaPlaywright(opts: {
   const timeout = normalizeTimeoutMs(opts.timeoutMs, 20_000);
 
   if (typeof opts.timeMs === "number" && Number.isFinite(opts.timeMs)) {
-    await page.waitForTimeout(resolveBoundedDelayMs(opts.timeMs, "wait timeMs", MAX_WAIT_TIME_MS));
+    await page.waitForTimeout(Math.max(0, opts.timeMs));
   }
   if (opts.text) {
     await page.getByText(opts.text).first().waitFor({
@@ -703,197 +648,3 @@ export async function setInputFilesViaPlaywright(opts: {
     // Best-effort for sites that don't react to setInputFiles alone.
   }
 }
-
-const MAX_BATCH_DEPTH = 5;
-
-async function executeSingleAction(
-  action: BrowserActRequest,
-  cdpUrl: string,
-  targetId?: string,
-  evaluateEnabled?: boolean,
-  depth = 0,
-): Promise<void> {
-  if (depth > MAX_BATCH_DEPTH) {
-    throw new Error(`Batch nesting depth exceeds maximum of ${MAX_BATCH_DEPTH}`);
-  }
-  const effectiveTargetId = action.targetId ?? targetId;
-  switch (action.kind) {
-    case "click":
-      await clickViaPlaywright({
-        cdpUrl,
-        targetId: effectiveTargetId,
-        ref: action.ref,
-        selector: action.selector,
-        doubleClick: action.doubleClick,
-        button: action.button as "left" | "right" | "middle" | undefined,
-        modifiers: action.modifiers as Array<
-          "Alt" | "Control" | "ControlOrMeta" | "Meta" | "Shift"
-        >,
-        delayMs: action.delayMs,
-        timeoutMs: action.timeoutMs,
-      });
-      break;
-    case "type":
-      await typeViaPlaywright({
-        cdpUrl,
-        targetId: effectiveTargetId,
-        ref: action.ref,
-        selector: action.selector,
-        text: action.text,
-        submit: action.submit,
-        slowly: action.slowly,
-        timeoutMs: action.timeoutMs,
-      });
-      break;
-    case "press":
-      await pressKeyViaPlaywright({
-        cdpUrl,
-        targetId: effectiveTargetId,
-        key: action.key,
-        delayMs: action.delayMs,
-      });
-      break;
-    case "hover":
-      await hoverViaPlaywright({
-        cdpUrl,
-        targetId: effectiveTargetId,
-        ref: action.ref,
-        selector: action.selector,
-        timeoutMs: action.timeoutMs,
-      });
-      break;
-    case "scrollIntoView":
-      await scrollIntoViewViaPlaywright({
-        cdpUrl,
-        targetId: effectiveTargetId,
-        ref: action.ref,
-        selector: action.selector,
-        timeoutMs: action.timeoutMs,
-      });
-      break;
-    case "drag":
-      await dragViaPlaywright({
-        cdpUrl,
-        targetId: effectiveTargetId,
-        startRef: action.startRef,
-        startSelector: action.startSelector,
-        endRef: action.endRef,
-        endSelector: action.endSelector,
-        timeoutMs: action.timeoutMs,
-      });
-      break;
-    case "select":
-      await selectOptionViaPlaywright({
-        cdpUrl,
-        targetId: effectiveTargetId,
-        ref: action.ref,
-        selector: action.selector,
-        values: action.values,
-        timeoutMs: action.timeoutMs,
-      });
-      break;
-    case "fill":
-      await fillFormViaPlaywright({
-        cdpUrl,
-        targetId: effectiveTargetId,
-        fields: action.fields,
-        timeoutMs: action.timeoutMs,
-      });
-      break;
-    case "resize":
-      await resizeViewportViaPlaywright({
-        cdpUrl,
-        targetId: effectiveTargetId,
-        width: action.width,
-        height: action.height,
-      });
-      break;
-    case "wait":
-      if (action.fn && !evaluateEnabled) {
-        throw new Error("wait --fn is disabled by config (browser.evaluateEnabled=false)");
-      }
-      await waitForViaPlaywright({
-        cdpUrl,
-        targetId: effectiveTargetId,
-        timeMs: action.timeMs,
-        text: action.text,
-        textGone: action.textGone,
-        selector: action.selector,
-        url: action.url,
-        loadState: action.loadState,
-        fn: action.fn,
-        timeoutMs: action.timeoutMs,
-      });
-      break;
-    case "evaluate":
-      if (!evaluateEnabled) {
-        throw new Error("act:evaluate is disabled by config (browser.evaluateEnabled=false)");
-      }
-      await evaluateViaPlaywright({
-        cdpUrl,
-        targetId: effectiveTargetId,
-        fn: action.fn,
-        ref: action.ref,
-        timeoutMs: action.timeoutMs,
-      });
-      break;
-    case "close":
-      await closePageViaPlaywright({
-        cdpUrl,
-        targetId: effectiveTargetId,
-      });
-      break;
-    case "batch": {
-      // Nested batches: delegate recursively
-      const nestedFailures = (
-        await batchViaPlaywright({
-          cdpUrl,
-          targetId: effectiveTargetId,
-          actions: action.actions,
-          stopOnError: action.stopOnError,
-          evaluateEnabled,
-          depth: depth + 1,
-        })
-      ).results.filter((result) => !result.ok);
-      if (nestedFailures.length > 0) {
-        throw new Error(
-          nestedFailures.map((result) => result.error ?? "Nested batch action failed").join("; "),
-        );
-      }
-      break;
-    }
-    default:
-      throw new Error(`Unsupported batch action kind: ${(action as { kind: string }).kind}`);
-  }
-}
-
-export async function batchViaPlaywright(opts: {
-  cdpUrl: string;
-  targetId?: string;
-  actions: BrowserActRequest[];
-  stopOnError?: boolean;
-  evaluateEnabled?: boolean;
-  depth?: number;
-}): Promise<{ results: Array<{ ok: boolean; error?: string }> }> {
-  const depth = opts.depth ?? 0;
-  if (depth > MAX_BATCH_DEPTH) {
-    throw new Error(`Batch nesting depth exceeds maximum of ${MAX_BATCH_DEPTH}`);
-  }
-  if (opts.actions.length > MAX_BATCH_ACTIONS) {
-    throw new Error(`Batch exceeds maximum of ${MAX_BATCH_ACTIONS} actions`);
-  }
-  const results: Array<{ ok: boolean; error?: string }> = [];
-  for (const action of opts.actions) {
-    try {
-      await executeSingleAction(action, opts.cdpUrl, opts.targetId, opts.evaluateEnabled, depth);
-      results.push({ ok: true });
-    } catch (err) {
-      const message = err instanceof Error ? err.message : String(err);
-      results.push({ ok: false, error: message });
-      if (opts.stopOnError !== false) {
-        break;
-      }
-    }
-  }
-  return { results };
-}