Skip to content

Commit c94bfda

Browse files
grimmerkgrimmerk
committed
fix: robust tool() parameter parsing for undefined values
Rewrites parsing to find callback first, then parse args sequentially. Handles edge cases where optional params may be undefined. Fixes: tool(name, undefined, schema, undefined, callback)
1 parent 856d9ec commit c94bfda

File tree

2 files changed

+232
-23
lines changed

2 files changed

+232
-23
lines changed

‎src/server/mcp.test.ts‎

Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -908,6 +908,193 @@ describe("tool()", () => {
908908
expect(result.tools[1].annotations).toEqual(result.tools[0].annotations);
909909
});
910910

911+
/***
912+
* Test: Internal Robustness - Tool Registration Parameter Parsing
913+
* Tests that internal parsing logic correctly handles edge cases even when
914+
* TypeScript types are bypassed (e.g., via loose tsconfig or type assertions)
915+
*/
916+
test("should correctly parse callback position when undefined is passed in annotations position", async () => {
917+
const mcpServer = new McpServer({
918+
name: "test server",
919+
version: "1.0",
920+
});
921+
const client = new Client({
922+
name: "test client",
923+
version: "1.0",
924+
});
925+
926+
// Testing internal robustness: even if someone bypasses TypeScript and passes undefined
927+
// the callback should still be found at the correct position
928+
(mcpServer.tool as any)(
929+
"test",
930+
"A tool description",
931+
{ name: z.string() },
932+
undefined, // Not officially supported, but internal logic should handle it
933+
async ({ name }: { name: string }) => ({
934+
content: [{ type: "text", text: `Hello, ${name}!` }]
935+
})
936+
);
937+
938+
const [clientTransport, serverTransport] =
939+
InMemoryTransport.createLinkedPair();
940+
941+
await Promise.all([
942+
client.connect(clientTransport),
943+
mcpServer.server.connect(serverTransport),
944+
]);
945+
946+
const result = await client.request(
947+
{ method: "tools/list" },
948+
ListToolsResultSchema,
949+
);
950+
951+
expect(result.tools).toHaveLength(1);
952+
expect(result.tools[0].name).toBe("test");
953+
expect(result.tools[0].description).toBe("A tool description");
954+
expect(result.tools[0].inputSchema).toMatchObject({
955+
type: "object",
956+
properties: { name: { type: "string" } }
957+
});
958+
expect(result.tools[0].annotations).toBeUndefined();
959+
960+
// Verify that the callback was correctly identified and works
961+
const callResult = await client.request(
962+
{
963+
method: "tools/call",
964+
params: {
965+
name: "test",
966+
arguments: { name: "World" }
967+
}
968+
},
969+
CallToolResultSchema,
970+
);
971+
972+
expect(callResult.content).toHaveLength(1);
973+
expect((callResult.content[0] as TextContent).text).toBe("Hello, World!");
974+
});
975+
976+
/***
977+
* Test: Internal Robustness - Description Undefined
978+
*/
979+
test("should correctly parse when description is undefined", async () => {
980+
const mcpServer = new McpServer({
981+
name: "test server",
982+
version: "1.0",
983+
});
984+
const client = new Client({
985+
name: "test client",
986+
version: "1.0",
987+
});
988+
989+
// Testing: tool(name, undefined, schema, callback)
990+
(mcpServer.tool as any)(
991+
"test",
992+
undefined, // description is undefined
993+
{ name: z.string() },
994+
async ({ name }: { name: string }) => ({
995+
content: [{ type: "text", text: `Hello, ${name}!` }]
996+
})
997+
);
998+
999+
const [clientTransport, serverTransport] =
1000+
InMemoryTransport.createLinkedPair();
1001+
1002+
await Promise.all([
1003+
client.connect(clientTransport),
1004+
mcpServer.server.connect(serverTransport),
1005+
]);
1006+
1007+
const result = await client.request(
1008+
{ method: "tools/list" },
1009+
ListToolsResultSchema,
1010+
);
1011+
1012+
expect(result.tools).toHaveLength(1);
1013+
expect(result.tools[0].name).toBe("test");
1014+
expect(result.tools[0].description).toBeUndefined();
1015+
expect(result.tools[0].inputSchema).toMatchObject({
1016+
type: "object",
1017+
properties: { name: { type: "string" } }
1018+
});
1019+
1020+
// Verify callback works
1021+
const callResult = await client.request(
1022+
{
1023+
method: "tools/call",
1024+
params: {
1025+
name: "test",
1026+
arguments: { name: "World" }
1027+
}
1028+
},
1029+
CallToolResultSchema,
1030+
);
1031+
1032+
expect(callResult.content).toHaveLength(1);
1033+
expect((callResult.content[0] as TextContent).text).toBe("Hello, World!");
1034+
});
1035+
1036+
/***
1037+
* Test: Internal Robustness - ParamsSchema Undefined
1038+
*/
1039+
test("should correctly parse when paramsSchema is undefined", async () => {
1040+
const mcpServer = new McpServer({
1041+
name: "test server",
1042+
version: "1.0",
1043+
});
1044+
const client = new Client({
1045+
name: "test client",
1046+
version: "1.0",
1047+
});
1048+
1049+
// Testing: tool(name, description, undefined, annotations, callback)
1050+
(mcpServer.tool as any)(
1051+
"test",
1052+
"A tool description",
1053+
undefined, // paramsSchema is undefined
1054+
{ title: "Test Tool" },
1055+
async () => ({
1056+
content: [{ type: "text", text: "No params" }]
1057+
})
1058+
);
1059+
1060+
const [clientTransport, serverTransport] =
1061+
InMemoryTransport.createLinkedPair();
1062+
1063+
await Promise.all([
1064+
client.connect(clientTransport),
1065+
mcpServer.server.connect(serverTransport),
1066+
]);
1067+
1068+
const result = await client.request(
1069+
{ method: "tools/list" },
1070+
ListToolsResultSchema,
1071+
);
1072+
1073+
expect(result.tools).toHaveLength(1);
1074+
expect(result.tools[0].name).toBe("test");
1075+
expect(result.tools[0].description).toBe("A tool description");
1076+
expect(result.tools[0].annotations?.title).toBe("Test Tool");
1077+
expect(result.tools[0].inputSchema).toMatchObject({
1078+
type: "object",
1079+
properties: {}
1080+
});
1081+
1082+
// Verify callback works
1083+
const callResult = await client.request(
1084+
{
1085+
method: "tools/call",
1086+
params: {
1087+
name: "test",
1088+
arguments: {}
1089+
}
1090+
},
1091+
CallToolResultSchema,
1092+
);
1093+
1094+
expect(callResult.content).toHaveLength(1);
1095+
expect((callResult.content[0] as TextContent).text).toBe("No params");
1096+
});
1097+
9111098
/***
9121099
* Test: Tool Argument Validation
9131100
*/

