fix: address notification review findings

hub/src/push/notificationCopy.test.ts

@@ -64,6 +64,18 @@ describe('loadNotificationCopy', () => {
             await rm(dir, { recursive: true, force: true })
         }
     })
+
+    it('returns empty config when persisted copy is malformed', async () => {
+        const dir = await mkdtemp(join(tmpdir(), 'hapi-copy-test-'))
+        try {
+            await writeFile(join(dir, 'settings.json'), JSON.stringify({
+                notificationCopy: { ready: { title: 42, body: 'hello' } }
+            }))
+            expect(await loadNotificationCopy(dir)).toEqual({})
+        } finally {
+            await rm(dir, { recursive: true, force: true })
+        }
+    })
 })
 
 describe('resolveCopy', () => {
@@ -71,12 +83,18 @@ describe('resolveCopy', () => {
         expect(resolveCopy('ready', {})).toEqual(DEFAULT_COPY.ready)
     })
 
-    it('returns the default when title is empty', () => {
-        expect(resolveCopy('ready', { ready: { title: '  ', body: 'custom' } })).toEqual(DEFAULT_COPY.ready)
+    it('falls back only the title when title is empty', () => {
+        expect(resolveCopy('ready', { ready: { title: '  ', body: 'custom' } })).toEqual({
+            title: DEFAULT_COPY.ready.title,
+            body: 'custom'
+        })
     })
 
-    it('returns the default when body is empty', () => {
-        expect(resolveCopy('ready', { ready: { title: 'custom', body: '' } })).toEqual(DEFAULT_COPY.ready)
+    it('falls back only the body when body is empty', () => {
+        expect(resolveCopy('ready', { ready: { title: 'custom', body: '' } })).toEqual({
+            title: 'custom',
+            body: DEFAULT_COPY.ready.body
+        })
     })
 
     it('returns the stored template when both fields are non-empty', () => {

hub/src/push/notificationCopy.ts

@@ -1,4 +1,5 @@
 import type { SessionEndReason } from '@hapi/protocol'
+import { z } from 'zod'
 import { getSettingsFile, readSettings } from '../config/settings'
 import type { TaskNotification } from '../notifications/notificationTypes'
 import { getAgentName, getSessionName } from '../notifications/sessionInfo'
@@ -6,12 +7,21 @@ import type { Session } from '../sync/syncEngine'
 
 export type CopyKey = 'permissionRequest' | 'ready' | 'taskCompleted' | 'taskFailed' | 'sessionCompletion'
 
-export type CopyTemplate = {
-    title: string
-    body: string
-}
+const copyTemplateSchema = z.object({
+    title: z.string().max(500),
+    body: z.string().max(500)
+})
 
-export type NotificationCopyConfig = Partial<Record<CopyKey, CopyTemplate>>
+export const notificationCopySchema = z.object({
+    permissionRequest: copyTemplateSchema.optional(),
+    ready: copyTemplateSchema.optional(),
+    taskCompleted: copyTemplateSchema.optional(),
+    taskFailed: copyTemplateSchema.optional(),
+    sessionCompletion: copyTemplateSchema.optional()
+})
+
+export type CopyTemplate = z.infer<typeof copyTemplateSchema>
+export type NotificationCopyConfig = z.infer<typeof notificationCopySchema>
 
 export const COPY_KEYS: readonly CopyKey[] = [
     'permissionRequest',
@@ -36,22 +46,24 @@ export const DEFAULT_COPY: Record<CopyKey, CopyTemplate> = {
 export async function loadNotificationCopy(dataDir: string): Promise<NotificationCopyConfig> {
     try {
         const settings = await readSettings(getSettingsFile(dataDir))
-        return settings?.notificationCopy ?? {}
+        const parsed = notificationCopySchema.safeParse(settings?.notificationCopy ?? {})
+        return parsed.success ? parsed.data : {}
     } catch {
         return {}
     }
 }
 
 /**
- * A stored template only takes effect when BOTH title and body are non-empty;
- * an empty field falls back to the default template.
+ * Empty fields fall back independently so a title-only or body-only override
+ * still takes effect.
  */
 export function resolveCopy(key: CopyKey, stored: NotificationCopyConfig): CopyTemplate {
     const template = stored[key]
-    if (template && template.title.trim() && template.body.trim()) {
-        return { title: template.title, body: template.body }
+    const defaults = DEFAULT_COPY[key]
+    return {
+        title: template?.title.trim() ? template.title : defaults.title,
+        body: template?.body.trim() ? template.body : defaults.body
     }
-    return DEFAULT_COPY[key]
 }
 
 /**

hub/src/push/pushService.test.ts

@@ -0,0 +1,69 @@
+import { beforeEach, describe, expect, it, mock } from 'bun:test'
+import { PushService, type PushPayload } from './pushService'
+
+const sendNotification = mock(async (_subscription: unknown, _body: string) => ({}))
+
+mock.module('web-push', () => ({
+    setVapidDetails: mock(() => {}),
+    sendNotification
+}))
+
+type Subscription = {
+    endpoint: string
+    p256dh: string
+    auth: string
+}
+
+function createService(subscriptions: Subscription[]) {
+    const removePushSubscription = mock(() => {})
+    const store = {
+        push: {
+            getPushSubscriptionsByNamespace: mock(() => subscriptions),
+            removePushSubscription
+        }
+    }
+    return {
+        service: new PushService(
+            { publicKey: 'test-public', privateKey: 'test-private' },
+            'mailto:test@example.com',
+            store as never
+        ),
+        removePushSubscription
+    }
+}
+
+const payload: PushPayload = {
+    title: 'Test',
+    body: 'Test body'
+}
+
+beforeEach(() => {
+    sendNotification.mockClear()
+    sendNotification.mockImplementation(async () => ({}))
+})
+
+describe('PushService.sendToNamespace', () => {
+    it('returns zero when the namespace has no subscriptions', async () => {
+        const { service } = createService([])
+
+        expect(await service.sendToNamespace('default', payload)).toBe(0)
+        expect(sendNotification).not.toHaveBeenCalled()
+    })
+
+    it('counts only successful deliveries', async () => {
+        const subscriptions = [
+            { endpoint: 'https://push.example/ok', p256dh: 'key-1', auth: 'auth-1' },
+            { endpoint: 'https://push.example/gone', p256dh: 'key-2', auth: 'auth-2' }
+        ]
+        sendNotification.mockImplementation(async (subscription: unknown) => {
+            if ((subscription as { endpoint: string }).endpoint.endsWith('/gone')) {
+                throw { statusCode: 410 }
+            }
+            return {}
+        })
+        const { service, removePushSubscription } = createService(subscriptions)
+
+        expect(await service.sendToNamespace('default', payload)).toBe(1)
+        expect(removePushSubscription).toHaveBeenCalledWith('default', 'https://push.example/gone')
+    })
+})

hub/src/push/pushService.ts

@@ -38,23 +38,24 @@ export class PushService {
         webPush.setVapidDetails(this.subject, this.vapidKeys.publicKey, this.vapidKeys.privateKey)
     }
 
-    async sendToNamespace(namespace: string, payload: PushPayload): Promise<void> {
+    async sendToNamespace(namespace: string, payload: PushPayload): Promise<number> {
         const subscriptions = this.store.push.getPushSubscriptionsByNamespace(namespace)
         if (subscriptions.length === 0) {
-            return
+            return 0
         }
 
         const body = JSON.stringify(payload)
-        await Promise.all(subscriptions.map((subscription) => {
+        const delivered = await Promise.all(subscriptions.map((subscription) => {
             return this.sendToSubscription(namespace, subscription, body)
         }))
+        return delivered.filter(Boolean).length
     }
 
     private async sendToSubscription(
         namespace: string,
         subscription: StoredSubscription,
         body: string
-    ): Promise<void> {
+    ): Promise<boolean> {
         const pushSubscription: PushSubscription = {
             endpoint: subscription.endpoint,
             keys: {
@@ -65,6 +66,7 @@ export class PushService {
 
         try {
             await webPush.sendNotification(pushSubscription, body)
+            return true
         } catch (error) {
             const statusCode = typeof (error as { statusCode?: unknown }).statusCode === 'number'
                 ? (error as { statusCode: number }).statusCode
@@ -72,10 +74,11 @@ export class PushService {
 
             if (statusCode === 410) {
                 this.store.push.removePushSubscription(namespace, subscription.endpoint)
-                return
+                return false
             }
 
             console.error('[PushService] Failed to send notification:', error)
+            return false
         }
     }
 }

hub/src/web/routes/notificationCopy.test.ts

@@ -57,6 +57,16 @@ describe('GET /api/notification-copy', () => {
         const body = await res.json() as { copy: Record<string, unknown> }
         expect(body.copy).toEqual({})
     })
+
+    it('returns empty copy when persisted notification copy is malformed', async () => {
+        await writeFile(join(dir, 'settings.json'), JSON.stringify({
+            notificationCopy: { ready: { title: 42, body: 'hello' } }
+        }))
+        const app = await createApp('default')
+        const res = await app.request('/api/notification-copy')
+        const body = await res.json() as { copy: Record<string, unknown> }
+        expect(body.copy).toEqual({})
+    })
 })
 
 describe('PUT /api/notification-copy', () => {

hub/src/web/routes/notificationCopy.ts

@@ -1,22 +1,14 @@
 import { Hono } from 'hono'
-import { z } from 'zod'
 import { getSettingsFile, readSettingsOrThrow, writeSettings } from '../../config/settings'
-import { COPY_KEYS, DEFAULT_COPY, type CopyKey, type NotificationCopyConfig } from '../../push/notificationCopy'
+import {
+    COPY_KEYS,
+    DEFAULT_COPY,
+    notificationCopySchema,
+    type CopyKey,
+    type NotificationCopyConfig
+} from '../../push/notificationCopy'
 import type { WebAppEnv } from '../middleware/auth'
 
-const copyTemplateSchema = z.object({
-    title: z.string().max(500),
-    body: z.string().max(500)
-})
-
-const updateSchema = z.object({
-    permissionRequest: copyTemplateSchema.optional(),
-    ready: copyTemplateSchema.optional(),
-    taskCompleted: copyTemplateSchema.optional(),
-    taskFailed: copyTemplateSchema.optional(),
-    sessionCompletion: copyTemplateSchema.optional()
-})
-
 function isAdmin(namespace: string): boolean {
     return namespace === 'default'
 }
@@ -31,8 +23,9 @@ export function createNotificationCopyRoutes(dataDir: string): Hono<WebAppEnv> {
             return c.json({ error: 'Forbidden: admin only' }, 403)
         }
         const settings = await readSettingsOrThrow(settingsFile)
+        const parsed = notificationCopySchema.safeParse(settings.notificationCopy ?? {})
         return c.json({
-            copy: settings.notificationCopy ?? {},
+            copy: parsed.success ? parsed.data : {},
             defaults: DEFAULT_COPY
         })
     })
@@ -43,7 +36,7 @@ export function createNotificationCopyRoutes(dataDir: string): Hono<WebAppEnv> {
             return c.json({ error: 'Forbidden: admin only' }, 403)
         }
         const json = await c.req.json().catch(() => null)
-        const parsed = updateSchema.safeParse(json)
+        const parsed = notificationCopySchema.safeParse(json)
         if (!parsed.success) {
             return c.json({ error: 'Invalid body', issues: parsed.error.flatten() }, 400)
         }

hub/src/web/routes/push.test.ts

@@ -0,0 +1,40 @@
+import { describe, expect, it } from 'bun:test'
+import { Hono } from 'hono'
+import type { PushPayload } from '../../push/pushService'
+import type { WebAppEnv } from '../middleware/auth'
+import { createPushRoutes } from './push'
+
+function createApp(sendToNamespace: (namespace: string, payload: PushPayload) => Promise<number>): Hono<WebAppEnv> {
+    const app = new Hono<WebAppEnv>()
+    app.use('*', async (c, next) => {
+        c.set('namespace', 'default')
+        await next()
+    })
+    app.route('/api', createPushRoutes({} as never, 'test-key', { sendToNamespace } as never))
+    return app
+}
+
+describe('POST /api/push/test', () => {
+    it('returns success when at least one subscription receives the push', async () => {
+        const sent: Array<{ namespace: string; payload: PushPayload }> = []
+        const app = createApp(async (namespace, payload) => {
+            sent.push({ namespace, payload })
+            return 1
+        })
+
+        const response = await app.request('/api/push/test', { method: 'POST' })
+
+        expect(response.status).toBe(200)
+        expect(sent[0]?.namespace).toBe('default')
+        expect(sent[0]?.payload.tag).toBe('test-push')
+    })
+
+    it('returns 503 when no subscription receives the push', async () => {
+        const app = createApp(async () => 0)
+
+        const response = await app.request('/api/push/test', { method: 'POST' })
+
+        expect(response.status).toBe(503)
+        expect(await response.json()).toEqual({ error: 'No push notification was delivered' })
+    })
+})

hub/src/web/routes/push.ts

@@ -61,7 +61,7 @@ export function createPushRoutes(
     app.post('/push/test', async (c) => {
         const namespace = c.get('namespace')
         try {
-            await pushService.sendToNamespace(namespace, {
+            const delivered = await pushService.sendToNamespace(namespace, {
                 title: 'HAPI Test Notification',
                 body: 'Your push notifications are working.',
                 tag: 'test-push',
@@ -71,6 +71,9 @@ export function createPushRoutes(
                     url: '/settings/notifications'
                 }
             })
+            if (delivered === 0) {
+                return c.json({ error: 'No push notification was delivered' }, 503)
+            }
             return c.json({ ok: true })
         } catch (error) {
             console.error('[PushRoutes] Test push failed:', error)

web/src/routes/settings/notifications.test.tsx

@@ -152,4 +152,12 @@ describe('SettingsNotificationsPage', () => {
         renderPage()
         expect(await screen.findByText(/Claude is waiting in My Project/)).toBeTruthy()
     })
+
+    it('previews a title-only override with the default body', async () => {
+        renderPage()
+        const titleInputs = await screen.findAllByLabelText('Title')
+        fireEvent.change(titleInputs[1], { target: { value: 'Custom {agentName}' } })
+
+        expect(await screen.findByText(/Custom Claude.*Claude is waiting in My Project/)).toBeTruthy()
+    })
 })

web/src/routes/settings/notifications.tsx

@@ -177,12 +177,9 @@ export default function SettingsNotificationsPage() {
         const d = draft[block]
         const defaults = copyQuery.data?.defaults
         const def = defaults ? defaults[block] as CopyTemplate | undefined : undefined
-        if (d && d.title.trim() && d.body.trim()) {
-            return { title: renderTemplate(d.title, PREVIEW_VARS), body: renderTemplate(d.body, PREVIEW_VARS) }
-        }
         return {
-            title: renderTemplate(def?.title ?? '', PREVIEW_VARS),
-            body: renderTemplate(def?.body ?? '', PREVIEW_VARS)
+            title: renderTemplate(d?.title.trim() ? d.title : (def?.title ?? ''), PREVIEW_VARS),
+            body: renderTemplate(d?.body.trim() ? d.body : (def?.body ?? ''), PREVIEW_VARS)
         }
     }