fix(telegram): emit sent hooks for preview finals#50938
fix(telegram): emit sent hooks for preview finals#50938xiwuqi wants to merge 2 commits intoopenclaw:mainfrom
Conversation
openclaw-barnacle
bot
added
channel: telegram
Greptile SummaryThis PR fixes a long-standing gap where Telegram DM replies that complete by finalising or retaining a streaming preview never emitted Key observations:
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/telegram/src/bot-message-dispatch.ts
Line: 511-522
Comment:
**`async` without any `await`**
`emitDeliveredReplyHooks` returns `void` (it uses `fireAndForgetHook` internally), so marking this callback `async` adds no value — the returned Promise resolves immediately on the next microtick. Future readers may expect the hook emissions to be awaited before the caller continues, which is not what happens.
```suggestion
onFinalPreviewDelivered: ({ text, messageId }) => {
emitDeliveredReplyHooks({
sessionKeyForInternalHooks: ctxPayload.SessionKey,
chatId: String(chatId),
accountId: route.accountId,
content: text,
success: true,
messageId,
isGroup,
groupId: isGroup ? String(chatId) : undefined,
});
},
```
If the callback signature in `CreateLaneTextDelivererParams` also needs updating:
```ts
// lane-delivery-text-deliverer.ts
onFinalPreviewDelivered?: (params: {
laneName: LaneName;
text: string;
messageId?: number;
}) => void;
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/telegram/src/bot/delivery.replies.ts
Line: 560-565
Comment:
**`getGlobalHookRunner()` called on every invocation**
The previous `deliverReplies` code captured `hookRunner` and `hasMessageSentHooks` once before the delivery loop. Now that `emitDeliveredReplyHooks` is the shared entry point, both `getGlobalHookRunner()` and `hasHooks("message_sent")` are re-evaluated on every call — once per reply chunk in `deliverReplies`, and once for each final-preview path in the lane deliverer. This is a minor inefficiency. If `getGlobalHookRunner()` is cheap/returns a singleton it doesn't matter, but it is worth noting for future maintainers.
No code change required unless `getGlobalHookRunner()` turns out to have observable cost, but consider a brief comment explaining the intentional fire-and-forget semantics here to prevent a well-meaning refactor from accidentally turning this into a blocking call.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "fix(telegram): emit ..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| onFinalPreviewDelivered: async ({ text, messageId }) => { | ||
| emitDeliveredReplyHooks({ | ||
| sessionKeyForInternalHooks: ctxPayload.SessionKey, | ||
| chatId: String(chatId), | ||
| accountId: route.accountId, | ||
| content: text, | ||
| success: true, | ||
| messageId, | ||
| isGroup, | ||
| groupId: isGroup ? String(chatId) : undefined, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
async without any await
emitDeliveredReplyHooks returns void (it uses fireAndForgetHook internally), so marking this callback async adds no value — the returned Promise resolves immediately on the next microtick. Future readers may expect the hook emissions to be awaited before the caller continues, which is not what happens.
| onFinalPreviewDelivered: async ({ text, messageId }) => { | |
| emitDeliveredReplyHooks({ | |
| sessionKeyForInternalHooks: ctxPayload.SessionKey, | |
| chatId: String(chatId), | |
| accountId: route.accountId, | |
| content: text, | |
| success: true, | |
| messageId, | |
| isGroup, | |
| groupId: isGroup ? String(chatId) : undefined, | |
| }); | |
| }, | |
| onFinalPreviewDelivered: ({ text, messageId }) => { | |
| emitDeliveredReplyHooks({ | |
| sessionKeyForInternalHooks: ctxPayload.SessionKey, | |
| chatId: String(chatId), | |
| accountId: route.accountId, | |
| content: text, | |
| success: true, | |
| messageId, | |
| isGroup, | |
| groupId: isGroup ? String(chatId) : undefined, | |
| }); | |
| }, |
If the callback signature in CreateLaneTextDelivererParams also needs updating:
// lane-delivery-text-deliverer.ts
onFinalPreviewDelivered?: (params: {
laneName: LaneName;
text: string;
messageId?: number;
}) => void;Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/telegram/src/bot-message-dispatch.ts
Line: 511-522
Comment:
**`async` without any `await`**
`emitDeliveredReplyHooks` returns `void` (it uses `fireAndForgetHook` internally), so marking this callback `async` adds no value — the returned Promise resolves immediately on the next microtick. Future readers may expect the hook emissions to be awaited before the caller continues, which is not what happens.
```suggestion
onFinalPreviewDelivered: ({ text, messageId }) => {
emitDeliveredReplyHooks({
sessionKeyForInternalHooks: ctxPayload.SessionKey,
chatId: String(chatId),
accountId: route.accountId,
content: text,
success: true,
messageId,
isGroup,
groupId: isGroup ? String(chatId) : undefined,
});
},
```
If the callback signature in `CreateLaneTextDelivererParams` also needs updating:
```ts
// lane-delivery-text-deliverer.ts
onFinalPreviewDelivered?: (params: {
laneName: LaneName;
text: string;
messageId?: number;
}) => void;
```
How can I resolve this? If you propose a fix, please make it concise.| const hookRunner = getGlobalHookRunner(); | ||
| emitMessageSentHooks({ | ||
| hookRunner, | ||
| enabled: hookRunner?.hasHooks("message_sent") ?? false, | ||
| ...params, | ||
| }); |
There was a problem hiding this comment.
getGlobalHookRunner() called on every invocation
The previous deliverReplies code captured hookRunner and hasMessageSentHooks once before the delivery loop. Now that emitDeliveredReplyHooks is the shared entry point, both getGlobalHookRunner() and hasHooks("message_sent") are re-evaluated on every call — once per reply chunk in deliverReplies, and once for each final-preview path in the lane deliverer. This is a minor inefficiency. If getGlobalHookRunner() is cheap/returns a singleton it doesn't matter, but it is worth noting for future maintainers.
No code change required unless getGlobalHookRunner() turns out to have observable cost, but consider a brief comment explaining the intentional fire-and-forget semantics here to prevent a well-meaning refactor from accidentally turning this into a blocking call.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/telegram/src/bot/delivery.replies.ts
Line: 560-565
Comment:
**`getGlobalHookRunner()` called on every invocation**
The previous `deliverReplies` code captured `hookRunner` and `hasMessageSentHooks` once before the delivery loop. Now that `emitDeliveredReplyHooks` is the shared entry point, both `getGlobalHookRunner()` and `hasHooks("message_sent")` are re-evaluated on every call — once per reply chunk in `deliverReplies`, and once for each final-preview path in the lane deliverer. This is a minor inefficiency. If `getGlobalHookRunner()` is cheap/returns a singleton it doesn't matter, but it is worth noting for future maintainers.
No code change required unless `getGlobalHookRunner()` turns out to have observable cost, but consider a brief comment explaining the intentional fire-and-forget semantics here to prevent a well-meaning refactor from accidentally turning this into a blocking call.
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: 945994445a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (shouldSkipRegressive) { | ||
| params.markDelivered(); | ||
| if (context === "final") { | ||
| await notifyFinalPreviewDelivered(laneName, text, previewMessageId); |
There was a problem hiding this comment.
When the streamed preview overshoots and the final answer is a strict prefix of lastPartialText, this branch intentionally keeps the longer preview visible and skips the edit. The new callback still emits message_sent/message:sent with the shorter final text, so any logger or automation consuming the hook will record content that the Telegram user never actually saw in this regression case.
Useful? React with 👍 / 👎.
| params.markDelivered(); | ||
| await notifyFinalPreviewDelivered(laneName, text); |
There was a problem hiding this comment.
This branch only knows that a preview send was attempted and the response was lost (sendMayHaveLanded()), so it keeps the existing preview specifically to avoid a duplicate. Emitting a successful sent hook with the new final text here can falsely claim delivery when Telegram may have left the old partial preview on screen, which makes downstream message_sent/message:sent consumers treat an unconfirmed send as ground truth.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b317782f84
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| params.markDelivered(); | ||
| await notifyFinalPreviewDelivered(args.laneName, args.text, args.messageId); |
There was a problem hiding this comment.
When this branch keeps an alternate visible preview because the archived finalize target disappeared, the delivered message is no longer args.messageId—tryUpdatePreviewForLane() only sets retainAlternatePreviewOnMissingTarget when a different active preview is still present. Emitting notifyFinalPreviewDelivered(..., args.messageId) here makes message:sent hooks advertise the vanished archived message ID (and final text) even though users are looking at a different preview, so any hook automation that uses context.messageId to edit, pin, or react will target the wrong message.
Useful? React with 👍 / 👎.
1 participant
Summary
message:senthooks because they do not go throughdeliverReplies.message:sentmiss normal Telegram DM deliveries when preview streaming is active, so logging and automations silently lose visibility.message:senthook emitter, added a final-preview delivery callback in the lane deliverer, and invoked the hook emitter from the Telegram dispatch path whenever a final preview becomes the delivered message.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
message:senthooks now fire when a DM reply completes by finalizing or retaining a streaming preview instead of sending a new payload.Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation:Repro + Verification
Environment
Steps
message:sentinternal or plugin hook for Telegram.Expected
message:sentfires for final preview deliveries just as it does fordeliverRepliessends.Actual
message:sent.Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
deliverRepliespath still uses the same shared emitter.Review Conversations
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoFailure Recovery (if this breaks)
extensions/telegram/src/bot-message-dispatch.ts,extensions/telegram/src/bot/delivery.replies.ts,extensions/telegram/src/lane-delivery-text-deliverer.tsRisks and Mitigations
deliverReplies.deliverRepliespayload is sent, and the regression tests assert the final text is not re-sent viadeliverReplies.AI-assisted: yes. Context preparation used ContextForge before implementation.