diff --git a/AGENTS.md b/AGENTS.md index 1b70c7347b..6e2c3834af 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -43,3 +43,10 @@ Prefer the narrowest test layer that proves the behavior. This follows standard - Use `apps/vscode-e2e` only when the behavior depends on the real VS Code extension host, VS Code workspace APIs, extension activation, webview/extension messaging, file watcher behavior, or a complete user workflow. - Keep e2e tests focused on high-value smoke coverage across boundaries. Avoid placing detailed protocol, parsing, storage, retry, or edge-case assertions in e2e when they can be covered reliably at a lower layer. - When fixing a regression, add the regression test at the lowest layer that would have failed for the bug. Add an e2e test only if lower-level tests cannot represent the failure mode. + +## Maintaining this file + +Keep this file for knowledge useful to almost every future agent session in this project. +Do not repeat what the codebase already shows; point to the authoritative file or command instead. +Prefer rewriting or pruning existing entries over appending new ones. +When updating this file, preserve this bar for all agents and keep entries concise. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 120000 index 0000000000..47dc3e3d86 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +AGENTS.md \ No newline at end of file diff --git a/apps/vscode-e2e/fixtures/hooks.json b/apps/vscode-e2e/fixtures/hooks.json new file mode 100644 index 0000000000..b14dbe82ff --- /dev/null +++ b/apps/vscode-e2e/fixtures/hooks.json @@ -0,0 +1,19 @@ +{ + "fixtures": [ + { + "match": { + "sequenceIndex": 0, + "userMessage": "HOOKS_PRE_TOOL_BLOCK_E2E" + }, + "response": { + "toolCalls": [ + { + "name": "read_file", + "arguments": "{\"path\":\"hooks-block-target.txt\",\"mode\":\"slice\",\"offset\":1,\"limit\":20}", + "id": "call_hooks_block_read_001" + } + ] + } + } + ] +} diff --git a/apps/vscode-e2e/src/fixtures/hooks.ts b/apps/vscode-e2e/src/fixtures/hooks.ts new file mode 100644 index 0000000000..62976becfc --- /dev/null +++ b/apps/vscode-e2e/src/fixtures/hooks.ts @@ -0,0 +1,44 @@ +import { LLMock, type ChatCompletionRequest } from "@copilotkit/aimock" + +function requestContains(req: ChatCompletionRequest, ...expected: string[]) { + const serialized = JSON.stringify(req.messages) + return expected.every((value) => serialized.includes(value)) +} + +export function addHookFixtures(mock: InstanceType) { + mock.addFixture({ + match: { + predicate: (req) => + requestContains( + req, + "HOOKS_SESSION_START_E2E", + '', + "HOOK_SESSION_CONTEXT_MARKER", + ), + }, + response: { + toolCalls: [ + { + name: "attempt_completion", + arguments: JSON.stringify({ result: "The deterministic session hook context reached the model." }), + id: "call_hooks_session_done_001", + }, + ], + }, + }) + + mock.addFixture({ + match: { + predicate: (req) => requestContains(req, "HOOKS_PRE_TOOL_BLOCK_E2E", "Pre-tool hook", "blocked read_file"), + }, + response: { + toolCalls: [ + { + name: "attempt_completion", + arguments: JSON.stringify({ result: "The deterministic pre-tool hook blocked read_file." }), + id: "call_hooks_block_done_002", + }, + ], + }, + }) +} diff --git a/apps/vscode-e2e/src/runTest.ts b/apps/vscode-e2e/src/runTest.ts index 482e73e945..9c0808ef73 100644 --- a/apps/vscode-e2e/src/runTest.ts +++ b/apps/vscode-e2e/src/runTest.ts @@ -21,6 +21,7 @@ import { addSearchFilesResultFixtures } from "./fixtures/search-files" import { addSubtaskFixtures } from "./fixtures/subtasks" import { addUseMcpToolResultFixtures } from "./fixtures/use-mcp-tool" import { addWriteToFileResultFixtures } from "./fixtures/write-to-file" +import { addHookFixtures } from "./fixtures/hooks" function getCliFlagValue(flag: string) { return process.argv.find((arg, index) => process.argv[index - 1] === flag) @@ -129,6 +130,7 @@ async function main() { addUseMcpToolResultFixtures(mock) addWriteToFileResultFixtures(mock) addDeepSeekV4Fixtures(mock) + addHookFixtures(mock) // The modes test (switch_mode → ask) triggers a second API call whose last // user message starts with directly — no diff --git a/apps/vscode-e2e/src/suite/hooks.test.ts b/apps/vscode-e2e/src/suite/hooks.test.ts new file mode 100644 index 0000000000..b5dbe3b887 --- /dev/null +++ b/apps/vscode-e2e/src/suite/hooks.test.ts @@ -0,0 +1,273 @@ +import * as assert from "assert" +import * as fs from "fs/promises" +import * as path from "path" +import * as vscode from "vscode" + +import { RooCodeEventName, type ClineMessage, type HookDefinition } from "@roo-code/types" + +import { setDefaultSuiteTimeout } from "./test-utils" +import { sleep, waitFor, waitUntilAborted, waitUntilCompleted } from "./utils" + +const FIXTURE_SOURCE = ` +const fs = require("fs") +const { spawn } = require("child_process") + +const [mode, outputPath] = process.argv.slice(2) +const invocation = JSON.parse(fs.readFileSync(process.env.ZOO_CODE_HOOK_INVOCATION_FILE, "utf8")) + +if (mode === "session") { + fs.appendFileSync(outputPath, JSON.stringify(invocation) + "\\n") + process.stdout.write("HOOK_SESSION_CONTEXT_MARKER\\n") +} else if (mode === "block") { + fs.appendFileSync(outputPath, JSON.stringify(invocation) + "\\n") + process.exit(2) +} else if (mode === "wait") { + const child = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { stdio: "ignore" }) + fs.writeFileSync(outputPath, JSON.stringify({ rootPid: process.pid, childPid: child.pid, invocation })) + setInterval(() => {}, 1000) +} +` + +function isProcessAlive(pid: number) { + try { + process.kill(pid, 0) + return true + } catch { + return false + } +} + +suite("Hooks MVP real-host smoke", function () { + setDefaultSuiteTimeout(this) + + const api = globalThis.api + const workspacePath = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath + assert.ok(workspacePath, "The E2E harness must provide a workspace") + + const fixturePath = path.join(workspacePath, "hooks-e2e-fixture.cjs") + const invocationLogPath = path.join(workspacePath, "hooks-e2e-invocations.jsonl") + const cancellationPath = path.join(workspacePath, "hooks-e2e-cancellation.json") + const blockTargetPath = path.join(workspacePath, "hooks-block-target.txt") + const nodeExecutable = process.env.npm_node_execpath ?? process.execPath + + const definition = ( + values: Pick & + Partial, "toolMatcher">> & { + mode: string + output: string + }, + ): HookDefinition => + ({ + id: values.id, + name: values.name, + enabled: true, + phase: values.phase, + executable: nodeExecutable, + argv: [fixturePath, values.mode, values.output], + ...(values.phase === "preToolUse" && { toolMatcher: values.toolMatcher }), + }) as HookDefinition + + suiteSetup(async () => { + await fs.writeFile(fixturePath, FIXTURE_SOURCE) + await fs.writeFile(blockTargetPath, "This content must not be read.") + }) + + setup(async () => { + await api.cancelCurrentTask().catch(() => undefined) + await api.clearCurrentTask().catch(() => undefined) + await api.setConfiguration({ hookDefinitions: [] }) + await Promise.all([fs.rm(invocationLogPath, { force: true }), fs.rm(cancellationPath, { force: true })]) + }) + + teardown(async () => { + await api.cancelCurrentTask().catch(() => undefined) + await api.clearCurrentTask().catch(() => undefined) + await api.setConfiguration({ hookDefinitions: [] }) + }) + + suiteTeardown(async () => { + await Promise.all([ + fs.rm(fixturePath, { force: true }), + fs.rm(invocationLogPath, { force: true }), + fs.rm(cancellationPath, { force: true }), + fs.rm(blockTargetPath, { force: true }), + ]) + }) + + test("runs sessionStart, sends output to the model, persists settings, and reopens history", async () => { + const hook = definition({ + id: "e2e-session-start", + name: "E2E session context", + phase: "sessionStart", + mode: "session", + output: invocationLogPath, + }) + await api.setConfiguration({ hookDefinitions: [hook] }) + + const messages: ClineMessage[] = [] + const onMessage = ({ message }: { message: ClineMessage }) => messages.push(message) + api.on(RooCodeEventName.Message, onMessage) + try { + const taskId = await waitUntilCompleted({ + api, + start: () => + api.startNewTask({ + configuration: { mode: "ask", autoApprovalEnabled: true }, + text: "HOOKS_SESSION_START_E2E", + }), + }) + + const hookMessage = messages.find( + (message) => + message.say === "hook" && message.hook?.hookId === hook.id && message.hook.status === "succeeded", + ) + assert.deepStrictEqual( + hookMessage?.hook && { + phase: hookMessage.hook.phase, + status: hookMessage.hook.status, + outputSummary: hookMessage.hook.outputSummary?.trim(), + }, + { phase: "sessionStart", status: "succeeded", outputSummary: "HOOK_SESSION_CONTEXT_MARKER" }, + ) + assert.ok( + messages.some( + (message) => + message.say === "completion_result" && + message.text?.includes("session hook context reached the model"), + ), + "aimock should only complete after receiving the hook context", + ) + + const invocations = (await fs.readFile(invocationLogPath, "utf8")) + .trim() + .split("\n") + .map((line) => JSON.parse(line)) + assert.strictEqual(invocations.length, 1) + assert.deepStrictEqual( + { + phase: invocations[0].phase, + taskId: invocations[0].taskId, + workspacePath: invocations[0].workspacePath, + }, + { phase: "sessionStart", taskId, workspacePath }, + ) + assert.deepStrictEqual(api.getConfiguration().hookDefinitions, [hook]) + assert.strictEqual(await api.isTaskInHistory(taskId), true) + + await api.clearCurrentTask() + await api.resumeTask(taskId) + await waitFor(() => api.getCurrentTaskStack().includes(taskId)) + await sleep(250) + assert.deepStrictEqual(api.getConfiguration().hookDefinitions, [hook]) + assert.strictEqual((await fs.readFile(invocationLogPath, "utf8")).trim().split("\n").length, 1) + } finally { + api.off(RooCodeEventName.Message, onMessage) + } + }) + + test("blocks a matching tool before execution and reports the decision to the model", async () => { + const hook = definition({ + id: "e2e-pre-tool-block", + name: "E2E read blocker", + phase: "preToolUse", + toolMatcher: ["read_file"], + mode: "block", + output: invocationLogPath, + }) + await api.setConfiguration({ hookDefinitions: [hook] }) + + const messages: ClineMessage[] = [] + const onMessage = ({ message }: { message: ClineMessage }) => messages.push(message) + api.on(RooCodeEventName.Message, onMessage) + try { + const taskId = await waitUntilCompleted({ + api, + start: () => + api.startNewTask({ + configuration: { mode: "code", autoApprovalEnabled: true, alwaysAllowReadOnly: true }, + text: "HOOKS_PRE_TOOL_BLOCK_E2E", + }), + }) + + const hookMessage = messages.find( + (message) => + message.say === "hook" && message.hook?.hookId === hook.id && message.hook.status === "blocked", + ) + assert.deepStrictEqual( + hookMessage?.hook && { + phase: hookMessage.hook.phase, + status: hookMessage.hook.status, + matchedTool: hookMessage.hook.matchedTool, + }, + { phase: "preToolUse", status: "blocked", matchedTool: "read_file" }, + ) + assert.ok( + messages.some( + (message) => + message.say === "completion_result" && message.text?.includes("hook blocked read_file"), + ), + "aimock should only complete after receiving the hook's blocking tool result", + ) + + const invocation = JSON.parse((await fs.readFile(invocationLogPath, "utf8")).trim()) + assert.deepStrictEqual( + { phase: invocation.phase, taskId: invocation.taskId, tool: invocation.tool }, + { phase: "preToolUse", taskId, tool: { name: "read_file" } }, + ) + } finally { + api.off(RooCodeEventName.Message, onMessage) + } + }) + + test("cancels a running hook and terminates its process tree", async () => { + const hook = definition({ + id: "e2e-session-cancel", + name: "E2E cancellable hook", + phase: "sessionStart", + mode: "wait", + output: cancellationPath, + }) + await api.setConfiguration({ hookDefinitions: [hook] }) + + const messages: ClineMessage[] = [] + const onMessage = ({ message }: { message: ClineMessage }) => messages.push(message) + api.on(RooCodeEventName.Message, onMessage) + try { + const taskId = await api.startNewTask({ + configuration: { mode: "ask", autoApprovalEnabled: true }, + text: "HOOKS_CANCELLATION_E2E", + }) + await waitFor( + async () => + messages.some( + (message) => + message.say === "hook" && + message.hook?.hookId === hook.id && + message.hook.status === "running", + ) && + (await fs.stat(cancellationPath).then( + () => true, + () => false, + )), + ) + + const processInfo = JSON.parse(await fs.readFile(cancellationPath, "utf8")) + const aborted = waitUntilAborted({ api, taskId }) + await api.cancelCurrentTask() + await aborted + await waitFor(() => !isProcessAlive(processInfo.rootPid) && !isProcessAlive(processInfo.childPid)) + + const hookMessage = messages.find( + (message) => + message.say === "hook" && message.hook?.hookId === hook.id && message.hook.status === "cancelled", + ) + assert.ok(hookMessage, "The persisted hook row should transition from running to cancelled") + assert.deepStrictEqual( + { phase: processInfo.invocation.phase, taskId: processInfo.invocation.taskId }, + { phase: "sessionStart", taskId }, + ) + } finally { + api.off(RooCodeEventName.Message, onMessage) + } + }) +}) diff --git a/docs/hooks.md b/docs/hooks.md new file mode 100644 index 0000000000..7b6623c927 --- /dev/null +++ b/docs/hooks.md @@ -0,0 +1,163 @@ +# Hooks + +Hooks run trusted local programs at defined task lifecycle boundaries. Configure them in the global **Hooks** settings panel. + +## Security model + +- Hooks are global user configuration. Zoo Code does not discover or execute hook definitions from a repository. +- Enabled hooks execute on the VS Code extension host. In a remote window, container, Codespace, SSH session, or WSL session, that normally means the remote extension host, not the computer displaying the UI. +- Every run uses the task's captured file-system workspace as its current working directory. A missing directory, a relative path, or an unsupported non-file workspace fails safely. +- Zoo Code starts the configured executable directly with the configured argument array. It does not concatenate a shell command, invoke a shell, or interpolate tool input into arguments. +- Standard input is closed. Interactive commands are unsupported. +- The timeout is fixed at 10 seconds. It is not configurable per hook. Timeout or task cancellation kills the process tree and waits for settlement. +- Hook definitions are snapshotted when a task instance is created, even if no `sessionStart` hook is configured. Settings edits affect the next task instance, not a running task. +- Only the phase metadata, task identity, workspace path, and canonical tool name are available through the temporary invocation file named by `ZOO_CODE_HOOK_INVOCATION_FILE`. Raw tool arguments are not exposed. +- Treat enabled executables and scripts as trusted code. They inherit the extension-host process environment and can access anything that host account can access. + +Project-controlled hooks are intentionally deferred. Automatically executing repository configuration requires a separate workspace-trust, symlink, ownership, and change-approval design. + +## Phases and ordering + +Zoo Code supports two phases: + +- `sessionStart` runs once for each new or resumed task instance before its first model request. Failures are visible but do not stop the task. +- `preToolUse` runs before an eligible tool call. Matching hooks run sequentially in their configured order and stop at the first block or failure. + +For static tools, custom tools, and native MCP tools, the pre-tool boundary is: + +1. Finalize and parse the complete model tool call. +2. Apply built-in tool, mode, argument, MCP existence, MCP server authorization, and repetition checks as applicable. +3. Run matching `preToolUse` hooks in configured order. +4. If allowed, continue through the existing approval, checkpoint, and execution path. +5. If blocked, publish exactly one result for the model's requested tool and skip approval, checkpoint creation, and execution. + +Incomplete, invalid, unauthorized, and repetition-blocked calls do not run hooks. Existing authorization always wins; a hook cannot grant access that built-in policy denied. + +```mermaid +sequenceDiagram + autonumber + participant M as Model + participant D as Tool dispatcher + participant T as Task + participant H as HookRunner + participant P as Extension-host process + participant A as Approval / checkpoint / tool + + M->>D: complete requested tool call + D->>D: parse and built-in validation + D->>D: mode/MCP authorization + D->>D: repetition check + alt invalid, unauthorized, repeated, or incomplete + D-->>M: one requested-tool result + else eligible + D->>T: runPreToolUseHooks(canonical name) + loop exact matches in configured order + T->>H: run(snapshot definition, task signal) + H->>P: executable + direct argv, cwd, no stdin + P-->>H: bounded stdout/stderr + exit + H-->>T: typed outcome + end + alt every hook exits 0 + T-->>D: allow + bounded stdout context + D->>A: normal approval/checkpoint/execution + A-->>M: one requested-tool result + hook context + else exit 2, timeout, spawn failure, or other nonzero + T-->>D: block + prior successful context + D-->>M: one requested-tool error result + end + end +``` + +## Matching + +Matching is exact and case-sensitive. There are no regular expressions, globs, prefixes, or an implicit all-tools matcher. + +- Static tools use their canonical names, such as `read_file`, `execute_command`, or `use_mcp_tool`. The settings UI lists every supported static dispatch name. +- Custom tools use the exact registered custom tool name, such as `company_lookup`. +- Native MCP tools use the exact canonical name sent in the native tool call, such as `mcp_local_server_search-docs`. This is distinct from the static `use_mcp_tool` wrapper. + +Dynamic custom and native MCP names can be entered as comma-separated exact names. A broad matcher such as `mcp_*` is a literal name and does not match other MCP tools. + +## Exit and output contract + +| Result | `sessionStart` | `preToolUse` | +| ------------------- | ------------------------------------- | --------------------------------- | +| Exit `0` | Continue and expose non-empty stdout | Allow and expose non-empty stdout | +| Exit `2` | Record failure and continue | Intentionally block | +| Other nonzero | Record failure and continue | Fail closed | +| Spawn/start failure | Record failure and continue | Fail closed | +| Timeout | Record timeout and continue | Fail closed | +| Cancellation | Stop and discard late model/UI writes | Stop tool dispatch | + +Stdout is arbitrary text; it is not parsed as JSON. "Invalid output" means output or process-start behavior that cannot be safely represented by the runner contract, not JSON parse failure. Terminal escapes and unsafe control characters are removed. + +Combined stdout and stderr capture is capped at 64 KiB. Persisted/model summaries are capped at 16 KiB and include a clear truncation marker. Only successful, non-empty stdout becomes model-visible context, delimited as ordinary text: + +```text + +bounded, sanitized stdout + +``` + +This text is added to the pending user turn beside the legitimate requested-tool result. It is not a system instruction, environment-details block, or fabricated `tool_result`. Stderr and failed/blocked stdout are not sent to the model. + +## Components and persistence + +```mermaid +flowchart LR + UI[Global Hooks settings] -->|validated definitions| CP[ContextProxy] + CP -->|snapshot at construction| T[Task instance] + T -->|sessionStart / preToolUse| HR[HookRunner] + HR -->|direct executable + argv| EP[Extension-host process] + EP -->|bounded streams + exit| HR + HR -->|typed result| T + T -->|complete hook rows| UH[UI message history] + T -->|successful stdout blocks| AH[API conversation history] + T -->|allow / block| TD[Static, custom, native MCP dispatch] + TD --> AP[Approval and checkpoint] + TD --> TR[Exactly one requested-tool result] +``` + +The chat history persists a structured hook row with the hook name, phase, status, bounded summaries, timestamps, and matched canonical tool name. A running row left by extension-host termination is shown as `interrupted` when history reloads and is not restarted. Model context is persisted independently in API conversation history and is never reconstructed from the chat row. + +Hook definitions and rows may contain sensitive local information. Commands are not shown in the normal chat row. Zoo Code does not add executable paths, arguments, output, cwd, workspace, hook names, tool input, or error bodies to hook telemetry. Hook definitions are excluded from settings import and export; configure trusted hooks separately on each extension host. + +## Examples + +### Load context + +Create an enabled `sessionStart` hook with: + +```text +Executable: /usr/bin/env +Arguments: node, /absolute/path/to/session-context.js +``` + +Write concise context to stdout and exit `0`. Write diagnostics to stderr only when needed; stderr is visible in hook details but is not model context. + +### Deterministically block a tool + +Create a `preToolUse` hook matching `execute_command` with: + +```text +Executable: /usr/bin/env +Arguments: node, /absolute/path/to/block-commands.js +``` + +The script can read the invocation file path from `ZOO_CODE_HOOK_INVOCATION_FILE`. Exit `2` to block. The invocation intentionally contains the canonical tool name but not the requested command or other raw tool input. + +## Troubleshooting + +- **`interrupted` row:** The extension host stopped while the hook was running. Start a new task or resume the task to create a new instance; interrupted hooks are never restarted automatically. +- **Workspace unavailable:** Open a file-system workspace and start a new task. Hooks reject missing, relative, and unsupported workspace locations. +- **Timeout:** Make the program deterministic and complete within 10 seconds. Interactive input and long-running daemons are unsupported. +- **Executable not found remotely:** Install it on the machine/container running the VS Code extension host and use a path valid there. +- **Hook does not match:** Compare the configured name with the exact canonical static, custom, or native MCP tool name. Patterns are not supported. +- **Settings edit has no effect:** Start a new task instance. Active tasks intentionally retain their original immutable definition snapshot. +- **Output is truncated:** Reduce output. Hooks are context providers and policy gates, not artifact or log streaming channels. + +## Compatibility and future changes + +Unknown phases and action shapes are rejected instead of guessed or silently executed. Future phases and action types, if introduced, will be additive. Post-tool rewriting, project hooks, recursive MCP/prompt actions, output artifacts/streaming, configurable timeouts, and additional lifecycle phases are not implemented by this feature. + +The deterministic VS Code end-to-end smoke configures global hooks on the real extension host, verifies session output reaches the mocked model, reopens persisted history without rerunning completed work, blocks a model-requested static tool, and confirms cancellation terminates a hook process tree. Detailed static, custom, and native MCP ordering and edge cases remain at the narrower integration layers. diff --git a/packages/types/src/__tests__/hooks.test.ts b/packages/types/src/__tests__/hooks.test.ts index a36168f2df..530d9c6658 100644 --- a/packages/types/src/__tests__/hooks.test.ts +++ b/packages/types/src/__tests__/hooks.test.ts @@ -57,7 +57,14 @@ describe("hook definition contracts", () => { it("enforces phase-specific exact tool matchers", () => { expect(hookDefinitionSchema.safeParse({ ...sessionHook, toolMatcher: ["read_file"] }).success).toBe(false) expect(hookDefinitionSchema.safeParse({ ...preToolHook, toolMatcher: [] }).success).toBe(false) - expect(hookDefinitionSchema.safeParse({ ...preToolHook, toolMatcher: ["read"] }).success).toBe(false) + expect(hookDefinitionSchema.safeParse({ ...preToolHook, toolMatcher: [""] }).success).toBe(false) + expect(hookDefinitionSchema.safeParse({ ...preToolHook, toolMatcher: ["bad\0tool"] }).success).toBe(false) + expect( + hookDefinitionSchema.safeParse({ + ...preToolHook, + toolMatcher: ["my_custom_tool", "mcp_local_server-search"], + }).success, + ).toBe(true) }) it("rejects NUL characters in executables and arguments", () => { @@ -107,6 +114,17 @@ describe("hook matching", () => { expect(getMatchingHooks(definitions, "preToolUse", "read_file")).toEqual([later, preToolHook]) expect(getMatchingHooks(definitions, "preToolUse", "write_to_file")).toEqual([]) }) + + it("matches dynamic custom and native MCP names exactly without patterns", () => { + const dynamic = { + ...preToolHook, + toolMatcher: ["my_custom_tool", "mcp_local_search"], + } + + expect(getMatchingHooks([dynamic], "preToolUse", "my_custom_tool")).toEqual([dynamic]) + expect(getMatchingHooks([dynamic], "preToolUse", "mcp_local_search")).toEqual([dynamic]) + expect(getMatchingHooks([dynamic], "preToolUse", "mcp_local_other")).toEqual([]) + }) }) describe("hook exit policy", () => { diff --git a/packages/types/src/hooks.ts b/packages/types/src/hooks.ts index 6f75be8817..255e8e8b01 100644 --- a/packages/types/src/hooks.ts +++ b/packages/types/src/hooks.ts @@ -1,6 +1,6 @@ import { z } from "zod" -import { toolNamesSchema, type ToolName } from "./tool.js" +import { toolNames } from "./tool.js" export const HOOK_TIMEOUT_MS = 10_000 export const HOOK_CAPTURE_MAX_BYTES = 64 * 1024 @@ -22,6 +22,15 @@ const nulFreeString = (max: number, field: string) => .max(max) .refine((value) => !value.includes("\0"), `${field} must not contain NUL characters`) +export const hookToolNameSchema = z + .string() + .trim() + .min(1) + .max(256) + .refine((value) => !value.includes("\0"), "Tool name must not contain NUL characters") +export type HookToolName = z.infer +export const hookStaticToolNames = toolNames.filter((name) => name !== "custom_tool") + const hookDefinitionBaseSchema = z.object({ id: z.string().min(1).max(64), name: z.string().trim().min(1).max(80), @@ -42,7 +51,7 @@ export const sessionStartHookDefinitionSchema = hookDefinitionBaseSchema.extend( export const preToolUseHookDefinitionSchema = hookDefinitionBaseSchema.extend({ phase: z.literal("preToolUse"), - toolMatcher: z.array(toolNamesSchema).min(1).max(MAX_HOOK_TOOL_MATCHERS), + toolMatcher: z.array(hookToolNameSchema).min(1).max(MAX_HOOK_TOOL_MATCHERS), }) export const hookDefinitionSchema = z.discriminatedUnion("phase", [ @@ -96,7 +105,7 @@ export const hookInvocationSchema = z.discriminatedUnion("phase", [ taskId: z.string().min(1).max(128), instanceId: z.string().min(1).max(128), workspacePath: z.string().min(1).max(4096), - tool: z.object({ name: toolNamesSchema }), + tool: z.object({ name: hookToolNameSchema }), }), ]) export type HookInvocation = z.infer @@ -128,7 +137,7 @@ export const hookMessageSchema = z.object({ name: z.string().min(1).max(80), phase: hookPhaseSchema, status: hookMessageStatusSchema, - matchedTool: toolNamesSchema.optional(), + matchedTool: hookToolNameSchema.optional(), outputSummary: z.string().optional(), errorSummary: z.string().optional(), truncated: z.boolean().optional(), @@ -138,8 +147,8 @@ export const hookMessageSchema = z.object({ export type HookMessage = z.infer export function hookMatches(definition: HookDefinition, phase: "sessionStart"): boolean -export function hookMatches(definition: HookDefinition, phase: "preToolUse", toolName: ToolName): boolean -export function hookMatches(definition: HookDefinition, phase: HookPhase, toolName?: ToolName): boolean { +export function hookMatches(definition: HookDefinition, phase: "preToolUse", toolName: HookToolName): boolean +export function hookMatches(definition: HookDefinition, phase: HookPhase, toolName?: HookToolName): boolean { if (definition.phase !== phase) { return false } @@ -154,17 +163,17 @@ export function getMatchingHooks(definitions: readonly HookDefinition[], phase: export function getMatchingHooks( definitions: readonly HookDefinition[], phase: "preToolUse", - toolName: ToolName, + toolName: HookToolName, ): HookDefinition[] export function getMatchingHooks( definitions: readonly HookDefinition[], phase: HookPhase, - toolName?: ToolName, + toolName?: HookToolName, ): HookDefinition[] { return definitions.filter((definition) => definition.enabled && hookMatchesDefinition(definition, phase, toolName)) } -function hookMatchesDefinition(definition: HookDefinition, phase: HookPhase, toolName?: ToolName): boolean { +function hookMatchesDefinition(definition: HookDefinition, phase: HookPhase, toolName?: HookToolName): boolean { if (phase === "sessionStart") { return hookMatches(definition, phase) } diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts index 7838553835..47ef4e4652 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts @@ -1,8 +1,11 @@ // npx vitest src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.ts import { describe, it, expect, beforeEach, vi } from "vitest" +import { parametersSchema as z, type CustomToolDefinition } from "@roo-code/types" import { presentAssistantMessage } from "../presentAssistantMessage" import { validateToolUse } from "../../tools/validateToolUse" +import { readFileTool } from "../../tools/ReadFileTool" +import { writeToFileTool } from "../../tools/WriteToFileTool" // Mock dependencies vi.mock("../../task/Task") @@ -23,6 +26,17 @@ vi.mock("@roo-code/core", () => ({ }, })) +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + instance: { + captureToolUsage: vi.fn(), + captureConsecutiveMistakeError: vi.fn(), + captureException: vi.fn(), + }, + }, +})) + +import { TelemetryService } from "@roo-code/telemetry" import { customToolRegistry } from "@roo-code/core" describe("presentAssistantMessage - Custom Tool Recording", () => { @@ -31,6 +45,8 @@ describe("presentAssistantMessage - Custom Tool Recording", () => { beforeEach(() => { // Reset all mocks vi.clearAllMocks() + vi.mocked(customToolRegistry.has).mockReturnValue(false) + vi.mocked(customToolRegistry.get).mockReturnValue(undefined) // Create a mock Task with minimal properties needed for testing mockTask = { @@ -52,6 +68,7 @@ describe("presentAssistantMessage - Custom Tool Recording", () => { }, recordToolUsage: vi.fn(), recordToolError: vi.fn(), + runPreToolUseHooks: vi.fn().mockResolvedValue({ decision: "allow", context: [] }), toolRepetitionDetector: { check: vi.fn().mockReturnValue({ allowExecution: true }), }, @@ -111,6 +128,187 @@ describe("presentAssistantMessage - Custom Tool Recording", () => { }) }) + describe("preToolUse dispatch", () => { + it("runs a static hook after repetition and before approval/execution", async () => { + const order: string[] = [] + mockTask.assistantMessageContent = [ + { + type: "tool_use", + id: "static-hook", + name: "read_file", + params: { path: "test.txt" }, + nativeArgs: { path: "test.txt" }, + partial: false, + }, + ] + mockTask.toolRepetitionDetector.check.mockImplementation(() => { + order.push("repetition") + return { allowExecution: true } + }) + mockTask.runPreToolUseHooks.mockImplementation(async () => { + order.push("hook") + return { decision: "allow", context: [{ type: "text", text: "safe" }] } + }) + vi.spyOn(readFileTool, "handle").mockImplementationOnce(async (_task, _block, callbacks) => { + order.push("execution") + await callbacks.askApproval("tool", "read") + callbacks.pushToolResult("read result") + }) + + await presentAssistantMessage(mockTask) + + expect(order).toEqual(["repetition", "hook", "execution"]) + expect(mockTask.runPreToolUseHooks).toHaveBeenCalledWith("read_file") + expect(mockTask.userMessageContent).toEqual([ + { type: "text", text: "safe" }, + expect.objectContaining({ type: "tool_result", tool_use_id: "static-hook" }), + ]) + }) + + it("blocks a custom tool after argument validation with one real tool result", async () => { + const execute = vi.fn().mockResolvedValue("custom result") + const parameters = z.object({ secret: z.string() }) + const parse = vi.spyOn(parameters, "parse") + mockTask.assistantMessageContent = [ + { + type: "tool_use", + id: "custom-block", + name: "my_custom_tool", + nativeArgs: { secret: "not-forwarded" }, + params: {}, + partial: false, + }, + ] + vi.mocked(customToolRegistry.get).mockReturnValue({ + name: "my_custom_tool", + description: "custom", + parameters, + execute, + }) + mockTask.runPreToolUseHooks.mockResolvedValue({ + decision: "block", + context: [], + status: "blocked", + reason: "blocked by hook", + }) + + await presentAssistantMessage(mockTask) + + expect(parse).toHaveBeenCalledBefore(mockTask.runPreToolUseHooks) + expect(mockTask.runPreToolUseHooks).toHaveBeenCalledWith("my_custom_tool") + expect(mockTask.runPreToolUseHooks.mock.calls[0]).toEqual(["my_custom_tool"]) + expect(execute).not.toHaveBeenCalled() + expect(mockTask.ask).not.toHaveBeenCalled() + expect( + mockTask.userMessageContent.filter((item: { type?: string }) => item.type === "tool_result"), + ).toHaveLength(1) + }) + + it("skips approval, checkpoint, and static execution when blocked", async () => { + mockTask.assistantMessageContent = [ + { + type: "tool_use", + id: "static-block", + name: "write_to_file", + params: { path: "blocked.txt", content: "blocked" }, + nativeArgs: { path: "blocked.txt", content: "blocked" }, + partial: false, + }, + ] + mockTask.currentStreamingDidCheckpoint = false + mockTask.checkpointSave = vi.fn() + mockTask.runPreToolUseHooks.mockResolvedValue({ + decision: "block", + context: [], + status: "blocked", + reason: "blocked by hook", + }) + const execute = vi.spyOn(writeToFileTool, "handle") + + await presentAssistantMessage(mockTask) + + expect(mockTask.ask).not.toHaveBeenCalled() + expect(mockTask.checkpointSave).not.toHaveBeenCalled() + expect(execute).not.toHaveBeenCalled() + expect( + mockTask.userMessageContent.filter((item: { type?: string }) => item.type === "tool_result"), + ).toHaveLength(1) + }) + + it("does not run a hook for invalid custom arguments", async () => { + mockTask.assistantMessageContent = [ + { + type: "tool_use", + id: "custom-invalid", + name: "my_custom_tool", + nativeArgs: { invalid: true }, + params: {}, + partial: false, + }, + ] + vi.mocked(customToolRegistry.get).mockReturnValue({ + name: "my_custom_tool", + description: "custom", + parameters: z.object({ expected: z.string() }), + execute: vi.fn(), + }) + + await presentAssistantMessage(mockTask) + + expect(mockTask.runPreToolUseHooks).not.toHaveBeenCalled() + expect( + mockTask.userMessageContent.filter((item: { type?: string }) => item.type === "tool_result"), + ).toHaveLength(1) + }) + + it("does not run a hook when built-in mode validation rejects the call", async () => { + mockTask.assistantMessageContent = [ + { + type: "tool_use", + id: "static-invalid", + name: "read_file", + params: { path: "secret" }, + nativeArgs: { path: "secret" }, + partial: false, + }, + ] + vi.mocked(validateToolUse).mockImplementationOnce(() => { + throw new Error("not allowed") + }) + + await presentAssistantMessage(mockTask) + + expect(mockTask.toolRepetitionDetector.check).not.toHaveBeenCalled() + expect(mockTask.runPreToolUseHooks).not.toHaveBeenCalled() + }) + + it("does not run a hook when repetition blocks the call", async () => { + mockTask.assistantMessageContent = [ + { + type: "tool_use", + id: "static-repeated", + name: "read_file", + params: { path: "repeat" }, + nativeArgs: { path: "repeat" }, + partial: false, + }, + ] + mockTask.toolRepetitionDetector.check.mockReturnValue({ + allowExecution: false, + askUser: { messageKey: "mistake_limit_reached", messageDetail: "Repeated {toolName}" }, + }) + mockTask.apiConfiguration = { apiProvider: "test" } + mockTask.consecutiveMistakeLimit = 3 + + await presentAssistantMessage(mockTask) + + expect(mockTask.runPreToolUseHooks).not.toHaveBeenCalled() + expect( + mockTask.userMessageContent.filter((item: { type?: string }) => item.type === "tool_result"), + ).toHaveLength(1) + }) + }) + describe("Custom tool error recording", () => { it("should record custom tool error as 'custom_tool'", async () => { const toolCallId = "tool_call_custom_error_123" @@ -149,6 +347,7 @@ describe("presentAssistantMessage - Custom Tool Recording", () => { id: toolCallId, name: "read_file", params: { path: "test.txt" }, + nativeArgs: { path: "test.txt" }, partial: false, }, ] @@ -174,6 +373,11 @@ describe("presentAssistantMessage - Custom Tool Recording", () => { tool_name: "test-tool", arguments: "{}", }, + nativeArgs: { + server_name: "test-server", + tool_name: "test-tool", + arguments: {}, + }, partial: false, }, ] diff --git a/src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts b/src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts index fcf778b8f8..7f69e50227 100644 --- a/src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts +++ b/src/core/assistant-message/__tests__/presentAssistantMessage-images.spec.ts @@ -46,6 +46,8 @@ describe("presentAssistantMessage - Image Handling in Native Tool Calling", () = getModel: () => ({ id: "test-model", info: {} }), }, recordToolUsage: vi.fn(), + recordToolError: vi.fn(), + runPreToolUseHooks: vi.fn().mockResolvedValue({ decision: "allow", context: [] }), toolRepetitionDetector: { check: vi.fn().mockReturnValue({ allowExecution: true }), }, @@ -215,6 +217,84 @@ describe("presentAssistantMessage - Image Handling in Native Tool Calling", () = expect(toolResult.content).toBeTruthy() }) + describe("native MCP preToolUse dispatch", () => { + it("matches the canonical native name after MCP validation and before approval", async () => { + const callTool = vi.fn().mockResolvedValue({ content: [{ type: "text", text: "native result" }] }) + mockTask.providerRef = { + deref: () => ({ + getState: vi.fn().mockResolvedValue({ mode: "code", customModes: [] }), + getMcpHub: () => ({ + findServerNameBySanitizedName: () => "local server", + getAllServers: () => [{ name: "local server", tools: [{ name: "search-docs" }] }], + callTool, + }), + postMessageToWebview: vi.fn(), + }), + } + mockTask.assistantMessageContent = [ + { + type: "mcp_tool_use", + id: "native-mcp", + name: "mcp_local_server_search-docs", + serverName: "local_server", + toolName: "search-docs", + arguments: { query: "safe" }, + partial: false, + }, + ] + + await presentAssistantMessage(mockTask) + + expect(mockTask.toolRepetitionDetector.check).toHaveBeenCalled() + expect(mockTask.runPreToolUseHooks).toHaveBeenCalledWith("mcp_local_server_search-docs") + expect(mockTask.runPreToolUseHooks).toHaveBeenCalledBefore(mockTask.ask) + expect(callTool).toHaveBeenCalledOnce() + }) + + it("publishes one native result and skips approval/execution when blocked", async () => { + const callTool = vi.fn() + mockTask.providerRef = { + deref: () => ({ + getState: vi.fn().mockResolvedValue({ mode: "code", customModes: [] }), + getMcpHub: () => ({ + findServerNameBySanitizedName: () => "local", + getAllServers: () => [{ name: "local", tools: [{ name: "search" }] }], + callTool, + }), + postMessageToWebview: vi.fn(), + }), + } + mockTask.runPreToolUseHooks.mockResolvedValue({ + decision: "block", + context: [], + status: "timedOut", + reason: "Pre-tool hook failed closed (timedOut).", + }) + mockTask.assistantMessageContent = [ + { + type: "mcp_tool_use", + id: "native-block", + name: "mcp_local_search", + serverName: "local", + toolName: "search", + arguments: {}, + partial: false, + }, + ] + + await presentAssistantMessage(mockTask) + + expect(mockTask.ask).not.toHaveBeenCalled() + expect(callTool).not.toHaveBeenCalled() + expect( + mockTask.userMessageContent.filter((item: { type?: string }) => item.type === "tool_result"), + ).toHaveLength(1) + expect(mockTask.userMessageContent).not.toContainEqual( + expect.objectContaining({ tool_use_id: expect.stringContaining("hook") }), + ) + }) + }) + describe("Multiple tool calls handling", () => { it("should send tool_result with is_error for skipped tools in native tool calling when didRejectTool is true", async () => { // Simulate multiple tool calls with native protocol (all have IDs) diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index 12a5bfb4a2..ef9ce81fb3 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -2,7 +2,7 @@ import { serializeError } from "serialize-error" import { Anthropic } from "@anthropic-ai/sdk" import type { ToolName, ClineAsk, ToolProgressStatus } from "@roo-code/types" -import { ConsecutiveMistakeError, TelemetryEventName } from "@roo-code/types" +import { ConsecutiveMistakeError, TelemetryEventName, hookStaticToolNames } from "@roo-code/types" import { TelemetryService } from "@roo-code/telemetry" import { customToolRegistry } from "@roo-code/core" @@ -41,6 +41,64 @@ import { codebaseSearchTool } from "../tools/CodebaseSearchTool" import { formatResponse } from "../prompts/responses" import { sanitizeToolUseId } from "../../utils/tool-id" +async function checkToolRepetition( + cline: Task, + block: ToolUse, + pushToolResult: (content: ToolResponse) => void, +): Promise { + const repetitionCheck = cline.toolRepetitionDetector.check(block) + if (repetitionCheck.allowExecution || !repetitionCheck.askUser) { + return true + } + + const { response, text, images } = await cline.ask( + repetitionCheck.askUser.messageKey as ClineAsk, + repetitionCheck.askUser.messageDetail.replace("{toolName}", block.name), + ) + if (response === "messageResponse") { + cline.userMessageContent.push( + { type: "text" as const, text: `Tool repetition limit reached. User feedback: ${text}` }, + ...formatResponse.imageBlocks(images), + ) + await cline.say("user_feedback", text, images) + } + + TelemetryService.instance.captureConsecutiveMistakeError(cline.taskId) + TelemetryService.instance.captureException( + new ConsecutiveMistakeError( + `Tool repetition limit reached for ${block.name}`, + cline.taskId, + cline.consecutiveMistakeCount, + cline.consecutiveMistakeLimit, + "tool_repetition", + cline.apiConfiguration.apiProvider, + cline.api.getModel().id, + ), + ) + pushToolResult( + formatResponse.toolError( + `Tool call repetition limit reached for ${block.name}. Please try a different approach.`, + ), + ) + return false +} + +async function runPreToolHooks( + cline: Task, + toolName: string, + pushToolResult: (content: ToolResponse) => void, +): Promise { + const result = await cline.runPreToolUseHooks(toolName) + cline.userMessageContent.push(...result.context) + if (result.decision === "allow") { + return true + } + if (!cline.abort && result.status !== "cancelled") { + pushToolResult(formatResponse.toolError(result.reason)) + } + return false +} + /** * Processes and presents assistant message content to the user interface. * @@ -272,6 +330,9 @@ export async function presentAssistantMessage(cline: Task) { askApproval, handleError, pushToolResult, + beforeMcpExecution: async () => + (await checkToolRepetition(cline, syntheticToolUse, pushToolResult)) && + (await runPreToolHooks(cline, mcpBlock.name, pushToolResult)), }) break } @@ -621,54 +682,18 @@ export async function presentAssistantMessage(cline: Task) { } } - // Check for identical consecutive tool calls. - if (!block.partial) { - // Use the detector to check for repetition, passing the ToolUse - // block directly. - const repetitionCheck = cline.toolRepetitionDetector.check(block) - - // If execution is not allowed, notify user and break. - if (!repetitionCheck.allowExecution && repetitionCheck.askUser) { - // Handle repetition similar to mistake_limit_reached pattern. - const { response, text, images } = await cline.ask( - repetitionCheck.askUser.messageKey as ClineAsk, - repetitionCheck.askUser.messageDetail.replace("{toolName}", block.name), - ) - - if (response === "messageResponse") { - // Add user feedback to userContent. - cline.userMessageContent.push( - { - type: "text" as const, - text: `Tool repetition limit reached. User feedback: ${text}`, - }, - ...formatResponse.imageBlocks(images), - ) - - // Add user feedback to chat. - await cline.say("user_feedback", text, images) - } - - // Track tool repetition in telemetry via PostHog exception tracking and event. - TelemetryService.instance.captureConsecutiveMistakeError(cline.taskId) - TelemetryService.instance.captureException( - new ConsecutiveMistakeError( - `Tool repetition limit reached for ${block.name}`, - cline.taskId, - cline.consecutiveMistakeCount, - cline.consecutiveMistakeLimit, - "tool_repetition", - cline.apiConfiguration.apiProvider, - cline.api.getModel().id, - ), - ) + // MCP performs existence and server authorization inside its handler, so its + // repetition and hook boundary is deferred until those checks have passed. + if (!block.partial && block.name !== "use_mcp_tool") { + if (!(await checkToolRepetition(cline, block, pushToolResult))) { + break + } + } - // Return tool result message about the repetition - pushToolResult( - formatResponse.toolError( - `Tool call repetition limit reached for ${block.name}. Please try a different approach.`, - ), - ) + const customTool = stateExperiments?.customTools ? customToolRegistry.get(block.name) : undefined + const isStaticTool = (hookStaticToolNames as readonly string[]).includes(block.name) + if (!block.partial && block.name !== "use_mcp_tool" && !customTool && isStaticTool) { + if (!(await runPreToolHooks(cline, block.name, pushToolResult))) { break } } @@ -778,6 +803,9 @@ export async function presentAssistantMessage(cline: Task) { askApproval, handleError, pushToolResult, + beforeMcpExecution: async () => + (await checkToolRepetition(cline, block, pushToolResult)) && + (await runPreToolHooks(cline, "use_mcp_tool", pushToolResult)), }) break case "access_mcp_resource": @@ -858,8 +886,6 @@ export async function presentAssistantMessage(cline: Task) { break } - const customTool = stateExperiments?.customTools ? customToolRegistry.get(block.name) : undefined - if (customTool) { try { let customToolArgs @@ -877,6 +903,10 @@ export async function presentAssistantMessage(cline: Task) { } } + if (!(await runPreToolHooks(cline, block.name, pushToolResult))) { + break + } + const result = await customTool.execute(customToolArgs, { mode: mode ?? defaultModeSlug, task: cline, diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 76adad6d18..2e70701095 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -37,6 +37,7 @@ import { type ClineApiReqCancelReason, type ClineApiReqInfo, type HookDefinition, + type HookRunStatus, type HookMessage, type HookRunResult, RooCodeEventName, @@ -281,7 +282,7 @@ export class Task extends EventEmitter implements TaskLike { private readonly taskLifetimeAbortController = new AbortController() private readonly activeHookRuns = new Set>() private readonly activeHookRows = new Map() - private hookDefinitionsSnapshot?: HookDefinition[] + private readonly hookDefinitionsSnapshot: Promise private sessionStartHooksRun = false private readonly hookRunner = new HookRunner() skipPrevResponseIdOnce: boolean = false @@ -546,6 +547,23 @@ export class Task extends EventEmitter implements TaskLike { this.consecutiveMistakeLimit = consecutiveMistakeLimit ?? DEFAULT_CONSECUTIVE_MISTAKE_LIMIT this.providerRef = new WeakRef(provider) + const initialProviderState = + typeof provider.getState === "function" ? provider.getState() : Promise.resolve(undefined) + this.hookDefinitionsSnapshot = initialProviderState + .then((state) => { + const snapshot: HookDefinition[] = structuredClone( + state?.hookDefinitions?.filter(({ enabled }) => enabled) ?? [], + ) + for (const definition of snapshot) { + Object.freeze(definition.argv) + if (definition.phase === "preToolUse") { + Object.freeze(definition.toolMatcher) + } + Object.freeze(definition) + } + return Object.freeze(snapshot) + }) + .catch(() => Object.freeze([])) this.globalStoragePath = provider.context.globalStorageUri.fsPath this.diffViewProvider = new DiffViewProvider(this.cwd, this) this.enableCheckpoints = enableCheckpoints @@ -1230,6 +1248,11 @@ export class Task extends EventEmitter implements TaskLike { await this.updateClineMessage(message) } + private isCurrentTaskInstance(): boolean { + const currentTask = this.providerRef.deref()?.getCurrentTask() + return currentTask?.taskId === this.taskId && currentTask.instanceId === this.instanceId + } + private interruptStaleHookMessages(messages: ClineMessage[]): ClineMessage[] { const completedAt = Date.now() return messages.map((message) => @@ -1249,15 +1272,82 @@ export class Task extends EventEmitter implements TaskLike { private formatHookResultForModel(definition: HookDefinition, result: HookRunResult): string | undefined { const escapeAttribute = (value: string) => value.replaceAll("&", "&").replaceAll('"', """).replaceAll("<", "<") - const content = - result.status === "succeeded" - ? result.stdoutSummary?.trim() - : `The session start hook ${result.status}; task execution continued.` + const content = result.status === "succeeded" ? result.stdoutSummary?.trim() : undefined if (!content) { return undefined } - return `\n${content}\n` + return `\n${content}\n` + } + + async runPreToolUseHooks( + toolName: string, + ): Promise< + | { decision: "allow"; context: Anthropic.TextBlockParam[] } + | { decision: "block"; context: Anthropic.TextBlockParam[]; status: HookRunStatus; reason: string } + > { + const hooks = getMatchingHooks(await this.hookDefinitionsSnapshot, "preToolUse", toolName) + const context: Anthropic.TextBlockParam[] = [] + + for (const definition of hooks) { + if (this.taskLifetimeAbortController.signal.aborted || !this.isCurrentTaskInstance()) { + return { decision: "block", context: [], status: "cancelled", reason: "The task was cancelled." } + } + + const hookRunId = crypto.randomUUID() + const startedAt = Date.now() + await this.addHookMessage({ + hookRunId, + hookId: definition.id, + name: definition.name, + phase: "preToolUse", + status: "running", + matchedTool: toolName, + startedAt, + }) + + const execution = this.hookRunner + .run( + definition, + { + version: 1, + hookRunId, + phase: "preToolUse", + taskId: this.taskId, + instanceId: this.instanceId, + workspacePath: this.cwd, + tool: { name: toolName }, + }, + this.taskLifetimeAbortController.signal, + ) + .then(async (result) => { + await this.updateHookMessage(result) + return result + }) + this.activeHookRuns.add(execution) + + try { + const result = await execution + if (this.taskLifetimeAbortController.signal.aborted || !this.isCurrentTaskInstance()) { + return { decision: "block", context: [], status: "cancelled", reason: "The task was cancelled." } + } + const text = this.formatHookResultForModel(definition, result) + if (text) { + context.push({ type: "text", text }) + } + if (result.status !== "succeeded") { + const reason = + result.status === "blocked" + ? `Pre-tool hook "${definition.name}" blocked ${toolName}.` + : `Pre-tool hook "${definition.name}" failed closed (${result.status}).` + return { decision: "block", context, status: result.status, reason } + } + } finally { + this.activeHookRuns.delete(execution) + } + } + + return { decision: "allow", context } } private async runSessionStartHooks(): Promise { @@ -1266,10 +1356,7 @@ export class Task extends EventEmitter implements TaskLike { } this.sessionStartHooksRun = true - this.hookDefinitionsSnapshot ??= structuredClone( - (await this.providerRef.deref()?.getState())?.hookDefinitions?.filter(({ enabled }) => enabled) ?? [], - ) - const hooks = getMatchingHooks(this.hookDefinitionsSnapshot, "sessionStart") + const hooks = getMatchingHooks(await this.hookDefinitionsSnapshot, "sessionStart") const modelContent: Anthropic.TextBlockParam[] = [] for (const definition of hooks) { diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index b0a2bebe17..97f2b4846f 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -4,7 +4,7 @@ import * as os from "os" import * as path from "path" import * as vscode from "vscode" -import type { GlobalState, HookDefinition, ProviderSettings } from "@roo-code/types" +import type { GlobalState, HookDefinition, HookRunResult, ProviderSettings } from "@roo-code/types" import { TelemetryService } from "@roo-code/telemetry" import { Task } from "../Task" @@ -688,6 +688,180 @@ describe("Task persistence", () => { expect(second).toEqual([]) }) + it("captures pre-tool definitions even without session hooks and ignores later settings changes", async () => { + const definition = { + id: "pre-read", + name: "Read policy", + enabled: true, + phase: "preToolUse" as const, + toolMatcher: ["read_file"], + executable: process.execPath, + argv: [], + } + await mockHookDefinitions([definition]) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + vi.spyOn(mockProvider, "getCurrentTask").mockReturnValue(task) + await Promise.resolve() + definition.enabled = false + definition.toolMatcher[0] = "write_to_file" + const run = vi.fn().mockImplementation(async (_definition, invocation) => ({ + hookRunId: invocation.hookRunId, + hookId: "pre-read", + phase: "preToolUse", + status: "succeeded", + stdoutSummary: "read context", + truncated: false, + startedAt: 1, + completedAt: 2, + })) + task["hookRunner"].run = run + + const result = await task.runPreToolUseHooks("read_file") + + expect(run).toHaveBeenCalledOnce() + expect(result).toEqual({ + decision: "allow", + context: [ + { + type: "text", + text: '\nread context\n', + }, + ], + }) + }) + + it("runs matching pre-tool hooks sequentially and stops at the first block", async () => { + const definitions = ["first", "block", "never"].map((id) => ({ + id, + name: id, + enabled: true, + phase: "preToolUse" as const, + toolMatcher: ["my_custom_tool"], + executable: process.execPath, + argv: [], + })) + await mockHookDefinitions(definitions) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + vi.spyOn(mockProvider, "getCurrentTask").mockReturnValue(task) + const run = vi.fn().mockImplementation(async (definition, invocation) => ({ + hookRunId: invocation.hookRunId, + hookId: definition.id, + phase: "preToolUse", + status: definition.id === "block" ? "blocked" : "succeeded", + stdoutSummary: definition.id === "first" ? "first context" : "must not leak", + stderrSummary: definition.id === "block" ? "private stderr" : undefined, + truncated: false, + startedAt: 1, + completedAt: 2, + })) + task["hookRunner"].run = run + + const result = await task.runPreToolUseHooks("my_custom_tool") + + expect(run.mock.calls.map(([definition]) => definition.id)).toEqual(["first", "block"]) + expect(result).toMatchObject({ decision: "block", status: "blocked" }) + expect(JSON.stringify(result.context)).toContain("first context") + expect(JSON.stringify(result.context)).not.toContain("must not leak") + expect(JSON.stringify(result)).not.toContain("private stderr") + expect(task.clineMessages.filter((message) => message.say === "hook")).toHaveLength(2) + }) + + it("discards late pre-tool context after task-instance replacement", async () => { + const definition = { + id: "late", + name: "Late hook", + enabled: true, + phase: "preToolUse" as const, + toolMatcher: ["read_file"], + executable: process.execPath, + argv: [], + } + await mockHookDefinitions([definition]) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + vi.spyOn(mockProvider, "getCurrentTask").mockReturnValue(task) + let finish!: (result: Omit) => void + const run = vi.fn().mockImplementation( + (_definition, invocation) => + new Promise((resolve) => { + finish = (result) => resolve({ ...result, hookRunId: invocation.hookRunId }) + }), + ) + task["hookRunner"].run = run + const pending = task.runPreToolUseHooks("read_file") + await vi.waitFor(() => expect(run).toHaveBeenCalledOnce()) + vi.mocked(mockProvider.getCurrentTask).mockReturnValue({ + taskId: task.taskId, + instanceId: "replacement", + } as Task) + finish({ + hookId: "late", + phase: "preToolUse", + status: "succeeded", + stdoutSummary: "must not be appended", + truncated: false, + startedAt: 1, + completedAt: 2, + }) + + await expect(pending).resolves.toEqual({ + decision: "block", + context: [], + status: "cancelled", + reason: "The task was cancelled.", + }) + expect(task.clineMessages.at(-1)?.hook?.status).toBe("running") + }) + + it.each(["failed", "timedOut"] as const)("fails closed on %s without model stderr leakage", async (status) => { + const definition = { + id: "failure", + name: "Failure policy", + enabled: true, + phase: "preToolUse" as const, + toolMatcher: ["read_file"], + executable: process.execPath, + argv: [], + } + await mockHookDefinitions([definition]) + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + vi.spyOn(mockProvider, "getCurrentTask").mockReturnValue(task) + task["hookRunner"].run = vi.fn().mockImplementation(async (_definition, invocation) => ({ + hookRunId: invocation.hookRunId, + hookId: "failure", + phase: "preToolUse", + status, + stderrSummary: "raw private stderr", + truncated: false, + startedAt: 1, + completedAt: 2, + })) + + const result = await task.runPreToolUseHooks("read_file") + + expect(result).toMatchObject({ decision: "block", status, context: [] }) + expect(JSON.stringify(result)).not.toContain("raw private stderr") + }) + it("runs session hooks after the visible new-task row and before the first model turn", async () => { const definition = { id: "hook-1", diff --git a/src/core/tools/BaseTool.ts b/src/core/tools/BaseTool.ts index 7d574068a9..18c2d64fa0 100644 --- a/src/core/tools/BaseTool.ts +++ b/src/core/tools/BaseTool.ts @@ -11,6 +11,7 @@ export interface ToolCallbacks { handleError: HandleError pushToolResult: PushToolResult toolCallId?: string + beforeMcpExecution?: () => Promise } /** diff --git a/src/core/tools/UseMcpToolTool.ts b/src/core/tools/UseMcpToolTool.ts index da5ceb9403..b7056b659a 100644 --- a/src/core/tools/UseMcpToolTool.ts +++ b/src/core/tools/UseMcpToolTool.ts @@ -28,7 +28,7 @@ export class UseMcpToolTool extends BaseTool<"use_mcp_tool"> { readonly name = "use_mcp_tool" as const async execute(params: UseMcpToolParams, task: Task, callbacks: ToolCallbacks): Promise { - const { askApproval, handleError, pushToolResult } = callbacks + const { askApproval, beforeMcpExecution, handleError, pushToolResult } = callbacks try { // Validate parameters @@ -63,6 +63,10 @@ export class UseMcpToolTool extends BaseTool<"use_mcp_tool"> { // This handles cases where models mangle hyphens to underscores const resolvedToolName = toolValidation.resolvedToolName ?? toolName + if (beforeMcpExecution && !(await beforeMcpExecution())) { + return + } + // Reset mistake count on successful validation task.consecutiveMistakeCount = 0 diff --git a/src/core/tools/__tests__/useMcpToolTool.spec.ts b/src/core/tools/__tests__/useMcpToolTool.spec.ts index 6af93be0f4..87f4bc7618 100644 --- a/src/core/tools/__tests__/useMcpToolTool.spec.ts +++ b/src/core/tools/__tests__/useMcpToolTool.spec.ts @@ -210,6 +210,104 @@ describe("useMcpToolTool", () => { }) describe("successful execution", () => { + it("runs the pre-execution boundary after validation and skips approval when it blocks", async () => { + const block: ToolUse<"use_mcp_tool"> = { + type: "tool_use", + name: "use_mcp_tool", + params: { server_name: "test_server", tool_name: "test_tool" }, + nativeArgs: { server_name: "test_server", tool_name: "test_tool", arguments: {} }, + partial: false, + } + const callTool = vi.fn() + const beforeMcpExecution = vi.fn().mockResolvedValue(false) + mockProviderRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ mode: "code", customModes: [] }), + getMcpHub: () => ({ + getAllServers: () => [{ name: "test_server", tools: [{ name: "test_tool" }] }], + callTool, + }), + postMessageToWebview: vi.fn(), + }) + + await useMcpToolTool.handle(mockTask as Task, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + beforeMcpExecution, + }) + + expect(beforeMcpExecution).toHaveBeenCalledOnce() + expect(mockAskApproval).not.toHaveBeenCalled() + expect(callTool).not.toHaveBeenCalled() + }) + + it("does not run the pre-execution boundary for an unknown MCP tool", async () => { + const beforeMcpExecution = vi.fn() + const block: ToolUse<"use_mcp_tool"> = { + type: "tool_use", + name: "use_mcp_tool", + params: { server_name: "test_server", tool_name: "missing" }, + nativeArgs: { server_name: "test_server", tool_name: "missing", arguments: {} }, + partial: false, + } + mockProviderRef.deref.mockReturnValue({ + getMcpHub: () => ({ + getAllServers: () => [{ name: "test_server", tools: [{ name: "different_tool" }] }], + }), + postMessageToWebview: vi.fn(), + }) + + await useMcpToolTool.handle(mockTask as Task, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + beforeMcpExecution, + }) + + expect(beforeMcpExecution).not.toHaveBeenCalled() + expect(mockAskApproval).not.toHaveBeenCalled() + }) + + it("does not run the pre-execution boundary for a mode-disallowed MCP server", async () => { + const beforeMcpExecution = vi.fn() + const block: ToolUse<"use_mcp_tool"> = { + type: "tool_use", + name: "use_mcp_tool", + params: { server_name: "blocked_server", tool_name: "test_tool" }, + nativeArgs: { server_name: "blocked_server", tool_name: "test_tool", arguments: {} }, + partial: false, + } + mockProviderRef.deref.mockReturnValue({ + getState: vi.fn().mockResolvedValue({ + mode: "restricted", + customModes: [ + { + slug: "restricted", + name: "Restricted", + roleDefinition: "Restricted", + groups: ["mcp"], + allowedMcpServers: ["allowed_server"], + }, + ], + }), + getMcpHub: () => ({ + getAllServers: () => [{ name: "blocked_server", tools: [{ name: "test_tool" }] }], + }), + postMessageToWebview: vi.fn(), + }) + + await useMcpToolTool.handle(mockTask as Task, block, { + askApproval: mockAskApproval, + handleError: mockHandleError, + pushToolResult: mockPushToolResult, + beforeMcpExecution, + }) + + expect(beforeMcpExecution).not.toHaveBeenCalled() + expect(mockAskApproval).not.toHaveBeenCalled() + expect(mockPushToolResult).toHaveBeenCalledOnce() + }) + it("should execute tool successfully with valid parameters", async () => { const block: ToolUse = { type: "tool_use", diff --git a/webview-ui/src/components/settings/HooksSettings.tsx b/webview-ui/src/components/settings/HooksSettings.tsx index cae06388d6..ab4df8d4f4 100644 --- a/webview-ui/src/components/settings/HooksSettings.tsx +++ b/webview-ui/src/components/settings/HooksSettings.tsx @@ -4,9 +4,10 @@ import { Cable, Edit, Globe, Plus, Trash2, TriangleAlert, X } from "lucide-react import { hookDefinitionSchema, hookDefinitionsSchema, - toolNames, + hookStaticToolNames, type HookDefinition, type HookPhase, + type HookToolName, type ToolName, } from "@roo-code/types" @@ -45,7 +46,7 @@ type HookDraft = { name: string enabled: boolean phase: HookPhase - toolMatcher: ToolName[] + toolMatcher: HookToolName[] executable: string argv: string[] } @@ -149,6 +150,10 @@ export const HooksSettings: React.FC = ({ hookDefinitions, o })) } + const customToolMatchers = draft.toolMatcher.filter( + (tool) => !(hookStaticToolNames as readonly string[]).includes(tool), + ) + return (
@@ -284,7 +289,7 @@ export const HooksSettings: React.FC = ({ hookDefinitions, o {t("settings:hooks.fields.toolsHint")}

- {toolNames.map((tool) => ( + {hookStaticToolNames.map((tool) => ( ))}
+ { + const staticMatchers = draft.toolMatcher.filter((tool) => + (hookStaticToolNames as readonly string[]).includes(tool), + ) + const dynamicMatchers = event.target.value + .split(",") + .map((tool) => tool.trim()) + .filter(Boolean) + setDraft({ ...draft, toolMatcher: [...staticMatchers, ...dynamicMatchers] }) + }} + /> )}