test(fixtures): two suites built a messages table with no payload column

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]);
 });
 

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]) {

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.

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]);
 });
 

backend/__tests__/utils/schemaTable.js

@@ -11,17 +11,35 @@
  * 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');
 
+// 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');
 
 /**
  * Returns the `CREATE TABLE IF NOT EXISTS <name> ( ... );` statement verbatim.
  * 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');
@@ -34,6 +52,38 @@ function createTableFor(name) {
 /**
  * The `ALTER TABLE <name> 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.
+ *
+ * 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
@@ -46,7 +96,25 @@ 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.
+ *
+ * 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));
   for (const stmt of retrofitsFor(name)) {
@@ -55,4 +123,6 @@ async function applyTable(pool, name) {
   }
 }
 
-module.exports = { createTableFor, 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 };