From daaebb8558b38aca77c89b426cdd5862323600c4 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 17 Apr 2026 05:43:29 +0100 Subject: [PATCH] test: split browser snapshot target helper --- .../browser/routes/agent.snapshot-target.ts | 46 ++++++ .../src/browser/routes/agent.snapshot.test.ts | 48 ++---- .../src/browser/routes/agent.snapshot.ts | 43 +---- src/agents/skills.test.ts | 147 +++++++++--------- 4 files changed, 129 insertions(+), 155 deletions(-) create mode 100644 extensions/browser/src/browser/routes/agent.snapshot-target.ts diff --git a/extensions/browser/src/browser/routes/agent.snapshot-target.ts b/extensions/browser/src/browser/routes/agent.snapshot-target.ts new file mode 100644 index 00000000000..dd61efcc004 --- /dev/null +++ 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>; + retryDelayMs?: number; +}): Promise { + 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; +} diff --git a/extensions/browser/src/browser/routes/agent.snapshot.test.ts b/extensions/browser/src/browser/routes/agent.snapshot.test.ts index 5a3b6f31dbf..ebd1828ade5 100644 --- 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 { } 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(); }); }); diff --git a/extensions/browser/src/browser/routes/agent.snapshot.ts b/extensions/browser/src/browser/routes/agent.snapshot.ts index 816536195d0..5a23a19b382 100644 --- 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>; -}): Promise { - 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, diff --git a/src/agents/skills.test.ts b/src/agents/skills.test.ts index 5b291fc06c0..eb133a5ea4e 100644 --- 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 = ( } }; -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({