fix(agent-core-v2): rebuild hook index on config change so late [[hooks]] fire - #2822
fix(agent-core-v2): rebuild hook index on config change so late [[hooks]] fire#2822SnowingFox wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 9c29837 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1511ef5a9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (this.config.onDidChangeConfiguration !== undefined) { | ||
| this._register( | ||
| this.config.onDidChangeConfiguration(() => { | ||
| void this.reloadSafe(); |
There was a problem hiding this comment.
When a config change is immediately followed by a hook trigger, such as startup loading [[hooks]] and then creating a session that fires SessionStart, this fire-and-forget reload can still be pending: load() yields on config.ready and again on plugins.enabledHooks(), while triggerInner() only waits for the original constructor ready promise. The trigger can therefore read the old empty byEvent and skip the very hook this change is meant to pick up; serialize the current reload into a promise that triggers wait for, or otherwise rebuild the index before the change path returns.
Useful? React with 👍 / 👎.
| // 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()` — in the | ||
| // interactive TUI in particular, `[[hooks]]` may arrive late. Without this | ||
| // subscription the index would stay empty forever and every hook would |
There was a problem hiding this comment.
This package's AGENTS.md requires comments to live solely in the top-of-file header block, never beside methods or statements. This new constructor comment is an inline implementation narrative, so it violates the scoped convention; please fold any durable rationale into the file header or remove it.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L36-L38
Useful? React with 👍 / 👎.
…s are not missed Address codex P1 review comments on MoonshotAI#2822: fold the config-change rationale into the file header (AGENTS.md header-only comment convention) and serialize reloads into a promise chain that triggerInner awaits, so a hook trigger that lands right after a config change reads the rebuilt index instead of the stale empty one.
1 participant
Problem
Fixes #2779. On 0.34.0 the interactive TUI can silently run zero configured
[[hooks]](SessionStart / UserPromptSubmit / SessionHeartbeat), whilekimi -pon the sameconfig.tomlfires them. The engine gives no warning — the hooks "just never run" — matching the reporter's macOS repro.Root cause
ExternalHooksRunnerService(App scope) builds its event→hooks index exactly once, inloadSafe()at construction, and only ever rebuilds it onplugin.onDidReload. The user config file (config.toml) is (re)loaded into the layeredIConfigServiceasynchronously — in the interactive TUI the[[hooks]]section can arrive after this runner's one-shot index is built. BecauseloadSafe()/trigger()swallow errors andhasHooksFor()falls back to an empty index, every trigger silently returns[]and no hook ever fires, with no diagnostic.This is distinct from #2766 / PR #2772, which address the separate hook-spawn-failure path (logging). This PR targets the hooks-not-firing-at-all load path.
Fix
Subscribe
ExternalHooksRunnerServicetoIConfigService.onDidChangeConfigurationand re-runload()— the same rebuild the plugin-reload path already uses — so a hooks section that is present in, or (re)loaded into, the config is picked up instead of being permanently missed. A member check keeps the subscription a no-op against partialIConfigServicestubs in unit tests.Test
New regression test in
packages/agent-core-v2/test/app/externalHooksRunner/externalHooksRunner.test.ts: builds a runner whose config initially has no hooks, then delivers aSessionStarthook through a config-change event and assertshasHooksFor('SessionStart')becomes true and the hook fires. It fails on the pre-fix code (index never rebuilt) and passes after.Fixes #2779