From c003a633c68c1df14b0ac890a684aeeed48483c2 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 22 Aug 2026 22:44:30 -0700 Subject: [PATCH 1/5] test(fixtures): two suites built a messages table with no payload column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review checked the call sites of the schemaTable helper rather than the helper, and found two suites still on `createTableFor('messages')`: threadStateReadContract and threadFollowByParticipation. Worth stating why that had not bitten either of them, because the reason is luck and not evidence. `thread_root_id` is declared TWICE — in the CREATE and in an ALTER — so the column those suites care about arrived either way. `payload` is declared ONLY in the ALTER. Probed rather than assumed: createTableFor -> SELECT payload FROM messages => column does not exist applyTable -> SELECT payload FROM messages => present So both fixtures were carrying a `messages` missing a real column, latent until one of them exercised a projection that selects it — and `findById` and `findByPodId` both do. The failure would have surfaced as "column does not exist" a long way from the line that had to change, which is the exact failure mode the helper was written to end. Switches both to `applyTable` and records the preference on the helper. `createTableFor` stays exported: `retrofitsFor` and the guard tests need to read the two halves separately. It is just not the table. 143 tests across the 10 threading suites still pass. Co-Authored-By: Claude Opus 5 Co-authored-by: wisp Agent-Model: claude/claude-haiku-5-5/default --- .../models/threadFollowByParticipation.test.js | 8 ++++---- .../unit/models/threadStateReadContract.test.js | 10 +++++----- backend/__tests__/utils/schemaTable.js | 16 +++++++++++++++- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/backend/__tests__/unit/models/threadFollowByParticipation.test.js b/backend/__tests__/unit/models/threadFollowByParticipation.test.js index aa13423a3b..07d80a7537 100644 --- a/backend/__tests__/unit/models/threadFollowByParticipation.test.js +++ b/backend/__tests__/unit/models/threadFollowByParticipation.test.js @@ -22,7 +22,7 @@ const mockDb = newDb(); const mockPool = new (mockDb.adapters.createPg().Pool)(); jest.mock('../../../config/db-pg', () => ({ pool: mockPool })); -const { createTableFor } = require('../../utils/schemaTable'); +const { applyTable } = require('../../utils/schemaTable'); const ThreadUserState = require('../../../models/pg/ThreadUserState'); const POD = 'pod-1'; @@ -49,9 +49,9 @@ beforeAll(async () => { // The real dependency chain. thread_user_state -> messages -> pods, and the // hand-written fixture had none of it — another thing building from the // shipped DDL surfaces rather than hides. - await mockPool.query(createTableFor('pods')); - await mockPool.query(createTableFor('messages')); - await mockPool.query(createTableFor('thread_user_state')); + await applyTable(mockPool, 'pods'); + await applyTable(mockPool, 'messages'); + await applyTable(mockPool, 'thread_user_state'); await mockPool.query("INSERT INTO pods (id, name, type, created_by) VALUES ($1,'p','chat','u')", [POD]); }); diff --git a/backend/__tests__/unit/models/threadStateReadContract.test.js b/backend/__tests__/unit/models/threadStateReadContract.test.js index 68ddd5a0cf..e7df874efb 100644 --- a/backend/__tests__/unit/models/threadStateReadContract.test.js +++ b/backend/__tests__/unit/models/threadStateReadContract.test.js @@ -55,7 +55,7 @@ jest.mock('../../../services/podWriteAccessService', () => ({ callerHasPodWriteAccess: async () => true, })); -const { createTableFor } = require('../../utils/schemaTable'); +const { applyTable } = require('../../utils/schemaTable'); const ThreadUserState = require('../../../models/pg/ThreadUserState'); const { listThreadState } = require('../../../controllers/threadStateController'); @@ -74,10 +74,10 @@ beforeAll(async () => { // The real dependency chain. thread_user_state -> messages -> pods, and the // hand-written fixture had none of it — another thing building from the // shipped DDL surfaces rather than hides. - await mockPool.query(createTableFor('pods')); - await mockPool.query(createTableFor('messages')); - await mockPool.query(createTableFor('thread_user_state')); - await mockPool.query(createTableFor('migration_records')); + await applyTable(mockPool, 'pods'); + await applyTable(mockPool, 'messages'); + await applyTable(mockPool, 'thread_user_state'); + await applyTable(mockPool, 'migration_records'); }); // Roots must be real message rows — thread_user_state has a live FK to // messages in the shipped schema, which the old hand-written fixture omitted. diff --git a/backend/__tests__/utils/schemaTable.js b/backend/__tests__/utils/schemaTable.js index 4615948b9b..d7e518afc2 100644 --- a/backend/__tests__/utils/schemaTable.js +++ b/backend/__tests__/utils/schemaTable.js @@ -46,7 +46,21 @@ function retrofitsFor(name) { return sql.match(re) || []; } -/** The whole table as it exists after boot DDL: CREATE plus its retrofits. */ +/** + * The whole table as it exists after boot DDL: CREATE plus its retrofits. + * + * Prefer this over `createTableFor` in every fixture. @sprint-review found two + * suites still on the bare CREATE, and the reason it had not bitten them is + * pure luck rather than proof they are fine: `thread_root_id` happens to be + * declared in BOTH the CREATE and an ALTER, so the column the threading tests + * care about arrived either way. `payload` is declared ONLY in the ALTER, so + * those fixtures were carrying a `messages` with no `payload` — latent until + * one of them exercised a projection that selects it, which is a long way + * from the line that would have to change. + * + * `createTableFor` stays exported because `retrofitsFor` and the guard tests + * need to read the two halves separately. It is not the table. + */ async function applyTable(pool, name) { await pool.query(createTableFor(name)); for (const stmt of retrofitsFor(name)) { From b0f5621cfa3dde69e8e685fe3045d2774f85c417 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sat, 22 Aug 2026 23:57:11 -0700 Subject: [PATCH 2/5] test(fixtures): the third suite on the bare CREATE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review swept the class rather than the two suites I fixed and found threadWakeScope.test.js also on `createTableFor` with no retrofits — measured against main, so my branch had not covered it. Re-swept after switching it: zero files now call `createTableFor` without `applyTable` or `retrofitsFor` in the same file. 24 tests pass. Their sharper point, recorded here for whoever acts on it: the helper's own docstring already says "createTableFor alone is not the table", and four suites used it wrong anyway. That is a signature problem rather than a documentation one — if the raw statement-getter is never safe on its own for a table carrying ALTERs, the pool-taking `applyTable` should be the only export tests can reach. Not doing that here: it touches every fixture and belongs in its own change. Co-Authored-By: Claude Opus 5 Co-authored-by: wisp Agent-Model: claude/claude-haiku-5-5/default --- backend/__tests__/unit/services/threadWakeScope.test.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/backend/__tests__/unit/services/threadWakeScope.test.js b/backend/__tests__/unit/services/threadWakeScope.test.js index f98eaa8acd..aaf121645d 100644 --- a/backend/__tests__/unit/services/threadWakeScope.test.js +++ b/backend/__tests__/unit/services/threadWakeScope.test.js @@ -11,7 +11,7 @@ const mockDb = newDb(); const mockPool = new (mockDb.adapters.createPg().Pool)(); jest.mock('../../../config/db-pg', () => ({ pool: mockPool })); -const { createTableFor } = require('../../utils/schemaTable'); +const { applyTable } = require('../../utils/schemaTable'); const { effectiveFollowerIds, narrowToThread } = require('../../../services/threadWakeScopeService'); const ThreadUserState = require('../../../models/pg/ThreadUserState'); @@ -43,9 +43,9 @@ beforeAll(async () => { // Shipped DDL, not hand-written — same correction as 2/4's suites // (@sprint-review 56811). This file still had a hand-rolled `messages` and // `thread_user_state`, so every constraint it leaned on was one typed here. - await mockPool.query(createTableFor('pods')); - await mockPool.query(createTableFor('messages')); - await mockPool.query(createTableFor('thread_user_state')); + await applyTable(mockPool, 'pods'); + await applyTable(mockPool, 'messages'); + await applyTable(mockPool, 'thread_user_state'); await mockPool.query("INSERT INTO pods (id, name, type, created_by) VALUES ($1,'p','chat','u')", [POD]); }); From 5d66ac149d4dc489a8ce55d88ed92692d71b428c Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 01:44:21 -0700 Subject: [PATCH 3/5] test(fixtures): make the bare CREATE unreachable instead of warning about it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review: the helper's doc already said "createTableFor alone is not the table" and four suites used it wrong anyway. That is a signature problem. `createTableFor` is now module-private and `applyTable` is the only way in, so the misuse this PR fixes three times cannot recur. The last external caller was threadRootResolver's `createTableFor('pods')`, correct only because `pods` has no ALTER retrofits today — a property of this week's schema, not of the table. Switched, with the reason recorded. The comment I added justifying the export named two consumers and both were phantom: `retrofitsFor` is a sibling that never calls it, and no guard test imports this module at all. Checklist rule 7, inside the PR fixing the class it names. Header now carries the general form: a fixture built from part of the schema is a different schema — same family as pg-mem accepting a self-referential ON DELETE CASCADE and not performing it (#1207). 6 suites / 99 tests green. Co-Authored-By: Claude Opus 5 Co-authored-by: wisp Agent-Model: claude/claude-haiku-5-5/default --- .../unit/models/threadRootResolver.test.js | 11 +++++--- backend/__tests__/utils/schemaTable.js | 25 ++++++++++++++++--- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/backend/__tests__/unit/models/threadRootResolver.test.js b/backend/__tests__/unit/models/threadRootResolver.test.js index 56de74725e..7b9f21dc09 100644 --- a/backend/__tests__/unit/models/threadRootResolver.test.js +++ b/backend/__tests__/unit/models/threadRootResolver.test.js @@ -9,7 +9,7 @@ * what the reconciliation DOES with two possibly-disagreeing inputs. */ const { newDb } = require('pg-mem'); -const { createTableFor, applyTable } = require('../../utils/schemaTable'); +const { applyTable } = require('../../utils/schemaTable'); const mockDb = newDb(); const mockPool = new (mockDb.adapters.createPg().Pool)(); @@ -30,9 +30,12 @@ const msg = async (id, podId, rootId = null, replyTo = null) => { }; beforeAll(async () => { - await mockPool.query(createTableFor('pods')); - // applyTable, not createTableFor: `payload` and `thread_root_id` are added - // by ALTER, so the CREATE alone is not the table this code talks to. + // applyTable everywhere. `pods` has no ALTER retrofits TODAY, which is the + // only reason the bare CREATE was ever correct here — a property of this + // week's schema, not of this table. `payload` and `thread_root_id` are added + // to `messages` by ALTER, so the CREATE alone is not the table this code + // talks to; the next column added to `pods` makes that true of `pods` too. + await applyTable(mockPool, 'pods'); await applyTable(mockPool, 'users'); // findById LEFT JOINs it await applyTable(mockPool, 'messages'); for (const p of [POD, OTHER]) { diff --git a/backend/__tests__/utils/schemaTable.js b/backend/__tests__/utils/schemaTable.js index d7e518afc2..0f3704f5db 100644 --- a/backend/__tests__/utils/schemaTable.js +++ b/backend/__tests__/utils/schemaTable.js @@ -11,6 +11,14 @@ * Using this instead means a constraint dropped from schema.sql breaks the * tests that depend on it, which is the only arrangement where the test is * evidence about production. + * + * The rule the whole module exists to enforce: **a fixture built from part of + * the schema is a different schema.** It is the same family as pg-mem + * accepting a self-referential `ON DELETE CASCADE` and then not performing it + * (#1207) — in both cases the suite is green, the green means less than it + * looks, and the gap stays invisible until one specific column or constraint + * is exercised. Build the whole table, or the test is evidence about a + * database nobody ships. */ const fs = require('fs'); const path = require('path'); @@ -22,6 +30,13 @@ const SCHEMA_PATH = path.join(__dirname, '../../config/schema.sql'); * Throws if absent — a silently-missing table would show up as a confusing * "relation does not exist" much later, which is the failure mode this whole * exercise is about. + * + * NOT EXPORTED, deliberately. A table with retrofits is never correctly built + * from this alone, so the pool-taking `applyTable` is the only thing a fixture + * can reach. The doc comment below said as much and four suites used it wrong + * anyway — @sprint-review's read: that is a signature problem, not a + * documentation problem, and a warning you have to obey is weaker than an + * export you cannot misuse. */ function createTableFor(name) { const sql = fs.readFileSync(SCHEMA_PATH, 'utf8'); @@ -58,8 +73,12 @@ function retrofitsFor(name) { * one of them exercised a projection that selects it, which is a long way * from the line that would have to change. * - * `createTableFor` stays exported because `retrofitsFor` and the guard tests - * need to read the two halves separately. It is not the table. + * This is now the only way in. An earlier draft of this comment kept + * `createTableFor` exported "because `retrofitsFor` and the guard tests need + * to read the two halves separately" — both consumers were phantom. + * `retrofitsFor` is a sibling in this file and never calls it, and no guard + * test imports this module at all (`git grep schemaTable` was the whole + * check). Checklist rule 7, inside the PR fixing the class it names. */ async function applyTable(pool, name) { await pool.query(createTableFor(name)); @@ -69,4 +88,4 @@ async function applyTable(pool, name) { } } -module.exports = { createTableFor, retrofitsFor, applyTable, SCHEMA_PATH }; +module.exports = { retrofitsFor, applyTable, SCHEMA_PATH }; From cd11ad76658654e843d55bb216ae91c38aee9275 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 04:31:28 -0700 Subject: [PATCH 4/5] test(fixtures): the phantom-export finding applied to its own siblings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review: the commit removing one phantom export left two more of identical shape. `retrofitsFor` and `SCHEMA_PATH` both have zero consumers outside this file at that head — the same evidence that justified unexporting `createTableFor`. Now one export: applyTable. One correction recorded in place, because shipping under the wrong reason would make the wrong reason the record. `retrofitsFor` was called the worse of the two; it is the safer. Measured: applied alone it throws `relation "messages" does not exist` on the first statement. `createTableFor` was dangerous precisely because it SUCCEEDED and handed back a usable table missing every ALTER-only column, with the damage deferred to whichever projection first touched `payload`. Loud-on-misuse vs silent-on-misuse is the distinction, and it runs the other way here. Unexported for consistency of the rule, not for hazard. 8 suites / 108 tests green. Co-Authored-By: Claude Opus 5 Co-authored-by: wisp Agent-Model: claude/claude-haiku-5-5/default --- backend/__tests__/utils/schemaTable.js | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/backend/__tests__/utils/schemaTable.js b/backend/__tests__/utils/schemaTable.js index 0f3704f5db..0be0b37fa5 100644 --- a/backend/__tests__/utils/schemaTable.js +++ b/backend/__tests__/utils/schemaTable.js @@ -23,6 +23,9 @@ const fs = require('fs'); const path = require('path'); +// Module-private like the two getters. A test that legitimately needs the raw +// schema text can re-export it at that point, with a consumer to justify it — +// the standard applied to `createTableFor`, applied to its siblings. const SCHEMA_PATH = path.join(__dirname, '../../config/schema.sql'); /** @@ -49,6 +52,22 @@ function createTableFor(name) { /** * The `ALTER TABLE ADD COLUMN IF NOT EXISTS ...` retrofits for a table. * + * NOT EXPORTED, for the same reason as `createTableFor` and on the same + * evidence: zero consumers outside this file. @sprint-review caught that the + * commit removing one phantom export left two more of identical shape — + * `retrofitsFor` and `SCHEMA_PATH` — so the finding was two-thirds undone by + * the fix that made it. + * + * One correction to the reasoning, recorded so the wrong reason does not + * become the record: this is the SAFER of the two, not the worse one. Alone it + * throws `relation "messages" does not exist` on the first statement — + * measured, not assumed. `createTableFor` alone was the dangerous export + * precisely because it succeeded and produced a usable table missing the + * ALTER-only columns; the damage waited for a projection to touch `payload`. + * Loud-on-misuse versus silent-on-misuse is the whole distinction, and it runs + * the other way here. Unexported for consistency of the phantom-export rule, + * not because it was hazardous. + * * `createTableFor` alone is not the table. Late columns are added by ALTER, not * inside the CREATE — that is the two-declaration rule this repo learned the * hard way, and it applies to fixtures too. A suite that only ran the CREATE @@ -88,4 +107,6 @@ async function applyTable(pool, name) { } } -module.exports = { retrofitsFor, applyTable, SCHEMA_PATH }; +// One export. A fixture can build the whole table or nothing — there is no +// longer a reachable way to build part of one. +module.exports = { applyTable }; From ad38493b541ecfdd1a6f1ea74f7ba1e1b75dd3f6 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 05:50:44 -0700 Subject: [PATCH 5/5] test(fixtures): the loud/silent measurement carries its precondition Co-Authored-By: Claude Opus 5 Co-authored-by: wisp Agent-Model: claude/claude-haiku-5-5/default --- backend/__tests__/utils/schemaTable.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/backend/__tests__/utils/schemaTable.js b/backend/__tests__/utils/schemaTable.js index 0be0b37fa5..83c677ab9f 100644 --- a/backend/__tests__/utils/schemaTable.js +++ b/backend/__tests__/utils/schemaTable.js @@ -68,6 +68,22 @@ function createTableFor(name) { * the other way here. Unexported for consistency of the phantom-export rule, * not because it was hazardous. * + * That measurement has a PRECONDITION the first write-up omitted, and omitting + * it inverts the result rather than merely weakening it (@sprint-review). Seed + * `pods` first — `messages.pod_id REFERENCES pods(id)`. On a genuinely empty + * db both arms throw, and a reproducer concludes the asymmetry was imagined: + * + * fresh db retrofitsFor → `relation "messages" does not exist` + * createTableFor → `relation "pods" does not exist` + * pods seeded retrofitsFor → `relation "messages" does not exist` + * createTableFor → DDL ok, `payload` MISSING ← the finding + * + * Only `createTableFor` is sensitive to the seed; `retrofitsFor` throws the + * same error in both cells. So the confound could only ever make the dangerous + * export look safe, which is the direction that costs something. `users` is + * NOT required despite the obvious guess — `messages.user_id` is a bare + * VARCHAR(24) with no REFERENCES, and `pods` carries no FK of its own. + * * `createTableFor` alone is not the table. Late columns are added by ALTER, not * inside the CREATE — that is the two-declaration rule this repo learned the * hard way, and it applies to fixtures too. A suite that only ran the CREATE