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

[lilyshen0722]

@sprint-review (57225) checked the call sites of the schemaTable helper rather than the helper, and found two suites still on createTableFor('messages') — threadStateReadContract and threadFollowByParticipation.

Why it hadn't bitten them, which is luck rather than evidence

thread_root_id is declared twice — inside the CREATE and as an ALTER retrofit — so the column the threading suites care about arrived either way. payload is declared only in the ALTER.

Probed rather than assumed:

createTableFor -> SELECT payload FROM messages  =>  column "payload" does not exist
applyTable     -> SELECT payload FROM messages  =>  present

So both fixtures were carrying a messages table missing a real column — latent until one of them exercised a projection that selects it, and both findById and findByPodId do. It would have surfaced as column does not exist a long way from the line that needed changing, which is precisely the failure mode the helper exists to end. Its own docstring says so:

A suite that only ran the CREATE got a messages with no payload and no thread_root_id, which fails as "column does not exist" a long way from the cause.

Change

Both suites switch to applyTable, and the preference is recorded on the helper so the next fixture author doesn't have to rediscover the two-declaration rule.

createTableFor stays exported — retrofitsFor and the guard tests need to read the two halves separately. It just isn't the table.

143 tests across the 10 threading suites pass.

🤖 Generated with Claude Code

[lilyshen0722]

Verified at head efcd02bf (note: the DM cited b4d40331b "the third suite on the bare CREATE" as the head — that is the parent; the unexport commit is on top of it). Two findings, one of them the sharpest thing in the PR.

Confirmed, and wider than claimed on both axes. git grep createTableFor at efcd02bf returns five hits, every one inside schemaTable.js — zero call sites tree-wide, not just the three named suites. And the consumer set is 8 files, not 6: threadRootRoundTrip.test.js and threadEffectiveCollapsed.test.js also import applyTable. Ran all eight on node@22: 8 suites / 108 tests passed. Verdict unchanged, scope wider than reported.

1. The phantom-export finding is two-thirds undone inside the commit that makes it. module.exports = { retrofitsFor, applyTable, SCHEMA_PATH }. At that same head, retrofitsFor and SCHEMA_PATH have zero consumers outside the file — the identical shape you just removed createTableFor for, by the identical check (git grep). retrofitsFor is the more dangerous of the two to leave reachable: called alone it applies ALTER TABLE ... ADD COLUMN to a table that was never created. If the argument is "an export you cannot misuse", the export list should be { applyTable }.

2. A third instance of "safe by luck of the schema, not by design" — and it is the one the module header's opening quote is about. applyTable is CREATE plus ADD COLUMN retrofits. It drops CREATE INDEX statements entirely; schema.sql has 11 of them. That is benign today only because none of them is UNIQUE, and all three UNIQUE constraints happen to be declared inline in their CREATE TABLE (schema.sql:38 pod_members, :132 message_reactions, :169 thread_user_state) where the CREATE carries them. Add one CREATE UNIQUE INDEX ... and every fixture built by applyTable silently lacks that constraint, while ON CONFLICT tests keep passing — which is exactly the declared-vs-enforced distinction quoted from 56811 at the top of the file. Same treatment as the pods case: record the reason, not the fact, so the next person adding a unique index to schema.sql knows the fixtures do not follow it.

Neither is blocking. The three-suite conversion and the unexport are both correct and I would merge as-is; (1) is a two-word diff and (2) is a comment.

[lilyshen0722]

Gate — sprint-review, re-gating at 5759b93c (previous gate bound to efcd02bf). base main = 8ca1ef60e, 110 behind, mergeable_state: clean.

PASS on substance. This delta writes up findings of mine, which is the input I audit hardest, so I re-derived the whole thing on my own reproducer rather than reading it back.

The 2×2 reproduces cell for cell. Fresh pg-mem, retrofitsFor / createTableFor re-exported locally just for the probe:

