fix(persistence): address PR #117 review feedback

Adopt 8 PR-agent (Qodo) findings; 4 declined (concurrency already guarded by
the putSyncedRecord/markRecordSynced guards + RPC optimistic concurrency;
SQL-injection / won-equality / microtask-race are false positives — see PR reply).

- markRecordSynced: guard on updatedAt too — softDeleteStory doesn't bump rev,
  so a same-rev newer local tombstone must not be marked synced by an older
  push's ack (symmetric with putSyncedRecord's guard)
- recordToEnvelope: fallback timestamps to 0 not Date.now() (a corrupt record
  should lose LWW, not win as "now")
- push/delete routes: validate rev/updatedAt as finite -> 400 (was silent 200);
  push: Content-Length pre-check before buffering the body
- pushDeletion: idbGet a single record instead of a full-store scan
- manifest: Cache-Control private,no-store + client fetch cache:no-store
- cloudSyncClient: Array.isArray narrowing on items/blobs
- RPC: `if found` instead of `v_row.id is not null` after RETURNING INTO

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

app/api/stories/delete/route.ts

@@ -1,7 +1,6 @@
 import { NextResponse } from "next/server";
 import { requireUser } from "@/lib/supabase/guard";
 import { cloudSoftDeleteStory } from "@/lib/persistence/cloudStore";
-import { coerceEpoch } from "@/lib/persistence/types";
 
 export const runtime = "nodejs";
 
@@ -24,9 +23,16 @@ export async function POST(req: Request) {
   if (!id) {
     return NextResponse.json({ error: "missing id" }, { status: 400 });
   }
-  const rev = typeof body.rev === "number" ? body.rev : 1;
+  // Validate rev/deletedAt as finite values (see push route rationale): reject
-  const deletedAt = coerceEpoch(body.deletedAt, Date.now());
+  // bad input with 400 rather than letting NaN/Infinity reach the PostgREST
+  // filter or toISOString().
+  if (typeof body.rev !== "number" || !Number.isFinite(body.rev) || body.rev <= 0) {
+    return NextResponse.json({ error: "invalid rev" }, { status: 400 });
+  }
+  if (typeof body.deletedAt !== "number" || !Number.isFinite(body.deletedAt)) {
+    return NextResponse.json({ error: "invalid deletedAt" }, { status: 400 });
+  }
 
-  const ok = await cloudSoftDeleteStory(id, rev, deletedAt);
+  const ok = await cloudSoftDeleteStory(id, body.rev, body.deletedAt);
   return NextResponse.json({ ok });
 }

app/api/stories/manifest/route.ts

@@ -15,5 +15,8 @@ export async function GET() {
   if (auth instanceof NextResponse) return auth;
 
   const items = await cloudStoryManifest();
-  return NextResponse.json({ items });
+  return NextResponse.json(
+    { items },
+    { headers: { "Cache-Control": "private, no-store" } },
+  );
 }

app/api/stories/push/route.ts

