diff --git a/.changeset/hook-fail-mode.md b/.changeset/hook-fail-mode.md new file mode 100644 index 0000000000..544285b303 --- /dev/null +++ b/.changeset/hook-fail-mode.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": minor +--- + +`[[hooks]]` entries accept a new optional `fail_mode` field. The default `"open"` keeps today's behavior: a hook that crashes, times out, fails to spawn, or exits with an unexpected code resolves to allow. Setting `fail_mode = "closed"` inverts that for hooks acting as security gates: any failure to deliver a verdict blocks the operation with an explicit reason. Exit codes 0/2 and structured stdout decisions behave exactly as before, and a user interrupt still resolves to allow in both modes. When identical hook commands are deduplicated, a fail-closed entry now wins over a fail-open duplicate. diff --git a/docs/en/customization/hooks.md b/docs/en/customization/hooks.md index 1f966e3412..42603405a2 100644 --- a/docs/en/customization/hooks.md +++ b/docs/en/customization/hooks.md @@ -17,10 +17,12 @@ The script's response is determined by two things: - **Exit code**: `0` means allow, `2` means block, other non-zero values default to allow - **Standard output** (stdout): can include explanatory text -Even if the script errors or times out, the CLI **will not interrupt your work** as a result — this "allow on failure" design is called fail-open, preventing hook errors from becoming blockers. +By default, even if the script errors or times out, the CLI **will not interrupt your work** as a result — this "allow on failure" design is called fail-open, preventing hook errors from becoming blockers. + +For hooks that act as security gates, this default can be inverted per hook with `fail_mode = "closed"`: if the hook cannot deliver a verdict (spawn failure, crash, timeout, or any exit code other than `0` or `2`), the operation is **blocked** instead of allowed. A user interrupt (Esc) still resolves to allow in both modes, since the turn is being cancelled anyway. ::: warning Note -Precisely because of fail-open, Hooks are suitable for alerts and lightweight interception, but **should not be used as the sole security barrier**. For truly high-risk operations, rely on permission approvals and manual confirmation. +With the default fail-open behavior, Hooks are suitable for alerts and lightweight interception, but **should not be used as the sole security barrier**. For truly high-risk operations, use `fail_mode = "closed"` on the hook, or rely on permission approvals and manual confirmation. Note that fail-closed means a broken or slow hook script will block every matching operation until fixed — that is the point, but it makes the hook script's reliability your responsibility. ::: ## Quick Start: A Minimal Hook @@ -47,10 +49,11 @@ All hook rules are written in the `[[hooks]]` array in `~/.kimi-code/config.toml | `matcher` | `string` | No | A regular expression to filter event targets; if omitted, matches all | | `command` | `string` | Yes | The shell command to run when triggered | | `timeout` | `integer` | No | Timeout in seconds, range 1–600; defaults to 30 seconds | +| `fail_mode` | `string` | No | `"open"` (default) or `"closed"`. Controls what happens when the hook itself fails; see [How Hooks Work](#how-hooks-work) | -`[[hooks]]` only allows these four fields; extra fields will cause the config file to fail to load. +`[[hooks]]` only allows these five fields; extra fields will cause the config file to fail to load. -**When multiple rules match the same event**, all matching hooks run in parallel; multiple rules with identical `command` values run only once. +**When multiple rules match the same event**, all matching hooks run in parallel; multiple rules with identical `command` values run only once. If any of the collapsed duplicates sets `fail_mode = "closed"`, the single run is fail-closed. The working directory for hook commands is the current session's project directory. On non-Windows platforms, hook processes are placed in a separate process group; on timeout, a signal is sent first to give the process a chance to clean up, then it is forcibly terminated. @@ -76,8 +79,8 @@ After the script exits, the CLI determines the hook's intent based on the exit c | --- | --- | --- | | `0` | Normal exit, allow | Continue execution; stdout content (if any) may be appended to context | | `2` | Intentional block | Stop the current operation; stderr content (printed via `console.error`) is used as the reason for blocking | -| Other non-zero | Script error | Default allow (fail-open) | -| Timeout or crash | Script exception | Default allow (fail-open) | +| Other non-zero | Script error | Default allow (fail-open); blocks when the hook sets `fail_mode = "closed"` | +| Timeout or crash | Script exception | Default allow (fail-open); blocks when the hook sets `fail_mode = "closed"` | You can also return a JSON object via stdout to block: diff --git a/docs/en/customization/plugins.md b/docs/en/customization/plugins.md index a3cab97df6..3786eaab79 100644 --- a/docs/en/customization/plugins.md +++ b/docs/en/customization/plugins.md @@ -285,7 +285,7 @@ Plugin MCP servers start after `/reload` or in new sessions. To enable or disabl ## Hooks in Plugins -A plugin can declare hook rules in its manifest that run on lifecycle events while the plugin is enabled. Each entry uses the same fields as a [`[[hooks]]` rule in `config.toml`](./hooks.md#configuration) (`event`, `matcher`, `command`, `timeout`): +A plugin can declare hook rules in its manifest that run on lifecycle events while the plugin is enabled. Each entry uses the same fields as a [`[[hooks]]` rule in `config.toml`](./hooks.md#configuration) (`event`, `matcher`, `command`, `timeout`, `fail_mode`): ```json { diff --git a/docs/zh/customization/hooks.md b/docs/zh/customization/hooks.md index a506923b9a..10ebf3f405 100644 --- a/docs/zh/customization/hooks.md +++ b/docs/zh/customization/hooks.md @@ -17,10 +17,12 @@ Hooks（钩子）是一种自动触发机制：你预先告诉 Kimi Code CLI"每 - **退出码**（exit code，程序结束时向操作系统报告的状态数字）：`0` 表示放行，`2` 表示阻断，其他数字默认放行 - **标准输出**（stdout，就是你用 `console.log` 或 `print` 打印出来的内容）：可以附带说明文字 -即使脚本报错、超时，CLI 也**不会因此中断你的工作**——这种"出错就放行"的设计叫 fail-open（失败开放），避免 hook 异常变成绊脚石。 +默认情况下，即使脚本报错、超时，CLI 也**不会因此中断你的工作**——这种"出错就放行"的设计叫 fail-open（失败开放），避免 hook 异常变成绊脚石。 + +对于承担安全拦截职责的 hook，可以按条规则用 `fail_mode = "closed"` 反转这一默认行为：当 hook 无法给出裁决时（进程无法启动、崩溃、超时，或退出码既不是 `0` 也不是 `2`），操作会被**拦截**而不是放行。用户主动中断（Esc）在两种模式下都按放行处理，因为整轮操作本来就要取消。 ::: warning 注意 -正因为 fail-open，Hooks 适合做提醒和轻量拦截，但**不应作为唯一的安全防线**。对真正高风险的操作，仍需依赖权限审批和人工确认。 +在默认的 fail-open 行为下，Hooks 适合做提醒和轻量拦截，但**不应作为唯一的安全防线**。对真正高风险的操作，可以给 hook 配置 `fail_mode = "closed"`，或依赖权限审批和人工确认。注意 fail-closed 意味着 hook 脚本一旦损坏或变慢，所有匹配的操作都会被拦截直到修复——这正是它的意义所在，但也意味着 hook 脚本的可靠性由你负责。 ::: ## 快速上手：一个最简单的 hook @@ -47,10 +49,11 @@ command = "terminal-notifier -title Kimi -message 'Task done'" | `matcher` | `string` | 否 | 用正则表达式（一种字符串匹配语法）过滤事件目标；不填则匹配全部 | | `command` | `string` | 是 | 触发时要运行的 Shell 命令 | | `timeout` | `integer` | 否 | 超时秒数，范围 1–600；默认 30 秒 | +| `fail_mode` | `string` | 否 | `"open"`（默认）或 `"closed"`。控制 hook 自身失败时的行为，见[Hooks 是怎么工作的](#hooks-是怎么工作的) | -`[[hooks]]` 只允许这四个字段，多写会导致配置文件加载失败。 +`[[hooks]]` 只允许这五个字段，多写会导致配置文件加载失败。 -**同一事件匹配多条规则时**，所有命中的 hook 并行运行；`command` 完全相同的多条规则只运行一次。 +**同一事件匹配多条规则时**，所有命中的 hook 并行运行；`command` 完全相同的多条规则只运行一次。被合并的重复项中只要有一条配置了 `fail_mode = "closed"`，这次运行就按 fail-closed 处理。 Hook 命令的工作目录是当前会话的项目目录。非 Windows 平台上，hook 进程放在独立进程组里，超时时先发信号让它有机会善后，之后才强制终止。 @@ -76,8 +79,8 @@ Hook 命令的工作目录是当前会话的项目目录。非 Windows 平台上 | --- | --- | --- | | `0` | 正常结束，放行 | 继续执行，若标准输出（stdout）有内容可附加到上下文 | | `2` | 主动阻断 | 停止当前操作；错误输出（stderr，`console.error` 打印的内容）作为阻断原因 | -| 其他非零值 | 脚本出错 | 默认放行（fail-open） | -| 超时或崩溃 | 脚本异常 | 默认放行（fail-open） | +| 其他非零值 | 脚本出错 | 默认放行（fail-open）；hook 配置了 `fail_mode = "closed"` 时拦截 | +| 超时或崩溃 | 脚本异常 | 默认放行（fail-open）；hook 配置了 `fail_mode = "closed"` 时拦截 | 也可以通过标准输出返回一段 JSON 来阻断： diff --git a/docs/zh/customization/plugins.md b/docs/zh/customization/plugins.md index f847491151..679fdb5269 100644 --- a/docs/zh/customization/plugins.md +++ b/docs/zh/customization/plugins.md @@ -285,7 +285,7 @@ Plugin MCP servers 会在 `/reload` 后或新会话中启动。启用或禁用 ## 插件中的 Hooks -plugin 可以在其 manifest 中声明 hook 规则，在 plugin 启用期间于生命周期事件上运行。每一项使用与 [`config.toml` 中的 `[[hooks]]` 规则](./hooks.md#配置)相同的字段（`event`、`matcher`、`command`、`timeout`）： +plugin 可以在其 manifest 中声明 hook 规则，在 plugin 启用期间于生命周期事件上运行。每一项使用与 [`config.toml` 中的 `[[hooks]]` 规则](./hooks.md#配置)相同的字段（`event`、`matcher`、`command`、`timeout`、`fail_mode`）： ```json { diff --git a/packages/agent-core-v2/src/agent/externalHooks/configSection.ts b/packages/agent-core-v2/src/agent/externalHooks/configSection.ts index 594bf9daa6..9a36fb942c 100644 --- a/packages/agent-core-v2/src/agent/externalHooks/configSection.ts +++ b/packages/agent-core-v2/src/agent/externalHooks/configSection.ts @@ -23,6 +23,7 @@ export const HookDefSchema = z matcher: z.string().optional(), command: z.string().min(1), timeout: z.number().int().min(1).max(600).optional(), + failMode: z.enum(['open', 'closed']).optional(), }) .strict(); diff --git a/packages/agent-core-v2/src/agent/externalHooks/runner.ts b/packages/agent-core-v2/src/agent/externalHooks/runner.ts index 7f50380e1f..5a3d029c03 100644 --- a/packages/agent-core-v2/src/agent/externalHooks/runner.ts +++ b/packages/agent-core-v2/src/agent/externalHooks/runner.ts @@ -4,13 +4,14 @@ import { z } from 'zod'; import { type IHostProcess, IHostProcessService } from '#/os/interface/hostProcess'; -import type { HookResult } from './types'; +import type { HookFailMode, HookResult } from './types'; export interface RunHookOptions { readonly timeout: number; readonly cwd?: string; readonly env?: Record; readonly signal?: AbortSignal; + readonly failMode?: HookFailMode; } export function buildHookSpawnOptions(options: { @@ -69,7 +70,9 @@ export async function runHook( env: options.env, }); } catch (error) { - return allowResult({ stderr: errorMessage(error) }); + return failureResult(options.failMode, `spawn failed: ${errorMessage(error)}`, { + stderr: errorMessage(error), + }); } return new Promise((resolve) => { @@ -104,17 +107,28 @@ export async function runHook( void Promise.all([proc.wait(), stdoutDone, stderrDone]).then( ([code]) => { proc.dispose(); - settle(resultFromExitCode(code, stdout, stderr)); + settle(resultFromExitCode(code, stdout, stderr, options.failMode)); }, (error) => { proc.dispose(); - settle(allowResult({ stdout, stderr: stderr + errorMessage(error) })); + settle( + failureResult(options.failMode, `hook errored: ${errorMessage(error)}`, { + stdout, + stderr: stderr + errorMessage(error), + }), + ); }, ); const timeout = setTimeout(() => { killProcess(proc); - settle(allowResult({ stdout, stderr, timedOut: true })); + settle( + failureResult( + options.failMode, + `timed out after ${timeoutSeconds(options.timeout)}s`, + { stdout, stderr, timedOut: true }, + ), + ); }, timeoutMs); const onAbort = (): void => { @@ -137,7 +151,12 @@ function timeoutSeconds(timeout: number): number { return Number.isFinite(timeout) && timeout > 0 ? timeout : DEFAULT_TIMEOUT_SECONDS; } -function resultFromExitCode(exitCode: number, stdout: string, stderr: string): HookResult { +function resultFromExitCode( + exitCode: number, + stdout: string, + stderr: string, + failMode?: HookFailMode, +): HookResult { if (exitCode === 2) { const message = stderr.trim(); return { @@ -150,6 +169,10 @@ function resultFromExitCode(exitCode: number, stdout: string, stderr: string): H }; } + if (exitCode !== 0 && failMode === 'closed') { + return failureResult(failMode, `exit code ${exitCode}`, { stdout, stderr, exitCode }); + } + const structured = exitCode === 0 ? structuredOutput(stdout) : undefined; if (structured?.action === 'block') { return { @@ -205,6 +228,28 @@ function structuredOutput( } } +// Abort (user interrupt) deliberately stays allow in both modes: the turn is +// being cancelled, so nothing the hook would have gated is going to execute. +function failureResult( + failMode: HookFailMode | undefined, + detail: string, + input: { + readonly stdout?: string; + readonly stderr?: string; + readonly exitCode?: number; + readonly timedOut?: boolean; + }, +): HookResult { + if (failMode !== 'closed') return allowResult(input); + const reason = `Hook failed (fail_mode=closed): ${detail}`; + return { + action: 'block', + message: reason, + reason, + ...input, + }; +} + function allowResult(input: { readonly message?: string; readonly stdout?: string; diff --git a/packages/agent-core-v2/src/agent/externalHooks/types.ts b/packages/agent-core-v2/src/agent/externalHooks/types.ts index 62fca0a255..a609ca1a08 100644 --- a/packages/agent-core-v2/src/agent/externalHooks/types.ts +++ b/packages/agent-core-v2/src/agent/externalHooks/types.ts @@ -21,11 +21,14 @@ export const HOOK_EVENT_TYPES = [ export type HookEventType = (typeof HOOK_EVENT_TYPES)[number]; +export type HookFailMode = 'open' | 'closed'; + export interface HookDef { readonly event: HookEventType; readonly matcher?: string; readonly command: string; readonly timeout?: number; + readonly failMode?: HookFailMode; readonly cwd?: string; readonly env?: Record; } diff --git a/packages/agent-core-v2/src/app/externalHooksRunner/runner.ts b/packages/agent-core-v2/src/app/externalHooksRunner/runner.ts index 2b065e0ca0..3f3c2c118b 100644 --- a/packages/agent-core-v2/src/app/externalHooksRunner/runner.ts +++ b/packages/agent-core-v2/src/app/externalHooksRunner/runner.ts @@ -54,12 +54,22 @@ export async function runMatchedHooks( const matcherValue = matcherValueText(args.matcherValue); const cwd = args.cwd ?? ''; const matched: HookDef[] = []; - const seen = new Set(); + const seen = new Map(); for (const hook of byEvent.get(event) ?? []) { if (!matches(hook.matcher ?? '', matcherValue)) continue; const key = (hook.cwd ?? '') + '\0' + hook.command; - if (seen.has(key)) continue; - seen.add(key); + const existing = seen.get(key); + if (existing !== undefined) { + // Identical commands collapse to one run; if any collapsed entry is + // fail-closed, the surviving entry must be too, or dedup order would + // silently weaken a security gate. + const kept = matched[existing]; + if (hook.failMode === 'closed' && kept !== undefined && kept.failMode !== 'closed') { + matched[existing] = { ...kept, failMode: 'closed' }; + } + continue; + } + seen.set(key, matched.length); matched.push(hook); } if (matched.length === 0) return []; @@ -83,6 +93,7 @@ export async function runMatchedHooks( cwd: hook.cwd ?? (cwd === '' ? undefined : cwd), env: hook.env, signal: args.signal, + failMode: hook.failMode, }), ), ); diff --git a/packages/agent-core-v2/src/app/plugin/manifest.ts b/packages/agent-core-v2/src/app/plugin/manifest.ts index 7ce3e150a1..43644cd1d1 100644 --- a/packages/agent-core-v2/src/app/plugin/manifest.ts +++ b/packages/agent-core-v2/src/app/plugin/manifest.ts @@ -295,7 +295,7 @@ function readHooks( } const out: HookDefConfig[] = []; raw.forEach((entry, i) => { - const parsed = HookDefSchema.safeParse(entry); + const parsed = HookDefSchema.safeParse(normalizeHookFailMode(entry)); if (!parsed.success) { diagnostics.push({ severity: 'warn', @@ -308,6 +308,20 @@ function readHooks( return out.length === 0 ? undefined : out; } +// The same kimi.plugin.json is read by both agent cores, whose hook schemas +// canonicalize the fail-mode field differently (fail_mode in agent-core, +// failMode here). Accept either spelling (exactly one) so a manifest written +// for one core is not silently dropped by the other; an entry carrying both +// spellings falls through to the strict schema and is rejected like any other +// unrecognized-key entry. +function normalizeHookFailMode(entry: unknown): unknown { + if (typeof entry !== 'object' || entry === null || Array.isArray(entry)) return entry; + const record = entry as Record; + if (!('fail_mode' in record) || 'failMode' in record) return entry; + const { fail_mode: failMode, ...rest } = record; + return { ...rest, failMode }; +} + async function readCommands( pluginRoot: string, raw: unknown, diff --git a/packages/agent-core-v2/test/agent/externalHooks/runner.test.ts b/packages/agent-core-v2/test/agent/externalHooks/runner.test.ts index 853ff1076f..c839acbd63 100644 --- a/packages/agent-core-v2/test/agent/externalHooks/runner.test.ts +++ b/packages/agent-core-v2/test/agent/externalHooks/runner.test.ts @@ -125,6 +125,86 @@ describe('runHook process runner', () => { }); }); +describe('runHook fail_mode=closed', () => { + it('blocks on non-zero, non-2 exit codes', async () => { + const result = await runHook( + hostProcess, + nodeCommand('process.exit(1);'), + { tool_name: 'Bash' }, + { timeout: 5, failMode: 'closed' }, + ); + + expect(result.action).toBe('block'); + expect(result.reason).toContain('fail_mode=closed'); + }); + + it('blocks with timedOut=true when the command exceeds the timeout', async () => { + const result = await runHook( + hostProcess, + nodeCommand('setTimeout(() => {}, 10000);'), + { tool_name: 'Bash' }, + { timeout: 0.05, failMode: 'closed' }, + ); + + expect(result.action).toBe('block'); + expect(result.timedOut).toBe(true); + expect(result.reason).toContain('fail_mode=closed'); + }); + + it('blocks when the hook process cannot be spawned', async () => { + const failingSpawn = { + spawn: async () => { + throw new Error('spawn boom'); + }, + } as unknown as typeof hostProcess; + + const result = await runHook( + failingSpawn, + nodeCommand('process.exit(0);'), + { tool_name: 'Bash' }, + { timeout: 5, failMode: 'closed' }, + ); + + expect(result.action).toBe('block'); + expect(result.reason).toContain('fail_mode=closed'); + }); + + it('still allows a clean exit 0', async () => { + const result = await runHook( + hostProcess, + nodeCommand('process.exit(0);'), + { tool_name: 'Bash' }, + { timeout: 5, failMode: 'closed' }, + ); + + expect(result.action).toBe('allow'); + }); + + it('blocks when the hook process dies by signal', async () => { + const result = await runHook( + hostProcess, + nodeCommand("process.kill(process.pid, 'SIGKILL');"), + { tool_name: 'Bash' }, + { timeout: 5, failMode: 'closed' }, + ); + + expect(result.action).toBe('block'); + expect(result.reason).toContain('fail_mode=closed'); + }); + + it('still blocks exit 2 with the stderr reason, not the fail-closed reason', async () => { + const result = await runHook( + hostProcess, + nodeCommand('process.stderr.write("nope"); process.exit(2);'), + { tool_name: 'Bash' }, + { timeout: 5, failMode: 'closed' }, + ); + + expect(result.action).toBe('block'); + expect(result.reason).toBe('nope'); + }); +}); + describe('buildHookSpawnOptions (Windows console-window regression)', () => { it('sets windowsHide:true so hooks do not flash a console on Windows', () => { expect(buildHookSpawnOptions({}).windowsHide).toBe(true); diff --git a/packages/agent-core-v2/test/app/externalHooksRunner/externalHooksRunner.test.ts b/packages/agent-core-v2/test/app/externalHooksRunner/externalHooksRunner.test.ts index 3b5e60b6e2..9e470c19f6 100644 --- a/packages/agent-core-v2/test/app/externalHooksRunner/externalHooksRunner.test.ts +++ b/packages/agent-core-v2/test/app/externalHooksRunner/externalHooksRunner.test.ts @@ -27,6 +27,58 @@ describe('ExternalHooksRunnerService', () => { expect(results[0]?.action).toBe('allow'); }); + it('propagates a hook fail_mode=closed so a crashing hook blocks', async () => { + const runner = makeHookRunner([ + { + event: 'PreToolUse', + matcher: 'Bash', + command: nodeCommand('process.exit(1);'), + timeout: 5, + failMode: 'closed', + }, + ]); + + const results = await runner.trigger('PreToolUse', { + matcherValue: 'Bash', + inputData: { toolName: 'Bash' }, + }); + + expect(results).toHaveLength(1); + expect(results[0]?.action).toBe('block'); + }); + + it('keeps fail_mode=closed when identical commands with different fail modes collide in dedup', async () => { + const crash = nodeCommand('process.exit(1);'); + const runner = makeHookRunner([ + { event: 'PreToolUse', matcher: 'Bash', command: crash, timeout: 5 }, + { event: 'PreToolUse', matcher: 'Bash', command: crash, timeout: 5, failMode: 'closed' }, + ]); + + const results = await runner.trigger('PreToolUse', { + matcherValue: 'Bash', + inputData: { toolName: 'Bash' }, + }); + + expect(results).toHaveLength(1); + expect(results[0]?.action).toBe('block'); + }); + + it('keeps fail_mode=closed when the closed entry comes first in dedup', async () => { + const crash = nodeCommand('process.exit(1);'); + const runner = makeHookRunner([ + { event: 'PreToolUse', matcher: 'Bash', command: crash, timeout: 5, failMode: 'closed' }, + { event: 'PreToolUse', matcher: 'Bash', command: crash, timeout: 5 }, + ]); + + const results = await runner.trigger('PreToolUse', { + matcherValue: 'Bash', + inputData: { toolName: 'Bash' }, + }); + + expect(results).toHaveLength(1); + expect(results[0]?.action).toBe('block'); + }); + it('returns no results when no hook matcher matches the matcher value', async () => { const runner = makeHookRunner([ { event: 'PreToolUse', matcher: 'Bash|Write', command: nodeCommand('process.exit(0);'), timeout: 5 }, diff --git a/packages/agent-core-v2/test/app/externalHooksRunner/integration.test.ts b/packages/agent-core-v2/test/app/externalHooksRunner/integration.test.ts index df54d14ec2..deb8a05d8e 100644 --- a/packages/agent-core-v2/test/app/externalHooksRunner/integration.test.ts +++ b/packages/agent-core-v2/test/app/externalHooksRunner/integration.test.ts @@ -703,6 +703,21 @@ describe('IExternalHooksRunnerService integration', () => { expect(hooksToToml(parsed, undefined)).toEqual(raw); }); + it('round-trips fail_mode through the externalHooks config transforms', () => { + const raw = [{ event: 'PreToolUse', matcher: 'Bash', command: 'echo ok', fail_mode: 'closed' }]; + + const parsed = (hooksFromToml(raw) as unknown[]).map((hook) => HookDefSchema.parse(hook)); + + expect(parsed[0]).toMatchObject({ failMode: 'closed' }); + expect(hooksToToml(parsed, undefined)).toEqual(raw); + }); + + it('rejects an invalid fail_mode value', () => { + const raw = [{ event: 'PreToolUse', command: 'echo ok', fail_mode: 'sometimes' }]; + const transformed = hooksFromToml(raw) as unknown[]; + expect(() => HookDefSchema.parse(transformed[0])).toThrow(); + }); + it('exposes a summary map of event name to registered hook count', async () => { const engine = makeHookRunner([ { event: 'PreToolUse', matcher: 'Bash', command: 'echo 1' }, diff --git a/packages/agent-core-v2/test/app/plugin/manifest.test.ts b/packages/agent-core-v2/test/app/plugin/manifest.test.ts index 8a05f8f2c2..69011dd9eb 100644 --- a/packages/agent-core-v2/test/app/plugin/manifest.test.ts +++ b/packages/agent-core-v2/test/app/plugin/manifest.test.ts @@ -42,6 +42,28 @@ describe('plugin manifest parser', () => { expect(result.diagnostics).toEqual([]); }); + it('accepts hook fail_mode in either spelling and normalizes to failMode', async () => { + await writeFile( + join(dir, 'kimi.plugin.json'), + JSON.stringify({ + name: 'demo', + hooks: [ + { event: 'PreToolUse', matcher: 'Bash', command: 'echo a', fail_mode: 'closed' }, + { event: 'PreToolUse', matcher: 'Write', command: 'echo b', failMode: 'closed' }, + ], + }), + 'utf8', + ); + + const result = await parseManifest(dir); + + expect(result.diagnostics).toEqual([]); + expect(result.manifest?.hooks).toEqual([ + { event: 'PreToolUse', matcher: 'Bash', command: 'echo a', failMode: 'closed' }, + { event: 'PreToolUse', matcher: 'Write', command: 'echo b', failMode: 'closed' }, + ]); + }); + it('warns on invalid hooks and command paths', async () => { await writeFile( join(dir, 'kimi.plugin.json'), diff --git a/packages/agent-core/src/config/schema.ts b/packages/agent-core/src/config/schema.ts index 06e7c9d307..2d909e3687 100644 --- a/packages/agent-core/src/config/schema.ts +++ b/packages/agent-core/src/config/schema.ts @@ -196,6 +196,9 @@ export const HookDefSchema = z matcher: z.string().optional(), command: z.string().min(1), timeout: z.number().int().min(1).max(600).optional(), + // Hook entries are TOML arrays of tables, which transformTomlData passes + // through without key camelization — so this field stays snake_case. + fail_mode: z.enum(['open', 'closed']).optional(), }) .strict(); diff --git a/packages/agent-core/src/plugin/manifest.ts b/packages/agent-core/src/plugin/manifest.ts index b0df19e38f..8197b81877 100644 --- a/packages/agent-core/src/plugin/manifest.ts +++ b/packages/agent-core/src/plugin/manifest.ts @@ -301,7 +301,7 @@ function readHooks( } const out: HookDefConfig[] = []; raw.forEach((entry, i) => { - const parsed = HookDefSchema.safeParse(entry); + const parsed = HookDefSchema.safeParse(normalizeHookFailMode(entry)); if (!parsed.success) { diagnostics.push({ severity: 'warn', @@ -314,6 +314,20 @@ function readHooks( return out.length === 0 ? undefined : out; } +// The same kimi.plugin.json is read by both agent cores, whose hook schemas +// canonicalize the fail-mode field differently (failMode in agent-core-v2, +// fail_mode here). Accept either spelling (exactly one) so a manifest written +// for one core is not silently dropped by the other; an entry carrying both +// spellings falls through to the strict schema and is rejected like any other +// unrecognized-key entry. +function normalizeHookFailMode(entry: unknown): unknown { + if (typeof entry !== 'object' || entry === null || Array.isArray(entry)) return entry; + const record = entry as Record; + if (!('failMode' in record) || 'fail_mode' in record) return entry; + const { failMode, ...rest } = record; + return { ...rest, fail_mode: failMode }; +} + async function readCommands( pluginRoot: string, raw: unknown, diff --git a/packages/agent-core/src/session/hooks/engine.ts b/packages/agent-core/src/session/hooks/engine.ts index f71491fbfc..07fab7308f 100644 --- a/packages/agent-core/src/session/hooks/engine.ts +++ b/packages/agent-core/src/session/hooks/engine.ts @@ -88,6 +88,7 @@ export class HookEngine { cwd: hook.cwd ?? (this.options.cwd === '' ? undefined : this.options.cwd), env: hook.env, signal: args.signal, + failMode: hook.fail_mode, }), ), ); @@ -97,14 +98,24 @@ export class HookEngine { } private matchingHooks(event: string, matcherValue: string): HookDef[] { - const seen = new Set(); + const seen = new Map(); const matched: HookDef[] = []; for (const hook of this.byEvent.get(event) ?? []) { if (!matches(hook.matcher ?? '', matcherValue)) continue; const key = (hook.cwd ?? '') + '\0' + hook.command; - if (seen.has(key)) continue; - seen.add(key); + const existing = seen.get(key); + if (existing !== undefined) { + // Identical commands collapse to one run; if any collapsed entry is + // fail-closed, the surviving entry must be too, or dedup order would + // silently weaken a security gate. + const kept = matched[existing]; + if (hook.fail_mode === 'closed' && kept !== undefined && kept.fail_mode !== 'closed') { + matched[existing] = { ...kept, fail_mode: 'closed' }; + } + continue; + } + seen.set(key, matched.length); matched.push(hook); } diff --git a/packages/agent-core/src/session/hooks/runner.ts b/packages/agent-core/src/session/hooks/runner.ts index d31e180d88..5dcd5cfb8b 100644 --- a/packages/agent-core/src/session/hooks/runner.ts +++ b/packages/agent-core/src/session/hooks/runner.ts @@ -2,13 +2,14 @@ import { spawn, type ChildProcessWithoutNullStreams, type SpawnOptionsWithoutStd import { z } from 'zod'; -import type { HookResult } from './types'; +import type { HookFailMode, HookResult } from './types'; export interface RunHookOptions { readonly timeout: number; readonly cwd?: string; readonly env?: Readonly>; readonly signal?: AbortSignal; + readonly failMode?: HookFailMode; } export function buildHookSpawnOptions(options: { @@ -66,7 +67,9 @@ export async function runHook( try { child = spawn(command, buildHookSpawnOptions({ cwd: options.cwd, env: options.env })); } catch (error) { - return allowResult({ stderr: errorMessage(error) }); + return failureResult(options.failMode, `spawn failed: ${errorMessage(error)}`, { + stderr: errorMessage(error), + }); } return new Promise((resolve) => { @@ -89,7 +92,13 @@ export async function runHook( const timeout = setTimeout(() => { killProcess(child); - settle(allowResult({ stdout, stderr, timedOut: true })); + settle( + failureResult( + options.failMode, + `timed out after ${timeoutSeconds(options.timeout)}s`, + { stdout, stderr, timedOut: true }, + ), + ); }, timeoutMs); const onAbort = (): void => { @@ -112,10 +121,26 @@ export async function runHook( stderr += chunk; }); child.on('error', (error) => { - settle(allowResult({ stdout, stderr: stderr + errorMessage(error) })); + settle( + failureResult(options.failMode, `hook errored: ${errorMessage(error)}`, { + stdout, + stderr: stderr + errorMessage(error), + }), + ); }); - child.on('close', (code) => { - settle(resultFromExitCode(code ?? 0, stdout, stderr)); + child.on('close', (code, signal) => { + // Signal death reports code=null; under fail-closed that is a failure + // to deliver a verdict, not the clean exit-0 the `?? 0` fallback implies. + if (code === null && options.failMode === 'closed') { + settle( + failureResult('closed', `killed by signal ${signal ?? 'unknown'}`, { + stdout, + stderr, + }), + ); + return; + } + settle(resultFromExitCode(code ?? 0, stdout, stderr, options.failMode)); }); child.stdin.on('error', () => {}); @@ -127,7 +152,12 @@ function timeoutSeconds(timeout: number): number { return Number.isFinite(timeout) && timeout > 0 ? timeout : DEFAULT_TIMEOUT_SECONDS; } -function resultFromExitCode(exitCode: number, stdout: string, stderr: string): HookResult { +function resultFromExitCode( + exitCode: number, + stdout: string, + stderr: string, + failMode?: HookFailMode, +): HookResult { if (exitCode === 2) { const message = stderr.trim(); return { @@ -140,6 +170,10 @@ function resultFromExitCode(exitCode: number, stdout: string, stderr: string): H }; } + if (exitCode !== 0 && failMode === 'closed') { + return failureResult(failMode, `exit code ${exitCode}`, { stdout, stderr, exitCode }); + } + const structured = exitCode === 0 ? structuredOutput(stdout) : undefined; if (structured?.action === 'block') { return { @@ -192,6 +226,28 @@ function structuredOutput( } } +// Abort (user interrupt) deliberately stays allow in both modes: the turn is +// being cancelled, so nothing the hook would have gated is going to execute. +function failureResult( + failMode: HookFailMode | undefined, + detail: string, + input: { + readonly stdout?: string; + readonly stderr?: string; + readonly exitCode?: number; + readonly timedOut?: boolean; + }, +): HookResult { + if (failMode !== 'closed') return allowResult(input); + const reason = `Hook failed (fail_mode=closed): ${detail}`; + return { + action: 'block', + message: reason, + reason, + ...input, + }; +} + function allowResult(input: { readonly message?: string; readonly stdout?: string; diff --git a/packages/agent-core/src/session/hooks/types.ts b/packages/agent-core/src/session/hooks/types.ts index 786296de27..5f16b0fd1b 100644 --- a/packages/agent-core/src/session/hooks/types.ts +++ b/packages/agent-core/src/session/hooks/types.ts @@ -21,11 +21,15 @@ export const HOOK_EVENT_TYPES = [ export type HookEventType = (typeof HOOK_EVENT_TYPES)[number]; +export type HookFailMode = 'open' | 'closed'; + export interface HookDef { readonly event: HookEventType; readonly matcher?: string; readonly command: string; readonly timeout?: number; + // snake_case to match the untransformed TOML config shape (see HookDefSchema) + readonly fail_mode?: HookFailMode; readonly cwd?: string; readonly env?: Readonly>; } diff --git a/packages/agent-core/test/config/configs.test.ts b/packages/agent-core/test/config/configs.test.ts index 71f7f8d003..02be425cd2 100644 --- a/packages/agent-core/test/config/configs.test.ts +++ b/packages/agent-core/test/config/configs.test.ts @@ -470,6 +470,35 @@ timeout = 5 ]); }); + it('parses hooks fail_mode from TOML and rejects invalid values', () => { + const config = parseConfigString( + ` +[[hooks]] +event = "PreToolUse" +matcher = "Shell" +command = "echo hi" +fail_mode = "closed" +`, + 'hooks.toml', + ); + + expect(config.hooks?.[0]?.fail_mode).toBe('closed'); + + expectKimiErrorCode( + () => + parseConfigString( + ` +[[hooks]] +event = "PreToolUse" +command = "echo hi" +fail_mode = "sometimes" +`, + 'hooks.toml', + ), + ErrorCodes.CONFIG_INVALID, + ); + }); + it('rejects invalid hooks config', () => { expectKimiErrorCode( () => diff --git a/packages/agent-core/test/hooks/engine.test.ts b/packages/agent-core/test/hooks/engine.test.ts index b489c38516..e21648d6ce 100644 --- a/packages/agent-core/test/hooks/engine.test.ts +++ b/packages/agent-core/test/hooks/engine.test.ts @@ -13,6 +13,7 @@ type HookDef = { matcher?: string; command: string; timeout?: number; + fail_mode?: 'open' | 'closed'; cwd?: string; env?: Readonly>; }; @@ -113,6 +114,53 @@ describe('HookEngine', () => { expect(results).toHaveLength(0); }); + it('propagates fail_mode=closed so a crashing hook blocks', async () => { + const { HookEngine } = await importEngine(); + const engine = new HookEngine([ + { + event: 'PreToolUse', + matcher: 'Shell', + command: 'exit 1', + timeout: 5, + fail_mode: 'closed', + }, + ]); + const results = await engine.trigger('PreToolUse', { + matcherValue: 'Shell', + inputData: { toolName: 'Shell' }, + }); + expect(results).toHaveLength(1); + expect(results[0]?.action).toBe('block'); + }); + + it('keeps fail_mode=closed when identical commands with different fail modes collide in dedup', async () => { + const { HookEngine } = await importEngine(); + const engine = new HookEngine([ + { event: 'PreToolUse', matcher: 'Shell', command: 'exit 1', timeout: 5 }, + { event: 'PreToolUse', matcher: 'Shell', command: 'exit 1', timeout: 5, fail_mode: 'closed' }, + ]); + const results = await engine.trigger('PreToolUse', { + matcherValue: 'Shell', + inputData: { toolName: 'Shell' }, + }); + expect(results).toHaveLength(1); + expect(results[0]?.action).toBe('block'); + }); + + it('keeps fail_mode=closed when the closed entry comes first in dedup', async () => { + const { HookEngine } = await importEngine(); + const engine = new HookEngine([ + { event: 'PreToolUse', matcher: 'Shell', command: 'exit 1', timeout: 5, fail_mode: 'closed' }, + { event: 'PreToolUse', matcher: 'Shell', command: 'exit 1', timeout: 5 }, + ]); + const results = await engine.trigger('PreToolUse', { + matcherValue: 'Shell', + inputData: { toolName: 'Shell' }, + }); + expect(results).toHaveLength(1); + expect(results[0]?.action).toBe('block'); + }); + it('maps exit code 2 to a block action', async () => { const { HookEngine } = await importEngine(); const engine = new HookEngine([ diff --git a/packages/agent-core/test/hooks/runner.test.ts b/packages/agent-core/test/hooks/runner.test.ts index becd49baba..66ccd8092c 100644 --- a/packages/agent-core/test/hooks/runner.test.ts +++ b/packages/agent-core/test/hooks/runner.test.ts @@ -17,7 +17,7 @@ interface HookResult { type RunHook = ( command: string, input: Record, - options: { timeout: number; cwd?: string }, + options: { timeout: number; cwd?: string; failMode?: 'open' | 'closed' }, ) => Promise; async function importRunHook(): Promise { @@ -107,6 +107,96 @@ describe('runHook process runner', () => { // `windowsHide:true` (mirrors KAOS' `buildLocalSpawnOptions` and the runner's // own taskkill spawn). The flag is only observable on Windows, so we assert // the spawn options builder directly. +describe('runHook fail_mode=closed', () => { + it('blocks on non-zero, non-2 exit codes', async () => { + const runHook = await importRunHook(); + const result = await runHook( + 'node -e "process.exit(1)"', + { tool_name: 'Bash' }, + { timeout: 5, failMode: 'closed' }, + ); + + expect(result.action).toBe('block'); + expect(result.reason).toContain('fail_mode=closed'); + }); + + it('blocks with timedOut=true when the command exceeds the timeout', async () => { + const runHook = await importRunHook(); + const result = await runHook( + 'node -e "setTimeout(() => {}, 10000)"', + { tool_name: 'Bash' }, + { timeout: 0.05, failMode: 'closed' }, + ); + + expect(result.action).toBe('block'); + expect(result.timedOut).toBe(true); + expect(result.reason).toContain('fail_mode=closed'); + }); + + it('still allows a clean exit 0', async () => { + const runHook = await importRunHook(); + const result = await runHook( + 'node -e "process.exit(0)"', + { tool_name: 'Bash' }, + { timeout: 5, failMode: 'closed' }, + ); + + expect(result.action).toBe('allow'); + }); + + it('blocks when the hook process cannot be spawned', async () => { + const runHook = await importRunHook(); + // An empty command makes spawn() throw synchronously, exercising the + // spawn-failure branch without stubbing child_process. + const result = await runHook('', { tool_name: 'Bash' }, { timeout: 5, failMode: 'closed' }); + + expect(result.action).toBe('block'); + expect(result.reason).toContain('fail_mode=closed'); + }); + + it('allows a spawn failure under the default fail-open mode', async () => { + const runHook = await importRunHook(); + const result = await runHook('', { tool_name: 'Bash' }, { timeout: 5 }); + + expect(result.action).toBe('allow'); + }); + + it('blocks when the hook process dies by signal', async () => { + const runHook = await importRunHook(); + const result = await runHook( + 'node -e "process.kill(process.pid, \'SIGKILL\')"', + { tool_name: 'Bash' }, + { timeout: 5, failMode: 'closed' }, + ); + + expect(result.action).toBe('block'); + expect(result.reason).toContain('fail_mode=closed'); + }); + + it('allows signal death under the default fail-open mode', async () => { + const runHook = await importRunHook(); + const result = await runHook( + 'node -e "process.kill(process.pid, \'SIGKILL\')"', + { tool_name: 'Bash' }, + { timeout: 5 }, + ); + + expect(result.action).toBe('allow'); + }); + + it('still blocks exit 2 with the stderr reason, not the fail-closed reason', async () => { + const runHook = await importRunHook(); + const result = await runHook( + "node -e \"process.stderr.write('nope');process.exit(2)\"", + { tool_name: 'Bash' }, + { timeout: 5, failMode: 'closed' }, + ); + + expect(result.action).toBe('block'); + expect(result.reason).toBe('nope'); + }); +}); + describe('buildHookSpawnOptions (Windows console-window regression)', () => { it('sets windowsHide:true so hooks do not flash a console on Windows', () => { expect(buildHookSpawnOptions({}).windowsHide).toBe(true); diff --git a/packages/agent-core/test/plugin/manifest.test.ts b/packages/agent-core/test/plugin/manifest.test.ts index d6d680bf57..f3768fc7e0 100644 --- a/packages/agent-core/test/plugin/manifest.test.ts +++ b/packages/agent-core/test/plugin/manifest.test.ts @@ -381,6 +381,24 @@ describe('parseManifest', () => { ]); }); + it('accepts hook fail_mode in either spelling and normalizes to fail_mode', async () => { + const root = await makePlugin({ + 'kimi.plugin.json': JSON.stringify({ + name: 'demo', + hooks: [ + { event: 'PreToolUse', matcher: 'Bash', command: 'echo a', fail_mode: 'closed' }, + { event: 'PreToolUse', matcher: 'Write', command: 'echo b', failMode: 'closed' }, + ], + }), + }); + const result = await parseManifest(root); + expect(result.diagnostics).toEqual([]); + expect(result.manifest?.hooks).toEqual([ + { event: 'PreToolUse', matcher: 'Bash', command: 'echo a', fail_mode: 'closed' }, + { event: 'PreToolUse', matcher: 'Write', command: 'echo b', fail_mode: 'closed' }, + ]); + }); + it('warns and skips a hook entry that is missing required fields', async () => { const root = await makePlugin({ 'kimi.plugin.json': JSON.stringify({ diff --git a/packages/migration-legacy/src/steps/config.ts b/packages/migration-legacy/src/steps/config.ts index 2cc83ac66e..43413f0fd9 100644 --- a/packages/migration-legacy/src/steps/config.ts +++ b/packages/migration-legacy/src/steps/config.ts @@ -304,7 +304,8 @@ export async function migrateConfigStep(input: ConfigStepInput): Promise