diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index 004f978fba..ac3dc3cf51 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -96,6 +96,7 @@ export interface ExtensionMessage { | "modes" | "taskWithAggregatedCosts" | "hookExecutionStatus" + | "hooksCopyHookResult" text?: string payload?: any // eslint-disable-line @typescript-eslint/no-explicit-any checkpointWarning?: { @@ -626,6 +627,7 @@ export interface WebviewMessage { | "hooksOpenHookFile" | "hooksCreateNew" | "hooksUpdateHook" + | "hooksCopyHook" text?: string editedMessageContent?: string tab?: "settings" | "history" | "mcp" | "modes" | "chat" | "marketplace" | "cloud" @@ -688,6 +690,12 @@ export interface WebviewMessage { hookUpdates?: { events?: string[] matcher?: string + id?: string + command?: string + enabled?: boolean + description?: string + shell?: string + includeConversationHistory?: boolean timeout?: number } // For hooksUpdateHook filePath?: string // For hooksOpenHookFile diff --git a/src/core/webview/__tests__/webviewMessageHandler.hooks.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.hooks.spec.ts index aaa0200e21..59621f96a6 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.hooks.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.hooks.spec.ts @@ -70,6 +70,14 @@ vi.mock("../../../utils/safeWriteText", () => ({ safeWriteText: vi.fn().mockResolvedValue(undefined), })) +vi.mock("../../../services/hooks/HookConfigWriter", async () => { + const actual = await vi.importActual("../../../services/hooks/HookConfigWriter") + return { + ...actual, + copyHookConfig: vi.fn(), + } +}) + vi.mock("../../../api/providers/fetchers/modelCache") import * as vscode from "vscode" @@ -78,6 +86,7 @@ import * as fsUtils from "../../../utils/fs" import { safeWriteJson } from "../../../utils/safeWriteJson" import { webviewMessageHandler } from "../webviewMessageHandler" import type { ClineProvider } from "../ClineProvider" +import { copyHookConfig } from "../../../services/hooks/HookConfigWriter" // Create mock HookManager const createMockHookManager = (): IHookManager => ({ @@ -395,13 +404,10 @@ describe("webviewMessageHandler - hooks commands", () => { vi.mocked(fs.readFile).mockResolvedValueOnce( JSON.stringify({ - version: "1", - hooks: { - PreToolUse: [ - { id: hookId, command: "echo hi" }, - { id: "keep", command: "echo keep" }, - ], - }, + hooks: [ + { id: hookId, events: ["PreToolUse"], command: "echo hi" }, + { id: "keep", events: ["PreToolUse"], command: "echo keep" }, + ], }), ) @@ -413,10 +419,7 @@ describe("webviewMessageHandler - hooks commands", () => { expect(safeWriteJson).toHaveBeenCalledWith( hookFilePath, expect.objectContaining({ - version: "1", - hooks: { - PreToolUse: [{ id: "keep", command: "echo keep" }], - }, + hooks: [{ id: "keep", events: ["PreToolUse"], command: "echo keep" }], }), ) expect(mockHookManager.reloadHooksConfig).toHaveBeenCalledTimes(1) @@ -450,10 +453,7 @@ describe("webviewMessageHandler - hooks commands", () => { vi.mocked(fs.readFile).mockResolvedValueOnce( JSON.stringify({ - version: "1", - hooks: { - PreToolUse: [{ id: "keep", command: "echo keep" }], - }, + hooks: [{ id: "keep", events: ["PreToolUse"], command: "echo keep" }], }), ) @@ -469,6 +469,94 @@ describe("webviewMessageHandler - hooks commands", () => { }) }) + describe("hooksCopyHook", () => { + it("should copy hook via HookConfigWriter.copyHookConfig and then reload + post state", async () => { + const hookId = "hook-to-copy" + const hookFilePath = "/mock/workspace/.roo/hooks/hooks.json" + + const hooksById = new Map() + hooksById.set(hookId, { + id: hookId, + event: "PreToolUse" as any, + matcher: ".*", + command: "echo hi", + enabled: true, + source: "project" as any, + timeout: 30, + filePath: hookFilePath, + includeConversationHistory: false, + } as any) + + vi.mocked(mockHookManager.getConfigSnapshot).mockReturnValue({ + hooksByEvent: new Map(), + hooksById, + loadedAt: new Date(), + disabledHookIds: new Set(), + hasProjectHooks: true, + } as HooksConfigSnapshot) + + vi.mocked(copyHookConfig as any).mockResolvedValueOnce("hook-to-copy-copy") + + await webviewMessageHandler(mockClineProvider, { + type: "hooksCopyHook", + hookId, + } as any) + + expect(copyHookConfig).toHaveBeenCalledWith(hookFilePath, hookId) + expect(mockHookManager.reloadHooksConfig).toHaveBeenCalledTimes(1) + expect(mockClineProvider.postStateToWebview).toHaveBeenCalledTimes(1) + expect(mockClineProvider.postMessageToWebview).toHaveBeenCalledWith( + expect.objectContaining({ + type: "hooksCopyHookResult", + success: true, + values: expect.objectContaining({ hookId: "hook-to-copy-copy", filePath: hookFilePath }), + }), + ) + }) + + it("should show error and send failure result when copy fails", async () => { + const hookId = "hook-to-copy" + const hookFilePath = "/mock/workspace/.roo/hooks/hooks.json" + + const hooksById = new Map() + hooksById.set(hookId, { + id: hookId, + event: "PreToolUse" as any, + matcher: ".*", + command: "echo hi", + enabled: true, + source: "project" as any, + timeout: 30, + filePath: hookFilePath, + includeConversationHistory: false, + } as any) + + vi.mocked(mockHookManager.getConfigSnapshot).mockReturnValue({ + hooksByEvent: new Map(), + hooksById, + loadedAt: new Date(), + disabledHookIds: new Set(), + hasProjectHooks: true, + } as HooksConfigSnapshot) + + vi.mocked(copyHookConfig as any).mockRejectedValueOnce(new Error("boom")) + + await webviewMessageHandler(mockClineProvider, { + type: "hooksCopyHook", + hookId, + } as any) + + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("Failed to copy hook") + expect(mockClineProvider.postMessageToWebview).toHaveBeenCalledWith( + expect.objectContaining({ + type: "hooksCopyHookResult", + success: false, + error: expect.stringContaining("boom"), + }), + ) + }) + }) + describe("hooksOpenHookFile", () => { it("should open hook file in editor when filePath is provided and file exists", async () => { const hookFilePath = "/mock/workspace/.roo/hooks/hooks.json" diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 56c55ada36..6fb15c4e56 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -8,6 +8,7 @@ import { type HookEventType, type HookUpdateData, } from "../../services/hooks/types" +import { copyHookConfig } from "../../services/hooks/HookConfigWriter" import { getRooDirectoriesForCwd } from "../../services/roo-config/index.js" import pWaitFor from "p-wait-for" import * as vscode from "vscode" @@ -3457,14 +3458,22 @@ export const webviewMessageHandler = async ( return false } + // New hook-centric format: hooks: [ ... ] let removed = false - for (const [event, defs] of Object.entries(hooks)) { - if (!Array.isArray(defs)) continue - const before = defs.length - const after = defs.filter((d: any) => d?.id !== hookId) - if (after.length !== before) { - removed = true - ;(hooks as any)[event] = after + if (Array.isArray(hooks)) { + const before = hooks.length + ;(parsed as any).hooks = hooks.filter((d: any) => d?.id !== hookId) + removed = (parsed as any).hooks.length !== before + } else { + // Legacy event-keyed format: hooks: { PreToolUse: [ ... ] } + for (const [event, defs] of Object.entries(hooks)) { + if (!Array.isArray(defs)) continue + const before = defs.length + const after = defs.filter((d: any) => d?.id !== hookId) + if (after.length !== before) { + removed = true + ;(hooks as any)[event] = after + } } } @@ -3534,6 +3543,87 @@ export const webviewMessageHandler = async ( break } + case "hooksCopyHook": { + const hookManager = provider.getHookManager() + if (!hookManager || !message.hookId) { + break + } + + try { + const hookId = message.hookId + const snapshot = hookManager.getConfigSnapshot() + const targetHook = snapshot?.hooksById.get(hookId) + const targetFilePath = targetHook?.filePath + + let newId: string | undefined + let copiedFromFilePath: string | undefined + + if (typeof targetFilePath === "string" && targetFilePath.length > 0) { + newId = await copyHookConfig(targetFilePath, hookId) + copiedFromFilePath = targetFilePath + } else { + // Fallback: scan all loaded roo directories for hook configs and copy matching id. + const cwd = provider.cwd + const rooDirs = getRooDirectoriesForCwd(cwd) + const candidateDirs: string[] = [] + candidateDirs.push(path.join(rooDirs[0], "hooks")) + if (message.hooksSource === "mode") { + const mode = (await provider.getState()).mode + candidateDirs.push(path.join(rooDirs[1], `hooks-${mode}`)) + } + candidateDirs.push(path.join(rooDirs[1], "hooks")) + + for (const dir of candidateDirs) { + let entries: any[] = [] + try { + entries = await fs.readdir(dir, { withFileTypes: true }) + } catch { + continue + } + const files = entries + .filter((e) => e.isFile()) + .map((e) => path.join(dir, e.name)) + .filter((p) => { + const l = p.toLowerCase() + return l.endsWith(".json") || l.endsWith(".yaml") || l.endsWith(".yml") + }) + .sort() + for (const filePath of files) { + try { + newId = await copyHookConfig(filePath, hookId) + copiedFromFilePath = filePath + break + } catch { + // not found / not writable; keep scanning + } + } + if (newId) break + } + } + + if (!newId) { + throw new Error("Hook not found in any writable config file") + } + + await hookManager.reloadHooksConfig() + await provider.postStateToWebview() + await provider.postMessageToWebview({ + type: "hooksCopyHookResult", + success: true, + values: { hookId: newId, filePath: copiedFromFilePath }, + }) + } catch (error) { + provider.log(`Failed to copy hook: ${error instanceof Error ? error.message : String(error)}`) + vscode.window.showErrorMessage("Failed to copy hook") + await provider.postMessageToWebview({ + type: "hooksCopyHookResult", + success: false, + error: error instanceof Error ? error.message : String(error), + }) + } + break + } + case "hooksOpenHookFile": { const { filePath: hookFilePath } = message if (!hookFilePath) { @@ -3572,7 +3662,31 @@ export const webviewMessageHandler = async ( ) : undefined, matcher: typeof message.hookUpdates.matcher === "string" ? message.hookUpdates.matcher : undefined, + command: + typeof (message.hookUpdates as any).command === "string" + ? (message.hookUpdates as any).command + : undefined, timeout: typeof message.hookUpdates.timeout === "number" ? message.hookUpdates.timeout : undefined, + enabled: + typeof (message.hookUpdates as any).enabled === "boolean" + ? (message.hookUpdates as any).enabled + : undefined, + description: + typeof (message.hookUpdates as any).description === "string" + ? (message.hookUpdates as any).description + : undefined, + shell: + typeof (message.hookUpdates as any).shell === "string" + ? (message.hookUpdates as any).shell + : undefined, + includeConversationHistory: + typeof (message.hookUpdates as any).includeConversationHistory === "boolean" + ? (message.hookUpdates as any).includeConversationHistory + : undefined, + id: + typeof (message.hookUpdates as any).id === "string" + ? (message.hookUpdates as any).id + : undefined, } await hookManager.updateHook(message.filePath, message.hookId, hookUpdates) @@ -3605,15 +3719,15 @@ export const webviewMessageHandler = async ( break } - // Create the example hook file - const exampleContent = `version: "1" -hooks: - PreToolUse: - - id: example-hook - matcher: "edit" - enabled: true - command: 'echo "Verification hook triggered"' - timeout: 5 + // Create the example hook file (Phase 2 hook-centric schema) + const exampleContent = `hooks: + - id: example-hook + events: ["PreToolUse"] + matcher: "edit" + enabled: true + command: |- + echo "Verification hook triggered" + timeout: 5 ` await safeWriteText(exampleFilePath, exampleContent) diff --git a/src/services/hooks/HookConfigLoader.ts b/src/services/hooks/HookConfigLoader.ts index 77f11f5f72..1199981ba6 100644 --- a/src/services/hooks/HookConfigLoader.ts +++ b/src/services/hooks/HookConfigLoader.ts @@ -16,6 +16,7 @@ import YAML from "yaml" import { z } from "zod" import { HooksConfigFileSchema, + LegacyHooksConfigFileSchema, HooksConfigSnapshot, HookDefinition, ResolvedHook, @@ -98,6 +99,46 @@ function validateConfig( return { success: false, errors } } +function toHooksByEventMap(data: z.infer): Map { + const hooks = new Map() + + // New format: hooks: HookDefinitionWithEvents[] + if (Array.isArray((data as any).hooks)) { + for (const def of (data as any).hooks as Array) { + const events: HookEventType[] = Array.isArray(def?.events) ? def.events : [] + for (const event of events) { + const hookDef: HookDefinition = { + id: def.id, + matcher: def.matcher, + enabled: def.enabled, + command: def.command, + timeout: def.timeout, + description: def.description, + shell: def.shell, + includeConversationHistory: def.includeConversationHistory, + } + if (!hooks.has(event)) { + hooks.set(event, []) + } + hooks.get(event)!.push(hookDef) + } + } + return hooks + } + + // Legacy format: hooks: Record + const legacy = LegacyHooksConfigFileSchema.parse(data) + const hooksRecord = legacy.hooks || {} + for (const [eventStr, definitions] of Object.entries(hooksRecord)) { + const event = eventStr as HookEventType + if (definitions && definitions.length > 0) { + hooks.set(event, definitions) + } + } + + return hooks +} + /** * Load a single config file. */ @@ -119,14 +160,7 @@ async function loadConfigFile(filePath: string, source: HookSource): Promise 0) { - result.hooks.set(event, definitions) - } - } + result.hooks = toHooksByEventMap(validated.data) } catch (err) { if ((err as NodeJS.ErrnoException).code === "ENOENT") { // File doesn't exist - not an error, just skip diff --git a/src/services/hooks/HookConfigWriter.ts b/src/services/hooks/HookConfigWriter.ts index 47b05c41c0..7cf0dee8b4 100644 --- a/src/services/hooks/HookConfigWriter.ts +++ b/src/services/hooks/HookConfigWriter.ts @@ -1,9 +1,50 @@ import YAML from "yaml" -import { HookUpdateData, HookEventType, HookDefinition } from "./types" +import { HookUpdateData, HookEventType, HookDefinitionWithEvents } from "./types" import fs from "fs/promises" import { safeWriteJson } from "../../utils/safeWriteJson" import { safeWriteText } from "../../utils/safeWriteText" +function parseConfigContent(content: string, filePath: string): any { + const isJson = filePath.toLowerCase().endsWith(".json") + try { + return isJson ? JSON.parse(content) : YAML.parse(content) + } catch (e) { + throw new Error(`Failed to parse config file ${filePath}: ${e}`) + } +} + +function ensureNewFormatHooksArray(parsed: any): HookDefinitionWithEvents[] { + if (!parsed || typeof parsed !== "object") { + throw new Error("Invalid config file format") + } + if (!parsed.hooks) { + parsed.hooks = [] + } + if (!Array.isArray(parsed.hooks)) { + throw new Error( + "This operation requires a hook-centric config file (hooks: [ ... ]). " + + "Legacy event-keyed hook configs are supported for loading but are not writable by the UI.", + ) + } + return parsed.hooks as HookDefinitionWithEvents[] +} + +function generateCopyHookId(existingIds: Set, originalId: string): string { + const base = `${originalId}-copy` + if (!existingIds.has(base)) { + return base + } + + for (let i = 2; i < 10_000; i++) { + const candidate = `${base}-${i}` + if (!existingIds.has(candidate)) { + return candidate + } + } + + throw new Error(`Unable to generate unique copied hook ID for '${originalId}'`) +} + /** * Update a hook configuration in a YAML/JSON file. * @@ -15,107 +56,83 @@ export async function updateHookConfig(filePath: string, hookId: string, updates const isJson = filePath.toLowerCase().endsWith(".json") const content = await fs.readFile(filePath, "utf-8") - let parsed: any - try { - if (isJson) { - parsed = JSON.parse(content) - } else { - parsed = YAML.parse(content) - } - } catch (e) { - throw new Error(`Failed to parse config file ${filePath}: ${e}`) - } + const parsed = parseConfigContent(content, filePath) + const hooks = ensureNewFormatHooksArray(parsed) - if (!parsed || typeof parsed !== "object") { - throw new Error(`Invalid config file format: ${filePath}`) - } - - if (!parsed.hooks) { - parsed.hooks = {} - } - - // Find the hook definition first to ensure it exists and get a template - let templateHook: HookDefinition | undefined - - // Iterate all events to find the hook - for (const eventKey of Object.keys(parsed.hooks)) { - const hooks = parsed.hooks[eventKey] - if (Array.isArray(hooks)) { - const found = hooks.find((h: any) => h.id === hookId) - if (found) { - templateHook = { ...found } - break - } - } - } - - if (!templateHook) { + const hookIndex = hooks.findIndex((h) => h?.id === hookId) + if (hookIndex === -1) { throw new Error(`Hook with ID '${hookId}' not found in ${filePath}`) } - // Apply simple property updates to the template first, so they carry over to new events - if (updates.matcher !== undefined) { - if (updates.matcher === "") { - delete templateHook.matcher - } else { - templateHook.matcher = updates.matcher + const hook = hooks[hookIndex] + + if (updates.id !== undefined) { + const trimmed = updates.id.trim() + if (trimmed.length === 0) { + throw new Error("Hook ID cannot be empty") } - } - if (updates.timeout !== undefined) { - templateHook.timeout = updates.timeout + if (trimmed.length > 100) { + throw new Error("Hook ID must be 100 characters or less") + } + if (!/^[A-Za-z0-9_-]+$/.test(trimmed)) { + throw new Error("Hook ID must contain only letters, numbers, hyphens, and underscores") + } + const duplicate = hooks.some((h, idx) => idx !== hookIndex && h?.id === trimmed) + if (duplicate) { + throw new Error("Hook ID must be unique within this file") + } + hook.id = trimmed } - // Handle Event Updates if (updates.events) { if (updates.events.length === 0) { throw new Error("Hook must have at least one event") } - const newEventsSet = new Set(updates.events) + hook.events = [...updates.events] + } - // 1. Remove hook from events that are NOT in the new set - for (const eventKey of Object.keys(parsed.hooks)) { - const event = eventKey as HookEventType - if (!newEventsSet.has(event)) { - const hooks = parsed.hooks[event] - if (Array.isArray(hooks)) { - parsed.hooks[event] = hooks.filter((h: any) => h?.id !== hookId) - } - } - } - - // 2. Add hook to events that are in the new set - for (const event of updates.events) { - if (!parsed.hooks[event]) { - parsed.hooks[event] = [] - } - const hooks = parsed.hooks[event] - // Check if already exists - const existing = hooks.find((h: any) => h.id === hookId) - if (!existing) { - // Add the template hook - hooks.push({ ...templateHook }) - } + if (updates.matcher !== undefined) { + if (updates.matcher === "") { + delete (hook as any).matcher + } else { + hook.matcher = updates.matcher } } - // Apply property updates to ALL instances of the hook in the file - for (const eventKey of Object.keys(parsed.hooks)) { - const hooks = parsed.hooks[eventKey] - if (Array.isArray(hooks)) { - const hook = hooks.find((h: any) => h.id === hookId) - if (hook) { - if (updates.matcher !== undefined) { - if (updates.matcher === "") { - delete hook.matcher - } else { - hook.matcher = updates.matcher - } - } - if (updates.timeout !== undefined) { - hook.timeout = updates.timeout - } - } + if (updates.command !== undefined) { + const trimmed = updates.command.trim() + if (trimmed.length === 0) { + throw new Error("Command cannot be empty") } + hook.command = updates.command + } + + if (updates.timeout !== undefined) { + hook.timeout = updates.timeout + } + + if (updates.enabled !== undefined) { + hook.enabled = updates.enabled + } + + if (updates.description !== undefined) { + if (updates.description === "") { + delete (hook as any).description + } else { + hook.description = updates.description + } + } + + if (updates.shell !== undefined) { + if (updates.shell === "") { + delete (hook as any).shell + } else { + hook.shell = updates.shell + } + } + + if (updates.includeConversationHistory !== undefined) { + hook.includeConversationHistory = updates.includeConversationHistory } // Write back (atomic) @@ -125,3 +142,46 @@ export async function updateHookConfig(filePath: string, hookId: string, updates await safeWriteText(filePath, YAML.stringify(parsed)) } } + +/** + * Duplicate a hook within the same config file. + * + * The copied hook will be inserted after the source hook in the array. + * The new hook will receive a unique ID within the file: + * - `${id}-copy` + * - `${id}-copy-2`, `${id}-copy-3`, ... + */ +export async function copyHookConfig(filePath: string, hookId: string): Promise { + const isJson = filePath.toLowerCase().endsWith(".json") + const content = await fs.readFile(filePath, "utf-8") + + const parsed = parseConfigContent(content, filePath) + const hooks = ensureNewFormatHooksArray(parsed) + + const hookIndex = hooks.findIndex((h) => h?.id === hookId) + if (hookIndex === -1) { + throw new Error(`Hook with ID '${hookId}' not found in ${filePath}`) + } + + const sourceHook = hooks[hookIndex] + const existingIds = new Set(hooks.map((h) => h?.id).filter((id): id is string => typeof id === "string")) + const newId = generateCopyHookId(existingIds, sourceHook.id) + + const copied: HookDefinitionWithEvents = { + ...sourceHook, + id: newId, + events: Array.isArray(sourceHook.events) ? [...sourceHook.events] : [], + } + + // Insert right after the original hook for better UX. + hooks.splice(hookIndex + 1, 0, copied) + + // Write back (atomic) + if (isJson) { + await safeWriteJson(filePath, parsed) + } else { + await safeWriteText(filePath, YAML.stringify(parsed)) + } + + return newId +} diff --git a/src/services/hooks/__tests__/HookConfigLoader.spec.ts b/src/services/hooks/__tests__/HookConfigLoader.spec.ts index 252cb5ae00..7cd65cec38 100644 --- a/src/services/hooks/__tests__/HookConfigLoader.spec.ts +++ b/src/services/hooks/__tests__/HookConfigLoader.spec.ts @@ -49,13 +49,12 @@ describe("HookConfigLoader", () => { it("should parse YAML config files", async () => { const yamlContent = ` -version: "1" hooks: - PreToolUse: - - id: lint-check - matcher: "Edit|Write" - command: "./lint.sh" - timeout: 30 + - id: lint-check + events: ["PreToolUse"] + matcher: "Edit|Write" + command: "./lint.sh" + timeout: 30 ` mockFsPromises.readdir.mockImplementation(async (dirPath) => { const dir = dirPath.toString() @@ -84,10 +83,7 @@ hooks: it("should parse JSON config files", async () => { const jsonContent = JSON.stringify({ - version: "1", - hooks: { - PostToolUse: [{ id: "notify-slack", command: "./notify.sh" }], - }, + hooks: [{ id: "notify-slack", events: ["PostToolUse"], command: "./notify.sh" }], }) mockFsPromises.readdir.mockImplementation(async (dirPath) => { @@ -113,11 +109,10 @@ hooks: it("should report validation errors for invalid config", async () => { // Missing required 'command' field const invalidYaml = ` -version: "1" hooks: - PreToolUse: - - id: bad-hook - command: "" + - id: bad-hook + events: ["PreToolUse"] + command: "" ` mockFsPromises.readdir.mockImplementation(async (dirPath) => { const dir = dirPath.toString() @@ -142,18 +137,16 @@ hooks: it("should merge configs with project taking precedence over global", async () => { const globalYaml = ` -version: "1" hooks: - PreToolUse: - - id: shared-hook - command: "global-command" + - id: shared-hook + events: ["PreToolUse"] + command: "global-command" ` const projectYaml = ` -version: "1" hooks: - PreToolUse: - - id: shared-hook - command: "project-command" + - id: shared-hook + events: ["PreToolUse"] + command: "project-command" ` mockFsPromises.readdir.mockImplementation(async (dirPath) => { const dir = dirPath.toString() @@ -184,11 +177,10 @@ hooks: it("should include mode-specific hooks when mode is provided", async () => { const modeYaml = ` -version: "1" hooks: - PreToolUse: - - id: mode-hook - command: "./mode-specific.sh" + - id: mode-hook + events: ["PreToolUse"] + command: "./mode-specific.sh" ` mockFsPromises.readdir.mockImplementation(async (dirPath) => { const dir = dirPath.toString() @@ -212,11 +204,10 @@ hooks: it("should set hasProjectHooks flag when project hooks exist", async () => { const projectYaml = ` -version: "1" hooks: - PreToolUse: - - id: project-hook - command: "./project.sh" + - id: project-hook + events: ["PreToolUse"] + command: "./project.sh" ` mockFsPromises.readdir.mockImplementation(async (dirPath) => { const dir = dirPath.toString() diff --git a/src/services/hooks/types.ts b/src/services/hooks/types.ts index 664c2476db..461896dc37 100644 --- a/src/services/hooks/types.ts +++ b/src/services/hooks/types.ts @@ -58,34 +58,46 @@ export function isBlockingEvent(event: HookEventType): boolean { /** * Schema for a single hook definition within a config file. */ -export const HookDefinitionSchema = z.object({ - /** Unique identifier for this hook */ - id: z.string().min(1, "Hook ID cannot be empty"), +export const HookDefinitionSchema = z + .object({ + /** Unique identifier for this hook */ + id: z.string().min(1, "Hook ID cannot be empty"), - /** Tool name filter (regex/glob pattern). If omitted, matches all tools. */ - matcher: z.string().optional(), + /** Tool name filter (regex/glob pattern). If omitted, matches all tools. */ + matcher: z.string().optional(), - /** Whether this hook is enabled. Defaults to true. */ - enabled: z.boolean().optional().default(true), + /** Whether this hook is enabled. Defaults to true. */ + enabled: z.boolean().optional().default(true), - /** Shell command to execute */ - command: z.string().min(1, "Command cannot be empty"), + /** Shell command to execute */ + command: z.string().min(1, "Command cannot be empty"), - /** Timeout in seconds. Defaults to 60. */ - timeout: z.number().positive().optional().default(60), + /** Timeout in seconds. Defaults to 60. */ + timeout: z.number().positive().optional().default(60), - /** Human-readable description of what this hook does */ - description: z.string().optional(), + /** Human-readable description of what this hook does */ + description: z.string().optional(), - /** Override shell (default: user's shell on Unix, PowerShell on Windows) */ - shell: z.string().optional(), + /** Override shell (default: user's shell on Unix, PowerShell on Windows) */ + shell: z.string().optional(), - /** Opt-in to receive conversation history in stdin. Defaults to false. */ - includeConversationHistory: z.boolean().optional().default(false), -}) + /** Opt-in to receive conversation history in stdin. Defaults to false. */ + includeConversationHistory: z.boolean().optional().default(false), + }) + .strip() export type HookDefinition = z.infer +/** + * Schema for a single hook definition in the new (hook-centric) config format. + */ +export const HookDefinitionWithEventsSchema = HookDefinitionSchema.extend({ + /** Event types this hook should run for */ + events: z.array(HookEventType).min(1, "Hook must have at least one event"), +}).strip() + +export type HookDefinitionWithEvents = z.infer + // ============================================================================ // Hook Config File Schema // ============================================================================ @@ -93,13 +105,27 @@ export type HookDefinition = z.infer /** * Schema for a hooks configuration file (.roo/hooks/*.yaml or *.json). */ -export const HooksConfigFileSchema = z.object({ - /** Config format version */ - version: z.literal("1"), +export const LegacyHooksConfigFileSchema = z + .object({ + /** Hooks organized by event type (legacy format) */ + hooks: z.record(HookEventType, z.array(HookDefinitionSchema)).optional().default({}), + }) + .strip() - /** Hooks organized by event type */ - hooks: z.record(HookEventType, z.array(HookDefinitionSchema)).optional().default({}), -}) +export type LegacyHooksConfigFile = z.infer + +export const HooksConfigFileSchema = z.union([ + z + .object({ + /** Hooks stored as an array with an events field (new format) */ + hooks: z.array(HookDefinitionWithEventsSchema).optional().default([]), + }) + .strip(), + LegacyHooksConfigFileSchema, +]) +// Strip unknown keys (e.g., legacy version key) +// so the system does not read/write/require version keys. +// Note: .strip() is already applied to each branch above. export type HooksConfigFile = z.infer @@ -116,9 +142,16 @@ export type HookSource = "project" | "mode" | "global" * Data for updating a hook configuration. */ export interface HookUpdateData { + /** Rename the hook ID (must be unique within the file) */ + id?: string events?: HookEventType[] matcher?: string + command?: string timeout?: number + enabled?: boolean + description?: string + shell?: string + includeConversationHistory?: boolean } /** diff --git a/webview-ui/src/components/settings/HooksSettings.tsx b/webview-ui/src/components/settings/HooksSettings.tsx index c76e9d6d1b..b58f0867f7 100644 --- a/webview-ui/src/components/settings/HooksSettings.tsx +++ b/webview-ui/src/components/settings/HooksSettings.tsx @@ -1,5 +1,5 @@ -import React, { useCallback, useEffect, useMemo, useState } from "react" -import { RefreshCw, FolderOpen, AlertTriangle, Clock, FishingHook, X, Plus } from "lucide-react" +import React, { useCallback, useEffect, useMemo, useRef, useState } from "react" +import { RefreshCw, FolderOpen, AlertTriangle, Clock, FishingHook, X, Plus, Copy } from "lucide-react" import { VSCodeDropdown, VSCodeOption, @@ -51,6 +51,7 @@ export const HooksSettings: React.FC = () => { const { hooks, hooksEnabled } = useExtensionState() const [executionHistory, setExecutionHistory] = useState(hooks?.executionHistory || []) const [isUpdatingEnabled, setIsUpdatingEnabled] = useState(false) + const [pendingFocusHookId, setPendingFocusHookId] = useState(null) // Master toggle state - defaults to true if not explicitly set const isHooksEnabled = hooksEnabled ?? true @@ -80,6 +81,12 @@ export const HooksSettings: React.FC = () => { setExecutionHistory((prev) => [record, ...prev].slice(0, 50)) // Keep last 50 } } + + if (message.type === "hooksCopyHookResult") { + if (message.success && message.values?.hookId) { + setPendingFocusHookId(String(message.values.hookId)) + } + } } window.addEventListener("message", handleMessage) @@ -217,7 +224,13 @@ export const HooksSettings: React.FC = () => { ) : (
{enabledHooks.map((hook) => ( - + setPendingFocusHookId(null)} + /> ))}
)} @@ -269,14 +282,22 @@ export const HooksSettings: React.FC = () => { interface HookItemProps { hook: HookInfo onToggle: (hookId: string, enabled: boolean) => void + autoExpandHookId?: string | null + onAutoExpanded?: () => void } -const HookItem: React.FC = ({ hook, onToggle }) => { +const HookItem: React.FC = ({ hook, onToggle, autoExpandHookId, onAutoExpanded }) => { const { t } = useAppTranslation() const { hooks } = useExtensionState() const [isExpanded, setIsExpanded] = useState(false) const [hookLogs, setHookLogs] = useState([]) const [isUpdatingConfig, setIsUpdatingConfig] = useState(false) + const [hookIdDraft, setHookIdDraft] = useState(hook.id) + const [hookIdError, setHookIdError] = useState(null) + const [commandDraft, setCommandDraft] = useState(hook.commandPreview) + const commandTextAreaRef = useRef(null) + + const canEditConfig = Boolean(hook.filePath) const hooksForId = useMemo(() => { const enabledHooks = hooks?.enabledHooks ?? [] @@ -289,6 +310,29 @@ const HookItem: React.FC = ({ hook, onToggle }) => { return HOOK_EVENT_OPTIONS.filter((e) => events.includes(e)) }, [hooksForId]) + const eventTooltipText = useCallback( + (event: HookEventOption) => { + // Keep translation fallback-friendly: if keys don't exist yet, show English. + const fallbackMap: Record = { + PreToolUse: "Before a tool is executed. Can block execution.", + PostToolUse: "After a tool completes successfully.", + PostToolUseFailure: "After a tool fails. Can perform cleanup.", + PermissionRequest: "When a permission dialog is shown. Can auto-approve/deny.", + UserPromptSubmit: "When user submits a prompt. Can modify or block.", + Stop: "When task completes or stops. Can perform cleanup.", + SubagentStop: "When a subtask completes.", + SubagentStart: "When a subtask begins.", + SessionStart: "When a new task session starts.", + SessionEnd: "When task session fully ends.", + Notification: "When status notifications are sent.", + PreCompact: "Before context compaction occurs.", + } + + return (t(`settings:hooks.eventTooltips.${event}`) as string) || fallbackMap[event] || event + }, + [t], + ) + const matcherRaw = hook.matcher ?? "" const { matcherGroups, matcherCustom } = useMemo(() => { const parts = matcherRaw @@ -312,6 +356,15 @@ const HookItem: React.FC = ({ hook, onToggle }) => { setCustomMatcher(matcherCustom) }, [matcherCustom]) + useEffect(() => { + setHookIdDraft(hook.id) + setHookIdError(null) + }, [hook.id]) + + useEffect(() => { + setCommandDraft(hook.commandPreview) + }, [hook.commandPreview]) + const timeoutSeconds = hook.timeout const timeoutSelection = useMemo(() => { const match = TIMEOUT_OPTIONS.find((o) => o.seconds === timeoutSeconds) @@ -319,7 +372,13 @@ const HookItem: React.FC = ({ hook, onToggle }) => { }, [timeoutSeconds]) const postHookUpdate = useCallback( - (updates: { events?: HookEventOption[]; matcher?: string; timeout?: number }) => { + (updates: { + events?: HookEventOption[] + matcher?: string + timeout?: number + id?: string + command?: string + }) => { if (!hook.filePath) return setIsUpdatingConfig(true) vscode.postMessage({ @@ -333,6 +392,70 @@ const HookItem: React.FC = ({ hook, onToggle }) => { [hook.filePath, hook.id], ) + const validateHookIdDraft = useCallback( + (nextId: string): string | null => { + const trimmed = nextId.trim() + if (trimmed.length === 0) return t("settings:hooks.idErrors.required") + if (trimmed.length > 100) return t("settings:hooks.idErrors.maxLength") + if (!/^[A-Za-z0-9_-]+$/.test(trimmed)) return t("settings:hooks.idErrors.invalidChars") + + // Best-effort client-side uniqueness check within current view. + const enabledHooks = hooks?.enabledHooks ?? [] + const duplicate = enabledHooks.some( + (h) => h.filePath === hook.filePath && h.id === trimmed && h.id !== hook.id, + ) + if (duplicate) return t("settings:hooks.idErrors.notUnique") + return null + }, + [hooks?.enabledHooks, hook.filePath, hook.id, t], + ) + + const handleSaveHookId = useCallback(() => { + if (!canEditConfig) return + const error = validateHookIdDraft(hookIdDraft) + setHookIdError(error) + if (error) return + const trimmed = hookIdDraft.trim() + if (trimmed === hook.id) return + postHookUpdate({ id: trimmed }) + }, [canEditConfig, hook.id, hookIdDraft, postHookUpdate, validateHookIdDraft]) + + const handleCopyHook = useCallback(() => { + if (!hook.filePath) return + vscode.postMessage({ + type: "hooksCopyHook", + hookId: hook.id, + }) + }, [hook.filePath, hook.id]) + + const handleSaveCommand = useCallback(() => { + if (!canEditConfig) return + const trimmed = commandDraft.trim() + if (trimmed.length === 0) { + return + } + postHookUpdate({ command: commandDraft }) + }, [canEditConfig, commandDraft, postHookUpdate]) + + const handleCommandKeyDown = useCallback( + (e: React.KeyboardEvent) => { + if (e.key !== "Tab") return + e.preventDefault() + const el = e.currentTarget + const start = el.selectionStart ?? 0 + const end = el.selectionEnd ?? 0 + const next = `${commandDraft.slice(0, start)}\t${commandDraft.slice(end)}` + setCommandDraft(next) + // Re-position cursor after state update. + requestAnimationFrame(() => { + if (!commandTextAreaRef.current) return + commandTextAreaRef.current.selectionStart = start + 1 + commandTextAreaRef.current.selectionEnd = start + 1 + }) + }, + [commandDraft], + ) + const buildMatcherString = useCallback((nextGroups: ToolGroup[], nextCustom: string) => { const parts: string[] = [...nextGroups] const trimmedCustom = nextCustom.trim() @@ -402,7 +525,13 @@ const HookItem: React.FC = ({ hook, onToggle }) => { }) } - const canEditConfig = Boolean(hook.filePath) + useEffect(() => { + if (!autoExpandHookId) return + if (autoExpandHookId !== hook.id) return + setIsExpanded(true) + onAutoExpanded?.() + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [autoExpandHookId, hook.id]) const getEnabledDotColor = () => { return hook.enabled ? "var(--vscode-testing-iconPassed)" : "var(--vscode-descriptionForeground)" @@ -434,6 +563,17 @@ const HookItem: React.FC = ({ hook, onToggle }) => {
e.stopPropagation()}> + + + + + ))}
@@ -559,6 +736,7 @@ const HookItem: React.FC = ({ hook, onToggle }) => { setCustomMatcher(e.target.value)} @@ -612,19 +790,29 @@ const HookItem: React.FC = ({ hook, onToggle }) => { - +
-
- - {hook.commandPreview} - + + {t("settings:hooks.command")} + +