@@ -19,6 +19,13 @@ export async function POST(req: Request) {
   const auth = await requireUser();
   if (auth instanceof NextResponse) return auth;
 
+  // Pre-check Content-Length to reject an oversized body before buffering it.
+  // The post-read byteLength check below still covers chunked/omitted headers.
+  const contentLength = req.headers.get("content-length");
+  if (contentLength && Number(contentLength) > MAX_PUSH_BYTES) {
+    return NextResponse.json({ error: "payload too large" }, { status: 413 });
+  }
+
   let raw: string;
   try {
     raw = await req.text();
@@ -38,14 +45,30 @@ export async function POST(req: Request) {
   if (!env?.id || typeof env.id !== "string") {
     return NextResponse.json({ error: "missing id" }, { status: 400 });
   }
+  // Validate the LWW-ordering fields as finite values: a non-finite rev /
+  // updatedAt would otherwise reach the RPC, throw at toISOString(), and surface
+  // as a silent { stored:null, won:false } 200 — return 400 so the caller can
+  // diagnose a bad request rather than mistake it for a normal lost conflict.
+  if (typeof env.rev !== "number" || !Number.isFinite(env.rev) || env.rev <= 0) {
+    return NextResponse.json({ error: "invalid rev" }, { status: 400 });
+  }
+  if (typeof env.updatedAt !== "number" || !Number.isFinite(env.updatedAt)) {
+    return NextResponse.json({ error: "invalid updatedAt" }, { status: 400 });
+  }
+  if (
+    env.deletedAt != null &&
+    (typeof env.deletedAt !== "number" || !Number.isFinite(env.deletedAt))
+  ) {
+    return NextResponse.json({ error: "invalid deletedAt" }, { status: 400 });
+  }
 
   // Defensive coercion at the trust boundary (the slim session itself is left to
   // the client — it's reconstructible and never security-sensitive after slim).
   const result = await cloudSaveStory({
     ...env,
     orientation: coerceOrientation(env.orientation),
-    updatedAt: coerceEpoch(env.updatedAt, Date.now()),
+    updatedAt: coerceEpoch(env.updatedAt, 0),
-    deletedAt: env.deletedAt == null ? null : coerceEpoch(env.deletedAt, Date.now()),
+    deletedAt: env.deletedAt == null ? null : coerceEpoch(env.deletedAt, 0),
   });
   return NextResponse.json(result);
 }

lib/persistence/cloudSync.ts

@@ -25,6 +25,7 @@ import {
   putSyncedRecord,
   markRecordSynced,
 } from "./localStore";
+import { idbGet, STORIES_STORE } from "./idb";
 import { coerceEpoch, type StoryRecord, type StorySyncMeta, type StorySyncEnvelope } from "./types";
 
 // Keep in lockstep with the pull route's MAX_PULL_IDS.
@@ -88,8 +89,8 @@ function recordToEnvelope(rec: StoryRecord): StorySyncEnvelope {
     sceneCount: rec.sceneCount ?? 0,
     rev: rec.rev ?? 1,
     session: rec.session,
-    updatedAt: coerceEpoch(rec.updatedAt, Date.now()),
+    updatedAt: coerceEpoch(rec.updatedAt, 0),
-    deletedAt: rec.deletedAt == null ? null : coerceEpoch(rec.deletedAt, Date.now()),
+    deletedAt: rec.deletedAt == null ? null : coerceEpoch(rec.deletedAt, 0),
   };
 }
 
@@ -105,7 +106,7 @@ async function pushOne(rec: StoryRecord): Promise<void> {
   const res = await pushBlob(recordToEnvelope(rec));
   if (!res) return; // network/auth failure → leave pending for next reconcile
   if (res.won) {
-    await markRecordSynced(rec.id, rec.rev ?? 1);
+    await markRecordSynced(rec.id, rec.rev ?? 1, coerceEpoch(rec.updatedAt, 0));
   } else if (res.stored) {
     await putSyncedRecord(res.stored); // we lost → adopt the newer cloud state
   }
@@ -177,7 +178,7 @@ async function reconcile(): Promise<void> {
   for (const rec of toDelete) {
     try {
       const ok = await pushDelete(rec.id, rec.rev ?? 1, coerceEpoch(rec.deletedAt, Date.now()));
-      if (ok) await markRecordSynced(rec.id, rec.rev ?? 1);
+      if (ok) await markRecordSynced(rec.id, rec.rev ?? 1, coerceEpoch(rec.updatedAt, 0));
       // !ok → cloud has a newer row; the next reconcile pulls it back.
     } catch {
       /* leave pending */
@@ -186,7 +187,7 @@ async function reconcile(): Promise<void> {
   // Mark already-consistent records synced.
   for (const rec of toMarkSynced) {
     try {
-      await markRecordSynced(rec.id, rec.rev ?? 1);
+      await markRecordSynced(rec.id, rec.rev ?? 1, coerceEpoch(rec.updatedAt, 0));
     } catch {
       /* best-effort */
     }
@@ -236,10 +237,10 @@ export async function pushDeletion(id: string): Promise<void> {
   if (!AUTH_ENABLED || !id) return;
   try {
     if (!(await isAuthed())) return;
-    const rec = (await listAllRecordsForSync()).find((r) => r.id === id);
+    const rec = await idbGet<StoryRecord>(STORIES_STORE, id);
     if (!rec || !rec.deletedAt) return; // not a tombstone / already gone
     const ok = await pushDelete(id, rec.rev ?? 1, coerceEpoch(rec.deletedAt, Date.now()));
-    if (ok) await markRecordSynced(id, rec.rev ?? 1);
+    if (ok) await markRecordSynced(id, rec.rev ?? 1, coerceEpoch(rec.updatedAt, 0));
   } catch {
     /* leave pending */
   }

lib/persistence/cloudSyncClient.ts

@@ -36,10 +36,10 @@ async function postJson<T>(url: string, body: unknown): Promise<T | null> {
 export async function pullManifest(): Promise<StorySyncMeta[]> {
   if (!AUTH_ENABLED) return [];
   try {
-    const res = await fetch("/api/stories/manifest", { method: "GET" });
+    const res = await fetch("/api/stories/manifest", { method: "GET", cache: "no-store" });
     if (!res.ok) return [];
-    const data = (await res.json()) as { items?: StorySyncMeta[] };
+    const data = (await res.json()) as { items?: unknown };
-    return data.items ?? [];
+    return Array.isArray(data.items) ? (data.items as StorySyncMeta[]) : [];
   } catch {
     return [];
   }
@@ -48,11 +48,8 @@ export async function pullManifest(): Promise<StorySyncMeta[]> {
 /** Pull full envelopes for the given ids. [] on empty ids / failure / auth off. */
 export async function pullBlobs(ids: string[]): Promise<StorySyncEnvelope[]> {
   if (!AUTH_ENABLED || ids.length === 0) return [];
-  const data = await postJson<{ blobs?: StorySyncEnvelope[] }>(
+  const data = await postJson<{ blobs?: unknown }>("/api/stories/pull", { ids });
-    "/api/stories/pull",
+  return Array.isArray(data?.blobs) ? (data.blobs as StorySyncEnvelope[]) : [];
-    { ids },
-  );
-  return data?.blobs ?? [];
 }
 
 /** Push one envelope through the optimistic-concurrency RPC. Returns the

lib/persistence/localStore.ts

@@ -251,10 +251,15 @@ export async function putSyncedRecord(
  *  the rev we pushed. A newer local edit (rev moved past what we pushed) is left
  *  pending so the next reconcile re-pushes the newer content. No-op if the
  *  record is gone or already synced (Req 8.1). */
-export async function markRecordSynced(id: string, rev: number): Promise<void> {
+export async function markRecordSynced(id: string, rev: number, updatedAt: number): Promise<void> {
   const rec = await idbGet<StoryRecord>(STORIES_STORE, id);
   if (!rec) return;
+  // Guard on BOTH rev and updatedAt. softDeleteStory bumps updatedAt WITHOUT
+  // bumping rev, so a same-rev-but-newer local tombstone produced while a push
+  // was in flight must NOT be marked synced by that older push's ack (it still
+  // owes a delete push). Symmetric with putSyncedRecord's concurrency guard.
   if ((rec.rev ?? 1) !== rev) return;
+  if (coerceEpoch(rec.updatedAt, 0) !== coerceEpoch(updatedAt, 0)) return;
   if (rec.syncState === "synced") return;
   await idbPut(STORIES_STORE, { ...rec, syncState: "synced" });
 }

supabase/migrations/20260628095015_upsert_story_if_newer.sql

@@ -68,11 +68,12 @@ begin
            and excluded.updated_at > public.stories.updated_at)
   returning * into v_row;
 
-  -- v_row is populated on a fresh insert OR a winning update. It is NULL when
+  -- FOUND is the idiomatic PL/pgSQL test for whether RETURNING produced a row:
-  -- the row already existed AND the where-guard rejected the update (stale
+  -- true on a fresh insert OR a winning update; false when the row already
-  -- write) — in that case return the current cloud row so the caller sees it
+  -- existed AND the where-guard rejected the update (stale write). In the stale
+  -- case fall through and return the current cloud row so the caller sees it
   -- lost and can reconcile by pulling the newer cloud state.
-  if v_row.id is not null then
+  if found then
     return v_row;
   end if;