‎src/server/mcp.ts‎

Lines changed: 45 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -891,33 +891,55 @@ export class McpServer {
891891
// Support for this style is frozen as of protocol version 2025-03-26. Future additions
892892
// to tool definition should *NOT* be added.
893893

894-
if (typeof rest[0] === "string") {
895-
description = rest.shift() as string;
894+
// Find the callback first (always the last function in rest)
895+
let callbackIndex = rest.length - 1;
896+
while (callbackIndex >= 0 && typeof rest[callbackIndex] !== "function") {
897+
callbackIndex--;
896898
}
897899

898-
// Handle the different overload combinations
899-
if (rest.length > 1) {
900-
// We have at least one more arg before the callback
901-
const firstArg = rest[0];
902-
903-
if (isZodRawShape(firstArg)) {
904-
// We have a params schema as the first arg
905-
inputSchema = rest.shift() as ZodRawShape;
906-
907-
// Check if the next arg is potentially annotations
908-
if (rest.length > 1 && typeof rest[0] === "object" && rest[0] !== null && !(isZodRawShape(rest[0]))) {
909-
// Case: tool(name, paramsSchema, annotations, cb)
910-
// Or: tool(name, description, paramsSchema, annotations, cb)
911-
annotations = rest.shift() as ToolAnnotations;
912-
}
913-
} else if (typeof firstArg === "object" && firstArg !== null) {
914-
// Not a ZodRawShape, so must be annotations in this position
915-
// Case: tool(name, annotations, cb)
916-
// Or: tool(name, description, annotations, cb)
917-
annotations = rest.shift() as ToolAnnotations;
900+
if (callbackIndex < 0) {
901+
throw new Error(`Tool ${name} registration requires a callback function`);
902+
}
903+
904+
const callback = rest[callbackIndex] as ToolCallback<ZodRawShape | undefined>;
905+
// Get all args before the callback
906+
const args = rest.slice(0, callbackIndex);
907+
908+
// Parse args: [description?], [paramsSchema?], [annotations?]
909+
// Skip undefined values and parse based on type
910+
let argIndex = 0;
911+
912+
// Check for optional description (first string arg, skip if undefined)
913+
if (argIndex < args.length) {
914+
if (typeof args[argIndex] === "string") {
915+
description = args[argIndex] as string;
916+
argIndex++;
917+
} else if (args[argIndex] === undefined) {
918+
argIndex++; // Skip undefined
919+
}
920+
}
921+
922+
// Check for optional paramsSchema (ZodRawShape, skip if undefined)
923+
if (argIndex < args.length) {
924+
if (isZodRawShape(args[argIndex])) {
925+
inputSchema = args[argIndex] as ZodRawShape;
926+
argIndex++;
927+
} else if (args[argIndex] === undefined) {
928+
argIndex++; // Skip undefined
929+
}
930+
}
931+
932+
// Check for optional annotations (non-null object that's not a ZodRawShape, skip if undefined)
933+
if (argIndex < args.length) {
934+
if (typeof args[argIndex] === "object" &&
935+
args[argIndex] !== null &&
936+
!isZodRawShape(args[argIndex])) {
937+
annotations = args[argIndex] as ToolAnnotations;
938+
argIndex++;
939+
} else if (args[argIndex] === undefined) {
940+
argIndex++; // Skip undefined
918941
}
919942
}
920-
const callback = rest[0] as ToolCallback<ZodRawShape | undefined>;
921943

922944
return this._createRegisteredTool(name, undefined, description, inputSchema, outputSchema, annotations, undefined, callback)
923945
}

0 commit comments

Comments
 (0)