diff --git a/.changeset/silent-hooks-warn.md b/.changeset/silent-hooks-warn.md new file mode 100644 index 0000000000..7f400153b9 --- /dev/null +++ b/.changeset/silent-hooks-warn.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": patch +--- + +Warn in diagnostic logs when an external hook fails to launch, times out, or exits unexpectedly. diff --git a/packages/agent-core-v2/src/app/externalHooksRunner/externalHooksRunnerService.ts b/packages/agent-core-v2/src/app/externalHooksRunner/externalHooksRunnerService.ts index 6bacea0fde..74f10b3a01 100644 --- a/packages/agent-core-v2/src/app/externalHooksRunner/externalHooksRunnerService.ts +++ b/packages/agent-core-v2/src/app/externalHooksRunner/externalHooksRunnerService.ts @@ -7,7 +7,8 @@ * `runMatchedHooks`. The App-scope `IHostProcessService` is injected here and * threaded down to `runHook`, so hook commands spawn through the shared host * process service (cross-platform kill, hidden console on Windows) rather than - * `node:child_process` directly. Per-call caller facts (`cwd` defaulting to + * `node:child_process` directly, and reports operational hook failures through + * the App-scope `ILogService`. Per-call caller facts (`cwd` defaulting to * bootstrap cwd, `sessionId`, `signal`, payload) flow in through the args, so * this service keeps no per-scope state; the one payload field it contributes * itself is `clientType` (the host platform from bootstrap client identity), @@ -15,6 +16,7 @@ */ import { Disposable } from '#/_base/di/lifecycle'; +import { ILogService } from '#/_base/log/log'; import { LifecycleScope } from '#/app/scopes'; import { ScopeActivation, registerScopedService } from '#/_base/di/scope'; import { Emitter, type Event } from '#/_base/event'; @@ -47,6 +49,7 @@ export class ExternalHooksRunnerService extends Disposable implements IExternalH @IPluginService private readonly plugins: IPluginService, @IBootstrapService private readonly bootstrap: IBootstrapService, @IHostProcessService private readonly hostProcess: IHostProcessService, + @ILogService private readonly log: ILogService, private readonly callbacks: HookRunCallbacks = {}, ) { super(); @@ -113,7 +116,19 @@ export class ExternalHooksRunnerService extends Disposable implements IExternalH ...args.inputData, }, }, - this.callbacks, + { + ...this.callbacks, + onFailed: (failedEvent, command, result) => { + this.log.warn('external hook command failed', { + event: failedEvent, + command, + exitCode: result.exitCode, + timedOut: result.timedOut, + stderr: firstStderrLine(result.stderr), + }); + this.callbacks.onFailed?.(failedEvent, command, result); + }, + }, ); } @@ -138,6 +153,11 @@ export class ExternalHooksRunnerService extends Disposable implements IExternalH } } +function firstStderrLine(stderr: string | undefined): string | undefined { + const line = stderr?.split(/\r?\n/, 1)[0]?.trim(); + return line === '' ? undefined : line; +} + registerScopedService( LifecycleScope.App, IExternalHooksRunnerService, diff --git a/packages/agent-core-v2/src/app/externalHooksRunner/runner.ts b/packages/agent-core-v2/src/app/externalHooksRunner/runner.ts index 745846065c..a33a3b927e 100644 --- a/packages/agent-core-v2/src/app/externalHooksRunner/runner.ts +++ b/packages/agent-core-v2/src/app/externalHooksRunner/runner.ts @@ -32,6 +32,7 @@ export interface HookRunCallbacks { reason: string | undefined, durationMs: number, ) => void; + readonly onFailed?: (event: string, command: string, result: HookResult) => void; } export function indexHooks(hooks: readonly HookDef[]): Map { @@ -86,6 +87,12 @@ export async function runMatchedHooks( }), ), ); + for (const [index, result] of results.entries()) { + if (!isHookFailure(result, args.signal?.aborted === true)) continue; + try { + callbacks.onFailed?.(event, matched[index]!.command, result); + } catch {} + } const decision = blockDecision(event, results); try { @@ -101,6 +108,12 @@ export async function runMatchedHooks( return results; } +function isHookFailure(result: HookResult, aborted: boolean): boolean { + if (result.timedOut === true) return true; + if (result.exitCode !== undefined) return result.exitCode !== 0 && result.exitCode !== 2; + return !aborted && (result.stderr?.trim().length ?? 0) > 0; +} + export function blockDecision( event: string, results: readonly HookResult[], diff --git a/packages/agent-core-v2/test/agent/externalHooks/runner-stub.ts b/packages/agent-core-v2/test/agent/externalHooks/runner-stub.ts index f2052b52b0..87590e7e1a 100644 --- a/packages/agent-core-v2/test/agent/externalHooks/runner-stub.ts +++ b/packages/agent-core-v2/test/agent/externalHooks/runner-stub.ts @@ -11,6 +11,7 @@ */ import { Event } from '#/_base/event'; +import type { LogPayload } from '#/_base/log/log'; import { ExternalHooksRunnerService } from '#/app/externalHooksRunner/externalHooksRunnerService'; import { HOOKS_SECTION } from '#/agent/externalHooks/configSection'; import type { HookDef } from '#/agent/externalHooks/types'; @@ -19,6 +20,8 @@ import { IConfigService } from '#/app/config/config'; import { IPluginService } from '#/app/plugin/plugin'; import { HostProcessService } from '#/os/backends/node-local/hostProcessService'; +import { stubLog } from '../../_base/log/stubs'; + export function makeHookRunner( hooks: readonly HookDef[], options: { @@ -31,6 +34,7 @@ export function makeHookRunner( reason: string | undefined, durationMs: number, ) => void; + onWarn?: (message: string, payload?: LogPayload) => void; } = {}, ): ExternalHooksRunnerService { return new ExternalHooksRunnerService( @@ -50,6 +54,7 @@ export function makeHookRunner( clientIdentity: { productName: 'test', version: '0.0.0-test', platform: 'test_platform' }, } as unknown as IBootstrapService, new HostProcessService(), + { ...stubLog(), warn: options.onWarn ?? (() => {}) }, { onTriggered: options.onTriggered, onResolved: options.onResolved }, ); } 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 86809fa587..a1bcca40a8 100644 --- a/packages/agent-core-v2/test/app/externalHooksRunner/externalHooksRunner.test.ts +++ b/packages/agent-core-v2/test/app/externalHooksRunner/externalHooksRunner.test.ts @@ -255,6 +255,45 @@ describe('ExternalHooksRunnerService', () => { await expect(runner.fireAndForgetTrigger('Notification')).resolves.toEqual([]); }); + it('warns about a non-zero hook exit without changing fail-open behavior', async () => { + const warnings: Array<[string, unknown]> = []; + const command = nodeCommand( + 'process.stderr.write(["bad hook", "second line"].join(String.fromCharCode(10))); process.exit(1);', + ); + const runner = makeHookRunner([{ event: 'UserPromptSubmit', command, timeout: 5 }], { + onWarn: (message, payload) => warnings.push([message, payload]), + }); + + const results = await runner.trigger('UserPromptSubmit'); + + expect(results[0]?.action).toBe('allow'); + expect(warnings).toEqual([ + [ + 'external hook command failed', + { + event: 'UserPromptSubmit', + command, + exitCode: 1, + timedOut: undefined, + stderr: 'bad hook', + }, + ], + ]); + }); + + it('does not warn about an intentional exit-code-2 block', async () => { + const onWarn = vi.fn(); + const runner = makeHookRunner( + [{ event: 'PreToolUse', command: nodeCommand('process.exit(2);'), timeout: 5 }], + { onWarn }, + ); + + const results = await runner.trigger('PreToolUse'); + + expect(results[0]?.action).toBe('block'); + expect(onWarn).not.toHaveBeenCalled(); + }); + it('invokes onTriggered with (event,target,count) and onResolved with (event,target,action)', async () => { const triggered: Array<[string, string, number]> = []; const resolved: Array<[string, string, string]> = []; 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 a7b5fbfd94..5b1cf92de6 100644 --- a/packages/agent-core-v2/test/app/externalHooksRunner/integration.test.ts +++ b/packages/agent-core-v2/test/app/externalHooksRunner/integration.test.ts @@ -69,6 +69,7 @@ import { stubBootstrap } from '../bootstrap/stubs'; import { stubLoopWithHooks, stubToolExecutor } from '../../agent/loop/stubs'; import { registerStateServices } from '../../state/stubs'; import { registerTestAgentWireServices } from '../../wire/stubs'; +import { registerLogServices } from '../../_base/log/stubs'; function nodeCommand(source: string): string { return `node -e ${JSON.stringify(source.replaceAll(/\s*\n\s*/g, ' '))}`; @@ -602,6 +603,7 @@ describe('IExternalHooksRunnerService integration', () => { strict: true, additionalServices: (reg) => { registerStateServices(reg); + registerLogServices(reg); reg.defineInstance(IBootstrapService, stubBootstrap()); reg.defineInstance(ISessionContext, stubSessionContext()); reg.defineInstance(ISessionMetadata, stubSessionMetadata()); @@ -863,6 +865,7 @@ describe('IExternalHooksRunnerService integration', () => { strict: true, additionalServices: (reg) => { registerStateServices(reg); + registerLogServices(reg); reg.defineInstance(ISessionContext, { _serviceBrand: undefined, sessionId: 'session-1', @@ -1071,6 +1074,7 @@ describe('IExternalHooksRunnerService integration', () => { strict: true, additionalServices: (reg) => { registerStateServices(reg); + registerLogServices(reg); reg.defineInstance(ISessionContext, stubSessionContext()); reg.defineInstance(ISessionLifecycleHooks, lifecycleHooks); reg.defineInstance(ISessionMetadata, stubSessionMetadata('My Session')); diff --git a/packages/agent-core/src/session/hooks/engine.ts b/packages/agent-core/src/session/hooks/engine.ts index f71491fbfc..3cc03fbbe8 100644 --- a/packages/agent-core/src/session/hooks/engine.ts +++ b/packages/agent-core/src/session/hooks/engine.ts @@ -91,6 +91,10 @@ export class HookEngine { }), ), ); + for (const [index, result] of results.entries()) { + if (!isHookFailure(result, args.signal?.aborted === true)) continue; + this.emitFailed(event, matched[index]!.command, result); + } const { action, reason } = aggregateResults(event, results); this.emitResolved(event, matcherValue, action, reason, Date.now() - startedAt); return results; @@ -128,6 +132,18 @@ export class HookEngine { this.options.onResolved?.(event, target, action, reason, durationMs); } catch {} } + + private emitFailed(event: string, command: string, result: HookResult): void { + try { + this.options.onFailed?.(event, command, result); + } catch {} + } +} + +function isHookFailure(result: HookResult, aborted: boolean): boolean { + if (result.timedOut === true) return true; + if (result.exitCode !== undefined) return result.exitCode !== 0 && result.exitCode !== 2; + return !aborted && (result.stderr?.trim().length ?? 0) > 0; } function matches(pattern: string, value: string): boolean { diff --git a/packages/agent-core/src/session/hooks/types.ts b/packages/agent-core/src/session/hooks/types.ts index 786296de27..eb67685727 100644 --- a/packages/agent-core/src/session/hooks/types.ts +++ b/packages/agent-core/src/session/hooks/types.ts @@ -64,9 +64,12 @@ export type HookResolvedCallback = ( durationMs: number, ) => void; +export type HookFailedCallback = (event: string, command: string, result: HookResult) => void; + export interface HookEngineOptions { readonly cwd?: string; readonly sessionId?: string; readonly onTriggered?: HookTriggeredCallback; readonly onResolved?: HookResolvedCallback; + readonly onFailed?: HookFailedCallback; } diff --git a/packages/agent-core/src/session/index.ts b/packages/agent-core/src/session/index.ts index 50762884f8..1446cad6f6 100644 --- a/packages/agent-core/src/session/index.ts +++ b/packages/agent-core/src/session/index.ts @@ -277,6 +277,15 @@ export class Session { this.hookEngine = new HookEngine(options.hooks, { cwd: options.kaos.getcwd(), sessionId: options.id, + onFailed: (event, command, result) => { + this.log.warn('external hook command failed', { + event, + command, + exitCode: result.exitCode, + timedOut: result.timedOut, + stderr: firstStderrLine(result.stderr), + }); + }, }); this.telemetry = options.telemetry ?? noopTelemetryClient; this.toolKaos = options.kaos; @@ -1382,3 +1391,8 @@ function initCompletionReminder(agentsMd: string): string { latest, ].join('\n'); } + +function firstStderrLine(stderr: string | undefined): string | undefined { + const line = stderr?.split(/\r?\n/, 1)[0]?.trim(); + return line === '' ? undefined : line; +} diff --git a/packages/agent-core/test/hooks/engine.test.ts b/packages/agent-core/test/hooks/engine.test.ts index b489c38516..f3ac415302 100644 --- a/packages/agent-core/test/hooks/engine.test.ts +++ b/packages/agent-core/test/hooks/engine.test.ts @@ -22,6 +22,7 @@ interface HookResult { reason?: string; stdout?: string; stderr?: string; + exitCode?: number; timedOut?: boolean; } @@ -46,6 +47,7 @@ interface HookEngineCtor { reason: string | undefined, durationMs: number, ) => void; + onFailed?: (event: string, command: string, result: HookResult) => void; }, ): { trigger: ( @@ -326,6 +328,39 @@ describe('HookEngine', () => { await expect(engine.fireAndForgetTrigger('Notification')).resolves.toEqual([]); }); + it('reports a non-zero hook exit without changing fail-open behavior', async () => { + const { HookEngine } = await importEngine(); + const failures: Array<[string, string, HookResult]> = []; + const command = 'node -e "process.stderr.write(\'bad hook\\nsecond line\'); process.exit(1)"'; + const engine = new HookEngine([{ event: 'UserPromptSubmit', command, timeout: 5 }], { + onFailed: (event, failedCommand, result) => { + failures.push([event, failedCommand, result]); + }, + }); + + const results = await engine.trigger('UserPromptSubmit'); + + expect(results[0]?.action).toBe('allow'); + expect(failures).toHaveLength(1); + expect(failures[0]?.[0]).toBe('UserPromptSubmit'); + expect(failures[0]?.[1]).toBe(command); + expect(failures[0]?.[2]).toMatchObject({ exitCode: 1, stderr: 'bad hook\nsecond line' }); + }); + + it('does not report an intentional exit-code-2 block as a hook failure', async () => { + const { HookEngine } = await importEngine(); + const onFailed = vi.fn(); + const engine = new HookEngine( + [{ event: 'PreToolUse', command: 'node -e "process.exit(2)"', timeout: 5 }], + { onFailed }, + ); + + const results = await engine.trigger('PreToolUse'); + + expect(results[0]?.action).toBe('block'); + expect(onFailed).not.toHaveBeenCalled(); + }); + it('preserves a PreToolUse block result even when telemetry throws (no fail-open)', async () => { // Safety-critical: a telemetry failure MUST NOT silently bypass a block. const telemetry = await import('../../src/utils/telemetry' as string).catch(() => null);