Name a Signal reaction's target by author and sent timestamp on the v2 outbox path

cmd/r5_signal_reaction_test.go

@@ -0,0 +1,254 @@
+package cmd
+
+import (
+	"context"
+	"fmt"
+	"path/filepath"
+	"slices"
+	"strconv"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/rs/zerolog"
+
+	"github.com/maxghenis/openmessage/internal/bridge"
+	"github.com/maxghenis/openmessage/internal/bridgeadapters/scripted"
+	signaladapter "github.com/maxghenis/openmessage/internal/bridgeadapters/signal"
+	"github.com/maxghenis/openmessage/internal/db"
+	"github.com/maxghenis/openmessage/internal/ingest"
+	"github.com/maxghenis/openmessage/internal/messaging"
+	"github.com/maxghenis/openmessage/internal/signallive"
+	"github.com/maxghenis/openmessage/internal/storage/sqlite"
+	"github.com/maxghenis/openmessage/internal/v2keys"
+	"github.com/maxghenis/openmessage/internal/v2read"
+)
+
+// r5ReactingSignal gives the scripted Signal account, which has no reaction
+// sender of its own, one that records each request and confirms it.
+type r5ReactingSignal struct {
+	*scripted.Adapter
+
+	mu       sync.Mutex
+	requests []bridge.ReactionRequest
+}
+
+func (a *r5ReactingSignal) SendReaction(_ context.Context, req bridge.ReactionRequest) (bridge.SendResult, error) {
+	a.mu.Lock()
+	a.requests = append(a.requests, req)
+	a.mu.Unlock()
+	return bridge.SendResult{}, nil
+}
+
+func (a *r5ReactingSignal) reactionRequests() []bridge.ReactionRequest {
+	a.mu.Lock()
+	defer a.mu.Unlock()
+	return slices.Clone(a.requests)
+}
+
+// TestR5SignalReactionsTargetTheSentTimestampOnV2Primary runs Signal reactions
+// end to end on the quote-reply test's harness: a migrated v2-primary stack
+// with a scripted Signal account. A reaction goes through the messaging
+// service (no HTTP route submits v2 reactions; /api/react still calls the
+// legacy bridge), the durable dispatcher and the bridge request, and the
+// captured target then goes through the real Signal adapter conversion into
+// signallive's argument builder. It covers a live-ingested incoming message
+// (SHA-1 remote ID), this account's own send through the v2 outbox, and the
+// migrated messages whose Signal identity v2 cannot vouch for, which must be
+// refused: an incoming row under a decimal ID, an own "local:" row, and a
+// scheduled send caught in "sending".
+func TestR5SignalReactionsTargetTheSentTimestampOnV2Primary(t *testing.T) {
+	fixture := buildR5LegacyFixture(t)
+	now := time.Date(2026, 7, 17, 18, 0, 0, 0, time.UTC)
+	runR5Migrate(t, fixture.DataDir, filepath.Join(fixture.DataDir, "v2"), false, now)
+
+	t.Setenv("OPENMESSAGES_DATA_DIR", fixture.DataDir)
+	t.Setenv("OPENMESSAGES_DEMO", "0")
+	t.Setenv("OPENMESSAGES_APP_SANDBOX", "1")
+	t.Setenv("OPENMESSAGES_V2_PRIMARY", "1")
+	t.Setenv("OPENMESSAGES_V2_SEND", "")
+	t.Setenv("OPENMESSAGES_V2_INGEST", "")
+
+	stack, err := newV2Stack(v2StackDeps{DataDir: fixture.DataDir, Logger: zerolog.Nop()})
+	if err != nil {
+		t.Fatalf("open migrated v2 stack: %v", err)
+	}
+	t.Cleanup(func() { _ = stack.Store.Close() })
+	signal := &r5ReactingSignal{Adapter: scripted.New(r5SignalAccountID, bridge.PlatformSignal)}
+	if err := stack.RegisterAdapter(signal); err != nil {
+		t.Fatalf("register scripted Signal adapter: %v", err)
+	}
+	inertLegacy, err := db.New(":memory:")
+	if err != nil {
+		t.Fatalf("create inert legacy store: %v", err)
+	}
+	t.Cleanup(func() { _ = inertLegacy.Close() })
+	ctx, cancel := context.WithCancel(context.Background())
+	stopStack := stack.Start(ctx, inertLegacy, nil, true)
+	t.Cleanup(func() {
+		stopStack()
+		cancel()
+	})
+
+	messages, err := sqlite.NewMessageRepository(stack.Store, time.Now)
+	if err != nil {
+		t.Fatalf("NewMessageRepository(): %v", err)
+	}
+	reads := v2read.New(stack.Store)
+	signalConversation := v2keys.DeriveID("conversation", r5SignalAccountID, r5SignalConversation)
+
+	// react submits one reaction, waits for the dispatcher to hand it to the
+	// (scripted) transport, and returns the target the bridge request carried
+	// with what the Signal transport makes of it: the signal-cli target
+	// arguments, or the error it stops on before running signal-cli.
+	react := func(key, targetMessageID string) (bridge.MessageRef, []string, error) {
+		t.Helper()
+		before := len(signal.reactionRequests())
+		submission, err := stack.Service.SendReaction(context.Background(), messaging.SendReactionCommand{
+			CommonCommand: messaging.CommonCommand{
+				AccountID: r5SignalAccountID, ConversationID: signalConversation, IdempotencyKey: key,
+			},
+			TargetMessageID: targetMessageID,
+			Emoji:           "👍",
+		})
+		if err != nil {
+			t.Fatalf("SendReaction(%q): %v", targetMessageID, err)
+		}
+		waitR5(t, "reaction "+key, func() bool {
+			delivery, getErr := stack.Service.Get(context.Background(), submission.OutboxID)
+			return getErr == nil && delivery.State == messaging.OutboxConfirmed
+		})
+		requests := signal.reactionRequests()
+		if len(requests) != before+1 {
+			t.Fatalf("reaction %q reached the transport %d times, want once", key, len(requests)-before)
+		}
+		request := requests[before]
+		if request.Conversation.RemoteID != r5SignalConversation {
+			t.Fatalf("reaction %q conversation = %q, want %q", key, request.Conversation.RemoteID, r5SignalConversation)
+		}
+		args, err := signallive.ReactionTargetArgs(signaladapter.ReactionTarget(request.Target), r5SignalAccountAddress)
+		return request.Target, args, err
+	}
+	targetArgs := func(author string, timestamp int64) []string {
+		return []string{"-a", author, "-t", strconv.FormatInt(timestamp, 10)}
+	}
+
+	t.Run("reaction to a live-ingested incoming message", func(t *testing.T) {
+		const body = "r5 live incoming to react to"
+		timestamp := fixture.BaseMS + 50_000
+		line := []byte(fmt.Sprintf(
+			`{"account":%q,"envelope":{"sourceServiceId":%q,"sourceName":"R5 Signal Reacted","timestamp":%d,"dataMessage":{"timestamp":%d,"message":%q}}}`,
+			r5SignalAccountAddress, r5SignalACI, timestamp+9, timestamp, body,
+		))
+		record, ephemeral, err := ingest.BuildSignalIngress(
+			r5SignalAccountID, 1, r5SignalAccountAddress, line, "", "", time.UnixMilli(timestamp+9),
+		)
+		if err != nil || record == nil || ephemeral != nil {
+			t.Fatalf("BuildSignalIngress = %v, %v, %v", record, ephemeral, err)
+		}
+		if err := stack.Sink.AppendIngress(context.Background(), *record); err != nil {
+			t.Fatalf("AppendIngress: %v", err)
+		}
+		var incoming *db.Message
+		waitR5(t, "incoming projection", func() bool {
+			found, searchErr := reads.SearchMessagesFiltered(body, db.SearchFilter{Limit: 5})
+			if searchErr != nil || len(found) != 1 {
+				return false
+			}
+			incoming = found[0]
+			return true
+		})
+
+		target, args, err := react("r5-react-incoming", incoming.MessageID)
+		// The v2 remote ID is a SHA-1, which signal-cli's -t cannot take.
+		if want := v2keys.SignalIncomingSourceID(r5SignalConversation, r5SignalACI, timestamp); target.RemoteID != want {
+			t.Fatalf("reaction target remote ID = %q, want the SHA-1 %q", target.RemoteID, want)
+		}
+		if want := targetArgs(r5SignalACI, timestamp); err != nil || !slices.Equal(args, want) {
+			t.Fatalf("reaction target = %q, %v; want %q", args, err, want)
+		}
+	})
+
+	t.Run("reaction to a migrated incoming message under a decimal ID is refused", func(t *testing.T) {
+		// The fixture's incoming Signal row has a numeric legacy ID that is
+		// not its timestamp. No Signal receiver keys a message that way, so
+		// nothing vouches that its stored time is the one Signal knows it by:
+		// neither the ID nor the time may reach -t.
+		legacyFixture, err := db.New(fixture.StorePath)
+		if err != nil {
+			t.Fatalf("open legacy fixture store: %v", err)
+		}
+		defer legacyFixture.Close()
+		row, err := legacyFixture.GetMessageByID("signal:1700000001000")
+		if err != nil || row == nil || row.IsFromMe || row.TimestampMS == 1700000001000 {
+			t.Fatalf("legacy fixture row = %+v, %v; want an incoming row whose ID is not its timestamp", row, err)
+		}
+		v2ID := v2keys.DeriveID("message", r5SignalAccountID, r5SignalConversation+"\x1f1700000001000")
+		target, args, err := react("r5-react-migrated-incoming", v2ID)
+		if target.RemoteID != "1700000001000" || target.Outgoing || target.AuthorID != row.SenderNumber ||
+			!target.SentAt.Equal(time.UnixMilli(row.TimestampMS)) {
+			t.Fatalf("reaction target = %+v, want the migrated incoming row as stored", target)
+		}
+		if err == nil || err.Error() != "signal reaction target timestamp is unavailable" || args != nil {
+			t.Fatalf("reaction target = %q, %v; want no timestamp to send", args, err)
+		}
+	})
+
+	t.Run("reactions to migrated own messages with no known Signal timestamp are refused", func(t *testing.T) {
+		// A "local:" row keeps whatever time the legacy store held, which for
+		// a send the legacy SendText made is the wall clock after signal-cli
+		// returned. A scheduled send caught in "sending" is imported under a
+		// derived request ID with its creation time and no outbox row.
+		sendingRequestID := v2keys.DeriveID("transport_request", r5SignalAccountID, "r5-sending")
+		for _, migrated := range []struct{ name, remoteID string }{
+			{name: "local alias", remoteID: "local:abc123r5"},
+			{name: "scheduled send left in sending", remoteID: sendingRequestID},
+		} {
+			v2ID := v2keys.DeriveID("message", r5SignalAccountID, r5SignalConversation+"\x1f"+migrated.remoteID)
+			stored, err := messages.GetMessage(context.Background(), v2ID)
+			if err != nil || stored.Direction != sqlite.MessageDirectionOutgoing || stored.OccurredAtMS <= 0 {
+				t.Fatalf("migrated %s = %+v, %v; want an own message with a stored time", migrated.name, stored, err)
+			}
+			target, args, err := react("r5-react-migrated-own-"+migrated.name, v2ID)
+			if target.RemoteID != migrated.remoteID || !target.Outgoing ||
+				!target.SentAt.Equal(time.UnixMilli(stored.OccurredAtMS)) {
+				t.Fatalf("reaction target for migrated %s = %+v, want the own message as stored", migrated.name, target)
+			}
+			if err == nil || err.Error() != "signal reaction target timestamp is unavailable" || args != nil {
+				t.Fatalf("reaction target for migrated %s = %q, %v; want no timestamp to send", migrated.name, args, err)
+			}
+		}
+	})
+
+	t.Run("reaction to this account's own v2 outbox send", func(t *testing.T) {
+		signal.EnqueueTextResult(bridge.SendResult{RemoteMessageID: "1752800000030"})
+		own, err := stack.Service.SendText(context.Background(), messaging.SendTextCommand{
+			CommonCommand: messaging.CommonCommand{
+				AccountID: r5SignalAccountID, ConversationID: signalConversation, IdempotencyKey: "r5-react-own-send",
+			},
+			Body: "r5 own outbox send to react to",
+		})
+		if err != nil {
+			t.Fatalf("submit own send: %v", err)
+		}
+		waitR5(t, "own send", func() bool {
+			delivery, getErr := stack.Service.Get(context.Background(), own.OutboxID)
+			return getErr == nil && delivery.State == messaging.OutboxConfirmed && delivery.RemoteMessageID == "1752800000030"
+		})
+		stored, err := messages.GetMessage(context.Background(), own.LocalMessageID)
+		if err != nil {
+			t.Fatalf("GetMessage(own send): %v", err)
+		}
+
+		target, args, err := react("r5-react-own", own.LocalMessageID)
+		// The stored occurred time is the submit time; the reaction targets
+		// the timestamp signal-cli reported for the send.
+		if !target.Outgoing || !target.SentAt.Equal(time.UnixMilli(stored.OccurredAtMS)) ||
+			stored.OccurredAtMS == 1752800000030 {
+			t.Fatalf("own send target = %+v, stored occurred at %d; want the submit time", target, stored.OccurredAtMS)
+		}
+		if want := targetArgs(r5SignalAccountAddress, 1752800000030); err != nil || !slices.Equal(args, want) {
+			t.Fatalf("reaction target = %q, %v; want %q", args, err, want)
+		}
+	})
+}

