fix(scratchlist): address HAPI Bot Major findings on PR tiann#896

Two real data-correctness paths the bot caught on the initial review.

1. Migration partial-failure data loss
   The migration loop swallowed each failed POST and still wrote the
   `migrated` flag, while the offline-cache effect mirrored the
   (partial) hub state back into `hapi.scratchlist.v1.<sessionId>` -
   so a transient error or cap rejection could leave entries neither
   on the hub nor in localStorage. Fix:
   - Track failed entries during migration and persist them back to
     localStorage; do NOT advance the flag if any entry failed, so a
     future mount retries.
   - Gate the offline-cache effect on the migration flag. Pre-
     migration, localStorage holds the v1 entries the migration
     reads; mirroring an empty hub fetch over them was the wipe.
   - Drop the "skip migration when hub is non-empty" gate. Combined
     with the duplicate-idempotent POST short-circuit (below), a
     retry against a session that another device already populated
     is a safe union.

2. Duplicate POST returned 409 at cap
   The route checked `count >= SCRATCHLIST_MAX_ENTRIES` BEFORE asking
   the store whether the supplied `entryId` already existed, so an
   idempotent migration retry against a 200-row session returned 409
   instead of 200. Fix: check duplicate first via a new
   `SyncEngine.getScratchlistEntry`, return the existing row with 200,
   and only run the cap check for genuinely new ids.

Tests added:
- hub/routes: at-cap + duplicate entryId returns 200 (not 409); at-cap
  + new entryId still 409.
- web/hook: partial-failure persists the failed entries back to
  localStorage and leaves the flag unset; offline-cache effect does
  not wipe pre-migration localStorage.

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: heavygee

hub/src/sync/syncEngine.ts

@@ -353,6 +353,27 @@ export class SyncEngine {
         return this.store.scratchlist.count(sessionId)
     }
 
