test: split browser snapshot target helper

2026-05-06 11:30:43 +00:00 · 2026-04-17 05:43:29 +01:00
parent f57ce21d73
commit daaebb8558
4 changed files with 129 additions and 155 deletions
--- a/extensions/browser/src/browser/routes/agent.snapshot-target.ts
+++ b/extensions/browser/src/browser/routes/agent.snapshot-target.ts
@@ -0,0 +1,46 @@
+/** Resolve the correct targetId after a navigation that may trigger a renderer swap. */
+export async function resolveTargetIdAfterNavigate(opts: {
+  oldTargetId: string;
+  navigatedUrl: string;
+  listTabs: () => Promise<Array<{ targetId: string; url: string }>>;
+  retryDelayMs?: number;
+}): Promise<string> {
+  let currentTargetId = opts.oldTargetId;
+  try {
+    const pickReplacement = (
+      tabs: Array<{ targetId: string; url: string }>,
+      options?: { allowSingleTabFallback?: boolean },
+    ): { targetId: string; shouldRetry: boolean } => {
+      if (tabs.some((tab) => tab.targetId === opts.oldTargetId)) {
+        return { targetId: opts.oldTargetId, shouldRetry: false };
+      }
+      const byUrl = tabs.filter((tab) => tab.url === opts.navigatedUrl);
+      if (byUrl.length === 1) {
+        return { targetId: byUrl[0]?.targetId ?? opts.oldTargetId, shouldRetry: false };
+      }
+      const uniqueReplacement = byUrl.filter((tab) => tab.targetId !== opts.oldTargetId);
+      if (uniqueReplacement.length === 1) {
+        return {
+          targetId: uniqueReplacement[0]?.targetId ?? opts.oldTargetId,
+          shouldRetry: false,
+        };
+      }
+      if (options?.allowSingleTabFallback && tabs.length === 1) {
+        return { targetId: tabs[0]?.targetId ?? opts.oldTargetId, shouldRetry: false };
+      }
+      return { targetId: opts.oldTargetId, shouldRetry: true };
+    };
+
+    const first = pickReplacement(await opts.listTabs());
+    currentTargetId = first.targetId;
+    if (first.shouldRetry) {
+      await new Promise((r) => setTimeout(r, opts.retryDelayMs ?? 800));
+      currentTargetId = pickReplacement(await opts.listTabs(), {
+        allowSingleTabFallback: true,
+      }).targetId;
+    }
+  } catch {
+    // Best-effort: fall back to pre-navigation targetId.
+  }
+  return currentTargetId;
+}
--- a/extensions/browser/src/browser/routes/agent.snapshot.test.ts
+++ b/extensions/browser/src/browser/routes/agent.snapshot.test.ts
@@ -1,5 +1,5 @@
-import { beforeEach, describe, expect, it, vi } from "vitest";
-import { resolveTargetIdAfterNavigate } from "./agent.snapshot.js";
+import { describe, expect, it } from "vitest";
+import { resolveTargetIdAfterNavigate } from "./agent.snapshot-target.js";

 type Tab = { targetId: string; url: string };

