Skip to content

Commit fa0d009

Browse files
cdcabreracdcabrera
committed
fix: review update
1 parent 9bc2b16 commit fa0d009

File tree

6 files changed

+186
-29
lines changed

6 files changed

+186
-29
lines changed

‎src/__tests__/__snapshots__/server.tools.test.ts.snap‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,22 @@ exports[`composeTools should attempt to setup creators, inline creators 1`] = `
107107
}
108108
`;
109109

110+
exports[`composeTools should attempt to setup creators, inline creators, missing toolNames 1`] = `
111+
{
112+
"log": [
113+
[
114+
"Tool creator function is missing the static name property, "toolName." Set creator.toolName = "<name>",
115+
or author the tool as a tuple/object (example ['<name>', { description, inputSchema }, handler]).",
116+
],
117+
[
118+
"Tool creator function is missing the static name property, "toolName." Set creator.toolName = "<name>",
119+
or author the tool as a tuple/object (example ['<name>', { description, inputSchema }, handler]).",
120+
],
121+
],
122+
"toolsCount": 3,
123+
}
124+
`;
125+
110126
exports[`composeTools should attempt to setup creators, inline duplicate creators 1`] = `
111127
{
112128
"log": [
@@ -268,6 +284,14 @@ For local files, prefer a file:// URL.",
268284
}
269285
`;
270286
287+
exports[`getBuiltInToolNames should log a warning when a tool name does not exist: warning 1`] = `
288+
[
289+
[
290+
"Built-in tool at index 0 is missing the static name property, "toolName"",
291+
],
292+
]
293+
`;
294+
271295
exports[`getFilePackageToolModules, should return filtered tool modules 1`] = `
272296
[
273297
"@scope/pkg",

‎src/__tests__/__snapshots__/server.toolsUser.test.ts.snap‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -303,7 +303,7 @@ exports[`normalizeTools should normalize configs, a creator 1`] = `
303303
[
304304
{
305305
"index": 0,
306-
"toolName": undefined,
306+
"toolName": "loremIpsum",
307307
"type": "creator",
308308
},
309309
]
@@ -323,12 +323,12 @@ exports[`normalizeTools should normalize configs, array of creators 1`] = `
323323
[
324324
{
325325
"index": 0,
326-
"toolName": undefined,
326+
"toolName": "loremIpsum",
327327
"type": "creator",
328328
},
329329
{
330330
"index": 1,
331-
"toolName": undefined,
331+
"toolName": "dolorSit",
332332
"type": "creator",
333333
},
334334
]
@@ -393,7 +393,7 @@ exports[`normalizeTools should normalize configs, mix of package, object, tuple,
393393
},
394394
{
395395
"index": 3,
396-
"toolName": undefined,
396+
"toolName": "dolorSit",
397397
"type": "creator",
398398
},
399399
]

‎src/__tests__/server.tools.test.ts‎

