fix(imessage): native approval binding requires GUID, not numeric id

omarshahine · omarshahine · commit ab3af892ba6e · 2026-05-24T10:07:22.000-07:00
ClawSweeper third P1 review finding on #85952. approval-handler.runtime.ts deliverPending was using result.messageId as the approval-reaction binding key, but that field can be a numeric ROWID coerced to a string ('12345') when the imsg bridge returns only message_id. Inbound tapbacks carry reacted_to_guid which is always a GUID, so a numeric-id binding can never match. Fix mirrors the send.ts forwarding-path treatment: - IMessageSendResult now exposes a separate guid?: string field, populated from the same resolveOutboundMessageGuid helper send.ts already uses for the forwarding-path binding. The generic messageId field is unchanged so reply-cache, echo-cache, and receipt-building paths still see the broadest id form. - deliverPending now binds against result.guid; when it's undefined (numeric ROWID or 'ok'/'unknown' placeholders), the function returns null instead of binding against an id the inbound tapback can't possibly match. Tests: approval-handler.runtime.test.ts gets a deliverPending GUID-only binding describe block with three regression cases (numeric ROWID refused, GUID accepted, ok/unknown placeholders refused). vi.mock isolates sendMessageIMessage so the cases run synchronously without spawning imsg. 11 tests pass across handler.runtime + send specs.
diff --git a/extensions/imessage/src/approval-handler.runtime.test.ts b/extensions/imessage/src/approval-handler.runtime.test.ts
@@ -1,6 +1,14 @@
-import { describe, expect, it } from "vitest";
+import { beforeEach, describe, expect, it, vi } from "vitest";
 import { imessageApprovalNativeRuntime } from "./approval-handler.runtime.js";
 
