Skip to content

Commit 80980fe

Browse files
vincentkocvincentkoc
committed
fix(mcp): snapshot plugin tool bridge metadata
(cherry picked from commit 03f8592)
1 parent 71b1359 commit 80980fe

4 files changed

Lines changed: 271 additions & 41 deletions

File tree

‎src/mcp/plugin-tools-handlers.test.ts‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
import { describe, expect, it, vi } from "vitest";
2+
import type { AnyAgentTool } from "../agents/tools/common.js";
3+
import { createPluginToolsMcpHandlers } from "./plugin-tools-handlers.js";
4+
5+
function createToolWithUnreadableField(field: "name" | "parameters") {
6+
const tool = {
7+
name: "poisoned_tool",
8+
description: "Poisoned tool",
9+
parameters: { type: "object", properties: {} },
10+
execute: vi.fn(),
11+
};
12+
Object.defineProperty(tool, field, {
13+
enumerable: true,
14+
get() {
15+
throw new Error(`${field} revoked`);
16+
},
17+
});
18+
return tool as never;
19+
}
20+
21+
function createToolWithPoisonedExecuteBind() {
22+
const execute = vi.fn().mockResolvedValue({ content: "Poisoned execute still callable." });
23+
Object.defineProperty(execute, "bind", {
24+
get() {
25+
throw new Error("bind revoked");
26+
},
27+
});
28+
return {
29+
name: "poisoned_bind",
30+
description: "Poisoned bind",
31+
parameters: { type: "object", properties: {} },
32+
execute,
33+
} as unknown as AnyAgentTool;
34+
}
35+
36+
describe("createPluginToolsMcpHandlers", () => {
37+
it("isolates unreadable plugin tool metadata on the MCP bridge", async () => {
38+
const execute = vi.fn().mockResolvedValue({ content: "Stored." });
39+
const healthyTool = {
40+
name: "memory_recall",
41+
description: "Recall stored memory",
42+
parameters: {
43+
type: "object",
44+
properties: {
45+
query: { type: "string" },
46+
},
47+
},
48+
execute,
49+
} as unknown as AnyAgentTool;
50+
51+
const handlers = createPluginToolsMcpHandlers([
52+
createToolWithUnreadableField("parameters"),
53+
healthyTool,
54+
]);
55+
56+
await expect(handlers.listTools()).resolves.toEqual({
57+
tools: [
58+
{
59+
name: "memory_recall",
60+
description: "Recall stored memory",
61+
inputSchema: {
62+
type: "object",
63+
properties: {
64+
query: { type: "string" },
65+
},
66+
},
67+
},
68+
],
69+
});
70+
71+
const result = await handlers.callTool({
72+
name: "memory_recall",
73+
arguments: { query: "remember this" },
74+
});
75+
expect(execute).toHaveBeenCalledOnce();
76+
expect(result.content).toEqual([{ type: "text", text: "Stored." }]);
77+
});
78+
79+
it("does not read plugin execute bind while snapshotting MCP tools", async () => {
80+
const handlers = createPluginToolsMcpHandlers([createToolWithPoisonedExecuteBind()]);
81+
82+
await expect(handlers.listTools()).resolves.toEqual({
83+
tools: [
84+
{
85+
name: "poisoned_bind",
86+
description: "Poisoned bind",
87+
inputSchema: { type: "object", properties: {} },
88+
},
89+
],
90+
});
91+
await expect(
92+
handlers.callTool({
93+
name: "poisoned_bind",
94+
arguments: {},
95+
}),
96+
).resolves.toEqual({
97+
content: [{ type: "text", text: "Poisoned execute still callable." }],
98+
});
99+
});
100+
});

‎src/mcp/plugin-tools-handlers.ts‎

Lines changed: 125 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,10 @@ import {
44
rewrapToolWithBeforeToolCallHook,
55
wrapToolWithBeforeToolCallHook,
66
} from "../agents/agent-tools.before-tool-call.js";
7+
import { copyChannelAgentToolMeta } from "../agents/channel-tools.js";
78
import type { AnyAgentTool } from "../agents/tools/common.js";
89
import { formatErrorMessage } from "../infra/errors.js";
10+
import { copyPluginToolMeta } from "../plugins/tool-metadata.js";
911
import { coerceChatContentText } from "../shared/chat-content.js";
1012

