diff --git a/src/agents/skills.buildworkspaceskillsnapshot.test.ts b/src/agents/skills.buildworkspaceskillsnapshot.test.ts index 1ec75e42059..82ed358a89a 100644 --- a/src/agents/skills.buildworkspaceskillsnapshot.test.ts +++ b/src/agents/skills.buildworkspaceskillsnapshot.test.ts @@ -3,7 +3,7 @@ import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import { createTrackedTempDirs } from "../test-utils/tracked-temp-dirs.js"; import { writeSkill } from "./skills.e2e-test-helpers.js"; -import { buildWorkspaceSkillSnapshot } from "./skills.js"; +import { buildWorkspaceSkillSnapshot, buildWorkspaceSkillsPrompt } from "./skills.js"; const tempDirs = createTrackedTempDirs(); @@ -51,6 +51,40 @@ describe("buildWorkspaceSkillSnapshot", () => { ]); }); + it("keeps prompt output aligned with buildWorkspaceSkillsPrompt", async () => { + const workspaceDir = await tempDirs.make("openclaw-"); + await writeSkill({ + dir: path.join(workspaceDir, "skills", "visible"), + name: "visible", + description: "Visible", + }); + await writeSkill({ + dir: path.join(workspaceDir, "skills", "hidden"), + name: "hidden", + description: "Hidden", + frontmatterExtra: "disable-model-invocation: true", + }); + const config = { + skills: { + limits: { + maxSkillsInPrompt: 1, + maxSkillsPromptChars: 200, + }, + }, + } as const; + const opts = { + config, + managedSkillsDir: path.join(workspaceDir, ".managed"), + bundledSkillsDir: path.join(workspaceDir, ".bundled"), + eligibility: { remote: { note: "Remote note" } }, + } as const; + + const snapshot = buildWorkspaceSkillSnapshot(workspaceDir, opts); + const prompt = buildWorkspaceSkillsPrompt(workspaceDir, opts); + + expect(snapshot.prompt).toBe(prompt); + }); + it("truncates the skills prompt when it exceeds the configured char budget", async () => { const workspaceDir = await tempDirs.make("openclaw-"); diff --git a/src/agents/skills/workspace.ts b/src/agents/skills/workspace.ts index 3d6071839ac..50f71d582bc 100644 --- a/src/agents/skills/workspace.ts +++ b/src/agents/skills/workspace.ts @@ -445,45 +445,9 @@ function applySkillsPromptLimits(params: { skills: Skill[]; config?: OpenClawCon export function buildWorkspaceSkillSnapshot( workspaceDir: string, - opts?: { - config?: OpenClawConfig; - managedSkillsDir?: string; - bundledSkillsDir?: string; - entries?: SkillEntry[]; - /** If provided, only include skills with these names */ - skillFilter?: string[]; - eligibility?: SkillEligibilityContext; - snapshotVersion?: number; - }, + opts?: WorkspaceSkillBuildOptions & { snapshotVersion?: number }, ): SkillSnapshot { - const skillEntries = opts?.entries ?? loadSkillEntries(workspaceDir, opts); - const eligible = filterSkillEntries( - skillEntries, - opts?.config, - opts?.skillFilter, - opts?.eligibility, - ); - const promptEntries = eligible.filter( - (entry) => entry.invocation?.disableModelInvocation !== true, - ); - const resolvedSkills = promptEntries.map((entry) => entry.skill); - const remoteNote = opts?.eligibility?.remote?.note?.trim(); - const { skillsForPrompt, truncated } = applySkillsPromptLimits({ - skills: resolvedSkills, - config: opts?.config, - }); - - const truncationNote = truncated - ? `⚠️ Skills truncated: included ${skillsForPrompt.length} of ${resolvedSkills.length}. Run \`openclaw skills check\` to audit.` - : ""; - - const prompt = [ - remoteNote, - truncationNote, - formatSkillsForPrompt(compactSkillPaths(skillsForPrompt)), - ] - .filter(Boolean) - .join("\n"); + const { eligible, prompt, resolvedSkills } = resolveWorkspaceSkillPromptState(workspaceDir, opts); const skillFilter = normalizeSkillFilter(opts?.skillFilter); return { prompt, @@ -500,16 +464,29 @@ export function buildWorkspaceSkillSnapshot( export function buildWorkspaceSkillsPrompt( workspaceDir: string, - opts?: { - config?: OpenClawConfig; - managedSkillsDir?: string; - bundledSkillsDir?: string; - entries?: SkillEntry[]; - /** If provided, only include skills with these names */ - skillFilter?: string[]; - eligibility?: SkillEligibilityContext; - }, + opts?: WorkspaceSkillBuildOptions, ): string { + return resolveWorkspaceSkillPromptState(workspaceDir, opts).prompt; +} + +type WorkspaceSkillBuildOptions = { + config?: OpenClawConfig; + managedSkillsDir?: string; + bundledSkillsDir?: string; + entries?: SkillEntry[]; + /** If provided, only include skills with these names */ + skillFilter?: string[]; + eligibility?: SkillEligibilityContext; +}; + +function resolveWorkspaceSkillPromptState( + workspaceDir: string, + opts?: WorkspaceSkillBuildOptions, +): { + eligible: SkillEntry[]; + prompt: string; + resolvedSkills: Skill[]; +} { const skillEntries = opts?.entries ?? loadSkillEntries(workspaceDir, opts); const eligible = filterSkillEntries( skillEntries, @@ -529,9 +506,14 @@ export function buildWorkspaceSkillsPrompt( const truncationNote = truncated ? `⚠️ Skills truncated: included ${skillsForPrompt.length} of ${resolvedSkills.length}. Run \`openclaw skills check\` to audit.` : ""; - return [remoteNote, truncationNote, formatSkillsForPrompt(compactSkillPaths(skillsForPrompt))] + const prompt = [ + remoteNote, + truncationNote, + formatSkillsForPrompt(compactSkillPaths(skillsForPrompt)), + ] .filter(Boolean) .join("\n"); + return { eligible, prompt, resolvedSkills }; } export function resolveSkillsPromptForRun(params: { diff --git a/src/agents/tools/browser-tool.test.ts b/src/agents/tools/browser-tool.test.ts index 41b25d98b1f..d3ef8d66078 100644 --- a/src/agents/tools/browser-tool.test.ts +++ b/src/agents/tools/browser-tool.test.ts @@ -96,6 +96,18 @@ vi.mock("./common.js", async () => { import { DEFAULT_AI_SNAPSHOT_MAX_CHARS } from "../../browser/constants.js"; import { createBrowserTool } from "./browser-tool.js"; +function mockSingleBrowserProxyNode() { + nodesUtilsMocks.listNodes.mockResolvedValue([ + { + nodeId: "node-1", + displayName: "Browser Node", + connected: true, + caps: ["browser"], + commands: ["browser.proxy"], + }, + ]); +} + describe("browser tool snapshot maxChars", () => { afterEach(() => { vi.clearAllMocks(); @@ -210,15 +222,7 @@ describe("browser tool snapshot maxChars", () => { }); it("routes to node proxy when target=node", async () => { - nodesUtilsMocks.listNodes.mockResolvedValue([ - { - nodeId: "node-1", - displayName: "Browser Node", - connected: true, - caps: ["browser"], - commands: ["browser.proxy"], - }, - ]); + mockSingleBrowserProxyNode(); const tool = createBrowserTool(); await tool.execute?.("call-1", { action: "status", target: "node" }); @@ -234,15 +238,7 @@ describe("browser tool snapshot maxChars", () => { }); it("keeps sandbox bridge url when node proxy is available", async () => { - nodesUtilsMocks.listNodes.mockResolvedValue([ - { - nodeId: "node-1", - displayName: "Browser Node", - connected: true, - caps: ["browser"], - commands: ["browser.proxy"], - }, - ]); + mockSingleBrowserProxyNode(); const tool = createBrowserTool({ sandboxBridgeUrl: "http://127.0.0.1:9999" }); await tool.execute?.("call-1", { action: "status" }); @@ -254,15 +250,7 @@ describe("browser tool snapshot maxChars", () => { }); it("keeps chrome profile on host when node proxy is available", async () => { - nodesUtilsMocks.listNodes.mockResolvedValue([ - { - nodeId: "node-1", - displayName: "Browser Node", - connected: true, - caps: ["browser"], - commands: ["browser.proxy"], - }, - ]); + mockSingleBrowserProxyNode(); const tool = createBrowserTool(); await tool.execute?.("call-1", { action: "status", profile: "chrome" }); diff --git a/src/agents/tools/browser-tool.ts b/src/agents/tools/browser-tool.ts index 9545ca3bcaa..27a0609f654 100644 --- a/src/agents/tools/browser-tool.ts +++ b/src/agents/tools/browser-tool.ts @@ -54,6 +54,27 @@ function wrapBrowserExternalJson(params: { }; } +function formatTabsToolResult(tabs: unknown[]) { + const wrapped = wrapBrowserExternalJson({ + kind: "tabs", + payload: { tabs }, + includeWarning: false, + }); + return { + content: [{ type: "text", text: wrapped.wrappedText }], + details: { ...wrapped.safeDetails, tabCount: tabs.length }, + }; +} + +function readOptionalTargetAndTimeout(params: Record) { + const targetId = typeof params.targetId === "string" ? params.targetId.trim() : undefined; + const timeoutMs = + typeof params.timeoutMs === "number" && Number.isFinite(params.timeoutMs) + ? params.timeoutMs + : undefined; + return { targetId, timeoutMs }; +} + type BrowserProxyFile = { path: string; base64: string; @@ -359,27 +380,11 @@ export function createBrowserTool(opts?: { profile, }); const tabs = (result as { tabs?: unknown[] }).tabs ?? []; - const wrapped = wrapBrowserExternalJson({ - kind: "tabs", - payload: { tabs }, - includeWarning: false, - }); - return { - content: [{ type: "text", text: wrapped.wrappedText }], - details: { ...wrapped.safeDetails, tabCount: tabs.length }, - }; + return formatTabsToolResult(tabs); } { const tabs = await browserTabs(baseUrl, { profile }); - const wrapped = wrapBrowserExternalJson({ - kind: "tabs", - payload: { tabs }, - includeWarning: false, - }); - return { - content: [{ type: "text", text: wrapped.wrappedText }], - details: { ...wrapped.safeDetails, tabCount: tabs.length }, - }; + return formatTabsToolResult(tabs); } case "open": { const targetUrl = readStringParam(params, "targetUrl", { @@ -712,11 +717,7 @@ export function createBrowserTool(opts?: { const ref = readStringParam(params, "ref"); const inputRef = readStringParam(params, "inputRef"); const element = readStringParam(params, "element"); - const targetId = typeof params.targetId === "string" ? params.targetId.trim() : undefined; - const timeoutMs = - typeof params.timeoutMs === "number" && Number.isFinite(params.timeoutMs) - ? params.timeoutMs - : undefined; + const { targetId, timeoutMs } = readOptionalTargetAndTimeout(params); if (proxyRequest) { const result = await proxyRequest({ method: "POST", @@ -748,11 +749,7 @@ export function createBrowserTool(opts?: { case "dialog": { const accept = Boolean(params.accept); const promptText = typeof params.promptText === "string" ? params.promptText : undefined; - const targetId = typeof params.targetId === "string" ? params.targetId.trim() : undefined; - const timeoutMs = - typeof params.timeoutMs === "number" && Number.isFinite(params.timeoutMs) - ? params.timeoutMs - : undefined; + const { targetId, timeoutMs } = readOptionalTargetAndTimeout(params); if (proxyRequest) { const result = await proxyRequest({ method: "POST", diff --git a/src/agents/tools/sessions-helpers.ts b/src/agents/tools/sessions-helpers.ts index c1e0babf5d4..6573b1e9cb5 100644 --- a/src/agents/tools/sessions-helpers.ts +++ b/src/agents/tools/sessions-helpers.ts @@ -15,6 +15,7 @@ export { export type { SessionReferenceResolution } from "./sessions-resolution.js"; export { isRequesterSpawnedSessionVisible, + isResolvedSessionVisibleToRequester, listSpawnedSessionKeys, looksLikeSessionId, looksLikeSessionKey, @@ -23,6 +24,7 @@ export { resolveMainSessionAlias, resolveSessionReference, shouldResolveSessionIdInput, + shouldVerifyRequesterSpawnedSessionVisibility, } from "./sessions-resolution.js"; import { extractTextFromChatContent } from "../../shared/chat-content.js"; import { sanitizeUserFacingText } from "../pi-embedded-helpers.js"; diff --git a/src/agents/tools/sessions-history-tool.ts b/src/agents/tools/sessions-history-tool.ts index 5532b45735b..7a56cdfdafd 100644 --- a/src/agents/tools/sessions-history-tool.ts +++ b/src/agents/tools/sessions-history-tool.ts @@ -8,7 +8,7 @@ import { jsonResult, readStringParam } from "./common.js"; import { createSessionVisibilityGuard, createAgentToAgentPolicy, - isRequesterSpawnedSessionVisible, + isResolvedSessionVisibleToRequester, resolveEffectiveSessionToolsVisibility, resolveSessionReference, resolveSandboxedSessionToolContext, @@ -183,17 +183,18 @@ export function createSessionsHistoryTool(opts?: { const resolvedKey = resolvedSession.key; const displayKey = resolvedSession.displayKey; const resolvedViaSessionId = resolvedSession.resolvedViaSessionId; - if (restrictToSpawned && !resolvedViaSessionId && resolvedKey !== effectiveRequesterKey) { - const ok = await isRequesterSpawnedSessionVisible({ - requesterSessionKey: effectiveRequesterKey, - targetSessionKey: resolvedKey, + + const visible = await isResolvedSessionVisibleToRequester({ + requesterSessionKey: effectiveRequesterKey, + targetSessionKey: resolvedKey, + restrictToSpawned, + resolvedViaSessionId, + }); + if (!visible) { + return jsonResult({ + status: "forbidden", + error: `Session not visible from this sandboxed agent session: ${sessionKeyParam}`, }); - if (!ok) { - return jsonResult({ - status: "forbidden", - error: `Session not visible from this sandboxed agent session: ${sessionKeyParam}`, - }); - } } const a2aPolicy = createAgentToAgentPolicy(cfg); diff --git a/src/agents/tools/sessions-resolution.test.ts b/src/agents/tools/sessions-resolution.test.ts index a71bd4a6b7a..2ed2d522816 100644 --- a/src/agents/tools/sessions-resolution.test.ts +++ b/src/agents/tools/sessions-resolution.test.ts @@ -1,11 +1,13 @@ import { describe, expect, it } from "vitest"; import type { OpenClawConfig } from "../../config/config.js"; import { + isResolvedSessionVisibleToRequester, looksLikeSessionId, looksLikeSessionKey, resolveDisplaySessionKey, resolveInternalSessionKey, resolveMainSessionAlias, + shouldVerifyRequesterSpawnedSessionVisibility, shouldResolveSessionIdInput, } from "./sessions-resolution.js"; @@ -75,3 +77,59 @@ describe("session reference shape detection", () => { expect(shouldResolveSessionIdInput("random-slug")).toBe(true); }); }); + +describe("resolved session visibility checks", () => { + it("requires spawned-session verification only for sandboxed key-based cross-session access", () => { + expect( + shouldVerifyRequesterSpawnedSessionVisibility({ + requesterSessionKey: "agent:main:main", + targetSessionKey: "agent:main:worker", + restrictToSpawned: true, + resolvedViaSessionId: false, + }), + ).toBe(true); + expect( + shouldVerifyRequesterSpawnedSessionVisibility({ + requesterSessionKey: "agent:main:main", + targetSessionKey: "agent:main:worker", + restrictToSpawned: false, + resolvedViaSessionId: false, + }), + ).toBe(false); + expect( + shouldVerifyRequesterSpawnedSessionVisibility({ + requesterSessionKey: "agent:main:main", + targetSessionKey: "agent:main:worker", + restrictToSpawned: true, + resolvedViaSessionId: true, + }), + ).toBe(false); + expect( + shouldVerifyRequesterSpawnedSessionVisibility({ + requesterSessionKey: "agent:main:main", + targetSessionKey: "agent:main:main", + restrictToSpawned: true, + resolvedViaSessionId: false, + }), + ).toBe(false); + }); + + it("returns true immediately when spawned-session verification is not required", async () => { + await expect( + isResolvedSessionVisibleToRequester({ + requesterSessionKey: "agent:main:main", + targetSessionKey: "agent:main:main", + restrictToSpawned: true, + resolvedViaSessionId: false, + }), + ).resolves.toBe(true); + await expect( + isResolvedSessionVisibleToRequester({ + requesterSessionKey: "agent:main:main", + targetSessionKey: "agent:main:other", + restrictToSpawned: false, + resolvedViaSessionId: false, + }), + ).resolves.toBe(true); + }); +}); diff --git a/src/agents/tools/sessions-resolution.ts b/src/agents/tools/sessions-resolution.ts index b3539d08d8f..f350adb1830 100644 --- a/src/agents/tools/sessions-resolution.ts +++ b/src/agents/tools/sessions-resolution.ts @@ -75,6 +75,43 @@ export async function isRequesterSpawnedSessionVisible(params: { return keys.has(params.targetSessionKey); } +export function shouldVerifyRequesterSpawnedSessionVisibility(params: { + requesterSessionKey: string; + targetSessionKey: string; + restrictToSpawned: boolean; + resolvedViaSessionId: boolean; +}): boolean { + return ( + params.restrictToSpawned && + !params.resolvedViaSessionId && + params.requesterSessionKey !== params.targetSessionKey + ); +} + +export async function isResolvedSessionVisibleToRequester(params: { + requesterSessionKey: string; + targetSessionKey: string; + restrictToSpawned: boolean; + resolvedViaSessionId: boolean; + limit?: number; +}): Promise { + if ( + !shouldVerifyRequesterSpawnedSessionVisibility({ + requesterSessionKey: params.requesterSessionKey, + targetSessionKey: params.targetSessionKey, + restrictToSpawned: params.restrictToSpawned, + resolvedViaSessionId: params.resolvedViaSessionId, + }) + ) { + return true; + } + return await isRequesterSpawnedSessionVisible({ + requesterSessionKey: params.requesterSessionKey, + targetSessionKey: params.targetSessionKey, + limit: params.limit, + }); +} + const SESSION_ID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; export function looksLikeSessionId(value: string): boolean { diff --git a/src/agents/tools/sessions-send-tool.ts b/src/agents/tools/sessions-send-tool.ts index 3479668182c..bb1693c8469 100644 --- a/src/agents/tools/sessions-send-tool.ts +++ b/src/agents/tools/sessions-send-tool.ts @@ -15,7 +15,7 @@ import { createSessionVisibilityGuard, createAgentToAgentPolicy, extractAssistantText, - isRequesterSpawnedSessionVisible, + isResolvedSessionVisibleToRequester, resolveEffectiveSessionToolsVisibility, resolveSessionReference, resolveSandboxedSessionToolContext, @@ -176,19 +176,19 @@ export function createSessionsSendTool(opts?: { const displayKey = resolvedSession.displayKey; const resolvedViaSessionId = resolvedSession.resolvedViaSessionId; - if (restrictToSpawned && !resolvedViaSessionId && resolvedKey !== effectiveRequesterKey) { - const ok = await isRequesterSpawnedSessionVisible({ - requesterSessionKey: effectiveRequesterKey, - targetSessionKey: resolvedKey, + const visible = await isResolvedSessionVisibleToRequester({ + requesterSessionKey: effectiveRequesterKey, + targetSessionKey: resolvedKey, + restrictToSpawned, + resolvedViaSessionId, + }); + if (!visible) { + return jsonResult({ + runId: crypto.randomUUID(), + status: "forbidden", + error: `Session not visible from this sandboxed agent session: ${sessionKey}`, + sessionKey: displayKey, }); - if (!ok) { - return jsonResult({ - runId: crypto.randomUUID(), - status: "forbidden", - error: `Session not visible from this sandboxed agent session: ${sessionKey}`, - sessionKey: displayKey, - }); - } } const timeoutSeconds = typeof params.timeoutSeconds === "number" && Number.isFinite(params.timeoutSeconds)