From 613a116d97a8c5ebe2d360138a9e7f2f93c30e03 Mon Sep 17 00:00:00 2001 From: Dale Date: Mon, 2 Mar 2026 13:16:17 -0500 Subject: [PATCH] fix(signal): silence dispatch from blank reaction envelopes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: mediaKindFromMime(undefined) returns "unknown" which is truthy, so the placeholder block was unconditionally setting placeholder = "" on every message where no attachment was fetched. This caused blank-body dataMessages (e.g. Signal emoji reactions delivered by signal-cli without a reaction sub-field) to be dispatched to the agent with as the body text. Fix: only invoke mediaKindFromMime when mediaType is actually defined. When kind resolves to "unknown" or no attachment is present, fall back to "" (or empty when no attachment at all). Also adds hasBareReactionField guard: if dataMessage.reaction is present but isSignalReactionMessage() returned null, surface it as a system event rather than leaking through. Reaction-removals silently dropped. Mixed-content messages pass through normally. UAT confirmed: 👍 long-press reaction on a DM is now silently dropped with no response dispatched to the agent. 7 tests: well-formed reactions, bare reactions emitting system events, timestamp inclusion, reaction-removals, mixed content, plain messages, reaction with null-contentType attachment. --- .../event-handler.emoji-reactions.test.ts | 247 ++++++++++++++++++ .../signal/src/monitor/event-handler.ts | 107 +++++++- 2 files changed, 349 insertions(+), 5 deletions(-) create mode 100644 extensions/signal/src/monitor/event-handler.emoji-reactions.test.ts diff --git a/extensions/signal/src/monitor/event-handler.emoji-reactions.test.ts b/extensions/signal/src/monitor/event-handler.emoji-reactions.test.ts new file mode 100644 index 00000000000..690e7a2ae38 --- /dev/null +++ b/extensions/signal/src/monitor/event-handler.emoji-reactions.test.ts @@ -0,0 +1,247 @@ +/** + * Tests for Signal emoji reaction handling. + * + * When a remote user reacts to a message with an emoji (👍, ❤️, etc.), + * signal-cli sends a dataMessage with a `reaction` field. These should be + * surfaced as system events to the agent session (matching Discord's pattern) + * rather than leaking through as . + * + * Two paths: + * 1. Well-formed reactions (isSignalReactionMessage returns true) → + * handled by existing handleReactionOnlyInbound + * 2. Bare/malformed reactions (isSignalReactionMessage returns false, + * e.g. targetAuthor absent) → new hasBareReactionField guard surfaces + * as system event + */ + +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { createSignalEventHandler } from "./event-handler.js"; +import { + createBaseSignalEventHandlerDeps, + createSignalReceiveEvent, +} from "./event-handler.test-harness.js"; + +const { dispatchInboundMessageMock, enqueueSystemEventMock } = vi.hoisted(() => ({ + dispatchInboundMessageMock: vi.fn().mockResolvedValue({ + queuedFinal: false, + counts: { tool: 0, block: 0, final: 0 }, + }), + enqueueSystemEventMock: vi.fn(), +})); + +vi.mock("../send.js", () => ({ + sendMessageSignal: vi.fn(), + sendTypingSignal: vi.fn().mockResolvedValue(true), + sendReadReceiptSignal: vi.fn().mockResolvedValue(true), +})); + +vi.mock("../../../../src/auto-reply/dispatch.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + dispatchInboundMessage: dispatchInboundMessageMock, + dispatchInboundMessageWithDispatcher: dispatchInboundMessageMock, + dispatchInboundMessageWithBufferedDispatcher: dispatchInboundMessageMock, + }; +}); + +vi.mock("../../../../src/infra/system-events.js", () => ({ + enqueueSystemEvent: enqueueSystemEventMock, +})); + +vi.mock("../../../../src/pairing/pairing-store.js", () => ({ + readChannelAllowFromStore: vi.fn().mockResolvedValue([]), + upsertChannelPairingRequest: vi.fn(), +})); + +describe("signal createSignalEventHandler emoji reaction handling", () => { + beforeEach(() => { + dispatchInboundMessageMock.mockClear(); + enqueueSystemEventMock.mockClear(); + }); + + it("drops a well-formed reaction envelope via handleReactionOnlyInbound", async () => { + const deps = createBaseSignalEventHandlerDeps({ + isSignalReactionMessage: (r): r is NonNullable => + Boolean(r?.emoji && r?.targetSentTimestamp && (r?.targetAuthor || r?.targetAuthorUuid)), + }); + const handler = createSignalEventHandler(deps); + + await handler( + createSignalReceiveEvent({ + dataMessage: { + timestamp: 1700000000000, + message: "", + reaction: { + emoji: "👍", + isRemove: false, + targetAuthor: "+15550001111", + targetSentTimestamp: 1699999000000, + }, + }, + }), + ); + + expect(dispatchInboundMessageMock).not.toHaveBeenCalled(); + }); + + it("surfaces a bare reaction (missing targetAuthor) as a system event", async () => { + // isSignalReactionMessage returns false (default harness) → bare reaction guard kicks in + const deps = createBaseSignalEventHandlerDeps({ + reactionMode: "all", + shouldEmitSignalReactionNotification: () => true, + buildSignalReactionSystemEventText: (params) => + `Signal reaction added: ${params.emojiLabel} by ${params.actorLabel} msg ${params.messageId}`, + }); + const handler = createSignalEventHandler(deps); + + await handler( + createSignalReceiveEvent({ + sourceName: "Alice", + dataMessage: { + timestamp: 1700000000000, + message: "", + reaction: { + emoji: "👍", + isRemove: false, + // targetAuthor / targetAuthorUuid deliberately absent + targetSentTimestamp: 1699999000000, + }, + }, + }), + ); + + expect(dispatchInboundMessageMock).not.toHaveBeenCalled(); + expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1); + expect(enqueueSystemEventMock).toHaveBeenCalledWith( + expect.stringContaining("👍"), + expect.objectContaining({ + contextKey: expect.stringContaining("reaction"), + }), + ); + }); + + it("includes targetSentTimestamp in system event text when available", async () => { + const capturedText: string[] = []; + const deps = createBaseSignalEventHandlerDeps({ + reactionMode: "all", + shouldEmitSignalReactionNotification: () => true, + buildSignalReactionSystemEventText: (params) => { + const text = `Signal reaction added: ${params.emojiLabel} by ${params.actorLabel} msg ${params.messageId}`; + capturedText.push(text); + return text; + }, + }); + const handler = createSignalEventHandler(deps); + + await handler( + createSignalReceiveEvent({ + sourceName: "Alice", + dataMessage: { + timestamp: 1700000000000, + message: "", + reaction: { + emoji: "❤️", + isRemove: false, + targetSentTimestamp: 1699999000000, + }, + }, + }), + ); + + expect(capturedText[0]).toContain("1699999000000"); + }); + + it("drops a bare reaction-removal (isRemove: true) silently", async () => { + const deps = createBaseSignalEventHandlerDeps(); + const handler = createSignalEventHandler(deps); + + await handler( + createSignalReceiveEvent({ + dataMessage: { + timestamp: 1700000000000, + message: "", + reaction: { + emoji: "👍", + isRemove: true, + targetSentTimestamp: 1699999000000, + }, + }, + }), + ); + + expect(dispatchInboundMessageMock).not.toHaveBeenCalled(); + expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + }); + + it("handles a bare reaction that arrives with a null-contentType attachment (signal-cli thumbnail)", async () => { + const deps = createBaseSignalEventHandlerDeps({ + reactionMode: "all", + shouldEmitSignalReactionNotification: () => true, + buildSignalReactionSystemEventText: (params) => + `Signal reaction added: ${params.emojiLabel} by ${params.actorLabel}`, + }); + const handler = createSignalEventHandler(deps); + + await handler( + createSignalReceiveEvent({ + sourceName: "Alice", + dataMessage: { + timestamp: 1700000000000, + message: "", + reaction: { + emoji: "👍", + isRemove: false, + targetSentTimestamp: 1699999000000, + }, + attachments: [{ id: "thumb1", contentType: null, size: 0 }], + }, + }), + ); + + expect(dispatchInboundMessageMock).not.toHaveBeenCalled(); + expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1); + expect(enqueueSystemEventMock).toHaveBeenCalledWith( + expect.stringContaining("👍"), + expect.objectContaining({ contextKey: expect.stringContaining("reaction") }), + ); + }); + + it("does NOT intercept a reaction envelope that also has message text", async () => { + const deps = createBaseSignalEventHandlerDeps(); + const handler = createSignalEventHandler(deps); + + await handler( + createSignalReceiveEvent({ + dataMessage: { + timestamp: 1700000000000, + message: "hello", + reaction: { + emoji: "👍", + isRemove: false, + targetSentTimestamp: 1699999000000, + }, + }, + }), + ); + + // Message has body text — should be dispatched normally + expect(dispatchInboundMessageMock).toHaveBeenCalledTimes(1); + }); + + it("does NOT intercept a plain message without a reaction field", async () => { + const deps = createBaseSignalEventHandlerDeps(); + const handler = createSignalEventHandler(deps); + + await handler( + createSignalReceiveEvent({ + dataMessage: { + timestamp: 1700000000000, + message: "hey there", + }, + }), + ); + + expect(dispatchInboundMessageMock).toHaveBeenCalledTimes(1); + }); +}); diff --git a/extensions/signal/src/monitor/event-handler.ts b/extensions/signal/src/monitor/event-handler.ts index 36eb0e8d276..52b14908a0c 100644 --- a/extensions/signal/src/monitor/event-handler.ts +++ b/extensions/signal/src/monitor/event-handler.ts @@ -524,9 +524,17 @@ export function createSignalEventHandler(deps: SignalEventHandlerDeps) { const messageText = normalizedMessage.trim(); const quoteText = dataMessage?.quote?.text?.trim() ?? ""; - const hasBodyContent = - Boolean(messageText || quoteText) || Boolean(!reaction && dataMessage?.attachments?.length); - const senderDisplay = formatSignalSenderDisplay(sender); + + // Guard: if dataMessage carries a reaction field but isSignalReactionMessage() + // returned null (e.g. targetAuthor/targetAuthorUuid absent in some signal-cli versions), + // and there is no real body content, surface it as a system event (matching the + // Discord reaction pattern) rather than leaking through as . + const bareReaction = dataMessage?.reaction; + // Note: some signal-cli builds attach a null-contentType attachment alongside + // the reaction field (e.g. thumbnail of the reacted-to message). We intentionally + // omit the attachments check so those are caught here rather than leaking as + // . + // Resolve full access state early — shared by bare reaction path and normal dispatch. const { resolveAccessDecision, dmAccess, effectiveDmAllow, effectiveGroupAllow } = await resolveSignalAccessState({ accountId: deps.accountId, @@ -537,6 +545,90 @@ export function createSignalEventHandler(deps: SignalEventHandlerDeps) { sender, }); + const hasBareReactionField = !reaction && Boolean(bareReaction) && !messageText && !quoteText; + if (hasBareReactionField && bareReaction) { + const senderDisplayBare = formatSignalSenderDisplay(sender); + const emojiLabel = (bareReaction as { emoji?: string | null }).emoji?.trim() || "emoji"; + const isRemove = Boolean((bareReaction as { isRemove?: boolean | null }).isRemove); + const targetTimestamp = (bareReaction as { targetSentTimestamp?: number | null }) + .targetSentTimestamp; + logVerbose(`signal: bare reaction (${emojiLabel}) from ${senderDisplayBare}`); + if (!isRemove) { + // P2: per-field fallback so a present-but-empty reaction.groupInfo doesn't shadow + // a populated dataMessage.groupInfo (e.g. Signal emits groupInfo={} on the reaction + // envelope while the real groupId/groupName live on dataMessage.groupInfo). + const bareReactionGroupInfo = bareReaction as { + groupInfo?: { groupId?: string; groupName?: string } | null; + }; + const groupId = + bareReactionGroupInfo.groupInfo?.groupId ?? dataMessage?.groupInfo?.groupId ?? undefined; + const groupName = + bareReactionGroupInfo.groupInfo?.groupName ?? + dataMessage?.groupInfo?.groupName ?? + undefined; + const isGroup = Boolean(groupId); + // Apply full access policy (dmPolicy/groupPolicy/pairing) — same as handleReactionOnlyInbound. + const bareAccessDecision = resolveAccessDecision(isGroup); + if (bareAccessDecision.decision !== "allow") { + logVerbose( + `signal: bare reaction from unauthorized sender ${senderDisplayBare}, dropping (${bareAccessDecision.reasonCode ?? "policy"})`, + ); + return; + } + // Apply notification-mode gating (off/own/all/allowlist). + const bareReactionTargets = deps.resolveSignalReactionTargets(bareReaction); + const shouldNotifyBare = deps.shouldEmitSignalReactionNotification({ + mode: deps.reactionMode, + account: deps.account, + targets: bareReactionTargets, + sender, + allowlist: deps.reactionAllowlist, + }); + if (!shouldNotifyBare) { + logVerbose(`signal: bare reaction suppressed (reactionMode=${deps.reactionMode})`); + return; + } + const senderName = envelope.sourceName ?? senderDisplayBare; + const senderPeerIdBare = resolveSignalPeerId(sender); + const routeBare = resolveAgentRoute({ + cfg: deps.cfg, + channel: "signal", + accountId: deps.accountId, + peer: { + kind: isGroup ? "group" : "direct", + id: isGroup ? (groupId ?? "unknown") : senderPeerIdBare, + }, + }); + const messageId = typeof targetTimestamp === "number" ? String(targetTimestamp) : "unknown"; + const groupLabel = isGroup ? `${groupName ?? "Signal Group"} id:${groupId}` : undefined; + const text = deps.buildSignalReactionSystemEventText({ + emojiLabel, + actorLabel: senderName, + messageId, + groupLabel, + }); + enqueueSystemEvent(text, { + sessionKey: routeBare.sessionKey, + contextKey: [ + "signal", + "reaction", + "added", + messageId, + senderPeerIdBare, + emojiLabel, + groupId ?? "", + ] + .filter(Boolean) + .join(":"), + }); + } + return; + } + + const hasBodyContent = + Boolean(messageText || quoteText) || Boolean(!reaction && dataMessage?.attachments?.length); + const senderDisplay = formatSignalSenderDisplay(sender); + if ( reaction && handleReactionOnlyInbound({ @@ -740,12 +832,17 @@ export function createSignalEventHandler(deps: SignalEventHandlerDeps) { if (mediaPaths.length > 1) { placeholder = formatAttachmentSummaryPlaceholder(mediaTypes); } else { - const kind = kindFromMime(mediaType ?? undefined); + const kind = mediaType ? kindFromMime(mediaType) : undefined; if (kind) { placeholder = ``; - } else if (attachments.length) { + } else if (mediaPath || attachments.length) { + // A path was resolved (real attachment, unrecognised MIME), fetch + // returned null, or we are intentionally skipping fetching — preserve + // the attachment placeholder so the message is not silently lost. placeholder = ""; } + // No attachments at all (blank reaction envelope): + // Leave placeholder empty; the !bodyText guard below will drop it silently. } const bodyText = messageText || placeholder || dataMessage.quote?.text?.trim() || "";