From ef4903a3c52f62cfdf8320fdd1bbd93075df214b Mon Sep 17 00:00:00 2001 From: Tom McKenzie Date: Thu, 2 Jul 2026 22:57:05 +1000 Subject: [PATCH 1/2] fix(server): reject WITHOUT ROWID tables in assertSyncCompatible (ADR-0015) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cold-snapshot/fetch reader defaults to `ORDER BY rowid` when the client sends no orderBy (ADR-0015). A WITHOUT ROWID table has no rowid, so the read threw `no such column: rowid` and hung the subscriber — after the table had already passed registration. assertSyncCompatible now probes for a usable rowid and rejects such a table loudly at registerSync, alongside the existing INTEGER PRIMARY KEY guard. Probes the exact property the reader depends on rather than parsing the DDL; an unrelated exec failure rethrows unchanged. Corrects the ADR-0015 / sql-compiler claim that the rowid is "always intact" under D9 (D9 forbade only the INTEGER-alias pk, not WITHOUT ROWID). Ordinary rowid tables (id TEXT PRIMARY KEY) are unaffected. Adds a reason-pinning test; full suite 192 green. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 12 ++++++++++++ docs/adr/0015-syncable-mixin.md | 5 ++++- src/server/changes.ts | 27 +++++++++++++++++++++++---- src/server/sql-compiler.ts | 7 ++++--- tests/assert-sync-compatible.test.ts | 12 ++++++++++++ 5 files changed, 55 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 036f0bd..339eeed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,18 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). While pre-1.0, the public API may change between 0.x releases. +## [Unreleased] + +### Fixed + +- **`registerSync` now rejects a `WITHOUT ROWID` table (ADR-0015).** The + cold-snapshot/fetch reader defaults to `ORDER BY rowid` when the client sends + no `orderBy`; a `WITHOUT ROWID` table has no rowid, so that read threw + `no such column: rowid` and hung the subscriber. `assertSyncCompatible` now + probes for a usable rowid and rejects such a table loudly at `registerSync`, + alongside the existing `INTEGER PRIMARY KEY` guard. Ordinary rowid tables + (the documented `id TEXT PRIMARY KEY` pattern) are unaffected. + ## [0.5.0] — 2026-07-02 ### Added diff --git a/docs/adr/0015-syncable-mixin.md b/docs/adr/0015-syncable-mixin.md index 3ac74e7..a325684 100644 --- a/docs/adr/0015-syncable-mixin.md +++ b/docs/adr/0015-syncable-mixin.md @@ -194,7 +194,10 @@ tddc design gap; a PR against Actors is possible in principle but out of scope. pk's autoindex over a rowid scan, returning pk-sorted rows instead of insertion order). `rowid` matches insertion order among currently-live rows and needs no schema change, since `assertSyncCompatible` (ADR-0007, D9) - already forbids the `INTEGER PRIMARY KEY` pk that would alias it. + already forbids the `INTEGER PRIMARY KEY` pk that would alias it. It also + forbids a `WITHOUT ROWID` table, which has no rowid at all — otherwise the + read would throw `no such column: rowid` and hang the subscriber. Both are + rejected at `registerSync`, not left to surface at read time. - One DO class can be both a framework host and a sync source (`class FeedAgent extends Syncable()(Agent)`), with no framework added to tddc's dependency graph — the app supplies `Base`. diff --git a/src/server/changes.ts b/src/server/changes.ts index b48621a..c23c02e 100644 --- a/src/server/changes.ts +++ b/src/server/changes.ts @@ -119,10 +119,12 @@ export function ensureTriggers( * Validate that an author-created table is sync-compatible (ADR-0007, D9): the * declared `pk` must be the table's SOLE primary key and have TEXT affinity (a * client-supplied stable key: TEXT/VARCHAR/CHAR/…). Rejects a missing table, a - * composite/wrong pk, and an `INTEGER PRIMARY KEY` (the rowid alias — - * server-assigned, which breaks optimistic id parity). Introspects the real - * table, so it works however the author created it. `pragma_table_info` is the - * table-valued form, so the table name binds as a parameter. + * composite/wrong pk, an `INTEGER PRIMARY KEY` (the rowid alias — server-assigned, + * which breaks optimistic id parity), and a `WITHOUT ROWID` table (the + * cold-snapshot/fetch reader orders by `rowid` when the client sends no orderBy — + * ADR-0015 — and such a table has none). Introspects the real table, so it works + * however the author created it. `pragma_table_info` is the table-valued form, so + * the table name binds as a parameter. */ export function assertSyncCompatible(sql: SqlStorage, tbl: string, pk: string): void { const cols = Array.from( @@ -147,6 +149,23 @@ export function assertSyncCompatible(sql: SqlStorage, tbl: string, pk: string): `key aliases rowid (server-assigned) and breaks optimistic id parity (D9).`, ) } + // A synced table must have a real `rowid`: the cold-snapshot/fetch reader + // defaults to `ORDER BY rowid` when the client sends no orderBy (ADR-0015). A + // `WITHOUT ROWID` table has none, so that read would throw `no such column: + // rowid` and hang the subscriber. Probe the exact property the reader relies on + // rather than parse the DDL — a compile-time "no such column: rowid" is the + // signal; anything else is an unrelated failure and rethrows unchanged. + try { + sql.exec(`SELECT rowid FROM "${tbl}" LIMIT 0`) + } catch (e) { + const msg = e instanceof Error ? e.message : String(e) + if (!/rowid/i.test(msg)) throw e + throw new Error( + `collection '${tbl}': table is WITHOUT ROWID — sync requires a rowid table so snapshot/fetch ` + + `reads have a deterministic default order (ORDER BY rowid, ADR-0015). Recreate '${tbl}' ` + + `without the WITHOUT ROWID clause.`, + ) + } } /** diff --git a/src/server/sql-compiler.ts b/src/server/sql-compiler.ts index 0537d8f..ce101fa 100644 --- a/src/server/sql-compiler.ts +++ b/src/server/sql-compiler.ts @@ -190,9 +190,10 @@ export function compileSubsetQuery(tbl: string, opts: SubsetQuery): { sql: strin // the planner pick the pk's autoindex instead, returning pk-sorted rows). // `rowid` also matches insertion order among currently-live rows: a rowid // table assigns each new row 1 + the current max, which is monotonic - // across inserts regardless of intervening deletes. Every synced table - // qualifies — `assertSyncCompatible` (ADR-0007, D9) forbids an `INTEGER - // PRIMARY KEY` pk, so the real rowid is always intact underneath. + // across inserts regardless of intervening deletes. Every synced table has a + // real rowid: `assertSyncCompatible` (changes.ts) forbids both an `INTEGER + // PRIMARY KEY` pk (which would alias it) and a `WITHOUT ROWID` table (which + // would have none), so this reference always resolves. sql += ` ORDER BY rowid` } diff --git a/tests/assert-sync-compatible.test.ts b/tests/assert-sync-compatible.test.ts index 32312ae..602f284 100644 --- a/tests/assert-sync-compatible.test.ts +++ b/tests/assert-sync-compatible.test.ts @@ -76,4 +76,16 @@ describe("assertSyncCompatible (ADR-0007, D9) — real-table introspection", () expect(() => assertSyncCompatible(sql, "t", "id")).toThrow(/sole PRIMARY KEY/) }) }) + + it("rejects a WITHOUT ROWID table (no rowid for the default snapshot order)", async () => { + // The pk is a valid sole TEXT key, so it clears every D9 check — but a + // WITHOUT ROWID table has no rowid, and the cold-snapshot/fetch reader + // defaults to `ORDER BY rowid` (ADR-0015). Without this guard the table + // registers, then the first orderBy-less sub throws `no such column: rowid` + // at read time and hangs the subscriber. Reject loudly at registerSync. + await withSql((sql) => { + sql.exec("CREATE TABLE t (id TEXT PRIMARY KEY, body TEXT) WITHOUT ROWID") + expect(() => assertSyncCompatible(sql, "t", "id")).toThrow(/WITHOUT ROWID/) + }) + }) }) From 8f5005c1b4c09ad8b6e92538c599085dafd0de17 Mon Sep 17 00:00:00 2001 From: Tom McKenzie Date: Thu, 2 Jul 2026 23:12:04 +1000 Subject: [PATCH 2/2] fix(server): also reject a declared `rowid` column (codex review) Adversarial review surfaced a shadow the SELECT-rowid probe alone misses: a user column named `rowid` resolves the probe (SQLite binds the name to the column), passes validation, then the compiler's default `ORDER BY rowid` silently sorts by that arbitrary column instead of insertion order. Reject a declared `rowid` column via the introspected pragma_table_info columns before the WITHOUT ROWID probe, which makes the probe unambiguous. Adds a test; docs and CHANGELOG updated to cover all three no-usable-rowid cases. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 15 ++++++----- docs/adr/0015-syncable-mixin.md | 9 ++++--- src/server/changes.ts | 38 ++++++++++++++++++++-------- src/server/sql-compiler.ts | 9 ++++--- tests/assert-sync-compatible.test.ts | 12 +++++++++ 5 files changed, 58 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 339eeed..8134996 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,13 +10,14 @@ While pre-1.0, the public API may change between 0.x releases. ### Fixed -- **`registerSync` now rejects a `WITHOUT ROWID` table (ADR-0015).** The - cold-snapshot/fetch reader defaults to `ORDER BY rowid` when the client sends - no `orderBy`; a `WITHOUT ROWID` table has no rowid, so that read threw - `no such column: rowid` and hung the subscriber. `assertSyncCompatible` now - probes for a usable rowid and rejects such a table loudly at `registerSync`, - alongside the existing `INTEGER PRIMARY KEY` guard. Ordinary rowid tables - (the documented `id TEXT PRIMARY KEY` pattern) are unaffected. +- **`registerSync` now rejects tables with no usable internal `rowid` (ADR-0015).** + The cold-snapshot/fetch reader defaults to `ORDER BY rowid` when the client + sends no `orderBy`. A `WITHOUT ROWID` table has no rowid, so that read threw + `no such column: rowid` and hung the subscriber; a table with a declared + `rowid` column shadows the internal one, so the read would silently sort by + that arbitrary column. `assertSyncCompatible` now rejects both loudly at + `registerSync`, alongside the existing `INTEGER PRIMARY KEY` guard. Ordinary + rowid tables (the documented `id TEXT PRIMARY KEY` pattern) are unaffected. ## [0.5.0] — 2026-07-02 diff --git a/docs/adr/0015-syncable-mixin.md b/docs/adr/0015-syncable-mixin.md index a325684..3ab10b1 100644 --- a/docs/adr/0015-syncable-mixin.md +++ b/docs/adr/0015-syncable-mixin.md @@ -195,9 +195,12 @@ tddc design gap; a PR against Actors is possible in principle but out of scope. insertion order). `rowid` matches insertion order among currently-live rows and needs no schema change, since `assertSyncCompatible` (ADR-0007, D9) already forbids the `INTEGER PRIMARY KEY` pk that would alias it. It also - forbids a `WITHOUT ROWID` table, which has no rowid at all — otherwise the - read would throw `no such column: rowid` and hang the subscriber. Both are - rejected at `registerSync`, not left to surface at read time. + forbids the two other ways a table can lack a usable internal rowid: a + `WITHOUT ROWID` table (none at all — the read would throw `no such column: + rowid` and hang the subscriber) and a declared `rowid` column (which shadows + the internal one, so `ORDER BY rowid` would silently sort by that arbitrary + column). All three are rejected at `registerSync`, not left to surface at read + time. - One DO class can be both a framework host and a sync source (`class FeedAgent extends Syncable()(Agent)`), with no framework added to tddc's dependency graph — the app supplies `Base`. diff --git a/src/server/changes.ts b/src/server/changes.ts index c23c02e..c1fe98a 100644 --- a/src/server/changes.ts +++ b/src/server/changes.ts @@ -120,11 +120,12 @@ export function ensureTriggers( * declared `pk` must be the table's SOLE primary key and have TEXT affinity (a * client-supplied stable key: TEXT/VARCHAR/CHAR/…). Rejects a missing table, a * composite/wrong pk, an `INTEGER PRIMARY KEY` (the rowid alias — server-assigned, - * which breaks optimistic id parity), and a `WITHOUT ROWID` table (the - * cold-snapshot/fetch reader orders by `rowid` when the client sends no orderBy — - * ADR-0015 — and such a table has none). Introspects the real table, so it works - * however the author created it. `pragma_table_info` is the table-valued form, so - * the table name binds as a parameter. + * which breaks optimistic id parity), and any table that lacks a usable internal + * `rowid` — a `WITHOUT ROWID` table (which has none) or one with a declared + * `rowid` column (which shadows it) — because the cold-snapshot/fetch reader + * orders by `rowid` when the client sends no orderBy (ADR-0015). Introspects the + * real table, so it works however the author created it. `pragma_table_info` is + * the table-valued form, so the table name binds as a parameter. */ export function assertSyncCompatible(sql: SqlStorage, tbl: string, pk: string): void { const cols = Array.from( @@ -149,12 +150,27 @@ export function assertSyncCompatible(sql: SqlStorage, tbl: string, pk: string): `key aliases rowid (server-assigned) and breaks optimistic id parity (D9).`, ) } - // A synced table must have a real `rowid`: the cold-snapshot/fetch reader - // defaults to `ORDER BY rowid` when the client sends no orderBy (ADR-0015). A - // `WITHOUT ROWID` table has none, so that read would throw `no such column: - // rowid` and hang the subscriber. Probe the exact property the reader relies on - // rather than parse the DDL — a compile-time "no such column: rowid" is the - // signal; anything else is an unrelated failure and rethrows unchanged. + // The cold-snapshot/fetch reader defaults to `ORDER BY rowid` when the client + // sends no orderBy (ADR-0015), so a synced table must expose SQLite's internal + // rowid — the insertion-order key that default is built on. Two ways an author + // can defeat that, both rejected here at registerSync rather than left to + // surface at read time: + // + // 1. A declared column named `rowid` SHADOWS the internal rowid, so + // `ORDER BY rowid` would silently sort by that arbitrary user column + // (and lose determinism on null/duplicate values). Check the introspected + // columns directly. + if (cols.some((c) => c.name.toLowerCase() === "rowid")) { + throw new Error( + `collection '${tbl}': a column named 'rowid' shadows SQLite's internal rowid, which the ` + + `snapshot/fetch reader orders by when the client sends no orderBy (ADR-0015). Rename that column.`, + ) + } + // 2. A `WITHOUT ROWID` table has NO rowid at all, so `ORDER BY rowid` throws + // `no such column: rowid` and hangs the subscriber. With a shadowing + // column already ruled out, probing the exact property the reader relies + // on is unambiguous — a compile-time "no such column: rowid" is the + // signal; anything else is an unrelated failure and rethrows unchanged. try { sql.exec(`SELECT rowid FROM "${tbl}" LIMIT 0`) } catch (e) { diff --git a/src/server/sql-compiler.ts b/src/server/sql-compiler.ts index ce101fa..e0a1b34 100644 --- a/src/server/sql-compiler.ts +++ b/src/server/sql-compiler.ts @@ -190,10 +190,11 @@ export function compileSubsetQuery(tbl: string, opts: SubsetQuery): { sql: strin // the planner pick the pk's autoindex instead, returning pk-sorted rows). // `rowid` also matches insertion order among currently-live rows: a rowid // table assigns each new row 1 + the current max, which is monotonic - // across inserts regardless of intervening deletes. Every synced table has a - // real rowid: `assertSyncCompatible` (changes.ts) forbids both an `INTEGER - // PRIMARY KEY` pk (which would alias it) and a `WITHOUT ROWID` table (which - // would have none), so this reference always resolves. + // across inserts regardless of intervening deletes. Every synced table + // exposes a real internal rowid: `assertSyncCompatible` (changes.ts) forbids + // an `INTEGER PRIMARY KEY` pk (which aliases it), a `WITHOUT ROWID` table + // (which has none), and a declared `rowid` column (which shadows it), so this + // reference always resolves to insertion order. sql += ` ORDER BY rowid` } diff --git a/tests/assert-sync-compatible.test.ts b/tests/assert-sync-compatible.test.ts index 602f284..23ce890 100644 --- a/tests/assert-sync-compatible.test.ts +++ b/tests/assert-sync-compatible.test.ts @@ -88,4 +88,16 @@ describe("assertSyncCompatible (ADR-0007, D9) — real-table introspection", () expect(() => assertSyncCompatible(sql, "t", "id")).toThrow(/WITHOUT ROWID/) }) }) + + it("rejects a declared `rowid` column that shadows SQLite's internal rowid", async () => { + // A user column named `rowid` clears every D9 check AND makes `SELECT rowid` + // resolve — but it shadows the internal insertion-order rowid, so the + // reader's default `ORDER BY rowid` (ADR-0015) would sort by this arbitrary + // column and lose determinism on null/duplicate values. Probing alone can't + // see the shadow; reject the declared column explicitly. + await withSql((sql) => { + sql.exec("CREATE TABLE t (id TEXT PRIMARY KEY, rowid TEXT)") + expect(() => assertSyncCompatible(sql, "t", "id")).toThrow(/shadows/) + }) + }) })