From 292eae6f9c850f21f6ce6b2ca11059092ac97da4 Mon Sep 17 00:00:00 2001 From: "J. Gavin Ray" Date: Fri, 20 Mar 2026 20:05:34 -0700 Subject: [PATCH] fix(tool-policy): include alsoAllow entries in plugin tool allowlist for subagents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: Plugin tools registered via the plugin system were silently unavailable in subagent sessions even when explicitly listed in tools.subagents.tools.alsoAllow. Users following the documented alsoAllow pattern would add the config, observe no error, but find their subagents unable to call the plugin tools they expected. Root cause (three places): 1. resolveSubagentToolPolicyForSession() in pi-tools.policy.ts returned { allow, deny } and dropped alsoAllow off the object entirely. When only alsoAllow was configured (no explicit allow list), allow remained undefined and alsoAllow entries were lost before reaching collectExplicitAllowlist(). 2. collectExplicitAllowlist() in tool-policy.ts only iterated policy.allow. The ToolPolicyLike type also lacked the alsoAllow field entirely. 3. expandPolicyWithPluginGroups() reconstructed the policy as { allow, deny } and silently discarded alsoAllow — a latent footgun for callers that expect the transformed policy to still carry the field. Fix: - sandbox/types.ts: add alsoAllow to SandboxToolPolicy so the field is recognized throughout the policy pipeline at the type level. - tool-policy.ts (ToolPolicyLike): add alsoAllow field. - tool-policy.ts (collectExplicitAllowlist): merge policy.allow and policy.alsoAllow before iterating so both sources contribute to the plugin tool allowlist passed to resolvePluginTools(). - tool-policy.ts (expandPolicyWithPluginGroups): pass alsoAllow through the expansion so the field is not silently dropped on transformed policies. - pi-tools.policy.ts (resolveSubagentToolPolicyForSession): preserve alsoAllow on the returned object. allow stays undefined in the alsoAllow-only case (correct for access gating via filterToolsByPolicy), but alsoAllow is no longer discarded before collectExplicitAllowlist(). - tool-policy.test.ts: add six unit tests directly exercising collectExplicitAllowlist with allow-only, alsoAllow-only, both, empty, undefined, and multi-policy inputs. Impact: With this fix, the documented config pattern works as intended: tools: subagents: tools: alsoAllow: ["openrag_search"] All 65 existing and new tool-policy and pi-tools.policy tests pass. No behavior change for callers that only use policy.allow. --- src/agents/pi-tools.policy.ts | 9 +++++++-- src/agents/sandbox/types.ts | 1 + src/agents/tool-policy.test.ts | 36 ++++++++++++++++++++++++++++++++++ src/agents/tool-policy.ts | 14 +++++++++---- 4 files changed, 54 insertions(+), 6 deletions(-) diff --git a/src/agents/pi-tools.policy.ts b/src/agents/pi-tools.policy.ts index 4e7cea7c94e..af3b9fe992b 100644 --- a/src/agents/pi-tools.policy.ts +++ b/src/agents/pi-tools.policy.ts @@ -89,7 +89,7 @@ export function resolveSubagentToolPolicy(cfg?: OpenClawConfig, depth?: number): ...(Array.isArray(configured?.deny) ? configured.deny : []), ]; const mergedAllow = allow && alsoAllow ? Array.from(new Set([...allow, ...alsoAllow])) : allow; - return { allow: mergedAllow, deny }; + return { allow: mergedAllow, alsoAllow, deny }; } export function resolveSubagentToolPolicyForSession( @@ -109,8 +109,13 @@ export function resolveSubagentToolPolicyForSession( ), ...(Array.isArray(configured?.deny) ? configured.deny : []), ]; + // Merge allow + alsoAllow when both are present (keeps allow list accurate for + // access gating). When only alsoAllow is set, preserve it on the returned + // object so collectExplicitAllowlist() can build the plugin tool allowlist — + // callers that only gate on `allow` remain unaffected because allow stays + // undefined, but the alsoAllow entries are no longer silently dropped. const mergedAllow = allow && alsoAllow ? Array.from(new Set([...allow, ...alsoAllow])) : allow; - return { allow: mergedAllow, deny }; + return { allow: mergedAllow, alsoAllow, deny }; } export function filterToolsByPolicy(tools: AnyAgentTool[], policy?: SandboxToolPolicy) { diff --git a/src/agents/sandbox/types.ts b/src/agents/sandbox/types.ts index 482ce6a922e..72430dcd43d 100644 --- a/src/agents/sandbox/types.ts +++ b/src/agents/sandbox/types.ts @@ -6,6 +6,7 @@ export type { SandboxDockerConfig } from "./types.docker.js"; export type SandboxToolPolicy = { allow?: string[]; + alsoAllow?: string[]; deny?: string[]; }; diff --git a/src/agents/tool-policy.test.ts b/src/agents/tool-policy.test.ts index 963c703a409..056cc7f26db 100644 --- a/src/agents/tool-policy.test.ts +++ b/src/agents/tool-policy.test.ts @@ -5,6 +5,7 @@ import type { SandboxToolPolicy } from "./sandbox/types.js"; import { TOOL_POLICY_CONFORMANCE } from "./tool-policy.conformance.js"; import { applyOwnerOnlyToolPolicy, + collectExplicitAllowlist, expandToolGroups, isOwnerOnlyToolName, normalizeToolName, @@ -225,3 +226,38 @@ describe("resolveSandboxToolPolicyForAgent", () => { expect(resolved.deny).toEqual(["image"]); }); }); + +describe("collectExplicitAllowlist", () => { + it("collects entries from allow", () => { + expect(collectExplicitAllowlist([{ allow: ["read", "write"] }])).toEqual(["read", "write"]); + }); + + it("collects entries from alsoAllow when allow is absent", () => { + expect(collectExplicitAllowlist([{ alsoAllow: ["openrag_search"] }])).toEqual([ + "openrag_search", + ]); + }); + + it("collects entries from both allow and alsoAllow", () => { + expect( + collectExplicitAllowlist([{ allow: ["read"], alsoAllow: ["openrag_search"] }]), + ).toEqual(["read", "openrag_search"]); + }); + + it("returns empty array when policy has neither allow nor alsoAllow", () => { + expect(collectExplicitAllowlist([{ deny: ["exec"] }])).toEqual([]); + }); + + it("returns empty array for undefined policies", () => { + expect(collectExplicitAllowlist([undefined, undefined])).toEqual([]); + }); + + it("collects alsoAllow across multiple policies", () => { + expect( + collectExplicitAllowlist([ + { alsoAllow: ["openrag_search"] }, + { alsoAllow: ["custom_tool"] }, + ]), + ).toEqual(["openrag_search", "custom_tool"]); + }); +}); diff --git a/src/agents/tool-policy.ts b/src/agents/tool-policy.ts index 5538fb765ce..49aeba5f002 100644 --- a/src/agents/tool-policy.ts +++ b/src/agents/tool-policy.ts @@ -58,6 +58,7 @@ export function applyOwnerOnlyToolPolicy(tools: AnyAgentTool[], senderIsOwner: b export type ToolPolicyLike = { allow?: string[]; + alsoAllow?: string[]; deny?: string[]; }; @@ -75,10 +76,14 @@ export type AllowlistResolution = { export function collectExplicitAllowlist(policies: Array): string[] { const entries: string[] = []; for (const policy of policies) { - if (!policy?.allow) { - continue; - } - for (const value of policy.allow) { + // Collect both `allow` and `alsoAllow` entries. `alsoAllow` is the additive + // form used when a profile is set and the user wants to extend it with + // additional tools (e.g. plugin tools for subagents) without replacing the + // full allow list. Without including `alsoAllow` here, plugin tools listed + // in `tools.subagents.tools.alsoAllow` are silently ignored and never reach + // subagent sessions. + const sources = [...(policy?.allow ?? []), ...(policy?.alsoAllow ?? [])]; + for (const value of sources) { if (typeof value !== "string") { continue; } @@ -149,6 +154,7 @@ export function expandPolicyWithPluginGroups( } return { allow: expandPluginGroups(policy.allow, groups), + alsoAllow: expandPluginGroups(policy.alsoAllow, groups), deny: expandPluginGroups(policy.deny, groups), }; }