Lines changed: 47 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { resolve } from 'node:path';
22
import { spawn } from 'child_process';
33
import { log } from '../logger';
44
import {
5-
getBuiltInToolName,
5+
getBuiltInToolNames,
66
computeFsReadAllowlist,
77
logWarningsErrors,
88
getFilePackageToolModules,
@@ -39,14 +39,28 @@ jest.mock('../server.toolsIpc', () => ({
3939
isManifestResult: jest.fn((id: string) => (msg: any) => msg?.t === 'manifest:result' && msg?.id === id)
4040
}));
4141

42-
describe('getBuiltInToolName', () => {
42+
describe('getBuiltInToolNames', () => {
43+
const MockLog = jest.mocked(log);
44+
45+
beforeEach(() => {
46+
jest.clearAllMocks();
47+
});
48+
4349
it('should return built-in tool name', () => {
4450
const toolName = 'loremIpsum';
4551
const creator = () => {};
4652

4753
creator.toolName = toolName;
4854

49-
expect(getBuiltInToolName(creator as any)).toBe(toolName.toLowerCase());
55+
expect(getBuiltInToolNames([creator] as any).has(toolName.toLowerCase())).toBe(true);
56+
});
57+
58+
it('should log a warning when a tool name does not exist', () => {
59+
const creator = () => {};
60+
61+
getBuiltInToolNames([creator] as any);
62+
63+
expect(MockLog.warn.mock.calls).toMatchSnapshot('warning');
5064
});
5165
});
5266

@@ -597,16 +611,37 @@ describe('composeTools', () => {
597611
description: 'inline creators',
598612
nodeVersion: 22,
599613
modules: [
600-
() => ['lorem', { description: 'ipsum', inputSchema: {} }, () => {}],
614+
(() => {
615+
const testing = () => ['lorem', { description: 'lorem ipsum', inputSchema: { type: 'object', properties: {} } }, () => {}];
616+
617+
(testing as any).toolName = 'lorem';
618+
619+
return testing;
620+
})(),
601621
{ name: 'dolor', description: 'sit amet', inputSchema: {}, handler: () => {} }
602622
],
603623
expectedModuleCount: 5
604624
},
625+
{
626+
description: 'inline creators, missing toolNames',
627+
nodeVersion: 22,
628+
modules: [
629+
() => ['lorem', { description: 'lorem ipsum', inputSchema: { type: 'object', properties: {} } }, () => {}],
630+
() => ['dolor', { description: 'sit amet', inputSchema: { type: 'object', properties: {} } }, () => {}]
631+
],
632+
expectedModuleCount: 3
633+
},
605634
{
606635
description: 'inline duplicate creators',
607636
nodeVersion: 22,
608637
modules: [
609-
() => ['lorem', { description: 'ipsum', inputSchema: {} }, () => {}],
638+
(() => {
639+
const testing = () => ['lorem', { description: 'lorem ipsum', inputSchema: { type: 'object', properties: {} } }, () => {}];
640+
641+
(testing as any).toolName = 'lorem';
642+
643+
return testing;
644+
})(),
610645
{ name: 'dolor', description: 'sit amet', inputSchema: {}, handler: () => {} },
611646
{ name: 'dolor', description: 'sit amet', inputSchema: {}, handler: () => {} }
612647
],
@@ -646,7 +681,13 @@ describe('composeTools', () => {
646681
description: 'inline and file package creators',
647682
nodeVersion: 22,
648683
modules: [
649-
() => ['lorem', { description: 'ipsum', inputSchema: {} }, () => {}],
684+
(() => {
685+
const testing = () => ['lorem', { description: 'lorem ipsum', inputSchema: { type: 'object', properties: {} } }, () => {}];
686+
687+
(testing as any).toolName = 'lorem';
688+
689+
return testing;
690+
})(),
650691
{ name: 'dolor', description: 'sit amet', inputSchema: {}, handler: () => {} },
651692
'file:///test/module.js',
652693
'@patternfly/tools'

‎src/__tests__/server.toolsUser.test.ts‎

Lines changed: 73 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -238,21 +238,37 @@ describe('normalizeFunction', () => {
238238
func: () => null
239239
}
240240
])('should normalize the config, $description', ({ func }) => {
241-
const out = normalizeFunction(func);
241+
const updatedFunc = func;
242+
243+
(updatedFunc as any).toolName = 'loremIpsum';
244+
245+
const out = normalizeFunction(updatedFunc);
242246
const updated = (out?.original as any)?.();
243247

244248
if (updated?.[1]?.inputSchema && isZodSchema(updated[1].inputSchema)) {
245249
updated[1].inputSchema = 'isZod = true';
246250
}
247251

252+
expect(out?.type).toBe('creator');
248253
expect(updated).toMatchSnapshot();
249254
});
250255

256+
it('should be an invalid creator if the toolName is missing', () => {
257+
const func = () => ['loremIpsum ', { description: 'lorem ipsum', inputSchema: z.any() }, async () => {}];
258+
259+
const updated = normalizeFunction(func);
260+
261+
expect(updated?.type).toBe('invalid');
262+
expect(updated?.error).toMatch(/missing.*toolname/i);
263+
});
264+
251265
it('should throw a predictable error on unwrap if the function errors', () => {
252266
const func = () => {
253267
throw new Error('Function error');
254268
};
255269

270+
(func as any).toolName = 'loremIpsum';
271+
256272
const updated = normalizeFunction(func);
257273

258274
expect(() => (updated?.value as any)?.()).toThrow('Tool failed to load:');
@@ -468,13 +484,31 @@ describe('normalizeTools', () => {
468484
it.each([
469485
{
470486
description: 'a creator',
471-
config: () => ['loremIpsum', { description: 'lorem ipsum', inputSchema: { type: 'object', properties: {} } }, () => {}]
487+
config: (() => {
488+
const testing = () => ['loremIpsum', { description: 'lorem ipsum', inputSchema: { type: 'object', properties: {} } }, () => {}];
489+
490+
(testing as any).toolName = 'loremIpsum';
491+
492+
return testing;
493+
})()
472494
},
473495
{
474496
description: 'array of creators',
475497
config: [
476-
() => ['loremIpsum', { description: 'lorem ipsum', inputSchema: { type: 'object', properties: {} } }, () => {}],
477-
() => ['dolorSit', { description: 'dolor sit', inputSchema: { type: 'object', properties: {} } }, () => {}]
498+
(() => {
499+
const testing = () => ['loremIpsum', { description: 'lorem ipsum', inputSchema: { type: 'object', properties: {} } }, () => {}];
500+
501+
(testing as any).toolName = 'loremIpsum';
502+
503+
return testing;
504+
})(),
505+
(() => {
506+
const testing = () => ['dolorSit', { description: 'dolor sit', inputSchema: { type: 'object', properties: {} } }, () => {}];
507+
508+
(testing as any).toolName = 'dolorSit';
509+
510+
return testing;
511+
})()
478512
]
479513
},
480514
{
@@ -487,7 +521,13 @@ describe('normalizeTools', () => {
487521
'@scope/pkg',
488522
{ name: 'ametDolor', description: 'amet dolor', inputSchema: { type: 'object', properties: {} }, handler: () => {} },
489523
['loremIpsum', { description: 'lorem ipsum', inputSchema: { type: 'object', properties: {} } }, () => {}],
490-
() => ['dolorSit', { description: 'dolor sit', inputSchema: { type: 'object', properties: {} } }, () => {}]
524+
(() => {
525+
const testing = () => ['dolorSit', { description: 'dolor sit', inputSchema: { type: 'object', properties: {} } }, () => {}];
526+
527+
(testing as any).toolName = 'dolorSit';
528+
529+
return testing;
530+
})()
491531
]
492532
},
493533
{
@@ -570,13 +610,31 @@ describe('createMcpTool', () => {
570610
it.each([
571611
{
572612
description: 'a creator',
573-
config: () => ['loremIpsum', { description: 'lorem ipsum', inputSchema: { type: 'object', properties: {} } }, () => {}]
613+
config: (() => {
614+
const testing = () => ['loremIpsum', { description: 'lorem ipsum', inputSchema: { type: 'object', properties: {} } }, () => {}];
615+
616+
(testing as any).toolName = 'loremIpsum';
617+
618+
return testing;
619+
})()
574620
},
575621
{
576622
description: 'array of creators',
577623
config: [
578-
() => ['loremIpsum', { description: 'lorem ipsum', inputSchema: { type: 'object', properties: {} } }, () => {}],
579-
() => ['dolorSit', { description: 'dolor sit', inputSchema: { type: 'object', properties: {} } }, () => {}]
624+
(() => {
625+
const testing = () => ['loremIpsum', { description: 'lorem ipsum', inputSchema: { type: 'object', properties: {} } }, () => {}];
626+
627+
(testing as any).toolName = 'loremIpsum';
628+
629+
return testing;
630+
})(),
631+
(() => {
632+
const testing = () => ['dolorSit', { description: 'dolor sit', inputSchema: { type: 'object', properties: {} } }, () => {}];
633+
634+
(testing as any).toolName = 'dolorSit';
635+
636+
return testing;
637+
})()
580638
]
581639
},
582640
{
@@ -589,7 +647,13 @@ describe('createMcpTool', () => {
589647
'@scope/pkg',
590648
{ name: 'ametDolor', description: 'amet dolor', inputSchema: { type: 'object', properties: {} }, handler: () => {} },
591649
['loremIpsum', { description: 'lorem ipsum', inputSchema: { type: 'object', properties: {} } }, () => {}],
592-
() => ['dolorSit', { description: 'dolor sit', inputSchema: { type: 'object', properties: {} } }, () => {}]
650+
(() => {
651+
const testing = () => ['dolorSit', { description: 'dolor sit', inputSchema: { type: 'object', properties: {} } }, () => {}];
652+
653+
(testing as any).toolName = 'dolorSit';
654+
655+
return testing;
656+
})()
593657
]
594658
},
595659
{

‎src/server.tools.ts‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,21 @@ type HostHandle = {
4040
const activeHostsBySession = new Map<string, HostHandle>();
4141

4242
/**
43-
* Get the tool name from a creator function.
43+
* Get a set of tool names from the builtin creators.
4444
*
45-
* @param creator - Tool creator function
45+
* @param builtinCreators - Array of builtin tool creators
46+
* @returns Set of tool names
4647
*/
47-
const getBuiltInToolName = (creator: McpToolCreator): string | undefined =>
48-
(creator as McpToolCreator & { toolName?: string })?.toolName?.trim?.()?.toLowerCase?.();
48+
const getBuiltInToolNames = (builtinCreators: McpToolCreator[]) =>
49+
new Set<string>(builtinCreators.map((creator, index) => {
50+
const builtInToolName = (creator as McpToolCreator & { toolName?: string })?.toolName?.trim?.()?.toLowerCase?.();
51+
52+
if (!builtInToolName) {
53+
log.warn(`Built-in tool at index ${index} is missing the static name property, "toolName"`);
54+
}
55+
56+
return builtInToolName;
57+
}).filter(Boolean) as string[]);
4958

5059
/**
5160
* Compute the allowlist for the Tools Host.
@@ -530,7 +539,7 @@ const composeTools = async (
530539
{ sessionId }: AppSession = getSessionOptions()
531540
): Promise<McpToolCreator[]> => {
532541
const toolCreators: McpToolCreator[] = [...builtinCreators];
533-
const usedNames = new Set<string>(builtinCreators.map(creator => getBuiltInToolName(creator)).filter(Boolean) as string[]);
542+
const usedNames = getBuiltInToolNames(builtinCreators);
534543

535544
if (!Array.isArray(toolModules) || toolModules.length === 0) {
536545
log.info('No external tools loaded.');
@@ -643,7 +652,7 @@ export {
643652
composeTools,
644653
computeFsReadAllowlist,
645654
debugChild,
646-
getBuiltInToolName,
655+
getBuiltInToolNames,
647656
getFilePackageTools,
648657
getInlineTools,
649658
getInvalidTools,

0 commit comments

Comments
 (0)