From 469d7feedd6df4b926b875d596d6b580245b4317 Mon Sep 17 00:00:00 2001 From: Toray Altas Date: Sat, 17 Jan 2026 19:47:05 -0500 Subject: [PATCH] fix(hooks): align tests and handler typing for update flow - Update hooksSetAllEnabled tests to match global hooksEnabled toggle behavior\n- Add IHookManager.updateHook to test mocks after interface change\n- Narrow hooksUpdateHook events to HookEventType via schema validation --- .../ClineProvider.hooks-dynamic-init.spec.ts | 1 + .../webviewMessageHandler.hooks.spec.ts | 70 ++++--------------- src/core/webview/webviewMessageHandler.ts | 18 ++++- .../__tests__/ToolExecutionHooks.spec.ts | 1 + 4 files changed, 31 insertions(+), 59 deletions(-) diff --git a/src/core/webview/__tests__/ClineProvider.hooks-dynamic-init.spec.ts b/src/core/webview/__tests__/ClineProvider.hooks-dynamic-init.spec.ts index 512441e612..6c25b15477 100644 --- a/src/core/webview/__tests__/ClineProvider.hooks-dynamic-init.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.hooks-dynamic-init.spec.ts @@ -102,6 +102,7 @@ const createMockHookManager = (): IHookManager => ({ totalDuration: 0, }), setHookEnabled: vi.fn().mockResolvedValue(undefined), + updateHook: vi.fn().mockResolvedValue(undefined), getHookExecutionHistory: vi.fn().mockReturnValue([]), getConfigSnapshot: vi.fn().mockReturnValue({ hooksByEvent: new Map(), diff --git a/src/core/webview/__tests__/webviewMessageHandler.hooks.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.hooks.spec.ts index 48bf065e9c..aaa0200e21 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.hooks.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.hooks.spec.ts @@ -96,6 +96,7 @@ const createMockHookManager = (): IHookManager => ({ totalDuration: 0, }), setHookEnabled: vi.fn().mockResolvedValue(undefined), + updateHook: vi.fn().mockResolvedValue(undefined), getHookExecutionHistory: vi.fn().mockReturnValue([]), getConfigSnapshot: vi.fn().mockReturnValue({ hooksByEvent: new Map(), @@ -264,45 +265,15 @@ describe("webviewMessageHandler - hooks commands", () => { }) describe("hooksSetAllEnabled", () => { - it("should call setHookEnabled for all hooks in snapshot and postStateToWebview", async () => { - const hooksById = new Map() - hooksById.set("hook-1", { - id: "hook-1", - event: "PreToolUse" as any, - matcher: ".*", - command: "echo 1", - enabled: true, - source: "global" as any, - timeout: 30, - includeConversationHistory: false, - } as any) - hooksById.set("hook-2", { - id: "hook-2", - event: "PostToolUse" as any, - matcher: ".*", - command: "echo 2", - enabled: true, - source: "project" as any, - timeout: 30, - includeConversationHistory: false, - } as any) - - vi.mocked(mockHookManager.getConfigSnapshot).mockReturnValue({ - hooksByEvent: new Map(), - hooksById, - loadedAt: new Date(), - disabledHookIds: new Set(), - hasProjectHooks: false, - } as HooksConfigSnapshot) - + it("should update global hooksEnabled and postStateToWebview", async () => { await webviewMessageHandler(mockClineProvider, { type: "hooksSetAllEnabled", hooksEnabled: false, }) - expect(mockHookManager.setHookEnabled).toHaveBeenCalledTimes(2) - expect(mockHookManager.setHookEnabled).toHaveBeenCalledWith("hook-1", false) - expect(mockHookManager.setHookEnabled).toHaveBeenCalledWith("hook-2", false) + // hooksSetAllEnabled no longer iterates hooks; it toggles global state. + expect(mockHookManager.setHookEnabled).not.toHaveBeenCalled() + expect((mockClineProvider as any).contextProxy.setValue).toHaveBeenCalledWith("hooksEnabled", false) expect(mockClineProvider.postStateToWebview).toHaveBeenCalledTimes(1) }) @@ -313,39 +284,22 @@ describe("webviewMessageHandler - hooks commands", () => { } as any) expect(mockHookManager.setHookEnabled).not.toHaveBeenCalled() + expect((mockClineProvider as any).contextProxy.setValue).not.toHaveBeenCalled() expect(mockClineProvider.postStateToWebview).not.toHaveBeenCalled() }) - it("should show error message when bulk setHookEnabled fails", async () => { - const hooksById = new Map() - hooksById.set("hook-1", { - id: "hook-1", - event: "PreToolUse" as any, - matcher: ".*", - command: "echo 1", - enabled: true, - source: "global" as any, - timeout: 30, - includeConversationHistory: false, - } as any) - - vi.mocked(mockHookManager.getConfigSnapshot).mockReturnValue({ - hooksByEvent: new Map(), - hooksById, - loadedAt: new Date(), - disabledHookIds: new Set(), - hasProjectHooks: false, - } as HooksConfigSnapshot) - - vi.mocked(mockHookManager.setHookEnabled).mockRejectedValueOnce(new Error("boom")) + it("should show error message when updating hooksEnabled global state fails", async () => { + vi.mocked((mockClineProvider as any).contextProxy.setValue).mockRejectedValueOnce(new Error("boom")) await webviewMessageHandler(mockClineProvider, { type: "hooksSetAllEnabled", hooksEnabled: true, }) - expect(mockClineProvider.log).toHaveBeenCalledWith("Failed to set all hooks enabled: boom") - expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("Failed to enable all hooks") + expect(mockHookManager.setHookEnabled).not.toHaveBeenCalled() + expect(mockClineProvider.postStateToWebview).not.toHaveBeenCalled() + expect(mockClineProvider.log).toHaveBeenCalledWith("Failed to set hooks enabled: boom") + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("Failed to enable hooks") }) }) diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index a916a757bf..56c55ada36 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -3,6 +3,11 @@ import { safeWriteText } from "../../utils/safeWriteText" import * as path from "path" import * as os from "os" import * as fs from "fs/promises" +import { + HookEventType as HookEventTypeSchema, + type HookEventType, + type HookUpdateData, +} from "../../services/hooks/types" import { getRooDirectoriesForCwd } from "../../services/roo-config/index.js" import pWaitFor from "p-wait-for" import * as vscode from "vscode" @@ -3559,7 +3564,18 @@ export const webviewMessageHandler = async ( } try { - await hookManager.updateHook(message.filePath, message.hookId, message.hookUpdates) + const hookUpdates: HookUpdateData = { + // Webview messages are not strongly typed, so validate and narrow. + events: Array.isArray(message.hookUpdates.events) + ? message.hookUpdates.events.filter( + (event): event is HookEventType => HookEventTypeSchema.safeParse(event).success, + ) + : undefined, + matcher: typeof message.hookUpdates.matcher === "string" ? message.hookUpdates.matcher : undefined, + timeout: typeof message.hookUpdates.timeout === "number" ? message.hookUpdates.timeout : undefined, + } + + await hookManager.updateHook(message.filePath, message.hookId, hookUpdates) await hookManager.reloadHooksConfig() await provider.postStateToWebview() } catch (error) { diff --git a/src/services/hooks/__tests__/ToolExecutionHooks.spec.ts b/src/services/hooks/__tests__/ToolExecutionHooks.spec.ts index 2f73e47062..9c511586b3 100644 --- a/src/services/hooks/__tests__/ToolExecutionHooks.spec.ts +++ b/src/services/hooks/__tests__/ToolExecutionHooks.spec.ts @@ -30,6 +30,7 @@ describe("ToolExecutionHooks", () => { totalDuration: 100, } as HooksExecutionResult), setHookEnabled: vi.fn(), + updateHook: vi.fn(), getEnabledHooks: vi.fn().mockReturnValue([]), getHookExecutionHistory: vi.fn().mockReturnValue([]), })