diff --git a/signal-reaction-debug.md b/signal-reaction-debug.md new file mode 100644 index 000000000..79999f09e --- /dev/null +++ b/signal-reaction-debug.md @@ -0,0 +1,28 @@ +Signal reaction notifications ("own" mode) investigation + +Context +- Code path: `src/signal/monitor.ts` handles reaction-only envelopes and calls `enqueueSystemEvent`. +- Default mode is "own" in config, which should notify when someone reacts to a message authored by the bot account. + +Findings +- Signal reaction handling only runs when `envelope.reactionMessage` is present and `dataMessage` is absent. If signal-cli includes `reactionMessage` alongside `dataMessage`, the reaction is ignored because the handler only runs in the `reactionMessage && !dataMessage` branch. +- `resolveSignalReactionTarget()` prefers `targetAuthorUuid` over `targetAuthor`. If signal-cli includes both (common for sender identity fields), the target becomes `kind: "uuid"`, even when a phone number is also present. +- In "own" mode, `shouldEmitSignalReactionNotification()` compares the configured `signal.account` string against the target. With a phone-configured account (e.g., `+14154668323`) and a UUID target, the check fails. +- The tests in `src/signal/monitor.tool-result.test.ts` only cover reaction payloads with `targetAuthor` (phone), so UUID-first handling is not exercised. +- Discord "own" mode compares `messageAuthorId` to `botUserId` (`shouldEmitDiscordReactionNotification`), which is strictly an ID match. The Signal implementation mirrors this pattern, but the target identity type mismatch (UUID vs E.164) breaks the comparison. + +Likely root cause +- Signal-cli reaction payloads appear to include `targetAuthorUuid` even when `targetAuthor` (phone) is present. Because `resolveSignalReactionTarget()` always prefers UUID, "own" mode never matches when `signal.account` is configured as E.164, causing no notification. + +Secondary risk +- If signal-cli includes `reactionMessage` alongside `dataMessage` (instead of reaction-only envelopes), the current handler never emits system events for reactions. + +Debug logging to confirm (if needed) +- Add a verbose log around the reaction handler to capture the identity fields and decision: + - `targetAuthor`, `targetAuthorUuid`, computed `target.kind/id`, `account`, `mode`, and `shouldNotify`. + - Log when `reactionMessage` is present but `dataMessage` is also present to verify whether reactions arrive as combined payloads. + +Suggested fixes (not applied) +- When mode is "own", compare the configured account against both `targetAuthor` (normalized E.164) and `targetAuthorUuid`, rather than selecting UUID first. +- Or, in `resolveSignalReactionTarget()`, if both values exist and the configured account is a phone number, prefer `targetAuthor` over UUID for the "own" check. +- Consider emitting reaction notifications even if `reactionMessage` and `dataMessage` coexist (guarded to avoid double-processing). diff --git a/src/signal/monitor.tool-result.test.ts b/src/signal/monitor.tool-result.test.ts index 0ba6eb130..b9a6a24bf 100644 --- a/src/signal/monitor.tool-result.test.ts +++ b/src/signal/monitor.tool-result.test.ts @@ -249,6 +249,60 @@ describe("monitorSignalProvider tool results", () => { ); }); + it("notifies on own reactions when target includes uuid + phone", async () => { + config = { + ...config, + signal: { + autoStart: false, + dmPolicy: "open", + allowFrom: ["*"], + account: "+15550002222", + reactionNotifications: "own", + }, + }; + const abortController = new AbortController(); + + streamMock.mockImplementation(async ({ onEvent }) => { + const payload = { + envelope: { + sourceNumber: "+15550001111", + sourceName: "Ada", + timestamp: 1, + reactionMessage: { + emoji: "✅", + targetAuthor: "+15550002222", + targetAuthorUuid: "123e4567-e89b-12d3-a456-426614174000", + targetSentTimestamp: 2, + }, + }, + }; + await onEvent({ + event: "receive", + data: JSON.stringify(payload), + }); + abortController.abort(); + }); + + await monitorSignalProvider({ + autoStart: false, + baseUrl: "http://127.0.0.1:8080", + abortSignal: abortController.signal, + }); + + await flush(); + + const route = resolveAgentRoute({ + cfg: config as ClawdbotConfig, + provider: "signal", + accountId: "default", + peer: { kind: "dm", id: normalizeE164("+15550001111") }, + }); + const events = peekSystemEvents(route.sessionKey); + expect(events.some((text) => text.includes("Signal reaction added"))).toBe( + true, + ); + }); + it("processes messages when reaction metadata is present", async () => { const abortController = new AbortController(); replyMock.mockResolvedValue({ text: "pong" }); diff --git a/src/signal/monitor.ts b/src/signal/monitor.ts index a5fab8022..30e34964a 100644 --- a/src/signal/monitor.ts +++ b/src/signal/monitor.ts @@ -124,36 +124,42 @@ type SignalReactionTarget = { display: string; }; -function resolveSignalReactionTarget( +function resolveSignalReactionTargets( reaction: SignalReactionMessage, -): SignalReactionTarget | null { +): SignalReactionTarget[] { + const targets: SignalReactionTarget[] = []; const uuid = reaction.targetAuthorUuid?.trim(); if (uuid) { - return { kind: "uuid", id: uuid, display: `uuid:${uuid}` }; + targets.push({ kind: "uuid", id: uuid, display: `uuid:${uuid}` }); } const author = reaction.targetAuthor?.trim(); - if (!author) return null; - const normalized = normalizeE164(author); - return { kind: "phone", id: normalized, display: normalized }; + if (author) { + const normalized = normalizeE164(author); + targets.push({ kind: "phone", id: normalized, display: normalized }); + } + return targets; } function shouldEmitSignalReactionNotification(params: { mode?: SignalReactionNotificationMode; account?: string | null; - target?: SignalReactionTarget | null; + targets?: SignalReactionTarget[]; sender?: ReturnType | null; allowlist?: string[]; }) { - const { mode, account, target, sender, allowlist } = params; + const { mode, account, targets, sender, allowlist } = params; const effectiveMode = mode ?? "own"; if (effectiveMode === "off") return false; if (effectiveMode === "own") { const accountId = account?.trim(); - if (!accountId || !target) return false; - if (target.kind === "uuid") { - return accountId === target.id || accountId === `uuid:${target.id}`; - } - return normalizeE164(accountId) === target.id; + if (!accountId || !targets || targets.length === 0) return false; + const normalizedAccount = normalizeE164(accountId); + return targets.some((target) => { + if (target.kind === "uuid") { + return accountId === target.id || accountId === `uuid:${target.id}`; + } + return normalizedAccount === target.id; + }); } if (effectiveMode === "allowlist") { if (!sender || !allowlist || allowlist.length === 0) return false; @@ -401,11 +407,11 @@ export async function monitorSignalProvider( const senderDisplay = formatSignalSenderDisplay(sender); const senderName = envelope.sourceName ?? senderDisplay; logVerbose(`signal reaction: ${emojiLabel} from ${senderName}`); - const target = resolveSignalReactionTarget(reaction); + const targets = resolveSignalReactionTargets(reaction); const shouldNotify = shouldEmitSignalReactionNotification({ mode: reactionMode, account, - target, + targets, sender, allowlist: reactionAllowlist, }); @@ -433,7 +439,7 @@ export async function monitorSignalProvider( emojiLabel, actorLabel: senderName, messageId, - targetLabel: target?.display, + targetLabel: targets[0]?.display, groupLabel, }); const senderId = formatSignalSenderId(sender);