Backfill cutoff tests pin text order, not control flow — both survive their own named regression

[lilyshen0722]

Two tests in backend/__tests__/unit/models/threadingCutoffRecord.test.js are described as pinning control-flow properties of backend/scripts/backfill-thread-root-id.ts. Both are SCRIPT.indexOf position comparisons over the script's source text, and a text-position comparison cannot see a return. I mutated each at c0da084f and both stayed green on a script that has the regression the test names.

Nothing here is a defect in the script. I read both blocks at the merged head and the behaviour is correct. This is about what the guards would catch if someone edits the script later.

1. the UPDATE and the ledger INSERT are one transaction (:97)

Comment: "losing the INSERT after the UPDATE leaves every chain rooted and no cutoff recorded."

Mutation — insert between the UPDATE and the INSERT, inside the try:

if (process.env.SKIP_LEDGER) return;

The ledger write becomes unreachable. All four anchors (BEGIN, UPDATE messages m, INSERT INTO migration_records, COMMIT) keep their relative positions, so begin < update < insert < commit still holds. 26/26 script tests passed, including this one.

(A first attempt that also added a COMMIT did trip the test — but via indexOf("await client.query('COMMIT')") matching the added line, not via the unreachable INSERT. That mutant was invalid; the bare return above is the isolated one.)

2. a run that finds the ledger row reports it and never re-measures (:85)

Asserts ledgerRead < populationRead. The behaviour it describes rests entirely on the return at :209 — reading the ledger first is not the property, stopping is.

Mutation — delete that one line:

       console.log(`${MIGRATION_NAME} already recorded at ${ledger.applied_at}: `
         + `cutoff = ${ledger.cutoff ?? '(none)'}. Nothing to do — the boundary is not re-measured.`);
-      return;

The script now reads the ledger, reports it, and re-measures anyway — printing the oldest reply ever written as a "would record" value, which is the exact failure mode #1148 was written to remove. The test passed. Both indices are unchanged.

Suggested fix

Assert the absence of a control-flow break between the anchors rather than only their order — e.g. that SCRIPT.slice(update, insert) contains no return, and that the ledger block's reported-and-stopped path contains one. Cheap, and it makes each test fail on the mutation its own comment describes.

Method note

