fix: preserve multi-event hooks and multi-group matchers

- Keep multiple events for same hook id when definitions match\n- Expand matcher groups when combined with | (e.g. read|edit)\n- Add regression tests for both cases
This commit is contained in:
Toray Altas 2026-01-17 23:46:20 -05:00
parent d9509c639a
commit 7d0d714258
9 changed files with 257 additions and 40 deletions

View file

@ -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 */

View file

@ -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,

View file

@ -3727,7 +3727,7 @@ export const webviewMessageHandler = async (
enabled: true
command: |-
echo "Verification hook triggered"
timeout: 5
timeout: 15
`
await safeWriteText(exampleFilePath, exampleContent)

View file

@ -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<HookEventType, HookDefinition[]>
errors: string[]
}
@ -146,11 +147,21 @@ async function loadConfigFile(filePath: string, source: HookSource): Promise<Loa
const result: LoadedConfigFile = {
filePath,
source,
createdAt: undefined,
hooks: new Map(),
errors: [],
}
try {
// Prefer file birthtime when available. Fall back to ctime/mtime.
const stat = await fs.stat(filePath)
result.createdAt =
stat.birthtimeMs && stat.birthtimeMs > 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<string, ResolvedHook>
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<string, ResolvedHook>()
@ -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<HookEventType>([...(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)
}
}
}

View file

@ -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
}
/**

View file

@ -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", () => {

View file

@ -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)

View file

@ -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
}

View file

@ -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<HookItemProps> = ({ hook, onToggle, autoExpandHookId, o
}
}, [matcherRaw])
const customMatcherTooltip = useMemo(() => {
return (
<div className="text-xs leading-snug">
<div className="font-medium mb-1">Regex / matcher tips</div>
<ul className="list-disc list-inside space-y-1">
<li>
This field matches tool names. Use regex alternation with <code className="font-mono">|</code>{" "}
(e.g. <code className="font-mono">fetch_instructions|search_files</code>).
</li>
<li>
If you select tool groups and also add a custom matcher, they are combined with{" "}
<code className="font-mono">|</code>.
</li>
<li>
Available tool groups: <code className="font-mono">read</code>,{" "}
<code className="font-mono">edit</code>, <code className="font-mono">browser</code>,{" "}
<code className="font-mono">command</code>, <code className="font-mono">mcp</code>,{" "}
<code className="font-mono">modes</code>.
</li>
</ul>
</div>
)
}, [])
const [customMatcher, setCustomMatcher] = useState(matcherCustom)
useEffect(() => {
setCustomMatcher(matcherCustom)
@ -392,6 +429,27 @@ const HookItem: React.FC<HookItemProps> = ({ 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<HookEventOption>(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<HookItemProps> = ({ 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)}
/>
<span className="flex items-center gap-1">
<span>{event}</span>
@ -731,9 +780,23 @@ const HookItem: React.FC<HookItemProps> = ({ hook, onToggle, autoExpandHookId, o
))}
</div>
<div className="mt-2">
<label className="text-xs text-vscode-descriptionForeground block mb-1">
Custom matcher
</label>
<div className="flex items-center gap-2">
<label className="text-xs text-vscode-descriptionForeground block mb-1">
Custom matcher
</label>
<StandardTooltip content={customMatcherTooltip} maxWidth={360}>
<button
type="button"
className="text-vscode-descriptionForeground hover:text-vscode-foreground"
aria-label="Custom matcher help"
onClick={(e) => e.preventDefault()}>
<span
className="codicon codicon-question"
style={{ fontSize: "12px" }}
/>
</button>
</StandardTooltip>
</div>
<input
className="w-full text-xs bg-vscode-input-background border border-vscode-input-border rounded px-2 py-1 text-vscode-foreground"
aria-label="Custom matcher"
@ -744,7 +807,7 @@ const HookItem: React.FC<HookItemProps> = ({ hook, onToggle, autoExpandHookId, o
const next = buildMatcherString(matcherGroups, customMatcher)
postHookUpdate({ matcher: next })
}}
placeholder="file.*\\.ts"
placeholder="fetch_instructions|search_files"
/>
</div>
</div>