fix(signal): fall back to toolContext.currentMessageId for reactions#32217
Conversation
Signal reactions required an explicit messageId parameter, unlike Telegram which already fell back to toolContext.currentMessageId. This made agent-initiated reactions fail on Signal because the inbound message ID was available in tool context but never used. - Destructure toolContext in Signal action handler - Fall back to toolContext.currentMessageId when messageId omitted - Update reaction schema descriptions (not Telegram-specific) - Add tests for fallback and missing-messageId rejection Closes openclaw#17651
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e36ba384d1
ℹ️ 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".
| readStringParam(params, "messageId") ?? | ||
| (toolContext?.currentMessageId != null ? String(toolContext.currentMessageId) : undefined); |
There was a problem hiding this comment.
Honor numeric messageId before falling back to tool context
When react is called with a numeric messageId (a common shape for timestamp IDs) and toolContext.currentMessageId is present, readStringParam returns undefined for the numeric value and the ?? fallback silently uses the inbound message ID instead. In that scenario the reaction is applied to the wrong message rather than the explicitly requested one. Coerce/validate messageId from both string and number before consulting the fallback.
Useful? React with 👍 / 👎.
Greptile SummaryImplemented fallback to Key changes:
The implementation is clean, surgical, and follows established patterns. The type conversions are correct, error messages are clear, and test coverage is thorough. Confidence Score: 5/5
Last reviewed commit: e36ba38 |
|
|
|
Live-patched the fix onto my running OpenClaw instance and tested Signal reactions end-to-end before the PR was even open. The toolContext.currentMessageId fallback works exactly as expected — first successful agent-initiated Signal reaction. 🔥 reacted and received. |
The followup runner (which processes queued messages) was calling runEmbeddedPiAgent without currentChannelId or currentThreadTs. This meant the message tool's toolContext had no channel routing info, causing reactions (and other target-inferred actions) to fail with 'Action react requires a target' on queued messages. Pass originatingTo as currentChannelId so the message tool can infer the reaction target from context, matching the behavior of the initial (non-queued) agent run.
|
Changelog + regression-coverage follow-up landed in #32274 after merge sequencing race:
Contributor authorship for this fix remains retained on the merged commit. |
Problem
Signal reactions required an explicit
messageIdparameter from the agent, but the agent has no reliable way to obtain it. The inbound message ID is available intoolContext.currentMessageId(populated fromMessageSid), but the Signal action handler never used it as a fallback.Telegram already implements this fallback pattern — Signal was the gap.
Closes #17651
Changes
src/channels/plugins/actions/signal.ts: DestructuretoolContextin the handler and fall back totoolContext.currentMessageIdwhenmessageIdisn't provided explicitly. Same pattern as Telegram.src/agents/tools/message-tool.ts: Updated reaction schema descriptions to remove the Telegram-specific qualifier — this fallback now works across channels.src/channels/plugins/actions/actions.test.ts: Two new tests verifying the fallback works and that missingmessageIdwithout context still rejects properly. UpdatedrunSignalActionhelper to passtoolContext.Testing
vitest run src/channels/plugins/actions/actions.test.ts)