fix: emit message_received for queued inbound messages#64569
fix: emit message_received for queued inbound messages#64569EronFan wants to merge 1 commit intoopenclaw:mainfrom
Conversation
openclaw-barnacle
Bot
added
size: M
r: too-many-prs
|
Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit. |
Greptile SummaryThis PR adds
Confidence Score: 2/5Not safe to merge as-is: the hook emission added in enqueue.ts fires a second time for every message that dispatch already processed, duplicating message_received events for any queued message. The P1 finding is a definite present defect:
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/auto-reply/reply/queue/enqueue.ts
Line: 45-71
Comment:
**Double-fire of `message_received` for queued messages**
`dispatch-from-config.ts` already emits both the plugin and internal `message_received` hooks unconditionally for every inbound non-plugin-claimed message (before calling into `runPreparedReply` → `runReplyAgent` → `enqueueFollowupRun`). Adding a second emission here means every message queued while the session is busy fires the hooks **twice** — once in dispatch and once here. The "session is active" check happens inside `runReplyAgent`, which is called *after* dispatch has already fired the hooks, so there is no guard that prevents the duplication.
To fix this, the emission in `dispatch-from-config.ts` (lines ~486–507) should be suppressed when the message is going to be enqueued, or the new emission here should only happen for code paths that do not go through dispatch — but currently `enqueueFollowupRun` has exactly one production call site and it always arrives via `dispatch-from-config.ts`.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: emit queued message_received hooks" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function emitQueuedMessageReceivedHooks(run: FollowupRun): void { | ||
| const canonical = run.inboundHookContext; | ||
| if (!canonical) { | ||
| return; | ||
| } | ||
| const hookRunner = getGlobalHookRunner(); | ||
| if (hookRunner?.hasHooks("message_received")) { | ||
| fireAndForgetHook( | ||
| hookRunner.runMessageReceived( | ||
| toPluginMessageReceivedEvent(canonical), | ||
| toPluginMessageContext(canonical), | ||
| ), | ||
| "queue/enqueue: message_received plugin hook failed", | ||
| ); | ||
| } | ||
| if (run.run.sessionKey) { | ||
| fireAndForgetHook( | ||
| triggerInternalHook( | ||
| createInternalHookEvent("message", "received", run.run.sessionKey, { | ||
| ...toInternalMessageReceivedContext(canonical), | ||
| timestamp: canonical.timestamp, | ||
| }), | ||
| ), | ||
| "queue/enqueue: message_received internal hook failed", | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
message_received for queued messages
dispatch-from-config.ts already emits both the plugin and internal message_received hooks unconditionally for every inbound non-plugin-claimed message (before calling into runPreparedReply → runReplyAgent → enqueueFollowupRun). Adding a second emission here means every message queued while the session is busy fires the hooks twice — once in dispatch and once here. The "session is active" check happens inside runReplyAgent, which is called after dispatch has already fired the hooks, so there is no guard that prevents the duplication.
To fix this, the emission in dispatch-from-config.ts (lines ~486–507) should be suppressed when the message is going to be enqueued, or the new emission here should only happen for code paths that do not go through dispatch — but currently enqueueFollowupRun has exactly one production call site and it always arrives via dispatch-from-config.ts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/reply/queue/enqueue.ts
Line: 45-71
Comment:
**Double-fire of `message_received` for queued messages**
`dispatch-from-config.ts` already emits both the plugin and internal `message_received` hooks unconditionally for every inbound non-plugin-claimed message (before calling into `runPreparedReply` → `runReplyAgent` → `enqueueFollowupRun`). Adding a second emission here means every message queued while the session is busy fires the hooks **twice** — once in dispatch and once here. The "session is active" check happens inside `runReplyAgent`, which is called *after* dispatch has already fired the hooks, so there is no guard that prevents the duplication.
To fix this, the emission in `dispatch-from-config.ts` (lines ~486–507) should be suppressed when the message is going to be enqueued, or the new emission here should only happen for code paths that do not go through dispatch — but currently `enqueueFollowupRun` has exactly one production call site and it always arrives via `dispatch-from-config.ts`.
How can I resolve this? If you propose a fix, please make it concise.![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: 3cd8abaf52
ℹ️ 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".
| } | ||
|
|
||
| queue.items.push(run); | ||
| emitQueuedMessageReceivedHooks(run); |
There was a problem hiding this comment.
dispatchReplyFromConfig already emits plugin and internal message_received hooks for each inbound message before reply resolution (src/auto-reply/reply/dispatch-from-config.ts:486-507), so calling emitQueuedMessageReceivedHooks(run) during enqueue causes the same inbound turn to fire those hooks a second time whenever active-session policy chooses enqueue-followup. In those queueing scenarios, hook consumers can now run side effects twice (for example automations, counters, or persistence) for a single user message.
Useful? React with 👍 / 👎.
1 participant
Summary\n- emit queued plugin/internal hooks when followup runs are enqueued for active sessions\n- carry canonical inbound hook context into queued followup runs so restarted drains preserve original message metadata\n- add regression coverage for direct enqueue and idle-drain restart paths\n\n## Testing\n- pnpm test src/auto-reply/reply/queue.message-received.test.ts src/auto-reply/reply/queue.drain-restart.test.ts src/auto-reply/reply/dispatch-from-config.test.ts\n- pnpm test src/auto-reply/reply/followup-runner.test.ts src/auto-reply/reply/agent-runner.runreplyagent.e2e.test.ts src/auto-reply/reply/agent-runner-direct-runtime-config.test.ts\n