Skip to content

Commit b74a7ed

Browse files
vincentkocvincentkoc
committed
fix(agents): ignore unreadable session tool metadata
1 parent 6c76442 commit b74a7ed

9 files changed

Lines changed: 274 additions & 26 deletions

File tree

‎src/agents/sessions/agent-session.ts‎

Lines changed: 35 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,10 @@ import { type BuildSystemPromptOptions, buildSystemPrompt } from "./system-promp
9393
import type { BashOperations } from "./tools/bash-operations.js";
9494
import { createLocalBashOperations } from "./tools/bash.js";
9595
import { createAllToolDefinitions } from "./tools/index.js";
96-
import { createToolDefinitionFromAgentTool } from "./tools/tool-definition-wrapper.js";
96+
import {
97+
createToolDefinitionFromAgentTool,
98+
snapshotReadableToolDefinition,
99+
} from "./tools/tool-definition-wrapper.js";
97100

98101
function unwrapCoreResult<T>(result: { ok: true; value: T } | { ok: false; error: Error }): T {
99102
if (result.ok) {
@@ -283,6 +286,10 @@ interface ToolDefinitionEntry {
283286
sourceInfo: SourceInfo;
284287
}
285288

289+
function createSyntheticToolSourcePath(kind: "builtin" | "sdk", toolName: string): string {
290+
return `<${kind}:${toolName.replace(/[\r\n]/g, " ")}>`;
291+
}
292+
286293
type ActiveToolPromptMetadata = {
287294
validToolNames: string[];
288295
toolSnippets: Record<string, string>;
@@ -2389,13 +2396,26 @@ export class AgentSession {
23892396
const isAllowedTool = (name: string): boolean =>
23902397
!isDisabledBuiltInToolName(name) && (!allowedToolNames || allowedToolNames.has(name));
23912398

2392-
const registeredTools = this.currentExtensionRunner.getAllRegisteredTools();
2399+
const registeredTools = this.currentExtensionRunner.getAllRegisteredTools().flatMap((tool) => {
2400+
const definition = snapshotReadableToolDefinition(tool.definition);
2401+
return definition ? [{ definition, sourceInfo: tool.sourceInfo }] : [];
2402+
});
23932403
const allCustomTools = [
23942404
...registeredTools,
2395-
...this.customTools.map((definition) => ({
2396-
definition,
2397-
sourceInfo: createSyntheticSourceInfo(`<sdk:${definition.name}>`, { source: "sdk" }),
2398-
})),
2405+
...this.customTools.flatMap((toolDefinition) => {
2406+
const definition = snapshotReadableToolDefinition(toolDefinition);
2407+
return definition
2408+
? [
2409+
{
2410+
definition,
2411+
sourceInfo: createSyntheticSourceInfo(
2412+
createSyntheticToolSourcePath("sdk", definition.name),
2413+
{ source: "sdk" },
2414+
),
2415+
},
2416+
]
2417+
: [];
2418+
}),
23992419
].filter((tool) => isAllowedTool(tool.definition.name));
24002420
const definitionRegistry = new Map<string, ToolDefinitionEntry>(
24012421
Array.from(this.baseToolDefinitions.entries())
@@ -2404,7 +2424,9 @@ export class AgentSession {
24042424
name,
24052425
{
24062426
definition,
2407-
sourceInfo: createSyntheticSourceInfo(`<builtin:${name}>`, { source: "builtin" }),
2427+
sourceInfo: createSyntheticSourceInfo(createSyntheticToolSourcePath("builtin", name), {
2428+
source: "builtin",
2429+
}),
24082430
},
24092431
]),
24102432
);
@@ -2438,9 +2460,12 @@ export class AgentSession {
24382460
.filter((definition) => isAllowedTool(definition.name))
24392461
.map((definition) => ({
24402462
definition,
2441-
sourceInfo: createSyntheticSourceInfo(`<builtin:${definition.name}>`, {
2442-
source: "builtin",
2443-
}),
2463+
sourceInfo: createSyntheticSourceInfo(
2464+
createSyntheticToolSourcePath("builtin", definition.name),
2465+
{
2466+
source: "builtin",
2467+
},
2468+
),
24442469
})),
24452470
runner,
24462471
);

‎src/agents/sessions/extensions/loader.test.ts‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,49 @@ afterEach(async () => {
1111
});
1212

1313
describe("loadExtensions", () => {
14+
it("ignores extension tools with unreadable names during registration", async () => {
15+
const dir = await mkdtemp(join(tmpdir(), "openclaw-extension-tool-"));
16+
tempDirs.push(dir);
17+
const extensionPath = join(dir, "extension.ts");
18+
await writeFile(
19+
extensionPath,
20+
`
21+
export default async function(api) {
22+
const badTool = {
23+
label: "Broken Name",
24+
description: "Should be ignored.",
25+
parameters: { type: "object", properties: {} },
26+
async execute() {
27+
return { content: [{ type: "text", text: "bad" }], details: {} };
28+
},
29+
};
30+
Object.defineProperty(badTool, "name", {
31+
get() {
32+
throw new Error("bad\\nname");
33+
},
34+
});
35+
36+
api.registerTool(badTool);
37+
api.registerTool({
38+
name: "extension_lookup",
39+
label: "Extension Lookup",
40+
description: "Looks up a test value.",
41+
parameters: { type: "object", properties: {} },
42+
async execute() {
43+
return { content: [{ type: "text", text: "ok" }], details: {} };
44+
},
45+
});
46+
}
47+
`,
48+
);
49+
50+
const result = await loadExtensions([extensionPath], dir);
51+
52+
expect(result.errors).toEqual([]);
53+
expect(result.extensions).toHaveLength(1);
54+
expect(Array.from(result.extensions[0]?.tools.keys() ?? [])).toEqual(["extension_lookup"]);
55+
});
56+
1457
it("resolves plugin SDK subpaths in jiti-loaded extensions", async () => {
1558
const dir = await mkdtemp(join(tmpdir(), "openclaw-extension-sdk-"));
1659
tempDirs.push(dir);

‎src/agents/sessions/extensions/loader.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import type { ExecOptions } from "../exec.js";
2929
import { execCommand } from "../exec.js";
3030
import * as bundledAgentSessions from "../extension-sdk.js";
3131
import { createSyntheticSourceInfo } from "../source-info.js";
32+
import { readToolDefinitionName } from "../tools/tool-definition-wrapper.js";
3233
import type {
3334
Extension,
3435
ExtensionAPI,
@@ -226,7 +227,11 @@ function createExtensionAPI(
226227

227228
registerTool(tool: ToolDefinition): void {
228229
runtime.assertActive();
229-
extension.tools.set(tool.name, {
230+
const name = readToolDefinitionName(tool);
231+
if (name === undefined) {
232+
return;
233+
}
234+
extension.tools.set(name, {
230235
definition: tool,
231236
sourceInfo: extension.sourceInfo,
232237
});

‎src/agents/sessions/extensions/runner.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import type { KeybindingsConfig } from "../keybindings.js";
1111
import type { ModelRegistry } from "../model-registry.js";
1212
import type { SessionManager } from "../session-manager.js";
1313
import type { BuildSystemPromptOptions } from "../system-prompt.js";
14+
import { readToolDefinitionName } from "../tools/tool-definition-wrapper.js";
1415
import type {
1516
BeforeAgentStartEvent,
1617
BeforeAgentStartEventResult,
@@ -400,8 +401,9 @@ export class ExtensionRunner {
400401
const toolsByName = new Map<string, RegisteredTool>();
401402
for (const ext of this.extensions) {
402403
for (const tool of ext.tools.values()) {
403-
if (!toolsByName.has(tool.definition.name)) {
404-
toolsByName.set(tool.definition.name, tool);
404+
const name = readToolDefinitionName(tool.definition);
405+
if (name !== undefined && !toolsByName.has(name)) {
406+
toolsByName.set(name, tool);
405407
}
406408
}
407409
}

‎src/agents/sessions/extensions/wrapper.ts‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,11 @@
66
*/
77

88
import type { AgentTool } from "../../runtime/index.js";
9-
import { wrapToolDefinition, wrapToolDefinitions } from "../tools/tool-definition-wrapper.js";
9+
import {
10+
snapshotReadableToolDefinition,
11+
wrapToolDefinition,
12+
wrapToolDefinitions,
13+
} from "../tools/tool-definition-wrapper.js";
1014
import type { ExtensionRunner } from "./runner.js";
1115
import type { RegisteredTool } from "./types.js";
1216

@@ -30,7 +34,10 @@ export function wrapRegisteredTools(
3034
runner: ExtensionRunner,
3135
): AgentTool[] {
3236
return wrapToolDefinitions(
33-
registeredTools.map((registeredTool) => registeredTool.definition),
37+
registeredTools.flatMap((registeredTool) => {
38+
const definition = snapshotReadableToolDefinition(registeredTool.definition);
39+
return definition ? [definition] : [];
40+
}),
3441
() => runner.createContext(),
3542
);
3643
}

‎src/agents/sessions/sdk.test.ts‎

Lines changed: 90 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { describe, expect, it } from "vitest";
33
import type { Model } from "../../llm/types.js";
44
import { AuthStorage } from "./auth-storage.js";
55
import { createExtensionRuntime } from "./extensions/loader.js";
6-
import type { LoadExtensionsResult, ToolDefinition } from "./extensions/types.js";
6+
import type { LoadExtensionsResult, RegisteredTool, ToolDefinition } from "./extensions/types.js";
77
import { ModelRegistry } from "./model-registry.js";
88
import type { ResourceLoader } from "./resource-loader.js";
99
import { createAgentSession } from "./sdk.js";
@@ -30,17 +30,18 @@ function createEmptyResourceLoader(): ResourceLoader {
3030

3131
function createResourceLoaderWithHandlers(
3232
handlers: Map<string, Array<(...args: unknown[]) => Promise<unknown>>>,
33+
tools: Map<string, RegisteredTool> = new Map(),
3334
): ResourceLoader {
3435
const extensionsResult: LoadExtensionsResult = {
3536
extensions:
36-
handlers.size > 0
37+
handlers.size > 0 || tools.size > 0
3738
? [
3839
{
3940
path: "<test-extension>",
4041
resolvedPath: "<test-extension>",
4142
sourceInfo: createSyntheticSourceInfo("<test-extension>", { source: "temporary" }),
4243
handlers,
43-
tools: new Map(),
44+
tools,
4445
messageRenderers: new Map(),
4546
commands: new Map(),
4647
flags: new Map(),
@@ -64,6 +65,52 @@ function createResourceLoaderWithHandlers(
6465
};
6566
}
6667

68+
function createTextTool(name: string): ToolDefinition {
69+
return {
70+
name,
71+
label: "Test Tool",
72+
description: "Looks up a test value.",
73+
parameters: Type.Object({}),
74+
execute: async () => ({
75+
content: [{ type: "text", text: "ok" }],
76+
details: {},
77+
}),
78+
};
79+
}
80+
81+
function createUnreadableNameTool(): ToolDefinition {
82+
const tool = {
83+
label: "Broken Name",
84+
description: "Should be ignored.",
85+
parameters: Type.Object({}),
86+
execute: async () => ({
87+
content: [{ type: "text", text: "bad" }],
88+
details: {},
89+
}),
90+
} as unknown as ToolDefinition;
91+
Object.defineProperty(tool, "name", {
92+
get() {
93+
throw new Error("bad\nname");
94+
},
95+
});
96+
return tool;
97+
}
98+
99+
function createUnreadableParametersTool(): ToolDefinition {
100+
return {
101+
name: "broken_params",
102+
label: "Broken Params",
103+
description: "Should be ignored.",
104+
get parameters() {
105+
throw new Error("bad\nparameters");
106+
},
107+
execute: async () => ({
108+
content: [{ type: "text", text: "bad" }],
109+
details: {},
110+
}),
111+
} as unknown as ToolDefinition;
112+
}
113+
67114
describe("createAgentSession tool defaults", () => {
68115
it("forwards max thinking budgets from settings to the agent", async () => {
69116
const { session } = await createAgentSession({
@@ -117,6 +164,46 @@ describe("createAgentSession tool defaults", () => {
117164
expect(session.getActiveToolNames()).toEqual(["custom_lookup"]);
118165
});
119166

167+
it("ignores custom tools with unreadable registry metadata", async () => {
168+
const { session } = await createAgentSession({
169+
model: testModel,
170+
noTools: "builtin",
171+
customTools: [
172+
createUnreadableNameTool(),
173+
createUnreadableParametersTool(),
174+
createTextTool("custom_lookup"),
175+
],
176+
resourceLoader: createEmptyResourceLoader(),
177+
sessionManager: SessionManager.inMemory(),
178+
settingsManager: SettingsManager.inMemory(),
179+
modelRegistry: ModelRegistry.inMemory(AuthStorage.inMemory()),
180+
});
181+
182+
expect(session.getActiveToolNames()).toEqual(["custom_lookup"]);
183+
expect(session.getAllTools().map((tool) => tool.name)).toEqual(["custom_lookup"]);
184+
});
185+
186+
it("ignores extension tools with unreadable registry metadata", async () => {
187+
const sourceInfo = createSyntheticSourceInfo("<test-extension>", { source: "temporary" });
188+
const tools = new Map<string, RegisteredTool>([
189+
["unreadable_name", { definition: createUnreadableNameTool(), sourceInfo }],
190+
["broken_params", { definition: createUnreadableParametersTool(), sourceInfo }],
191+
["extension_lookup", { definition: createTextTool("extension_lookup"), sourceInfo }],
192+
]);
193+
194+
const { session } = await createAgentSession({
195+
model: testModel,
196+
noTools: "builtin",
197+
resourceLoader: createResourceLoaderWithHandlers(new Map(), tools),
198+
sessionManager: SessionManager.inMemory(),
199+
settingsManager: SettingsManager.inMemory(),
200+
modelRegistry: ModelRegistry.inMemory(AuthStorage.inMemory()),
201+
});
202+
203+
expect(session.getActiveToolNames()).toEqual(["extension_lookup"]);
204+
expect(session.getAllTools().map((tool) => tool.name)).toEqual(["extension_lookup"]);
205+
});
206+
120207
it("preserves an exact base system prompt when active tools change", async () => {
121208
const customTool: ToolDefinition = {
122209
name: "custom_lookup",

‎src/agents/sessions/sdk.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ import {
3232
createReadOnlyTools,
3333
createReadTool,
3434
createWriteTool,
35+
readToolDefinitionName,
3536
type ToolName,
3637
withFileMutationQueue,
3738
} from "./tools/index.js";
@@ -283,7 +284,11 @@ export async function createAgentSession(
283284
}
284285

285286
const defaultActiveToolNames: ToolName[] = ["read", "bash", "edit", "write"];
286-
const customToolNames = options.customTools?.map((tool) => tool.name) ?? [];
287+
const customToolNames =
288+
options.customTools?.flatMap((tool) => {
289+
const name = readToolDefinitionName(tool);
290+
return name === undefined ? [] : [name];
291+
}) ?? [];
287292
const allowedToolNames = options.tools ?? (options.noTools === "all" ? [] : undefined);
288293
const disableBuiltInTools = !options.tools && options.noTools === "builtin";
289294
const initialActiveToolNames: string[] = options.tools

‎src/agents/sessions/tools/index.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,13 @@ export {
6969
type WriteOperations,
7070
type WriteToolOptions,
7171
} from "./write.js";
72+
export {
73+
createToolDefinitionFromAgentTool,
74+
readToolDefinitionName,
75+
snapshotReadableToolDefinition,
76+
wrapToolDefinition,
77+
wrapToolDefinitions,
78+
} from "./tool-definition-wrapper.js";
7279

7380
import type { AgentTool } from "../../runtime/index.js";
7481
import type { ToolDefinition } from "../extensions/types.js";

0 commit comments

Comments
 (0)