diff --git a/packages/types/src/global-settings.ts b/packages/types/src/global-settings.ts index 9a17834ced..7f33757837 100644 --- a/packages/types/src/global-settings.ts +++ b/packages/types/src/global-settings.ts @@ -176,6 +176,7 @@ export const globalSettingsSchema = z.object({ mcpEnabled: z.boolean().optional(), enableMcpServerCreation: z.boolean().optional(), + hooksEnabled: z.boolean().optional(), mode: z.string().optional(), modeApiConfigs: z.record(z.string(), z.string()).optional(), diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index 87aec143b5..ac1d976877 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -382,6 +382,7 @@ export type ExtensionState = Pick< mcpEnabled: boolean enableMcpServerCreation: boolean + hooksEnabled: boolean mode: string customModes: ModeConfig[] diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 7960ca2d56..790c3ce630 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -541,13 +541,23 @@ export class Task extends EventEmitter implements TaskLike { }) // Initialize tool execution hooks (only if hooks experiment is enabled) - const hooksEnabled = experiments.isEnabled(experimentsConfig ?? {}, EXPERIMENT_IDS.HOOKS) + const hooksExperimentEnabled = experiments.isEnabled(experimentsConfig ?? {}, EXPERIMENT_IDS.HOOKS) this.toolExecutionHooks = createToolExecutionHooks( - hooksEnabled ? (provider.getHookManager() ?? null) : null, + hooksExperimentEnabled ? (provider.getHookManager() ?? null) : null, (status) => provider.postHookStatusToWebview(status), async (type, text) => { await this.say(type as ClineSay, text) }, + // Getter for global hooksEnabled state - checks both experiment and user setting + () => { + const experimentEnabled = experiments.isEnabled( + provider.contextProxy.getValue("experiments") ?? {}, + EXPERIMENT_IDS.HOOKS, + ) + // Default to true if hooksEnabled is undefined (backwards compatibility) + const userEnabled = provider.contextProxy.getValue("hooksEnabled") ?? true + return experimentEnabled && userEnabled + }, ) this.diffEnabled = enableDiff diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index bdddb9618d..786ce1fd41 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -1986,6 +1986,7 @@ export class ClineProvider fuzzyMatchThreshold, mcpEnabled, enableMcpServerCreation, + hooksEnabled, currentApiConfigName, listApiConfigMeta, pinnedApiConfigs, @@ -2130,6 +2131,7 @@ export class ClineProvider fuzzyMatchThreshold: fuzzyMatchThreshold ?? 1.0, mcpEnabled: mcpEnabled ?? true, enableMcpServerCreation: enableMcpServerCreation ?? true, + hooksEnabled: hooksEnabled ?? true, currentApiConfigName: currentApiConfigName ?? "default", listApiConfigMeta: listApiConfigMeta ?? [], pinnedApiConfigs: pinnedApiConfigs ?? {}, @@ -2450,6 +2452,7 @@ export class ClineProvider language: stateValues.language ?? formatLanguage(vscode.env.language), mcpEnabled: stateValues.mcpEnabled ?? true, enableMcpServerCreation: stateValues.enableMcpServerCreation ?? true, + hooksEnabled: stateValues.hooksEnabled ?? true, mcpServers: this.mcpHub?.getAllServers() ?? [], currentApiConfigName: stateValues.currentApiConfigName ?? "default", listApiConfigMeta: stateValues.listApiConfigMeta ?? [], diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index ff186892f2..a9305c47c6 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -561,6 +561,7 @@ describe("ClineProvider", () => { fuzzyMatchThreshold: 1.0, mcpEnabled: true, enableMcpServerCreation: false, + hooksEnabled: true, mode: defaultModeSlug, customModes: [], experiments: experimentDefault, diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 06287acaf3..056500e79a 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -3404,24 +3404,17 @@ export const webviewMessageHandler = async ( if (!experiments.isEnabled(hooksExperimentsState, EXPERIMENT_IDS.HOOKS)) { break } - // Enable or disable ALL currently known hooks. - // This mirrors MCP's "Enable MCP Servers" top-level toggle. - const hookManager = provider.getHookManager() - if (hookManager && typeof message.hooksEnabled === "boolean") { + // Enable or disable hooks globally via the master toggle. + // This is stored in global state and checked before executing any hook. + if (typeof message.hooksEnabled === "boolean") { try { - const snapshot = hookManager.getConfigSnapshot() - const allHookIds = snapshot ? Array.from(snapshot.hooksById.keys()) : [] - - for (const hookId of allHookIds) { - await hookManager.setHookEnabled(hookId, message.hooksEnabled) - } - + await updateGlobalState("hooksEnabled", message.hooksEnabled) await provider.postStateToWebview() } catch (error) { provider.log( - `Failed to set all hooks enabled: ${error instanceof Error ? error.message : String(error)}`, + `Failed to set hooks enabled: ${error instanceof Error ? error.message : String(error)}`, ) - vscode.window.showErrorMessage(`Failed to ${message.hooksEnabled ? "enable" : "disable"} all hooks`) + vscode.window.showErrorMessage(`Failed to ${message.hooksEnabled ? "enable" : "disable"} hooks`) } } break diff --git a/src/services/hooks/ToolExecutionHooks.ts b/src/services/hooks/ToolExecutionHooks.ts index ab1ee8da23..2c0113434f 100644 --- a/src/services/hooks/ToolExecutionHooks.ts +++ b/src/services/hooks/ToolExecutionHooks.ts @@ -75,6 +75,11 @@ export type HookStatusCallback = (status: { */ export type SayCallback = (type: string, text?: string) => Promise +/** + * Callback to check if hooks are globally enabled. + */ +export type HooksEnabledGetter = () => boolean + /** * Tool Execution Hooks Service * @@ -84,11 +89,18 @@ export class ToolExecutionHooks { private hookManager: IHookManager | null private statusCallback?: HookStatusCallback private sayCallback?: SayCallback + private hooksEnabledGetter?: HooksEnabledGetter - constructor(hookManager: IHookManager | null, statusCallback?: HookStatusCallback, sayCallback?: SayCallback) { + constructor( + hookManager: IHookManager | null, + statusCallback?: HookStatusCallback, + sayCallback?: SayCallback, + hooksEnabledGetter?: HooksEnabledGetter, + ) { this.hookManager = hookManager this.statusCallback = statusCallback this.sayCallback = sayCallback + this.hooksEnabledGetter = hooksEnabledGetter } /** @@ -112,13 +124,32 @@ export class ToolExecutionHooks { this.sayCallback = callback } + /** + * Update the hooks enabled getter. + */ + setHooksEnabledGetter(getter: HooksEnabledGetter | undefined): void { + this.hooksEnabledGetter = getter + } + + /** + * Check if hooks are globally enabled. + * Returns true if no getter is set (backwards compatibility) or if the getter returns true. + */ + private isHooksEnabled(): boolean { + if (!this.hooksEnabledGetter) { + return true // Default to enabled for backwards compatibility + } + return this.hooksEnabledGetter() + } + /** * Execute PreToolUse hooks before a tool is executed. * * @returns Result indicating whether to proceed, and optionally modified input */ async executePreToolUse(context: ToolExecutionContext): Promise { - if (!this.hookManager) { + // Check global hooks enabled state first + if (!this.isHooksEnabled() || !this.hookManager) { // No hooks configured - proceed normally return { proceed: true, @@ -210,7 +241,8 @@ export class ToolExecutionHooks { output: unknown, duration: number, ): Promise { - if (!this.hookManager) { + // Check global hooks enabled state first + if (!this.isHooksEnabled() || !this.hookManager) { return { results: [], blocked: false, @@ -269,7 +301,8 @@ export class ToolExecutionHooks { error: string, errorMessage: string, ): Promise { - if (!this.hookManager) { + // Check global hooks enabled state first + if (!this.isHooksEnabled() || !this.hookManager) { return { results: [], blocked: false, @@ -329,7 +362,8 @@ export class ToolExecutionHooks { * @returns Result indicating whether to proceed with showing the prompt */ async executePermissionRequest(context: ToolExecutionContext): Promise { - if (!this.hookManager) { + // Check global hooks enabled state first + if (!this.isHooksEnabled() || !this.hookManager) { return { proceed: true, hookResult: { @@ -408,7 +442,7 @@ export class ToolExecutionHooks { * Check if hooks are configured and available. */ hasHooks(): boolean { - return this.hookManager !== null && this.hookManager.getConfigSnapshot() !== null + return this.isHooksEnabled() && this.hookManager !== null && this.hookManager.getConfigSnapshot() !== null } /** @@ -494,6 +528,7 @@ export function createToolExecutionHooks( hookManager: IHookManager | null, statusCallback?: HookStatusCallback, sayCallback?: SayCallback, + hooksEnabledGetter?: HooksEnabledGetter, ): ToolExecutionHooks { - return new ToolExecutionHooks(hookManager, statusCallback, sayCallback) + return new ToolExecutionHooks(hookManager, statusCallback, sayCallback, hooksEnabledGetter) } diff --git a/src/services/hooks/__tests__/ToolExecutionHooks.spec.ts b/src/services/hooks/__tests__/ToolExecutionHooks.spec.ts new file mode 100644 index 0000000000..2f73e47062 --- /dev/null +++ b/src/services/hooks/__tests__/ToolExecutionHooks.spec.ts @@ -0,0 +1,223 @@ +/** + * Tests for ToolExecutionHooks + * + * Covers: + * - Master toggle (hooksEnabled) enforcement + * - Pre/Post tool use hook execution + * - Permission request hooks + * - Backwards compatibility when no getter is provided + */ + +import { + ToolExecutionHooks, + createToolExecutionHooks, + type ToolExecutionContext, + type HookStatusCallback, + type SayCallback, + type HooksEnabledGetter, +} from "../ToolExecutionHooks" +import type { IHookManager, HooksExecutionResult, HooksConfigSnapshot } from "../types" + +describe("ToolExecutionHooks", () => { + // Mock IHookManager + const createMockHookManager = (): IHookManager => ({ + loadHooksConfig: vi.fn(), + reloadHooksConfig: vi.fn(), + getConfigSnapshot: vi.fn().mockReturnValue({} as HooksConfigSnapshot), + executeHooks: vi.fn().mockResolvedValue({ + results: [], + blocked: false, + totalDuration: 100, + } as HooksExecutionResult), + setHookEnabled: vi.fn(), + getEnabledHooks: vi.fn().mockReturnValue([]), + getHookExecutionHistory: vi.fn().mockReturnValue([]), + }) + + const createMockContext = (): ToolExecutionContext => ({ + toolName: "Write", + toolInput: { filePath: "/test/file.ts", content: "test" }, + session: { taskId: "task_1", sessionId: "session_1", mode: "code" }, + project: { directory: "/test/project", name: "test-project" }, + }) + + describe("Master toggle (hooksEnabled) enforcement", () => { + it("should not execute hooks when hooksEnabledGetter returns false", async () => { + const mockManager = createMockHookManager() + const hooksEnabledGetter: HooksEnabledGetter = () => false + + const hooks = new ToolExecutionHooks(mockManager, undefined, undefined, hooksEnabledGetter) + + const result = await hooks.executePreToolUse(createMockContext()) + + expect(result.proceed).toBe(true) + expect(result.hookResult.results).toEqual([]) + expect(result.hookResult.totalDuration).toBe(0) + expect(mockManager.executeHooks).not.toHaveBeenCalled() + }) + + it("should execute hooks when hooksEnabledGetter returns true", async () => { + const mockManager = createMockHookManager() + const hooksEnabledGetter: HooksEnabledGetter = () => true + + const hooks = new ToolExecutionHooks(mockManager, undefined, undefined, hooksEnabledGetter) + + await hooks.executePreToolUse(createMockContext()) + + expect(mockManager.executeHooks).toHaveBeenCalledWith("PreToolUse", expect.any(Object)) + }) + + it("should default to enabled when no getter is provided (backwards compatibility)", async () => { + const mockManager = createMockHookManager() + + // No hooksEnabledGetter provided + const hooks = new ToolExecutionHooks(mockManager, undefined, undefined, undefined) + + await hooks.executePreToolUse(createMockContext()) + + expect(mockManager.executeHooks).toHaveBeenCalled() + }) + + it("should not execute PostToolUse hooks when disabled", async () => { + const mockManager = createMockHookManager() + const hooksEnabledGetter: HooksEnabledGetter = () => false + + const hooks = new ToolExecutionHooks(mockManager, undefined, undefined, hooksEnabledGetter) + + const result = await hooks.executePostToolUse(createMockContext(), "output", 100) + + expect(result.results).toEqual([]) + expect(result.totalDuration).toBe(0) + expect(mockManager.executeHooks).not.toHaveBeenCalled() + }) + + it("should not execute PostToolUseFailure hooks when disabled", async () => { + const mockManager = createMockHookManager() + const hooksEnabledGetter: HooksEnabledGetter = () => false + + const hooks = new ToolExecutionHooks(mockManager, undefined, undefined, hooksEnabledGetter) + + const result = await hooks.executePostToolUseFailure(createMockContext(), "error", "error message") + + expect(result.results).toEqual([]) + expect(result.totalDuration).toBe(0) + expect(mockManager.executeHooks).not.toHaveBeenCalled() + }) + + it("should not execute PermissionRequest hooks when disabled", async () => { + const mockManager = createMockHookManager() + const hooksEnabledGetter: HooksEnabledGetter = () => false + + const hooks = new ToolExecutionHooks(mockManager, undefined, undefined, hooksEnabledGetter) + + const result = await hooks.executePermissionRequest(createMockContext()) + + expect(result.proceed).toBe(true) + expect(result.hookResult.results).toEqual([]) + expect(result.hookResult.totalDuration).toBe(0) + expect(mockManager.executeHooks).not.toHaveBeenCalled() + }) + + it("hasHooks should return false when disabled", () => { + const mockManager = createMockHookManager() + const hooksEnabledGetter: HooksEnabledGetter = () => false + + const hooks = new ToolExecutionHooks(mockManager, undefined, undefined, hooksEnabledGetter) + + expect(hooks.hasHooks()).toBe(false) + }) + + it("hasHooks should return true when enabled and manager has config", () => { + const mockManager = createMockHookManager() + const hooksEnabledGetter: HooksEnabledGetter = () => true + + const hooks = new ToolExecutionHooks(mockManager, undefined, undefined, hooksEnabledGetter) + + expect(hooks.hasHooks()).toBe(true) + }) + }) + + describe("createToolExecutionHooks factory", () => { + it("should create instance with hooksEnabledGetter", async () => { + const mockManager = createMockHookManager() + const hooksEnabledGetter: HooksEnabledGetter = () => false + + const hooks = createToolExecutionHooks(mockManager, undefined, undefined, hooksEnabledGetter) + + // Verify hooks don't execute when disabled + await hooks.executePreToolUse(createMockContext()) + expect(mockManager.executeHooks).not.toHaveBeenCalled() + }) + + it("should work without hooksEnabledGetter for backwards compatibility", async () => { + const mockManager = createMockHookManager() + + const hooks = createToolExecutionHooks(mockManager, undefined, undefined) + + await hooks.executePreToolUse(createMockContext()) + expect(mockManager.executeHooks).toHaveBeenCalled() + }) + }) + + describe("setHooksEnabledGetter", () => { + it("should allow updating the getter after construction", async () => { + const mockManager = createMockHookManager() + + const hooks = new ToolExecutionHooks(mockManager, undefined, undefined, () => true) + + // First call with enabled + await hooks.executePreToolUse(createMockContext()) + expect(mockManager.executeHooks).toHaveBeenCalledTimes(1) + + // Update getter to disabled + hooks.setHooksEnabledGetter(() => false) + + // Second call with disabled + await hooks.executePreToolUse(createMockContext()) + expect(mockManager.executeHooks).toHaveBeenCalledTimes(1) // Still 1, no new call + }) + }) + + describe("No hook manager", () => { + it("should return default results when hookManager is null", async () => { + const hooks = new ToolExecutionHooks(null, undefined, undefined, () => true) + + const result = await hooks.executePreToolUse(createMockContext()) + + expect(result.proceed).toBe(true) + expect(result.hookResult.results).toEqual([]) + }) + + it("hasHooks should return false when hookManager is null", () => { + const hooks = new ToolExecutionHooks(null, undefined, undefined, () => true) + + expect(hooks.hasHooks()).toBe(false) + }) + }) + + describe("Status callback", () => { + it("should not emit status when disabled", async () => { + const mockManager = createMockHookManager() + const statusCallback: HookStatusCallback = vi.fn() + const hooksEnabledGetter: HooksEnabledGetter = () => false + + const hooks = new ToolExecutionHooks(mockManager, statusCallback, undefined, hooksEnabledGetter) + + await hooks.executePreToolUse(createMockContext()) + + expect(statusCallback).not.toHaveBeenCalled() + }) + + it("should emit status when enabled", async () => { + const mockManager = createMockHookManager() + const statusCallback: HookStatusCallback = vi.fn() + const hooksEnabledGetter: HooksEnabledGetter = () => true + + const hooks = new ToolExecutionHooks(mockManager, statusCallback, undefined, hooksEnabledGetter) + + await hooks.executePreToolUse(createMockContext()) + + expect(statusCallback).toHaveBeenCalled() + }) + }) +}) diff --git a/webview-ui/src/components/settings/HooksSettings.tsx b/webview-ui/src/components/settings/HooksSettings.tsx index 49ba78b798..1a2e9bf8f2 100644 --- a/webview-ui/src/components/settings/HooksSettings.tsx +++ b/webview-ui/src/components/settings/HooksSettings.tsx @@ -11,9 +11,12 @@ import { Section } from "./Section" export const HooksSettings: React.FC = () => { const { t } = useAppTranslation() - const { hooks } = useExtensionState() + const { hooks, hooksEnabled } = useExtensionState() const [executionHistory, setExecutionHistory] = useState(hooks?.executionHistory || []) - const [isUpdatingAllEnabled, setIsUpdatingAllEnabled] = useState(false) + const [isUpdatingEnabled, setIsUpdatingEnabled] = useState(false) + + // Master toggle state - defaults to true if not explicitly set + const isHooksEnabled = hooksEnabled ?? true // Listen for realtime hookExecutionStatus messages useEffect(() => { @@ -65,12 +68,12 @@ export const HooksSettings: React.FC = () => { vscode.postMessage({ type: "hooksSetEnabled", hookId, hookEnabled: enabled }) }, []) - const handleToggleAllHooks = useCallback((enabled: boolean) => { - setIsUpdatingAllEnabled(true) + const handleToggleHooksEnabled = useCallback((enabled: boolean) => { + setIsUpdatingEnabled(true) vscode.postMessage({ type: "hooksSetAllEnabled", hooksEnabled: enabled }) // Optimistically clear the "updating" flag after a short delay. // The extension will send updated state via postStateToWebview(). - setTimeout(() => setIsUpdatingAllEnabled(false), 500) + setTimeout(() => setIsUpdatingEnabled(false), 500) }, []) const handleCreateNewHook = useCallback(() => { @@ -80,7 +83,6 @@ export const HooksSettings: React.FC = () => { const enabledHooks = hooks?.enabledHooks || [] const hasProjectHooks = hooks?.hasProjectHooks || false const snapshotTimestamp = hooks?.snapshotTimestamp - const allHooksEnabled = enabledHooks.length > 0 && enabledHooks.every((h) => h.enabled) return (
@@ -98,125 +100,130 @@ export const HooksSettings: React.FC = () => { {t("settings:hooks.description")}
- {/* Enable all hooks - matching MCP checkbox styling */} - {enabledHooks.length > 0 && ( -
- -

- {t("settings:hooks.enableHooksDescription")} -

-
- )} - - {/* Header */} -
-

{t("settings:hooks.configuredHooks")}

- {snapshotTimestamp && ( - - - - )} + {/* Master enable hooks toggle - always visible, similar to MCP toggle */} +
+ +

+ {t("settings:hooks.enableHooksDescription")} +

- {/* Security warning for project hooks */} - {hasProjectHooks && ( -
- -
-
{t("settings:hooks.projectHooksWarningTitle")}
-
- {t("settings:hooks.projectHooksWarningMessage")} -
+ {/* Only show the rest of the content when hooks are enabled */} + {isHooksEnabled && ( + <> + {/* Header */} +
+

{t("settings:hooks.configuredHooks")}

+ {snapshotTimestamp && ( + + + + )}
-
+ + {/* Security warning for project hooks */} + {hasProjectHooks && ( +
+ +
+
+ {t("settings:hooks.projectHooksWarningTitle")} +
+
+ {t("settings:hooks.projectHooksWarningMessage")} +
+
+
+ )} + + {/* Note about edits requiring reload */} +
+ {t("settings:hooks.reloadNote")} +
+ {t("settings:hooks.matcherNote")} +
+ {t("settings:hooks.matcherExamplesLabel")} +
    +
  • + {t("settings:hooks.matcherExamples.writeOrEdit")} +
    • +
  • + {t("settings:hooks.matcherExamples.readOnly")} +
    • +
+
+ + {/* Hooks list */} + {enabledHooks.length === 0 ? ( +
+ +

{t("settings:hooks.noHooksConfigured")}

+

{t("settings:hooks.noHooksHint")}

+
+ ) : ( +
+ {enabledHooks.map((hook) => ( + + ))} +
+ )} + + {/* Hook Activity Log */} + + + {/* Bottom Action Buttons - mirroring MCP settings order: create, global, project, refresh */} +
+ + + + + + +
+ )} - - {/* Note about edits requiring reload */} -
- {t("settings:hooks.reloadNote")} -
- {t("settings:hooks.matcherNote")} -
- {t("settings:hooks.matcherExamplesLabel")} -
    -
  • - {t("settings:hooks.matcherExamples.writeOrEdit")} -
    • -
  • - {t("settings:hooks.matcherExamples.readOnly")} -
    • -
-
- - {/* Hooks list */} - {enabledHooks.length === 0 ? ( -
- -

{t("settings:hooks.noHooksConfigured")}

-

{t("settings:hooks.noHooksHint")}

-
- ) : ( -
- {enabledHooks.map((hook) => ( - - ))} -
- )} - - {/* Hook Activity Log */} - - - {/* Bottom Action Buttons - mirroring MCP settings order: create, global, project, refresh */} -
- - - - - - -
) diff --git a/webview-ui/src/context/ExtensionStateContext.tsx b/webview-ui/src/context/ExtensionStateContext.tsx index d2ff79a8e0..8d199e1d98 100644 --- a/webview-ui/src/context/ExtensionStateContext.tsx +++ b/webview-ui/src/context/ExtensionStateContext.tsx @@ -279,6 +279,7 @@ export const ExtensionStateContextProvider: React.FC<{ children: React.ReactNode openRouterImageGenerationSelectedModel: "", includeCurrentTime: true, includeCurrentCost: true, + hooksEnabled: true, // Enable hooks by default }) const [didHydrateState, setDidHydrateState] = useState(false) diff --git a/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx b/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx index 3835c78f33..d525e65eb7 100644 --- a/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx +++ b/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx @@ -220,6 +220,7 @@ describe("mergeExtensionState", () => { featureRoomoteControlEnabled: false, isBrowserSessionActive: false, checkpointTimeout: DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, // Add the checkpoint timeout property + hooksEnabled: true, // Enable hooks by default } const prevState: ExtensionState = {