From 78ddbd499db10e0bdb033f2f7a6d3162b3b28eaf Mon Sep 17 00:00:00 2001 From: Charles Packer Date: Mon, 27 Oct 2025 16:25:33 -0700 Subject: [PATCH] fix: add parameter validation to all tools to prevent undefined insertions (#14) Co-authored-by: Letta --- .gitignore | 1 + src/tests/tools/bash.test.ts | 6 ++++ src/tests/tools/edit.test.ts | 46 ++++++++++++++++++++++++++ src/tests/tools/grep.test.ts | 6 ++++ src/tests/tools/ls.test.ts | 6 ++++ src/tests/tools/multiedit.test.ts | 55 +++++++++++++++++++++++++++++++ src/tests/tools/read.test.ts | 6 ++++ src/tests/tools/write.test.ts | 19 +++++++++++ src/tools/impl/Bash.ts | 2 ++ src/tools/impl/BashOutput.ts | 2 ++ src/tools/impl/Edit.ts | 6 ++++ src/tools/impl/ExitPlanMode.ts | 3 ++ src/tools/impl/Glob.ts | 2 ++ src/tools/impl/Grep.ts | 2 ++ src/tools/impl/KillBash.ts | 2 ++ src/tools/impl/LS.ts | 2 ++ src/tools/impl/MultiEdit.ts | 7 ++++ src/tools/impl/Read.ts | 2 ++ src/tools/impl/TodoWrite.ts | 3 ++ src/tools/impl/Write.ts | 2 ++ src/tools/impl/validation.ts | 14 ++++++++ 21 files changed, 194 insertions(+) create mode 100644 src/tools/impl/validation.ts diff --git a/.gitignore b/.gitignore index cac67e7..241d278 100644 --- a/.gitignore +++ b/.gitignore @@ -4,6 +4,7 @@ bin/ letta.js letta.js.map .DS_Store +.letta # Logs logs diff --git a/src/tests/tools/bash.test.ts b/src/tests/tools/bash.test.ts index 3893f09..ac7d6f0 100644 --- a/src/tests/tools/bash.test.ts +++ b/src/tests/tools/bash.test.ts @@ -78,4 +78,10 @@ describe("Bash tool", () => { expect(result.content).toBeDefined(); expect(result.content[0].text).toBeDefined(); }); + + test("throws error when command is missing", async () => { + await expect(bash({} as any)).rejects.toThrow( + /missing required parameter.*command/, + ); + }); }); diff --git a/src/tests/tools/edit.test.ts b/src/tests/tools/edit.test.ts index 74f46ab..cf77dac 100644 --- a/src/tests/tools/edit.test.ts +++ b/src/tests/tools/edit.test.ts @@ -65,4 +65,50 @@ describe("Edit tool", () => { expect(readFileSync(file, "utf-8")).toBe("qux bar qux baz qux"); expect(result.replacements).toBe(3); }); + + test("throws error when file_path is missing", async () => { + await expect( + edit({ + old_string: "foo", + new_string: "bar", + } as any), + ).rejects.toThrow(/missing required parameter.*file_path/); + }); + + test("throws error when old_string is missing", async () => { + testDir = new TestDirectory(); + const file = testDir.createFile("test.txt", "Hello, World!"); + + await expect( + edit({ + file_path: file, + new_string: "bar", + } as any), + ).rejects.toThrow(/missing required parameter.*old_string/); + }); + + test("throws error when new_string is missing", async () => { + testDir = new TestDirectory(); + const file = testDir.createFile("test.txt", "Hello, World!"); + + await expect( + edit({ + file_path: file, + old_string: "foo", + } as any), + ).rejects.toThrow(/missing required parameter.*new_string/); + }); + + test("throws error when using typo'd parameter name (new_str instead of new_string)", async () => { + testDir = new TestDirectory(); + const file = testDir.createFile("test.txt", "Hello, World!"); + + await expect( + edit({ + file_path: file, + old_string: "World", + new_str: "Bun", + } as any), + ).rejects.toThrow(/missing required parameter.*new_string/); + }); }); diff --git a/src/tests/tools/grep.test.ts b/src/tests/tools/grep.test.ts index f6047b9..8d20621 100644 --- a/src/tests/tools/grep.test.ts +++ b/src/tests/tools/grep.test.ts @@ -56,4 +56,10 @@ describe("Grep tool", () => { } } }); + + test("throws error when pattern is missing", async () => { + await expect(grep({} as any)).rejects.toThrow( + /missing required parameter.*pattern/, + ); + }); }); diff --git a/src/tests/tools/ls.test.ts b/src/tests/tools/ls.test.ts index 6afb3ea..19fa6da 100644 --- a/src/tests/tools/ls.test.ts +++ b/src/tests/tools/ls.test.ts @@ -45,4 +45,10 @@ describe("LS tool", () => { await expect(ls({ path: file })).rejects.toThrow(/Not a directory/); }); + + test("throws error when path is missing", async () => { + await expect(ls({} as any)).rejects.toThrow( + /missing required parameter.*path/, + ); + }); }); diff --git a/src/tests/tools/multiedit.test.ts b/src/tests/tools/multiedit.test.ts index dd906ec..b927391 100644 --- a/src/tests/tools/multiedit.test.ts +++ b/src/tests/tools/multiedit.test.ts @@ -40,4 +40,59 @@ describe("MultiEdit tool", () => { expect(readFileSync(file, "utf-8")).toBe("xxx yyy"); expect(result.edits_applied).toBe(2); }); + + test("throws error when file_path is missing", async () => { + await expect( + multi_edit({ + edits: [{ old_string: "foo", new_string: "bar" }], + } as any), + ).rejects.toThrow(/missing required parameter.*file_path/); + }); + + test("throws error when edits is missing", async () => { + testDir = new TestDirectory(); + const file = testDir.createFile("test.txt", "foo bar"); + + await expect( + multi_edit({ + file_path: file, + } as any), + ).rejects.toThrow(/missing required parameter.*edits/); + }); + + test("throws error when an edit is missing old_string", async () => { + testDir = new TestDirectory(); + const file = testDir.createFile("test.txt", "foo bar"); + + await expect( + multi_edit({ + file_path: file, + edits: [{ new_string: "bar" } as any], + }), + ).rejects.toThrow(/missing required parameter.*old_string/); + }); + + test("throws error when an edit is missing new_string", async () => { + testDir = new TestDirectory(); + const file = testDir.createFile("test.txt", "foo bar"); + + await expect( + multi_edit({ + file_path: file, + edits: [{ old_string: "foo" } as any], + }), + ).rejects.toThrow(/missing required parameter.*new_string/); + }); + + test("throws error when using typo'd parameter in edit (new_str instead of new_string)", async () => { + testDir = new TestDirectory(); + const file = testDir.createFile("test.txt", "foo bar"); + + await expect( + multi_edit({ + file_path: file, + edits: [{ old_string: "foo", new_str: "baz" } as any], + }), + ).rejects.toThrow(/missing required parameter.*new_string/); + }); }); diff --git a/src/tests/tools/read.test.ts b/src/tests/tools/read.test.ts index 391e65c..5259c0d 100644 --- a/src/tests/tools/read.test.ts +++ b/src/tests/tools/read.test.ts @@ -102,4 +102,10 @@ export default box; expect(result.content).toContain("│ Header │"); expect(result.content).toContain("TypeScript file"); }); + + test("throws error when file_path is missing", async () => { + await expect(read({} as any)).rejects.toThrow( + /missing required parameter.*file_path/, + ); + }); }); diff --git a/src/tests/tools/write.test.ts b/src/tests/tools/write.test.ts index 848f713..9ff45a8 100644 --- a/src/tests/tools/write.test.ts +++ b/src/tests/tools/write.test.ts @@ -48,4 +48,23 @@ describe("Write tool", () => { expect(readFileSync(filePath, "utf-8")).toBe(content); }); + + test("throws error when file_path is missing", async () => { + await expect( + write({ + content: "Hello", + } as any), + ).rejects.toThrow(/missing required parameter.*file_path/); + }); + + test("throws error when content is missing", async () => { + testDir = new TestDirectory(); + const filePath = testDir.resolve("test.txt"); + + await expect( + write({ + file_path: filePath, + } as any), + ).rejects.toThrow(/missing required parameter.*content/); + }); }); diff --git a/src/tools/impl/Bash.ts b/src/tools/impl/Bash.ts index 951976e..3770a30 100644 --- a/src/tools/impl/Bash.ts +++ b/src/tools/impl/Bash.ts @@ -2,6 +2,7 @@ import type { ExecOptions } from "node:child_process"; import { exec, spawn } from "node:child_process"; import { promisify } from "node:util"; import { backgroundProcesses, getNextBashId } from "./process_manager.js"; +import { validateRequiredParams } from "./validation.js"; const execAsync = promisify(exec); @@ -21,6 +22,7 @@ interface BashResult { } export async function bash(args: BashArgs): Promise { + validateRequiredParams(args, ["command"], "Bash"); const { command, timeout = 120000, diff --git a/src/tools/impl/BashOutput.ts b/src/tools/impl/BashOutput.ts index 87756eb..14f411d 100644 --- a/src/tools/impl/BashOutput.ts +++ b/src/tools/impl/BashOutput.ts @@ -1,4 +1,5 @@ import { backgroundProcesses } from "./process_manager.js"; +import { validateRequiredParams } from "./validation.js"; interface BashOutputArgs { bash_id: string; @@ -11,6 +12,7 @@ interface BashOutputResult { export async function bash_output( args: BashOutputArgs, ): Promise { + validateRequiredParams(args, ["bash_id"], "BashOutput"); const { bash_id, filter } = args; const proc = backgroundProcesses.get(bash_id); if (!proc) diff --git a/src/tools/impl/Edit.ts b/src/tools/impl/Edit.ts index d89f363..b2be2f8 100644 --- a/src/tools/impl/Edit.ts +++ b/src/tools/impl/Edit.ts @@ -1,5 +1,6 @@ import { promises as fs } from "node:fs"; import * as path from "node:path"; +import { validateRequiredParams } from "./validation.js"; interface EditArgs { file_path: string; @@ -13,6 +14,11 @@ interface EditResult { } export async function edit(args: EditArgs): Promise { + validateRequiredParams( + args, + ["file_path", "old_string", "new_string"], + "Edit", + ); const { file_path, old_string, new_string, replace_all = false } = args; if (!path.isAbsolute(file_path)) throw new Error(`File path must be absolute, got: ${file_path}`); diff --git a/src/tools/impl/ExitPlanMode.ts b/src/tools/impl/ExitPlanMode.ts index c258844..41412d4 100644 --- a/src/tools/impl/ExitPlanMode.ts +++ b/src/tools/impl/ExitPlanMode.ts @@ -3,6 +3,8 @@ * Exits plan mode by presenting the plan to the user for approval */ +import { validateRequiredParams } from "./validation.js"; + interface ExitPlanModeArgs { plan: string; } @@ -10,6 +12,7 @@ interface ExitPlanModeArgs { export async function exit_plan_mode( args: ExitPlanModeArgs, ): Promise<{ message: string }> { + validateRequiredParams(args, ["plan"], "ExitPlanMode"); const { plan: _plan } = args; // Return confirmation message that plan was approved diff --git a/src/tools/impl/Glob.ts b/src/tools/impl/Glob.ts index f3be3d1..d0384b4 100644 --- a/src/tools/impl/Glob.ts +++ b/src/tools/impl/Glob.ts @@ -1,6 +1,7 @@ import { promises as fs } from "node:fs"; import * as path from "node:path"; import picomatch from "picomatch"; +import { validateRequiredParams } from "./validation.js"; interface GlobArgs { pattern: string; @@ -32,6 +33,7 @@ async function walkDirectory(dir: string): Promise { } export async function glob(args: GlobArgs): Promise { + validateRequiredParams(args, ["pattern"], "Glob"); const { pattern, path: searchPath } = args; const userCwd = process.env.USER_CWD || process.cwd(); let baseDir: string; diff --git a/src/tools/impl/Grep.ts b/src/tools/impl/Grep.ts index 19546a4..a164d28 100644 --- a/src/tools/impl/Grep.ts +++ b/src/tools/impl/Grep.ts @@ -3,6 +3,7 @@ import { createRequire } from "node:module"; import * as path from "node:path"; import { fileURLToPath } from "node:url"; import { promisify } from "node:util"; +import { validateRequiredParams } from "./validation.js"; const execFileAsync = promisify(execFile); @@ -40,6 +41,7 @@ interface GrepResult { } export async function grep(args: GrepArgs): Promise { + validateRequiredParams(args, ["pattern"], "Grep"); const { pattern, path: searchPath, diff --git a/src/tools/impl/KillBash.ts b/src/tools/impl/KillBash.ts index ab7d84a..85536ff 100644 --- a/src/tools/impl/KillBash.ts +++ b/src/tools/impl/KillBash.ts @@ -1,4 +1,5 @@ import { backgroundProcesses } from "./process_manager.js"; +import { validateRequiredParams } from "./validation.js"; interface KillBashArgs { shell_id: string; @@ -8,6 +9,7 @@ interface KillBashResult { } export async function kill_bash(args: KillBashArgs): Promise { + validateRequiredParams(args, ["shell_id"], "KillBash"); const { shell_id } = args; const proc = backgroundProcesses.get(shell_id); if (!proc) return { killed: false }; diff --git a/src/tools/impl/LS.ts b/src/tools/impl/LS.ts index 888c807..325f266 100644 --- a/src/tools/impl/LS.ts +++ b/src/tools/impl/LS.ts @@ -1,6 +1,7 @@ import { readdir, stat } from "node:fs/promises"; import { join, resolve } from "node:path"; import picomatch from "picomatch"; +import { validateRequiredParams } from "./validation.js"; interface LSArgs { path: string; @@ -15,6 +16,7 @@ interface FileInfo { export async function ls( args: LSArgs, ): Promise<{ content: Array<{ type: string; text: string }> }> { + validateRequiredParams(args, ["path"], "LS"); const { path: inputPath, ignore = [] } = args; const dirPath = resolve(inputPath); try { diff --git a/src/tools/impl/MultiEdit.ts b/src/tools/impl/MultiEdit.ts index 67be3b4..27a6eb6 100644 --- a/src/tools/impl/MultiEdit.ts +++ b/src/tools/impl/MultiEdit.ts @@ -1,5 +1,6 @@ import { promises as fs } from "node:fs"; import * as path from "node:path"; +import { validateRequiredParams } from "./validation.js"; interface Edit { old_string: string; @@ -18,11 +19,17 @@ interface MultiEditResult { export async function multi_edit( args: MultiEditArgs, ): Promise { + validateRequiredParams(args, ["file_path", "edits"], "MultiEdit"); const { file_path, edits } = args; if (!path.isAbsolute(file_path)) throw new Error(`File path must be absolute, got: ${file_path}`); if (!edits || edits.length === 0) throw new Error("No edits provided"); for (let i = 0; i < edits.length; i++) { + validateRequiredParams( + edits[i] as Record, + ["old_string", "new_string"], + `MultiEdit (edit ${i + 1})`, + ); if (edits[i].old_string === edits[i].new_string) throw new Error( `Edit ${i + 1}: No changes to make: old_string and new_string are exactly the same.`, diff --git a/src/tools/impl/Read.ts b/src/tools/impl/Read.ts index f9817a3..3dcb0ba 100644 --- a/src/tools/impl/Read.ts +++ b/src/tools/impl/Read.ts @@ -1,5 +1,6 @@ import { promises as fs } from "node:fs"; import * as path from "node:path"; +import { validateRequiredParams } from "./validation.js"; interface ReadArgs { file_path: string; @@ -79,6 +80,7 @@ function formatWithLineNumbers( } export async function read(args: ReadArgs): Promise { + validateRequiredParams(args, ["file_path"], "Read"); const { file_path, offset, limit } = args; if (!path.isAbsolute(file_path)) throw new Error(`File path must be absolute, got: ${file_path}`); diff --git a/src/tools/impl/TodoWrite.ts b/src/tools/impl/TodoWrite.ts index e148d02..ca0c9fc 100644 --- a/src/tools/impl/TodoWrite.ts +++ b/src/tools/impl/TodoWrite.ts @@ -1,3 +1,5 @@ +import { validateRequiredParams } from "./validation.js"; + interface TodoItem { content: string; status: "pending" | "in_progress" | "completed"; @@ -14,6 +16,7 @@ interface TodoWriteResult { export async function todo_write( args: TodoWriteArgs, ): Promise { + validateRequiredParams(args, ["todos"], "TodoWrite"); if (!args.todos || !Array.isArray(args.todos)) throw new Error("todos must be an array"); for (const todo of args.todos) { diff --git a/src/tools/impl/Write.ts b/src/tools/impl/Write.ts index 92dabe9..b978f5f 100644 --- a/src/tools/impl/Write.ts +++ b/src/tools/impl/Write.ts @@ -1,5 +1,6 @@ import { promises as fs } from "node:fs"; import * as path from "node:path"; +import { validateRequiredParams } from "./validation.js"; interface WriteArgs { file_path: string; @@ -10,6 +11,7 @@ interface WriteResult { } export async function write(args: WriteArgs): Promise { + validateRequiredParams(args, ["file_path", "content"], "Write"); const { file_path, content } = args; if (!path.isAbsolute(file_path)) throw new Error(`File path must be absolute, got: ${file_path}`); diff --git a/src/tools/impl/validation.ts b/src/tools/impl/validation.ts new file mode 100644 index 0000000..7c7ae67 --- /dev/null +++ b/src/tools/impl/validation.ts @@ -0,0 +1,14 @@ +export function validateRequiredParams( + args: Record, + required: string[], + toolName: string, +): void { + const missing = required.filter((key) => args[key] === undefined); + if (missing.length > 0) { + const received = Object.keys(args).join(", "); + throw new Error( + `${toolName} tool missing required parameter${missing.length > 1 ? "s" : ""}: ${missing.join(", ")}. ` + + `Received parameters: ${received}`, + ); + } +}