refactor(signal): unify reaction auth flow and table-drive tests

2026-03-07 22:44:16 +00:00 · 2026-02-26 01:01:58 +01:00
parent f83719937a
commit c6dfa26f03
2 changed files with 133 additions and 114 deletions
--- a/src/signal/monitor.tool-result.sends-tool-summaries-responseprefix.test.ts
+++ b/src/signal/monitor.tool-result.sends-tool-summaries-responseprefix.test.ts
@@ -378,65 +378,49 @@ describe("monitorSignalProvider tool results", () => {
    expect(events.some((text) => text.includes("Signal reaction added"))).toBe(true);
  });

-  it("blocks reaction notifications from unauthorized senders when dmPolicy is allowlist", async () => {
-    setReactionNotificationConfig("all", {
-      dmPolicy: "allowlist",
-      allowFrom: ["+15550007777"],
-    });
+  it.each([
+    {
+      name: "blocks reaction notifications from unauthorized senders when dmPolicy is allowlist",
+      mode: "all" as const,
+      extra: { dmPolicy: "allowlist", allowFrom: ["+15550007777"] } as Record<string, unknown>,
+      targetAuthor: "+15550002222",
+      shouldEnqueue: false,
+    },
+    {
+      name: "blocks reaction notifications from unauthorized senders when dmPolicy is pairing",
+      mode: "own" as const,
+      extra: {
+        dmPolicy: "pairing",
+        allowFrom: [],
+        account: "+15550009999",
+      } as Record<string, unknown>,
+      targetAuthor: "+15550009999",
+      shouldEnqueue: false,
+    },
+    {
+      name: "allows reaction notifications for allowlisted senders when dmPolicy is allowlist",
+      mode: "all" as const,
+      extra: { dmPolicy: "allowlist", allowFrom: ["+15550001111"] } as Record<string, unknown>,
+      targetAuthor: "+15550002222",
+      shouldEnqueue: true,
+    },
+  ])("$name", async ({ mode, extra, targetAuthor, shouldEnqueue }) => {
+    setReactionNotificationConfig(mode, extra);
    await receiveSingleEnvelope({
      ...makeBaseEnvelope(),
      reactionMessage: {
        emoji: "✅",
-        targetAuthor: "+15550002222",
+        targetAuthor,
        targetSentTimestamp: 2,
      },
    });

    const events = getDirectSignalEventsFor("+15550001111");
-    expect(events.some((text) => text.includes("Signal reaction added"))).toBe(false);
+    expect(events.some((text) => text.includes("Signal reaction added"))).toBe(shouldEnqueue);
    expect(sendMock).not.toHaveBeenCalled();
    expect(upsertPairingRequestMock).not.toHaveBeenCalled();
  });

-  it("blocks reaction notifications from unauthorized senders when dmPolicy is pairing", async () => {
-    setReactionNotificationConfig("own", {
-      dmPolicy: "pairing",
-      allowFrom: [],
-      account: "+15550009999",
-    });
-    await receiveSingleEnvelope({
-      ...makeBaseEnvelope(),
-      reactionMessage: {
-        emoji: "✅",
-        targetAuthor: "+15550009999",
-        targetSentTimestamp: 2,
-      },
-    });
-
-    const events = getDirectSignalEventsFor("+15550001111");
-    expect(events.some((text) => text.includes("Signal reaction added"))).toBe(false);
-    expect(sendMock).not.toHaveBeenCalled();
-    expect(upsertPairingRequestMock).not.toHaveBeenCalled();
-  });
-
-  it("allows reaction notifications for allowlisted senders when dmPolicy is allowlist", async () => {
-    setReactionNotificationConfig("all", {
-      dmPolicy: "allowlist",
-      allowFrom: ["+15550001111"],
-    });
-    await receiveSingleEnvelope({
-      ...makeBaseEnvelope(),
-      reactionMessage: {
-        emoji: "✅",
-        targetAuthor: "+15550002222",
-        targetSentTimestamp: 2,
-      },
-    });
-
-    const events = getDirectSignalEventsFor("+15550001111");
-    expect(events.some((text) => text.includes("Signal reaction added"))).toBe(true);
-  });
-
  it("notifies on own reactions when target includes uuid + phone", async () => {
    setReactionNotificationConfig("own", { account: "+15550002222" });
    await receiveSingleEnvelope({
--- a/src/signal/monitor/event-handler.ts
+++ b/src/signal/monitor/event-handler.ts
@@ -49,9 +49,15 @@ import {
  resolveSignalPeerId,
  resolveSignalRecipient,
  resolveSignalSender,
+  type SignalSender,
 } from "../identity.js";
 import { sendMessageSignal, sendReadReceiptSignal, sendTypingSignal } from "../send.js";
-import type { SignalEventHandlerDeps, SignalReceivePayload } from "./event-handler.types.js";
+import type {
+  SignalEnvelope,
+  SignalEventHandlerDeps,
+  SignalReactionMessage,
+  SignalReceivePayload,
+} from "./event-handler.types.js";
 import { renderSignalMentions } from "./mentions.js";
 export function createSignalEventHandler(deps: SignalEventHandlerDeps) {
  const inboundDebounceMs = resolveInboundDebounceMs({ cfg: deps.cfg, channel: "signal" });
@@ -321,6 +327,85 @@ export function createSignalEventHandler(deps: SignalEventHandlerDeps) {
    },
  });

+  function handleReactionOnlyInbound(params: {
+    envelope: SignalEnvelope;
+    sender: SignalSender;
+    senderDisplay: string;
+    reaction: SignalReactionMessage;
+    hasBodyContent: boolean;
+    resolveAccessDecision: (isGroup: boolean) => {
+      decision: "allow" | "block" | "pairing";
+      reason: string;
+    };
+  }): boolean {
+    if (params.hasBodyContent) {
+      return false;
+    }
+    if (params.reaction.isRemove) {
+      return true; // Ignore reaction removals
+    }
+    const emojiLabel = params.reaction.emoji?.trim() || "emoji";
+    const senderName = params.envelope.sourceName ?? params.senderDisplay;
+    logVerbose(`signal reaction: ${emojiLabel} from ${senderName}`);
+    const groupId = params.reaction.groupInfo?.groupId ?? undefined;
+    const groupName = params.reaction.groupInfo?.groupName ?? undefined;
+    const isGroup = Boolean(groupId);
+    const reactionAccess = params.resolveAccessDecision(isGroup);
+    if (reactionAccess.decision !== "allow") {
+      logVerbose(
+        `Blocked signal reaction sender ${params.senderDisplay} (${reactionAccess.reason})`,
+      );
+      return true;
+    }
+    const targets = deps.resolveSignalReactionTargets(params.reaction);
+    const shouldNotify = deps.shouldEmitSignalReactionNotification({
+      mode: deps.reactionMode,
+      account: deps.account,
+      targets,
+      sender: params.sender,
+      allowlist: deps.reactionAllowlist,
+    });
+    if (!shouldNotify) {
+      return true;
+    }
+
+    const senderPeerId = resolveSignalPeerId(params.sender);
+    const route = resolveAgentRoute({
+      cfg: deps.cfg,
+      channel: "signal",
+      accountId: deps.accountId,
+      peer: {
+        kind: isGroup ? "group" : "direct",
+        id: isGroup ? (groupId ?? "unknown") : senderPeerId,
+      },
+    });
+    const groupLabel = isGroup ? `${groupName ?? "Signal Group"} id:${groupId}` : undefined;
+    const messageId = params.reaction.targetSentTimestamp
+      ? String(params.reaction.targetSentTimestamp)
+      : "unknown";
+    const text = deps.buildSignalReactionSystemEventText({
+      emojiLabel,
+      actorLabel: senderName,
+      messageId,
+      targetLabel: targets[0]?.display,
+      groupLabel,
+    });
+    const senderId = formatSignalSenderId(params.sender);
+    const contextKey = [
+      "signal",
+      "reaction",
+      "added",
+      messageId,
+      senderId,
+      emojiLabel,
+      groupId ?? "",
+    ]
+      .filter(Boolean)
+      .join(":");
+    enqueueSystemEvent(text, { sessionKey: route.sessionKey, contextKey });
+    return true;
+  }
+
  return async (event: { event?: string; data?: string }) => {
    if (event.event !== "receive" || !event.data) {
      return;
@@ -375,13 +460,13 @@ export function createSignalEventHandler(deps: SignalEventHandlerDeps) {
      deps.dmPolicy === "allowlist"
        ? []
        : await readChannelAllowFromStore("signal").catch(() => []);
-    const { effectiveAllowFrom: effectiveDmAllow } = resolveEffectiveAllowFromLists({
-      allowFrom: deps.allowFrom,
-      groupAllowFrom: deps.groupAllowFrom,
-      storeAllowFrom,
-      dmPolicy: deps.dmPolicy,
-    });
-    const effectiveGroupAllow = [...deps.groupAllowFrom, ...storeAllowFrom];
+    const { effectiveAllowFrom: effectiveDmAllow, effectiveGroupAllowFrom: effectiveGroupAllow } =
+      resolveEffectiveAllowFromLists({
+        allowFrom: deps.allowFrom,
+        groupAllowFrom: deps.groupAllowFrom,
+        storeAllowFrom,
+        dmPolicy: deps.dmPolicy,
+      });
    const resolveAccessDecision = (isGroup: boolean) =>
      resolveDmGroupAccessDecision({
        isGroup,
@@ -394,67 +479,17 @@ export function createSignalEventHandler(deps: SignalEventHandlerDeps) {
    const dmAccess = resolveAccessDecision(false);
    const dmAllowed = dmAccess.decision === "allow";

-    if (reaction && !hasBodyContent) {
-      if (reaction.isRemove) {
-        return;
-      } // Ignore reaction removals
-      const emojiLabel = reaction.emoji?.trim() || "emoji";
-      const senderName = envelope.sourceName ?? senderDisplay;
-      logVerbose(`signal reaction: ${emojiLabel} from ${senderName}`);
-      const groupId = reaction.groupInfo?.groupId ?? undefined;
-      const groupName = reaction.groupInfo?.groupName ?? undefined;
-      const isGroup = Boolean(groupId);
-      const reactionAccess = resolveAccessDecision(isGroup);
-      if (reactionAccess.decision !== "allow") {
-        logVerbose(`Blocked signal reaction sender ${senderDisplay} (${reactionAccess.reason})`);
-        return;
-      }
-      const targets = deps.resolveSignalReactionTargets(reaction);
-      const shouldNotify = deps.shouldEmitSignalReactionNotification({
-        mode: deps.reactionMode,
-        account: deps.account,
-        targets,
+    if (
+      reaction &&
+      handleReactionOnlyInbound({
+        envelope,
        sender,
-        allowlist: deps.reactionAllowlist,
-      });
-      if (!shouldNotify) {
-        return;
-      }
-
-      const senderPeerId = resolveSignalPeerId(sender);
-      const route = resolveAgentRoute({
-        cfg: deps.cfg,
-        channel: "signal",
-        accountId: deps.accountId,
-        peer: {
-          kind: isGroup ? "group" : "direct",
-          id: isGroup ? (groupId ?? "unknown") : senderPeerId,
-        },
-      });
-      const groupLabel = isGroup ? `${groupName ?? "Signal Group"} id:${groupId}` : undefined;
-      const messageId = reaction.targetSentTimestamp
-        ? String(reaction.targetSentTimestamp)
-        : "unknown";
-      const text = deps.buildSignalReactionSystemEventText({
-        emojiLabel,
-        actorLabel: senderName,
-        messageId,
-        targetLabel: targets[0]?.display,
-        groupLabel,
-      });
-      const senderId = formatSignalSenderId(sender);
-      const contextKey = [
-        "signal",
-        "reaction",
-        "added",
-        messageId,
-        senderId,
-        emojiLabel,
-        groupId ?? "",
-      ]
-        .filter(Boolean)
-        .join(":");
-      enqueueSystemEvent(text, { sessionKey: route.sessionKey, contextKey });
+        senderDisplay,
+        reaction,
+        hasBodyContent,
+        resolveAccessDecision,
+      })
+    ) {
      return;
    }
    if (!dataMessage) {