Skip to content

Commit f96cfa3

Browse files
pgondhi987pgondhi987
committed
addressing codex review
1 parent 3cfd674 commit f96cfa3

8 files changed

Lines changed: 218 additions & 2 deletions

‎src/agents/pi-embedded-runner/effective-tool-policy.test.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
import fs from "node:fs";
2+
import os from "node:os";
3+
import path from "node:path";
14
import { describe, expect, it } from "vitest";
25
import { setPluginToolMeta } from "../../plugins/tools.js";
36
import { providerAliasCases } from "../test-helpers/provider-alias-cases.js";
@@ -46,6 +49,43 @@ describe("applyFinalEffectiveToolPolicy", () => {
4649
expect(filtered.map((tool) => tool.name)).toEqual(["mcp__bundle__fs_read"]);
4750
});
4851

52+
it("filters bundled tools through inherited subagent allowlists", () => {
53+
const agentId = `bundled-inherited-allow-${Date.now()}-${Math.random().toString(16).slice(2)}`;
54+
const sessionKey = `agent:${agentId}:subagent:limited`;
55+
const storePath = path.join(os.tmpdir(), `openclaw-bundled-inherited-allow-${agentId}.json`);
56+
fs.writeFileSync(
57+
storePath,
58+
JSON.stringify(
59+
{
60+
[sessionKey]: {
61+
sessionId: "limited-session",
62+
updatedAt: Date.now(),
63+
spawnDepth: 1,
64+
subagentRole: "orchestrator",
65+
subagentControlScope: "children",
66+
inheritedToolAllow: ["mcp__bundle__fs_read"],
67+
},
68+
},
69+
null,
70+
2,
71+
),
72+
"utf-8",
73+
);
74+
75+
const filtered = applyFinalEffectiveToolPolicy({
76+
bundledTools: [makeTool("mcp__bundle__fs_delete"), makeTool("mcp__bundle__fs_read")],
77+
config: {
78+
session: {
79+
store: storePath,
80+
},
81+
},
82+
sessionKey,
83+
warn: () => {},
84+
});
85+
86+
expect(filtered.map((tool) => tool.name)).toEqual(["mcp__bundle__fs_read"]);
87+
});
88+
4989
it("applies channel-normalized per-sender policy to bundled tools", () => {
5090
const filtered = applyFinalEffectiveToolPolicy({
5191
bundledTools: [makeTool("mcp__bundle__exec"), makeTool("mcp__bundle__read")],

‎src/agents/pi-embedded-runner/effective-tool-policy.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { getPluginToolMeta } from "../../plugins/tools.js";
33
import {
44
resolveEffectiveToolPolicy,
55
resolveGroupToolPolicy,
6+
resolveInheritedToolPolicyForSession,
67
resolveTrustedGroupId,
78
resolveSubagentToolPolicyForSession,
89
} from "../pi-tools.policy.js";
@@ -136,6 +137,13 @@ export function applyFinalEffectiveToolPolicy(
136137
store: subagentStore,
137138
})
138139
: undefined;
140+
const inheritedToolPolicy = resolveInheritedToolPolicyForSession(
141+
params.config,
142+
params.sessionKey,
143+
{
144+
store: subagentStore,
145+
},
146+
);
139147
const ownerFiltered = applyOwnerOnlyToolPolicy(
140148
params.bundledTools,
141149
params.senderIsOwner === true,
@@ -169,6 +177,7 @@ export function applyFinalEffectiveToolPolicy(
169177
}),
170178
{ policy: params.sandboxToolPolicy, label: "sandbox tools.allow" },
171179
{ policy: subagentPolicy, label: "subagent tools.allow" },
180+
{ policy: inheritedToolPolicy, label: "inherited tools" },
172181
].map((step) => Object.assign({}, step, { suppressUnavailableCoreToolWarning: true }));
173182
return applyToolPolicyPipeline({
174183
tools: ownerFiltered,

‎src/agents/pi-embedded-runner/run/attempt.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ import {
125125
import {
126126
resolveEffectiveToolPolicy,
127127
resolveGroupToolPolicy,
128+
resolveInheritedToolPolicyForSession,
128129
resolveSubagentToolPolicyForSession,
129130
} from "../../pi-tools.policy.js";
130131
import { wrapStreamFnTextTransforms } from "../../plugin-text-transforms.js";
@@ -763,6 +764,13 @@ function collectAttemptExplicitToolAllowlistSources(params: {
763764
store: subagentStore,
764765
})
765766
: undefined;
767+
const inheritedToolPolicy = resolveInheritedToolPolicyForSession(
768+
params.config,
769+
params.sandboxSessionKey,
770+
{
771+
store: subagentStore,
772+
},
773+
);
766774
return collectExplicitToolAllowlistSources([
767775
{ label: "tools.allow", allow: globalPolicy?.allow },
768776
{ label: "tools.byProvider.allow", allow: globalProviderPolicy?.allow },
@@ -777,6 +785,7 @@ function collectAttemptExplicitToolAllowlistSources(params: {
777785
{ label: "group tools.allow", allow: groupPolicy?.allow },
778786
{ label: "sandbox tools.allow", allow: params.sandboxToolPolicy?.allow },
779787
{ label: "subagent tools.allow", allow: subagentPolicy?.allow },
788+
{ label: "inherited tools.allow", allow: inheritedToolPolicy?.allow },
780789
{ label: "runtime toolsAllow", allow: params.toolsAllow, enforceWhenToolsDisabled: true },
781790
]);
782791
}

‎src/agents/pi-tools.create-openclaw-coding-tools.test.ts‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -485,6 +485,41 @@ describe("createOpenClawCodingTools", () => {
485485
expectListIncludes(latestCreateOpenClawToolsOptions().pluginToolDenylist, ["pdf"]);
486486
});
487487

488+
it("passes inherited allowlist entries to OpenClaw plugin discovery", async () => {
489+
const createOpenClawToolsMock = vi.mocked(createOpenClawTools);
490+
createOpenClawToolsMock.mockClear();
491+
const agentId = `inherited-allow-${Date.now()}-${Math.random().toString(16).slice(2)}`;
492+
const storeTemplate = path.join(
493+
os.tmpdir(),
494+
`openclaw-session-store-${agentId}-{agentId}.json`,
495+
);
496+
await writeSessionStore(storeTemplate, agentId, {
497+
[`agent:${agentId}:subagent:limited`]: {
498+
sessionId: "limited-session",
499+
updatedAt: Date.now(),
500+
spawnDepth: 1,
501+
subagentRole: "orchestrator",
502+
subagentControlScope: "children",
503+
inheritedToolAllow: ["custom_plugin_tool", "sessions_spawn"],
504+
},
505+
});
506+
507+
createOpenClawCodingTools({
508+
sessionKey: `agent:${agentId}:subagent:limited`,
509+
config: {
510+
session: {
511+
store: storeTemplate,
512+
},
513+
},
514+
});
515+
516+
expect(createOpenClawToolsMock).toHaveBeenCalledTimes(1);
517+
expectListIncludes(latestCreateOpenClawToolsOptions().pluginToolAllowlist, [
518+
"custom_plugin_tool",
519+
"sessions_spawn",
520+
]);
521+
});
522+
488523
it("passes effective allow-list-restricted tool surface to spawned sessions", () => {
489524
const createOpenClawToolsMock = vi.mocked(createOpenClawTools);
490525
createOpenClawToolsMock.mockClear();

‎src/agents/pi-tools.policy.test.ts‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -408,6 +408,88 @@ describe("resolveSubagentToolPolicy depth awareness", () => {
408408
expect(isToolAllowedByPolicyName("sessions_spawn", policy)).toBe(true);
409409
});
410410

411+
it("applies inherited tool allows from stored subagent sessions", () => {
412+
const storePath = path.join(
413+
os.tmpdir(),
414+
`openclaw-subagent-inherited-allow-${Date.now()}-${Math.random().toString(16).slice(2)}.json`,
415+
);
416+
fs.mkdirSync(path.dirname(storePath), { recursive: true });
417+
fs.writeFileSync(
418+
storePath,
419+
JSON.stringify(
420+
{
421+
"agent:main:subagent:limited": {
422+
sessionId: "limited-session",
423+
updatedAt: Date.now(),
424+
spawnDepth: 1,
425+
subagentRole: "orchestrator",
426+
subagentControlScope: "children",
427+
inheritedToolAllow: ["sessions_spawn", "memory_search"],
428+
},
429+
},
430+
null,
431+
2,
432+
),
433+
"utf-8",
434+
);
435+
const cfg = {
436+
...baseCfg,
437+
session: {
438+
store: storePath,
439+
},
440+
} as unknown as OpenClawConfig;
441+
442+
const policy = resolveSubagentToolPolicyForSession(cfg, "agent:main:subagent:limited");
443+
expect(isToolAllowedByPolicyName("sessions_spawn", policy)).toBe(true);
444+
expect(isToolAllowedByPolicyName("memory_search", policy)).toBe(true);
445+
expect(isToolAllowedByPolicyName("read", policy)).toBe(false);
446+
expect(isToolAllowedByPolicyName("exec", policy)).toBe(false);
447+
});
448+
449+
it("intersects configured subagent allows with inherited tool allows", () => {
450+
const storePath = path.join(
451+
os.tmpdir(),
452+
`openclaw-subagent-inherited-allow-intersection-${Date.now()}-${Math.random().toString(16).slice(2)}.json`,
453+
);
454+
fs.mkdirSync(path.dirname(storePath), { recursive: true });
455+
fs.writeFileSync(
456+
storePath,
457+
JSON.stringify(
458+
{
459+
"agent:main:subagent:limited": {
460+
sessionId: "limited-session",
461+
updatedAt: Date.now(),
462+
spawnDepth: 1,
463+
subagentRole: "orchestrator",
464+
subagentControlScope: "children",
465+
inheritedToolAllow: ["sessions_spawn", "read"],
466+
},
467+
},
468+
null,
469+
2,
470+
),
471+
"utf-8",
472+
);
473+
const cfg = {
474+
...baseCfg,
475+
tools: {
476+
subagents: {
477+
tools: {
478+
allow: ["sessions_spawn", "exec"],
479+
},
480+
},
481+
},
482+
session: {
483+
store: storePath,
484+
},
485+
} as unknown as OpenClawConfig;
486+
487+
const policy = resolveSubagentToolPolicyForSession(cfg, "agent:main:subagent:limited");
488+
expect(isToolAllowedByPolicyName("sessions_spawn", policy)).toBe(true);
489+
expect(isToolAllowedByPolicyName("read", policy)).toBe(false);
490+
expect(isToolAllowedByPolicyName("exec", policy)).toBe(false);
491+
});
492+
411493
it("applies inherited tool policy from stored ACP sessions without subagent metadata", () => {
412494
const storePath = path.join(
413495
os.tmpdir(),

‎src/agents/pi-tools.policy.ts‎

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import {
3030
} from "./subagent-capabilities.js";
3131
import { isToolAllowedByPolicyName } from "./tool-policy-match.js";
3232
import {
33+
expandToolGroups,
3334
mergeAlsoAllowPolicy,
3435
normalizeToolName,
3536
resolveToolProfilePolicy,
@@ -60,6 +61,7 @@ const SUBAGENT_TOOL_DENY_LEAF = [
6061
"sessions_history",
6162
"sessions_spawn",
6263
];
64+
const NO_MATCHING_TOOL_ALLOWLIST_ENTRY = "__openclaw_no_matching_subagent_tool_allow__";
6365

6466
/**
6567
* Build the deny list for a sub-agent at a given depth.
@@ -86,6 +88,39 @@ function resolveSubagentDenyListForRole(role: SubagentSessionRole): string[] {
8688
return [...SUBAGENT_TOOL_DENY_ALWAYS];
8789
}
8890

91+
function mergeConfiguredSubagentAllow(
92+
allow: string[] | undefined,
93+
alsoAllow: string[] | undefined,
94+
): string[] | undefined {
95+
return allow && alsoAllow ? Array.from(new Set([...allow, ...alsoAllow])) : allow;
96+
}
97+
98+
function intersectToolAllowlists(
99+
left: string[] | undefined,
100+
right: string[] | undefined,
101+
): string[] | undefined {
102+
if (!right || right.length === 0) {
103+
return left;
104+
}
105+
if (!left || left.length === 0) {
106+
return right;
107+
}
108+
const leftPolicy = { allow: left };
109+
const rightPolicy = { allow: right };
110+
const result = new Set<string>();
111+
for (const entry of expandToolGroups(left)) {
112+
if (isToolAllowedByPolicyName(entry, rightPolicy)) {
113+
result.add(entry);
114+
}
115+
}
116+
for (const entry of expandToolGroups(right)) {
117+
if (isToolAllowedByPolicyName(entry, leftPolicy)) {
118+
result.add(entry);
119+
}
120+
}
121+
return result.size > 0 ? [...result] : [NO_MATCHING_TOOL_ALLOWLIST_ENTRY];
122+
}
123+
89124
export function resolveSubagentToolPolicy(cfg?: OpenClawConfig, depth?: number): SandboxToolPolicy {
90125
const configured = cfg?.tools?.subagents?.tools;
91126
const maxSpawnDepth =
@@ -101,7 +136,7 @@ export function resolveSubagentToolPolicy(cfg?: OpenClawConfig, depth?: number):
101136
...baseDeny.filter((toolName) => !explicitAllow.has(normalizeToolName(toolName))),
102137
...(Array.isArray(configured?.deny) ? configured.deny : []),
103138
];
104-
const mergedAllow = allow && alsoAllow ? Array.from(new Set([...allow, ...alsoAllow])) : allow;
139+
const mergedAllow = mergeConfiguredSubagentAllow(allow, alsoAllow);
105140
return { allow: mergedAllow, deny };
106141
}
107142

@@ -124,6 +159,7 @@ export function resolveSubagentToolPolicyForSession(
124159
const inheritedToolPolicy = resolveInheritedToolPolicyForSession(cfg, sessionKey, {
125160
store,
126161
});
162+
const inheritedToolAllow = inheritedToolPolicy?.allow;
127163
const inheritedToolDeny = inheritedToolPolicy?.deny ?? [];
128164
const allow = Array.isArray(configured?.allow) ? configured.allow : undefined;
129165
const alsoAllow = Array.isArray(configured?.alsoAllow) ? configured.alsoAllow : undefined;
@@ -137,7 +173,10 @@ export function resolveSubagentToolPolicyForSession(
137173
...inheritedToolDeny,
138174
...(Array.isArray(configured?.deny) ? configured.deny : []),
139175
];
140-
const mergedAllow = allow && alsoAllow ? Array.from(new Set([...allow, ...alsoAllow])) : allow;
176+
const mergedAllow = intersectToolAllowlists(
177+
mergeConfiguredSubagentAllow(allow, alsoAllow),
178+
inheritedToolAllow,
179+
);
141180
return { allow: mergedAllow, deny };
142181
}
143182

‎src/agents/pi-tools.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -782,6 +782,7 @@ export function createOpenClawCodingTools(options?: {
782782
senderPolicy,
783783
sandboxToolPolicy,
784784
subagentPolicy,
785+
inheritedToolPolicy,
785786
options?.runtimeToolAllowlist ? { allow: options.runtimeToolAllowlist } : undefined,
786787
]);
787788
const pluginToolDenylist = collectExplicitDenylist([

‎src/gateway/tool-resolution.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,7 @@ export function resolveGatewayScopedTools(params: {
176176
agentProviderPolicy,
177177
groupPolicy,
178178
subagentPolicy,
179+
inheritedToolPolicy,
179180
gatewayRequestedTools.length > 0 ? { allow: gatewayRequestedTools } : undefined,
180181
]),
181182
pluginToolDenylist: explicitDenylist,

0 commit comments

Comments
 (0)