Repository navigation
Backfill cutoff tests pin text order, not control flow — both survive their own named regression #1149
Description
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.
Activity
lilyshen0722 commentedon Aug 22, 2026
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.jswere removed with a note saying they were "regex overCONTROLLER_SRC", and that the properties are now asserted by running the thing. That claim is true:threadStateReadContract.test.jsstands up pg-mem, imports the reallistThreadStatehandler and calls it, and takesgetCallerIdfromrequireActual— 16 executing tests. "Covered strictly better than before" holds.(My first pass at verifying this greped for
supertest|request(app)|res.jsonand 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_SRCregex assertions remain in the samedescribeblock, immediately below the paragraph explaining why that pattern can't establish behaviour:/} catch \{[\s\S]{0,400}return \{ cutoff: null, cutoffUnknown: true \};//cutoffUnknown//EXPAND EVERYTHING/.not.toMatch(/backfillPending/):217is the one worth fixing.EXPAND EVERYTHINGoccurs exactly once incontrollers/threadStateController.ts, at line 63, inside a JSDoc block:So the test goes red when someone rewords a comment and stays green when someone breaks the behaviour the comment describes — precisely inverted.
:216is 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.jsacross 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 commentedon Aug 23, 2026
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_SRCregexes inthreadUserState.test.jsare gone or justified::210catch-shape regex:216/cutoffUnknown/:217/EXPAND EVERYTHING/:219.not.toMatch(/backfillPending/)@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
:210also turned out to close a real gap rather than trade coverage. I checked all three suites touchingcutoffUnknownon main for a rejecting query or a thrown ledger read: there was none. The table was exercised for row-present and row-missing; thecatchbranch 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
readThreadingCutoffoutcomes now execute through the real handler, verified by enumerating the suite rather than taking the claim::294a row with a cutoff governs, and orphans do not override it:283a NULL cutoff is knowledge, so collapse:268no row is unknown, so expand — with:278pinning that un-rooted history does not flip it:355the query throwing expands, with a real CONTROL at:362NOT resolved — both original findings, still live on
mainbackend/__tests__/unit/models/threadingCutoffRecord.test.jsstill compares source positions only, and nothing asserts the absence of a control-flow break between the anchors:Both mutations in the original report still pass:
returnin the ledger-first block → the script reads the ledger, reports it, and re-measures anyway;a run that finds the ledger row … never re-measuresstays greenreturnbetween the UPDATE and the ledger INSERT → the write becomes unreachable, all four anchors keep their order,the UPDATE and the ledger INSERT are one transactionstays greenThe 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 noreturn. The suite already usesslicebetween anchors at:123and:147, so the idiom is in the file.Leaving this open on those two.
lilyshen0722 commentedon Aug 25, 2026
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
catchbranch had no executing coverage at all.docs(review): rule 18 — absence of code cannot be demonstrated by exe…
docs(review): rule 18 — absence of code cannot be demonstrated by exe…
docs(checklist): rule 22 — a join across two measurements needs a tim…
test(backfill): the two cutoff guards pin reachability, not text order (