1113
type CallPluginToolParams = {
@@ -21,14 +23,132 @@ function resolveJsonSchemaForTool(tool: AnyAgentTool): Record<string, unknown> {
2123
return { type: "object", properties: {} };
2224
}
2325

24-
export function createPluginToolsMcpHandlers(tools: AnyAgentTool[]) {
25-
const wrappedTools = tools.map((tool) => {
26+
function snapshotMcpPluginTool(tool: AnyAgentTool): AnyAgentTool | undefined {
27+
let name: unknown;
28+
let description: unknown;
29+
let parameters: unknown;
30+
let execute: unknown;
31+
try {
32+
name = tool.name;
33+
description = tool.description;
34+
parameters = tool.parameters;
35+
execute = tool.execute;
36+
} catch {
37+
return undefined;
38+
}
39+
if (typeof name !== "string" || !name.trim() || typeof execute !== "function") {
40+
return undefined;
41+
}
42+
43+
let prototype: object | null;
44+
try {
45+
prototype = Reflect.getPrototypeOf(tool);
46+
} catch {
47+
return undefined;
48+
}
49+
const descriptors = copyReadableMcpPluginToolDescriptors(tool);
50+
if (!descriptors) {
51+
return undefined;
52+
}
53+
descriptors.name = {
54+
configurable: true,
55+
enumerable: true,
56+
value: name.trim(),
57+
writable: true,
58+
};
59+
descriptors.description = {
60+
configurable: true,
61+
enumerable: true,
62+
value: typeof description === "string" ? description : "",
63+
writable: true,
64+
};
65+
descriptors.parameters = {
66+
configurable: true,
67+
enumerable: true,
68+
value: parameters,
69+
writable: true,
70+
};
71+
descriptors.execute = {
72+
configurable: true,
73+
enumerable: true,
74+
value: (...args: Parameters<AnyAgentTool["execute"]>) =>
75+
Reflect.apply(execute, tool, args) as ReturnType<AnyAgentTool["execute"]>,
76+
writable: true,
77+
};
78+
79+
const snapshot = Object.create(prototype) as AnyAgentTool;
80+
Object.defineProperties(snapshot, descriptors);
81+
copyPluginToolMeta(tool, snapshot);
82+
copyChannelAgentToolMeta(tool as never, snapshot as never);
83+
return snapshot;
84+
}
85+
86+
function copyReadableMcpPluginToolDescriptors(
87+
tool: AnyAgentTool,
88+
): PropertyDescriptorMap | undefined {
89+
let keys: PropertyKey[];
90+
try {
91+
keys = Reflect.ownKeys(tool);
92+
} catch {
93+
return undefined;
94+
}
95+
96+
const descriptors: PropertyDescriptorMap = {};
97+
for (const key of keys) {
98+
if (key === "name" || key === "description" || key === "parameters" || key === "execute") {
99+
continue;
100+
}
101+
let descriptor: PropertyDescriptor | undefined;
102+
try {
103+
descriptor = Reflect.getOwnPropertyDescriptor(tool, key);
104+
} catch {
105+
return undefined;
106+
}
107+
if (!descriptor) {
108+
continue;
109+
}
110+
if ("value" in descriptor) {
111+
descriptors[key] = {
112+
configurable: true,
113+
enumerable: descriptor.enumerable,
114+
value: descriptor.value,
115+
writable: true,
116+
};
117+
continue;
118+
}
119+
try {
120+
descriptors[key] = {
121+
configurable: true,
122+
enumerable: descriptor.enumerable,
123+
value: Reflect.get(tool, key, tool),
124+
writable: true,
125+
};
126+
} catch {
127+
continue;
128+
}
129+
}
130+
return descriptors;
131+
}
132+
133+
function wrapPluginToolForMcp(tool: AnyAgentTool): AnyAgentTool | undefined {
134+
try {
26135
if (isToolWrappedWithBeforeToolCallHook(tool)) {
27136
return rewrapToolWithBeforeToolCallHook(tool, undefined, { approvalMode: "report" });
28137
}
29-
// The ACPX MCP bridge should enforce the same pre-execution hook boundary
30-
// as the agent and HTTP tool execution paths.
31-
return wrapToolWithBeforeToolCallHook(tool, undefined, { approvalMode: "report" });
138+
} catch {
139+
return undefined;
140+
}
141+
const snapshot = snapshotMcpPluginTool(tool);
142+
if (!snapshot) {
143+
return undefined;
144+
}
145+
return wrapToolWithBeforeToolCallHook(snapshot, undefined, { approvalMode: "report" });
146+
}
147+
148+
export function createPluginToolsMcpHandlers(tools: AnyAgentTool[]) {
149+
const wrappedTools = tools.flatMap((tool) => {
150+
const wrapped = wrapPluginToolForMcp(tool);
151+
return wrapped ? [wrapped] : [];
32152
});
33153
const toolMap = new Map<string, AnyAgentTool>();
34154
for (const tool of wrappedTools) {

‎src/plugins/tool-metadata.ts‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
import type { AnyAgentTool } from "../agents/tools/common.js";
2+
3+
/** MCP bridge metadata attached to plugin tools surfaced through agent tool lists. */
4+
export type PluginToolMcpMeta = {
5+
serverName: string;
6+
safeServerName: string;
7+
toolName: string;
8+
operation: "tool" | "resources_list" | "resources_read" | "prompts_list" | "prompts_get";
9+
};
10+
11+
/** Runtime metadata used to trace an agent tool back to its owning plugin registration. */
12+
export type PluginToolMeta = {
13+
pluginId: string;
14+
optional: boolean;
15+
trustedLocalMedia?: boolean;
16+
mcp?: PluginToolMcpMeta;
17+
};
18+
19+
const pluginToolMeta = new WeakMap<AnyAgentTool, PluginToolMeta>();
20+
21+
/** Attaches plugin ownership metadata to a concrete agent tool instance. */
22+
export function setPluginToolMeta(tool: AnyAgentTool, meta: PluginToolMeta): void {
23+
pluginToolMeta.set(tool, meta);
24+
}
25+
26+
/** Reads plugin ownership metadata for a concrete agent tool instance. */
27+
export function getPluginToolMeta(tool: AnyAgentTool): PluginToolMeta | undefined {
28+
return pluginToolMeta.get(tool);
29+
}
30+
31+
/** Copies plugin ownership metadata when wrappers replace a tool object. */
32+
export function copyPluginToolMeta(source: AnyAgentTool, target: AnyAgentTool): void {
33+
const meta = pluginToolMeta.get(source);
34+
if (meta) {
35+
pluginToolMeta.set(target, meta);
36+
}
37+
}

‎src/plugins/tools.ts‎

Lines changed: 9 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -35,28 +35,20 @@ import {
3535
type PluginToolDescriptorConfigCacheKeyMemo,
3636
writeCachedPluginToolDescriptors,
3737
} from "./tool-descriptor-cache.js";
38+
import { copyPluginToolMeta, setPluginToolMeta } from "./tool-metadata.js";
3839
import type { OpenClawPluginToolContext } from "./types.js";
3940

4041
export {
4142
resetPluginToolDescriptorCache,
4243
resetPluginToolDescriptorCache as resetPluginToolFactoryCache,
4344
} from "./tool-descriptor-cache.js";
44-
45-
/** MCP bridge metadata attached to plugin tools surfaced through agent tool lists. */
46-
export type PluginToolMcpMeta = {
47-
serverName: string;
48-
safeServerName: string;
49-
toolName: string;
50-
operation: "tool" | "resources_list" | "resources_read" | "prompts_list" | "prompts_get";
51-
};
52-
53-
/** Runtime metadata used to trace an agent tool back to its owning plugin registration. */
54-
export type PluginToolMeta = {
55-
pluginId: string;
56-
optional: boolean;
57-
trustedLocalMedia?: boolean;
58-
mcp?: PluginToolMcpMeta;
59-
};
45+
export {
46+
copyPluginToolMeta,
47+
getPluginToolMeta,
48+
setPluginToolMeta,
49+
type PluginToolMcpMeta,
50+
type PluginToolMeta,
51+
} from "./tool-metadata.js";
6052

6153
type PluginToolFactoryTimingResult = "array" | "error" | "null" | "single";
6254

@@ -82,27 +74,8 @@ const PLUGIN_TOOL_FACTORY_WARN_TOTAL_MS = 5_000;
8274
const PLUGIN_TOOL_FACTORY_WARN_FACTORY_MS = 1_000;
8375
const PLUGIN_TOOL_FACTORY_SUMMARY_LIMIT = 20;
8476

85-
const pluginToolMeta = new WeakMap<AnyAgentTool, PluginToolMeta>();
8677
const scopedPluginTools = new WeakMap<AnyAgentTool, Map<string, AnyAgentTool>>();
8778

88-
/** Attaches plugin ownership metadata to a concrete agent tool instance. */
89-
export function setPluginToolMeta(tool: AnyAgentTool, meta: PluginToolMeta): void {
90-
pluginToolMeta.set(tool, meta);
91-
}
92-
93-
/** Reads plugin ownership metadata for a concrete agent tool instance. */
94-
export function getPluginToolMeta(tool: AnyAgentTool): PluginToolMeta | undefined {
95-
return pluginToolMeta.get(tool);
96-
}
97-
98-
/** Copies plugin ownership metadata when wrappers replace a tool object. */
99-
export function copyPluginToolMeta(source: AnyAgentTool, target: AnyAgentTool): void {
100-
const meta = pluginToolMeta.get(source);
101-
if (meta) {
102-
pluginToolMeta.set(target, meta);
103-
}
104-
}
105-
10679
function pluginToolScopeKey(entry: PluginToolRegistration): string {
10780
return JSON.stringify([entry.pluginId, entry.source]);
10881
}
@@ -1468,7 +1441,7 @@ export function resolvePluginTools(params: {
14681441
manifestPlugin,
14691442
toolName,
14701443
});
1471-
pluginToolMeta.set(tool, {
1444+
setPluginToolMeta(tool, {
14721445
pluginId: entry.pluginId,
14731446
optional,
14741447
trustedLocalMedia: isTrustedManifestLocalMediaTool({

0 commit comments

Comments
 (0)