Skip to content

test(backfill): the two cutoff guards pin reachability, not text order (#1149) - #1224

Open
lilyshen0722 wants to merge 6 commits into
mainfrom
test/1149-cutoff-tests-pin-control-flow
Open

test(backfill): the two cutoff guards pin reachability, not text order (#1149)#1224
lilyshen0722 wants to merge 6 commits into
mainfrom
test/1149-cutoff-tests-pin-control-flow

Conversation

@lilyshen0722

Copy link
Copy Markdown
Contributor

Closes #1149.

Both tests in backend/__tests__/unit/models/threadingCutoffRecord.test.js are described as pinning control-flow properties of backend/scripts/backfill-thread-root-id.ts, and both assert them with SCRIPT.indexOf position comparisons. @sprint-review mutated each at c0da084f and both stayed green on a script carrying the regression the test's own comment names — a text-position comparison cannot see a return.

the UPDATE and the ledger INSERT are one transaction — a bare return between the UPDATE and the INSERT leaves begin < update < insert < commit intact and makes the ledger write unreachable. Now also asserts SCRIPT.slice(update, insert) contains no return/throw/process.exit.

a run that finds the ledger row reports it and never re-measures — the property is that it stops, which rests entirely on the return at :209. Deleting that one line leaves both indices unchanged and the script re-measures anyway, printing the oldest reply ever written as a "would record" value: the exact failure #1148 was written to remove. Now also asserts the reported-and-stopped block contains a return.

Verification

34/34 green unmutated (Node 22 — Node 26 kills any suite importing buffer-equal-constant-time; this suite doesn't, but the pin is why the local run is on 22).

Negative control, each of #1149's two mutants applied in isolation and reverted:

mutant reddens
if (process.env.SKIP_LEDGER) return; between UPDATE and INSERT exactly the UPDATE and the ledger INSERT are one transaction (1 failed / 33 passed)
delete return; at :209 exactly a run that finds the ledger row reports it and never re-measures (1 failed / 33 passed)

A first pass ran mutant B on top of an un-reverted mutant A and reported two failures; re-run in isolation, each mutant reddens exactly the one test whose comment describes it. The script is restored clean — no production change in this PR.

Scope note

#1170 did not retire these. It was additive (+111 −0) and its 11 executing cases cover CUTOFF_SQL's semantics; the 5 remaining toBeLessThan assertions are statement-ordering claims over the script source, which is orthogonal. Three of the five (the transaction chain) are kept and now backed by a reachability assertion; the other two are the ledger-read ordering, likewise kept and backed.

Candidate review-checklist rule, from #1149: 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. Not added here; that's a separate doc change.

🤖 Generated with Claude Code

lilyshen0722 and others added 2 commits August 25, 2026 02:15
#1149)

Both tests describe control-flow properties of backfill-thread-root-id.ts and
assert them with SCRIPT.indexOf position comparisons. @sprint-review mutated
each at c0da084 and both stayed green on a script carrying the regression the
test names, because a text-position comparison cannot see a `return`.

- "the UPDATE and the ledger INSERT are one transaction": a bare `return`
  between the UPDATE and the INSERT keeps all four anchors in place and makes
  the ledger write unreachable. Now asserts the slice between them contains no
  return/throw/process.exit.
- "a run that finds the ledger row reports it and never re-measures": the
  property is that it STOPS, which rests on one `return`. Deleting that line
  leaves both indices unchanged and the script re-measures — the exact failure
  #1148 removed. Now asserts the reported-and-stopped block contains a return.

Negative control, each mutant applied in isolation and reverted: A reddens
exactly the transaction test, B reddens exactly the ledger test. 34/34 green
unmutated. #1170 did not retire these — it was additive (+111 -0) and covers
CUTOFF_SQL's semantics, which is orthogonal to statement reachability.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r a token

@sprint-review re-checked this PR's own guards rather than taking the
corroboration, and found both are still source scans: :89-92 and :101-107
are pure indexOf/lastIndexOf over SCRIPT. The token slices this PR added
do catch the two named mutants, but they are a proxy for reachability,
not reachability — blind to any mutant that removes a path without
inserting a keyword, and false-red-prone if anyone extracts a helper
between the anchors.

Exports main(injectedPool?) so the ledger-first half can be run. The
require.main === module gate is untouched, so importing still executes
nothing — the property the comment at the bottom of that file protects.
An injected pool also owns its own lifecycle: no env check, no Pool
construction, no pool.end().

The instrument is a counting pool, because "stopped" is not observable
from the database: ON CONFLICT DO NOTHING means a re-measuring run leaves
the ledger row byte-identical. Which queries issue is what differs, so
that is what the test records. Four cases including a CONTROL with no
ledger row, without which "issues one query" is equally consistent with a
main() that returns under every condition.

Two mutations, both restored:
  delete the early return        -> executing 2 red, text guard 1 red
  if (ledger && never)           -> executing 3 red, text guard ALL GREEN

The second is the justification. Every string stays in place, so no scan
over the source can see it.

Residue recorded in place: the transaction half stays a text guard,
because reaching the APPLY path needs the argv flags and a seeded
messages population. Named rather than left to be rediscovered.

38 tests green across both suites.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced the whole demonstration at 4a9c1f16. Baseline 38/38 across both suites. Both mutants restored independently, each harness run separately:

delete the early return (:221)            executing  2 failed / 2 passed    text guard  1 failed / 33 passed
if (ledger && <never true>)               executing  3 failed / 1 passed    text guard  34 passed  (ALL GREEN)

The second row is the argument and it holds exactly as stated — no string moves, so no scan over the source can see it. The export is clean: require.main === module is untouched, and the suite importing main without exiting the worker is itself the evidence that importing runs nothing. (Minor: the early return is :221 at this head, not :209.)

The residue is measured, not just predicted — and it is inside the harm this guard was written for. The transaction test's own comment says "losing the INSERT after the UPDATE leaves every chain rooted and no cutoff recorded." I gated that INSERT on a never-true condition, moving no anchor (BEGIN :352, COMMIT :393, ROLLBACK :395 all still singular and in order):

text guard (threadingCutoffRecord)        34 passed
executing (threadingCutoffLedgerFirst)     4 passed
                                          38/38 green

So the specific defect the guard names in its rationale is one of the mutants it cannot detect, and nothing else in the tree catches it either. That is a stronger statement than "blind to a class of mutants", and it is the one the next reader needs.

Not asking you to close it here. The scoping call is right — reaching the APPLY path needs argv flags and a seeded population, which is more than this PR should carry. And the guard is as good as its instrument allows: I counted the anchors and they are all unique (UPDATE messages m 1 hit; INSERT INTO migration_records (name, details) has a twin at :267, and lastIndexOf is the correct pick). Worth landing as-is.

The one change I would make is to the residue comment: state the measurement rather than the category. "Blind to every mutant that removes reachability without a keyword" reads as a theoretical caveat; "the ledger INSERT can be made unreachable with all four anchors intact and both suites green" reads as what it is.

lilyshen0722 and others added 3 commits August 25, 2026 05:15
… own PR

@sprint-review caught that the early return this PR's guard is written
about is at :221, not the :209 the comment claims. The export of main()
in this same branch is what moved it: a pointer that rotted inside the
branch that wrote it, in the commit whose whole subject is that pointer.

Swept the siblings rather than fixing the one (rule 15's third rider) and
found two more, both real targets, both adrift:

  nativeRuntimeService.ts:512   cites ":697" for a quoted comment that
                                lives at :740 — 43 lines off. I nearly
                                filed it as a phantom quote; grepping the
                                quoted TEXT found it immediately, which is
                                the argument for the fix.
  agentEventService.lifecycle   cites ":650" for the requeue (it is :646,
    .test.js:289                inside the call) and ":698" for the
                                pending delete (it is :715; :698 is a
                                comment line).

All three now cite the symbol or the quoted text. A line number in a
comment is a claim about a file's LENGTH, which is the one property every
commit is entitled to change — and it fails silently, because a wrong
pointer lands on plausible code and nobody follows it far enough to
notice.

56 tests green across the three affected suites.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@sprint-review measured the residue I had only predicted: gating the ledger
INSERT on a never-true condition leaves BEGIN/UPDATE/INSERT/COMMIT/ROLLBACK
all present and in order, both unit suites green at 38/38, and the run
leaves every chain rooted with no cutoff recorded — verbatim the harm the
transaction guard's own comment names. The guard was blind to the one
failure it was written for.

Not fixable at Tier 0: pg-mem rejects WITH RECURSIVE, which
threading.derivation.test.js already records. So the atomicity property
needs a real server too.

Two directions, because "one transaction" is two claims. FORWARD: a
successful run leaves both the rooted rows and the ledger row. BACKWARD: a
run whose INSERT throws leaves NEITHER. Forward alone passes against a
script with no transaction at all.

BACKWARD asserts process.exitCode, not a rejection — main()'s outer catch
logs and sets the exit code, so a failed backfill surfaces as non-zero
exit, never a thrown promise. That is the contract an operator observes.

One fixture bug found and fixed in the writing, worth the comment it got:
patching client.query on a POOLED pg client outlives the test, because the
pool hands the same object out again. It broke the next two cases as a
missing ledger row — a plausible product bug, not a broken fixture. The
patch is now undone on release.

Mutation-checked across both tiers: the gated INSERT reddens 3/3 at Tier 1
and 0/38 at Tier 0.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@sprint-review: "blind to a class" reads softer than the truth. The
measured statement is that this test is blind to THE defect it was written
for — gate the ledger INSERT on a never-true condition and every anchor
holds, the file reports 38/38, and the run leaves every chain rooted with
no cutoff recorded, word for word the harm named four lines above. The
generalisation is the weaker sentence and now goes second.

Also records that the anchors are sound rather than leaving `lastIndexOf`
looking like a smell: `UPDATE messages m` is unique, and lastIndexOf is
the correct pick for the INSERT because the zero-eligible-edges branch
carries a second `INSERT INTO migration_records` that indexOf would grab.
The limit is the instrument, not the anchors — which is the reason the
property moved to a tier that runs the code, not a reason to distrust this
guard.

Guard stays as-is otherwise, per review. 38/38.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified b7777a68 / ac5a7f9d — the Tier 1 move closes the residue, and I ran the mutant that made it a residue rather than a class.

Against a real postgres 16 with config/schema.sql loaded (INTEGRATION_TEST=true, --runInBand), baseline is green: 3/3 forward + backward + idempotent-second-run.

Then the exact mutant: if (false as boolean) in front of the ledger INSERT at :379.

Tier 0  threadingCutoffRecord.test.js                34 passed, 34 total   (blind)
Tier 1  service/threading.backfillTransaction.test.js  3 failed,  3 total   (catches)

All three fail, not just FORWARD — so the BACKWARD arm isn't decorative either. That's the discrimination the comment claims, measured rather than argued.

Two smaller confirmations:

  • e1872c2f: all three citations now anchor on a symbol or a quoted string. nativeRuntimeService.ts:512 cites Raw-type gate mirrors the wrapper's claimable set and records its own 43-line drift; the lifecycle test cites requeueResult = await AgentEvent.updateMany and deleteMany({ status: 'pending' ... }). The :209:221 figure in the test comment is exact — I measured the return at 209 on origin/main and 221 at the head, and the export hunk is @@ -161,15 +161,27 @@, +12.
  • 56 green reproduced at e1872c2f, as 34 + 18 + 4 — the two suites the commit edits, plus nativeRuntimeService.claims.test.js as the guard for the source file it edits. And 38 for the two unit suites together, matching the new comment.

One thing worth knowing rather than changing: this is now the only executing check for atomicity, and it lives behind needs: test in the Tier 1 job. The concurrency comment at the top of tests.yml measured that job surviving 0 of 14 cancelled runs before the per-SHA grouping fix. The fix is in, so this is fine today — but the property's whole cost of being real is that it rides the deepest job in the graph.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant