forcingfx · forcingfx · Aug 18, 2026
@@ -424,6 +424,7 @@ Working code-review checklist (adapted from [Lukaszuk's clean-code summary](http
 - **Core modules must not import from UI components.** Stores (`src/stores/`) and core (`src/core/`) are the shared backend; they must never import from `src/components/`. If a store needs a UI side effect, use an event, a shared utility in `src/utils/`/`src/core/`, or let the UI react to store state changes.
 - **Pull-before-mutate invariant**: Any flow that reads remote state and then modifies local state must fetch the remote data **before** any destructive local op (`deleteDatabase`, `tryDeleteServerVault`). The recovery flow calls `pullVault()` first, then `initFresh(skipServerCleanup: true)`. Otherwise you destroy the vault you're trying to recover. Workflows with destructive + read operations on shared remote state need integration tests; mocked unit tests can't catch temporal coupling across module boundaries.
 - **No-auto-destroy invariant**: No automated code path may delete server-side vault data. `destroy()` (`src/core/storage/key-manager.ts`) has exactly one sanctioned caller — `useAppStore.getState().resetApp`, which must be invoked from an explicit user-confirmation UI (the "Wipe and start over" `<AlertDialog>` on `InvalidKeysScreen` or the equivalent Settings reset). Boot-time canary failures route to `recoveryMode: "invalid-keys"` instead of `destroy()`. Issue #117 root-caused a chain of silent vault deletion to a boot-time auto-destroy cascade; ADR 018 is the durable rule. The runtime check `assertKeyDataCoupling()` is called at the end of every key-touching flow (`initFresh`, `applyCloudVault`, `restore`) to enforce the key-data coupling invariant mechanically rather than by convention.
+- **API handlers answer JSON on every path; clients never treat an unparseable body as a network error.** A handler that throws returns no response, so the platform substitutes an HTML error page — and a client calling `res.json()` in the same `try` as its `fetch` reports that server crash as a *connection* failure, hiding the status code that identifies the real layer. Narrow untrusted payload fields to their expected type (`readTrimmedString` in `src/core/feedback/feedback-handler.ts`) instead of reaching for `.trim()` on whatever `JSON.parse` returned — `null`, arrays, and `{"message": 42}` are all valid JSON. On the client, only a rejected `fetch` may say "check your connection"; every answered request reports the server's own `error`, falling back to the status code (`submitFeedback` in `src/components/feedback/feedback-dialog.tsx`).
 - **Shared mutable headers leak Content-Length**: `@hono/node-server` mutates the `headers` record passed to `new Response(body, { headers })` by appending the computed `Content-Length`. A `const HEADERS = {...}` shared across responses lets a small response's `Content-Length` leak into a large response's headers, truncating the body at the receiver. The shared-state pattern is now a code-review smell — see `apiHeaders()` in `src/core/sync/sync-handler.ts` for the correct pattern (function returning a fresh object per call). This was the proximate cause of issue #117's `JSON.parse: unterminated string` reports.
 - **Test-only adapters are branded; resolvers refuse them in production.** Every in-memory adapter (`createMemoryAdapter`, `createMemoryCatalogAdapter`, `MemoryLicenseStorage`, `MemorySeenEventStore`) is branded via `markTestOnly()` from `src/core/test-only-brand.ts`. The four resolvers call `assertNotTestOnlyInProduction()` at the point they hand the adapter back; a misconfigured production deploy throws at module-load instead of silently routing writes to a per-cold-start `Map`. New backends MUST follow the same pattern: brand the memory implementation, guard the fallthrough in the resolver. Without this, the next "credential silently missing → adapter silently swapped" incident reads exactly like the 2026-05-12 sync regression and the 2026-05-14 stats-always-zero one.
 - **Feature gating is honor-system open-core**: Client-side tier gates live in `src/core/features/feature-gates.ts` and consume tier from `useLicenseStore` (`src/stores/license-store.ts`). React components call `useFeatureGate(feature)`; store actions call `enforceFeature(feature)` / `isFeatureEnabled(feature)` from `src/stores/enforce-feature.ts` for defense-in-depth. Self-hosters bypass via `VITE_SELF_HOSTED=1` at build time. Coming-soon features stay locked regardless. See ADR 012.

@@ -156,6 +156,24 @@ Endpoints:
 - `POST /api/checkout/create-session` — Stripe Checkout Session creation with priceId allowlist. Injects a server-controlled 30-day free trial via `subscription_data.trial_period_days` — see [ADR 015](decisions/015-stripe-side-trial.md).
 - `GET /api/health`, `GET /api/icon`, `POST /api/feedback`, `GET /api/favicon` — Operational endpoints.
 
+#### JSON on every path, including the ones nobody calls
+
+A handler must answer JSON for *every* outcome it can produce — validation
+failures, wrong methods, malformed payloads — and must never let an exception
+escape. The two rules are the same rule: a handler that throws does not return
+a response at all, so the platform substitutes its own HTML error page.
+
+Clients read the server's `error` field to tell the user what went wrong. Any
+response they cannot parse collapses into a generic message and takes the
+status code — the only clue identifying which layer broke — with it. This is
+not hypothetical: `/api/feedback` read `body.message.trim()` straight off the
+parsed payload, so a body of `null`, an array, or `{"message": 42}` (all valid
+JSON) threw a `TypeError` out of the handler, and the feedback dialog reported
+the resulting HTML 500 to the user as *"check your connection"* — sending them
+to inspect the one part of the system that was working. Narrow untrusted
+fields to their expected type before using them, and return `jsonResponse` from
+the method guard too.
+
 ### Production data layer: Upstash KV
 
 Per [ADR 008](decisions/008-upstash-as-production-data-layer.md), five distinct server-side concerns share one Upstash REST KV instance with non-overlapping key prefixes (`license:*`, `customer:*`, `vault:*`, `seen-event:*`, `catalog:*`, `ratelimit:*`). The credential cascade `UPSTASH_REDIS_REST_URL/TOKEN` → `KV_REST_API_URL/TOKEN` → memory fallback is shared by every adapter, so an operator configures Upstash once and all five subsystems pick it up.

@@ -33,6 +33,68 @@ import { toast } from "sonner";
 const MAX_LENGTH = 2000;
 const GITHUB_ISSUES_URL = "https://github.com/forcingfx/feedzero/issues";
 
+interface FeedbackPayload {
+  message: string;
+  email?: string;
+}
+
+type SubmitOutcome = { sent: true } | { sent: false; error: string };
+
+/**
+ * Parse a response body as JSON, or `null` when it isn't JSON at all.
+ *
+ * The endpoint answers JSON on every path it controls, but the layers in front
+ * of it do not: a crashed function, a gateway timeout or a misrouted request
+ * come back as an HTML error page, and `res.json()` throws on those.
+ */
+async function readJsonBody(
+  res: Response,
+): Promise<{ ok?: boolean; error?: string } | null> {
+  try {
+    return (await res.json()) as { ok?: boolean; error?: string };
+  } catch {
+    return null;
+  }
+}
+
+/**
+ * POST the note and turn every ending — sent, rejected, unreachable — into a
+ * message the user can act on.
+ *
+ * Only a rejected `fetch` is a connection problem. This used to catch the
+ * rejected `fetch` and a failed `res.json()` in one block, so a server that
+ * answered with an HTML error page (the shape of a crashed serverless
+ * function) was reported as a broken network: the user was sent to check the
+ * one part of the system that demonstrably worked, and the status code that
+ * would have identified the real fault never reached them.
+ */
+async function submitFeedback(
+  payload: FeedbackPayload,
+): Promise<SubmitOutcome> {
+  let res: Response;
+  try {
+    res = await fetch("/api/feedback", {
+      method: "POST",
+      headers: { "Content-Type": "application/json" },
+      body: JSON.stringify(payload),
+    });
+  } catch {
+    return {
+      sent: false,
+      error: "Could not send feedback. Check your connection.",
+    };
+  }
+
+  const data = await readJsonBody(res);
+  if (res.ok && data?.ok) return { sent: true };
+
+  return {
+    sent: false,
+    error:
+      data?.error ?? `Could not send feedback (server error ${res.status}).`,
+  };
+}
+
 interface FeedbackDialogProps {
   open: boolean;
   onOpenChange: (open: boolean) => void;
@@ -49,30 +111,20 @@ export function FeedbackDialog({ open, onOpenChange }: FeedbackDialogProps) {
     if (!trimmedMessage) return;
 
     const trimmedEmail = email.trim();
-    const payload: { message: string; email?: string } = {
-      message: trimmedMessage,
-    };
+    const payload: FeedbackPayload = { message: trimmedMessage };
     if (trimmedEmail) payload.email = trimmedEmail;
 
     setIsSending(true);
     try {
-      const res = await fetch("/api/feedback", {
-        method: "POST",
-        headers: { "Content-Type": "application/json" },
-        body: JSON.stringify(payload),
-      });
-      const data = await res.json();
-
-      if (data.ok) {
-        toast.success("Thanks for your feedback!");
-        setMessage("");
-        setEmail("");
-        onOpenChange(false);
-      } else {
-        toast.error(data.error || "Could not send feedback");
+      const outcome = await submitFeedback(payload);
+      if (!outcome.sent) {
+        toast.error(outcome.error);
+        return;
       }
-    } catch {
-      toast.error("Could not send feedback. Check your connection.");
+      toast.success("Thanks for your feedback!");
+      setMessage("");
+      setEmail("");
+      onOpenChange(false);
     } finally {
       setIsSending(false);
     }

@@ -11,11 +11,6 @@
  * before the user types it — see <FeedbackDialog>.
  */
 
-interface FeedbackBody {
-  message?: string;
-  email?: string;
-}
-
 const MAX_MESSAGE_LENGTH = 2000;
 const MAX_EMAIL_LENGTH = 254;
 
@@ -30,7 +25,7 @@ export async function handleFeedbackRequest(
   request: Request,
 ): Promise<Response> {
   if (request.method !== "POST") {
-    return new Response("Method not allowed", { status: 405 });
+    return jsonResponse({ ok: false, error: "Method not allowed" }, 405);
   }
 
   const token = process.env.GITHUB_FEEDBACK_TOKEN;
@@ -43,14 +38,14 @@ export async function handleFeedbackRequest(
     );
   }
 
-  let body: FeedbackBody;
+  let payload: unknown;
   try {
-    body = await request.json();
+    payload = await request.json();
   } catch {
     return jsonResponse({ ok: false, error: "Invalid JSON" }, 400);
   }
 
-  const message = body.message?.trim();
+  const message = readTrimmedString(payload, "message");
   if (!message) {
     return jsonResponse({ ok: false, error: "Message is required" }, 400);
   }
@@ -64,7 +59,7 @@ export async function handleFeedbackRequest(
   // Email is optional. Reject obviously malformed values so a stray copy-paste
   // doesn't end up in the public issue body, but keep validation permissive —
   // the maintainer is the one who'll actually try replying.
-  const email = body.email?.trim();
+  const email = readTrimmedString(payload, "email");
   if (email) {
     if (email.length > MAX_EMAIL_LENGTH || !email.includes("@")) {
       return jsonResponse(
@@ -112,6 +107,27 @@ export async function handleFeedbackRequest(
   }
 }
 
+/**
+ * Read one field of the request payload as a trimmed string, or `undefined`.
+ *
+ * Every value `JSON.parse` accepts arrives here: `null`, arrays, bare strings
+ * and numbers are all valid JSON documents, and a well-formed object can still
+ * carry a number where a string belongs. Reaching straight for `.trim()` threw
+ * a TypeError that escaped the handler, and a rejected handler promise is not
+ * a response — the serverless platform answered with its own HTML 500 page.
+ * The dialog cannot parse HTML, so it reported a *server crash* as a *network
+ * failure* and pointed the user at their connection. Narrowing to string here
+ * keeps every malformed payload on the 400-JSON path.
+ */
+function readTrimmedString(
+  payload: unknown,
+  field: string,
+): string | undefined {
+  if (typeof payload !== "object" || payload === null) return undefined;
+  const value = (payload as Record<string, unknown>)[field];
+  return typeof value === "string" ? value.trim() : undefined;
+}
+
 function jsonResponse(body: unknown, status = 200): Response {
   return new Response(JSON.stringify(body), {
     status,

@@ -209,6 +209,58 @@ describe("FeedbackDialog", () => {
     });
   });
 
+  // A crashed serverless function, a gateway timeout or a misrouted request
+  // answers with an HTML error page, not JSON. Reporting that as a connection
+  // problem sends the user to check the one thing that is working; the status
+  // code is the only clue they can pass on to support.
+  it("reports the HTTP status when the server answers with a non-JSON error page", async () => {
+    mockFetch.mockResolvedValue(
+      new Response("<!doctype html><h1>500: INTERNAL_SERVER_ERROR</h1>", {
+        status: 500,
+        headers: { "Content-Type": "text/html" },
+      }),
+    );
+
+    render(<FeedbackDialog open={true} onOpenChange={vi.fn()} />);
+
+    await userEvent.type(
+      screen.getByPlaceholderText("What's on your mind?"),
+      "hello",
+    );
+    await userEvent.click(screen.getByRole("button", { name: /send/i }));
+
+    await vi.waitFor(() => {
+      expect(mockToast.error).toHaveBeenCalledWith(
+        expect.stringContaining("500"),
+      );
+    });
+    expect(mockToast.error).not.toHaveBeenCalledWith(
+      expect.stringMatching(/connection/i),
+    );
+  });
+
+  it("keeps the dialog open with the message intact when submission fails", async () => {
+    const onOpenChange = vi.fn();
+    mockFetch.mockResolvedValue(
+      new Response("<!doctype html><h1>502</h1>", {
+        status: 502,
+        headers: { "Content-Type": "text/html" },
+      }),
+    );
+
+    render(<FeedbackDialog open={true} onOpenChange={onOpenChange} />);
+
+    const textarea = screen.getByPlaceholderText("What's on your mind?");
+    await userEvent.type(textarea, "hello");
+    await userEvent.click(screen.getByRole("button", { name: /send/i }));
+
+    await vi.waitFor(() => {
+      expect(mockToast.error).toHaveBeenCalled();
+    });
+    expect(onOpenChange).not.toHaveBeenCalledWith(false);
+    expect(textarea).toHaveValue("hello");
+  });
+
   it("falls back to a connection error toast when fetch throws", async () => {
     mockFetch.mockRejectedValue(new TypeError("network down"));
 

@@ -44,6 +44,51 @@ describe("handleFeedbackRequest", () => {
     expect(res.status).toBe(405);
   });
 
+  it("answers a non-POST with a JSON body the client can parse", async () => {
+    const res = await handleFeedbackRequest(
+      new Request(ENDPOINT, { method: "GET" }),
+    );
+    expect(res.headers.get("content-type")).toBe("application/json");
+    await expect(res.json()).resolves.toMatchObject({ ok: false });
+  });
+
+  // Every entry below is accepted by JSON.parse, so the handler reaches the
+  // field reads with a value it did not expect. Each one used to throw a
+  // TypeError that escaped the handler entirely: the serverless platform
+  // turned the rejected promise into an HTML 500 page, which the dialog could
+  // not parse, so the user was told to check their connection.
+  describe.each([
+    ["a null body", null],
+    ["an array body", []],
+    ["a string body", "just text"],
+    ["a number body", 42],
+    ["a non-string message", { message: 42 }],
+    ["an object message", { message: { nested: true } }],
+  ])("malformed payload: %s", (_label, payload) => {
+    it("answers 400 JSON instead of throwing", async () => {
+      const res = await handleFeedbackRequest(postJson(payload));
+
+      expect(res.status).toBe(400);
+      expect(res.headers.get("content-type")).toBe("application/json");
+      await expect(res.json()).resolves.toMatchObject({ ok: false });
+    });
+  });
+
+  it("treats a non-string email as absent rather than throwing", async () => {
+    const fetchMock = vi
+      .fn()
+      .mockResolvedValue(new Response(null, { status: 201 }));
+    vi.stubGlobal("fetch", fetchMock);
+
+    const res = await handleFeedbackRequest(
+      postJson({ message: "Hi", email: 7 }),
+    );
+
+    expect(res.status).toBe(200);
+    const sent = JSON.parse(fetchMock.mock.calls[0][1].body);
+    expect(sent.body).toBe("Hi");
+  });
+
   it("returns 503 when GITHUB_FEEDBACK_TOKEN is missing", async () => {
     delete process.env.GITHUB_FEEDBACK_TOKEN;
     const res = await handleFeedbackRequest(postJson({ message: "Hi" }));