diff --git a/src/databases/officer_db/src/chat-notification-prefs/queries.ts b/src/databases/officer_db/src/chat-notification-prefs/queries.ts index 23c4ed2a..387eff82 100644 --- a/src/databases/officer_db/src/chat-notification-prefs/queries.ts +++ b/src/databases/officer_db/src/chat-notification-prefs/queries.ts @@ -1,25 +1,32 @@ -import { inArray } from 'drizzle-orm'; +import { and, eq, 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 { +// Every query here is scoped to a user. That is the whole point of the column — see ./schema.ts. + +/** Set one session's notification preference for one user. Upsert; last write wins. */ +export async function setChatNotifyPreference( + userId: number, + sessionId: string, + preference: NotifyPreference, +): Promise { await db .insert(chatNotificationPrefs) - .values({ sessionId, preference }) + .values({ userId, sessionId, preference }) .onConflictDoUpdate({ - target: chatNotificationPrefs.sessionId, + // The unique index is the arbiter — a composite primary key would churn on every push. + target: [chatNotificationPrefs.userId, 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 { +/** One session's stored preference, or `null` when nothing has been set (treat as `'none'`). */ +export async function getChatNotifyPreference(userId: number, sessionId: string): Promise { const [row] = await db .select({ preference: chatNotificationPrefs.preference }) .from(chatNotificationPrefs) - .where(inArray(chatNotificationPrefs.sessionId, [sessionId])) + .where(and(eq(chatNotificationPrefs.userId, userId), eq(chatNotificationPrefs.sessionId, sessionId))) .limit(1); return (row?.preference as NotifyPreference | undefined) ?? null; } @@ -29,11 +36,14 @@ export async function getChatNotifyPreference(sessionId: string): Promise> { +export async function getChatNotifyPreferences( + userId: number, + 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)); + .where(and(eq(chatNotificationPrefs.userId, userId), 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 index c236c6c2..e6f7fcb1 100644 --- a/src/databases/officer_db/src/chat-notification-prefs/schema.ts +++ b/src/databases/officer_db/src/chat-notification-prefs/schema.ts @@ -1,22 +1,42 @@ -import { pgTable, text, timestamp, check } from 'drizzle-orm/pg-core'; +import { pgTable, serial, integer, text, timestamp, check, uniqueIndex } from 'drizzle-orm/pg-core'; import { sql } from 'drizzle-orm'; +import { users } from '../auth/schema'; // 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. +// so this claims only the field the transcript cannot express. +// +// ── Why `user_id` is here, and not optional ── +// +// This was keyed on `session_id` alone, on the reasoning that a preference for a session the caller +// cannot reach is inert. It is not: delivery targets the userId the SIDECAR reports for the running +// session, while the preference was read globally by session id. So any account holding `chat` that knew +// a session's uuid could set `none` on someone else's conversation and silently switch off their +// notifications — or `every_turn` to make their phone buzz all night. +// +// Scoped both ways now: the row belongs to a user, and delivery only ever reads that user's own. +// +// Surrogate `id` plus a unique INDEX rather than a composite primary key — a composite `primaryKey` is +// re-created on every `db:push` by a drizzle-kit diffing bug with no index form to escape to. See +// src/databases/CLAUDE.md → "Composite keys". const NOTIFY_PREFERENCES = ['every_turn', 'input_needed', 'none'] as const; export const chatNotificationPrefs = pgTable( 'chat_notification_prefs', { + id: serial('id').primaryKey(), + userId: integer('user_id') + .notNull() + .references(() => users.id, { onDelete: 'cascade' }), /** The session's transcript uuid — same natural key as `chat_session_models.sessionId`. */ - sessionId: text('session_id').primaryKey(), + sessionId: text('session_id').notNull(), preference: text('preference').notNull().default('none'), updatedAt: timestamp('updated_at', { withTimezone: true }).notNull().defaultNow(), }, (table) => [ + uniqueIndex('uq_chat_notification_prefs_user_session').on(table.userId, table.sessionId), // 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( diff --git a/src/servers/api/chat/chat.ts b/src/servers/api/chat/chat.ts index 0fb53c36..680409fb 100644 --- a/src/servers/api/chat/chat.ts +++ b/src/servers/api/chat/chat.ts @@ -167,9 +167,10 @@ chatRouter.get('/sessions', async (ctx) => { // 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(), - ); + const notifyPrefs = await getChatNotifyPreferences( + user.id, + sessions.map((s) => s.id), + ).catch(() => new Map()); return ctx.json({ sessions: sessions.map((s) => { @@ -194,7 +195,8 @@ chatRouter.get('/sessions', async (ctx) => { // response carries `total` (full length) and `offset` (absolute index of messages[0]) so the client knows // where the window sits and whether older messages remain above it. chatRouter.get('/sessions/:id', async (ctx) => { - const who = await chatIdentity(ctx.get('user')); + const user = ctx.get('user'); + const who = await chatIdentity(user); const id = ctx.req.param('id'); const cwd = cwdOf(ctx, who.home); // Fall back to a by-id scan when the (default) cwd doesn't hold it — a fresh /chat/ deep-link/refresh @@ -237,7 +239,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'; + const notifyPreference = (await getChatNotifyPreference(user.id, id).catch(() => null)) ?? 'none'; return ctx.json({ ...detail, @@ -359,17 +361,21 @@ chatRouter.patch('/sessions/:id/title', async (ctx) => { 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). +// PATCH /chat/sessions/:id/notify-pref — set this session's bell preference, FOR THIS CALLER. +// +// The row is keyed on (user_id, session_id), so writing one for a session you cannot reach affects only +// your own row and nothing ever fires it. That was previously claimed of a table keyed on session_id +// alone, where it was false — delivery read the preference globally, so setting one on someone else's +// conversation switched off THEIR notifications. See the schema for the full note. chatRouter.patch('/sessions/:id/notify-pref', async (ctx) => { - await chatIdentity(ctx.get('user')); + const user = ctx.get('user'); + await chatIdentity(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); + await setChatNotifyPreference(user.id, id, preference as NotifyPreference); return ctx.json({ preference }); }); diff --git a/src/servers/api/chat/notify-decision.test.ts b/src/servers/api/chat/notify-decision.test.ts new file mode 100644 index 00000000..fda043f7 --- /dev/null +++ b/src/servers/api/chat/notify-decision.test.ts @@ -0,0 +1,28 @@ +import { describe, expect, test } from 'bun:test'; +import { notifyMatches } from './notify-decision'; + +describe('who gets told about a chat', () => { + // The two kinds are disjoint at the source: `turn-ended` fires on result/error/stopped, `input-needed` + // at a plan or question gate — which parks the turn and never emits a result. Matching them one-to-one + // meant "every end of turn" was silent for exactly the event you most want to hear about. + test('every_turn covers input-needed too — it is a superset, not a sibling', () => { + expect(notifyMatches('every_turn', 'turn-ended')).toBe(true); + expect(notifyMatches('every_turn', 'input-needed')).toBe(true); + }); + + test('input_needed is only the gates', () => { + expect(notifyMatches('input_needed', 'input-needed')).toBe(true); + expect(notifyMatches('input_needed', 'turn-ended')).toBe(false); + }); + + test('none is none', () => { + expect(notifyMatches('none', 'turn-ended')).toBe(false); + expect(notifyMatches('none', 'input-needed')).toBe(false); + }); + + // No stored row is the default state of every conversation ever created, so it must be quiet. + test('an unset preference is silent, and so is an unreadable one', () => { + expect(notifyMatches(null, 'turn-ended')).toBe(false); + expect(notifyMatches(null, 'input-needed')).toBe(false); + }); +}); diff --git a/src/servers/api/chat/notify-decision.ts b/src/servers/api/chat/notify-decision.ts new file mode 100644 index 00000000..2dc97610 --- /dev/null +++ b/src/servers/api/chat/notify-decision.ts @@ -0,0 +1,35 @@ +import type { NotifyPreference } from 'officerdb/types'; +import { getChatNotifyPreference } from 'officerdb'; + +// Whether a chat's ping is worth telling someone about — the DECISION, deliberately apart from any +// delivery. +// +// It lived inside `notify-delivery.ts`, which is the browser socket. That was fine while the browser was +// the only listener, and stops being fine the moment phone push subscribes to the same signal: two +// copies of this rule drift, and the symptom is a bell and a phone disagreeing about the same +// conversation, which is close to impossible to debug from either end. +// +// So both channels ask here. The rule lives once. + +export type NotifyKind = 'turn-ended' | 'input-needed'; + +/** + * The rule, pure so it can be tested without a database. + * + * **`every_turn` is a SUPERSET of `input_needed`, not a sibling.** The two kinds are disjoint at the + * source — `turn-ended` fires on `result`/`error`/`stopped`, while `input-needed` fires at a plan gate or + * a question gate, which PARK the turn and never emit a result. So matching them one-to-one meant + * choosing "every end of turn" delivered nothing when the agent stopped to ask you something: the most + * urgent case, silently the quietest, for whoever picked the most inclusive setting. + */ +export const notifyMatches = (preference: NotifyPreference | null, kind: NotifyKind): boolean => { + if (!preference || preference === 'none') return false; + if (preference === 'every_turn') return true; + return kind === 'input-needed'; +}; + +/** The same rule against stored state. A preference that cannot be read is a no — never a notification. */ +export async function shouldNotify(userId: number, sessionKey: string, kind: NotifyKind): Promise { + const preference = await getChatNotifyPreference(userId, sessionKey).catch(() => null); + return notifyMatches(preference, kind); +} diff --git a/src/servers/api/chat/notify-delivery.ts b/src/servers/api/chat/notify-delivery.ts index ee1b362c..07d22c98 100644 --- a/src/servers/api/chat/notify-delivery.ts +++ b/src/servers/api/chat/notify-delivery.ts @@ -1,6 +1,7 @@ import type { ServerWebSocket } from 'bun'; import * as sidecar from '@@/sidecar-registry'; -import { getChatNotifyPreference, getUserById } from 'officerdb'; +import { getUserById } from 'officerdb'; +import { shouldNotify } from './notify-decision'; import { resolveHomeDir } from '@@/user-home'; import { liveSessionTitle } from './claude-sessions'; @@ -58,11 +59,9 @@ 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; + // The rule lives in ./notify-decision.ts so phone push can ask the same question rather than + // reimplementing it. Do not inline it back here. + if (!(await shouldNotify(userId, sessionKey, kind))) return; const title = await resolveTitle(userId, sessionKey); const payload = JSON.stringify({ type: 'notify', sessionId: sessionKey, kind, title });