refactor(agents): dedupe workspace and session tool flows

2026-05-09 03:18:26 +00:00 · 2026-02-22 21:18:02 +00:00
parent 2f8c68ae4d
commit 06bdd53658
9 changed files with 227 additions and 128 deletions
--- 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" });

--- 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<string, unknown>) {
+  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",
--- 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";
--- 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);
--- 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);
+  });
+});
--- 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<boolean> {
+  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 {
--- 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)