From 46d621c91b7ec8692a4e0e9bf0a69464b021ce02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Wed, 19 Aug 2026 22:00:17 +0000 Subject: [PATCH] chat notifications reach the phone, on the same rule as the bell MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the browser bell and phone push are now two subscribers to one signal. both ask shouldNotify(); neither owns the rule. deliberately a second subscriber rather than a branch inside notify-delivery. the two channels share only the question "does this account want to hear about this" — the browser needs an open tab and sends the chat's real title over a loopback socket on the owner's own tailnet, the phone needs no tab and must send NO title, because that hop goes through apple. one function doing both means an `if (isPush)` at every step, and the first thing to go wrong is a title leaking because the branch was missed in one place. two codes, officer/core:chat.turn-ended and chat.input-needed. the route is written out with its scheme rather than derived from the target: the hub has no chat screen — chat is Butler, a sibling app — so these deep-link OUT over officer-chat://, and two of the suite's twenty-one schemes already break the officer- pattern (officer-videos, officer-readaloud). deriving that works until it silently does not. `thread` interpolates now, so `chat:{id}` groups per conversation. a static thread forces a choice between "three pings from one chat collapse" and "two busy chats stay apart", and the wrong answer is that every chat is the same chat. dropped officer/core:agent: chat.turn-ended supersedes it and its route pointed at officer://chat/{id}, a hub screen that does not exist. the banner cannot name which conversation. a chat's title is content and content does not travel; it says what happened, and tapping opens the chat, which is where the name lives. --- src/databases/CLAUDE.md | 18 ++++++++- src/server.tsx | 6 +++ src/servers/api/chat/notify-push.ts | 53 ++++++++++++++++++++++++++ src/servers/notifications/catalogue.ts | 37 ++++++++++++++---- src/servers/sidecar/notify/render.ts | 5 ++- 5 files changed, 109 insertions(+), 10 deletions(-) create mode 100644 src/servers/api/chat/notify-push.ts diff --git a/src/databases/CLAUDE.md b/src/databases/CLAUDE.md index 5f0e93bf..7a9a10d5 100644 --- a/src/databases/CLAUDE.md +++ b/src/databases/CLAUDE.md @@ -101,6 +101,21 @@ index form to escape to — a primary key must be a constraint. `music_now_playi re-adds `pk_music_now_playing` on every push. Two statements, silent, no prompt even with rows in the table, data unaffected. Naming it explicitly does not help. Leave it. +**Four foreign keys are over 63 characters and churn the same way**, found 2026-08-19 while pushing +`chat_notification_prefs`. These are the case the paragraph above says to name explicitly, and none of +them is: + + command_hq_session_tags_session_row_id_command_hq_sessions_id_fk 64 + command_hq_watcher_fires_session_row_id_command_hq_sessions_id_fk 65 + bitcoin_wallet_frozen_utxos_wallet_id_bitcoin_wallet_wallets_id_fk 66 + bitcoin_wallet_chain_cache_wallet_id_bitcoin_wallet_wallets_id_fk 65 + +The tell is in the plan itself: the `DROP` names them at 63 characters and the `ADD` at their full +length, which is Postgres's truncation and drizzle's copy disagreeing. Eight statements per push, no +prompt, data unaffected — deferred rather than fixed (see `TODO.md`), because it is noise rather than +a fault. Two live in `officer_db/src/command-hq/schema.ts`; the other two are the bitcoin-wallet +plugin's, in its own repository. + **Rules that still apply:** 1. **Never answer "Yes, truncate the table"** if a prompt ever does appear. It destroys rows and fixes @@ -111,7 +126,8 @@ table, data unaffected. Naming it explicitly does not help. Leave it. branch it takes at a truncate prompt. 4. **Read the plan before applying it:** `bunx drizzle-kit push --config=drizzle.config.ts --verbose` prints every statement first. Expect the - two `pk_music_now_playing` lines. Anything else is your change. + two `pk_music_now_playing` lines and the eight from the four over-long foreign keys above — ten in + all. Anything else is your change. To test a schema change without touching `officer_dev`: `createdb officer_scratch`, then `POSTGRES_URL=postgresql://postgres:postgres@127.0.0.1:5432/officer_scratch bunx drizzle-kit push diff --git a/src/server.tsx b/src/server.tsx index ffd8cd05..5aab2447 100644 --- a/src/server.tsx +++ b/src/server.tsx @@ -18,6 +18,7 @@ import { isTokenBlacklisted, getUserById } from 'officerdb'; import { terminalWebsocket } from './servers/api/terminal/websocket'; import { chatWebsocket } from './servers/api/chat/websocket'; import { chatNotifyWebsocket } from './servers/api/chat/notify-delivery'; +import { registerChatPushNotifications } from './servers/api/chat/notify-push'; import { taskRunnerWebsocket } from './servers/api/tasks/task-executor'; import { pipelineWebsocket } from './servers/api/tasks/pipeline-executor'; // import { desktopWebsocket } from './servers/api/desktop/websocket'; @@ -160,6 +161,11 @@ assertPermissionTotality({ wsProviders: Object.keys(handlers), }); +// Chat's phone channel. Subscribes to the same signal the browser bell uses and asks the same question; +// it just answers it into APNs/FCM instead of a websocket. Registered here, and explicitly, because a +// module whose only job is a side effect is one an import sweep quietly deletes. +registerChatPushNotifications(); + // That the working directory is the repo, because every path derives from its parent. First of the three, // since a wrong answer here makes the other two check the wrong files. assertInstallLayout(); diff --git a/src/servers/api/chat/notify-push.ts b/src/servers/api/chat/notify-push.ts new file mode 100644 index 00000000..7045ed98 --- /dev/null +++ b/src/servers/api/chat/notify-push.ts @@ -0,0 +1,53 @@ +import * as sidecar from '@@/sidecar-registry'; +import { getNotifyServerUrl } from '../notify/router'; +import { shouldNotify, type NotifyKind } from './notify-decision'; + +// The phone half of the bell: the same signal the browser socket listens to, delivered to APNs/FCM. +// +// ── Why this is a second subscriber and not a branch inside notify-delivery ── +// +// The two channels have nothing in common except the question "does this account want to hear about +// this". That question lives in `notify-decision.ts` and both ask it; everything else differs. The +// browser needs an open tab and sends the chat's real title over a loopback socket on the owner's own +// tailnet. The phone needs no tab at all and must send NO title, because that hop goes through Apple. +// +// Folding both into one function would mean a `if (isPush)` inside every step of it, and the first thing +// to go wrong would be a title leaking into a payload because the branch was missed in one place. + +/** Which code a kind maps to. Both live in `notifications/catalogue.ts` as `officer/core:chat.*`. */ +const CODE_FOR: Record = { + 'turn-ended': 'officer/core:chat.turn-ended', + 'input-needed': 'officer/core:chat.input-needed', +}; + +/** + * Subscribe the push channel to chat's notify signal. + * + * Called explicitly from `server.tsx` rather than run on import: a module whose only purpose is a side + * effect is one an unused-import sweep deletes, and the failure would be silent. + */ +export function registerChatPushNotifications(): void { + sidecar.onClaudeNotify(async (sessionKey, userId, kind) => { + try { + if (!(await shouldNotify(userId, sessionKey, kind))) return; + + const base = getNotifyServerUrl(); + // The sidecar has not reported in — either it is starting, or push is not running on this install. + // Not an error: the browser bell still works, and this is the channel that is allowed to be absent. + if (!base) return; + + // `id` is the session key so the device can deep-link straight to the conversation. It is an + // opaque uuid, which is the only reason it may travel: NO title, and no message content, because + // this payload is read by Apple and Google. See docs/push-notifications.md. + await fetch(`${base}/_officer/notify`, { + method: 'POST', + headers: { 'content-type': 'application/json', 'X-Officer-User': String(userId) }, + body: JSON.stringify({ code: CODE_FOR[kind], userId, id: sessionKey }), + signal: AbortSignal.timeout(10_000), + }); + } catch (err) { + // A notification that fails must never disturb the turn that produced it. + console.error('[chat:push] failed to dispatch:', err instanceof Error ? err.message : err); + } + }); +} diff --git a/src/servers/notifications/catalogue.ts b/src/servers/notifications/catalogue.ts index 72e86f57..82b89237 100644 --- a/src/servers/notifications/catalogue.ts +++ b/src/servers/notifications/catalogue.ts @@ -34,7 +34,13 @@ export type NotificationEntry = { target?: string; /** The deep link, `{id}` interpolated. */ route?: string; - /** Grouping key, so like-notifications collapse rather than stack. Defaults to the code's namespace. */ + /** + * Grouping key, so like-notifications collapse rather than stack. Defaults to the code's namespace. + * + * A TEMPLATE, like the others — `chat:{id}` groups per conversation, so two busy chats do not collapse + * into one banner while three pings from the SAME chat still do. A static thread would force a choice + * between those, and the wrong one is "every chat is the same chat". + */ thread?: string; }; @@ -58,6 +64,29 @@ export type Catalogue = { * on a machine that has never installed anything, because it is what first light is proved with. */ const CORE_ENTRIES: Record = { + // ── Chat ── + // + // The hub has no chat screen; chat is Butler, a sibling app. So these route OUT of the hub over + // `officer-chat://`, and the scheme is written here rather than derived from the target: two of the + // suite's twenty-one schemes already break the `officer-` pattern (`officer-videos`, + // `officer-readaloud`), so deriving it works until it silently does not. + // + // The banner cannot name WHICH conversation — a chat's title is content, and content does not travel. + // It says what happened; tapping opens the chat, which is where the name lives. + 'officer/core:chat.turn-ended': { + title: 'Chat', + body: 'Finished its turn', + target: 'chat', + route: 'officer-chat://session/{id}', + thread: 'chat:{id}', + }, + 'officer/core:chat.input-needed': { + title: 'Chat', + body: 'Needs your input', + target: 'chat', + route: 'officer-chat://session/{id}', + thread: 'chat:{id}', + }, 'officer/core:test': { title: 'Officer', body: 'Test notification', @@ -76,12 +105,6 @@ const CORE_ENTRIES: Record = { route: 'officer://jobs/{id}', thread: 'officer/core:job', }, - 'officer/core:agent': { - title: 'Agent finished', - target: 'mobile', - route: 'officer://chat/{id}', - thread: 'officer/core:agent', - }, }; /** FNV-1a, 32-bit. Small, stable across processes, and not a security boundary. */ diff --git a/src/servers/sidecar/notify/render.ts b/src/servers/sidecar/notify/render.ts index 48026569..fd8fc516 100644 --- a/src/servers/sidecar/notify/render.ts +++ b/src/servers/sidecar/notify/render.ts @@ -38,11 +38,12 @@ export function renderNotification(n: Notification, catalogue: Catalogue): Rende return { title: interpolate(entry.title, n), ...(body ? { body } : {}), - thread: entry.thread ?? n.code, + thread: interpolate(entry.thread ?? n.code, n), }; } /** The grouping key for a code, whether or not the catalogue knows it. */ export function threadFor(n: Notification, catalogue: Catalogue): string { - return catalogue.entries[n.code]?.thread ?? n.code; + // Interpolated, so `chat:{id}` groups per conversation rather than lumping every chat together. + return interpolate(catalogue.entries[n.code]?.thread ?? n.code, n); }