bell: scope the preference to a user, and make every_turn mean every turn
review of 73cdc2cb before building the phone half on it. three changes, and
the third is the one that matters for mobile.
1. "every end of turn" was silent when the agent asked you something.
the two kinds are disjoint at the source: turn-ended fires on
result/error/stopped, input-needed at a plan or question gate — which PARK
the turn and never emit a result. matching them one-to-one meant whoever
picked the most inclusive setting got nothing for the most urgent event.
every_turn is a superset now.
2. the preference is keyed on (user_id, session_id), not session_id alone.
the route skipped an ownership check on the reasoning that a preference for
a session you cannot reach is inert. it was not: delivery targets the
userId the sidecar reports while the preference was read globally, so any
account holding `chat` that knew a session uuid could set 'none' on someone
else's conversation and switch off their notifications, or every_turn to
make their phone buzz all night. surrogate id plus a unique INDEX rather
than a composite primary key, which drizzle-kit re-creates on every push.
3. the decision moved out of the browser socket into notify-decision.ts.
it lived inside notify-delivery.ts, which was fine while the browser was
the only listener and stops being fine the moment push subscribes to the
same signal. two copies of the rule drift, and the symptom is a bell and a
phone disagreeing about one conversation — close to undebuggable from
either end. both channels ask the same function now, and the rule is pure
so it is tested without a database.
the rest of the commit reviewed well: the ws upgrade is authenticated AND
permission-gated, there is a registry entry so totality passes, the list query
is batched rather than per-row, reads are defensively caught, and
Notification.requestPermission is requested lazily on opt-in.
alpha was missing the table entirely — reads degraded via .catch but the PATCH
would have 500'd. pushed there too.
This commit is contained in:
@@ -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<string, NotifyPreference>(),
|
||||
);
|
||||
const notifyPrefs = await getChatNotifyPreferences(
|
||||
user.id,
|
||||
sessions.map((s) => s.id),
|
||||
).catch(() => new Map<string, NotifyPreference>());
|
||||
|
||||
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/<id> 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 });
|
||||
});
|
||||
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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<boolean> {
|
||||
const preference = await getChatNotifyPreference(userId, sessionKey).catch(() => null);
|
||||
return notifyMatches(preference, kind);
|
||||
}
|
||||
@@ -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 });
|
||||
|
||||
Reference in New Issue
Block a user