fix(policy): preserve restrictive tool allowlists

Co-authored-by: David Silva <david.silva@gendigital.com>

src/agents/sandbox-tool-policy.test.ts

@@ -0,0 +1,27 @@
+import { describe, expect, it } from "vitest";
+import { pickSandboxToolPolicy } from "./sandbox-tool-policy.js";
+
+describe("pickSandboxToolPolicy", () => {
+  it("treats alsoAllow without allow as a restrictive allowlist", () => {
+    expect(
+      pickSandboxToolPolicy({
+        alsoAllow: ["web_search"],
+      }),
+    ).toEqual({
+      allow: ["web_search"],
+      deny: undefined,
+    });
+  });
+
+  it("preserves allow-all semantics for allow: [] plus alsoAllow", () => {
+    expect(
+      pickSandboxToolPolicy({
+        allow: [],
+        alsoAllow: ["web_search"],
+      }),
+    ).toEqual({
+      allow: [],
+      deny: undefined,
+    });
+  });
+});

src/agents/sandbox-tool-policy.ts

@@ -10,10 +10,11 @@ function unionAllow(base?: string[], extra?: string[]): string[] | undefined {
   if (!Array.isArray(extra) || extra.length === 0) {
     return base;
   }
-  // If the user is using alsoAllow without an allowlist, treat it as additive on top of
-  // an implicit allow-all policy.
-  if (!Array.isArray(base) || base.length === 0) {
-    return Array.from(new Set(["*", ...extra]));
+  if (!Array.isArray(base)) {
+    return Array.from(new Set(extra));
+  }
+  if (base.length === 0) {
+    return base;
   }
   return Array.from(new Set([...base, ...extra]));
 }

src/agents/tool-policy-pipeline.test.ts

@@ -36,7 +36,7 @@ describe("tool-policy-pipeline", () => {
     resetToolPolicyWarningCacheForTest();
   });
 
-  test("strips allowlists that would otherwise disable core tools", () => {
+  test("preserves plugin-only allowlists instead of silently stripping them", () => {
     const tools = [{ name: "exec" }, { name: "plugin_tool" }] as unknown as DummyTool[];
     const filtered = applyToolPolicyPipeline({
       // oxlint-disable-next-line typescript/no-explicit-any
@@ -53,7 +53,7 @@ describe("tool-policy-pipeline", () => {
       ],
     });
     const names = filtered.map((t) => (t as unknown as DummyTool).name).toSorted();
-    expect(names).toEqual(["exec", "plugin_tool"]);
+    expect(names).toEqual(["plugin_tool"]);
   });
 
   test("warns about unknown allowlist entries", () => {

src/agents/tool-policy-pipeline.ts

@@ -129,7 +129,7 @@ export function applyToolPolicyPipeline(params: {
           })
         ) {
           const suffix = describeUnknownAllowlistSuffix({
-            strippedAllowlist: resolved.strippedAllowlist,
+            pluginOnlyAllowlist: resolved.pluginOnlyAllowlist,
             hasGatedCoreEntries: gatedCoreEntries.length > 0,
             hasOtherEntries: otherEntries.length > 0,
           });
@@ -164,12 +164,12 @@ function shouldSuppressUnavailableCoreToolWarning(params: {
 }
 
 function describeUnknownAllowlistSuffix(params: {
-  strippedAllowlist: boolean;
+  pluginOnlyAllowlist: boolean;
   hasGatedCoreEntries: boolean;
   hasOtherEntries: boolean;
 }): string {
-  const preface = params.strippedAllowlist
-    ? "Ignoring allowlist so core tools remain available."
+  const preface = params.pluginOnlyAllowlist
+    ? "Allowlist contains only plugin entries; core tools will not be available."
     : "";
   const detail =
     params.hasGatedCoreEntries && params.hasOtherEntries

src/agents/tool-policy.plugin-only-allowlist.test.ts

@@ -8,15 +8,17 @@ const pluginGroups: PluginToolGroups = {
 const coreTools = new Set(["read", "write", "exec", "session_status"]);
 
 describe("stripPluginOnlyAllowlist", () => {
-  it("strips allowlist when it only targets plugin tools", () => {
+  it("preserves allowlist when it only targets plugin tools", () => {
     const policy = stripPluginOnlyAllowlist({ allow: ["lobster"] }, pluginGroups, coreTools);
-    expect(policy.policy?.allow).toBeUndefined();
+    expect(policy.policy?.allow).toEqual(["lobster"]);
+    expect(policy.pluginOnlyAllowlist).toBe(true);
     expect(policy.unknownAllowlist).toEqual([]);
   });
 
-  it("strips allowlist when it only targets plugin groups", () => {
+  it("preserves allowlist when it only targets plugin groups", () => {
     const policy = stripPluginOnlyAllowlist({ allow: ["group:plugins"] }, pluginGroups, coreTools);
-    expect(policy.policy?.allow).toBeUndefined();
+    expect(policy.policy?.allow).toEqual(["group:plugins"]);
+    expect(policy.pluginOnlyAllowlist).toBe(true);
     expect(policy.unknownAllowlist).toEqual([]);
   });
 
@@ -36,10 +38,11 @@ describe("stripPluginOnlyAllowlist", () => {
     expect(policy.unknownAllowlist).toEqual([]);
   });
 
-  it("strips allowlist with unknown entries when no core tools match", () => {
+  it("preserves allowlist with unknown entries when no core tools match", () => {
     const emptyPlugins: PluginToolGroups = { all: [], byPlugin: new Map() };
     const policy = stripPluginOnlyAllowlist({ allow: ["lobster"] }, emptyPlugins, coreTools);
-    expect(policy.policy?.allow).toBeUndefined();
+    expect(policy.policy?.allow).toEqual(["lobster"]);
+    expect(policy.pluginOnlyAllowlist).toBe(true);
     expect(policy.unknownAllowlist).toEqual(["lobster"]);
   });
 

src/agents/tool-policy.ts

@@ -69,7 +69,7 @@ export type PluginToolGroups = {
 export type AllowlistResolution = {
   policy: ToolPolicyLike | undefined;
   unknownAllowlist: string[];
-  strippedAllowlist: boolean;
+  pluginOnlyAllowlist: boolean;
 };
 
 export function collectExplicitAllowlist(policies: Array<ToolPolicyLike | undefined>): string[] {
@@ -159,11 +159,11 @@ export function stripPluginOnlyAllowlist(
   coreTools: Set<string>,
 ): AllowlistResolution {
   if (!policy?.allow || policy.allow.length === 0) {
-    return { policy, unknownAllowlist: [], strippedAllowlist: false };
+    return { policy, unknownAllowlist: [], pluginOnlyAllowlist: false };
   }
   const normalized = normalizeToolList(policy.allow);
   if (normalized.length === 0) {
-    return { policy, unknownAllowlist: [], strippedAllowlist: false };
+    return { policy, unknownAllowlist: [], pluginOnlyAllowlist: false };
   }
   const pluginIds = new Set(groups.byPlugin.keys());
   const pluginTools = new Set(groups.all);
@@ -185,17 +185,11 @@ export function stripPluginOnlyAllowlist(
       unknownAllowlist.push(entry);
     }
   }
-  const strippedAllowlist = !hasCoreEntry;
-  // When an allowlist contains only plugin tools, we strip it to avoid accidentally
-  // disabling core tools. Users who want additive behavior should prefer `tools.alsoAllow`.
-  if (strippedAllowlist) {
-    // Note: logging happens in the caller (pi-tools/tools-invoke) after this function returns.
-    // We keep this note here for future maintainers.
-  }
+  const pluginOnlyAllowlist = !hasCoreEntry;
   return {
-    policy: strippedAllowlist ? { ...policy, allow: undefined } : policy,
+    policy,
     unknownAllowlist: Array.from(new Set(unknownAllowlist)),
-    strippedAllowlist,
+    pluginOnlyAllowlist,
   };
 }