fix(signal): enable inbound status reactions#72025
Conversation
Greptile SummaryThis PR wires
Confidence Score: 3/5Needs a fix before merging: the One P1 logic bug in the core changed file brings the ceiling to 4; the absence of a test for the affected branch drops it further to 3. extensions/signal/src/monitor/event-handler.ts — the try/catch/finally status reaction termination logic Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/signal/src/monitor/event-handler.ts
Line: 441-448
Comment:
**`setDone()` fires unconditionally even on the `!queuedFinal` early-return path**
When `dispatchInboundMessage` returns `queuedFinal: false`, the try block calls `statusReactions.restoreInitial()` and then `return`. Because the `finally` block always executes before the return, `statusReactions.setDone()` is also fired (since `dispatchError` is `false`). Both calls are `void` fire-and-forget, so `setDone()` races with — and typically overrides — `restoreInitial()`, leaving a ✅ done emoji on messages that received no final reply. Slack (`dispatch.ts`) and Telegram (`bot-message-dispatch.ts`) avoid this by tracking terminal state and calling `setDone`/`restoreInitial` after the try/catch block, not inside `finally`. The fix is to gate the `finally` branch on whether `queuedFinal` was reached, or restructure to match the Slack/Telegram pattern of deciding the terminal transition only after all paths are known.
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/signal/src/monitor/event-handler.status-reactions.test.ts
Line: 15-17
Comment:
**`!queuedFinal` path for status reactions is not tested**
Both test cases mock `dispatchInboundMessage` to always return `{ queuedFinal: true }`, so the early-return branch (`!queuedFinal` → `restoreInitial()`) is never exercised. Adding a third case where the mock returns `queuedFinal: false` would have caught the `finally`-block double-fire described above and would lock in the correct "no done reaction on non-final dispatch" contract.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Signal: add status reactions" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } finally { | ||
| if (statusReactions) { | ||
| if (dispatchError) { | ||
| void statusReactions.setError(); | ||
| } else { | ||
| void statusReactions.setDone(); | ||
| } | ||
| } |
There was a problem hiding this comment.
setDone() fires unconditionally even on the !queuedFinal early-return path
When dispatchInboundMessage returns queuedFinal: false, the try block calls statusReactions.restoreInitial() and then return. Because the finally block always executes before the return, statusReactions.setDone() is also fired (since dispatchError is false). Both calls are void fire-and-forget, so setDone() races with — and typically overrides — restoreInitial(), leaving a ✅ done emoji on messages that received no final reply. Slack (dispatch.ts) and Telegram (bot-message-dispatch.ts) avoid this by tracking terminal state and calling setDone/restoreInitial after the try/catch block, not inside finally. The fix is to gate the finally branch on whether queuedFinal was reached, or restructure to match the Slack/Telegram pattern of deciding the terminal transition only after all paths are known.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/signal/src/monitor/event-handler.ts
Line: 441-448
Comment:
**`setDone()` fires unconditionally even on the `!queuedFinal` early-return path**
When `dispatchInboundMessage` returns `queuedFinal: false`, the try block calls `statusReactions.restoreInitial()` and then `return`. Because the `finally` block always executes before the return, `statusReactions.setDone()` is also fired (since `dispatchError` is `false`). Both calls are `void` fire-and-forget, so `setDone()` races with — and typically overrides — `restoreInitial()`, leaving a ✅ done emoji on messages that received no final reply. Slack (`dispatch.ts`) and Telegram (`bot-message-dispatch.ts`) avoid this by tracking terminal state and calling `setDone`/`restoreInitial` after the try/catch block, not inside `finally`. The fix is to gate the `finally` branch on whether `queuedFinal` was reached, or restructure to match the Slack/Telegram pattern of deciding the terminal transition only after all paths are known.
How can I resolve this? If you propose a fix, please make it concise.| await params.replyOptions?.onToolStart?.({ name: "read_file" }); | ||
| return { queuedFinal: true, counts: { tool: 1, block: 0, final: 1 } }; | ||
| }, |
There was a problem hiding this comment.
!queuedFinal path for status reactions is not tested
Both test cases mock dispatchInboundMessage to always return { queuedFinal: true }, so the early-return branch (!queuedFinal → restoreInitial()) is never exercised. Adding a third case where the mock returns queuedFinal: false would have caught the finally-block double-fire described above and would lock in the correct "no done reaction on non-final dispatch" contract.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/signal/src/monitor/event-handler.status-reactions.test.ts
Line: 15-17
Comment:
**`!queuedFinal` path for status reactions is not tested**
Both test cases mock `dispatchInboundMessage` to always return `{ queuedFinal: true }`, so the early-return branch (`!queuedFinal` → `restoreInitial()`) is never exercised. Adding a third case where the mock returns `queuedFinal: false` would have caught the `finally`-block double-fire described above and would lock in the correct "no done reaction on non-final dispatch" contract.
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: 3cee97dfed
ℹ️ 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".
| } else { | ||
| void statusReactions.setDone(); |
There was a problem hiding this comment.
The finally block always calls statusReactions.setDone() when dispatchError is false, even when queuedFinal is false and the code just called restoreInitial() in the early-return path. In silent-success turns (no final response queued), this incorrectly shows a terminal done emoji and overrides the intended restore behavior, so users see completion feedback for turns that produced no final reply.
Useful? React with 👍 / 👎.
| isDirect: !entry.isGroup, | ||
| isGroup: entry.isGroup, | ||
| isMentionableGroup: entry.isGroup, | ||
| requireMention: false, |
There was a problem hiding this comment.
Status reaction enablement calls shouldAckReaction with requireMention: false; with the default messages.ackReactionScope of group-mentions, that gate will always return false for Signal groups, so mentioned group messages never get lifecycle reactions. This hardcoded value bypasses the channel’s real mention policy/decision and makes the new feature unexpectedly inert in common group configurations.
Useful? React with 👍 / 👎.
21895f8 to
f95f983
Compare
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close this older branch as superseded by the newer maintainer-labeled Signal status-reactions PR, which covers the same user problem with stronger terminal-state handling and real Signal proof. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Canonical path: Close this branch and use #98791 as the canonical landing path for Signal inbound status reactions. So I’m closing this here and keeping the remaining discussion on #98791. Review detailsBest possible solution: Close this branch and use #98791 as the canonical landing path for Signal inbound status reactions. Do we have a high-confidence way to reproduce the issue? Yes, source-level. Current main dispatches Signal inbound messages without status-reaction lifecycle callbacks, while sibling channels and the canonical PR show the expected controller wiring path. Is this the best way to solve the issue? No. The Signal plugin is the right owner, but this branch is not the best fix because the newer canonical PR handles visible-delivery finalization and includes live Signal proof. Security review: Security review cleared: No concrete security or supply-chain concern found; the diff is limited to Signal runtime wiring and tests with no dependency, workflow, lockfile, secret, or permission changes. AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model internal, reasoning high; reviewed against 36dd9ee3c3c1. |
f95f983 to
ed7b702
Compare
openclaw-barnacle
Bot
added
docs
bae67b8 to
bdc7cb5
Compare
openclaw-barnacle
Bot
added
docs
github-actions
Bot
added
the
dependencies-changed
openclaw-barnacle
Bot
added
channel: slack
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
Co-authored-by: Copilot <[email protected]>
|
ClawSweeper applied the proposed close for this PR.
|
|
Closing this as superseded by #98791, which is the newer canonical Signal status-reactions implementation and has now shipped/merged. Thanks for the earlier work here. |
2 participants
Summary
Describe the problem and fix in 2-5 bullets:
messages.statusReactions.enabledinto the shared status reaction lifecycle.dispatchInboundMessage, and send reactions viasendReactionSignalfor direct and group messages.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
StatusReactionControlleror passing lifecycle callbacks (onReplyStart,onToolStart, compaction callbacks, terminal state handling) intodispatchInboundMessage.messages.statusReactions.enabled.Regression Test Plan (if applicable)
extensions/signal/src/monitor/event-handler.status-reactions.test.tsmessages.statusReactions.enabledsend queued and done reactions through the correct direct recipient or groupId/targetAuthor route.User-visible / Behavior Changes
Signal now honors
messages.statusReactions.enabledfor inbound runs by updating reactions on the triggering message during the agent lifecycle.Diagram (if applicable)
Security Impact (required)
NoNoNoNoNoYes, explain risk + mitigation: N/ARepro + Verification
Environment
{ "messages": { "ackReaction": "👀", "ackReactionScope": "all", "statusReactions": { "enabled": true } }, "channels": { "signal": { "dmPolicy": "open", "allowFrom": ["*"] } } }Steps
messages.statusReactions.enabled: true.Expected
Actual
Evidence
Targeted verification:
pnpm test extensions/signal/src/monitor/event-handler.status-reactions.test.tspnpm lint:extensionsHuman Verification (required)
What you personally verified (not just CI), and how:
groupIdandtargetAuthor.Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
YesNoNoRisks and Mitigations