mirror of
https://github.com/MoonshotAI/kimi-code.git
synced 2026-07-09 17:29:12 +00:00
refactor(agent-core-v2): run external hooks through IHostProcessService
- inject IHostProcessService into ExternalHooksRunnerService and thread it through runMatchedHooks to runHook instead of spawning node:child_process - route hook termination through the service's cross-platform process-tree kill - settle on the exit code plus drained stdout/stderr so fast-exiting hooks keep their trailing output - hide the child console window on Windows via the service default - update externalHooks tests for the new dependency
This commit is contained in:
parent
84b2989e8f
commit
7fdddbb1ad
6 changed files with 89 additions and 72 deletions
|
|
@ -6,6 +6,8 @@ import {
|
|||
|
||||
import { z } from 'zod';
|
||||
|
||||
import { type IHostProcess, IHostProcessService } from '#/os/interface/hostProcess';
|
||||
|
||||
import type { HookResult } from './types';
|
||||
|
||||
export interface RunHookOptions {
|
||||
|
|
@ -53,7 +55,7 @@ const HookSpecificOutputSchema = z.preprocess(
|
|||
.looseObject({
|
||||
message: OptionalStringSchema,
|
||||
permissionDecision: z.unknown().optional(),
|
||||
permissionDecisionReason: OptionalStringSchema,
|
||||
permissionDecisionReason: z.unknown().optional(),
|
||||
})
|
||||
.optional(),
|
||||
);
|
||||
|
|
@ -63,13 +65,18 @@ const HookJsonOutputSchema = z.looseObject({
|
|||
});
|
||||
|
||||
export async function runHook(
|
||||
hostProcess: IHostProcessService,
|
||||
command: string,
|
||||
input: Record<string, unknown>,
|
||||
options: RunHookOptions,
|
||||
): Promise<HookResult> {
|
||||
let child: ChildProcessWithoutNullStreams;
|
||||
let proc: IHostProcess;
|
||||
try {
|
||||
child = spawn(command, buildHookSpawnOptions({ cwd: options.cwd, env: options.env }));
|
||||
proc = await hostProcess.spawn(command, [], {
|
||||
shell: true,
|
||||
cwd: options.cwd,
|
||||
env: options.env,
|
||||
});
|
||||
} catch (error) {
|
||||
return allowResult({ stderr: errorMessage(error) });
|
||||
}
|
||||
|
|
@ -80,7 +87,7 @@ export async function runHook(
|
|||
let settled = false;
|
||||
const timeoutMs = timeoutSeconds(options.timeout) * 1000;
|
||||
|
||||
const cleanup = () => {
|
||||
const cleanup = (): void => {
|
||||
clearTimeout(timeout);
|
||||
options.signal?.removeEventListener('abort', onAbort);
|
||||
};
|
||||
|
|
@ -92,13 +99,40 @@ export async function runHook(
|
|||
resolve(result);
|
||||
};
|
||||
|
||||
proc.stdout.setEncoding('utf8');
|
||||
proc.stderr.setEncoding('utf8');
|
||||
proc.stdout.on('data', (chunk: string) => {
|
||||
stdout += chunk;
|
||||
});
|
||||
proc.stderr.on('data', (chunk: string) => {
|
||||
stderr += chunk;
|
||||
});
|
||||
|
||||
// Settle on the exit code AND drained stdio, not on `wait()` alone:
|
||||
// `wait()` resolves at the child's 'exit', which can precede the
|
||||
// stdout/stderr 'end', so a fast-exiting hook would otherwise lose its
|
||||
// trailing output. `proc.dispose()` runs here for every path (clean exit,
|
||||
// timeout, abort) once the process has exited and the streams have closed.
|
||||
const stdoutDone = new Promise<void>((done) => proc.stdout.once('end', done));
|
||||
const stderrDone = new Promise<void>((done) => proc.stderr.once('end', done));
|
||||
void Promise.all([proc.wait(), stdoutDone, stderrDone]).then(
|
||||
([code]) => {
|
||||
proc.dispose();
|
||||
settle(resultFromExitCode(code, stdout, stderr));
|
||||
},
|
||||
(error) => {
|
||||
proc.dispose();
|
||||
settle(allowResult({ stdout, stderr: stderr + errorMessage(error) }));
|
||||
},
|
||||
);
|
||||
|
||||
const timeout = setTimeout(() => {
|
||||
killProcess(child);
|
||||
void killProcess(proc);
|
||||
settle(allowResult({ stdout, stderr, timedOut: true }));
|
||||
}, timeoutMs);
|
||||
|
||||
const onAbort = (): void => {
|
||||
killProcess(child);
|
||||
void killProcess(proc);
|
||||
settle(allowResult({ stdout, stderr }));
|
||||
};
|
||||
|
||||
|
|
@ -108,23 +142,8 @@ export async function runHook(
|
|||
return;
|
||||
}
|
||||
|
||||
child.stdout.setEncoding('utf8');
|
||||
child.stderr.setEncoding('utf8');
|
||||
child.stdout.on('data', (chunk: string) => {
|
||||
stdout += chunk;
|
||||
});
|
||||
child.stderr.on('data', (chunk: string) => {
|
||||
stderr += chunk;
|
||||
});
|
||||
child.on('error', (error) => {
|
||||
settle(allowResult({ stdout, stderr: stderr + errorMessage(error) }));
|
||||
});
|
||||
child.on('close', (code) => {
|
||||
settle(resultFromExitCode(code ?? 0, stdout, stderr));
|
||||
});
|
||||
|
||||
child.stdin.on('error', () => {});
|
||||
child.stdin.end(JSON.stringify(input));
|
||||
proc.stdin.on('error', () => {});
|
||||
proc.stdin.end(JSON.stringify(input));
|
||||
});
|
||||
}
|
||||
|
||||
|
|
@ -189,8 +208,11 @@ function structuredOutput(
|
|||
return {
|
||||
action: 'block',
|
||||
message: result.message,
|
||||
reason: hookSpecificOutput.permissionDecisionReason,
|
||||
structuredOutput: true,
|
||||
reason:
|
||||
typeof hookSpecificOutput.permissionDecisionReason === 'string'
|
||||
? hookSpecificOutput.permissionDecisionReason
|
||||
: undefined,
|
||||
structuredOutput: true as const,
|
||||
};
|
||||
} catch {
|
||||
return undefined;
|
||||
|
|
@ -216,47 +238,14 @@ function allowResult(input: {
|
|||
};
|
||||
}
|
||||
|
||||
function killProcess(child: ChildProcessWithoutNullStreams): void {
|
||||
tryKillProcess(child, 'SIGTERM');
|
||||
function killProcess(proc: IHostProcess): void {
|
||||
void proc.kill('SIGTERM');
|
||||
const killTimer = setTimeout(() => {
|
||||
tryKillProcess(child, 'SIGKILL');
|
||||
void proc.kill('SIGKILL');
|
||||
}, KILL_GRACE_MS);
|
||||
killTimer.unref();
|
||||
}
|
||||
|
||||
function tryKillProcess(child: ChildProcessWithoutNullStreams, signal: NodeJS.Signals): void {
|
||||
if (process.platform === 'win32') {
|
||||
killProcessTreeWindows(child, signal === 'SIGKILL');
|
||||
return;
|
||||
}
|
||||
try {
|
||||
if (child.pid !== undefined) {
|
||||
process.kill(-child.pid, signal);
|
||||
} else {
|
||||
child.kill(signal);
|
||||
}
|
||||
} catch {
|
||||
try {
|
||||
child.kill(signal);
|
||||
} catch {}
|
||||
}
|
||||
}
|
||||
|
||||
function killProcessTreeWindows(child: ChildProcessWithoutNullStreams, force: boolean): void {
|
||||
if (child.pid === undefined) return;
|
||||
const args = force
|
||||
? ['/T', '/F', '/PID', String(child.pid)]
|
||||
: ['/T', '/PID', String(child.pid)];
|
||||
try {
|
||||
const killer = spawn('taskkill', args, { stdio: 'ignore', windowsHide: true });
|
||||
killer.once('error', () => {});
|
||||
} catch {
|
||||
try {
|
||||
child.kill('SIGTERM');
|
||||
} catch {}
|
||||
}
|
||||
}
|
||||
|
||||
function isRecord(value: unknown): value is Record<string, unknown> {
|
||||
return typeof value === 'object' && value !== null && !Array.isArray(value);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -4,9 +4,12 @@
|
|||
* Owns the configured-hook lifecycle: builds the event→hooks index from
|
||||
* `IConfigService` (`[[hooks]]`) + `IPluginService.enabledHooks()`, reloads it
|
||||
* on `plugin.onDidReload`, and dispatches each trigger through the pure
|
||||
* `runMatchedHooks`. 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. Bound at App scope.
|
||||
* `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
|
||||
* bootstrap cwd, `sessionId`, `signal`, payload) flow in through the args, so
|
||||
* this service keeps no per-scope state. Bound at App scope.
|
||||
*/
|
||||
|
||||
import { InstantiationType } from '#/_base/di/extensions';
|
||||
|
|
@ -17,6 +20,7 @@ import { IConfigService } from '#/app/config/config';
|
|||
import { IPluginService } from '#/app/plugin/plugin';
|
||||
import { HOOKS_SECTION, type HookDefConfig } from '#/agent/externalHooks/configSection';
|
||||
import type { HookBlockDecision, HookDef, HookResult } from '#/agent/externalHooks/types';
|
||||
import { IHostProcessService } from '#/os/interface/hostProcess';
|
||||
|
||||
import {
|
||||
IExternalHooksRunnerService,
|
||||
|
|
@ -35,6 +39,7 @@ export class ExternalHooksRunnerService extends Disposable implements IExternalH
|
|||
@IConfigService private readonly config: IConfigService,
|
||||
@IPluginService private readonly plugins: IPluginService,
|
||||
@IBootstrapService private readonly bootstrap: IBootstrapService,
|
||||
@IHostProcessService private readonly hostProcess: IHostProcessService,
|
||||
private readonly callbacks: HookRunCallbacks = {},
|
||||
) {
|
||||
super();
|
||||
|
|
@ -86,6 +91,7 @@ export class ExternalHooksRunnerService extends Disposable implements IExternalH
|
|||
): Promise<HookResult[]> {
|
||||
await this.ready;
|
||||
return runMatchedHooks(
|
||||
this.hostProcess,
|
||||
this.byEvent,
|
||||
event,
|
||||
{
|
||||
|
|
|
|||
|
|
@ -4,8 +4,9 @@
|
|||
* Owns everything the `IExternalHooksRunnerService` needs to decide *which*
|
||||
* hooks run for an event and to execute them: building the event→hooks index,
|
||||
* regex matching by matcher value, de-duplication per `(cwd, command)`, and
|
||||
* spawning each matched command via the shared `runHook` spawner. Holds no
|
||||
* config/plugin state and no per-scope facts — those come in per call. Pure
|
||||
* spawning each matched command via the shared `runHook` spawner (which runs
|
||||
* through the App-scope `IHostProcessService` passed in by the service). Holds
|
||||
* no config/plugin state and no per-scope facts — those come in per call. Pure
|
||||
* helper module, not a scoped Service.
|
||||
*/
|
||||
|
||||
|
|
@ -16,6 +17,7 @@ import type {
|
|||
HookMatcherValue,
|
||||
HookResult,
|
||||
} from '#/agent/externalHooks/types';
|
||||
import type { IHostProcessService } from '#/os/interface/hostProcess';
|
||||
|
||||
import type { ExternalHooksRunnerTriggerArgs } from './externalHooksRunner';
|
||||
|
||||
|
|
@ -45,6 +47,7 @@ export function indexHooks(hooks: readonly HookDef[]): Map<string, HookDef[]> {
|
|||
|
||||
/** Run every hook in `byEvent` whose matcher matches `args.matcherValue`. */
|
||||
export async function runMatchedHooks(
|
||||
hostProcess: IHostProcessService,
|
||||
byEvent: ReadonlyMap<string, readonly HookDef[]>,
|
||||
event: string,
|
||||
args: ExternalHooksRunnerTriggerArgs,
|
||||
|
|
@ -77,7 +80,7 @@ export async function runMatchedHooks(
|
|||
const startedAt = Date.now();
|
||||
const results = await Promise.all(
|
||||
matched.map((hook) =>
|
||||
runHook(hook.command, inputData, {
|
||||
runHook(hostProcess, hook.command, inputData, {
|
||||
timeout: hook.timeout ?? DEFAULT_HOOK_TIMEOUT_SECONDS,
|
||||
cwd: hook.cwd ?? (cwd === '' ? undefined : cwd),
|
||||
env: hook.env,
|
||||
|
|
|
|||
|
|
@ -45,6 +45,8 @@ import { IConfigService } from '#/app/config/config';
|
|||
import { IEventBus } from '#/app/event/eventBus';
|
||||
import { EventBusService } from '#/app/event/eventBusService';
|
||||
import { IPluginService } from '#/app/plugin/plugin';
|
||||
import { IHostProcessService } from '#/os/interface/hostProcess';
|
||||
import { HostProcessService } from '#/os/backends/node-local/hostProcessService';
|
||||
import {
|
||||
ISessionLifecycleService,
|
||||
type SessionLifecycleHooks,
|
||||
|
|
@ -592,6 +594,7 @@ describe('IExternalHooksRunnerService integration', () => {
|
|||
IAgentWireService,
|
||||
disposables.add(new WireService({ logScope: 'wire', logKey: 'external-hooks' })),
|
||||
);
|
||||
reg.define(IHostProcessService, HostProcessService);
|
||||
},
|
||||
});
|
||||
ix.set(IExternalHooksRunnerService, new SyncDescriptor(ExternalHooksRunnerService));
|
||||
|
|
@ -854,6 +857,7 @@ describe('IExternalHooksRunnerService integration', () => {
|
|||
onDidReload: Event.None as IPluginService['onDidReload'],
|
||||
});
|
||||
reg.defineInstance(IBootstrapService, stubBootstrap());
|
||||
reg.define(IHostProcessService, HostProcessService);
|
||||
},
|
||||
});
|
||||
ix.set(IExternalHooksRunnerService, new SyncDescriptor(ExternalHooksRunnerService));
|
||||
|
|
|
|||
|
|
@ -3,10 +3,11 @@
|
|||
* from a list of hook definitions.
|
||||
*
|
||||
* The runner is App-scoped in production; in tests we construct it directly
|
||||
* (its only constructor params are the three App services it reads) with stub
|
||||
* `IConfigService` / `IPluginService` / `IBootstrapService`. This keeps the
|
||||
* matching / dedupe / stdin-payload behavior under test identical to
|
||||
* production while letting a test feed an arbitrary hook list.
|
||||
* (its constructor params are the App services it reads plus the host process
|
||||
* service) with stub `IConfigService` / `IPluginService` / `IBootstrapService`
|
||||
* and a real `HostProcessService`. This keeps the matching / dedupe /
|
||||
* stdin-payload behavior under test identical to production while letting a
|
||||
* test feed an arbitrary hook list.
|
||||
*/
|
||||
|
||||
import { Event } from '#/_base/event';
|
||||
|
|
@ -16,6 +17,7 @@ import type { HookDef } from '#/agent/externalHooks/types';
|
|||
import { IBootstrapService } from '#/app/bootstrap/bootstrap';
|
||||
import { IConfigService } from '#/app/config/config';
|
||||
import { IPluginService } from '#/app/plugin/plugin';
|
||||
import { HostProcessService } from '#/os/backends/node-local/hostProcessService';
|
||||
|
||||
export function makeHookRunner(
|
||||
hooks: readonly HookDef[],
|
||||
|
|
@ -43,6 +45,7 @@ export function makeHookRunner(
|
|||
onDidReload: Event.None as IPluginService['onDidReload'],
|
||||
} as unknown as IPluginService,
|
||||
{ _serviceBrand: undefined, cwd: options.cwd ?? '' } as unknown as IBootstrapService,
|
||||
new HostProcessService(),
|
||||
{ onTriggered: options.onTriggered, onResolved: options.onResolved },
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,6 +1,9 @@
|
|||
import { describe, expect, it } from 'vitest';
|
||||
|
||||
import { buildHookSpawnOptions, runHook } from '#/agent/externalHooks/runner';
|
||||
import { runHook } from '#/agent/externalHooks/runner';
|
||||
import { HostProcessService } from '#/os/backends/node-local/hostProcessService';
|
||||
|
||||
const hostProcess = new HostProcessService();
|
||||
|
||||
function nodeCommand(source: string): string {
|
||||
return `node -e ${JSON.stringify(source.replace(/\s*\n\s*/g, ' '))}`;
|
||||
|
|
@ -9,6 +12,7 @@ function nodeCommand(source: string): string {
|
|||
describe('runHook process runner', () => {
|
||||
it('returns allow when the hook exits 0 and captures stdout', async () => {
|
||||
const result = await runHook(
|
||||
hostProcess,
|
||||
nodeCommand('process.stdout.write("ok\\n");'),
|
||||
{ tool_name: 'Bash' },
|
||||
{ timeout: 5 },
|
||||
|
|
@ -20,6 +24,7 @@ describe('runHook process runner', () => {
|
|||
|
||||
it('parses stdout JSON message into a hook result message', async () => {
|
||||
const result = await runHook(
|
||||
hostProcess,
|
||||
nodeCommand('process.stdout.write(JSON.stringify({ message: "hook says hi" }));'),
|
||||
{},
|
||||
{ timeout: 5 },
|
||||
|
|
@ -32,6 +37,7 @@ describe('runHook process runner', () => {
|
|||
|
||||
it('marks structured stdout JSON without message as empty hook output', async () => {
|
||||
const emptyObject = await runHook(
|
||||
hostProcess,
|
||||
nodeCommand('process.stdout.write("{}");'),
|
||||
{},
|
||||
{ timeout: 5 },
|
||||
|
|
@ -41,6 +47,7 @@ describe('runHook process runner', () => {
|
|||
expect(emptyObject.structuredOutput).toBe(true);
|
||||
|
||||
const emptyHookSpecificOutput = await runHook(
|
||||
hostProcess,
|
||||
nodeCommand('process.stdout.write(JSON.stringify({ hookSpecificOutput: {} }));'),
|
||||
{},
|
||||
{ timeout: 5 },
|
||||
|
|
@ -52,6 +59,7 @@ describe('runHook process runner', () => {
|
|||
|
||||
it('returns block when the hook exits 2 and captures stderr as the reason', async () => {
|
||||
const result = await runHook(
|
||||
hostProcess,
|
||||
nodeCommand('process.stderr.write("blocked\\n"); process.exit(2);'),
|
||||
{ tool_name: 'Bash' },
|
||||
{ timeout: 5 },
|
||||
|
|
@ -63,6 +71,7 @@ describe('runHook process runner', () => {
|
|||
|
||||
it('returns allow on non-zero, non-2 exit codes', async () => {
|
||||
const result = await runHook(
|
||||
hostProcess,
|
||||
nodeCommand('process.exit(1);'),
|
||||
{ tool_name: 'Bash' },
|
||||
{ timeout: 5 },
|
||||
|
|
@ -73,6 +82,7 @@ describe('runHook process runner', () => {
|
|||
|
||||
it('returns allow with timedOut=true when the command exceeds the timeout', async () => {
|
||||
const result = await runHook(
|
||||
hostProcess,
|
||||
nodeCommand('setTimeout(() => {}, 10000);'),
|
||||
{ tool_name: 'Bash' },
|
||||
{ timeout: 1 },
|
||||
|
|
@ -84,6 +94,7 @@ describe('runHook process runner', () => {
|
|||
|
||||
it('parses stdout JSON permissionDecision=deny into a block result with the supplied reason', async () => {
|
||||
const result = await runHook(
|
||||
hostProcess,
|
||||
nodeCommand(
|
||||
'process.stdout.write(JSON.stringify({ hookSpecificOutput: { permissionDecision: "deny", permissionDecisionReason: "use rg" } }));',
|
||||
),
|
||||
|
|
@ -97,6 +108,7 @@ describe('runHook process runner', () => {
|
|||
|
||||
it('writes the input payload to the hook process stdin as JSON', async () => {
|
||||
const result = await runHook(
|
||||
hostProcess,
|
||||
nodeCommand([
|
||||
'let input = "";',
|
||||
'process.stdin.on("data", (chunk) => { input += chunk; });',
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue