Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/silent-hooks-warn.md
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,16 @@
* `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),
* merged under the caller's `inputData`. Bound at App scope.
*/

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';
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
},
},
);
}

Expand All @@ -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,
Expand Down
13 changes: 13 additions & 0 deletions packages/agent-core-v2/src/app/externalHooksRunner/runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, HookDef[]> {
Expand Down Expand Up @@ -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 {
Expand All @@ -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[],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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: {
Expand All @@ -31,6 +34,7 @@ export function makeHookRunner(
reason: string | undefined,
durationMs: number,
) => void;
onWarn?: (message: string, payload?: LogPayload) => void;
} = {},
): ExternalHooksRunnerService {
return new ExternalHooksRunnerService(
Expand All @@ -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 },
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -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]> = [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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, ' '))}`;
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -863,6 +865,7 @@ describe('IExternalHooksRunnerService integration', () => {
strict: true,
additionalServices: (reg) => {
registerStateServices(reg);
registerLogServices(reg);
reg.defineInstance(ISessionContext, {
_serviceBrand: undefined,
sessionId: 'session-1',
Expand Down Expand Up @@ -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'));
Expand Down
16 changes: 16 additions & 0 deletions packages/agent-core/src/session/hooks/engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
3 changes: 3 additions & 0 deletions packages/agent-core/src/session/hooks/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
14 changes: 14 additions & 0 deletions packages/agent-core/src/session/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
35 changes: 35 additions & 0 deletions packages/agent-core/test/hooks/engine.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ interface HookResult {
reason?: string;
stdout?: string;
stderr?: string;
exitCode?: number;
timedOut?: boolean;
}

Expand All @@ -46,6 +47,7 @@ interface HookEngineCtor {
reason: string | undefined,
durationMs: number,
) => void;
onFailed?: (event: string, command: string, result: HookResult) => void;
},
): {
trigger: (
Expand Down Expand Up @@ -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);
Expand Down