docs/agent-runbook.md

@@ -1504,6 +1504,83 @@ Rules worth knowing when a quote looks wrong:
   target by its legacy ID (`signal:<ts>`) and records no sender or attachment.
   Such an ID still quotes from its legacy row when that row exists.
 
+### Signal reactions through the v2 outbox
+
+Signal names the message a reaction targets by its author and sent timestamp
+(`signal-cli sendReaction -a <author> -t <timestamp>`), never by an ID
+OpenMessage stores. The dispatcher (`messaging.targetRefForLease`) puts the
+target's sender, direction (`Outgoing`) and `occurred_at_ms` on the request's
+`bridge.MessageRef`, and `signallive.ReactionTargetArgs` sends a reaction only
+when the store vouches for both halves of that name:
+
+- **An incoming message** is named by its stored sender and `occurred_at_ms`,
+  and only when its remote ID is a SHA-1 (40 hex digits,
+  `v2keys.SignalIncomingSourceID`). That is the ID a Signal receiver gives a
+  message it keyed by sender and sent timestamp, and each writer that uses it
+  stores that timestamp as the occurred time: the v2 decoder, the legacy
+  receiver, and a Signal Desktop import of a row that has a sent time. The ID
+  itself is not sent.
+- **A message this account sent** is named by this account and its remote ID,
+  and only when that ID is a decimal timestamp. That is how a send Signal
+  accepted is stored when its timestamp was kept: an outbox confirm sets the ID
+  to the timestamp signal-cli reported, a sync message from the phone arrives
+  under its own, and the legacy `SendMedia` and legacy-primary projector keyed
+  their rows by signal-cli's. Its `occurred_at_ms` is not used. This is stricter than the
+  quote rule above, because a quote carries the quoted text with it and a
+  reaction carries nothing but the name.
+
+Anything else stops before signal-cli runs. The row goes `not_dispatched`
+(`error_class: transient`, `error_code: send_reaction` on
+`GET /api/v1/outbox/<id>`) and retries every 5 s with no cap. The reason is in
+the `error_detail` column of the `outbox` table in the v2 store:
+
+- `signal reaction target timestamp is unavailable`, for this account's own
+  message: its remote ID is not a Signal timestamp.
+  - *Still on its outbox request ID.* A pending send: the reaction goes out on
+    a retry once the send is confirmed. A canceled or failed send: never. An
+    uncertain one: not while it is unresolved. Also a send Signal accepted
+    whose local confirm failed (`store_failed`): the outbox row holds the
+    timestamp as its result, but the message is not moved to it until that
+    confirm is repaired (`RepairStoreFailed`).
+  - *A migrated `local:<sha1>` message.* The legacy `SendText` stamped its row
+    with the wall clock after signal-cli returned, not with Signal's
+    timestamp. The phone's own sends were stored under the same alias with the
+    right time, and nothing tells the two apart, so none of them can be
+    reacted to through the outbox.
+  - *A migrated scheduled send caught in `sending`*: imported under a derived
+    request ID with its creation time and no outbox row.
+- `signal reaction target timestamp is unavailable`, for an incoming message:
+  its remote ID is not a SHA-1, or it has no occurred time.
+  - *`received:<sha1>`*: a Signal Desktop row that had no sent time. The
+    importer stores it under the time it was received and marks its source ID
+    (`v2keys.SignalReceivedSourceID`).
+  - *`signal:<id>`*: a legacy ID the legacy-primary mirror kept.
+- `signal reaction target author is unavailable`: an incoming message stored
+  with no sender. The legacy receiver stored a group message with no source
+  that way, and the legacy-primary mirror (`v2wire.MirrorReplyTarget`) records
+  none.
+
+A reaction that can never be named keeps retrying until it is canceled
+(`POST /api/v1/outbox/<id>/cancel`).
+
+One gap remains that nothing can mark. Before the importer marked them, a
+Signal Desktop row with no sent time was stored under a plain SHA-1 of its
+received time. A re-import rewrites that source ID in the legacy store, but a
+v2 store migrated before then still holds the row under the unmarked hash,
+indistinguishable from a message keyed by its sent timestamp. A reaction to
+it names the received time.
+
+Before this the transport passed the v2 remote ID to `-t`. For a decoded
+incoming message that is a SHA-1. signal-cli 0.14.8 refuses a non-integer
+while parsing its arguments (`could not convert '<id>' to integer (64 bits)`,
+exit 1), and the row ended `uncertain` with nothing sent.
+
+No surface submits a reaction to the v2 outbox yet. The web UI's `/api/react`
+and the `react_to_message` MCP tool both call the legacy
+`signallive.Bridge.SendReaction`, which looks the target up in the legacy
+`messages.db` by message ID. Only `MessageService.SendReaction` (tests, so
+far) reaches the path above.
+
 ## Deploying a new build to a live install
 
 **`RELEASE=1` is required.** Without it `build.sh` stamps the dev bundle id

