Fix: Plugin hooks registered but never fire #3249#3993
Closed
WallaceSociety wants to merge 1 commit intoopenclaw:mainfrom
Closed
Fix: Plugin hooks registered but never fire #3249#3993WallaceSociety wants to merge 1 commit intoopenclaw:mainfrom
WallaceSociety wants to merge 1 commit intoopenclaw:mainfrom
Conversation
WallaceSociety
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+256
to
+268
| // Bridge to typed hooks: if any event matches a PluginHookName, also register | ||
| // in typedHooks so the hook runner can find it. This allows plugins to use | ||
| // either api.registerHook() or api.on() for typed hooks like message_received. | ||
| for (const event of normalizedEvents) { | ||
| if (TYPED_HOOK_NAMES.has(event as PluginHookName)) { | ||
| registry.typedHooks.push({ | ||
| pluginId: record.id, | ||
| hookName: event as PluginHookName, | ||
| handler: handler as unknown as PluginHookHandlerMap[PluginHookName], | ||
| priority: opts?.priority, | ||
| source: record.source, | ||
| } as TypedPluginHookRegistration); | ||
| record.hookCount += 1; |
Contributor
There was a problem hiding this comment.
[P1] record.hookCount is incremented twice for typed hook events registered via api.registerHook().
registerHook() already pushes a legacy hook into registry.hooks, but the new bridge also does record.hookCount += 1 for each typed event (src/plugins/registry.ts:256-269). Since hookCount is also incremented in registerTypedHook() (src/plugins/registry.ts:464), this likely makes hookCount inconsistent across registration paths (legacy vs typed), and can over-count for multi-event registrations. Consider either not adjusting hookCount in the bridge, or explicitly defining whether hookCount should include typed hook registrations and keep that consistent across both APIs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugins/registry.ts
Line: 256:268
Comment:
[P1] `record.hookCount` is incremented twice for typed hook events registered via `api.registerHook()`.
`registerHook()` already pushes a legacy hook into `registry.hooks`, but the new bridge also does `record.hookCount += 1` for each typed event (src/plugins/registry.ts:256-269). Since `hookCount` is also incremented in `registerTypedHook()` (src/plugins/registry.ts:464), this likely makes `hookCount` inconsistent across registration paths (legacy vs typed), and can over-count for multi-event registrations. Consider either not adjusting `hookCount` in the bridge, or explicitly defining whether `hookCount` should include typed hook registrations and keep that consistent across both APIs.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
|
Closing with Tak approval: older root complaint already addressed/superseded |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root Cause
The codebase had two separate hook systems that weren't properly connected:
Legacy hooks: api.registerHook() → registry.hooks
Typed hooks: api.on() → registry.typedHooks
The hook runner's hasHooks() and execution logic only checked registry.typedHooks, but the log message counted registry.hooks — creating a misleading situation where hooks appeared registered but never fired.
registerHook()to bridge totypedHookswhen the event name matches a typed hook likemessage_receivedGreptile Overview
Greptile Summary
This PR fixes a mismatch between plugin hook registration and execution by bridging legacy
api.registerHook()registrations into the typed hook registry when the event name matches a typed hook (e.g.message_received). It also corrects the global hook runner init log to report the number of typed hooks (the only ones the runner checks).Overall, this aligns registration and execution so hooks that appear registered will actually fire through the typed hook runner path.
Confidence Score: 4/5
hookCount) consistency due to the new bridge incrementing counts.(2/5) Greptile learns from your feedback when you react with thumbs up/down!