From d2baf6aa015ad9c50eaf443df3ec5078060971b2 Mon Sep 17 00:00:00 2001 From: Preciousgift Ejere Date: Wed, 21 Oct 2026 02:22:30 +0100 Subject: [PATCH] docs: message edit/delete semantics, AI assistant bridge, push decision matrix, contract function how-to (#618, #619, #622, #635) concepts-message-edit-delete.md (#618): edit_message inserts a new linked message rather than mutating the original (editsMessageId always points at the root, not the previous edit in a chain, not a literal backward linked list despite how the read-side comment on supersededRows describes it); delete_message nulls ciphertext, hard deletes every message_envelopes row, and conditionally soft-deletes an attached file only if no other live message still references it. Documents the sender-only authorization rule for both; that a deleted message's envelope can never reach a device via /sync (both because the envelopes are already gone and because of an explicit isNull(messages.deletedAt) filter); that an edit's sibling-device requirement runs through the exact same findMissingSiblingDeviceIds send_message uses; the honest limit that delete cannot un-reveal plaintext a recipient already decrypted; and that delete_message actually double-emits message_deleted to two different room names every connected socket joins, so a client receives it twice, not once, a real finding from tracing the two io.to() calls against index.ts's connection setup, not something I assumed from the existing code comments alone. concepts-ai-assistant-bridge.md (#619): traces client event to rate-limit bucket to the hardcoded http://localhost:8000/chat call to the reply being persisted and fanned out through the same message_envelope pipeline as any other message. States plainly that the reply is stored unencrypted (same storage column named ciphertext, actually plaintext) and that the user's message leaves Clicked's infrastructure entirely and reaches OpenAI, per the AI agent's own api-chat.md. Documents the single try/catch collapsing every distinct failure mode, connection refused, non-2xx, malformed JSON, DB error, into one generic error with no way for a client to tell them apart. concepts-push-decision-matrix.md (#622): the full filter chain in pushFilter.ts as a table, confirming from presence.ts's isOnline that online status is checked per user via a single presence hash, not per device, so one online device suppresses push to every other device that user owns, before the separate per device isDeviceConnected check even runs. Layers in the 2 second coalescing window and 30 second per-device rate limit both push paths share via queueCoalescedPush, confirms the push payload carries no message content, and notes a real asymmetry: dispatchOfflinePush always passes an explicit recipientDeviceIds list but sendPushForMessage never does, so that filter stage is a no-op for the file-message path today. howto-add-contract-function.md (#635): worked example of a read-only function (is_initialized) and a state-changing one (set_paused, which also gates transfer and emits a new event) on token_transfer, chosen specifically because it is the one contract with an in-place upgrade path; both examples were actually compiled against the real contract and its existing test suite (8/8 passing) before being written up, not written from reading the code alone. Documents regenerating test_snapshots/ and that the CI check that fails if it's forgotten is simply cargo test's own storage-footprint mismatch detection, not a separate check. Covers the frontend side (building, simulating, signing, submitting via soroban.ts's existing transferToken pattern) and when the backend's Stellar listener (#331) needs its topic filter and decode logic extended for a new event, linking the events reference (#351). Added index entries for all four docs in docs/README.md, matching the existing per-doc one-line-description convention. --- .../docs/concepts-ai-assistant-bridge.md | 150 +++++++++ .../docs/concepts-message-edit-delete.md | 292 ++++++++++++++++++ .../docs/concepts-push-decision-matrix.md | 138 +++++++++ contracts/docs/howto-add-contract-function.md | 257 +++++++++++++++ docs/README.md | 4 + 5 files changed, 841 insertions(+) create mode 100644 apps/backend/docs/concepts-ai-assistant-bridge.md create mode 100644 apps/backend/docs/concepts-message-edit-delete.md create mode 100644 apps/backend/docs/concepts-push-decision-matrix.md create mode 100644 contracts/docs/howto-add-contract-function.md diff --git a/apps/backend/docs/concepts-ai-assistant-bridge.md b/apps/backend/docs/concepts-ai-assistant-bridge.md new file mode 100644 index 0000000..7fb725f --- /dev/null +++ b/apps/backend/docs/concepts-ai-assistant-bridge.md @@ -0,0 +1,150 @@ +# AI assistant bridge (`ask_assistant`) + +This document traces the full `ask_assistant` request path and states plainly what it +means for the E2EE model: **a server-authored reply is not end-to-end encrypted, and the +message content a user sends the assistant is seen in plaintext by both the backend and +a third-party LLM provider.** This is a deliberate boundary in the current design, not a +bug, but it is easy to miss from reading only the client code, so this document exists to +make it explicit. + +## Contents + +- [The request path, end to end](#the-request-path-end-to-end) +- [The hardcoded agent URL](#the-hardcoded-agent-url) +- [What plaintext leaves the client, and where it goes](#what-plaintext-leaves-the-client-and-where-it-goes) +- [Failure behavior](#failure-behavior) +- [Implementation references](#implementation-references) + +## The request path, end to end + +1. **Client emits `ask_assistant`** with `{ conversationId, content }`, where `content` + is the plaintext the user typed (not ciphertext — see + [below](#what-plaintext-leaves-the-client-and-where-it-goes) for why that matters). +2. **Trigger check**: the handler (`socket/messaging.ts`) only proceeds if + `content.trim().startsWith('@assistant')`. Anything else is silently ignored — no + error is emitted for a message that doesn't start with the trigger, since this handler + doesn't know whether the client meant to invoke it. +3. **Membership check**: the caller must be a member of `conversationId` + (`conversationMembers` lookup), or it gets + `{ event: 'ask_assistant', message: 'Not a member of this conversation' }`. +4. **Rate limit**: `consumeRateLimit('socket_ask_assistant', `user:${userId}`)` — charged + per user, not per device, because the resource being protected is the downstream AI + call, which one account could run up from any of its devices. Configured alongside + every other limit in `config/rateLimits.ts`. On rejection: + `{ event: 'rate_limited', limitedEvent: 'ask_assistant', limit, retryAfterSeconds }`. +5. **HTTP call to the AI agent**: + ```ts + const response = await fetch('http://localhost:8000/chat', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ message: content, conversation_id: conversationId }), + }); + ``` + `content` — the user's plaintext — is sent verbatim as `message`. See + [`apps/ai_agent/docs/api-chat.md`](../../ai_agent/docs/api-chat.md) (the AI agent API + reference, #302) for the full request/response contract, model (`gpt-4o-mini`), system + prompt, and failure modes of this endpoint. +6. **Ensure the assistant "user" exists**: a fixed, well-known id + (`ASSISTANT_USER_ID = '00000000-0000-4000-8000-000000000000'`) is upserted into + `users` (`ON CONFLICT (id) DO NOTHING`) and added to `conversation_members` for this + conversation if not already present. This is how the assistant's reply gets a + `senderId` and shows up as a conversation participant like any other user. +7. **Reply persisted and fanned out as a normal message**: + - `buildBroadcastEnvelopes` (`lib/messageFanout.ts`) resolves every active device of + every conversation member (the sender's own other devices included — the reply must + reach all of them, not just the device that asked) and builds one envelope per + device, **all containing the identical plaintext reply** — a server-authored + message has no per-recipient key material to vary the envelope by. + - The message and its envelopes are inserted in one transaction, with + `ciphertext: data.reply` — the field is named `ciphertext` (matching every other + message row's schema) but what's actually stored is the AI's plaintext reply, + unencrypted. + - `deliverMessage(io, replyMessage, conversationId)` runs the same fan-out pipeline + every other message goes through (see + [`concepts-delivery-fanout.md`](./concepts-delivery-fanout.md)) — recipients receive + it as an ordinary `message_envelope` + `new_message`, indistinguishable at the + transport level from an end-to-end encrypted message. + - Conversation-list caches are invalidated for every member. + +## The hardcoded agent URL + +`http://localhost:8000/chat` is a literal string in `socket/messaging.ts`, not read from +`process.env` or `config/*`. This is the only place in the codebase that addresses the AI +agent this way — contrast with every other external dependency (database, Redis, S3), +which goes through `config/index.ts`. + +**What this means for deployment**: the backend and the AI agent (`apps/ai_agent`) must +run on the same host, with the agent bound to port 8000, for `ask_assistant` to work at +all. In any topology where they're on separate hosts or containers — which is how +`docker-compose`-based deployments normally separate services — this call fails with a +connection error every time, and `ask_assistant` silently degrades to +["Failed to get AI reply"](#failure-behavior) for every user. There is no environment +variable to override this today; fixing it means adding one (e.g. `AI_AGENT_URL`) and +reading it here, the same way every other external dependency is configured. + +## What plaintext leaves the client, and where it goes + +Walking the full path by what crosses a trust boundary: + +1. **Client → backend**: the user's full message text (`content`), in plaintext, over + the `ask_assistant` socket event. This is different from `send_message`, where the + client sends opaque ciphertext the server never decrypts — `ask_assistant` is not an + E2EE message send, it's a plaintext RPC the server happens to echo back through the + messaging pipeline. +2. **Backend → AI agent**: the same plaintext, verbatim, as the `message` field of the + `/chat` request body — the backend never encrypts, transforms, or redacts it in transit. +3. **AI agent → OpenAI**: per + [`apps/ai_agent/docs/api-chat.md`](../../ai_agent/docs/api-chat.md) and + [`apps/ai_agent/docs/concepts-prompts-and-models.md`](../../ai_agent/docs/concepts-prompts-and-models.md), + the agent forwards the message to OpenAI's `gpt-4o-mini` as part of a chat completion + call, alongside a fixed system prompt. **The user's message therefore leaves the + Clicked infrastructure entirely and reaches a third-party API.** +4. **AI agent → backend → every recipient device**: the reply comes back as plaintext + and is stored and fanned out as plaintext, as described above — every device in the + conversation, not just the asker's, receives the assistant's reply in a form any of + them can read without decryption. + +**How a client should present this to the user**: any UI affordance for `@assistant` +should make clear, before the message is sent, that this is not a private, encrypted +message the way the rest of the conversation is — it is a request that leaves the +device in plaintext, is seen by the backend, is forwarded to an external AI provider, +and the reply is visible to every member of the conversation and every one of their +devices, exactly as if it had been posted as a message to the whole conversation. A user +who assumes `@assistant` messages carry the same confidentiality as the rest of an E2EE +conversation would be wrong, and the UI should not imply otherwise — e.g. by using the +same compose box and send affordance with no visual distinction. + +This document is the privacy-boundary reference (#355) for this specific feature. For +how the AI agent itself handles the data once received (model, prompt, logging, retention +on that side), see the `apps/ai_agent/docs/` set linked above and below. + +## Failure behavior + +Everything from the HTTP call onward is inside one `try`/`catch`. Any failure — +connection refused (the hardcoded-URL case above), a non-2xx response (`response.ok` +check throws a generic `Error('AI agent error')`), a malformed JSON body, or a database +error during the insert — is caught and reduces to the same single outcome: + +```ts +console.error('ask_assistant error:', err); +socket.emit('error', { event: 'ask_assistant', message: 'Failed to get AI reply' }); +``` + +There is no distinction surfaced to the client between "the agent is unreachable," +"the agent returned an error," "the agent was slow" (the agent enforces its own 30-second +request timeout per `api-chat.md`, but nothing here imposes a client-side timeout of its +own — a hung `fetch` would hang this handler for however long the underlying TCP +connection takes to fail, if ever), and "something else failed after a successful AI +response" (e.g. the database insert). A caller cannot tell from the error alone whether +retrying is likely to help. + +## Implementation references + +- `apps/backend/src/socket/messaging.ts` — `ask_assistant` handler, `ASSISTANT_USER_ID` +- `apps/backend/src/lib/messageFanout.ts` — `buildBroadcastEnvelopes` +- `apps/backend/src/config/rateLimits.ts` — `socket_ask_assistant` bucket +- `apps/backend/src/services/deliveryPipeline.ts` — `deliverMessage`, the fan-out the reply goes through +- `apps/ai_agent/main.py` — the `/chat` endpoint implementation +- [`apps/ai_agent/docs/api-chat.md`](../../ai_agent/docs/api-chat.md) — AI agent API reference (#302) +- [`apps/ai_agent/docs/concepts-prompts-and-models.md`](../../ai_agent/docs/concepts-prompts-and-models.md) — system prompt and model configuration +- [`concepts-delivery-fanout.md`](./concepts-delivery-fanout.md) — the fan-out pipeline the assistant's reply is delivered through like any other message diff --git a/apps/backend/docs/concepts-message-edit-delete.md b/apps/backend/docs/concepts-message-edit-delete.md new file mode 100644 index 0000000..fc0b46c --- /dev/null +++ b/apps/backend/docs/concepts-message-edit-delete.md @@ -0,0 +1,292 @@ +# Message edit and delete semantics + +This document describes what `edit_message` and `delete_message` actually do to the +database and to every connected client, and the honest limits of both under the server's +"ciphertext only, per-device" storage model. It is written against the code as it +currently behaves (`apps/backend/src/socket/messaging.ts`), not an idealized design. + +## Contents + +- [Authorization](#authorization) +- [`edit_message`](#edit_message) +- [`delete_message`](#delete_message) +- [What an offline device sees on reconnect / `/sync`](#what-an-offline-device-sees-on-reconnect--sync) +- [The honest limit of delete under E2EE](#the-honest-limit-of-delete-under-e2ee) +- [Interaction with file GC](#interaction-with-file-gc) +- [Interaction with local search indexes](#interaction-with-local-search-indexes) +- [Implementation references](#implementation-references) + +## Authorization + +Both handlers apply the same rule: **only the original sender may act on a message.** + +- `edit_message` loads the message at `originalMessageId` and rejects with + `{ event: 'edit_message', message: 'Only the original sender can edit this message' }` + if `original.senderId !== userId`. +- `delete_message` loads the message at `messageId` and rejects with + `{ event: 'delete_message', message: 'Message not found or not sender' }` if it + doesn't exist or `message.senderId !== userId` — the same error for both cases, so a + client can't distinguish "not found" from "not yours" by the error alone. + +Neither handler checks conversation membership separately; ownership of the message +(`senderId`) is the only gate. + +## `edit_message` + +An edit is **not** an in-place mutation. It inserts a brand-new `messages` row with a +fresh per-device envelope fan-out, and links it back to the message it replaces via +`editsMessageId`: + +```ts +const rootMessageId = original.editsMessageId ?? original.id; +// ... +await tx.insert(messages).values({ + id: messageId, + conversationId, + senderId: userId, + senderDeviceId: deviceId, + contentType: contentType || original.contentType, + ciphertext: ciphertext || null, + editsMessageId: rootMessageId, +}); +await insertMessageEnvelopes(tx, messageId, envelopes); +``` + +Two details that matter for anyone reasoning about edit chains: + +- **`editsMessageId` always points at the root, not the previous edit.** If a message is + edited twice, the second edit's `editsMessageId` is the *original* message's id, not the + first edit's id — `rootMessageId` is resolved by following `original.editsMessageId` one + hop, not by walking a chain. This means looking up "every version of this message" is a + single `WHERE editsMessageId = :rootId OR id = :rootId` query, not a recursive one. See + `routes/conversations.ts`'s comment on `supersededRows` for the read side of this: the + set of every id referenced by some row's `editsMessageId` is exactly the set of + non-latest versions, collapsed out of the default message list in one extra query. +- **An edit is validated exactly like a new `send_message`**, not like a lightweight + patch: it requires non-empty `ciphertext`, runs `checkEnvelopeSizes`, and — the + requirement called out separately below — full sibling-device envelope coverage. The + only field it can omit is `contentType`, which falls back to the original message's. + +Validation order, all of which can reject before anything is persisted: + +1. `findForbiddenSessionStateField` — rejects if the payload contains session/ratchet/ + private-key fields; the server never accepts those, only opaque ciphertext and public + routing metadata. +2. `originalMessageId` and `messageId` both required. +3. `ciphertext` must be non-empty after trimming. +4. `checkEnvelopeSizes` — per-device envelope size limit. +5. The message at `originalMessageId` must exist and belong to the caller (see + [Authorization](#authorization)). +6. **Idempotency**: if a message already exists at `messageId`, the handler re-emits + `message_ack` for it and returns — it does not re-insert or re-validate anything, + including sibling coverage. A retried edit with the same `messageId` is a no-op past + this point even if its payload differs from the first attempt. +7. Sibling-device coverage (next section). + +### Sibling-device requirement + +Before persisting, the handler calls the exact same function `send_message` calls: + +```ts +const missingSiblings = await findMissingSiblingDeviceIds(userId, deviceId, envelopes); +if (missingSiblings.length > 0) { + socket.emit('error', { + event: 'device_set_mismatch', + message: `Missing envelopes for ${missingSiblings.length} sibling device(s)`, + missingDeviceIds: missingSiblings, + }); + return; +} +``` + +`findMissingSiblingDeviceIds` (`lib/messageFanout.ts`) checks that the envelope map +covers every one of the *sender's own* other active devices — the same scope +`send_message` enforces (see +[`concepts-delivery-fanout.md`](./concepts-delivery-fanout.md), which also notes this is +narrower than full "every recipient's active devices" coverage: that stronger check, +`fanoutMessage`, exists in `services/fanout.ts` but has no callers on this path either). +A client that edits from one device without re-encrypting for its own other devices gets +rejected with `device_set_mismatch`, identically to a `send_message` with the same gap. + +On success: the sender gets `message_ack`, the new message is delivered through the +normal `deliverMessage` fan-out pipeline (so every recipient device gets a fresh +`message_envelope` for the edited content), conversation-list caches for every member +are invalidated, and the conversation room receives: + +```ts +io.to(conversationRoom(conversationId)).emit('message_edited', { + originalMessageId: rootMessageId, + newMessageId: messageId, +}); +``` + +Note this is a *separate* event from the `new_message`/`message_envelope` the fan-out +pipeline also emits for the new row — a client needs to handle both: the envelope +delivers the new ciphertext to decrypt, and `message_edited` tells every other open +client (including the sender's own other devices, once their envelope arrives) which +earlier message this supersedes. + +## `delete_message` + +Delete is destructive at the server, by design — it does not keep a tombstone the way +some systems do. Given a `messageId` whose sender matches the caller: + +```ts +await db + .update(messages) + .set({ deletedAt: new Date(), ciphertext: null }) + .where(eq(messages.id, messageId)); + +await db.delete(messageEnvelopes).where(eq(messageEnvelopes.messageId, messageId)); + +if (message.fileId) { + const { softDeleteFile } = await import('../services/fileCleanup.js'); + await softDeleteFile(message.fileId); +} + +io.to(message.conversationId).emit('message_deleted', { messageId }); +io.to(conversationRoom(message.conversationId)).emit('message_deleted', { messageId }); +``` + +Three things happen, in this order, none of them reversible: + +1. **The `messages` row is kept, but gutted**: `ciphertext` is nulled and `deletedAt` is + stamped. The row survives (so ids, timestamps, and `editsMessageId` links referencing + it don't dangle), but the encrypted payload is gone — there is nothing left to decrypt + even with the right keys. +2. **Every `message_envelopes` row for the message is hard-deleted**, not soft-deleted. + This is the only one of the three steps with no `deletedAt`-style flag; the per-device + delivery units are simply gone. +3. **If the message carried a file**, `softDeleteFile` is called — see + [Interaction with file GC](#interaction-with-file-gc) for exactly what that does and + doesn't do. + +`message_deleted` is then broadcast via two separate `io.to(...).emit(...)` calls: one +to the room literally named by `message.conversationId`, and one to +`conversationRoom(message.conversationId)` (`room:conversation:${id}`) — two genuinely +different Socket.IO room names. Every connected socket joins *both* of these at connect +time (`index.ts`'s connection setup joins each conversation under both its bare id and +its `conversationRoom()`-prefixed form), so **a client receives `message_deleted` twice +per delete** — this is not deduplicated, since they're two independent `.emit()` calls, +not two rooms targeted by one call. The same double-room pattern, and the same resulting +duplicate delivery, is called out for `device_delivery_receipt` in +[`concepts-delivery-fanout.md`](./concepts-delivery-fanout.md#gaps--code-not-wired-into-the-live-path) — +this is a recurring pattern in this file, not unique to delete. A client should +deduplicate on `messageId` rather than assume one `message_deleted` per actual deletion. + +There is no equivalent of the edit handler's idempotency check (no "already deleted" +short-circuit) — calling `delete_message` twice for the same id runs the same four steps +twice; the second `UPDATE`/`DELETE`s are no-ops against already-empty state, so this is +harmless but not explicitly guarded. + +## What an offline device sees on reconnect / `/sync` + +`routes/sync.ts`'s envelope-catch-up endpoint filters with: + +```ts +.where( + and( + eq(messageEnvelopes.recipientDeviceId, deviceId), + /* cursor */ + or(isNull(messageEnvelopes.deliveredAt), gt(messageEnvelopes.createdAt, ttlCutoff)), + isNull(messages.deletedAt), + ), +) +``` + +**A deleted message's envelope can never be delivered via `/sync`**, for two independent +reasons that both have to be true for this to matter in practice, but either alone would +block it: `delete_message` already hard-deletes every `messageEnvelopes` row for that +message (so there's nothing left to join against), and even if an envelope somehow +survived, the `isNull(messages.deletedAt)` filter excludes it. A device that was offline +for the entire lifetime of a message — sent, then deleted before that device ever +connected — never learns the message existed at all; it does not receive a +`message_deleted` event for something it never saw, because `/sync` is a REST catch-up +endpoint for missed *envelopes*, not a log of every state-changing socket event that +happened while the device was away. + +For an edit, reconnect sees it as what it actually is: **a new, separate message.** The +edited message is a normal row with its own envelope, so `/sync` returns it like any +other undelivered message — with `editsMessageId` pointing at the root. The client is +responsible for recognizing that linkage and replacing the message it's displaying for +`rootMessageId`/`originalMessageId` with the new content; the server does not retroactively +notify a reconnecting device of the `message_edited` room event it missed while offline, +only the new message's envelope. + +## The honest limit of delete under E2EE + +The server can delete its own copy of the ciphertext and the per-device envelopes. It +**cannot** delete anything from a recipient device that already decrypted the message. +Once a client has received and decrypted a message, the plaintext — and any copy the +client made of it (chat UI state, local database, OS notification history, screenshot, +clipboard, the client's own local search index — see +[below](#interaction-with-local-search-indexes)) — is entirely outside the server's +reach. `delete_message` deleting the server's ciphertext stops *future* delivery to +devices that haven't fetched it yet (via `/sync`, as covered above); it is not a recall +of something already read. Any UI that frames "delete" as removing a message from +everyone's view should be worded to reflect this: it removes the message from devices +that hadn't already received it, and from the sender's and other devices' live view going +forward, but it cannot un-reveal plaintext a recipient has already seen. + +## Interaction with file GC + +`softDeleteFile` (`services/fileCleanup.ts`), called from `delete_message` when the +deleted message had a `fileId`, is conditional, not unconditional: + +```sql +UPDATE files SET deleted_at = now() +WHERE id = :fileId + AND hard_deleted_at IS NULL + AND NOT EXISTS ( + SELECT 1 FROM messages + WHERE file_id = :fileId AND deleted_at IS NULL + ) +``` + +A file is only soft-deleted if **no other non-deleted message still references it** — +`messages.fileId` has no uniqueness constraint, so the same file row can in principle be +attached to more than one message. Deleting one message that references a shared file +does not delete the file out from under another message still using it. + +Once a file is soft-deleted (whether via this path or any other), it becomes a candidate +for the hard-delete pass that actually removes the underlying object, subject to +`FILE_HARD_DELETE_GRACE_MS` (default 0ms, so effectively the next tick). See +[`concepts-gc-jobs.md`](./concepts-gc-jobs.md#file-cleanup--servicesfilecleanupts) for +that pass's full retention/scheduling detail. `runHardDeletePass` deletes through +`getObjectStore()`, the same `ObjectStoreLike` interface documented in +[`concepts-local-object-store.md`](./concepts-local-object-store.md) (#330) — so this +soft-delete-then-hard-delete path behaves identically whether the active backend is +production S3 or the dev/test local-disk store. + +Editing a message never touches file GC: `edit_message` does not accept or change a +`fileId`, so an edited text message's file association (if the root message ever had one) +is untouched by the edit. + +## Interaction with local search indexes + +This is a client-side concern, not something the server enforces, and the current web +client has a real gap here: `lib/search/searchClient.ts` exports `removeMessages(ids)`, +intended to drop messages from the local IndexedDB-backed search index, but it has no +callers anywhere in `apps/web/src` — neither the `message_deleted` nor the +`message_edited` socket handler invokes it. `useMessageSearchIndex` indexes whatever +messages it's given (decrypting ciphertext client-side first) but has no logic to react +to either event. In the current client, a deleted message's plaintext can remain +searchable in the local index after the server has discarded its ciphertext, and an +edited message is indexed as an additional, separate entry (its own `id`) rather than +replacing the original's index entry — consistent with the fact that an edit really is a +new message server-side, but worth calling out explicitly since nothing currently +reconciles the two entries into one on the client either. + +## Implementation references + +- `apps/backend/src/socket/messaging.ts` — `edit_message`, `delete_message` handlers +- `apps/backend/src/lib/messageFanout.ts` — `findMissingSiblingDeviceIds` +- `apps/backend/src/routes/sync.ts` — envelope catch-up endpoint, `isNull(messages.deletedAt)` filter +- `apps/backend/src/routes/conversations.ts` — `supersededRows`, how the message list collapses an edit chain to its tip +- `apps/backend/src/services/fileCleanup.ts` — `softDeleteFile`, `runHardDeletePass` +- `apps/backend/src/db/schema.ts` — `messages.editsMessageId`, `messages.deletedAt`, `messages.fileId` +- `apps/web/src/lib/search/searchClient.ts` — `removeMessages` (unused), `indexMessages` +- `apps/web/src/hooks/useMessageSearchIndex.ts` — client-side indexing hook +- [`concepts-delivery-fanout.md`](./concepts-delivery-fanout.md) — the fan-out pipeline an edit's new envelope goes through, and the sibling-coverage gap this document's sibling check shares +- [`concepts-gc-jobs.md`](./concepts-gc-jobs.md) — file hard-delete pass scheduling and retention +- [`concepts-local-object-store.md`](./concepts-local-object-store.md) — #330, the dev/test storage backend the hard-delete pass also targets diff --git a/apps/backend/docs/concepts-push-decision-matrix.md b/apps/backend/docs/concepts-push-decision-matrix.md new file mode 100644 index 0000000..c0b018e --- /dev/null +++ b/apps/backend/docs/concepts-push-decision-matrix.md @@ -0,0 +1,138 @@ +# Push notification decision matrix + +The question this document answers: **will this user get a push notification for this +message?** The rules are shared by both send paths — `dispatchOfflinePush` (text +messages, `services/pushNotification.ts`) and `sendPushForMessage` (file/image/video/ +audio messages, `services/push.ts`) — both call the same filter, +`getEligiblePushRecipients` in `services/pushFilter.ts`, then both hand their eligible +device list to the same `queueCoalescedPush`, so from the coalescing window onward the +two paths are identical. + +## Contents + +- [Decision table](#decision-table) +- [Online granularity: does one device suppress push to the rest?](#online-granularity-does-one-device-suppress-push-to-the-rest) +- [Coalescing and rate limiting](#coalescing-and-rate-limiting) +- [What the push payload contains](#what-the-push-payload-contains) +- [A real asymmetry between the two paths](#a-real-asymmetry-between-the-two-paths) +- [Tests that pin this behavior](#tests-that-pin-this-behavior) +- [Implementation references](#implementation-references) + +## Decision table + +`getEligiblePushRecipients` runs these filters in order; a device that fails any one of +them never reaches the coalescing/send step. Each row below is independently sufficient +to suppress a push for a given device — the end result is the intersection of every +row's "eligible" column. + +| Filter | Checked against | Device is excluded when | Checked in | +|---|---|---|---| +| Is the sender | `senderId` param | The device's owner is the message's sender | Step 1 (member filter) | +| Conversation muted | `conversationMembers.isMuted` | The device owner has muted this conversation | Step 1 | +| User online (any device) | Redis presence hash, `isOnline()` | The device owner has **any** live socket connection, on any device | Step 2 | +| `devices.pushEnabled` | `devices.pushEnabled` | The device has push disabled | Step 3 | +| Device revoked | `devices.revokedAt` | The device has been revoked (`IS NOT NULL`) | Step 3 | +| Explicit recipient list, if given | `recipientDeviceIds` param | The device isn't in the caller-supplied list (text path only — see [below](#a-real-asymmetry-between-the-two-paths)) | Step 4 | +| Device connected (per-device) | Redis connection registry, `isDeviceConnected()` | The device itself has a live socket, independent of the user-level check above | Step 5 | + +If `redis` is unavailable at all (the `options.redis` param is `null`/`undefined`), the +user-online check (step 2) is skipped entirely and every non-muted, non-sender member is +treated as offline — a deliberate best-effort fallback so push still works (if +noisier) when Redis presence tracking is down, rather than failing to push at all. + +## Online granularity: does one device suppress push to the rest? + +**Yes.** `isOnline(redis, userId)` (`services/presence.ts`) checks +`redis.hlen(presenceHashKey(userId)) > 0` — a single hash keyed by **user**, not by +device, where every currently-connected device of that user has an entry. If the user +has even one live socket connection anywhere, `isOnline` returns `true` for that user, +and step 2 excludes **all** of that user's devices from the candidate set before +`devices.pushEnabled`/`revokedAt` are even checked. + +This means: a user reading messages on their laptop receives no push notifications on +their phone for the same conversation, even though the phone itself has no open socket +and would otherwise be a perfectly eligible push target. Push suppression is per-user, +not per-device, at this layer. + +Step 5's `isDeviceConnected` check is a separate, genuinely per-device registry lookup +(`services/deviceRevocation.ts`, backed by the same cross-node Redis registry +device-revocation checks use) — it is not redundant with step 2. It exists to catch a +device that is itself connected but whose owner's aggregate presence hash hasn't +reflected that yet (a narrow race, not the common case), and it's the only point in the +pipeline that would ever disagree with step 2 in practice. For the common case — a user +with no connections anywhere — both checks agree and either one alone would suffice. + +## Coalescing and rate limiting + +Once a device passes every filter above, `queueCoalescedPush(deviceId, conversationId, +messageId)` is called — by both send paths — which layers two more behaviors on top of +the eligibility decision, per device: + +1. **Coalescing window (`COALESCE_WINDOW_MS`, 2 seconds)**: a `Map` keyed by + `` `${deviceId}:${conversationId}` `` tracks a pending push. Every call within the + window for the same device+conversation increments a `count` and updates + `latestMessageId` instead of scheduling a new push — a burst of N messages (including + a mix of text and file messages, since both paths share this map) to the same device + in the same conversation produces exactly one push, reporting the burst's `count` and + only the most recent `messageId`. +2. **Per-device rate limit (`RATE_LIMIT_WINDOW_MS`, 30 seconds)**: when the coalescing + timer fires (`flushPush`), it checks `lastPushSentAt` for that device; if a push was + sent to it within the last 30 seconds, this flush is dropped entirely — not queued, + not deferred, just skipped. A device that's receiving pushes faster than once every + 30 seconds will miss some flush cycles outright, not merely have them delayed. + +So the full answer to "will this user get a push for this message" has to account for +both layers: a device that's eligible per the table above still might not get **this +specific** push if it's absorbed into a coalesced batch (and reported via that batch's +`count` instead) or dropped by the 30-second rate limit. + +## What the push payload contains + +```ts +const payload = JSON.stringify({ type: 'new_message', conversationId, messageId, count }); +``` + +**No message content, plaintext or ciphertext, is ever in the push payload.** The client +receiving this payload has `conversationId`, `messageId`, and how many messages were +coalesced into this notification — enough to show a generic "N new messages in +[conversation]" notification and let the user's own client fetch and decrypt the actual +content once they open the app. This is consistent with the server never holding +decryptable plaintext for E2EE conversations in the first place (see +[`concepts-crypto-invariants.md`](./concepts-crypto-invariants.md)) — even if it wanted +to put message content in a push payload, it has no plaintext to put there for a real +E2EE message. (The one exception elsewhere in the codebase, the AI assistant's reply, is +delivered through the same `message_envelope` fan-out as any other message — see +[`concepts-ai-assistant-bridge.md`](./concepts-ai-assistant-bridge.md) — not through push, +so it doesn't change this.) + +## A real asymmetry between the two paths + +`dispatchOfflinePush` (text) is always called with an explicit `recipientDeviceIds` list +(step 4 of the table), so `getEligiblePushRecipients` intersects the DB-discovered +eligible devices against that caller-supplied set. `sendPushForMessage` (file messages, +in `services/push.ts`) calls `getEligiblePushRecipients` **without** +`recipientDeviceIds` at all, so step 4 is a no-op for that path — it always relies purely +on discovery from `conversationMembers`. In practice both paths are invoked once per +message for "every member's devices," so this rarely produces a different result, but +it is a real difference in the function's call contract between the two callers, not +just a documentation nuance: a future caller of `sendPushForMessage` that wanted to +target a specific device subset (the way `dispatchOfflinePush` can) would find no +parameter to do so. + +## Tests that pin this behavior + +- `apps/backend/src/__tests__/pushFilter.test.ts` — the eligibility table above: + sender exclusion, muted conversations, `devices.pushEnabled`, revoked devices, the + per-device connection check, and the Redis-unavailable fallback. +- `apps/backend/src/__tests__/pushNotification.test.ts` — coalescing window behavior, + the per-device rate limit, dead-subscription pruning (410/404) and transient-failure + backoff, and the payload shape. + +## Implementation references + +- `apps/backend/src/services/pushFilter.ts` — `getEligiblePushRecipients`, the shared decision table +- `apps/backend/src/services/presence.ts` — `isOnline` (per-user presence hash) +- `apps/backend/src/services/deviceRevocation.ts` — `isDeviceConnected` (per-device registry) +- `apps/backend/src/services/pushNotification.ts` — `dispatchOfflinePush`, `queueCoalescedPush`, `flushPush`, `sendWebPush`, backoff/pruning hygiene +- `apps/backend/src/services/push.ts` — `sendPushForMessage`, the file-message path +- `apps/backend/src/__tests__/pushFilter.test.ts`, `apps/backend/src/__tests__/pushNotification.test.ts` diff --git a/contracts/docs/howto-add-contract-function.md b/contracts/docs/howto-add-contract-function.md new file mode 100644 index 0000000..621893c --- /dev/null +++ b/contracts/docs/howto-add-contract-function.md @@ -0,0 +1,257 @@ +# How-to: add a contract function and expose it in the app + +This walks the full path from a new `#[contractimpl]` function to a working UI action, +using `token_transfer` throughout because it's the one contract in this repo that +supports in-place upgrade (see [§6](#6-when-the-backend-listener-must-change) for why +that matters, and what the other two contracts require instead). + +## Contents + +1. [Worked example 1: a small read-only function](#1-worked-example-1-a-small-read-only-function) +2. [Worked example 2: a small state-changing function](#2-worked-example-2-a-small-state-changing-function) +3. [Write tests](#3-write-tests) +4. [Regenerate `test_snapshots/`](#4-regenerate-test_snapshots) +5. [Rebuild and redeploy](#5-rebuild-and-redeploy) +6. [When the backend listener must change](#6-when-the-backend-listener-must-change) +7. [Frontend: invoking the new function](#7-frontend-invoking-the-new-function) +8. [Checklist](#8-checklist) + +## 1. Worked example 1: a small read-only function + +Say you want to add a way to check how much of the configured token this contract has +been authorized to move (vs. querying the SEP-41 token directly). A read-only function +takes `&self`-equivalent access via `env` only, touches no mutable storage, and emits no +event: + +```rust +// contracts/contracts/token_transfer/src/lib.rs, inside impl TokenTransferContract + +/// Returns whether the contract has been initialised (admin + token set). +pub fn is_initialized(env: Env) -> bool { + env.storage().instance().has(&DataKey::Admin) +} +``` + +This mirrors the existing `balance` and `token_contract` functions in the same file — +read `env.storage().instance()`, return a value, no `require_auth()` needed because +nothing is being authorized, no event because no state changed. + +## 2. Worked example 2: a small state-changing function + +Now something that writes storage and requires authorization — say, an admin-only pause +flag: + +```rust +// storage.rs +#[derive(Clone)] +#[contracttype] +pub enum DataKey { + Admin, + TokenContract, + Paused, // new +} + +// lib.rs, inside impl TokenTransferContract + +/// Admin-only: pause or unpause transfers. +pub fn set_paused(env: Env, paused: bool) { + let admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("not initialized"); + admin.require_auth(); + env.storage().instance().set(&DataKey::Paused, &paused); + + env.events() + .publish((Symbol::new(&env, "paused_changed"),), paused); +} +``` + +And — for the function to actually do anything — `transfer` would check it: + +```rust +pub fn transfer(env: Env, from: Address, to: Address, amount: i128, memo: Bytes) { + let paused: bool = env.storage().instance().get(&DataKey::Paused).unwrap_or(false); + if paused { + panic!("transfers are paused"); + } + // ...existing logic unchanged +} +``` + +This follows the exact pattern `set_token_contract` already uses in the same file: +load the admin, `require_auth()`, write storage. The event is new here, not present on +`set_token_contract` — added because, unlike a config value nobody off-chain currently +reads, a pause toggle is exactly the kind of state change a backend consumer plausibly +wants to react to. Whether to add one is a judgment call per function, not a rule; see +[`contracts-events.md`](./contracts-events.md) for every event this codebase currently +emits and why each one exists. + +## 3. Write tests + +Add a test in the same contract's `src/test.rs`, following the existing tests for +`transfer`/`set_token_contract` in that file for the mock-auth and test-environment +setup pattern already in use. At minimum, mirror the existing coverage style: + +- the happy path (the flag/value is actually read back correctly after being set) +- the `require_auth()` gate (a non-admin caller's attempt panics or is rejected) +- for `set_paused` specifically: that `transfer` actually panics while paused, and + succeeds again once unpaused + +See [`contracts/docs/testing.md`](./testing.md) for the full testing guide — the +mock-auth helpers, how `Env::default()` test environments are constructed, and the +in-repo conventions this codebase's existing contract tests already follow. + +## 4. Regenerate `test_snapshots/` + +Run the contract's tests: + +```bash +cd contracts +cargo test -p token_transfer +``` + +This both runs your new test and regenerates +`contracts/contracts/token_transfer/test_snapshots/` to reflect the new storage key +(`DataKey::Paused`) and the new event. **Do not hand-edit anything under +`test_snapshots/`** — see +[`testing.md`'s `test_snapshots/` section](./testing.md#the-test_snapshots-directory) for +why these files are generated-but-committed, and why an unexpected diff in one during +review is a signal to look closer, not noise to dismiss. + +**The CI check that fails if this is forgotten** is simply `cargo test`'s own pass/fail: +`.github/workflows/contracts-ci.yml` runs `cargo test -p ${{ matrix.package }}` for each +contract. Soroban's test harness itself detects a storage-footprint mismatch against the +committed snapshot and fails the test if the snapshot wasn't regenerated to match new +behavior — there's no separate "check the snapshot is fresh" CI step beyond the test +suite catching it directly. If you add a function but never run `cargo test` locally and +never commit the updated snapshot, CI fails on this contract's test job, not silently. + +## 5. Rebuild and redeploy + +Build the wasm for this contract (same command CI and the deploy scripts use — see +[`api-deployment-invocation.md`](./api-deployment-invocation.md#2-building-the-contracts-verified) +for the full build reference): + +```bash +cargo build -p token_transfer --target wasm32-unknown-unknown --release +``` + +**This is where `token_transfer` and the other two contracts diverge, and it matters +which one you're changing.** `token_transfer` exposes `upgrade(env, new_wasm_hash)` — +the only in-place upgrade path in this codebase. `group_treasury` and `proposals` expose +no such entrypoint; changing their behavior means deploying an entirely new contract +instance and repointing every consumer at the new contract ID. See +[`concepts-upgrades.md`](./concepts-upgrades.md) for the full mechanism, authorization +model, and the storage-layout compatibility rules an in-place upgrade must respect (you +cannot, for example, repurpose an existing `DataKey` variant's meaning across an +upgrade — only add new ones, as the `Paused` variant above does). + +For `token_transfer`, upgrading an already-deployed instance: + +1. Install the new wasm to get its hash: + ```bash + stellar contract install --wasm target/wasm32-unknown-unknown/release/token_transfer.wasm \ + --network testnet --source + ``` +2. Invoke `upgrade` on the existing contract ID with that hash, authorized as the + contract's admin (the same admin `set_token_contract` and the new `set_paused` above + both gate on): + ```bash + stellar contract invoke --id --network testnet \ + --source -- upgrade --new_wasm_hash + ``` + +The contract ID does not change — every existing reference to it (backend `.env`, +frontend `NEXT_PUBLIC_TOKEN_TRANSFER_CONTRACT`) keeps working against the same address +with the new code live. + +## 6. When the backend listener must change + +Only relevant if your new function emits an event (`env.events().publish(...)`) that the +backend should react to — a read-only function or a state change nobody off-chain needs +to know about (like `set_token_contract`, which emits nothing) needs no listener change. + +`apps/backend/src/services/stellarListener.ts` (#331) polls Soroban RPC `getEvents` +filtered to specific contract IDs and **specific topic strings** — for `token_transfer`, +that filter is currently `topics: [['transfer']]`, matching the contract's +`Symbol::new(&env, "transfer")`. A new event under a different topic, like the +`paused_changed` example above, is invisible to the listener until its topic filter and +decode logic are extended to include it — the listener does not subscribe to "every +event this contract emits," only the ones it's been told to look for. + +Adding support means: + +1. Add the new topic string to the relevant `topics` filter array in + `stellarListener.ts`. +2. Add a decode branch for the new event's payload shape (mirroring how the existing + `transfer` event is decoded into a `StellarTransferEvent`). +3. Decide what the backend does with it — a database write, a WebSocket push to + affected users, both, or (if it's purely informational) nothing beyond logging. + +See [`contracts-events.md`](./contracts-events.md) for the full table of every event +currently emitted across all three contracts and whether each one is actually consumed +by the listener today — several are not, which is itself documented there as a known +gap rather than an oversight to silently "fix" by wiring one up as a side effect of an +unrelated change. + +## 7. Frontend: invoking the new function + +`apps/web/src/lib/soroban.ts`'s `transferToken` is the existing pattern for a +state-changing call from the browser via Freighter. A new function follows the same +five-step shape: + +1. **Build the operation** — `contract.call('', ...scValArgs)`, with each + argument converted via `nativeToScVal` to the matching Soroban type (`i128` for a + signed 128-bit int, `address` for an `Address`, etc. — match the Rust function + signature's parameter types exactly, or the call fails at simulation). +2. **Simulate** — `server.simulateTransaction(tx)`, and check + `SorobanRpc.Api.isSimulationError(simResult)` before proceeding. This is also how you + discover a read-only function's return value without spending a signature: for + `is_initialized` above, simulation alone is enough — skip steps 3–5 entirely for a + read-only call, since there's nothing to sign or submit. +3. **Sign** — `freighter.signTransaction(txXdr, { networkPassphrase })`, after + `SorobanRpc.assembleTransaction(tx, simResult).build()` incorporates the simulation's + resource footprint into the transaction. +4. **Submit** — `server.sendTransaction(signedTx)`. +5. **Poll for confirmation** — `server.getTransaction(hash)` in a loop until `SUCCESS` or + `FAILED`, exactly as `transferToken`'s existing 30-iteration/2-second poll loop does. + +For `set_paused`, the only differences from `transferToken` are the function name +(`'set_paused'`) and argument list (a single boolean argument converted via +`nativeToScVal`, instead of the four transfer arguments — check the `stellar-sdk` +version pinned in `apps/web/package.json` for its exact boolean type-hint string, since +this helper's API has shifted across SDK versions) — everything else, including which +identity signs (the +connected Freighter wallet must be the contract's admin, or `require_auth()` on-chain +rejects the call after it's already been submitted and the user has already seen a +signing prompt), is the same shape. + +## 8. Checklist + +- [ ] Function added to the contract's `lib.rs`, with `require_auth()` if it changes + state restricted to a specific caller +- [ ] New `DataKey` variant added (never repurpose an existing variant's meaning if the + contract is upgradeable — see [§5](#5-rebuild-and-redeploy)) +- [ ] Event added, if off-chain systems need to react to this change +- [ ] Tests added in `src/test.rs`: happy path, auth gate, and (if applicable) the + behavior the new state actually gates +- [ ] `cargo test -p ` run locally, `test_snapshots/` diff reviewed and + committed +- [ ] Wasm rebuilt and, for `token_transfer`, upgraded in place via `upgrade`; for + `group_treasury`/`proposals`, a new instance deployed and every consumer repointed +- [ ] If a new event topic was added: `stellarListener.ts`'s topic filter and decode + logic updated, and `contracts-events.md`'s table updated to reflect it +- [ ] Frontend call site added in `apps/web/src/lib/soroban.ts` (or a new function + following the same pattern), and the UI action that triggers it wired up + +## References + +- `contracts/contracts/token_transfer/src/lib.rs` — the contract this guide is worked against +- [`contracts/docs/testing.md`](./testing.md) — test conventions and the `test_snapshots/` directory +- [`contracts/docs/concepts-upgrades.md`](./concepts-upgrades.md) — the upgrade mechanism, authorization, and storage-compatibility rules +- [`contracts/docs/api-deployment-invocation.md`](./api-deployment-invocation.md) — build, deploy, and invoke reference for all three contracts +- [`contracts/docs/contracts-events.md`](./contracts-events.md) — every event emitted, topic, payload, and listener-consumption status (#351) +- `apps/backend/src/services/stellarListener.ts` — the backend's chain listener (#331) +- `apps/web/src/lib/soroban.ts` — the frontend's Freighter + Soroban RPC helper diff --git a/docs/README.md b/docs/README.md index b2f8d94..70af3d1 100644 --- a/docs/README.md +++ b/docs/README.md @@ -63,6 +63,9 @@ listener. | [Replay protection and event idempotency](../apps/backend/docs/concepts-replay-protection.md) | The device-scoped `eventId` dedup, the message-level `messageId` idempotency, the TTL, and the `dispatch_ack` duplicate flag. | | [Backend caching reference](../apps/backend/docs/concepts-caching.md) | The conversation-list cache: key, TTL, payload, every invalidation site, and the behaviour when Redis is down. | | [Presence, heartbeat & connection liveness](../apps/backend/docs/concepts-presence-heartbeat.md) | The Redis presence key structure, the 30s/90s heartbeat timing contract, multi-device aggregation, and the debounced offline-broadcast behaviour. | +| [Message edit and delete semantics](../apps/backend/docs/concepts-message-edit-delete.md) | Authorization, how an edit is a new linked message rather than a mutation, what a deleted message's envelope does on `/sync`, and the honest limit of delete under E2EE. | +| [AI assistant bridge](../apps/backend/docs/concepts-ai-assistant-bridge.md) | The full `ask_assistant` request path, the hardcoded agent URL, and exactly what plaintext leaves the client and reaches the backend and the AI agent. | +| [Push notification decision matrix](../apps/backend/docs/concepts-push-decision-matrix.md) | The full filter chain deciding whether a device gets a push, why one online device suppresses push to a user's other devices, and the coalescing/rate-limit layer on top. | | [Testing strategy and conventions](testing.md) | The Drizzle mocking pattern, driving socket handlers through the `dispatch` envelope, and the in-process counters that leak between tests. | | [Backend testing guide](../apps/backend/docs/testing.md) | The standard route-test mock set with a copyable skeleton, the Drizzle chain traps, and the state a suite has to reset between tests. | @@ -157,6 +160,7 @@ The Soroban workspace in `contracts/`: `token_transfer`, `group_treasury`, and ` | [Token transfer flow](../contracts/docs/concepts-token-transfer-flow.md) | The end-to-end flow of an in-chat payment through the contract. | | [Token transfer storage](../contracts/docs/contracts-token-transfer-storage.md) | Storage keys and value types used by `token_transfer`. | | [WASM size and resource budget](../contracts/docs/concepts-resource-budget.md) | The 100 KB per-contract CI gate, current sizes and headroom, and the levers available when a contract approaches the limit. | +| [How-to: add a contract function](../contracts/docs/howto-add-contract-function.md) | The full path from a new `#[contractimpl]` function to a working UI action: implementation, tests, snapshot regeneration, upgrade/redeploy, listener changes, and the frontend call. | ### Operator