From 521cb33bf12ff516c5a0bfec68489371fec4d241 Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Mon, 5 Jan 2026 09:18:19 -0700 Subject: [PATCH] fix(orchestrator): allow missing path param for search_files/list_files --- .../tools/__tests__/validateToolUse.spec.ts | 21 +++-- src/core/tools/validateToolUse.ts | 84 +++++++++++++------ src/shared/__tests__/modes.spec.ts | 8 +- 3 files changed, 76 insertions(+), 37 deletions(-) diff --git a/src/core/tools/__tests__/validateToolUse.spec.ts b/src/core/tools/__tests__/validateToolUse.spec.ts index e90c1544e0..6045c32aa3 100644 --- a/src/core/tools/__tests__/validateToolUse.spec.ts +++ b/src/core/tools/__tests__/validateToolUse.spec.ts @@ -257,21 +257,28 @@ describe("mode-validator", () => { }, ] - // Empty string path must not be treated as an unrestricted root operation. - expect(() => validateToolUse("list_files", "skills-reader", customModes, undefined, { path: "" })).toThrow( - /can only read files matching pattern/, - ) + // search_files & list_files: empty/missing path is treated as a missing-param error by the tool + // implementation (no filesystem operation), so permission checks should allow it. + expect(() => + validateToolUse("list_files", "skills-reader", customModes, undefined, { path: "" }), + ).not.toThrow() expect(() => validateToolUse("search_files", "skills-reader", customModes, undefined, { path: "", regex: ".*" }), - ).toThrow(/can only read files matching pattern/) + ).not.toThrow() + expect(() => + validateToolUse("search_files", "skills-reader", customModes, undefined, { regex: ".*" }), + ).not.toThrow() + + // codebase_search: missing/empty path means “search entire workspace”, so it must be rejected. expect(() => validateToolUse("codebase_search", "skills-reader", customModes, undefined, { query: "x", path: "" }), ).toThrow(/can only read files matching pattern/) - - // Omitted path must also be rejected under read fileRegex. expect(() => validateToolUse("codebase_search", "skills-reader", customModes, undefined, { query: "x" }), ).toThrow(/can only read files matching pattern/) + expect(() => + validateToolUse("codebase_search", "skills-reader", customModes, undefined, { query: "x", path: null }), + ).toThrow(/can only read files matching pattern/) // Allowed directory should be accepted. expect(() => diff --git a/src/core/tools/validateToolUse.ts b/src/core/tools/validateToolUse.ts index 9047eadcfe..4e95814cf0 100644 --- a/src/core/tools/validateToolUse.ts +++ b/src/core/tools/validateToolUse.ts @@ -317,44 +317,74 @@ export function isToolAllowedForMode( } } - // Restrict search_files, codebase_search, and list_files path parameter - // These tools operate on directories, so we derive a directory pattern from the fileRegex + // Restrict directory-scoped tools to the directories implied by fileRegex. + // NOTE: Some tools treat a missing/empty path as a *missing param* (no operation), + // while others treat it as an *unrestricted workspace search*. if (["search_files", "codebase_search", "list_files"].includes(tool)) { const rawPathParam = toolParams?.path - // IMPORTANT: When read group fileRegex is configured, we must not allow an empty/omitted - // path to be interpreted as an unrestricted workspace-root operation. - if (typeof rawPathParam !== "string" || rawPathParam.trim().length === 0) { - throw new FileRestrictionError( - mode.name, - options.fileRegex, - options.description, - typeof rawPathParam === "string" ? rawPathParam : "(missing path)", - tool, - "read", - ) - } - - const pathParam = rawPathParam.trim() - if (pathParam.length > 0) { - // Derive directory pattern: strip trailing file pattern (e.g., SKILL.md$) and match directory prefix - // For pattern like: (^|.*[\\/])\.roo[\\/]skills(-[a-zA-Z0-9-]+)?[\\/][^\\/]+[\\/]SKILL\.md$ - // Directory should be within: .roo/skills or .roo/skills- - const dirPattern = options.fileRegex - .replace(/\[\^[^\]]*\]\+[\\\\/]?[^$]*\$$/, "") // Remove [^\/]+\/SKILL\.md$ part - .replace(/\$$/, "") // Remove trailing $ if present - const dirRegex = dirPattern ? `${dirPattern}($|[\\\\/])` : options.fileRegex - - if (!doesFileMatchRegex(pathParam, dirRegex) && !doesFileMatchRegex(pathParam + "/", dirRegex)) { + // codebase_search: path is optional at the tool layer, and missing/null typically + // means “search the entire workspace”, so we must reject missing/empty here. + if (tool === "codebase_search") { + if (typeof rawPathParam !== "string" || rawPathParam.trim().length === 0) { throw new FileRestrictionError( mode.name, options.fileRegex, options.description, - pathParam, + typeof rawPathParam === "string" ? rawPathParam : "(missing path)", tool, "read", ) } + } else { + // search_files & list_files: missing/empty path is treated as a missing param by the tool + // implementation (no filesystem access), so allow it to pass validation. + if (rawPathParam === undefined || rawPathParam === null || rawPathParam === "") { + return true + } + + // Reject non-string types and whitespace-only strings (tools treat these as “present” and + // could attempt filesystem access). + if (typeof rawPathParam !== "string") { + throw new FileRestrictionError( + mode.name, + options.fileRegex, + options.description, + "(invalid path)", + tool, + "read", + ) + } + if (rawPathParam.trim().length === 0) { + throw new FileRestrictionError( + mode.name, + options.fileRegex, + options.description, + rawPathParam, + tool, + "read", + ) + } + } + + const pathParam = (rawPathParam as string).trim() + // Derive directory pattern: strip trailing file pattern (e.g., SKILL.md$) and match directory prefix + // For pattern like: (^|.*[\\/])\.roo[\\/]skills(-[a-zA-Z0-9-]+)?[\\/][^\\/]+[\\/]SKILL\.md$ + // Directory should be within: .roo/skills or .roo/skills- + const dirPattern = options.fileRegex + .replace(/\[\^[^\]]*\]\+[\\\\/]?[^$]*\$$/, "") // Remove [^\/]+\/SKILL\.md$ part + .replace(/\$$/, "") // Remove trailing $ if present + const dirRegex = dirPattern ? `${dirPattern}($|[\\\\/])` : options.fileRegex + + if (!doesFileMatchRegex(pathParam, dirRegex) && !doesFileMatchRegex(pathParam + "/", dirRegex)) { + throw new FileRestrictionError( + mode.name, + options.fileRegex, + options.description, + pathParam, + tool, + "read", + ) } } } diff --git a/src/shared/__tests__/modes.spec.ts b/src/shared/__tests__/modes.spec.ts index 8c87c89358..a98264a966 100644 --- a/src/shared/__tests__/modes.spec.ts +++ b/src/shared/__tests__/modes.spec.ts @@ -561,14 +561,16 @@ describe("FileRestrictionError", () => { }) it("allows empty path for search tools (workspace root)", () => { - // Empty path must not be treated as an unrestricted workspace-root operation when fileRegex is set. - expect(() => + // search_files: empty path is treated as a missing param error by the tool implementation. + // Permission validation should not block the tool from producing a missing-param prompt. + expect( isToolAllowedForMode("search_files", "orchestrator", [], undefined, { path: "", regex: ".*", }), - ).toThrow(FileRestrictionError) + ).toBe(true) + // codebase_search: missing/empty path means “search entire workspace”, so must be rejected. expect(() => isToolAllowedForMode("codebase_search", "orchestrator", [], undefined, { path: "",