@@ -8,10 +8,6 @@ function staticListTabs(tabs: Tab[]): () => Promise<Tab[]> {
 }

 describe("resolveTargetIdAfterNavigate", () => {
-  beforeEach(() => {
-    vi.useRealTimers();
-  });
-
  it("returns original targetId when old target still exists (no swap)", async () => {
    const result = await resolveTargetIdAfterNavigate({
      oldTargetId: "old-123",
@@ -37,6 +33,7 @@ describe("resolveTargetIdAfterNavigate", () => {
    const result = await resolveTargetIdAfterNavigate({
      oldTargetId: "old-123",
      navigatedUrl: "https://example.com",
+      retryDelayMs: 0,
      listTabs: staticListTabs([
        { targetId: "preexisting-000", url: "https://example.com" },
        { targetId: "fresh-777", url: "https://example.com" },
@@ -47,12 +44,12 @@ describe("resolveTargetIdAfterNavigate", () => {
  });

  it("retries and resolves targetId when first listTabs has no URL match", async () => {
-    vi.useFakeTimers();
    let calls = 0;

-    const result$ = resolveTargetIdAfterNavigate({
+    const result = await resolveTargetIdAfterNavigate({
      oldTargetId: "old-123",
      navigatedUrl: "https://delayed.com",
+      retryDelayMs: 0,
      listTabs: async () => {
        calls++;
        if (calls === 1) {
@@ -62,50 +59,33 @@ describe("resolveTargetIdAfterNavigate", () => {
      },
    });

-    await vi.advanceTimersByTimeAsync(800);
-    const result = await result$;
-
    expect(result).toBe("delayed-999");
    expect(calls).toBe(2);
-
-    vi.useRealTimers();
  });

  it("falls back to original targetId when no match found after retry", async () => {
-    vi.useFakeTimers();
-
-    const result$ = resolveTargetIdAfterNavigate({
+    const result = await resolveTargetIdAfterNavigate({
      oldTargetId: "old-123",
      navigatedUrl: "https://no-match.com",
+      retryDelayMs: 0,
      listTabs: staticListTabs([
        { targetId: "unrelated-1", url: "https://unrelated.com" },
        { targetId: "unrelated-2", url: "https://unrelated2.com" },
      ]),
    });

-    await vi.advanceTimersByTimeAsync(800);
-    const result = await result$;
-
    expect(result).toBe("old-123");
-
-    vi.useRealTimers();
  });

  it("falls back to single remaining tab when no URL match after retry", async () => {
-    vi.useFakeTimers();
-
-    const result$ = resolveTargetIdAfterNavigate({
+    const result = await resolveTargetIdAfterNavigate({
      oldTargetId: "old-123",
      navigatedUrl: "https://single-tab.com",
+      retryDelayMs: 0,
      listTabs: staticListTabs([{ targetId: "only-tab", url: "https://some-other.com" }]),
    });

-    await vi.advanceTimersByTimeAsync(800);
-    const result = await result$;
-
    expect(result).toBe("only-tab");
-
-    vi.useRealTimers();
  });

  it("falls back to original targetId when listTabs throws", async () => {
@@ -120,22 +100,16 @@ describe("resolveTargetIdAfterNavigate", () => {
  });

  it("keeps the old target when multiple replacement candidates still match after retry", async () => {
-    vi.useFakeTimers();
-
-    const result$ = resolveTargetIdAfterNavigate({
+    const result = await resolveTargetIdAfterNavigate({
      oldTargetId: "old-123",
      navigatedUrl: "https://example.com",
+      retryDelayMs: 0,
      listTabs: staticListTabs([
        { targetId: "preexisting-000", url: "https://example.com" },
        { targetId: "fresh-777", url: "https://example.com" },
      ]),
    });

-    await vi.advanceTimersByTimeAsync(800);
-    const result = await result$;
-
    expect(result).toBe("old-123");
-
-    vi.useRealTimers();
  });
 });
--- a/extensions/browser/src/browser/routes/agent.snapshot.ts
+++ b/extensions/browser/src/browser/routes/agent.snapshot.ts
@@ -32,6 +32,7 @@ import {
  withPlaywrightRouteContext,
  withRouteTabContext,
 } from "./agent.shared.js";
+import { resolveTargetIdAfterNavigate } from "./agent.snapshot-target.js";
 import {
  resolveSnapshotPlan,
  shouldUsePlaywrightForAriaSnapshot,
@@ -172,48 +173,6 @@ async function saveBrowserMediaResponse(params: {
  });
 }

-/** Resolve the correct targetId after a navigation that may trigger a renderer swap. */
-export async function resolveTargetIdAfterNavigate(opts: {
-  oldTargetId: string;
-  navigatedUrl: string;
-  listTabs: () => Promise<Array<{ targetId: string; url: string }>>;
-}): Promise<string> {
-  let currentTargetId = opts.oldTargetId;
-  try {
-    const pickReplacement = (
-      tabs: Array<{ targetId: string; url: string }>,
-      options?: { allowSingleTabFallback?: boolean },
-    ) => {
-      if (tabs.some((tab) => tab.targetId === opts.oldTargetId)) {
-        return opts.oldTargetId;
-      }
-      const byUrl = tabs.filter((tab) => tab.url === opts.navigatedUrl);
-      if (byUrl.length === 1) {
-        return byUrl[0]?.targetId ?? opts.oldTargetId;
-      }
-      const uniqueReplacement = byUrl.filter((tab) => tab.targetId !== opts.oldTargetId);
-      if (uniqueReplacement.length === 1) {
-        return uniqueReplacement[0]?.targetId ?? opts.oldTargetId;
-      }
-      if (options?.allowSingleTabFallback && tabs.length === 1) {
-        return tabs[0]?.targetId ?? opts.oldTargetId;
-      }
-      return opts.oldTargetId;
-    };
-
-    currentTargetId = pickReplacement(await opts.listTabs());
-    if (currentTargetId === opts.oldTargetId) {
-      await new Promise((r) => setTimeout(r, 800));
-      currentTargetId = pickReplacement(await opts.listTabs(), {
-        allowSingleTabFallback: true,
-      });
-    }
-  } catch {
-    // Best-effort: fall back to pre-navigation targetId
-  }
-  return currentTargetId;
-}
-
 export function registerBrowserAgentSnapshotRoutes(
  app: BrowserRouteRegistrar,
  ctx: BrowserRouteContext,
--- a/src/agents/skills.test.ts
+++ b/src/agents/skills.test.ts
@@ -15,8 +15,8 @@ import {
  applySkillEnvOverridesFromSnapshot,
  buildWorkspaceSkillCommandSpecs,
  buildWorkspaceSkillsPrompt,
-  buildWorkspaceSkillSnapshot,
-  loadWorkspaceSkillEntries,
+  type SkillEntry,
+  type SkillSnapshot,
 } from "./skills.js";
 import { getActiveSkillEnvKeys } from "./skills/env-overrides.js";
 import {
@@ -65,14 +65,43 @@ const withClearedEnv = <T>(
  }
 };

-async function writeEnvSkill(workspaceDir: string) {
-  const skillDir = path.join(workspaceDir, "skills", "env-skill");
-  await writeSkill({
-    dir: skillDir,
-    name: "env-skill",
-    description: "Needs env",
-    metadata: '{"openclaw":{"requires":{"env":["ENV_KEY"]},"primaryEnv":"ENV_KEY"}}',
-  });
+function makeSkillEntry(
+  name: string,
+  metadata: SkillEntry["metadata"],
+  description = "Needs env",
+): SkillEntry {
+  const baseDir = `/virtual/${name}`;
+  const filePath = `${baseDir}/SKILL.md`;
+  return {
+    skill: {
+      name,
+      description,
+      filePath,
+      baseDir,
+      source: "test",
+      sourceInfo: { path: filePath, source: "test", scope: "temporary", origin: "top-level" },
+      disableModelInvocation: false,
+    },
+    frontmatter: {},
+    metadata,
+  };
+}
+
+function envSkillEntries(name: string, metadata: SkillEntry["metadata"]): SkillEntry[] {
+  return [makeSkillEntry(name, metadata)];
+}
+
+function envSkillSnapshot(name: string, metadata: SkillEntry["metadata"]): SkillSnapshot {
+  return {
+    prompt: "",
+    skills: [
+      {
+        name,
+        primaryEnv: metadata?.primaryEnv,
+        requiredEnv: metadata?.requires?.env,
+      },
+    ],
+  };
 }

 beforeAll(async () => {
@@ -461,10 +490,10 @@ describe("buildWorkspaceSkillsPrompt", () => {

 describe("applySkillEnvOverrides", () => {
  it("sets and restores env vars", async () => {
-    const workspaceDir = await makeWorkspace();
-    await writeEnvSkill(workspaceDir);
-
-    const entries = loadWorkspaceSkillEntries(workspaceDir, resolveTestSkillDirs(workspaceDir));
+    const entries = envSkillEntries("env-skill", {
+      primaryEnv: "ENV_KEY",
+      requires: { env: ["ENV_KEY"] },
+    });

    withClearedEnv(["ENV_KEY"], () => {
      const restore = applySkillEnvOverrides({
@@ -484,10 +513,10 @@ describe("applySkillEnvOverrides", () => {
  });

  it("keeps env keys tracked until all overlapping overrides restore", async () => {
-    const workspaceDir = await makeWorkspace();
-    await writeEnvSkill(workspaceDir);
-
-    const entries = loadWorkspaceSkillEntries(workspaceDir, resolveTestSkillDirs(workspaceDir));
+    const entries = envSkillEntries("env-skill", {
+      primaryEnv: "ENV_KEY",
+      requires: { env: ["ENV_KEY"] },
+    });

    withClearedEnv(["ENV_KEY"], () => {
      const config = { skills: { entries: { "env-skill": { [apiKeyField]: "injected" } } } }; // pragma: allowlist secret
@@ -510,12 +539,9 @@ describe("applySkillEnvOverrides", () => {
  });

  it("applies env overrides from snapshots", async () => {
-    const workspaceDir = await makeWorkspace();
-    await writeEnvSkill(workspaceDir);
-
-    const snapshot = buildWorkspaceSkillSnapshot(workspaceDir, {
-      ...resolveTestSkillDirs(workspaceDir),
-      config: { skills: { entries: { "env-skill": { apiKey: "snap-key" } } } }, // pragma: allowlist secret
+    const snapshot = envSkillSnapshot("env-skill", {
+      primaryEnv: "ENV_KEY",
+      requires: { env: ["ENV_KEY"] },
    });

    withClearedEnv(["ENV_KEY"], () => {
@@ -534,10 +560,10 @@ describe("applySkillEnvOverrides", () => {
  });

  it("prefers the active runtime snapshot over raw SecretRef skill config", async () => {
-    const workspaceDir = await makeWorkspace();
-    await writeEnvSkill(workspaceDir);
-
-    const entries = loadWorkspaceSkillEntries(workspaceDir, resolveTestSkillDirs(workspaceDir));
+    const entries = envSkillEntries("env-skill", {
+      primaryEnv: "ENV_KEY",
+      requires: { env: ["ENV_KEY"] },
+    });
    const sourceConfig: OpenClawConfig = {
      skills: {
        entries: {
@@ -578,10 +604,10 @@ describe("applySkillEnvOverrides", () => {
  });

  it("prefers resolved caller skill config when the active runtime snapshot is still raw", async () => {
-    const workspaceDir = await makeWorkspace();
-    await writeEnvSkill(workspaceDir);
-
-    const entries = loadWorkspaceSkillEntries(workspaceDir, resolveTestSkillDirs(workspaceDir));
+    const entries = envSkillEntries("env-skill", {
+      primaryEnv: "ENV_KEY",
+      requires: { env: ["ENV_KEY"] },
+    });
    const sourceConfig: OpenClawConfig = {
      skills: {
        entries: {
@@ -622,10 +648,10 @@ describe("applySkillEnvOverrides", () => {
  });

  it("does not resolve raw skill apiKey refs when the host already provides primaryEnv", async () => {
-    const workspaceDir = await makeWorkspace();
-    await writeEnvSkill(workspaceDir);
-
-    const entries = loadWorkspaceSkillEntries(workspaceDir, resolveTestSkillDirs(workspaceDir));
+    const entries = envSkillEntries("env-skill", {
+      primaryEnv: "ENV_KEY",
+      requires: { env: ["ENV_KEY"] },
+    });

    withClearedEnv(["ENV_KEY"], () => {
      process.env.ENV_KEY = "host-key";
@@ -657,18 +683,11 @@ describe("applySkillEnvOverrides", () => {
  });

  it("blocks unsafe env overrides but allows declared secrets", async () => {
-    const workspaceDir = await makeWorkspace();
-    const skillDir = path.join(workspaceDir, "skills", "unsafe-env-skill");
-    await writeSkill({
-      dir: skillDir,
-      name: "unsafe-env-skill",
-      description: "Needs env",
-      metadata:
-        '{"openclaw":{"requires":{"env":["OPENAI_API_KEY","NODE_OPTIONS"]},"primaryEnv":"OPENAI_API_KEY"}}',
+    const entries = envSkillEntries("unsafe-env-skill", {
+      primaryEnv: "OPENAI_API_KEY",
+      requires: { env: ["OPENAI_API_KEY", "NODE_OPTIONS"] },
    });

-    const entries = loadWorkspaceSkillEntries(workspaceDir, resolveTestSkillDirs(workspaceDir));
-
    withClearedEnv(["OPENAI_API_KEY", "NODE_OPTIONS"], () => {
      const restore = applySkillEnvOverrides({
        skills: entries,
@@ -698,17 +717,10 @@ describe("applySkillEnvOverrides", () => {
  });

  it("blocks dangerous host env overrides even when declared", async () => {
-    const workspaceDir = await makeWorkspace();
-    const skillDir = path.join(workspaceDir, "skills", "dangerous-env-skill");
-    await writeSkill({
-      dir: skillDir,
-      name: "dangerous-env-skill",
-      description: "Needs env",
-      metadata: '{"openclaw":{"requires":{"env":["BASH_ENV","SHELL"]}}}',
+    const entries = envSkillEntries("dangerous-env-skill", {
+      requires: { env: ["BASH_ENV", "SHELL"] },
    });

-    const entries = loadWorkspaceSkillEntries(workspaceDir, resolveTestSkillDirs(workspaceDir));
-
    withClearedEnv(["BASH_ENV", "SHELL"], () => {
      const restore = applySkillEnvOverrides({
        skills: entries,
@@ -738,18 +750,10 @@ describe("applySkillEnvOverrides", () => {
  });

  it("blocks override-only host env overrides in skill config", async () => {
-    const workspaceDir = await makeWorkspace();
-    const skillDir = path.join(workspaceDir, "skills", "override-env-skill");
-    await writeSkill({
-      dir: skillDir,
-      name: "override-env-skill",
-      description: "Needs env",
-      metadata:
-        '{"openclaw":{"requires":{"env":["HTTPS_PROXY","NODE_TLS_REJECT_UNAUTHORIZED","DOCKER_HOST"]}}}',
+    const entries = envSkillEntries("override-env-skill", {
+      requires: { env: ["HTTPS_PROXY", "NODE_TLS_REJECT_UNAUTHORIZED", "DOCKER_HOST"] },
    });

-    const entries = loadWorkspaceSkillEntries(workspaceDir, resolveTestSkillDirs(workspaceDir));
-
    withClearedEnv(["HTTPS_PROXY", "NODE_TLS_REJECT_UNAUTHORIZED", "DOCKER_HOST"], () => {
      const restore = applySkillEnvOverrides({
        skills: entries,
@@ -782,13 +786,8 @@ describe("applySkillEnvOverrides", () => {
  });

  it("allows required env overrides from snapshots", async () => {
-    const workspaceDir = await makeWorkspace();
-    const skillDir = path.join(workspaceDir, "skills", "snapshot-env-skill");
-    await writeSkill({
-      dir: skillDir,
-      name: "snapshot-env-skill",
-      description: "Needs env",
-      metadata: '{"openclaw":{"requires":{"env":["OPENAI_API_KEY"]}}}',
+    const snapshot = envSkillSnapshot("snapshot-env-skill", {
+      requires: { env: ["OPENAI_API_KEY"] },
    });

    const config = {
@@ -802,10 +801,6 @@ describe("applySkillEnvOverrides", () => {
        },
      },
    };
-    const snapshot = buildWorkspaceSkillSnapshot(workspaceDir, {
-      ...resolveTestSkillDirs(workspaceDir),
-      config,
-    });

    withClearedEnv(["OPENAI_API_KEY"], () => {
      const restore = applySkillEnvOverridesFromSnapshot({