internal/bridge/contracts.go

@@ -64,15 +64,20 @@ type ConversationRef struct {
 // read-receipt target, or the message a reply quotes. RemoteID is the
 // transport's identity for it. The dispatcher fills the other fields from the
 // stored message. AuthorID is the author's canonical identity. It is empty
-// when the stored message names no sender, which is always the case for a
-// message this account sent, and adapters read empty as this account. SentAt
-// is the message's occurred time. A zero
-// SentAt means the dispatcher had no stored message to describe (it does not
-// hold one under RemoteID, or the message is an outgoing one still waiting
-// for its transport ID), so only RemoteID is meaningful.
+// when the stored message names no sender. That is always the case for a
+// message this account sent, but an incoming message can lack a sender too
+// (the legacy Signal receiver stored group messages with no source, and the
+// legacy-primary mirror records none), so an empty AuthorID does not by itself
+// mean this account. Outgoing reports that this account sent the message.
+// SentAt is the message's occurred time. Reaction and read refs always
+// describe the message they name. A reply ref with a zero SentAt is
+// undescribed (the dispatcher holds no message under RemoteID, or the quoted
+// message is an outgoing one still on its outbox request ID), so only its
+// RemoteID is meaningful.
 type MessageRef struct {
 	RemoteID string
 	AuthorID string
+	Outgoing bool
 	SentAt   time.Time
 	// Text, HasAttachment and AttachmentMIME are filled for reply targets
 	// only. Text is the stored body. HasAttachment reports whether the message