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/hooks-reload-on-config-change.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@moonshot-ai/kimi-code": patch
---

Rebuild the external-hook index when the config changes so `[[hooks]]` that arrive after engine construction (e.g. the interactive TUI loading config.toml) fire instead of silently never running.
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@
*
* 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
* on plugin reload and on config change (the interactive TUI can (re)load
* `config.toml` into the layered config after app-scope construction, so the
* index must be rebuilt or late `[[hooks]]` would silently never fire, #2779),
* and dispatches each trigger through the pure
* `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
Expand Down Expand Up @@ -38,6 +41,9 @@ export class ExternalHooksRunnerService extends Disposable implements IExternalH

private byEvent = new Map<string, HookDef[]>();
readonly ready: Promise<void>;
// Serializes index rebuilds so a trigger that lands right after a config
// change awaits the newest index instead of reading the stale one.
private reloadChain: Promise<void>;

private readonly _onDidReload = this._register(new Emitter<void>());
readonly onDidReload: Event<void> = this._onDidReload.event;
Expand All @@ -51,11 +57,24 @@ export class ExternalHooksRunnerService extends Disposable implements IExternalH
) {
super();
this.ready = this.loadSafe();
this.reloadChain = Promise.resolve();
this._register(
this.plugins.onDidReload(() => {
void this.reloadSafe();
this.queueReload();
}),
);
// Rebuild the hook index when the config changes, not just on plugin
// reload: the user config file (`config.toml`) can be (re)loaded into the
// layered config service after this runner's initial `loadSafe()`, and in
// the interactive TUI `[[hooks]]` may arrive late. The member check keeps
// the subscription a no-op against partial `IConfigService` stubs.
if (this.config.onDidChangeConfiguration !== undefined) {
this._register(
this.config.onDidChangeConfiguration(() => {
this.queueReload();
}),
);
}
}

get summary(): Record<string, number> {
Expand Down Expand Up @@ -101,6 +120,7 @@ export class ExternalHooksRunnerService extends Disposable implements IExternalH
args: ExternalHooksRunnerTriggerArgs,
): Promise<HookResult[]> {
await this.ready;
await this.reloadChain;
return runMatchedHooks(
this.hostProcess,
this.byEvent,
Expand All @@ -123,10 +143,8 @@ export class ExternalHooksRunnerService extends Disposable implements IExternalH
} catch {}
}

private async reloadSafe(): Promise<void> {
try {
await this.load();
} catch {}
private queueReload(): void {
this.reloadChain = this.reloadChain.then(() => this.loadSafe());
}

private async load(): Promise<void> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,16 @@ export function makeHookRunner(
reason: string | undefined,
durationMs: number,
) => void;
/** Emit when the backing config gains/loses hook sections (mirrors `IConfigService.onDidChangeConfiguration`). */
onDidChangeConfiguration?: Event<void>;
} = {},
): ExternalHooksRunnerService {
return new ExternalHooksRunnerService(
{
_serviceBrand: undefined,
ready: Promise.resolve(),
get: (section: string) => (section === HOOKS_SECTION ? hooks : undefined),
onDidChangeConfiguration: options.onDidChangeConfiguration ?? Event.None,
} as unknown as IConfigService,
{
_serviceBrand: undefined,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { realpathSync } from 'node:fs';
import { tmpdir } from 'node:os';

import { Emitter } from '#/_base/event';
import type { HookDef } from '#/agent/externalHooks/types';
import type { ContentPart } from '#/kosong/contract/message';
import { describe, expect, it, vi } from 'vitest';

Expand Down Expand Up @@ -341,4 +343,26 @@ describe('ExternalHooksRunnerService', () => {
expect(runner.hasHooksFor('SessionHeartbeat')).toBe(true);
expect(runner.hasHooksFor('Stop')).toBe(false);
});

it('rebuilds the hook index when hooks arrive through a config change', async () => {
const hooks: HookDef[] = [];
const emitter = new Emitter<void>();
const runner = makeHookRunner(hooks, { onDidChangeConfiguration: emitter.event });

await runner.ready;
expect(runner.hasHooksFor('SessionStart')).toBe(false);

// The config gains a [[hooks]] section after the runner was constructed
// (the interactive TUI can load config.toml after app-scope construction).
// The runner must re-read the config instead of keeping the empty index
// forever, or the hook would silently never fire.
hooks.push({ event: 'SessionStart', command: nodeCommand('process.exit(0);'), timeout: 5 });
emitter.fire();

await vi.waitFor(() => {
expect(runner.hasHooksFor('SessionStart')).toBe(true);
});
const results = await runner.trigger('SessionStart');
expect(results).toHaveLength(1);
});
});