diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index ac3dc3cf51..da524b34f7 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -237,6 +237,11 @@ export interface HookInfo { enabled: boolean /** Source of this hook configuration */ source: "project" | "mode" | "global" + /** + * File creation timestamp (ms since epoch) for the config file this hook came from. + * Used for stable UI sorting. + */ + createdAt?: number /** Timeout in seconds */ timeout: number /** Override shell if specified */ diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 749112113b..4208e58b4e 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2263,6 +2263,7 @@ export class ClineProvider commandPreview: hook.command, enabled: (hook.enabled ?? true) && !(snapshot?.disabledHookIds?.has(hook.id) ?? false), source: hook.source, + createdAt: hook.createdAt, timeout: hook.timeout ?? 60, shell: hook.shell, description: hook.description, diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 6fb15c4e56..8672347a58 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -3727,7 +3727,7 @@ export const webviewMessageHandler = async ( enabled: true command: |- echo "Verification hook triggered" - timeout: 5 + timeout: 15 ` await safeWriteText(exampleFilePath, exampleContent) diff --git a/src/services/hooks/HookConfigLoader.ts b/src/services/hooks/HookConfigLoader.ts index 1199981ba6..1d5093d727 100644 --- a/src/services/hooks/HookConfigLoader.ts +++ b/src/services/hooks/HookConfigLoader.ts @@ -11,7 +11,7 @@ */ import * as path from "path" -import fs from "fs/promises" +import * as fs from "fs/promises" import YAML from "yaml" import { z } from "zod" import { @@ -31,6 +31,7 @@ import { getGlobalRooDirectory, getProjectRooDirectoryForCwd } from "../roo-conf interface LoadedConfigFile { filePath: string source: HookSource + createdAt?: number hooks: Map errors: string[] } @@ -146,11 +147,21 @@ async function loadConfigFile(filePath: string, source: HookSource): Promise 0 + ? stat.birthtimeMs + : stat.ctimeMs && stat.ctimeMs > 0 + ? stat.ctimeMs + : stat.mtimeMs + const content = await fs.readFile(filePath, "utf-8") const parsed = parseConfigContent(content, filePath) const validated = validateConfig(parsed, filePath) @@ -226,6 +237,59 @@ function mergeConfigs(loadedConfigs: LoadedConfigFile[]): { hooksById: Map hasProjectHooks: boolean } { + function isSameHookDefinition(a: ResolvedHook, b: ResolvedHook): boolean { + return ( + a.id === b.id && + a.command === b.command && + a.matcher === b.matcher && + a.enabled === b.enabled && + a.timeout === b.timeout && + a.description === b.description && + a.shell === b.shell && + a.includeConversationHistory === b.includeConversationHistory + ) + } + + function removeHookIdFromAllEvents(hookId: string): void { + for (const [evt, list] of hooksByEvent) { + const next = list.filter((h) => h.id !== hookId) + if (next.length === 0) { + hooksByEvent.delete(evt) + } else if (next.length !== list.length) { + hooksByEvent.set(evt, next) + } + } + } + + function upsertHookInEventList(event: HookEventType, hook: ResolvedHook): void { + if (!hooksByEvent.has(event)) { + hooksByEvent.set(event, []) + } + const list = hooksByEvent.get(event)! + const idx = list.findIndex((h) => h.id === hook.id) + if (idx === -1) { + list.push(hook) + } else { + list[idx] = hook + } + } + + function syncHookInstances(hookId: string, canonical: ResolvedHook): void { + // Keep any existing per-event instances aligned with the canonical definition. + for (const [evt, list] of hooksByEvent) { + for (let i = 0; i < list.length; i++) { + const h = list[i] + if (h.id !== hookId) continue + list[i] = { + ...canonical, + event: evt, + // Ensure the event list is consistent across all instances + events: canonical.events, + } + } + } + } + // Track hooks by ID to detect overrides const hooksById = new Map() @@ -256,31 +320,44 @@ function mergeConfigs(loadedConfigs: LoadedConfigFile[]): { const resolved: ResolvedHook = { ...def, source: config.source, + createdAt: config.createdAt, event, + events: [event], filePath: config.filePath, } // Check for existing hook with same ID const existing = hooksById.get(def.id) if (existing) { - // Remove from its event list - const eventList = hooksByEvent.get(existing.event) - if (eventList) { - const idx = eventList.findIndex((h) => h.id === def.id) - if (idx !== -1) { - eventList.splice(idx, 1) - } + // If this is effectively the same hook definition, merge events instead of overwriting. + if (isSameHookDefinition(existing, resolved)) { + const mergedEvents = new Set([...(existing.events ?? [existing.event]), event]) + existing.events = Array.from(mergedEvents) + + // Prefer metadata from the later (higher precedence / later file) definition. + existing.source = resolved.source + existing.createdAt = resolved.createdAt + existing.filePath = resolved.filePath + + // Ensure hooksByEvent has an entry for this event. + upsertHookInEventList(event, { + ...existing, + event, + events: existing.events, + }) + + // Keep all existing per-event hook instances up to date. + syncHookInstances(def.id, existing) + continue } + + // Different hook definition: higher precedence replaces the entire hook (and its events). + removeHookIdFromAllEvents(def.id) } // Add/replace in lookup maps hooksById.set(def.id, resolved) - - // Add to event list - if (!hooksByEvent.has(event)) { - hooksByEvent.set(event, []) - } - hooksByEvent.get(event)!.push(resolved) + upsertHookInEventList(event, resolved) } } } diff --git a/src/services/hooks/HookMatcher.ts b/src/services/hooks/HookMatcher.ts index d881130b47..92fa4cde55 100644 --- a/src/services/hooks/HookMatcher.ts +++ b/src/services/hooks/HookMatcher.ts @@ -14,27 +14,38 @@ import { getToolsForGroup } from "../../shared/tools" * 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 + // Don't expand groups in complex patterns that contain regex metacharacters. + // NOTE: we intentionally exclude the pipe character here so that we can expand + // simple group alternations like "read|edit" without breaking regex support. const regexMetaChars = /[*^$+.()[\]{}\\]/ if (regexMetaChars.test(pattern)) { return pattern // Keep complex patterns as-is } + // Expand group alternations, but only when the pattern is an alternation of *group names*. + // This preserves existing regex alternation behavior for non-group patterns like "Edit|Write". + if (pattern.includes("|")) { + const parts = pattern.split("|") + const expandedParts = parts.map((part) => getToolsForGroup(part)) + + // Only expand if every alternation part is a known group. + if (expandedParts.every(Boolean)) { + return expandedParts.map((tools) => tools!.join("|")).join("|") + } + + return pattern + } + // 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 } + + // 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 } /** diff --git a/src/services/hooks/__tests__/HookConfigLoader.spec.ts b/src/services/hooks/__tests__/HookConfigLoader.spec.ts index 7cd65cec38..5246db7d71 100644 --- a/src/services/hooks/__tests__/HookConfigLoader.spec.ts +++ b/src/services/hooks/__tests__/HookConfigLoader.spec.ts @@ -15,6 +15,7 @@ import type { HooksConfigSnapshot, HookEventType } from "../types" const mockFsPromises = vi.hoisted(() => ({ readdir: vi.fn(), readFile: vi.fn(), + stat: vi.fn(), access: vi.fn(), })) @@ -22,6 +23,7 @@ vi.mock("fs/promises", () => ({ default: mockFsPromises, readdir: mockFsPromises.readdir, readFile: mockFsPromises.readFile, + stat: mockFsPromises.stat, access: mockFsPromises.access, })) @@ -33,6 +35,12 @@ vi.mock("../../roo-config", () => ({ describe("HookConfigLoader", () => { beforeEach(() => { vi.clearAllMocks() + // Default stat() implementation so loader can attach createdAt. + mockFsPromises.stat.mockResolvedValue({ + birthtimeMs: 1, + ctimeMs: 1, + mtimeMs: 1, + } as any) }) describe("loadHooksConfig", () => { @@ -79,6 +87,7 @@ hooks: expect(hooks[0].id).toBe("lint-check") expect(hooks[0].command).toBe("./lint.sh") expect(hooks[0].timeout).toBe(30) + expect(hooks[0].createdAt).toBe(1) }) it("should parse JSON config files", async () => { @@ -227,6 +236,36 @@ hooks: expect(result.snapshot.hasProjectHooks).toBe(true) }) + + it("should preserve multiple events for the same hook ID", async () => { + const yamlContent = ` +hooks: + - id: multi-event + events: ["PreToolUse", "PostToolUse"] + command: "./hook.sh" +` + mockFsPromises.readdir.mockImplementation(async (dirPath) => { + const dir = dirPath.toString() + if (dir.endsWith("/.roo/hooks")) { + return [{ name: "hooks.yaml", isFile: () => true, isDirectory: () => false }] + } + throw { code: "ENOENT" } + }) + mockFsPromises.readFile.mockResolvedValue(yamlContent) + + const result = await loadHooksConfig({ cwd: "/project" }) + + expect(result.errors).toHaveLength(0) + + const preHooks = result.snapshot.hooksByEvent.get("PreToolUse") || [] + const postHooks = result.snapshot.hooksByEvent.get("PostToolUse") || [] + expect(preHooks.map((h) => h.id)).toContain("multi-event") + expect(postHooks.map((h) => h.id)).toContain("multi-event") + + const hookById = getHookById(result.snapshot, "multi-event") + expect(hookById).toBeDefined() + expect(hookById!.events?.sort()).toEqual(["PostToolUse", "PreToolUse"].sort()) + }) }) describe("getHooksForEvent", () => { diff --git a/src/services/hooks/__tests__/HookMatcher.spec.ts b/src/services/hooks/__tests__/HookMatcher.spec.ts index e20c8f5b09..302911a26b 100644 --- a/src/services/hooks/__tests__/HookMatcher.spec.ts +++ b/src/services/hooks/__tests__/HookMatcher.spec.ts @@ -150,6 +150,15 @@ describe("HookMatcher", () => { expect(matcher.matches("write_to_file")).toBe(false) }) + it('should expand group alternation like "read|edit"', () => { + const matcher = compileMatcher("read|edit") + expect(matcher.type).toBe("regex") + expect(matcher.matches("read_file")).toBe(true) + expect(matcher.matches("write_to_file")).toBe(true) + // sanity check: ensure we didn't accidentally match an unrelated tool + expect(matcher.matches("execute_command")).toBe(false) + }) + it('should expand "browser" group', () => { const matcher = compileMatcher("browser") expect(matcher.matches("browser_action")).toBe(true) diff --git a/src/services/hooks/types.ts b/src/services/hooks/types.ts index 461896dc37..f58c4df4cf 100644 --- a/src/services/hooks/types.ts +++ b/src/services/hooks/types.ts @@ -162,9 +162,21 @@ export interface ResolvedHook extends HookDefinition { /** Which config source this hook came from */ source: HookSource + /** + * File creation timestamp (ms since epoch) for the config file this hook came from. + * Used for stable UI ordering. + */ + createdAt?: number + /** The event type this hook is registered for */ + // NOTE: A hook ID can be registered for multiple events. The loader stores the full set + // of events on `events`. `event` is retained for backwards-compatibility and will be set + // to the first event encountered for the hook. event: HookEventType + /** All event types this hook ID is registered for */ + events?: HookEventType[] + /** File path where this hook was defined */ filePath: string } diff --git a/webview-ui/src/components/settings/HooksSettings.tsx b/webview-ui/src/components/settings/HooksSettings.tsx index b58f0867f7..be43ae5116 100644 --- a/webview-ui/src/components/settings/HooksSettings.tsx +++ b/webview-ui/src/components/settings/HooksSettings.tsx @@ -124,7 +124,20 @@ export const HooksSettings: React.FC = () => { vscode.postMessage({ type: "hooksCreateNew" }) }, []) - const enabledHooks = hooks?.enabledHooks || [] + const enabledHooks = useMemo(() => { + const list = hooks?.enabledHooks ? [...hooks.enabledHooks] : [] + // Stable ordering to prevent jitter: sort by config file creation time (oldest first). + // Fall back to filePath/id as a deterministic tie-breaker. + return list.sort((a, b) => { + const aCreated = a.createdAt ?? Number.MAX_SAFE_INTEGER + const bCreated = b.createdAt ?? Number.MAX_SAFE_INTEGER + if (aCreated !== bCreated) return aCreated - bCreated + const aFile = a.filePath ?? "" + const bFile = b.filePath ?? "" + if (aFile !== bFile) return aFile.localeCompare(bFile) + return a.id.localeCompare(b.id) + }) + }, [hooks?.enabledHooks]) const hasProjectHooks = hooks?.hasProjectHooks || false const snapshotTimestamp = hooks?.snapshotTimestamp @@ -351,6 +364,30 @@ const HookItem: React.FC = ({ hook, onToggle, autoExpandHookId, o } }, [matcherRaw]) + const customMatcherTooltip = useMemo(() => { + return ( +
+
Regex / matcher tips
+
    +
  • + This field matches tool names. Use regex alternation with |{" "} + (e.g. fetch_instructions|search_files). +
    • +
  • + If you select tool groups and also add a custom matcher, they are combined with{" "} + |. +
    • +
  • + Available tool groups: read,{" "} + edit, browser,{" "} + command, mcp,{" "} + modes. +
    • +
+
+ ) + }, []) + const [customMatcher, setCustomMatcher] = useState(matcherCustom) useEffect(() => { setCustomMatcher(matcherCustom) @@ -392,6 +429,27 @@ const HookItem: React.FC = ({ hook, onToggle, autoExpandHookId, o [hook.filePath, hook.id], ) + const selectedEventsSet = useMemo(() => new Set(selectedEvents), [selectedEvents]) + + const handleEventToggle = useCallback( + (event: HookEventOption, checked: boolean) => { + // Base events should come from the current resolved view, but we use a Set to + // ensure we merge cleanly and avoid duplicates before sending to the backend. + const nextSet = new Set(selectedEventsSet) + if (checked) { + nextSet.add(event) + } else { + nextSet.delete(event) + } + const next = HOOK_EVENT_OPTIONS.filter((e) => nextSet.has(e)) + if (next.length === 0) { + return + } + postHookUpdate({ events: next }) + }, + [postHookUpdate, selectedEventsSet], + ) + const validateHookIdDraft = useCallback( (nextId: string): string | null => { const trimmed = nextId.trim() @@ -668,16 +726,7 @@ const HookItem: React.FC = ({ hook, onToggle, autoExpandHookId, o type="checkbox" checked={selectedEvents.includes(event)} disabled={!canEditConfig || isUpdatingConfig} - onChange={(e) => { - const checked = e.target.checked - const next = checked - ? Array.from(new Set([...selectedEvents, event])) - : selectedEvents.filter((x) => x !== event) - if (next.length === 0) { - return - } - postHookUpdate({ events: next }) - }} + onChange={(e) => handleEventToggle(event, e.target.checked)} /> {event} @@ -731,9 +780,23 @@ const HookItem: React.FC = ({ hook, onToggle, autoExpandHookId, o ))}
- +
+ + + + +
= ({ hook, onToggle, autoExpandHookId, o const next = buildMatcherString(matcherGroups, customMatcher) postHookUpdate({ matcher: next }) }} - placeholder="file.*\\.ts" + placeholder="fetch_instructions|search_files" />