fix(hooks): bridge message_sending event to internal hook registry#73570
fix(hooks): bridge message_sending event to internal hook registry#73570SymbolStar wants to merge 6 commits into
Conversation
Greptile SummaryThis PR bridges the Confidence Score: 3/5Not safe to merge — two P1 defects mean internal hook subscribers receive wrong content and spurious events for cancelled sends. Two independent P1 defects: (1) the hook event always carries pre-plugin-modification content because the outer payloadSummary is used instead of hookResult.payloadSummary, and (2) the hook fires even when the send is cancelled because the guard is placed after the fireAndForgetHook call. Both affect the correctness of the new feature on its primary code path. src/infra/outbound/deliver.ts — specifically the new block around lines 1032–1053. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/infra/outbound/deliver.ts
Line: 1041
Comment:
**Stale payload content sent to internal hook**
The internal hook event is built from the outer `payloadSummary` variable (captured before `applyMessageSendingHook` ran), not from `hookResult.payloadSummary`. When a plugin hook modifies the message content (`sendingResult.content != null`), `applyMessageSendingHook` returns an updated `payloadSummary` in `hookResult.payloadSummary` — but that modified content is never used here. Internal hook subscribers will always see the pre-plugin-modification content, even when a plugin rewrote it.
```suggestion
content: hookResult.payloadSummary.hookContent ?? hookResult.payloadSummary.text,
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/infra/outbound/deliver.ts
Line: 1032-1053
Comment:
**Internal hook fires for cancelled sends**
The `fireAndForgetHook` call happens unconditionally before the `if (hookResult.cancelled) { continue; }` guard on line 1054. When a plugin hook sets `cancel: true`, the internal `message_sending` hook fires anyway — telling user-defined hooks that a message is being sent when it will actually never leave the system. Add a `!hookResult.cancelled` guard to the condition.
```suggestion
if (sessionKeyForInternalHooks && !hookResult.cancelled) {
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(hooks): bridge message_sending event..." | Re-trigger Greptile |
| sessionKeyForInternalHooks, | ||
| { | ||
| to, | ||
| content: payloadSummary.hookContent ?? payloadSummary.text, |
There was a problem hiding this comment.
The internal hook event is built from the outer payloadSummary variable (captured before applyMessageSendingHook ran), not from hookResult.payloadSummary. When a plugin hook modifies the message content (sendingResult.content != null), applyMessageSendingHook returns an updated payloadSummary in hookResult.payloadSummary — but that modified content is never used here. Internal hook subscribers will always see the pre-plugin-modification content, even when a plugin rewrote it.
| content: payloadSummary.hookContent ?? payloadSummary.text, | |
| content: hookResult.payloadSummary.hookContent ?? hookResult.payloadSummary.text, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/outbound/deliver.ts
Line: 1041
Comment:
**Stale payload content sent to internal hook**
The internal hook event is built from the outer `payloadSummary` variable (captured before `applyMessageSendingHook` ran), not from `hookResult.payloadSummary`. When a plugin hook modifies the message content (`sendingResult.content != null`), `applyMessageSendingHook` returns an updated `payloadSummary` in `hookResult.payloadSummary` — but that modified content is never used here. Internal hook subscribers will always see the pre-plugin-modification content, even when a plugin rewrote it.
```suggestion
content: hookResult.payloadSummary.hookContent ?? hookResult.payloadSummary.text,
```
How can I resolve this? If you propose a fix, please make it concise.| if (sessionKeyForInternalHooks) { | ||
| fireAndForgetHook( | ||
| triggerInternalHook( | ||
| createInternalHookEvent( | ||
| "message", | ||
| "sending", | ||
| sessionKeyForInternalHooks, | ||
| { | ||
| to, | ||
| content: payloadSummary.hookContent ?? payloadSummary.text, | ||
| channelId: channel, | ||
| accountId: accountId ?? undefined, | ||
| conversationId: to, | ||
| }, | ||
| ), | ||
| ), | ||
| "deliverOutboundPayloads: message:sending internal hook failed", | ||
| (message) => { | ||
| log.warn(message); | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The fireAndForgetHook call happens unconditionally before the if (hookResult.cancelled) { continue; } guard on line 1054. When a plugin hook sets cancel: true, the internal message_sending hook fires anyway — telling user-defined hooks that a message is being sent when it will actually never leave the system. Add a !hookResult.cancelled guard to the condition.
| if (sessionKeyForInternalHooks) { | |
| fireAndForgetHook( | |
| triggerInternalHook( | |
| createInternalHookEvent( | |
| "message", | |
| "sending", | |
| sessionKeyForInternalHooks, | |
| { | |
| to, | |
| content: payloadSummary.hookContent ?? payloadSummary.text, | |
| channelId: channel, | |
| accountId: accountId ?? undefined, | |
| conversationId: to, | |
| }, | |
| ), | |
| ), | |
| "deliverOutboundPayloads: message:sending internal hook failed", | |
| (message) => { | |
| log.warn(message); | |
| }, | |
| ); | |
| } | |
| if (sessionKeyForInternalHooks && !hookResult.cancelled) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/outbound/deliver.ts
Line: 1032-1053
Comment:
**Internal hook fires for cancelled sends**
The `fireAndForgetHook` call happens unconditionally before the `if (hookResult.cancelled) { continue; }` guard on line 1054. When a plugin hook sets `cancel: true`, the internal `message_sending` hook fires anyway — telling user-defined hooks that a message is being sent when it will actually never leave the system. Add a `!hookResult.cancelled` guard to the condition.
```suggestion
if (sessionKeyForInternalHooks && !hookResult.cancelled) {
```
How can I resolve this? If you propose a fix, please make it concise.|
Codex review: needs real behavior proof before merge. Latest ClawSweeper review: 2026-05-20 02:11 UTC / May 19, 2026, 10:11 PM ET. Workflow note: Future ClawSweeper reviews update this same comment in place. How this review workflow works
Summary Reproducibility: yes. at source level. Current main runs plugin PR rating Rank-up moves:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. Real behavior proof Risk before merge
Maintainer options:
Next step before merge Security Review findings
Review detailsBest possible solution: Define the standalone pre-send hook contract, add compatibility for the reported key or explicitly document the colon key, emit only after plugin rewrite/cancellation/effective-payload decisions are settled, restore focused tests, and prove it with a redacted hook-directory run. Do we have a high-confidence way to reproduce the issue? Yes, at source level. Current main runs plugin Is this the best way to solve the issue? No. Shared outbound delivery is the right area, but this patch needs the hook-key contract, cancellation/blank-payload timing, rewritten-content semantics, and test drift fixed before it is the maintainable solution. Label changes:
Label justifications:
Full review comments:
Overall correctness: patch is incorrect Security concerns:
Acceptance criteria:
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against a002c416c7af. |
7fa1008 to
31f0096
Compare
|
Hi @steipete — friendly ping 🙏 CI is green now (rebased on latest main). Would appreciate a look when you get a chance! |
31f0096 to
b8001cd
Compare
Fixes openclaw#73542 The message_sending event was only dispatched through the plugin hook runner (hookRunner.runMessageSending) but never bridged to the internal hook registry via triggerInternalHook. This meant user-defined hooks registered for message_sending were never invoked. Add triggerInternalHook(createInternalHookEvent("message", "sending", ...)) call mirroring the existing message_sent pattern.
The message:sending hook added in this PR calls createInternalHookEvent and triggerInternalHook before delivery, so tests expecting only the message:sent hook must now account for both calls.
Remove matrix/mattermost from publicPluginOwnedSdkEntrypoints since they are already classified in supportedBundledFacadeSdkEntrypoints, causing classification overlap failures.
b8001cd to
18e0269
Compare
…ding-internal-hook # Conflicts: # src/infra/outbound/deliver.test.ts
|
Merged latest |
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
…tics The 'silentReply.direct' field was removed from SilentReplyPolicyShape (direct is always disallowed by default). The expectation also needed to flip from 'rewrite then send' to 'suppress send' to match the policy that's actually implemented on main.
|
Updated
New HEAD: 9391b1b. |
Root Cause
The
message_sendingevent was dispatched only through the plugin hook runner (hookRunner.runMessageSending()) but was never bridged to the internal hook registry viatriggerInternalHook(). This meant user-defined hooks (registered in~/.openclaw/hooks/) listening formessage_sendingwere never invoked.In contrast,
message_sentcorrectly calls both paths: the plugin hook runner ANDtriggerInternalHook(createInternalHookEvent("message", "sent", ...)).Fix
Add a
triggerInternalHook(createInternalHookEvent("message", "sending", ...))call indeliverOutboundPayloads, immediately afterapplyMessageSendingHook()returns, mirroring the existingmessage_sentpattern.The internal hook is fire-and-forget (notification only) since
triggerInternalHookreturnsPromise<void>— content modification still goes through the plugin hook runner path. User-defined hooks will now receive the event notification as expected.Verification
message_sentbridging pattern at line ~667Fixes #73542