From 7fdddbb1ad3092f3165d7fbae497fcdc76908fef Mon Sep 17 00:00:00 2001 From: "haozhe.yang" Date: Wed, 8 Jul 2026 21:50:59 +0800 Subject: [PATCH] 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 --- .../src/agent/externalHooks/runner.ts | 111 ++++++++---------- .../externalHooksRunnerService.ts | 12 +- .../src/app/externalHooksRunner/runner.ts | 9 +- .../test/externalHooks/integration.test.ts | 4 + .../test/externalHooks/runner-stub.ts | 11 +- .../test/externalHooks/runner.test.ts | 14 ++- 6 files changed, 89 insertions(+), 72 deletions(-) diff --git a/packages/agent-core-v2/src/agent/externalHooks/runner.ts b/packages/agent-core-v2/src/agent/externalHooks/runner.ts index 5ab79f32a..3caf67678 100644 --- a/packages/agent-core-v2/src/agent/externalHooks/runner.ts +++ b/packages/agent-core-v2/src/agent/externalHooks/runner.ts @@ -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, options: RunHookOptions, ): Promise { - 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((done) => proc.stdout.once('end', done)); + const stderrDone = new Promise((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 { return typeof value === 'object' && value !== null && !Array.isArray(value); } diff --git a/packages/agent-core-v2/src/app/externalHooksRunner/externalHooksRunnerService.ts b/packages/agent-core-v2/src/app/externalHooksRunner/externalHooksRunnerService.ts index 92b66093e..5414ccadb 100644 --- a/packages/agent-core-v2/src/app/externalHooksRunner/externalHooksRunnerService.ts +++ b/packages/agent-core-v2/src/app/externalHooksRunner/externalHooksRunnerService.ts @@ -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 { await this.ready; return runMatchedHooks( + this.hostProcess, this.byEvent, event, { diff --git a/packages/agent-core-v2/src/app/externalHooksRunner/runner.ts b/packages/agent-core-v2/src/app/externalHooksRunner/runner.ts index d1890a7e2..48e3ecbc6 100644 --- a/packages/agent-core-v2/src/app/externalHooksRunner/runner.ts +++ b/packages/agent-core-v2/src/app/externalHooksRunner/runner.ts @@ -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 { /** Run every hook in `byEvent` whose matcher matches `args.matcherValue`. */ export async function runMatchedHooks( + hostProcess: IHostProcessService, byEvent: ReadonlyMap, 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, diff --git a/packages/agent-core-v2/test/externalHooks/integration.test.ts b/packages/agent-core-v2/test/externalHooks/integration.test.ts index 94df5964d..d8172d1a2 100644 --- a/packages/agent-core-v2/test/externalHooks/integration.test.ts +++ b/packages/agent-core-v2/test/externalHooks/integration.test.ts @@ -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)); diff --git a/packages/agent-core-v2/test/externalHooks/runner-stub.ts b/packages/agent-core-v2/test/externalHooks/runner-stub.ts index 75a8f5ff5..9799e001d 100644 --- a/packages/agent-core-v2/test/externalHooks/runner-stub.ts +++ b/packages/agent-core-v2/test/externalHooks/runner-stub.ts @@ -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 }, ); } diff --git a/packages/agent-core-v2/test/externalHooks/runner.test.ts b/packages/agent-core-v2/test/externalHooks/runner.test.ts index 114cd6bd6..75bb74cb9 100644 --- a/packages/agent-core-v2/test/externalHooks/runner.test.ts +++ b/packages/agent-core-v2/test/externalHooks/runner.test.ts @@ -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; });',