From af5a2fa68a4d99f216ad6507633edbb106b3e5dd Mon Sep 17 00:00:00 2001 From: Charles Packer Date: Fri, 31 Oct 2025 14:47:07 -0700 Subject: [PATCH] fix: add type validation for tool parameters (#45) Co-authored-by: Letta --- src/tests/tools/ls.test.ts | 54 +++++++++ src/tests/tools/validation.test.ts | 188 +++++++++++++++++++++++++++++ src/tools/impl/LS.ts | 4 +- src/tools/impl/validation.ts | 74 ++++++++++++ 4 files changed, 319 insertions(+), 1 deletion(-) create mode 100644 src/tests/tools/validation.test.ts diff --git a/src/tests/tools/ls.test.ts b/src/tests/tools/ls.test.ts index 276916c..c192ec5 100644 --- a/src/tests/tools/ls.test.ts +++ b/src/tests/tools/ls.test.ts @@ -51,4 +51,58 @@ describe("LS tool", () => { /missing required parameter.*path/, ); }); + + test("filters files using ignore parameter (array)", async () => { + testDir = new TestDirectory(); + testDir.createFile("file1.txt", ""); + testDir.createFile("file2.log", ""); + testDir.createFile("important.txt", ""); + testDir.createDir("node_modules"); + + const result = await ls({ + path: testDir.path, + ignore: ["*.log", "node_modules"], + }); + + expect(result.content[0].text).toContain("file1.txt"); + expect(result.content[0].text).toContain("important.txt"); + expect(result.content[0].text).not.toContain("file2.log"); + expect(result.content[0].text).not.toContain("node_modules"); + }); + + test("throws error when ignore is a string instead of array", async () => { + testDir = new TestDirectory(); + testDir.createFile("file.txt", ""); + + await expect( + ls({ + path: testDir.path, + ignore: '["*.log", "node_modules"]' as unknown as string[], + }), + ).rejects.toThrow(/must be an array/); + }); + + test("throws error when ignore is a number", async () => { + testDir = new TestDirectory(); + testDir.createFile("file.txt", ""); + + await expect( + ls({ + path: testDir.path, + ignore: 123 as unknown as string[], + }), + ).rejects.toThrow(/must be an array/); + }); + + test("throws error when ignore is an object", async () => { + testDir = new TestDirectory(); + testDir.createFile("file.txt", ""); + + await expect( + ls({ + path: testDir.path, + ignore: { pattern: "*.log" } as unknown as string[], + }), + ).rejects.toThrow(/must be an array/); + }); }); diff --git a/src/tests/tools/validation.test.ts b/src/tests/tools/validation.test.ts new file mode 100644 index 0000000..d121834 --- /dev/null +++ b/src/tests/tools/validation.test.ts @@ -0,0 +1,188 @@ +import { describe, expect, test } from "bun:test"; +import { + validateParamTypes, + validateRequiredParams, +} from "../../tools/impl/validation"; + +describe("validateRequiredParams", () => { + test("passes when all required params are present", () => { + expect(() => { + validateRequiredParams( + { path: "/test", name: "file" }, + ["path", "name"], + "TestTool", + ); + }).not.toThrow(); + }); + + test("throws when required param is missing", () => { + expect(() => { + validateRequiredParams({ name: "file" }, ["path", "name"], "TestTool"); + }).toThrow(/missing required parameter.*path/); + }); + + test("throws with multiple missing params", () => { + expect(() => { + validateRequiredParams({}, ["path", "name"], "TestTool"); + }).toThrow(/missing required parameters.*path.*name/); + }); +}); + +describe("validateParamTypes", () => { + test("passes when string param has correct type", () => { + const schema = { + type: "object", + properties: { + path: { type: "string" }, + }, + }; + + expect(() => { + validateParamTypes({ path: "/test" }, schema, "TestTool"); + }).not.toThrow(); + }); + + test("passes when array param has correct type", () => { + const schema = { + type: "object", + properties: { + ignore: { + type: "array", + items: { type: "string" }, + }, + }, + }; + + expect(() => { + validateParamTypes({ ignore: ["*.log"] }, schema, "TestTool"); + }).not.toThrow(); + }); + + test("throws when array param is given as string", () => { + const schema = { + type: "object", + properties: { + ignore: { + type: "array", + items: { type: "string" }, + }, + }, + }; + + expect(() => { + validateParamTypes({ ignore: '["*.log"]' }, schema, "TestTool"); + }).toThrow(/must be an array.*received string/); + }); + + test("throws when string param is given as number", () => { + const schema = { + type: "object", + properties: { + path: { type: "string" }, + }, + }; + + expect(() => { + validateParamTypes({ path: 123 }, schema, "TestTool"); + }).toThrow(/must be a string.*received integer/); + }); + + test("throws when object param is given as string", () => { + const schema = { + type: "object", + properties: { + config: { type: "object" }, + }, + }; + + expect(() => { + validateParamTypes({ config: '{"key": "value"}' }, schema, "TestTool"); + }).toThrow(/must be an object.*received string/); + }); + + test("throws when boolean param is given as string", () => { + const schema = { + type: "object", + properties: { + enabled: { type: "boolean" }, + }, + }; + + expect(() => { + validateParamTypes({ enabled: "true" }, schema, "TestTool"); + }).toThrow(/must be a boolean.*received string/); + }); + + test("allows optional params to be undefined", () => { + const schema = { + type: "object", + properties: { + path: { type: "string" }, + ignore: { type: "array" }, + }, + required: ["path"], + }; + + expect(() => { + validateParamTypes({ path: "/test" }, schema, "TestTool"); + }).not.toThrow(); + }); + + test("throws with clear error message indicating expected vs received type", () => { + const schema = { + type: "object", + properties: { + ignore: { + type: "array", + items: { type: "string" }, + }, + }, + }; + + expect(() => { + validateParamTypes({ ignore: "not-an-array" }, schema, "TestTool"); + }).toThrow( + /TestTool: Parameter 'ignore' must be an array, received string/, + ); + }); + + test("validates array item types", () => { + const schema = { + type: "object", + properties: { + items: { + type: "array", + items: { type: "string" }, + }, + }, + }; + + expect(() => { + validateParamTypes( + { items: ["valid", 123, "string"] }, + schema, + "TestTool", + ); + }).toThrow(/items\[1\].*must be a string.*received integer/); + }); + + test("passes when array items have correct types", () => { + const schema = { + type: "object", + properties: { + items: { + type: "array", + items: { type: "string" }, + }, + }, + }; + + expect(() => { + validateParamTypes( + { items: ["all", "strings", "here"] }, + schema, + "TestTool", + ); + }).not.toThrow(); + }); +}); diff --git a/src/tools/impl/LS.ts b/src/tools/impl/LS.ts index 325f266..25ddae1 100644 --- a/src/tools/impl/LS.ts +++ b/src/tools/impl/LS.ts @@ -1,7 +1,8 @@ import { readdir, stat } from "node:fs/promises"; import { join, resolve } from "node:path"; import picomatch from "picomatch"; -import { validateRequiredParams } from "./validation.js"; +import LSSchema from "../schemas/LS.json"; +import { validateParamTypes, validateRequiredParams } from "./validation.js"; interface LSArgs { path: string; @@ -17,6 +18,7 @@ export async function ls( args: LSArgs, ): Promise<{ content: Array<{ type: string; text: string }> }> { validateRequiredParams(args, ["path"], "LS"); + validateParamTypes(args, LSSchema, "LS"); const { path: inputPath, ignore = [] } = args; const dirPath = resolve(inputPath); try { diff --git a/src/tools/impl/validation.ts b/src/tools/impl/validation.ts index 6e517ee..fc75969 100644 --- a/src/tools/impl/validation.ts +++ b/src/tools/impl/validation.ts @@ -12,3 +12,77 @@ export function validateRequiredParams( ); } } + +interface JsonSchema { + type?: string; + properties?: Record; + items?: JsonSchema; + required?: string[]; + [key: string]: unknown; +} + +/** + * Validates that parameter values match their expected types from the JSON schema. + * Throws a clear error if types don't match. + */ +export function validateParamTypes( + args: Record, + schema: JsonSchema, + toolName: string, +): void { + if (!schema.properties) return; + + for (const [paramName, paramSchema] of Object.entries(schema.properties)) { + const value = args[paramName]; + + // Skip undefined optional parameters + if (value === undefined) continue; + + const expectedType = paramSchema.type; + if (!expectedType) continue; + + const actualType = getJsonSchemaType(value); + + if (actualType !== expectedType) { + const article = ["array", "object", "integer"].includes(expectedType) + ? "an" + : "a"; + throw new Error( + `${toolName}: Parameter '${paramName}' must be ${article} ${expectedType}, received ${actualType}`, + ); + } + + // Additional validation for arrays to ensure they contain the right element types + if (expectedType === "array" && Array.isArray(value) && paramSchema.items) { + const itemType = paramSchema.items.type; + if (itemType) { + for (let i = 0; i < value.length; i++) { + const itemActualType = getJsonSchemaType(value[i]); + if (itemActualType !== itemType) { + const article = ["array", "object", "integer"].includes(itemType) + ? "an" + : "a"; + throw new Error( + `${toolName}: Parameter '${paramName}[${i}]' must be ${article} ${itemType}, received ${itemActualType}`, + ); + } + } + } + } + } +} + +/** + * Gets the JSON Schema type name for a JavaScript value. + */ +function getJsonSchemaType(value: unknown): string { + if (value === null) return "null"; + if (Array.isArray(value)) return "array"; + if (typeof value === "object") return "object"; + if (typeof value === "boolean") return "boolean"; + if (typeof value === "number") { + return Number.isInteger(value) ? "integer" : "number"; + } + if (typeof value === "string") return "string"; + return "unknown"; +}