diff --git a/.roo/hooks/example.yaml b/.roo/hooks/example.yaml new file mode 100644 index 0000000000..ad6bb9a17b --- /dev/null +++ b/.roo/hooks/example.yaml @@ -0,0 +1,8 @@ +version: "1" +hooks: + PreToolUse: + - id: edit-verification + matcher: "edit" + enabled: true + command: 'echo "Edit verification triggered for $ROO_TOOL_NAME"' + timeout: 5 diff --git a/src/services/hooks/HookMatcher.ts b/src/services/hooks/HookMatcher.ts index bf10b4f54d..d881130b47 100644 --- a/src/services/hooks/HookMatcher.ts +++ b/src/services/hooks/HookMatcher.ts @@ -2,10 +2,40 @@ * Hook Matcher * * Provides pattern matching for hooks against tool names. - * Supports exact match, regex patterns, glob patterns, and match-all. + * Supports exact match, regex patterns, glob patterns, group names, and match-all. */ import { ResolvedHook } from "./types" +import { getToolsForGroup } from "../../shared/tools" + +/** + * Expand group patterns in a matcher pattern. + * Only expands groups if the pattern is a simple group name or alternation of group names. + * For complex regex patterns, groups are not expanded to avoid breaking existing behavior. + */ +function expandGroupPatterns(pattern: string): string { + // Don't expand groups in patterns that contain | (to preserve existing regex alternation behavior) + if (pattern.includes("|")) { + return pattern + } + + // Don't expand groups in complex patterns that contain regex metacharacters + const regexMetaChars = /[*^$+.()[\]{}\\]/ + if (regexMetaChars.test(pattern)) { + return pattern // Keep complex patterns as-is + } + + // Single group name + const tools = getToolsForGroup(pattern) + if (tools) { + // It's a known group, expand to all tools in the group + return tools.join("|") + } else { + // Not a group, keep as-is, but warn if it looks like it was intended as a group + console.warn(`Unknown tool group "${pattern}". Treating as literal tool name.`) + return pattern + } +} /** * Result of compiling a matcher pattern. @@ -46,17 +76,20 @@ export function compileMatcher(pattern: string | undefined): CompiledMatcher { } } - // Check if pattern looks like a regex (contains regex metacharacters except * and ?) - const regexMetaChars = /[|^$+.()[\]{}\\]/ - const isRegexPattern = regexMetaChars.test(pattern) + // Expand group patterns before processing + const expandedPattern = expandGroupPatterns(pattern) - // Check if pattern looks like a glob (contains * or ?) - const isGlobPattern = /[*?]/.test(pattern) && !isRegexPattern + // Check if expanded pattern looks like a regex (contains regex metacharacters except * and ?) + const regexMetaChars = /[|^$+.()[\]{}\\]/ + const isRegexPattern = regexMetaChars.test(expandedPattern) + + // Check if expanded pattern looks like a glob (contains * or ?) + const isGlobPattern = /[*?]/.test(expandedPattern) && !isRegexPattern if (isRegexPattern) { // Treat as regex pattern try { - const regex = new RegExp(`^(?:${pattern})$`, "i") + const regex = new RegExp(`^(?:${expandedPattern})$`, "i") return { pattern, type: "regex", @@ -69,7 +102,7 @@ export function compileMatcher(pattern: string | undefined): CompiledMatcher { return { pattern, type: "exact", - matches: (toolName: string) => toolName.toLowerCase() === pattern.toLowerCase(), + matches: (toolName: string) => toolName.toLowerCase() === expandedPattern.toLowerCase(), } } } @@ -77,7 +110,7 @@ export function compileMatcher(pattern: string | undefined): CompiledMatcher { if (isGlobPattern) { // Convert glob to regex // * matches any characters, ? matches single character - const regexPattern = pattern + const regexPattern = expandedPattern .replace(/[.+^${}()|[\]\\]/g, "\\$&") // Escape regex special chars except * and ? .replace(/\*/g, ".*") // * -> .* .replace(/\?/g, ".") // ? -> . @@ -96,7 +129,7 @@ export function compileMatcher(pattern: string | undefined): CompiledMatcher { return { pattern, type: "exact", - matches: (toolName: string) => toolName.toLowerCase() === pattern.toLowerCase(), + matches: (toolName: string) => toolName.toLowerCase() === expandedPattern.toLowerCase(), } } } @@ -105,7 +138,7 @@ export function compileMatcher(pattern: string | undefined): CompiledMatcher { return { pattern, type: "exact", - matches: (toolName: string) => toolName.toLowerCase() === pattern.toLowerCase(), + matches: (toolName: string) => toolName.toLowerCase() === expandedPattern.toLowerCase(), } } diff --git a/src/services/hooks/__tests__/HookMatcher.spec.ts b/src/services/hooks/__tests__/HookMatcher.spec.ts index caca0a9a24..e20c8f5b09 100644 --- a/src/services/hooks/__tests__/HookMatcher.spec.ts +++ b/src/services/hooks/__tests__/HookMatcher.spec.ts @@ -9,6 +9,7 @@ * - Cache behavior */ +import { vi } from "vitest" import { compileMatcher, getMatcher, clearMatcherCache, filterMatchingHooks, hookMatchesTool } from "../HookMatcher" import type { ResolvedHook } from "../types" @@ -77,9 +78,9 @@ describe("HookMatcher", () => { }) it("should be case-insensitive", () => { - const matcher = compileMatcher("edit|write") - expect(matcher.matches("Edit")).toBe(true) - expect(matcher.matches("WRITE")).toBe(true) + const matcher = compileMatcher("foo|bar") + expect(matcher.matches("FOO")).toBe(true) + expect(matcher.matches("BAR")).toBe(true) }) it("should fall back to exact match on invalid regex", () => { @@ -124,6 +125,105 @@ describe("HookMatcher", () => { expect(matcher.matches("MCP__TOOL")).toBe(true) }) }) + + describe("group expansion", () => { + it('should expand "edit" group to all edit tools', () => { + const matcher = compileMatcher("edit") + expect(matcher.type).toBe("regex") + expect(matcher.matches("apply_diff")).toBe(true) + expect(matcher.matches("write_to_file")).toBe(true) + expect(matcher.matches("generate_image")).toBe(true) + expect(matcher.matches("search_and_replace")).toBe(true) + expect(matcher.matches("search_replace")).toBe(true) + expect(matcher.matches("edit_file")).toBe(true) + expect(matcher.matches("apply_patch")).toBe(true) + expect(matcher.matches("read_file")).toBe(false) + }) + + it('should expand "read" group to all read tools', () => { + const matcher = compileMatcher("read") + expect(matcher.matches("read_file")).toBe(true) + expect(matcher.matches("fetch_instructions")).toBe(true) + expect(matcher.matches("search_files")).toBe(true) + expect(matcher.matches("list_files")).toBe(true) + expect(matcher.matches("codebase_search")).toBe(true) + expect(matcher.matches("write_to_file")).toBe(false) + }) + + it('should expand "browser" group', () => { + const matcher = compileMatcher("browser") + expect(matcher.matches("browser_action")).toBe(true) + expect(matcher.matches("read_file")).toBe(false) + }) + + it('should expand "command" group', () => { + const matcher = compileMatcher("command") + expect(matcher.matches("execute_command")).toBe(true) + expect(matcher.matches("read_file")).toBe(false) + }) + + it('should expand "mcp" group', () => { + const matcher = compileMatcher("mcp") + expect(matcher.matches("use_mcp_tool")).toBe(true) + expect(matcher.matches("access_mcp_resource")).toBe(true) + expect(matcher.matches("read_file")).toBe(false) + }) + + it('should expand "modes" group', () => { + const matcher = compileMatcher("modes") + expect(matcher.matches("switch_mode")).toBe(true) + expect(matcher.matches("new_task")).toBe(true) + expect(matcher.matches("read_file")).toBe(false) + }) + + it("should be case insensitive for groups", () => { + const matcher = compileMatcher("EDIT") + expect(matcher.matches("apply_diff")).toBe(true) + expect(matcher.matches("write_to_file")).toBe(true) + }) + + it("should still work with individual tool names", () => { + const matcher = compileMatcher("read_file") + expect(matcher.matches("read_file")).toBe(true) + expect(matcher.matches("write_to_file")).toBe(false) + }) + + it("should expand single group name", () => { + const matcher = compileMatcher("edit") + expect(matcher.matches("apply_diff")).toBe(true) + expect(matcher.matches("write_to_file")).toBe(true) + expect(matcher.matches("read_file")).toBe(false) + }) + + it("should treat unknown groups as literal tool names", () => { + // Mock console.warn to capture warnings + const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + + const matcher = compileMatcher("unknown_group") + expect(matcher.matches("unknown_group")).toBe(true) + expect(matcher.matches("read_file")).toBe(false) + + // Should have warned about unknown group + expect(consoleWarnSpy).toHaveBeenCalledWith( + expect.stringContaining('Unknown tool group "unknown_group"'), + ) + + consoleWarnSpy.mockRestore() + }) + + it("should not warn for glob patterns that look like groups", () => { + const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + + const matcher = compileMatcher("edit*") + expect(matcher.type).toBe("glob") + expect(matcher.matches("edit_file")).toBe(true) + + // Should not warn because it contains * + expect(consoleWarnSpy).not.toHaveBeenCalled() + + consoleWarnSpy.mockRestore() + }) + }) }) describe("getMatcher (caching)", () => { diff --git a/src/shared/__tests__/tools.spec.ts b/src/shared/__tests__/tools.spec.ts new file mode 100644 index 0000000000..512317799b --- /dev/null +++ b/src/shared/__tests__/tools.spec.ts @@ -0,0 +1,64 @@ +import { getToolsForGroup, TOOL_GROUPS } from "../tools" + +describe("getToolsForGroup", () => { + test("should return tools for 'read' group", () => { + const result = getToolsForGroup("read") + expect(result).toEqual(["read_file", "fetch_instructions", "search_files", "list_files", "codebase_search"]) + }) + + test("should return tools for 'edit' group (tools and customTools combined)", () => { + const result = getToolsForGroup("edit") + expect(result).toEqual([ + "apply_diff", + "write_to_file", + "generate_image", + "search_and_replace", + "search_replace", + "edit_file", + "apply_patch", + ]) + }) + + test("should return tools for 'browser' group", () => { + const result = getToolsForGroup("browser") + expect(result).toEqual(["browser_action"]) + }) + + test("should return tools for 'command' group", () => { + const result = getToolsForGroup("command") + expect(result).toEqual(["execute_command"]) + }) + + test("should return tools for 'mcp' group", () => { + const result = getToolsForGroup("mcp") + expect(result).toEqual(["use_mcp_tool", "access_mcp_resource"]) + }) + + test("should return tools for 'modes' group", () => { + const result = getToolsForGroup("modes") + expect(result).toEqual(["switch_mode", "new_task"]) + }) + + test("should be case insensitive", () => { + const result = getToolsForGroup("EDIT") + expect(result).toEqual([ + "apply_diff", + "write_to_file", + "generate_image", + "search_and_replace", + "search_replace", + "edit_file", + "apply_patch", + ]) + }) + + test("should return undefined for unknown groups", () => { + const result = getToolsForGroup("unknown") + expect(result).toBeUndefined() + }) + + test("should return undefined for empty string", () => { + const result = getToolsForGroup("") + expect(result).toBeUndefined() + }) +}) diff --git a/src/shared/tools.ts b/src/shared/tools.ts index f893a3d332..a586a5fb59 100644 --- a/src/shared/tools.ts +++ b/src/shared/tools.ts @@ -293,6 +293,19 @@ export const TOOL_GROUPS: Record = { }, } +/** + * Gets all tools for a given group name (case-insensitive). + * Returns all tools in both `tools` and `customTools` arrays combined. + * Returns undefined if the group doesn't exist. + */ +export function getToolsForGroup(groupName: string): string[] | undefined { + const group = TOOL_GROUPS[groupName.toLowerCase() as ToolGroup] + if (!group) { + return undefined + } + return [...(group.tools || []), ...(group.customTools || [])] +} + // Tools that are always available to all modes. export const ALWAYS_AVAILABLE_TOOLS: ToolName[] = [ "ask_followup_question",