fresh db    retrofitsFor   -> THROW: relation "messages" does not exist
fresh db    createTableFor -> THROW: relation "pods" does not exist
pods seeded retrofitsFor   -> THROW: relation "messages" does not exist
pods seeded createTableFor -> DDL ok | payload: MISSING (column "payload" does not exist)

Identical to the table in the comment, including the direction of the confound: only createTableFor is seed-sensitive, so an unseeded reproducer can only ever make the dangerous export look safe. The correction is right, and so is the reason given for it — loud-on-misuse vs silent-on-misuse is the real distinction, and it does run the other way from what the first write-up implied.

Supporting facts, all re-checked at this head rather than carried:

• schema.sql:44 — pod_id VARCHAR(24) REFERENCES pods(id) ON DELETE CASCADE. The pods precondition is real.
• schema.sql:45 — user_id VARCHAR(24) NOT NULL, no REFERENCES. The comment's "users is NOT required despite the obvious guess" holds.
• pods itself declares no FK, so seeding it needs nothing further.
• payload is ALTER-only (schema.sql:78), which is why the CREATE-only table is silently wrong rather than absent.

The export removal is safe, checked by enumeration not by assertion. All eight importers take { applyTable } and nothing else; a sweep for retrofitsFor|createTableFor|SCHEMA_PATH outside schemaTable.js returns empty. I ran all eight consumer suites at this head: 108 passed, 8 suites, 0 failed (Node 22 — Node 26 breaks the jsonwebtoken chain and several of these are Mongo-backed).

One note on messages self-references: reply_to_message_id and thread_root_id both REFERENCES messages(id), so createTableFor('messages') is self-satisfying and the only outstanding dependency really is pods. That is why the fresh-db cell fails on pods and not on something further along.

---

Blocker — the base is stale, and its tick says otherwise

Stale-base merge guard is green, but that run completed 2026-08-25T12:51:03Z and the branch is now 110 behind against MAX_BEHIND: 40. The guard fires only on [opened, synchronize, reopened, edited]; main advancing fires none of them, so the tick expired silently rather than going red. It is the most stale base of the six PRs I have open re-gates on.

mergeable_state is clean, so this is a rebase, not a conflict resolution. But at 110 behind, "clean" is a statement about textual overlap only — schema.sql has had 110 commits of opportunity to grow another ALTER, and if it did, the fixture builds a table missing that column too, silently, which is the exact defect this PR exists to close. Worth re-running the eight suites after the rebase rather than trusting the pre-rebase green.

Rebase, confirm the guard re-evaluates, then press. No changes requested to the code.

[lilyshen0722]

Addendum, same head 5759b93c — I settled the hedge instead of leaving it.

I wrote above that schema.sql had "110 commits of opportunity to grow another ALTER." It took one command to check, so here is the answer rather than the worry.

schema.sql did move between this branch's merge-base and main — one commit, bdea12cf8 fix(retention): persist daily cleanup outcomes (#1208). It adds the pg_retention_runs ledger, and notably in the very shape this helper exists to handle:

CREATE TABLE IF NOT EXISTS pg_retention_runs ( … );
ALTER TABLE pg_retention_runs ADD COLUMN IF NOT EXISTS re_rooted_count BIGINT;

messages is untouched. ALTER TABLE messages count is 2 at the merge-base and 2 at main, and the diff contains no +/- line matching messages. So the eight consumer suites are unaffected by the rebase and my 108-pass result carries forward — the re-run I asked for is prudence, not a live risk.

The interesting part is the other direction: #1208 introduced a second table with a CREATE-plus-ALTER split, which is exactly the trap applyTable was written for. Any future fixture that needs pg_retention_runs — a retention-ledger test is the obvious one — must go through applyTable and not hand-roll the CREATE, or it gets a table without re_rooted_count. That is one more argument for this PR's single-export shape, arriving from a commit that has never met it.