+    /**
+     * Read a single entry by id. The route layer uses this to short-
+     * circuit duplicate POSTs (migration retry) BEFORE running the
+     * server-side cap check; otherwise an idempotent retry against a
+     * session that has hit `SCRATCHLIST_MAX_ENTRIES` would 409 when it
+     * should 200 with the existing row.
+     */
+    getScratchlistEntry(
+        sessionId: string,
+        entryId: string
+    ): { entryId: string; text: string; createdAt: number; updatedAt: number } | null {
+        const row = this.store.scratchlist.get(sessionId, entryId)
+        if (!row) return null
+        return {
+            entryId: row.entryId,
+            text: row.text,
+            createdAt: row.createdAt,
+            updatedAt: row.updatedAt
+        }
+    }
+
     /**
      * Insert a scratchlist entry. Returns the canonical row on success
      * (so the route layer can serialise it without a follow-up read).

hub/src/web/routes/sessions-scratchlist.test.ts

@@ -61,6 +61,7 @@ function createSession(overrides?: Partial<Session>): Session {
 type EngineOverrides = Partial<{
     listScratchlistEntries: SyncEngine['listScratchlistEntries']
     countScratchlistEntries: SyncEngine['countScratchlistEntries']
+    getScratchlistEntry: SyncEngine['getScratchlistEntry']
     createScratchlistEntry: SyncEngine['createScratchlistEntry']
     updateScratchlistEntry: SyncEngine['updateScratchlistEntry']
     deleteScratchlistEntry: SyncEngine['deleteScratchlistEntry']
@@ -81,6 +82,7 @@ function createApp(session: Session, overrides: EngineOverrides = {}) {
         },
         listScratchlistEntries: overrides.listScratchlistEntries ?? (() => []),
         countScratchlistEntries: overrides.countScratchlistEntries ?? (() => 0),
+        getScratchlistEntry: overrides.getScratchlistEntry ?? (() => null),
         createScratchlistEntry: overrides.createScratchlistEntry
             ?? ((sessionId: string, text: string) => ({
                 outcome: 'created' as const,
@@ -225,6 +227,65 @@ describe('POST /api/sessions/:id/scratchlist', () => {
         expect(body.code).toBe('scratchlist_at_cap')
     })
 
+    it('still returns 200 for a duplicate entryId even when the session is at the cap (HAPI Bot, PR #896)', async () => {
+        // The cap check used to fire BEFORE the duplicate check, which
+        // turned an idempotent migration retry into a hard 409 the
+        // moment a session reached `SCRATCHLIST_MAX_ENTRIES`. The fix
+        // short-circuits on getScratchlistEntry first; this test pins
+        // that ordering.
+        const session = createSession()
+        const createCalls: number[] = []
+        const app = createApp(session, {
+            countScratchlistEntries: () => 200,
+            getScratchlistEntry: (_sessionId, entryId) => {
+                if (entryId === 'pre-existing') {
+                    return {
+                        entryId: 'pre-existing',
+                        text: 'already there',
+                        createdAt: 100,
+                        updatedAt: 100
+                    }
+                }
+                return null
+            },
+            createScratchlistEntry: () => {
+                createCalls.push(1)
+                return {
+                    outcome: 'created' as const,
+                    entry: { entryId: 'should-not-fire', text: 'noop', createdAt: 0, updatedAt: 0 }
+                }
+            }
+        })
+        const res = await app.request('/api/sessions/session-1/scratchlist', {
+            method: 'POST',
+            headers: { 'content-type': 'application/json' },
+            body: JSON.stringify({ text: 'replay', entryId: 'pre-existing' })
+        })
+        expect(res.status).toBe(200)
+        const body = await res.json() as { entry: { text: string; entryId: string } }
+        expect(body.entry.entryId).toBe('pre-existing')
+        expect(body.entry.text).toBe('already there')
+        // The route must NOT have called createScratchlistEntry: the
+        // duplicate short-circuit returns BEFORE reaching the engine.
+        expect(createCalls).toHaveLength(0)
+    })
+
+    it('still returns 409 for a NEW entryId at the cap', async () => {
+        // Mirror of the test above for the not-duplicate case: a fresh
+        // POST at cap stays a 409.
+        const session = createSession()
+        const app = createApp(session, {
+            countScratchlistEntries: () => 200,
+            getScratchlistEntry: () => null
+        })
+        const res = await app.request('/api/sessions/session-1/scratchlist', {
+            method: 'POST',
+            headers: { 'content-type': 'application/json' },
+            body: JSON.stringify({ text: 'fresh', entryId: 'never-seen' })
+        })
+        expect(res.status).toBe(409)
+    })
+
     it('returns 404 when the engine reports session-not-found post-auth', async () => {
         // This path covers a race: auth said the session was visible
         // (resolveSessionAccess.ok), but by the time we INSERT the row the

hub/src/web/routes/sessions.ts

@@ -652,6 +652,23 @@ export function createSessionsRoutes(getSyncEngine: () => SyncEngine | null): Ho
             return c.json({ error: 'Invalid body', issues: parsed.error.issues }, 400)
         }
 
+        // Idempotent-retry short-circuit (HAPI Bot, PR #896 review):
+        // when the caller supplies an explicit entryId AND that id
+        // already exists, return the canonical row with 200 BEFORE the
+        // cap check fires. Otherwise a session sitting at the
+        // 200-entry cap would 409 a duplicate POST that should be a
+        // no-op - which is exactly the path the localStorage migration
+        // retry uses after a partial failure.
+        if (parsed.data.entryId) {
+            const existing = engine.getScratchlistEntry(
+                sessionResult.sessionId,
+                parsed.data.entryId
+            )
+            if (existing) {
+                return c.json({ entry: existing }, 200)
+            }
+        }
+
         // Server-side cap enforcement. Mirrors the web-side cap so a
         // malicious / runaway client can't drive the table without
         // bound. Bypassing the optimistic add path on the web client

web/src/lib/use-hub-scratchlist.test.tsx

@@ -330,6 +330,86 @@ describe('useHubScratchlist - localStorage migration', () => {
         expect(create).not.toHaveBeenCalled()
         expect(result.current.migrationStatus).toBe('idle')
     })
+
+    it('persists FAILED entries back to localStorage and leaves the flag unset (HAPI Bot, PR #896)', async () => {
+        // Migration partial failure: 2 entries in localStorage, the
+        // first POST succeeds and the second throws. Per the bot
+        // review, the failed entry must be written back to
+        // localStorage and the migration flag must NOT advance, so a
+        // future mount can retry. The status drops back to 'idle'
+        // (banner does not render).
+        const sid = makeSid()
+        seedV1Entries(sid)
+        let postCall = 0
+        const create = vi.fn(async (_s: string, body: { text: string; entryId?: string; createdAt?: number }) => {
+            postCall += 1
+            if (postCall === 1) {
+                return {
+                    entry: {
+                        entryId: body.entryId ?? 'a',
+                        text: body.text,
+                        createdAt: body.createdAt ?? 0,
+                        updatedAt: 0
+                    }
+                }
+            }
+            throw new Error('HTTP 500: hub flaked on entry 2')
+        })
+        const api = createMockApi({
+            getScratchlist: async () => ({ entries: [] }),
+            createScratchlistEntry: create
+        })
+        const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() })
+        await waitFor(() => expect(create).toHaveBeenCalledTimes(2))
+        await waitFor(() => expect(result.current.migrationStatus).toBe('idle'))
+
+        // Flag must NOT be set: a future mount must retry.
+        expect(localStorage.getItem(`hapi.scratchlist.v2.migrated.${sid}`)).toBeNull()
+        // The failed entry (the second one) must be back in localStorage.
+        const persisted = localStorage.getItem(`hapi.scratchlist.v1.${sid}`)
+        expect(persisted).not.toBeNull()
+        const parsed = JSON.parse(persisted!) as Array<{ id: string; text: string }>
+        expect(parsed.map((e) => e.id)).toEqual(['old-2'])
+        expect(parsed[0]?.text).toBe('another')
+    })
+
+    it('does NOT mirror an empty hub fetch into localStorage before migration runs (HAPI Bot, PR #896)', async () => {
+        // Pre-fix the offline-cache effect would clobber the v1
+        // entries with `[]` the moment the initial fetch returned an
+        // empty list, racing the migration effect's localStorage
+        // read on a future mount. The fix gates the cache mirror on
+        // the migration flag; this test pins it.
+        const sid = makeSid()
+        seedV1Entries(sid)
+        const apiCalls: number[] = []
+        const api = createMockApi({
+            // Block on first fetch so we can inspect localStorage
+            // BEFORE the migration effect kicks off.
+            getScratchlist: async () => {
+                apiCalls.push(Date.now())
+                if (apiCalls.length === 1) {
+                    await new Promise((r) => setTimeout(r, 25))
+                    return { entries: [] }
+                }
+                return {
+                    entries: [
+                        { entryId: 'old-1', text: 'pre-v2 note', createdAt: 100, updatedAt: 100 },
+                        { entryId: 'old-2', text: 'another', createdAt: 200, updatedAt: 200 }
+                    ]
+                }
+            }
+        })
+        const { result } = renderHook(() => useHubScratchlist(sid, api), { wrapper: createWrapper() })
+        // Wait for migration to complete (flag set + status flips).
+        await waitFor(() => expect(result.current.migrationStatus).toBe('completed'), { timeout: 2000 })
+        // localStorage now mirrors hub state (post-migration). It must
+        // contain the v1 entries that round-tripped through the hub
+        // fetch, NOT an empty array.
+        const persisted = localStorage.getItem(`hapi.scratchlist.v1.${sid}`)
+        expect(persisted).not.toBeNull()
+        const parsed = JSON.parse(persisted!) as Array<{ id: string }>
+        expect(parsed.map((e) => e.id).sort()).toEqual(['old-1', 'old-2'])
+    })
 })
 
 describe('useHubScratchlist - reorder (local-only)', () => {

web/src/lib/use-hub-scratchlist.ts

@@ -4,6 +4,7 @@ import type { ApiClient } from '@/api/client'
 import { queryKeys } from '@/lib/query-keys'
 import {
     moveScratchlistEntry,
+    persistScratchlist,
     readScratchlist,
     SCRATCHLIST_MAX_ENTRIES,
     SCRATCHLIST_MAX_TEXT_LENGTH,
@@ -196,18 +197,21 @@ export function useHubScratchlist(
 
     // Migration trigger: runs ONCE per session when:
     //   - api is available
-    //   - hub returned an empty list
     //   - migration flag is unset
     //   - localStorage holds v1 entries
-    // The actual POSTs are sequential to keep retry semantics simple
-    // and to avoid bursts that could trip rate-limit guards. For the
-    // typical case of "a handful of stale entries" this is fine.
+    // Hub being non-empty does NOT block the migration: each POST uses
+    // the entry's original id and the route returns 200 for an
+    // already-existing id (idempotent). So a session that another
+    // device already populated is safely treated as a union with this
+    // device's local entries. The actual POSTs are sequential to keep
+    // retry semantics simple and to avoid bursts that could trip
+    // rate-limit guards. For the typical case of "a handful of stale
+    // entries" this is fine.
     useEffect(() => {
         if (!api || !sessionId) return
         if (migrationAttemptedRef.current) return
         if (query.isLoading || query.isFetching) return
         if (!query.data) return
-        if (query.data.entries.length > 0) return
         if (readMigrationFlag(sessionId)) return
 
         const localEntries = readScratchlist(sessionId)
@@ -222,6 +226,14 @@ export function useHubScratchlist(
         setMigrationStatus('migrating')
 
         void (async () => {
+            // HAPI Bot review on PR #896 caught a data-loss path here:
+            // swallowing per-entry POST failures and still writing the
+            // migration flag would strand entries (the offline-cache
+            // mirror would replace the original localStorage with the
+            // partial hub state). Track failed entries and persist them
+            // back so a future mount retries; do NOT set the flag until
+            // every local entry is reconciled.
+            const failedEntries: ScratchlistEntry[] = []
             try {
                 // Preserve creation order by POSTing in the order
                 // localStorage holds them. The hub orders by createdAt
@@ -240,21 +252,34 @@ export function useHubScratchlist(
                             createdAt: entry.createdAt
                         })
                     } catch {
-                        // Per-entry failure is non-fatal: the hub returns
-                        // 200 with the canonical row for duplicates, and
-                        // any genuine rejection (e.g. cap reached) just
-                        // drops the migrated entry. Logging to console
-                        // would be noise; the user can re-add manually.
+                        // Genuine rejection (cap, network, 5xx...). The
+                        // hub-side route returns 200 for duplicate
+                        // entryId so an idempotent retry doesn't land
+                        // here; only "really did not stick" failures do.
+                        failedEntries.push(entry)
                     }
                 }
+                if (failedEntries.length > 0) {
+                    // Write the unsynced subset back to localStorage so
+                    // a future mount can retry them; leave the flag
+                    // unset so the migration effect re-fires next time.
+                    persistScratchlist(sessionId, failedEntries)
+                    migrationAttemptedRef.current = false
+                    setMigrationStatus('idle')
+                    return
+                }
                 writeMigrationFlag(sessionId)
                 await queryClient.invalidateQueries({ queryKey })
                 setMigrationStatus('completed')
             } catch {
-                // Whole-flow failure (network out, etc): leave the flag
-                // unset so a future mount retries; clear the banner
-                // status so we don't show "completed" for a half-done
-                // migration.
+                // Whole-flow failure (network out, etc): persist the
+                // entries that hadn't been attempted yet plus any that
+                // failed up to the throw, leave the flag unset, and
+                // clear the banner status so we don't show "completed"
+                // for a half-done migration.
+                if (failedEntries.length > 0) {
+                    persistScratchlist(sessionId, failedEntries)
+                }
                 migrationAttemptedRef.current = false
                 setMigrationStatus('idle')
             }
@@ -431,8 +456,17 @@ export function useHubScratchlist(
     // surface (e.g. the standalone `ScratchlistPanel` used by tests)
     // working when offline, and protects against losing freshly-added
     // entries if the hub goes away mid-session.
+    //
+    // CRITICAL: gate on the migration flag. Pre-migration, localStorage
+    // holds the v1 entries that the migration effect needs to read; if
+    // we mirrored an empty hub fetch into localStorage on first render
+    // we'd wipe the very entries we're about to upload (HAPI Bot
+    // review on PR #896 caught a closely-related data-loss path). The
+    // flag also stays unset on partial-failure migrations, which keeps
+    // the failed-entry localStorage write from being clobbered.
     useEffect(() => {
         if (!sessionId) return
+        if (!readMigrationFlag(sessionId)) return
         const data = query.data
         if (!data) return
         try {
@@ -448,7 +482,7 @@ export function useHubScratchlist(
         } catch {
             // Non-fatal: storage quota / private mode.
         }
-    }, [sessionId, query.data])
+    }, [sessionId, query.data, migrationStatus])
 
     const entries: ScratchlistEntry[] = (query.data?.entries ?? []).map(toLocalEntry)