+const sendMock = vi.hoisted(() => ({
+  sendMessageIMessage: vi.fn(),
+}));
+
+vi.mock("./send.js", () => ({
+  sendMessageIMessage: sendMock.sendMessageIMessage,
+}));
+
 describe("imessageApprovalNativeRuntime", () => {
   it("renders allowed thumbs-only reactions in pending exec approvals", async () => {
     const payload = await imessageApprovalNativeRuntime.presentation.buildPendingPayload({
@@ -142,6 +150,87 @@ describe("imessageApprovalNativeRuntime", () => {
     });
   });
 
+  describe("deliverPending GUID-only binding", () => {
+    beforeEach(() => {
+      sendMock.sendMessageIMessage.mockReset();
+    });
+
+    const baseDeliverArgs = {
+      cfg: {} as never,
+      accountId: "default",
+      context: { accountId: "default" },
+      preparedTarget: { to: "+15551230000", accountId: "default" },
+      plannedTarget: {
+        surface: "origin" as const,
+        reason: "preferred" as const,
+        target: { to: "+15551230000" },
+      },
+      request: {
+        id: "exec-1",
+        request: { command: "echo hi" },
+        createdAtMs: 0,
+        expiresAtMs: 60_000,
+      },
+      approvalKind: "exec" as const,
+      view: {
+        approvalKind: "exec",
+        approvalId: "exec-1",
+        commandText: "echo hi",
+        actions: [],
+      } as never,
+      pendingPayload: {
+        text: "Reply with: /approve exec-1 allow-once",
+        allowedDecisions: ["allow-once" as const],
+      },
+    };
+
+    it("refuses to bind when the bridge returns only a numeric ROWID", async () => {
+      // Regression for ClawSweeper P1: native deliverPending must require a
+      // GUID for the binding because inbound `reacted_to_guid` is always a
+      // GUID — never the numeric ROWID. A bridge that returns just
+      // { message_id: 12345 } has no usable approval-reaction id.
+      sendMock.sendMessageIMessage.mockResolvedValue({
+        messageId: "12345",
+        sentText: "Reply with: /approve exec-1 allow-once",
+        receipt: { kind: "text" } as never,
+      });
+
+      await expect(
+        imessageApprovalNativeRuntime.transport.deliverPending(baseDeliverArgs),
+      ).resolves.toBeNull();
+    });
+
+    it("binds against the GUID when the bridge returns one", async () => {
+      sendMock.sendMessageIMessage.mockResolvedValue({
+        messageId: "p:0/abc-123",
+        guid: "p:0/abc-123",
+        sentText: "Reply with: /approve exec-1 allow-once",
+        receipt: { kind: "text" } as never,
+      });
+
+      await expect(
+        imessageApprovalNativeRuntime.transport.deliverPending(baseDeliverArgs),
+      ).resolves.toEqual({
+        accountId: "default",
+        to: "+15551230000",
+        conversation: { handle: "+15551230000" },
+        messageId: "p:0/abc-123",
+      });
+    });
+
+    it("refuses to bind when the bridge returns 'unknown' or 'ok' placeholders", async () => {
+      sendMock.sendMessageIMessage.mockResolvedValue({
+        messageId: "ok",
+        sentText: "Reply with: /approve exec-1 allow-once",
+        receipt: { kind: "text" } as never,
+      });
+
+      await expect(
+        imessageApprovalNativeRuntime.transport.deliverPending(baseDeliverArgs),
+      ).resolves.toBeNull();
+    });
+  });
+
   it("preserves group chat targets when preparing delivery", async () => {
     await expect(
       imessageApprovalNativeRuntime.transport.prepareTarget({
diff --git a/extensions/imessage/src/approval-handler.runtime.ts b/extensions/imessage/src/approval-handler.runtime.ts
@@ -206,7 +206,12 @@ export const imessageApprovalNativeRuntime = createChannelApprovalNativeRuntimeA
         config: cfg,
         ...(preparedTarget.accountId ? { accountId: preparedTarget.accountId } : {}),
       });
-      if (!result.messageId || result.messageId === "unknown" || result.messageId === "ok") {
+      // Approval reaction bindings must use the GUID-only id (matches the
+      // inbound tapback's `reacted_to_guid`). When the bridge only returned a
+      // numeric ROWID / `ok` / `unknown`, `result.guid` is undefined — refuse
+      // to bind so the reaction shortcut won't silently miss a real tap.
+      const guid = result.guid;
+      if (!guid) {
         return null;
       }
       const conversation = buildConversationKeyForTarget(preparedTarget.to);
@@ -217,7 +222,7 @@ export const imessageApprovalNativeRuntime = createChannelApprovalNativeRuntimeA
         ...(preparedTarget.accountId ? { accountId: preparedTarget.accountId } : {}),
         to: preparedTarget.to,
         conversation,
-        messageId: result.messageId,
+        messageId: guid,
       };
     },
     updateEntry: async ({ cfg, entry, payload }) => {
diff --git a/extensions/imessage/src/send.ts b/extensions/imessage/src/send.ts
@@ -54,8 +54,22 @@ type IMessageSendOpts = {
   createClient?: (params: { cliPath: string; dbPath?: string }) => Promise<IMessageRpcClient>;
 };
 
-type IMessageSendResult = {
+export type IMessageSendResult = {
+  /**
+   * Generic identifier returned by the bridge. May be a GUID string, a
+   * numeric ROWID stringified, or the literal "ok"/"unknown" placeholders
+   * when the bridge declines to return one. Most callers (reply cache, echo
+   * cache, receipts) want this field — it is the broadest match for
+   * downstream lookups.
+   */
   messageId: string;
+  /**
+   * GUID-only identifier suitable for matching inbound `reacted_to_guid`
+   * fields. Undefined when the bridge returned only a numeric ROWID or
+   * placeholder. Approval-reaction bindings MUST use this field so the
+   * outbound key matches what the inbound tapback will surface.
+   */
+  guid?: string;
   sentText: string;
   echoText?: string;
   receipt: MessageReceipt;
@@ -301,6 +315,10 @@ export async function sendMessageIMessage(
     });
     const resolvedId = resolveMessageId(result);
     const messageId = resolvedId ?? (result?.ok ? "ok" : "unknown");
+    // GUID-only id for approval-reaction binding (inbound `reacted_to_guid`
+    // never carries a numeric ROWID, so the bind key must match). Undefined
+    // when the bridge only returned a numeric or placeholder id.
+    const approvalBindingMessageId = resolveOutboundMessageGuid(result);
     const echoScope = resolveOutboundEchoScope({ accountId: account.accountId, target });
     if (echoScope) {
       rememberPersistedIMessageEcho({
@@ -329,7 +347,6 @@ export async function sendMessageIMessage(
         isFromMe: true,
       });
       if (message) {
-        const approvalBindingMessageId = resolveOutboundMessageGuid(result);
         if (approvalBindingMessageId) {
           const handleForKey =
             target.kind === "handle" ? normalizeIMessageHandle(target.to) : undefined;
@@ -350,6 +367,7 @@ export async function sendMessageIMessage(
     }
     return {
       messageId,
+      ...(approvalBindingMessageId ? { guid: approvalBindingMessageId } : {}),
       sentText: message,
       ...(echoText ? { echoText } : {}),
       receipt: createIMessageSendReceipt({