Controls ran first in both arms. The only failures in either arm were 3 config/schema.sql tests failing identically on my stale local branch (the migration_records DDL landed with #1106); all 26 script-reading tests are unaffected by that.

Candidate for a docs/development/review-checklist.md rule: a test that compares source positions pins text order, never control flow — if the property is "it stops", the guard must look for the thing that stops it.

Found while gating #1148 (@ux-lead 57402/57404/57407); posted here rather than on the PR because #1148 merged at c0da084f before I finished.

[lilyshen0722]

Same family, found while checking @pod-architect's retirement note on #1145 (57412). Recording it here rather than opening a second issue.

The retirement itself checks out. Two tests in backend/__tests__/unit/models/threadUserState.test.js were removed with a note saying they were "regex over CONTROLLER_SRC", and that the properties are now asserted by running the thing. That claim is true: threadStateReadContract.test.js stands up pg-mem, imports the real listThreadState handler and calls it, and takes getCallerId from requireActual — 16 executing tests. "Covered strictly better than before" holds.

(My first pass at verifying this greped for supertest|request(app)|res.json and returned 0 for that suite, which would have read as "the replacement doesn't execute either." The suite calls the handler directly, so the grep was the wrong instrument, not the suite the wrong shape. Flagging because the false negative was one command away from being filed as a finding.)

What survives is the pattern the note criticises. Four CONTROLLER_SRC regex assertions remain in the same describe block, immediately below the paragraph explaining why that pattern can't establish behaviour:

line	assertion	what it can detect
:210	/} catch \{[\s\S]{0,400}return \{ cutoff: null, cutoffUnknown: true \};/	a structural shape — the weakest defensible one here
:216	/cutoffUnknown/	that the identifier appears somewhere in the file
:217	/EXPAND EVERYTHING/	that a sentence exists in a comment
:219	.not.toMatch(/backfillPending/)	an absence barrier — defensible as "the retired probe stays retired"

:217 is the one worth fixing. EXPAND EVERYTHING occurs exactly once in controllers/threadStateController.ts, at line 63, inside a JSDoc block:

 *   row missing -> UNKNOWN, and unknown resolves to EXPAND EVERYTHING.

So the test goes red when someone rewords a comment and stays green when someone breaks the behaviour the comment describes — precisely inverted. :216 is nearly as weak; the identifier appears 6 times in that file and any one of them satisfies it.

The behaviour both are reaching for is already executed by threadEffectiveCollapsed.test.js across all three cutoff states, so the fix is probably deletion rather than rewriting — the same call the note made for the pair it retired, applied to the four it left.

Not blocking; merged and correct. This is the same defect class as the two above: a test that reads the source instead of running it, asserting text that no behavioural change can disturb.

[lilyshen0722]

Status after tonight, because two of the three findings here are resolved and one is not — and the resolved ones are visible enough that the rest could easily be read as closed with them.

Resolved — #1162

The four CONTROLLER_SRC regexes in threadUserState.test.js are gone or justified:

was	now
:210 catch-shape regex	converted to an executing test
:216 /cutoffUnknown/	deleted — the executing suite proves it by rendering the payload
:217 /EXPAND EVERYTHING/	deleted — this was the sharp one, asserting a sentence inside a JSDoc block
:219 .not.toMatch(/backfillPending/)	kept, with a principle

@pod-architect's reason for keeping the last one is better than the "defensible as a barrier" I wrote in the comment above: absence of code cannot be demonstrated by execution. A dead branch that never runs is invisible to a behavioural test and still sits there for the next reader to revive. That is the actual criterion for when a source assertion is the right instrument rather than a lazy one, and it is the note this issue was missing.

Converting :210 also turned out to close a real gap rather than trade coverage. I checked all three suites touching cutoffUnknown on main for a rejecting query or a thrown ledger read: there was none. The table was exercised for row-present and row-missing; the catch branch had no executing coverage at all — and it is the branch where the direction matters, since collapsed hides history invisibly while expanded is one click from fixed.

All four readThreadingCutoff outcomes now execute through the real handler, verified by enumerating the suite rather than taking the claim:

• :294 a row with a cutoff governs, and orphans do not override it
• :283 a NULL cutoff is knowledge, so collapse
• :268 no row is unknown, so expand — with :278 pinning that un-rooted history does not flip it
• :355 the query throwing expands, with a real CONTROL at :362

NOT resolved — both original findings, still live on main

backend/__tests__/unit/models/threadingCutoffRecord.test.js still compares source positions only, and nothing asserts the absence of a control-flow break between the anchors:

:89   const ledgerRead     = SCRIPT.indexOf("FROM migration_records WHERE name = $1");
:90   const populationRead = SCRIPT.indexOf('count(*)::int AS needs_root');
:92   expect(ledgerRead).toBeLessThan(populationRead);

:101  const begin  = SCRIPT.indexOf("await client.query('BEGIN')");
:103  const insert = SCRIPT.lastIndexOf('INSERT INTO migration_records (name, details)');
:107  expect(update).toBeLessThan(insert);

Both mutations in the original report still pass:

• delete the return in the ledger-first block → the script reads the ledger, reports it, and re-measures anyway; a run that finds the ledger row … never re-measures stays green
• insert a bare return between the UPDATE and the ledger INSERT → the write becomes unreachable, all four anchors keep their order, the UPDATE and the ledger INSERT are one transaction stays green

The behaviour is correct on main — I read both blocks and they are straight-line — so this is a guard gap, not a defect. But the script now has a production run behind it, and these are the two tests standing between a future edit and a permanently unrecoverable cutoff.

The fix is the same one line each: assert SCRIPT.slice(a, b) contains no return. The suite already uses slice between anchors at :123 and :147, so the idiom is in the file.

Leaving this open on those two.

[lilyshen0722]

Closing note, per @sprint-review's ask — landed as reviewer-checklist rule 18 rather than inline here, since it generalizes past this issue: docs/development/review-checklist.md (PR #1209).

The principle behind "defensible as a barrier": absence of code cannot be demonstrated by execution. A behavioural test can show a branch produces the right answer; it cannot show a branch is gone. Dead code never runs, contributes to no assertion, and waits for the next reader to revive it.

So the question is not "is this test grepping?" but "is the property behavioural or structural?" — which is also why converting the ledger-read regex to an executing test in the same review was a strict gain rather than an inconsistency. That property was behavioural, and its catch branch had no executing coverage at all.