Skip to content

fix(hooks): emit internal message:sent for dispatcher-only reply paths#30668

Open
ou524u wants to merge 2 commits intoopenclaw:mainfrom
ou524u:fix/internal-hooks-singleton
Open

fix(hooks): emit internal message:sent for dispatcher-only reply paths#30668
ou524u wants to merge 2 commits intoopenclaw:mainfrom
ou524u:fix/internal-hooks-singleton

Conversation

@ou524u
Copy link
Copy Markdown

@ou524u ou524u commented Mar 1, 2026

Summary

This fixes missing message:sent internal hook events for reply paths that use channel-provided dispatchers (not the standard deliverOutboundPayloads flow).

In practice, Feishu uses a custom dispatcher deliver callback for streaming/card/direct sends, so deliverOutboundPayloads-level emitMessageSent is bypassed and internal hooks never see message:sent.

Root cause

  • message:sent internal hooks were emitted from delivery plumbing that is only exercised by deliverOutboundPayloads/deliverOutboundPayloadsCore.
  • Dispatcher-only paths (e.g. Feishu custom deliver callback) successfully send messages but skip that emission point.
  • Additionally, the hook handler registry (Map) was module-scoped, so when bundling splits code across multiple chunks, registerInternalHook and triggerInternalHook could reference different Map instances — register succeeds but trigger finds no handlers.

Changes

1. Emit message:sent at dispatch layer (dispatch-from-config.ts)

After successful completion (recordProcessed("completed")), emit an internal message:sent hook when:

  • sessionKey is present, and
  • at least one payload was delivered (counts.tool + counts.block + counts.final > 0).

2. Global singleton hook registry (internal-hooks.ts)

Use Symbol.for("openclaw.internal-hooks.handlers") on globalThis so that registerInternalHook and triggerInternalHook always share one Map, even across bundled chunks.

Why dispatch-from-config.ts

dispatchReplyFromConfig is the common orchestration point across both:

  • routed deliveries (routeReply), and
  • dispatcher-driven deliveries (dispatcher.sendFinalReply).

Emitting here ensures internal hooks observe completion in both paths.

Validation

  • Unit tests: 64 passed (internal-hooks + dispatch-from-config)
  • Live Feishu e2e: Consecutive messages all show:
    • message:received → 👏 CLAP reaction added
    • message:sent → ✅ OK reaction added
    • 3/3 consecutive passes confirmed by end user

Related

@ou524u ou524u marked this pull request as draft March 1, 2026 13:23
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bfaf5dd755

ℹ️ 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".

createInternalHookEvent("message", "sent", sessionKey, {
to: ctx.To ?? ctx.From ?? "",
content: "",
success: true,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Emit message:sent only after delivery outcome is known

This new hook is emitted with success: true immediately after sendFinalReply/routing bookkeeping, but sendFinalReply only confirms enqueueing and returns before delivery runs (createReplyDispatcher enqueues at src/auto-reply/reply/reply-dispatcher.ts and executes deliver asynchronously). In any transport failure after enqueue, internal hooks will still receive a successful message:sent, which can trigger downstream automations/metrics as false positives.

Useful? React with 👍 / 👎.

Comment on lines +583 to +587
to: ctx.To ?? ctx.From ?? "",
content: "",
success: true,
channelId: channel,
accountId: ctx.AccountId,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use routed target fields for message:sent context

When replies are routed to a different originating channel (routeReply uses originatingChannel/originatingTo), this hook still reports to and channelId from the inbound context (ctx.To/ctx.From and channel). That produces incorrect metadata for cross-channel sends, so any internal hook filtering by destination/channel can execute against the wrong conversation.

Useful? React with 👍 / 👎.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 1, 2026

Greptile Summary

This PR successfully fixes missing message:sent internal hook events for channels that use custom dispatcher callbacks (like Feishu) instead of the standard deliverOutboundPayloads flow.

Key changes:

  • Added message:sent hook emission at the dispatch layer (dispatch-from-config.ts) after successful completion when sessionKey is present and at least one payload was delivered
  • Converted hook registry from module-scoped Map to global singleton using Symbol.for() on globalThis, fixing cross-chunk bundling issues where registration and triggering could reference different Map instances
  • Added test coverage for global singleton registry behavior

Implementation notes:

  • Hook emission at dispatch level uses empty content field (reasonable since multiple payloads may have been sent)
  • Channels using deliverOutboundPayloads may emit message:sent twice (once from deliver.ts with content, once from dispatch-from-config.ts without) — hooks should be idempotent as noted in comments
  • Emission occurs after queuing payloads for delivery, not after confirmed delivery (consistent with existing behavior)

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • Well-tested changes with clear intent. The global singleton pattern correctly solves the bundling issue, and the dispatch-level hook emission ensures coverage for all reply paths. E2E validation confirms the fix works in production. The duplicate emission scenario is acknowledged and acceptable given the architecture.
  • No files require special attention

Last reviewed commit: deb5208

…ndition

- Use Symbol.for('openclaw.internal-hooks.handlers') on globalThis so
  register and trigger always share one Map even across bundled chunks.
- Widen message:sent trigger from (queuedFinal || routedFinalCount > 0)
  to (deliveredCount > 0) covering tool/block/final counts.
- Add singleton registry test.
@ou524u ou524u marked this pull request as ready for review March 1, 2026 14:38
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: deb5208616

ℹ️ 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".

Comment on lines +580 to +581
const deliveredCount = counts.tool + counts.block + counts.final;
if (sessionKey && deliveredCount > 0) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exclude unsent routed payloads from message:sent emission

The new deliveredCount > 0 gate can report a sent message even when nothing was actually delivered on routed paths: routeReply returns ok: true for payloads that normalize to empty and are skipped (src/auto-reply/reply/route-reply.ts:85-99), and those ok results are counted before this hook is fired. In that case, message:sent is emitted with success: true although no outbound message was sent, which can trigger false positives in internal automations/metrics for cross-channel routing.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant