sync: /save answered 502 because Turso will not parse a bare HAVING (#385)

`/save` failed with 502 every time unless you passed --force. The cause is one
missing clause, and it survived because it is invisible to the test suite.

insertRevision() builds two statements. The --force path sends ifRevision: null
and inserts unconditionally. The ordinary path carries a precondition and
expressed it as a HAVING on an implicit single-group aggregate:

  INSERT INTO settings_snapshots (…) SELECT …
  FROM settings_snapshots WHERE user_id = ?
  HAVING COALESCE(MAX(revision),0) = ?

SQLite treats the whole result as one group and runs that happily. Turso's
parser rejects it outright:

  SQL string could not be parsed: near HAVING, "None": syntax error

The route threw, the platform returned 502, and the CLI reported exactly what
it saw. #380 added retries for "the app does not answer", which could never
help: the statement is deterministically unparseable, so every attempt failed
the same way.

The tests could not catch it. They run against `file:` — a different engine
from the deployment — and test/settings-sync.mjs does cover the precondition,
with a stale ifRevision refused as 409 and a current one accepted. It passed
throughout, because on SQLite the statement is valid.

Adding GROUP BY user_id makes it parse on both. It cannot change the answer:
the caller sets ifRevision to null when the account has no current revision, so
there is always at least one row for this user to group.

Verified against the real database rather than only locally — the bare form
fails to parse on Turso, the grouped form parses and inserts nothing when the
precondition does not hold, which is the conflict the caller reports as 409.

test/sql-portability.test.mjs now fails on any HAVING in src/ with no GROUP BY.
Static rather than behavioural on purpose: the behaviour is correct on the
engine the tests use, so only reading the SQL can catch this. It matches the
uppercase keyword with comments stripped, because "having" is also an English
word and four files that contain no SQL said it in prose.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: ralyodio

apps/pwa/src/routes/settings-sync.mjs

@@ -125,9 +125,22 @@ async function insertRevision({ userId, body, digest, host, version, ifRevision
        SELECT ?, ?, COALESCE(MAX(revision),0) + 1, ?, ?, ?, ?, ?, ?
        FROM settings_snapshots WHERE user_id = ?
        RETURNING revision`
+    // GROUP BY user_id is not decoration. A bare HAVING on an implicit
+    // single-group aggregate is accepted by the SQLite that backs a `file:`
+    // database and *rejected by Turso's parser*:
+    //
+    //   SQL string could not be parsed: near HAVING, "None": syntax error
+    //
+    // So this statement worked in every test and threw on every deployment,
+    // which is what `/save` returning 502 actually was. `--force` sends
+    // ifRevision: null and takes the branch above, which is why forcing was the
+    // only way to save. The rows always exist here — the caller sets ifRevision
+    // to null when the account has no current revision — so grouping by the
+    // user cannot lose the row the HAVING is meant to test.
     : `INSERT INTO settings_snapshots (id,user_id,revision,digest,host,version,size,body,created_at)
        SELECT ?, ?, COALESCE(MAX(revision),0) + 1, ?, ?, ?, ?, ?, ?
        FROM settings_snapshots WHERE user_id = ?
+       GROUP BY user_id
        HAVING COALESCE(MAX(revision),0) = ?
        RETURNING revision`;
   const args = [row.id, userId, digest, host, version, size, body, row.created_at, userId];

apps/pwa/test/sql-portability.test.mjs

@@ -0,0 +1,76 @@
+// SQL this app writes, against the parser that actually runs it.
+//
+// The app is developed against a `file:` database and deployed against Turso.
+// Those are not the same parser, and the gap is silent in exactly the wrong
+// direction: SQLite accepts things Turso rejects, so a statement can pass every
+// test here and throw on every request in production.
+//
+// That is not hypothetical. `/save` answered 502 for its entire life because
+// insertRevision() used a bare HAVING on an implicit single-group aggregate:
+//
+//   INSERT INTO settings_snapshots (…) SELECT …
+//   FROM settings_snapshots WHERE user_id = ?
+//   HAVING COALESCE(MAX(revision),0) = ?
+//
+// SQLite treats the whole result as one group and runs it. Turso answers
+// `SQL string could not be parsed: near HAVING, "None": syntax error`. The
+// test suite covered that path, passed, and proved nothing about the deployment.
+//
+// So this checks the source text rather than the behaviour. It cannot catch
+// every divergence, only the one that has already cost something — which is the
+// bar for a guard like this.
+import assert from "node:assert/strict";
+import fs from "node:fs";
+import path from "node:path";
+import test from "node:test";
+import { fileURLToPath } from "node:url";
+
+const SRC = path.join(path.dirname(fileURLToPath(import.meta.url)), "..", "src");
+
+/** Every .mjs under src/, recursively. */
+function sources(dir) {
+  const out = [];
+  for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
+    const full = path.join(dir, entry.name);
+    if (entry.isDirectory()) out.push(...sources(full));
+    else if (entry.name.endsWith(".mjs")) out.push(full);
+  }
+  return out;
+}
+
+/**
+ * Blank out comments, keeping offsets so reported line numbers stay true.
+ *
+ * Necessary because "having" is an ordinary English word and this codebase
+ * writes long comments. Matching it inside prose flagged four files that
+ * contain no SQL at all.
+ */
+function withoutComments(text) {
+  return text
+    .replace(/\/\*[\s\S]*?\*\//g, (m) => m.replace(/[^\n]/g, " "))
+    .replace(/(^|[^:])\/\/[^\n]*/g, (m, lead) => lead + " ".repeat(m.length - lead.length));
+}
+
+test("no HAVING without a GROUP BY — Turso refuses to parse it", () => {
+  const offenders = [];
+
+  for (const file of sources(SRC)) {
+    const text = withoutComments(fs.readFileSync(file, "utf8"));
+
+    // Case-sensitive on purpose: SQL keywords are written uppercase here, so
+    // this matches the keyword and not the English word.
+    for (const match of text.matchAll(/\bHAVING\b/g)) {
+      const before = text.slice(0, match.index);
+      // SQL lives in template literals, so the opening backtick is the left
+      // edge of the statement this HAVING belongs to.
+      const start = before.lastIndexOf("`");
+      const statement = text.slice(start === -1 ? 0 : start, match.index);
+      if (!/\bGROUP\s+BY\b/i.test(statement)) {
+        offenders.push(`${path.relative(SRC, file)}:${before.split("\n").length}`);
+      }
+    }
+  }
+
+  assert.deepEqual(offenders, [],
+    `HAVING with no GROUP BY parses on SQLite and fails on Turso:\n  ${offenders.join("\n  ")}`);
+});