diff --git a/docs/push-notifications.md b/docs/push-notifications.md index 3b1df8b5..ff378da5 100644 --- a/docs/push-notifications.md +++ b/docs/push-notifications.md @@ -275,11 +275,28 @@ tokens rotate. ## Failure handling -**APNs** answers inline: `410 Unregistered` or `400 BadDeviceToken` means delete the row immediately. -**FCM** likewise: `UNREGISTERED` / `INVALID_ARGUMENT` / `NOT_FOUND` means delete. +**Only "the app is gone" prunes a row.** The first version pruned on every 4xx Apple or Google offered, +and that was wrong in a way that hides itself. -Everything else increments `failure_count`; three strikes and the row goes. Without this the registry fills -with dead tokens and delivery quietly degrades. +| reason | what it means | what happens | +| ------------------------------------------------------------- | ------------------------------- | ---------------------------------------------------- | +| `Unregistered` (410), `UNREGISTERED`, `NOT_FOUND` | the app was uninstalled | **delete the row** | +| `BadDeviceToken`, `DeviceTokenNotForTopic`, `TopicDisallowed` | **our** configuration is wrong | keep it, log the diagnosis | +| `INVALID_ARGUMENT`, `SENDER_ID_MISMATCH` | **we** sent something malformed | keep it, log the diagnosis | +| anything else | transient | `failure_count += 1`, three strikes and the row goes | + +`BadDeviceToken` is almost always an environment mismatch — a token minted under +`aps-environment: development` sent to the production host, or the reverse. The device is fine, the app is +installed, the registration is good; the server dialled the wrong Apple. **Deleting the row there destroys +the evidence**: the phone re-registers on next launch, the row comes back looking healthy, and the next +send fails identically. Found in the field on the first real push, 2026-08-19. + +`INVALID_ARGUMENT` is worse still, because it is how FCM reports a malformed MESSAGE — including a +non-string in `data`, the mistake this codebase is most likely to make and has a test for. Pruning on it +means one bug in the payload builder wipes every Android device in the registry on a single send. + +**Misconfigured is not counted as a strike either.** Not deleting is not enough on its own: a persistent +mismatch would otherwise reach three failures and delete the row anyway. Same bug, slower fuse. ## Android and GrapheneOS — UNRESOLVED, and it decides Android diff --git a/src/databases/officer_db/src/chat-notification-prefs/index.ts b/src/databases/officer_db/src/chat-notification-prefs/index.ts new file mode 100644 index 00000000..66017c5b --- /dev/null +++ b/src/databases/officer_db/src/chat-notification-prefs/index.ts @@ -0,0 +1 @@ +export { setChatNotifyPreference, getChatNotifyPreference, getChatNotifyPreferences } from './queries'; diff --git a/src/databases/officer_db/src/chat-notification-prefs/queries.ts b/src/databases/officer_db/src/chat-notification-prefs/queries.ts new file mode 100644 index 00000000..23c4ed2a --- /dev/null +++ b/src/databases/officer_db/src/chat-notification-prefs/queries.ts @@ -0,0 +1,39 @@ +import { inArray } from 'drizzle-orm'; +import { db } from '../db'; +import { chatNotificationPrefs } from './schema'; +import type { NotifyPreference } from '../types'; + +/** Set a session's notification preference. Upsert — one row per session, last write wins. */ +export async function setChatNotifyPreference(sessionId: string, preference: NotifyPreference): Promise { + await db + .insert(chatNotificationPrefs) + .values({ sessionId, preference }) + .onConflictDoUpdate({ + target: chatNotificationPrefs.sessionId, + set: { preference, updatedAt: new Date() }, + }); +} + +/** One session's stored preference, or `null` when nothing has been set yet (treat as `'none'`). */ +export async function getChatNotifyPreference(sessionId: string): Promise { + const [row] = await db + .select({ preference: chatNotificationPrefs.preference }) + .from(chatNotificationPrefs) + .where(inArray(chatNotificationPrefs.sessionId, [sessionId])) + .limit(1); + return (row?.preference as NotifyPreference | undefined) ?? null; +} + +/** + * Stored preferences for a page of sessions, as a Map. Batched on purpose — the session list renders + * this per row, and the per-row alternative is one query per session. Returns an empty Map for an empty + * input rather than issuing `IN ()`, which Postgres rejects. + */ +export async function getChatNotifyPreferences(sessionIds: string[]): Promise> { + if (!sessionIds.length) return new Map(); + const rows = await db + .select({ sessionId: chatNotificationPrefs.sessionId, preference: chatNotificationPrefs.preference }) + .from(chatNotificationPrefs) + .where(inArray(chatNotificationPrefs.sessionId, sessionIds)); + return new Map(rows.map((r) => [r.sessionId, r.preference as NotifyPreference])); +} diff --git a/src/databases/officer_db/src/chat-notification-prefs/schema.ts b/src/databases/officer_db/src/chat-notification-prefs/schema.ts new file mode 100644 index 00000000..c236c6c2 --- /dev/null +++ b/src/databases/officer_db/src/chat-notification-prefs/schema.ts @@ -0,0 +1,30 @@ +import { pgTable, text, timestamp, check } from 'drizzle-orm/pg-core'; +import { sql } from 'drizzle-orm'; + +// Whether a chat session should tell you about itself — the bell menu on a chat's header. +// +// There is no `chat_sessions` table (a session IS its transcript, per `chat-session-models/schema.ts`), +// so this claims only the field the transcript cannot express, keyed the same way that table is. + +const NOTIFY_PREFERENCES = ['every_turn', 'input_needed', 'none'] as const; + +export const chatNotificationPrefs = pgTable( + 'chat_notification_prefs', + { + /** The session's transcript uuid — same natural key as `chat_session_models.sessionId`. */ + sessionId: text('session_id').primaryKey(), + preference: text('preference').notNull().default('none'), + updatedAt: timestamp('updated_at', { withTimezone: true }).notNull().defaultNow(), + }, + (table) => [ + // sql.raw per value — an interpolated string binds as a parameter, which Postgres refuses inside a + // CHECK and breaks push for the whole schema. See src/databases/CLAUDE.md. + check( + 'ck_chat_notification_prefs_preference', + sql`${table.preference} IN (${sql.join( + NOTIFY_PREFERENCES.map((p) => sql.raw(`'${p}'`)), + sql`, `, + )})`, + ), + ], +); diff --git a/src/databases/officer_db/src/index.ts b/src/databases/officer_db/src/index.ts index fe9850cc..957b50b6 100644 --- a/src/databases/officer_db/src/index.ts +++ b/src/databases/officer_db/src/index.ts @@ -34,6 +34,7 @@ export * from './auth'; export * from './permissions'; export * from './chat-events'; export * from './chat-session-models'; +export * from './chat-notification-prefs'; export * from './ai-provider-usage'; export * from './command-hq'; export * from './dashboards'; diff --git a/src/databases/officer_db/src/schema.ts b/src/databases/officer_db/src/schema.ts index c66373af..0a36ce10 100644 --- a/src/databases/officer_db/src/schema.ts +++ b/src/databases/officer_db/src/schema.ts @@ -32,6 +32,7 @@ export * from './server/schema'; // server_config (SMTP lives here), server_inte export * from './pipeline-jobs/schema'; // pipeline_jobs export * from './chat-events/schema'; // chat_session_events export * from './chat-session-models/schema'; // chat_session_models +export * from './chat-notification-prefs/schema'; // chat_notification_prefs export * from './ai-provider-usage/schema'; // ai_provider_usage — last usage reading per AI account export * from './command-hq/schema'; // command_hq_categories, command_hq_sessions, command_hq_tags, command_hq_session_tags export * from './agent-panels/schema'; // agent_panels diff --git a/src/databases/officer_db/src/types.ts b/src/databases/officer_db/src/types.ts index 40bcfbfc..4bf0f2ca 100644 --- a/src/databases/officer_db/src/types.ts +++ b/src/databases/officer_db/src/types.ts @@ -52,6 +52,14 @@ export type DockConfigInsert = typeof Schema.dockConfigs.$inferInsert; // ── Saved Sessions ── +/** Kept as a literal union, not inferred from the `text` column — the schema's CHECK is the only thing + * that actually enforces this set; this is what lets every other layer (queries, routes, client) agree + * with it instead of each re-typing the three strings. */ +export type NotifyPreference = 'every_turn' | 'input_needed' | 'none'; + +export type ChatNotificationPrefSelect = typeof Schema.chatNotificationPrefs.$inferSelect; +export type ChatNotificationPrefInsert = typeof Schema.chatNotificationPrefs.$inferInsert; + // ── Dashboards ── export type DashboardSelect = typeof Schema.dashboards.$inferSelect; diff --git a/src/server.tsx b/src/server.tsx index fc486e03..ffd8cd05 100644 --- a/src/server.tsx +++ b/src/server.tsx @@ -17,6 +17,7 @@ import { isWsProviderAllowed } from './servers/permissions/authorize'; 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 { taskRunnerWebsocket } from './servers/api/tasks/task-executor'; import { pipelineWebsocket } from './servers/api/tasks/pipeline-executor'; // import { desktopWebsocket } from './servers/api/desktop/websocket'; @@ -53,7 +54,7 @@ type WSData = { userId: number; email: string; username: string; - provider: 'terminal' | 'chat' | 'task-runner' | 'pipeline' | 'desktop' | 'sidecar'; + provider: 'terminal' | 'chat' | 'chat-notify' | 'task-runner' | 'pipeline' | 'desktop' | 'sidecar'; sessionId?: string; cwd?: string; command?: string; @@ -142,6 +143,7 @@ const handlers: Record = { terminal: terminalWebsocket, 'dev-proxy': devProxyWebsocket, chat: chatWebsocket, + 'chat-notify': chatNotifyWebsocket, 'task-runner': taskRunnerWebsocket, pipeline: pipelineWebsocket, // desktop: desktopWebsocket, @@ -292,7 +294,7 @@ console.log( async function upgradeWs( req: Request, server: any, - provider: 'terminal' | 'chat' | 'task-runner' | 'pipeline' | 'desktop', + provider: 'terminal' | 'chat' | 'chat-notify' | 'task-runner' | 'pipeline' | 'desktop', ) { const token = new URL(req.url).searchParams.get('token'); if (!token) return new Response('Unauthorized', { status: 401 }); @@ -422,6 +424,7 @@ const server = serve({ '/api/tasks/pipeline/ws': (req, server) => upgradeWs(req, server, 'pipeline'), '/api/terminal/ws': (req, server) => upgradeWs(req, server, 'terminal'), '/api/chat/ws': (req, server) => upgradeWs(req, server, 'chat'), + '/api/chat/notify/ws': (req, server) => upgradeWs(req, server, 'chat-notify'), // '/api/desktop/ws': (req, server) => upgradeWs(req, server, 'desktop'), // CalDAV/CardDAV. These live OUTSIDE /api because DAV clients are given a bare domain and probe // fixed, spec-defined paths — `/.well-known/caldav` unauthenticated, before they hold any diff --git a/src/servers/api/chat/chat.ts b/src/servers/api/chat/chat.ts index 5068537c..0fb53c36 100644 --- a/src/servers/api/chat/chat.ts +++ b/src/servers/api/chat/chat.ts @@ -4,7 +4,16 @@ import { pauseBackgroundTask, restartBackgroundTask } from './task-restart'; import { createRouter } from '../../create-router'; import * as errors from '@@/custom-errors'; import * as sidecar from '@@/sidecar-registry'; -import { getUserSettings, getChatSessionModel, getChatSessionModels, listCommandHqSessionIds } from 'officerdb'; +import { + getUserSettings, + getChatSessionModel, + getChatSessionModels, + listCommandHqSessionIds, + getChatNotifyPreference, + getChatNotifyPreferences, + setChatNotifyPreference, +} from 'officerdb'; +import type { NotifyPreference } from 'officerdb/types'; import { listClaudePwds, listClaudeSessions, @@ -156,13 +165,24 @@ chatRouter.get('/sessions', async (ctx) => { () => new Map(), ); + // Same batched-not-per-row shape as `models` above, for the same reason — the list renders a bell + // badge per row, and per-row queries do not scale to a page of a hundred sessions. + const notifyPrefs = await getChatNotifyPreferences(sessions.map((s) => s.id)).catch( + () => new Map(), + ); + return ctx.json({ sessions: sessions.map((s) => { const stored = models.get(s.id); // Spelled out field by field rather than spread: this row goes onto the wire untyped (`ctx.json`), // so widening the stored shape from a string to a record would have silently shipped an OBJECT as // `model` and broken every consumer that calls `.split('/')` on it — with nothing to catch it. - return { ...s, model: stored?.model ?? null, planMode: stored?.planMode ?? false }; + return { + ...s, + model: stored?.model ?? null, + planMode: stored?.planMode ?? false, + notifyPreference: notifyPrefs.get(s.id) ?? 'none', + }; }), }); }); @@ -217,6 +237,7 @@ chatRouter.get('/sessions/:id', async (ctx) => { */ const stored = await getChatSessionModel(id).catch(() => null); const officerModel = stored?.model ?? officerModelForApiModel(detail.model) ?? null; + const notifyPreference = (await getChatNotifyPreference(id).catch(() => null)) ?? 'none'; return ctx.json({ ...detail, @@ -226,6 +247,7 @@ chatRouter.get('/sessions/:id', async (ctx) => { // Whether this conversation is planning, so the composer's toggle comes back in the state the user left // it. Only a stored row can answer — plan mode leaves no trace in the transcript at all. planMode: stored?.planMode ?? false, + notifyPreference, messages, total, offset: windowed ? start : 0, @@ -335,6 +357,22 @@ chatRouter.patch('/sessions/:id/title', async (ctx) => { return ctx.json({ ok: true }); }); +const NOTIFY_PREFERENCES: NotifyPreference[] = ['every_turn', 'input_needed', 'none']; + +// PATCH /chat/sessions/:id/notify-pref — set this session's bell preference. No ownership check beyond +// `chatIdentity()` above: the row is keyed only on sessionId, same trust boundary the title route uses, +// and a preference on a session the caller cannot otherwise reach is inert (nothing will ever fire it). +chatRouter.patch('/sessions/:id/notify-pref', async (ctx) => { + await chatIdentity(ctx.get('user')); + const id = ctx.req.param('id'); + const { preference } = await ctx.req.json<{ preference?: string }>(); + if (!preference || !NOTIFY_PREFERENCES.includes(preference as NotifyPreference)) { + return ctx.text('preference must be one of: ' + NOTIFY_PREFERENCES.join(', '), 400); + } + await setChatNotifyPreference(id, preference as NotifyPreference); + return ctx.json({ preference }); +}); + // GET /chat/tasks/:id — what a background task is doing right now: a subagent's own trace, or the tail of // a backgrounded shell's log. Polled by the tray above the chat input while the task is running. // diff --git a/src/servers/api/chat/notify-delivery.ts b/src/servers/api/chat/notify-delivery.ts new file mode 100644 index 00000000..ee1b362c --- /dev/null +++ b/src/servers/api/chat/notify-delivery.ts @@ -0,0 +1,70 @@ +import type { ServerWebSocket } from 'bun'; +import * as sidecar from '@@/sidecar-registry'; +import { getChatNotifyPreference, getUserById } from 'officerdb'; +import { resolveHomeDir } from '@@/user-home'; +import { liveSessionTitle } from './claude-sessions'; + +// The browser-facing half of the bell feature: a per-user, session-independent socket that the shell +// holds open once, and the fan-out that decides whether a sidecar's turn-end/input-needed ping is +// actually worth telling anyone about. +// +// Deliberately separate from `chatWebsocket` (websocket.ts) — that one is bound to exactly one +// `sessionId` per connection, and retrofitting a sessionless "tell me about ALL my chats" mode into its +// already-dense state machine risked entangling two very different lifecycles. This one carries nothing +// but small, best-effort pings; a missed one (a reconnect gap, a restart) is not replayed and does not +// need to be — the badge and the stored preference are the durable truth, this socket is just a bell. + +type WSData = { userId: number }; + +const socketsByUser = new Map>>(); + +export const chatNotifyWebsocket = { + open(ws: ServerWebSocket) { + const { userId } = ws.data; + let set = socketsByUser.get(userId); + if (!set) { + set = new Set(); + socketsByUser.set(userId, set); + } + set.add(ws); + }, + message() { + // No inbound protocol — this socket only ever receives, it never sends a session id or a command. + }, + close(ws: ServerWebSocket) { + const { userId } = ws.data; + const set = socketsByUser.get(userId); + if (!set) return; + set.delete(ws); + if (set.size === 0) socketsByUser.delete(userId); + }, +}; + +/** Best-effort — a session with no group/cwd on record (never taken a turn, or the transcript vanished + * mid-lookup) still gets a notification, just with a generic label rather than the chat's real name. */ +async function resolveTitle(userId: number, sessionKey: string): Promise { + try { + const user = await getUserById(userId); + const resolved = await resolveHomeDir(userId); + if (!user || !resolved.ok) return 'Chat'; + const who = { email: user.email, home: resolved.home, osUser: resolved.osUser, isOwner: resolved.isOwner }; + return liveSessionTitle(who, sessionKey)?.title ?? 'Chat'; + } catch { + return 'Chat'; + } +} + +sidecar.onClaudeNotify(async (sessionKey, userId, kind) => { + const sockets = socketsByUser.get(userId); + if (!sockets?.size) return; // nobody listening — skip the DB round trip entirely + + const preference = await getChatNotifyPreference(sessionKey).catch(() => null); + const matches = + (kind === 'turn-ended' && preference === 'every_turn') || + (kind === 'input-needed' && preference === 'input_needed'); + if (!matches) return; + + const title = await resolveTitle(userId, sessionKey); + const payload = JSON.stringify({ type: 'notify', sessionId: sessionKey, kind, title }); + for (const ws of sockets) ws.send(payload); +}); diff --git a/src/servers/permissions/registry.test.ts b/src/servers/permissions/registry.test.ts index 8f775133..50085650 100644 --- a/src/servers/permissions/registry.test.ts +++ b/src/servers/permissions/registry.test.ts @@ -22,7 +22,7 @@ import { assertPermissionTotality, isExemptApiPath } from './totality'; // switched off 2026-08-13. It was listed here until 2026-08-18 and made this file's own totality tests // fail for five days — the mirror claiming a provider the server does not serve, which is precisely the // drift the comment above warns about, caught by the check and then ignored because it was "the baseline". -const REAL_WS = ['terminal', 'chat', 'task-runner', 'pipeline', 'sidecar']; +const REAL_WS = ['terminal', 'chat', 'chat-notify', 'task-runner', 'pipeline', 'sidecar']; const realApi = () => [...new Set(PERMISSIONS.flatMap((c) => c.api))]; const surface = () => ({ apiPrefixes: [...realApi(), '/auth', '/landing-page-data', '/waitlist', '/sidecar'], diff --git a/src/servers/permissions/registry.ts b/src/servers/permissions/registry.ts index 2489b9fa..a678f504 100644 --- a/src/servers/permissions/registry.ts +++ b/src/servers/permissions/registry.ts @@ -252,7 +252,11 @@ const CORE_REGISTRY: Permission[] = [ // two booleans about the caller's own home and nothing about anyone else, which is what lets a member be // told "run `claude` once to sign in" instead of being shown a tile that 403s. api: ['/chat', '/agent-status'], - ws: ['chat'], + // 'chat-notify' is a session-independent socket (the bell feature): it carries no sessionId and no + // transcript, just a ping that a chat this account can already reach has finished a turn or needs + // input. Riding this permission rather than a new one, because it is meaningless without `chat` + // already granted. + ws: ['chat', 'chat-notify'], routes: ['/chat'], }, // Confined rather than execution since 2026-08-11. Every path under `/file-browser` resolves its root diff --git a/src/servers/sidecar-registry.ts b/src/servers/sidecar-registry.ts index 6227d930..ef220945 100644 --- a/src/servers/sidecar-registry.ts +++ b/src/servers/sidecar-registry.ts @@ -428,6 +428,17 @@ export function onClaudeMessage(handler: (sessionKey: string, msg: TurnMessage, }); } +// See `PersistentSession.notify` in claude-manager.ts — a separate, non-durable ping, not part of the +// transcript the message above relays. +export function onClaudeNotify( + handler: (sessionKey: string, userId: number, kind: 'turn-ended' | 'input-needed') => void, +): () => void { + return on('claude:notify', (ev) => { + if (ev.type !== 'claude:notify') return; + handler(ev.sessionKey, ev.userId, ev.kind); + }); +} + // ── OpenCode Code (single sidecar, handling 'opencode') ── export async function spawnOpenCodeStreaming(params: OpenCodeRunParams): Promise { diff --git a/src/servers/sidecar/claude/claude-manager.ts b/src/servers/sidecar/claude/claude-manager.ts index 447c4f1d..f76f397e 100644 --- a/src/servers/sidecar/claude/claude-manager.ts +++ b/src/servers/sidecar/claude/claude-manager.ts @@ -227,6 +227,14 @@ type PersistentSession = { closeInput: () => void; abort: AbortController; emit: (event: ChatEvent) => void; + /** + * A lightweight, non-durable ping for notification delivery — deliberately separate from `emit`. + * `ChatEvent`/`TurnMessage` is the durable, replayed transcript; this is not part of it, so it fires + * even for events (`plan:proposed` parking, an `AskUserQuestion` gate) that have no event of their own + * to piggyback on. Officer decides whether to actually notify (preference lookup + fan-out) — this + * only says "something notify-worthy just happened for this session." + */ + notify: (kind: 'turn-ended' | 'input-needed') => void; isGenerating: boolean; pendingTasks: Set; // background tasks started but not yet notified; suppress idle-GC while non-empty /** @@ -390,6 +398,7 @@ function dropDeadSession(session: PersistentSession, detail: string): void { if (!wasGenerating) return; session.emit({ type: 'error', message: diagnosis.message }); + session.notify('turn-ended'); } /** Wrap a spawn so the session self-heals when its child goes away. */ @@ -502,7 +511,11 @@ function armIdle(session: PersistentSession): void { }, IDLE_TIMEOUT_MS); } -function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: ChatEvent) => void): PersistentSession { +function createSession( + params: ClaudeSpawnStreamingParams, + onEvent: (event: ChatEvent) => void, + onNotify: (kind: 'turn-ended' | 'input-needed') => void, +): PersistentSession { const { sessionKey } = params; const input = makeInputQueue(); const abort = new AbortController(); @@ -515,6 +528,7 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat closeInput: () => input.close(), abort, emit: onEvent, + notify: onNotify, isGenerating: false, pendingTasks: new Set(), planMode: params.planMode === true, @@ -767,6 +781,7 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat // Whatever ended the turn, the interrupt is spent — a later genuine error must not wear it. session.interrupted = false; if (session.pendingTasks.size === 0) armIdle(session); + session.notify('turn-ended'); } // Any event at all is a sign of life, so push the stall deadline back. Placed after the branch // above so a terminal event disarms rather than re-arms it. @@ -804,6 +819,7 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat export async function spawnClaudeStreaming( params: ClaudeSpawnStreamingParams, onEvent: (event: ChatEvent) => void, + onNotify: (kind: 'turn-ended' | 'input-needed') => void, ): Promise { let session = sessions.get(params.sessionKey); if (session && session.userId !== params.userId) { @@ -817,10 +833,11 @@ export async function spawnClaudeStreaming( } if (session) { session.emit = onEvent; // adopt the latest emitter (equivalent across turns; keeps events flowing) + session.notify = onNotify; await applyModelChange(session, params.model); await applyPlanModeChange(session, params.planMode); } else { - session = createSession(params, onEvent); + session = createSession(params, onEvent, onNotify); } session.pushTurn(params.prompt, params.images); } @@ -903,6 +920,7 @@ async function planGate( session.pendingPlans.set(requestId, { resolve, timer }); session.emit({ type: 'plan:proposed', requestId, plan }); + session.notify('input-needed'); }); session.emit({ type: 'plan:decided', requestId, approved: decision.approve, feedback: decision.feedback }); @@ -958,6 +976,7 @@ async function questionGate(session: PersistentSession, toolCallId: string): Pro }, QUESTION_ANSWER_TIMEOUT_MS); session.pendingQuestions.set(toolCallId, { resolve, timer }); + session.notify('input-needed'); }); } diff --git a/src/servers/sidecar/claude/user-instance.ts b/src/servers/sidecar/claude/user-instance.ts index db8f0f0c..a78d16e7 100644 --- a/src/servers/sidecar/claude/user-instance.ts +++ b/src/servers/sidecar/claude/user-instance.ts @@ -242,6 +242,11 @@ const sessionLog = createSessionLogStore((d) => connection.send({ type: 'claude:message', sessionKey: d.sessionId, msg: d.msg, seq: d.seq }), ); +// Notification delivery is a separate, non-durable ping — see `PersistentSession.notify` in +// claude-manager.ts for why it isn't folded into the message above. +const notifySignal = (sessionKey: string, userId: number, kind: 'turn-ended' | 'input-needed') => + connection.send({ type: 'claude:notify', sessionKey, userId, kind }); + // ── Command handlers ── type ReplyFn = (msg: SidecarEvent) => void; @@ -269,7 +274,11 @@ async function handleCommand(cmd: SidecarCommand, reply: ReplyFn) { const { sessionKey, durable = true } = cmd.params; claudeManager - .spawnClaudeStreaming(cmd.params, (event) => sessionLog.push(sessionKey, event, durable)) + .spawnClaudeStreaming( + cmd.params, + (event) => sessionLog.push(sessionKey, event, durable), + (kind) => notifySignal(sessionKey, cmd.params.userId, kind), + ) .catch((err) => { // Through the log like any other output, so a failure to start is durable and replayable too. const message = err instanceof Error ? err.message : String(err); diff --git a/src/servers/sidecar/notify/apns.test.ts b/src/servers/sidecar/notify/apns.test.ts index b900bfe1..45705cc6 100644 --- a/src/servers/sidecar/notify/apns.test.ts +++ b/src/servers/sidecar/notify/apns.test.ts @@ -2,6 +2,7 @@ import { describe, test, expect } from 'bun:test'; import { generateKeyPairSync, createVerify } from 'node:crypto'; import { ApnsAuthToken } from './apns-jwt'; import { buildApnsPayload, collapseId } from './apns-payload'; +import { __testing } from './apns'; import { renderNotification, FALLBACK_TITLE } from './render'; import type { Notification } from './types'; import type { Catalogue } from '../../notifications/catalogue'; @@ -127,3 +128,32 @@ describe('APNs payload', () => { expect(renderNotification({ code: 'nope/nope:x', userId: 1 }, catalogue).title).toBe(FALLBACK_TITLE); }); }); + +describe('what a failure does to the device row', () => { + // Reported from the field 2026-08-19: a first real push failed with BadDeviceToken and the server + // deleted the registration. The build was production-signed and the row said sandbox — a configuration + // mismatch, with the device and the app both perfectly fine. + // + // Pruning there destroys the evidence: the phone re-registers on next launch, the row returns looking + // healthy, and the next send fails identically. You can lose a day to that loop. + test('only Unregistered means the app is gone', () => { + expect(__testing.dispositionFor('Unregistered')).toBe('gone'); + }); + + test('an environment mismatch keeps the row so it can be diagnosed', () => { + expect(__testing.dispositionFor('BadDeviceToken')).toBe('misconfigured'); + expect(__testing.dispositionFor('DeviceTokenNotForTopic')).toBe('misconfigured'); + }); + + // Not merely "not deleted" — misconfigured must not count as a strike either, or a persistent mismatch + // deletes the row on the third send. Same bug, slower fuse. + test('misconfigured is neither pruned nor a strike', () => { + expect(__testing.dispositionFor('BadDeviceToken')).not.toBe('gone'); + expect(__testing.dispositionFor('BadDeviceToken')).not.toBe('transient'); + }); + + test('anything unrecognised is transient, so it counts toward three strikes', () => { + expect(__testing.dispositionFor('ServiceUnavailable')).toBe('transient'); + expect(__testing.dispositionFor('timeout')).toBe('transient'); + }); +}); diff --git a/src/servers/sidecar/notify/apns.ts b/src/servers/sidecar/notify/apns.ts index c6c8018b..b90018e8 100644 --- a/src/servers/sidecar/notify/apns.ts +++ b/src/servers/sidecar/notify/apns.ts @@ -20,8 +20,36 @@ const HOSTS = { sandbox: 'https://api.sandbox.push.apple.com', } as const; -/** Apple's reasons for "this token is dead" — the row goes immediately rather than after three strikes. */ -const DEAD_TOKEN_REASONS = new Set(['BadDeviceToken', 'Unregistered', 'DeviceTokenNotForTopic']); +// ── Telling "the app is gone" apart from "we are misconfigured" ── +// +// These used to be one set, and every one of them deleted the row. That is right for exactly one reason +// and actively harmful for the others. +// +// `Unregistered` (410) means Apple knows the app was uninstalled. The token will never work again, so +// pruning is correct and a registry full of those is how delivery quietly degrades. +// +// `BadDeviceToken` and `DeviceTokenNotForTopic` are CONFIGURATION problems, and almost always the same +// one: a token minted under `aps-environment: development` sent to the production host, or the reverse. +// The device is fine, the app is installed, the registration is good — the server dialled the wrong +// Apple. Deleting the row there destroys the evidence: the phone re-registers on next launch, the row +// comes back looking healthy, and the next send fails identically. You can lose a day to that loop +// without ever seeing the thing that is actually wrong. +// +// So: only `Unregistered` prunes. +const GONE_REASONS = new Set(['Unregistered']); + +/** + * Wrong on OUR side, not the device's. Never pruned, and deliberately not counted toward the + * three-strikes rule either — a persistent environment mismatch would otherwise delete the row on the + * third send, which is the same bug with a slower fuse. + */ +const MISCONFIGURED_REASONS = new Set(['BadDeviceToken', 'DeviceTokenNotForTopic', 'TopicDisallowed']); + +/** What to do about one failed send. */ +type Disposition = 'gone' | 'misconfigured' | 'transient'; + +const dispositionFor = (reason: string): Disposition => + GONE_REASONS.has(reason) ? 'gone' : MISCONFIGURED_REASONS.has(reason) ? 'misconfigured' : 'transient'; // Secret store first, env second. // @@ -73,7 +101,7 @@ function session(host: string): http2.ClientHttp2Session { return s; } -type SendOutcome = { ok: boolean; dead: boolean; reason?: string }; +type SendOutcome = { ok: boolean; disposition?: Disposition; reason?: string }; function sendOne( host: string, @@ -109,7 +137,7 @@ function sendOne( raw += chunk; }); req.on('end', () => { - if (status === 200) return resolve({ ok: true, dead: false }); + if (status === 200) return resolve({ ok: true }); // Apple answers failures with { "reason": "BadDeviceToken" } and nothing else useful. let reason = `status ${status}`; try { @@ -117,13 +145,13 @@ function sendOne( } catch { /* keep the status */ } - resolve({ ok: false, dead: DEAD_TOKEN_REASONS.has(reason), reason }); + resolve({ ok: false, disposition: dispositionFor(reason), reason }); }); - req.on('error', (err) => resolve({ ok: false, dead: false, reason: err.message })); + req.on('error', (err) => resolve({ ok: false, disposition: 'transient', reason: err.message })); req.setTimeout(10_000, () => { req.close(); - resolve({ ok: false, dead: false, reason: 'timeout' }); + resolve({ ok: false, disposition: 'transient', reason: 'timeout' }); }); req.end(body); @@ -159,14 +187,27 @@ export const apnsChannel: Channel = { failed++; console.error(`[notify:apns] ${d.bundleId} ${d.environment}: ${outcome.reason}`); - if (outcome.dead) { - // An uninstalled app or a rotated token. Delete now — retrying cannot help, and a registry - // full of dead tokens is how delivery quietly degrades. + if (outcome.disposition === 'gone') { + // Apple says the app is uninstalled. Retrying cannot help. dead.push(d.token); await deletePushDevice(d.token); - } else { - await recordPushFailure(d.token); + return; } + + if (outcome.disposition === 'misconfigured') { + // Left ALONE — not deleted, and not counted as a strike. Say what is actually wrong instead, + // because the reason code alone sends people to look at the device. + const other = d.environment === 'production' ? 'sandbox' : 'production'; + console.error( + `[notify:apns] '${outcome.reason}' is a configuration mismatch, not a dead device. This row ` + + `says environment='${d.environment}' and topic='${d.bundleId}'. If the installed build is ` + + `signed for '${other}', the registration is wrong and the device row is kept so it can be ` + + `corrected — check aps-environment in the SHIPPED artefact, not the intermediate build.`, + ); + return; + } + + await recordPushFailure(d.token); }), ); @@ -185,3 +226,6 @@ export function closeApnsSessions(): void { } sessions.clear(); } + +/** Exposed for tests: classifying a failure is the whole of the pruning policy. */ +export const __testing = { dispositionFor }; diff --git a/src/servers/sidecar/notify/fcm.ts b/src/servers/sidecar/notify/fcm.ts index e7833ccf..2e95de63 100644 --- a/src/servers/sidecar/notify/fcm.ts +++ b/src/servers/sidecar/notify/fcm.ts @@ -15,8 +15,27 @@ import { FcmAccessToken, parseServiceAccount } from './fcm-auth'; const ENDPOINT = (projectId: string) => `https://fcm.googleapis.com/v1/projects/${projectId}/messages:send`; -/** Google's ways of saying "this token is dead" — the row goes immediately. */ -const DEAD_TOKEN_CODES = new Set(['UNREGISTERED', 'INVALID_ARGUMENT', 'NOT_FOUND']); +// ── "The app is gone" versus "we sent something wrong" ── +// +// `UNREGISTERED` and `NOT_FOUND` mean the registration is dead: uninstalled, or a token that has been +// replaced. Pruning is correct. +// +// `INVALID_ARGUMENT` is NOT that, and it used to be in the same set. It is how FCM reports a malformed +// MESSAGE — including the one this codebase is most likely to produce, a non-string value in `data`, +// which has its own test for exactly that reason. Deleting on it means a single bug in the payload +// builder wipes every Android device from the registry on one send, and the symptom is "Android push +// stopped working" with no trace of why. +// +// `SENDER_ID_MISMATCH` is a wrong service account, i.e. also ours. +const GONE_CODES = new Set(['UNREGISTERED', 'NOT_FOUND']); + +/** Wrong on OUR side. Never pruned, and not counted toward three strikes either. */ +const MISCONFIGURED_CODES = new Set(['INVALID_ARGUMENT', 'SENDER_ID_MISMATCH', 'THIRD_PARTY_AUTH_ERROR']); + +type Disposition = 'gone' | 'misconfigured' | 'transient'; + +const dispositionFor = (code: string): Disposition => + GONE_CODES.has(code) ? 'gone' : MISCONFIGURED_CODES.has(code) ? 'misconfigured' : 'transient'; // Store first, env second — same reasoning as apns.ts. const raw = (() => { @@ -60,7 +79,7 @@ function buildMessage(token: string, n: Notification, ctx: SendContext): FcmMess return { message: { token, data, android: { priority: 'high' } } }; } -type SendOutcome = { ok: boolean; dead: boolean; reason?: string }; +type SendOutcome = { ok: boolean; disposition?: Disposition; reason?: string }; async function sendOne( accessToken: string, @@ -77,7 +96,7 @@ async function sendOne( signal: AbortSignal.timeout(10_000), }); - if (res.ok) return { ok: true, dead: false }; + if (res.ok) return { ok: true }; // FCM errors look like { error: { status: 'UNREGISTERED', message: '…' } }. The message can echo the // token, so only the status is logged. @@ -88,9 +107,9 @@ async function sendOne( } catch { /* keep the http status */ } - return { ok: false, dead: DEAD_TOKEN_CODES.has(status), reason: status }; + return { ok: false, disposition: dispositionFor(status), reason: status }; } catch (err) { - return { ok: false, dead: false, reason: err instanceof Error ? err.message : 'send failed' }; + return { ok: false, disposition: 'transient', reason: err instanceof Error ? err.message : 'send failed' }; } } @@ -131,12 +150,21 @@ export const fcmChannel: Channel = { failed++; console.error(`[notify:fcm] ${d.bundleId}: ${outcome.reason}`); - if (outcome.dead) { + if (outcome.disposition === 'gone') { dead.push(d.token); await deletePushDevice(d.token); - } else { - await recordPushFailure(d.token); + return; } + + if (outcome.disposition === 'misconfigured') { + console.error( + `[notify:fcm] '${outcome.reason}' is a problem with what WE sent, not with this device. ` + + `The row is kept so it can be diagnosed — check the message shape and the service account.`, + ); + return; + } + + await recordPushFailure(d.token); }), ); diff --git a/src/servers/sidecar/protocol.ts b/src/servers/sidecar/protocol.ts index 2ebde5c6..2c1c6a81 100644 --- a/src/servers/sidecar/protocol.ts +++ b/src/servers/sidecar/protocol.ts @@ -138,6 +138,10 @@ export type SidecarEvent = // and `seq` is its cursor id there; officer relays it verbatim. No `seq` means it is not durable — // an `assistant:delta` (superseded by the text that follows) or a message whose write failed. | { type: 'claude:message'; sessionKey: string; msg: TurnMessage; seq?: number } + // A lightweight, non-durable ping for notification delivery — deliberately NOT part of + // chat_session_events/TurnMessage. Officer decides whether to actually notify (preference lookup + + // fan-out to the browser); this only says "something notify-worthy just happened for this session." + | { type: 'claude:notify'; sessionKey: string; userId: number; kind: 'turn-ended' | 'input-needed' } | { type: 'claude:result'; id: string; result: ClaudeCodeResult } | { type: 'claude:error'; id: string; error: string } | { type: 'claude:killed'; id: string }