fix(policy): address review follow-ups

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

@@ -2,6 +2,10 @@ import { describe, expect, it } from "vitest";
 import { pickSandboxToolPolicy } from "./sandbox-tool-policy.js";
 
 describe("pickSandboxToolPolicy", () => {
+  it("returns undefined when neither allow nor deny is configured", () => {
+    expect(pickSandboxToolPolicy({})).toBeUndefined();
+  });
+
   it("treats alsoAllow without allow as a restrictive allowlist", () => {
     expect(
       pickSandboxToolPolicy({
@@ -13,6 +17,18 @@ describe("pickSandboxToolPolicy", () => {
     });
   });
 
+  it("merges allow and alsoAllow when both are present", () => {
+    expect(
+      pickSandboxToolPolicy({
+        allow: ["read"],
+        alsoAllow: ["write"],
+      }),
+    ).toEqual({
+      allow: ["read", "write"],
+      deny: undefined,
+    });
+  });
+
   it("preserves allow-all semantics for allow: [] plus alsoAllow", () => {
     expect(
       pickSandboxToolPolicy({
@@ -24,4 +40,15 @@ describe("pickSandboxToolPolicy", () => {
       deny: undefined,
     });
   });
+
+  it("passes deny through unchanged", () => {
+    expect(
+      pickSandboxToolPolicy({
+        deny: ["exec"],
+      }),
+    ).toEqual({
+      allow: undefined,
+      deny: ["exec"],
+    });
+  });
 });

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

@@ -175,6 +175,58 @@ describe("tool-policy-pipeline", () => {
     expect(warnings).toHaveLength(258);
   });
 
+  test("evicts the oldest warning when the dedupe cache is full", () => {
+    const warnings: string[] = [];
+    const tools = [{ name: "exec" }] as unknown as DummyTool[];
+
+    for (let i = 0; i < 256; i += 1) {
+      applyToolPolicyPipeline({
+        // oxlint-disable-next-line typescript/no-explicit-any
+        tools: tools as any,
+        // oxlint-disable-next-line typescript/no-explicit-any
+        toolMeta: () => undefined,
+        warn: (msg: string) => warnings.push(msg),
+        steps: [
+          {
+            policy: { allow: [`unknown_${i}`] },
+            label: "tools.allow",
+            stripPluginOnlyAllowlist: true,
+          },
+        ],
+      });
+    }
+
+    warnings.length = 0;
+
+    applyToolPolicyPipeline({
+      // oxlint-disable-next-line typescript/no-explicit-any
+      tools: tools as any,
+      // oxlint-disable-next-line typescript/no-explicit-any
+      toolMeta: () => undefined,
+      warn: (msg: string) => warnings.push(msg),
+      steps: [
+        {
+          policy: { allow: ["unknown_256"] },
+          label: "tools.allow",
+          stripPluginOnlyAllowlist: true,
+        },
+      ],
+    });
+    applyToolPolicyPipeline({
+      // oxlint-disable-next-line typescript/no-explicit-any
+      tools: tools as any,
+      // oxlint-disable-next-line typescript/no-explicit-any
+      toolMeta: () => undefined,
+      warn: (msg: string) => warnings.push(msg),
+      steps: [
+        { policy: { allow: ["unknown_0"] }, label: "tools.allow", stripPluginOnlyAllowlist: true },
+      ],
+    });
+
+    expect(warnings).toHaveLength(2);
+    expect(warnings[1]).toContain("unknown_0");
+  });
+
   test("applies allowlist filtering when core tools are explicitly listed", () => {
     const tools = [{ name: "exec" }, { name: "process" }] as unknown as DummyTool[];
     const filtered = applyToolPolicyPipeline({
@@ -193,4 +245,23 @@ describe("tool-policy-pipeline", () => {
     });
     expect(filtered.map((t) => (t as unknown as DummyTool).name)).toEqual(["exec"]);
   });
+
+  test("applies deny filtering after allow filtering", () => {
+    const tools = [{ name: "exec" }, { name: "process" }] as unknown as DummyTool[];
+    const filtered = applyToolPolicyPipeline({
+      // oxlint-disable-next-line typescript/no-explicit-any
+      tools: tools as any,
+      // oxlint-disable-next-line typescript/no-explicit-any
+      toolMeta: () => undefined,
+      warn: () => {},
+      steps: [
+        {
+          policy: { allow: ["exec", "process"], deny: ["process"] },
+          label: "tools.allow",
+          stripPluginOnlyAllowlist: true,
+        },
+      ],
+    });
+    expect(filtered.map((t) => (t as unknown as DummyTool).name)).toEqual(["exec"]);
+  });
 });

src/agents/tool-policy-pipeline.ts

@@ -2,27 +2,29 @@ import { filterToolsByPolicy } from "./pi-tools.policy.js";
 import type { AnyAgentTool } from "./pi-tools.types.js";
 import { isKnownCoreToolId } from "./tool-catalog.js";
 import {
+  analyzeAllowlistByToolType,
   buildPluginToolGroups,
   expandPolicyWithPluginGroups,
   normalizeToolName,
-  stripPluginOnlyAllowlist,
   type ToolPolicyLike,
 } from "./tool-policy.js";
 
 const MAX_TOOL_POLICY_WARNING_CACHE = 256;
 const seenToolPolicyWarnings = new Set<string>();
+const toolPolicyWarningOrder: string[] = [];
 
 function rememberToolPolicyWarning(warning: string): boolean {
   if (seenToolPolicyWarnings.has(warning)) {
     return false;
   }
   if (seenToolPolicyWarnings.size >= MAX_TOOL_POLICY_WARNING_CACHE) {
-    const oldest = seenToolPolicyWarnings.values().next().value;
+    const oldest = toolPolicyWarningOrder.shift();
     if (oldest) {
       seenToolPolicyWarnings.delete(oldest);
     }
   }
   seenToolPolicyWarnings.add(warning);
+  toolPolicyWarningOrder.push(warning);
   return true;
 }
 
@@ -114,7 +116,7 @@ export function applyToolPolicyPipeline(params: {
 
     let policy: ToolPolicyLike | undefined = step.policy;
     if (step.stripPluginOnlyAllowlist) {
-      const resolved = stripPluginOnlyAllowlist(policy, pluginGroups, coreToolNames);
+      const resolved = analyzeAllowlistByToolType(policy, pluginGroups, coreToolNames);
       if (resolved.unknownAllowlist.length > 0) {
         const entries = resolved.unknownAllowlist.join(", ");
         const gatedCoreEntries = resolved.unknownAllowlist.filter((entry) =>
@@ -122,8 +124,8 @@ export function applyToolPolicyPipeline(params: {
         );
         const otherEntries = resolved.unknownAllowlist.filter((entry) => !isKnownCoreToolId(entry));
         if (
-          !shouldSuppressUnavailableCoreToolWarning({
-            suppressUnavailableCoreToolWarning: step.suppressUnavailableCoreToolWarning === true,
+          shouldWarnAboutUnknownAllowlist({
+            suppressUnavailableCoreToolWarning: step.suppressUnavailableCoreToolWarning ?? false,
             hasGatedCoreEntries: gatedCoreEntries.length > 0,
             hasOtherEntries: otherEntries.length > 0,
           })
@@ -148,7 +150,7 @@ export function applyToolPolicyPipeline(params: {
   return filtered;
 }
 
-function shouldSuppressUnavailableCoreToolWarning(params: {
+function shouldWarnAboutUnknownAllowlist(params: {
   suppressUnavailableCoreToolWarning: boolean;
   hasGatedCoreEntries: boolean;
   hasOtherEntries: boolean;
@@ -158,9 +160,9 @@ function shouldSuppressUnavailableCoreToolWarning(params: {
     !params.hasGatedCoreEntries ||
     params.hasOtherEntries
   ) {
-    return false;
+    return true;
   }
-  return true;
+  return false;
 }
 
 function describeUnknownAllowlistSuffix(params: {
@@ -182,4 +184,5 @@ function describeUnknownAllowlistSuffix(params: {
 
 export function resetToolPolicyWarningCacheForTest(): void {
   seenToolPolicyWarnings.clear();
+  toolPolicyWarningOrder.length = 0;
 }

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

@@ -1,5 +1,9 @@
 import { describe, expect, it } from "vitest";
-import { stripPluginOnlyAllowlist, type PluginToolGroups } from "./tool-policy.js";
+import {
+  analyzeAllowlistByToolType,
+  buildPluginToolGroups,
+  type PluginToolGroups,
+} from "./tool-policy.js";
 
 const pluginGroups: PluginToolGroups = {
   all: ["lobster", "workflow_tool"],
@@ -7,29 +11,33 @@ const pluginGroups: PluginToolGroups = {
 };
 const coreTools = new Set(["read", "write", "exec", "session_status"]);
 
-describe("stripPluginOnlyAllowlist", () => {
+describe("analyzeAllowlistByToolType", () => {
   it("preserves allowlist when it only targets plugin tools", () => {
-    const policy = stripPluginOnlyAllowlist({ allow: ["lobster"] }, pluginGroups, coreTools);
+    const policy = analyzeAllowlistByToolType({ allow: ["lobster"] }, pluginGroups, coreTools);
     expect(policy.policy?.allow).toEqual(["lobster"]);
     expect(policy.pluginOnlyAllowlist).toBe(true);
     expect(policy.unknownAllowlist).toEqual([]);
   });
 
   it("preserves allowlist when it only targets plugin groups", () => {
-    const policy = stripPluginOnlyAllowlist({ allow: ["group:plugins"] }, pluginGroups, coreTools);
+    const policy = analyzeAllowlistByToolType(
+      { allow: ["group:plugins"] },
+      pluginGroups,
+      coreTools,
+    );
     expect(policy.policy?.allow).toEqual(["group:plugins"]);
     expect(policy.pluginOnlyAllowlist).toBe(true);
     expect(policy.unknownAllowlist).toEqual([]);
   });
 
   it('keeps allowlist when it uses "*"', () => {
-    const policy = stripPluginOnlyAllowlist({ allow: ["*"] }, pluginGroups, coreTools);
+    const policy = analyzeAllowlistByToolType({ allow: ["*"] }, pluginGroups, coreTools);
     expect(policy.policy?.allow).toEqual(["*"]);
     expect(policy.unknownAllowlist).toEqual([]);
   });
 
   it("keeps allowlist when it mixes plugin and core entries", () => {
-    const policy = stripPluginOnlyAllowlist(
+    const policy = analyzeAllowlistByToolType(
       { allow: ["lobster", "read"] },
       pluginGroups,
       coreTools,
@@ -40,7 +48,7 @@ describe("stripPluginOnlyAllowlist", () => {
 
   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);
+    const policy = analyzeAllowlistByToolType({ allow: ["lobster"] }, emptyPlugins, coreTools);
     expect(policy.policy?.allow).toEqual(["lobster"]);
     expect(policy.pluginOnlyAllowlist).toBe(true);
     expect(policy.unknownAllowlist).toEqual(["lobster"]);
@@ -48,7 +56,7 @@ describe("stripPluginOnlyAllowlist", () => {
 
   it("keeps allowlist with core tools and reports unknown entries", () => {
     const emptyPlugins: PluginToolGroups = { all: [], byPlugin: new Map() };
-    const policy = stripPluginOnlyAllowlist(
+    const policy = analyzeAllowlistByToolType(
       { allow: ["read", "lobster"] },
       emptyPlugins,
       coreTools,
@@ -56,4 +64,13 @@ describe("stripPluginOnlyAllowlist", () => {
     expect(policy.policy?.allow).toEqual(["read", "lobster"]);
     expect(policy.unknownAllowlist).toEqual(["lobster"]);
   });
+
+  it("ignores empty plugin ids when building groups", () => {
+    const groups = buildPluginToolGroups({
+      tools: [{ name: "lobster" }],
+      toolMeta: () => ({ pluginId: "" }),
+    });
+    expect(groups.all).toEqual(["lobster"]);
+    expect(groups.byPlugin.size).toBe(0);
+  });
 });

src/agents/tool-policy.ts

@@ -104,7 +104,10 @@ export function buildPluginToolGroups<T extends { name: string }>(params: {
     }
     const name = normalizeToolName(tool.name);
     all.push(name);
-    const pluginId = meta.pluginId.toLowerCase();
+    const pluginId = meta.pluginId.trim().toLowerCase();
+    if (!pluginId) {
+      continue;
+    }
     const list = byPlugin.get(pluginId) ?? [];
     list.push(name);
     byPlugin.set(pluginId, list);
@@ -153,7 +156,7 @@ export function expandPolicyWithPluginGroups(
   };
 }
 
-export function stripPluginOnlyAllowlist(
+export function analyzeAllowlistByToolType(
   policy: ToolPolicyLike | undefined,
   groups: PluginToolGroups,
   coreTools: Set<string>,