Skip to content

fix(vacuity): pair moved code git doesn't pair, only ever taking measure away (#1014) - #1068

Open
jayden1711 wants to merge 13 commits into
mainfrom
issue-1014-vacuity-moved-code
Open

jayden1711 wants to merge 13 commits into
mainfrom
issue-1014-vacuity-moved-code

Conversation

@jayden1711

@jayden1711 jayden1711 commented Oct 24, 2026 •

Copy link
Copy Markdown
Owner

What and why

Closes #1014. Code a PR moved in a way git doesn't pair as a rename or move read as added code, so a test that ran the moved code passed the vacuity gate for the whole PR, even when the real change beside it was untested (R1, R3, R4, M3, M4b, M5b, M5c, R3s, and a tested function moved into an existing app file).

The reader now measures each changed app file a second way (movedCode in src/vacuity.ts):

  • Every top-level statement (a function, a class, a declaration) whose code lines are all added is paired with one whose code lines are all removed, from any app file at the base. A deleted file counts: merge-flow sends its removed line spans as elsewhere.
  • It pairs by identical code first, then by declared name, and measures the statement against its source. In that measure, parentheses, commas and semicolons are no change, which covers formatter re-wraps.
  • A file counts only where it counts both with and without the pairing. So pairing only ever takes measure away: the gate can't pass a PR it held before (inferred: the only new pass condition is the added && in measureTouched).

The two older tests that broke round 13's unfinished attempt ("a moved line, inside a branch the test never takes" and its control) were understood first. That attempt read two lines swapped inside a function as a move and left nothing to measure. A reorder is a real change, so here a statement moved inside one that stays is in no moved unit and is measured as before. Both tests pass (measured).

Behaviour change: a PR that only moves code (unpaired) now holds with "ran none" instead of merging when a test runs the moved code, as a rename git pairs already holds (inferred).

How it was verified

  • Fail-before (measured): source reverted (git checkout of the three source files, fix kept as a patch), rebuilt, node --import ./dist/test/isolate.js --test dist/test/merge-vacuity-r12.test.js dist/test/merge-vacuity-r13.test.js: 39 tests, 29 pass, 10 fail. Each fails with actual [true, []]: R1, R3, R4, M3, M4b, M4b1, M5b, M5c, A6, R3s.
  • Pass-after (measured): r12, r13 and r9-precision 59/59 pass. Every merge-vacuity-*, vacuity and merge-policy-vacuity test: 280 tests, 271 pass, 0 fail, 9 todo (the existing V8-limit todos, unchanged).
  • Must-holds kept (measured): M1, M1b, M1c, D1, D2, C1, Q1a, Q2, I1, I1q and N1 hold. Every control merges.
  • Checks (measured): npm run typecheck clean. npm run check:denylist 1645 files clean. node scripts/push-gate.mjs --range origin/main..HEAD clean. CI-parity lints (change-files-renames, merge-policy, changelog, owner-terminal-removed, changelog-fragments, brand, boundaries, engine-guard-locks, docs-guardrails, merge-policy-weakening and others) 193/193 pass.
  • Tests: the seven todo limits in merge-vacuity-r12/-r13 become must-holds. M4b1 (a move past one 6-line function), M5c and A6 are added, each with a merging control.

This is the merge policy, so it is in the critical set: it needs independent and adversarial head-pinned reviews.

What remains

Checklist

  • Commits signed off
  • Tests fail without the fix and pass with it
  • CI green on this head
  • Independent and adversarial final reviews posted on this head

Adversarial review fixes (head 01163c9)

R18b's findings, as one new commit on top of 18b48ab:

  1. A division inside brackets no longer turns pairing off for the rest of a file. codeLines' top came from both lexer readings; the every-/-is-a-regex reading swallows the ) of Math.round(u / 2), so no later line started at the top level. top now comes from the first reading when it ends cleanly. P1a (division in the file g left) and P1b (in the file g moved to) are must-holds: both were auto: true at 18b48ab (measured); the u * 2 control holds.
  2. A moved unit that can't be placed holds the PR. movedCode now throws UNPLACEABLE, as an unplaceable hunk does, instead of measuring the file as added code. P2 (a 300-line g moved and rewritten untested into 280 lines, beside a tested unchanged h) is a must-hold: auto: true at 18b48ab (measured); its small-rewrite control holds as "ran none".
  3. Both halves of the both-ways rule now have dedicated tests (measured, mutants applied to the compiled reader, r11-align and r13 run):
    • removing the app-code check's unpaired half fails P3m (and L1);
    • removing the dist/ check's unpaired half fails P3d;
    • removing its paired half fails P5.

Correction: the earlier "What remains" bullet "No test fails if the both-ways rule is removed" was wrong. The existing L1 control in merge-vacuity-r11-align already caught the non-dist/ mutant (measured), and P3m now targets it directly.

Verified at 01163c9 (measured): all vacuity tests 288, 279 pass, 0 fail, 9 todo (existing V8 limits); the CI-parity set with merge-policy-weakening 193/193; typecheck clean; check:denylist 1645 files clean; push-gate --range origin/main..HEAD clean (2 commits).

Still open, not blocking (from R18b):

Round 3 fixes (head e19bdc3)

Both reviews of 01163c9 failed it. Each claim is labelled: measured (from a command or CI output, named) or inferred (reasoned, not run).

What changed

  • A unit paired by name that can't be placed is measured as added code, and the file's other pairings stay (R18c). Before, a tested move with a small rewrite held the PR with UNPLACEABLE.
    • A unit with its source's code is never aligned at all, because nothing in it changed. So the UNPLACEABLE throw from movedCode is gone.
    • FH1 (a 150-line function moved with two lines replaced) and FH2 (a different same-named function replacing a deleted file) must merge, and both held at 01163c9 (measured, r13).
    • P2 still holds, now as "ran none" (measured).
    • P2x pins the no-align rule: a large tested move re-wrapped with comments, beside an untested edit, holds. Aligning identical-code pairs would merge it (measured, mutant R11).
  • A top-level statement runs to its end (E1, E1b, E1f, E1s). It runs across a line wrapped after an operator such as =>, + or =. A new statement starts only after ; or }, or on a declaration line.
  • A closing line git keeps between a moved unit and the code written in its place doesn't stop the pairing, at either end (E2, E2d).
  • More declared names (E3, E3m, E3a, E3n). Names now also come from exports.x =, module.exports.x =, and TypeScript's abstract class, enum and namespace. Each form is pinned end to end except enum, which the gate can't measure differently: an enum's code runs only as the module loads, so a probe holds both at main and at this head (measured).
  • Where statements start comes from their own lexer reading (E4/BP1, E4b, E5, E7).
    • That reading takes a / after an if/for/while/with condition for a regular expression, and one after a postfix ++/-- for a division.
    • If its brackets don't balance, the every-/-a-regex reading is used.
    • codeLines' mask, which U measures, is unchanged.
    • E6 stays a holding control.

Limits, all reading as added code as on main

  • A file whose brackets no reading balances pairs nothing. It has a { todo } test, and that shape merges both at this head and at main's versions of the four files this PR changes (measured).
  • A unit paired by name but rewritten too far to align with its source (FH1 and FH2, measured).
  • Renamed or nested moves, as before (inferred).

These are named in docs/quickstart.md, the changelog fragment, and the headers of src/merge-policy-vacuity.ts and src/vacuity.ts.

Evidence

  • Fail-before at 01163c9 (measured): P2, FH1 and FH2 hold with UNPLACEABLE. E1, E1b, E1f, E2, E2d, E3, E3m, E3a, E3n, E4, E4b, E5 and E7 merge (auto: true). P2x holds with UNPLACEABLE. E1s passes there and guards the declaration rule (mutant R7).
  • Mutants of dist/src/vacuity.js (measured): all 16 are killed, one per new rule plus the earlier A, B and C.
  • Probes at this head (measured): R18d's 12, plus the in-place FH1, a one-line-edit FH1 and an enum probe: 15/15 behave as intended.
  • Tests (measured): all merge-vacuity-* plus vacuity and merge-policy-vacuity give 309 tests, 299 pass, 0 fail, 10 todo. The CI-parity set, including merge-policy-weakening, passes 193/193.
  • Other checks (measured): npm run typecheck and check:denylist are clean. push-gate --range origin/main..HEAD is clean.
  • Owner decision: this head needs a new weakening scan and decision. P2's expected reason changed from UNPLACEABLE to "ran none".

Round 4 fixes (head d76d96f)

False holds (measured: Z-F1, Z-F1b, Z-F6, Z-F7, Z-F9, Z-F10, Z-F11 and FH3 merge, as on main; the Z-F8 control merges).

  • A moved unit is never paired with the old text git already paired it with, so a statement edited in place is measured as git measured it.
  • In moved code only formatting is no change: whitespace, semicolons, a comma before a closing bracket, and parentheses that can't change meaning (a lone arrow parameter, or a whole value after =, =>, (, [, ,, ?, :, return, throw). Parentheses that change precedence count as code.

Statement starts (measured: X2, X2b, X3, Z-A1 to Z-A4, Z-A3c, Z-A4c, Z-B1, Z-B2 and E4c hold; E1f still holds; the Z-A1c and Z-B1c controls hold).

  • A column-0 export or static import line always starts a statement, at any bracket depth the lexer reports.
  • In a balanced reading, a line starting with export, const, let, var, enum, interface, declare, abstract, namespace or a static import also starts one, whatever the line before ends with.

Name steal (measured: Z-N1 to Z-N4 hold; the Z-N1z control holds).

  • Same-named pairs are taken best-aligned first, so a same-named unit never takes another's source by path order.
  • A source may keep its header line where git keeps it for the code rewritten in its place, provided at least one of its lines was removed (K-G: a unit kept whole inside new code is no source).

Probes (measured, both reviewers' files at head / e19bdc3 / main).

  • Every probe gives the intended result at head.
  • Must-merge probes Z-F1, Z-F1b, Z-F6, Z-F7, Z-F9 and Z-F10: pass / FAIL / pass.
  • Must-hold probes Z-A1, Z-A2, Z-A4, Z-B1, Z-B2, Z-N1, Z-N2 and Z-N3: pass / FAIL / FAIL.
  • X2, X2b, X3 and the ES-module TODO-limit: hold / merge / merge.
  • X1: merge at all three (a listed limit, the same as main).
  • No probe holds at head where main correctly merges.

Tests and mutants (measured).

  • New must-holds: E8, X2, X2b, X3, Z-A1 to Z-A4, Z-A3c, Z-A4c, E4c, Z-B1, Z-B2, Z-N1 to Z-N4, Q-S, Q-S2, Q-C and Q-P.
  • New must-merges: Z-F1, Z-F1b, Z-F6, Z-F7, Z-F9, Z-F10, Z-F11, FH3, K-G and O4.
  • At e19bdc3, 20 of them fail. At main, Z-A4c, E4c and the four Q tests also fail.
  • 22 of 23 mutants are killed: the git-pairing rule (both passes), every formatting rule, the anchor, the keyword rule, best-score pairing, the kept-header rule and its guard, O3, O4, the postfix exemption, and the earlier R3 and R6.
  • The survivor lets an indented export anchor a statement. It is harmless (inferred): such a line occurs only inside a TypeScript namespace, where anchoring only splits the block into its members, and the both-ways rule keeps it from merging anything main holds. All of r11-align, r12 and r13 pass under it (measured).

Limits (listed in quickstart, the changelog and the merge-policy-vacuity header).

  • A statement renamed as it moved, or moved inside one that stays (inferred).
  • A statement rewritten too far to align: in a 150-line statement two replaced lines are enough (X1, a todo; main merges it too; measured).
  • In a file no reading balances, statements that don't start on an export or import line (a todo; measured).

Checks (measured).

  • typecheck and denylist: clean.
  • Vacuity set: 335 pass, 0 fail, 11 todo.
  • CI-parity set plus the tests that exercise the weakening module: 406 pass, 0 fail, 10 todo.
  • node scripts/push-gate.mjs --range origin/main..HEAD: clean.
  • Correction: round 3 listed a merge-policy-weakening test file that doesn't exist.

Round 5 fixes (head 7f79694)

In-place false hold (measured: G1, G1b, D1 and D1d merge, as on main; the G1c and D1m controls merge).

  • A statement git paired in place with a removed statement of its own name (or two unnamed statements) now pairs with nothing. It is measured as git measured it, even when the same PR removes a copy of its new text elsewhere.
  • The same-name condition is required. Dropping it breaks M4b, M4b1 and E2d (measured by mutant).

Parentheses: the docs now say exactly what the code does (measured).

  • Parentheses count as formatting in two positions only:
    • around an arrow function's one parameter;
    • around one expression with no , or ; of its own that doesn't start with {, opened right after an assignment's =, =>, return, throw, (, [, ,, ? or :, and closed right before ;, ,, ), ], } or the end.
  • Every other parenthesis counts as code. This is now a listed limit: B8, B8m, B18, B19, B30, B17c and B7p merge at the head, as on main.
  • New pin tests: G2, G2e, G5 and B16b must merge; Q-Pq and Q-Pc must hold. Z-T3 pins the template guard on the column-0 export anchor.

Unbalanced-file limit (measured).

  • The r13 todo now imports its CommonJS module whole, so its own test passes and it reaches the measurement it names.
  • It merges at the head source, at d76d96f and at main. A balanced control holds.

Docs.

  • quickstart, the changelog fragment, the merge-policy-vacuity.ts header and the vacuity.ts header name the formatting positions and the parenthesis limit.
  • "Measured as git measured it" is now qualified with "under the same name".

Probes (measured, both reviewers' files at head and at main).

  • No probe that should merge holds at the head where main merges.
  • The head holds where main merges only for correct holds (B0, B5, B16, B18r, B6u, B6w, B7u, D3, C3, C5, GUc, G11) and for B2s and B22.
  • B2s and B22 are a whitespace change inside a string and a doubled ;. Both also count as no change in place on main, and the reviewer didn't require them.

Tests, fail-before and mutants (measured).

  • At d76d96f, G1, G1b, D1 and D1d fail. At main, Q-Pq, Q-Pc and Z-T3 fail.
  • All 7 new mutants are killed: the in-place exclusion removed, the in-place exclusion matching any name, the close check, the { guard, the ?? position, the : position, and the template guard.

Checks (measured).

  • typecheck and denylist: clean.
  • All merge-vacuity-*, vacuity and merge-policy-vacuity: 349 pass, 0 fail, 11 todo.
  • CI-parity set plus the tests that import merge-policy-weakening: 428 pass, 0 fail, 10 todo.
  • node scripts/push-gate.mjs --range origin/main..HEAD: clean.

Round 6 fixes (head 51538c8)

1. In-place skip narrowed (N5). A unit git pairs in a hunk with a removed statement of its own name (or, unnamed, an unnamed one) counts as edited in place only when the code characters both keep reach a similarity (2 × kept / both) of at least 0.8 (EDITED_IN_PLACE). Otherwise it took that statement's place and pairs like any moved unit. Removals git measured inside a placed unit are dropped with it (N6). Measured similarities: G1/D1/U1 0.95, Z-F6/F7/F10 ≥ 0.96, N5 0.67, N6 0.43.

  • Must hold: N5, N5x (control), N6. Must merge: G1, G1b, D1, D1d, U1. M4b, M4b1, E2d pass. (measured: merge-vacuity-r13, -r12)
  • Limits, each a limit: todo that fails on the verdict (measured), documented in the quickstart, the changelog fragment and both headers:
    • W1, W2: a statement moved within its file that git pairs with its own old text is measured as git measured it, so its re-formatting counts and they merge, as on main and d76d96f. Holding them conflicts with Z-F11, where a ; added in place must merge (inferred).
    • A statement rewritten in place past 0.8 to the exact text of a copy the PR removes elsewhere is read as moved from there and holds, as at d76d96f; main and 7f79694 merge it.

2. ?? precedence (PQ2, PQ3). Parentheses after ?? are formatting only when their content holds no ?, :, |, & or = outside brackets. The other position, the arrow function's one parameter, and the list of openers (=, =>, return, throw, (, [, ,, :) and closers (;, ,, ), ], }, end) are unchanged and stated exactly in the docs. PQ2 and PQ3 must merge; Q-Pq and Q-Pc still hold. (measured)

Fail-before (measured, src reverted through a patch): at 7f79694, N5, N6, PQ2 and PQ3 fail. At main e91629f, N5, N5x and N6 fail; PQ2 and PQ3 pass there because main pairs no moves. Everything passes at head.

Mutants (measured, r11-align + r13 + r12), all killed: loose never set → PQ2, PQ3; ?? ignores loose → PQ2, PQ3; edited() always true → N5, N6; edited() always false → G1, G1b, D1, D1d, U1; threshold 2/3 → N5; threshold 0.96 → G1, G1b, D1, D1d, U1; removals not dropped → N6.

Probes at head vs main (measured; R18g/R18h head verdicts identical to round-5 head):

  • Head holds, main merges: R18h B0, B5, B6u, B6w, B7u, B16, B18r, C3, C5, D3 (wanted hold); B2s, B22 (want: any); R18g G11, GUc (wanted hold); R18j N5, N5x, N6, W1x, W3 (wanted hold); PQ1, PQ1r, RA1 (wanted merge: follow-ups to be filed); F4 (info).
  • Head merges where a hold is wanted: R18h B7p, B7t, B8, B8m, B17c, B18, B19, B30 (main merges these too: existing limits); R18j W1, W2 (limits above).
  • R18h C1 (wanted merge) holds at head and at main.

Checks (measured): vacuity set 355 pass / 0 fail / 14 todo; CI-parity lints plus every test importing the weakening module (excluding desktop-removed) 449 / 0 / 10 todo; npm run typecheck clean; check:denylist 1645 files clean; push-gate --range origin/main..HEAD 6 commits, 1 tip clean.

Round 7 fixes (head ce7be5f)

1. edited() fails closed (R18k B3; R18l BUDf, BUD120, BUD160, BL1wb, BL2wb). Each same-named pair git puts in one hunk is now aligned on its own (alignChars, bounded by ALIGN_CELLS). The PR-wide nearCells budget is gone. A pair too large to align is not edited in place: it pairs like any moved unit, which can only take measure away.

  • Why per pair (inferred): codeRanges aligns every git hunk under the same per-call bound and no PR-wide budget, so this costs about what the module already spends per hunk. A PR-wide budget that failed closed would turn D1 into a false hold after any big in-place edit earlier in the PR. D1p pins that.
  • Must hold: BUDf, BUD120, B3, BL1wb. Controls (hold): BUDz, B3c. Must merge: D1p (D1 plus a 200-line in-place edit in a file that sorts first), BIGIN (a tested 110-line in-place edit).
  • Fail-before (measured, src at 51538c8 with head's tests): exactly BUDf, BUD120, B3 and BL1wb fail. G1, G1b, D1, D1d, U1, PQ2, PQ3, M4b, M4b1, E2d, the Z-F series, D1p and BIGIN pass there and at head.
  • Mutants (measured, r11-align + r12 + r13): too large to align counts as kept → BL1wb; 51538c8's budget and null rule → BUDf, BUD120, B3, BL1wb; PR-wide budget, fail-open → BUDf, BUD120, B3; PR-wide budget, fail-closed → D1p. All killed.

2. Docs. The quickstart, the changelog fragment and both headers state the rule: a same-named (or both-unnamed) git partner, a code-character similarity of at least 0.8, and a pair too large to align (about 2,000 code characters a side, each pair on its own) never counted as edited in place.

  • N5u is named as a limit, with a limit: todo that fails on the verdict (measured): git keeps the stale g's identical first line, so the moved g is never wholly added. It merges at head, 51538c8 and main. The changelog example now says "every code line of it replaced".
  • The rewrite limit's sentence now says it applies only when every code line of the statement is new; where git keeps a line, it merges as before.

3. R18l's inOne some-vs-every mutant is equivalent (inferred). ranSegments leaves out the module's top level, and ranOn counts a removal only where both sides ran. A removal straddling a moved unit's boundary has one side at a top-level statement start, so it can never count. Probes ST7 (a removal right before a moved g) and ST7c (the same in place) hold under both every and some (measured). No test added.

Probes (measured; R18g, R18h, R18j, R18k, R18l at head, 51538c8 and main cc34fd7):

  • Head vs 51538c8: BUD, BUD120, BUD160, BUDf, BL1wb, BL2wb and B3 (R18k) went from merge to hold. No other difference.
  • Head holds, main merges: R18h B0, B5, B6u, B6w, B7u, B16, B18r, C3, C5, D3 and R18g G11, GUc (wanted hold); R18h B2s, B22 (want: any); R18j/R18l N5, N5x, N6, W1x, W3, TN5, TN5b, BN5-mid, BL1w, BL1wb, BL2wb and the BUD series (wanted hold); R18k B0, B3, B3c, P1 (wanted hold); R18k L0 (the documented rewrite limit); PQ1, PQ1r, RA1 (follow-ups); F4 (info).
  • Head merges where a hold is wanted, as at 51538c8 and main: R18h B7p, B7t, B8, B8m, B17c, B18, B19, B30 (existing limits); W1, W2 (limits); N5u, plus BL1, BL1s, BL2, BL3, which are the N5u shape: stale g(u) with the same first line. These are follow-ups, not chased.
  • Head holds where a merge is wanted, as at 51538c8: R18h C1 (main holds too); PQ1, PQ1r, RA1; R18k L0.

Checks (measured): vacuity set 363 pass / 0 fail / 15 todo; CI-parity lints plus every test importing the weakening module (excluding desktop-removed) 449 / 0 / 10 todo; npm run typecheck clean; check:denylist 1645 files clean; push-gate --range origin/main..HEAD 7 commits, 1 tip clean.

Round 8 fixes (head 35d11f5)

1. By-name scoring fails closed (R18m ZN1spent; R18n ZN1B, ZN4B). The PR-wide MOVE_PAIR_CELLS budget is gone. Each same-named pair a unit or a source shares with another candidate is now scored on its own: by character (alignChars, bounded by ALIGN_CELLS), or, too large for that, by its lines' whitespace-free text. When even the lines are too many, the reader throws CoverageUnreadable and the PR waits. It never falls back to path order.

  • Why per pair (inferred): each call is bounded by ALIGN_CELLS, as edited() and codeRanges already are. Total work is backstopped by the 120 s reader timeout, which fails closed.
  • Must hold: ZN1spent (n=5), ZN1B, ZN4B, ZN1L (a 120-line g whose decoy is too large to align by character). Controls (hold, as at ce7be5f): ZN1Bs, ZN1Bz, M3B. Must merge: D1L (a ~1,300-character line edited in place, about 1.7M cells).
  • Fail-before (measured, src at ce7be5f with head's tests): exactly ZN1spent, ZN1B, ZN4B and ZN1L fail. The controls, D1L, G1, D1, U1, N5, the BUD series, B3, BL1wb, BIGIN, PQ2, PQ3, M4b, E2d and the Z-F series pass at both.
  • Mutants (measured): PR-wide budget, -Infinity once spent → ZN1spent, ZN1B, ZN4B; PR-wide budget, 0 once spent (path order) → the same three; no line fallback → ZN1L; a 1M-cell cap in edited() → D1L. All killed. Not killed: the line score also counting kept lines (S4), which is informational. The by-name-budget-removed mutant from R18m/R18n is moot because the budget is gone.
  • The throw path is not reached by any test (inferred: it needs two same-named statements of about 2,000 lines each).

2. PERF2 (measured, PERF2 probe on this machine). Contested pairs are now counted with a map, not named.filter per candidate. Q=150: 318 ms at head, 2,306 ms at ce7be5f, 273 ms at main. Q=399: 513 ms at head, 71,290 ms at ce7be5f, 273 ms at main. All merge, as wanted.

3. Docs. The quickstart's in-place sentence now says "a removed statement of the same name (or, unnamed, an unnamed one)". The quickstart, the changelog fragment and both headers say that each same-named pair is compared on its own, by character or by line, and that two too large to compare make the PR wait.

Probes (measured; R18m, R18n ZN set, R18g, R18h, R18j, R18k, R18l at head, ce7be5f and main cc34fd7):

  • Head vs ce7be5f: R18m ZN1spent n=5 and n=8, R18m ZN4spent n=5, R18n ZN1B and ZN4B went from merge to hold. No other difference: the R18g/R18h/R18k verdicts and the R18j/R18l output are identical.
  • Main merges every R18m and R18n ZN probe, including the ZN1Bs, ZN1Bz and M3B controls, which hold at head and ce7be5f.

Checks (measured): vacuity set 371 pass / 0 fail / 15 todo; CI-parity lints plus every test importing the weakening module (excluding desktop-removed) 253 / 0 outside the adversarial file, which hit the script's 300 s per-file limit under load; run alone, it passes 196 / 0 / 10 todo in 298 s at head (350 s at ce7be5f); npm run typecheck clean; check:denylist 1645 files clean; push-gate --range origin/main..HEAD 8 commits, 1 tip clean.

Round 9 fixes (head 08b5e15)

Both round-8 verdicts (R68o, R68p) failed 35d11f5 on the same cause: when a contested same-named pair was too large to align by character, closeness scored it on raw lines with only whitespace removed, so semicolons and comments decided which source a moved statement took.

What changed

  • eca4071: the by-line score reads code only. Each unit now keeps rows, its lines' code characters taken from the same mask as key. The line fallback aligns those, skipping empty rows, instead of lines.replace(/\s/g, ''). Every hunk, at every size, is also aligned code against code and comment against comment: the whole hunk, a run of unequal line counts, and each line pair. Before, only line pairs whose code matched were aligned this way.
  • 08b5e15: pins the line-pair half. CM5 (must hold) is CM3 with a comment on every line of g, the edited line's comment naming its new arm. CM5t (must merge) is CM5 with the arm the test takes edited too.
  • Round 8's "by its lines' whitespace-free text" no longer holds. The quickstart, the changelog fragment and both headers now say that the comparison reads only code.

Fail-before and pass-after (measured). I exported the tree twice under a private TMPDIR: head's tests with 35d11f5's src, and with head's src. I rebuilt each with tsc -p tsconfig.json and ran node --import ./dist/test/isolate.js --test --test-name-pattern=<cases> dist/test/merge-vacuity-r13.test.js.

  • With 35d11f5's src, these fail, each with actual [true, []]: SEMI, SEMI-z, SEMI-half, CMT, ZN1L-cmt, CM1, CM2, CM3, CM4 and CM5.
  • With 35d11f5's src, these pass: ZN1L, ZN4L, the SEMIc, SEMI-halfc and CMTc controls, and CM1t, CM3t, CM4t and CM5t.
  • At head, every one of these passes.

Mutants of compiled vacuity.js (measured: one line each, run against r11-align, r12 and r13, then restored and checked with cmp). All four are killed:

Mutant Killed by
line score on raw lines (35d11f5's) SEMI, SEMI-z, SEMI-half, CMT, ZN1L-cmt
whole hunk aligned untagged CM1, CM2, CM3, CM4
a run of unequal counts aligned untagged ZN1L-cmt
a line pair aligned untagged CM5 (it survived at eca4071, which led to 08b5e15)

Checks at 08b5e15 (measured)

  • Every vacuity test file (dist/test/*vacuity*.test.js, 23 files): 404 tests, 389 pass, 0 fail, 15 todo.
  • npm run typecheck: exit 0.
  • npm run check:denylist: 1647 files clean in the builder worktree. The script lists git ls-files -co, so this count includes the worktree's two untracked files; R68s measured 1645 in a clean checkout.
  • node scripts/push-gate.mjs --range origin/main..HEAD: 10 commits and 1 tip, clean.
  • npm test (run once under the shared lock, at eca4071, before CM5 existed): 5868 tests, 5756 pass, 18 fail, 69 skipped, 25 todo. The 18 failures are in add-project-api-key, coordinator (worktree dependency cache), desktop-removed (yaml resolving through the symlinked node_modules) and machine-sudoers-closed, -narrow and -removal. None is a vacuity test.
  • I reran those six files on exports of 35d11f5's and head's src: the same tests fail in both (19 each; the extra one is a git-checkout test that the export can't satisfy). So this change doesn't cause them (measured). They are environmental on this host (inferred). CI is the authoritative run.

Not re-run: R68o's surviving mutant (a line score that counts only the source side), the throw path, the PERF2 timings, the COST probe, and R68p's probes beyond the five pinned as tests.

Still open (inferred, unchanged from R68p): there is no PR-wide cost cap, and the 120 s reader timeout backstops it, failing closed (#1016's subject). An exact untested copy of g still takes the source by code before naming is tried.

This is the merge policy, in the critical set: it needs independent and adversarial head-pinned reviews of 08b5e15.

Round 10 fixes (head 4869afd)

R68q (final check) passed 08b5e15. R68r (adversarial) failed it on an overclaim. A contested same-named pair too large to align by character was scored by its line rows. So a moved g with half or more of its lines re-wrapped after = counted every wrapped line as changed, and a two-line decoy g took its source.

What changed

  • The fallback scores tokens. closeness now aligns the tokens of key (word runs and punctuation runs, /[\w$]+|[^\w$]+/g), and each unkept token weighs its characters. key holds no whitespace, so a line break never changes these tokens. The unused rows are gone.
    • I first tried single-character punctuation tokens. That pushed ZN1B, ZN1Bz, M3B and ZN4B (one-line statements of about 4,500 characters) past ALIGN_CELLS, so the reader threw and they failed (measured). Punctuation runs keep them aligned.
  • New must-holds: WRAP-1of2, WRAP-2of3 and WRAP-3of4, each with the decoy in b.js and in z.js. The controls WRAP-1of2c, WRAP-2of3c and WRAP-3of4c have no decoy and hold.
  • Mutant pins: ZN4L-S, ZN4L-U and ZN4L-N. Each adds a second removed g whose parameter is renamed at both ends, so it is scored by token. They pass at 08b5e15 too: they exist to catch the mutants below.
  • Docs: the quickstart and the changelog say the comparison is "by code token" and that "formatting, comments and line breaks never decide which source it takes". The too-far-to-align limit now names a 120-line statement re-wrapped on every line, or with pairs of its lines joined. Two statements too large to compare by token (more than about 2,000 unshared code tokens a side) make the PR wait. The 2,000 figure is inferred from ALIGN_CELLS = 4M, not probed.

Fail-before and pass-after (measured). I exported 08b5e15 under a private TMPDIR, copied in the new r13 test file, built it with tsc -p tsconfig.json, and ran node --import ./dist/test/isolate.js --test --test-name-pattern='WRAP|ZN4L' dist/test/merge-vacuity-r13.test.js.

  • At 08b5e15, all six WRAP must-holds fail, each with actual [true, []]. The three controls and the four ZN4L tests pass.
  • At head, all 13 pass.

Mutants of compiled vacuity.js (measured: one line each, run against r11-align, r12 and r13, then restored and checked with cmp)

Mutant Killed by
only the source's unkept tokens count (R68o's one-sided score) ZN4L-S
only the moved statement's unkept tokens count ZN4L-U, ZN4L-N
every token weighs 1 ZN4L-N (on the path-order tie: its source is in src/0.js)
one token per character 20 tests (R68s's count): ZN1B, ZN1Bz, M3B, ZN4B, ZN1L, SEMI, SEMI-z, SEMI-half, CMT, ZN1L-cmt, ZN4L, ZN4L-S, ZN4L-U, ZN4L-N and the six WRAP must-holds. My first list left out ZN4L-S, -U and -N.

R68r's two other survivors, keeping empty rows and weighting by line count, no longer apply: rows are gone (inferred, from the code).

R68r's probe file at head, 08b5e15 and main 028013d (measured, 43 probes).

  • Head and 08b5e15 differ only on WRAP-half, WRAP-1of2, WRAP-2of3, WRAP-2of3z and WRAP-3of4. Each went from merge to hold.
  • WRAP, WRAP-z, WRAPc, JOIN and JOINc merge at all three trees. They fall under the limit the docs now name.
  • O6 holds at all three trees. The probe labels it "merge", but its edited arm (? 2) is never taken with f(1), so the hold looks correct (inferred).
  • Every other probe matches what it wants at head.

Cost (measured)

  • node /var/tmp/wl-coord/r68r-perf2.mjs <tree> 100 (100 near-limit hunks, 3 runs each):
    • head: 691, 687 and 681 ms;
    • 08b5e15: 663, 691 and 633 ms;
    • main: 348, 379 and 383 ms.
    • Peak RSS was 113 to 131 MB on all three.
    • That script measures codeRanges, which this round doesn't change.
  • The 43 decisions in R68r's probe file took 13.7 s at head, 14.4 s at 08b5e15 and 14.1 s at main.

Checks at 4869afd (measured)

  • Every vacuity file (dist/test/*vacuity*.test.js): 416 tests, 401 pass, 0 fail, 15 todo.
  • boundaries, brand, changelog, changelog-fragments (with its self-check), docs-guardrails and merge-policy: 111 of 111 pass.
  • npm run typecheck: exit 0.
  • npm run check:denylist: 1647 files clean in the builder worktree, which includes two untracked files (the script lists git ls-files -co); R68s measured 1645 in a clean checkout.
  • node scripts/push-gate.mjs --range origin/main..HEAD: 11 commits and 1 tip, clean.
  • git merge-tree --write-tree origin/main HEAD: clean.
  • The full npm test was not re-run this round. CI is the authoritative run.

Behaviour change (inferred, not probed): before, a pair fell back from line rows to the throw only past about 2,000 lines a side. Now that happens past about 2,000 unshared tokens a side, which is some 165 lines of dense code. Two contested same-named statements that size now make the PR wait, where 08b5e15 scored them. This fails closed.

This is the merge policy, in the critical set: it needs independent and adversarial head-pinned reviews of 4869afd.

Round 11 fixes (head 29bb7f6)

R68s (final check) passed 4869afd. R68t (adversarial) failed it on an overclaim. In probe UB100, a 105-line g is moved with every word of 100 lines edited inside an if (u.never) block the test never enters. Scored by token, each edited name cost its full length on both sides. The moved g scored −3402 against a two-line decoy's −2818, so the decoy took the source and the PR merged.

What changed

  • Edited tokens cost only their edit. In closeness's token fallback, each run of tokens between kept ones is now aligned by character, and costs the characters it doesn't keep. A run too large to align by character costs its full length. So alpha edited to alphz costs 2, not 10.
  • New must-holds: UB100 (decoy in b.js) and UB100z (decoy in z.js).
    • UB100c (control, no decoy) holds.
    • UB100t (must merge) edits the arm the test takes too.
  • ZN1T pins the fail-closed path. A 204-line g is moved with its taken arm edited and tested, beside a two-line decoy. The pair is too large to tell apart by token, so the PR must wait with "two statements named g are too large to tell which one … was moved from".
  • ZN4L-P pins punctuation tokens. The second source is the moved g renamed at both ends, with every + turned into -.
  • ZN4L-S was rebuilt. Its second source now only drops lines (the first, the last and 10 more), so it has nothing unkept on the source side. With character-level runs, the old version no longer caught a source-side-only score.
  • Docs: the quickstart and the changelog say the token comparison costs "an edited token … only the characters it changes". The merge-policy header lists the new must-holds.

Fail-before and pass-after (measured). I exported 4869afd under a private TMPDIR, copied in the new r13 test file, built with tsc -p tsconfig.json, and ran node --import ./dist/test/isolate.js --test --test-name-pattern='UB100|ZN1T|ZN4L' dist/test/merge-vacuity-r13.test.js.

  • At 4869afd, UB100 and UB100z fail with actual [true, []]. UB100c, UB100t, ZN1T and every ZN4L test pass.
  • At head, all 10 pass (corrected in round 12: this said 11).

Mutants of compiled vacuity.js (measured: one line each, run against r11-align, r12 and r13, then restored and checked with cmp)

Mutant Killed by
the throw replaced by return -Infinity ZN1T
word-run tokens only ZN1T, ZN4L-P
a run of tokens costs its full length (4869afd's score) UB100, UB100z
a run costs only the moved side's unkept characters ZN4L-U, ZN4L-N
a run costs only the source side's unkept characters ZN4L-S
each run costs 1 ZN1L, ZN4L, ZN4L-N, ZN4L-P, ZN4L-S

Two mutants survive:

  • \w without $ in the token pattern: with character-level runs, a $ that splits a token is still aligned by character (inferred).
  • No \s filter in key: R68t inferred this is equivalent, because the code mask never marks whitespace (inferred).

R68r's and R68t's probe files at head, 4869afd and main 246e3f6 (measured, 95 probes, with R68T_R1=1).

  • Head and 4869afd differ only on UB100 and UB100z, which went from merge to hold.
  • At both trees, WRAP, WRAP-z, WRAPc, JOIN and JOINc merge (the named limit), and O6 holds (correct, as before).
  • Disclosed behaviour change (measured). These wait at both head and 4869afd with "too large to tell", where main merges them: ORD200 (two 200-line renders, each moved into another file with a tested edit), TC120t, and BND166 to BND180, both tested and untested. This is the fail-closed threshold that round 10 named: more than about 2,000 unshared code tokens a side. It turns ordinary large same-named moves with tested changes into PRs that wait for the owner. A row-score fallback before the throw would avoid it, but it isn't in this PR.

Cost (measured)

  • node /var/tmp/wl-coord/r68r-perf2.mjs <tree> 100, 3 runs each:
    • head: 816, 880 and 864 ms;
    • 4869afd: 847, 780 and 822 ms;
    • main: 496, 464 and 398 ms.
    • Peak RSS was 118 to 133 MB on all three.
  • The 95 probe decisions took 33.2 s at head, 32.2 s at 4869afd and 34.4 s at main.

Checks at 29bb7f6 (measured)

  • Every vacuity file (dist/test/*vacuity*.test.js): 422 tests, 407 pass, 0 fail, 15 todo.
  • boundaries, brand, changelog, changelog-fragments (with its self-check), docs-guardrails and merge-policy: 111 of 111 pass.
  • npm run typecheck: exit 0.
  • npm run check:denylist: 1647 files clean in the builder worktree, which includes two untracked files.
  • node scripts/push-gate.mjs --range origin/main..HEAD: 12 commits and 1 tip, clean.
  • git merge-tree --write-tree origin/main HEAD onto 246e3f6: clean.
  • The full npm test was not re-run this round. CI is the authoritative run.

Body corrections: the round 9 and round 10 denylist lines and round 10's "one token per character" row above are corrected in place, as R68s noted.

This is the merge policy, in the critical set: it needs independent and adversarial head-pinned reviews of 29bb7f6.

Round 12 fixes (head 1c87263)

R68u (final check) passed 29bb7f6. R68v (adversarial) failed it on an overclaim (R1). Once one run of edited tokens was past ALIGN_CELLS (about 2,000 code characters a side), it cost its full length. So a two-line decoy g took the source of a large moved g whose long expression was edited at every token, and the PR merged (BR220 to BR300, BRSIM300).

What changed

  • A run too large to align fails closed. In closeness's token fallback, a run of edited tokens that alignChars can't align now throws the same CoverageUnreadable as a too-large token alignment. The PR waits with "two statements named g are too large to tell which one … was moved from". A run is never weighed by its length any more. This was R68v's preferred fix; I didn't take the run-splitting alternative.
  • New tests in merge-vacuity-r13:
    • BR220 (decoy in b.js), BR220z (decoy in z.js), BR300, and BR220t (the taken arm edited and tested). Each must wait with the "too large to tell" reason.
    • BR220c is the no-decoy control. It holds on its untested code.
    • LNG100 (must hold) and LNG100t (must merge): a long name edited a little on each of 100 lines. The runs together are past ALIGN_CELLS, but each run is aligned on its own.
  • Docs: the quickstart and the changelog now say that one run of edited tokens too large to compare by character (more than about 2,000 code characters a side between two tokens the pair keeps) makes the PR wait, with the reason named, and that such a run is never weighed by its length. The merge-policy header lists the new must-holds.
  • Body correction: round 11's "all 11 pass" was a miscount. It is 10, now corrected in place.

Fail-before and pass-after (measured). In the builder worktree, with the new r13 tests and the unfixed src/vacuity.ts of 29bb7f6, I ran npx tsc -p . and then node --import ./dist/test/isolate.js --test --test-name-pattern='BR2|BR3|LNG100' dist/test/merge-vacuity-r13.test.js.

  • Unfixed: BR220, BR220z, BR300 and BR220t fail with expected: false, actual: true (they merge). BR220c, LNG100 and LNG100t pass.
  • At head: all 7 pass.

Mutants of compiled vacuity.js (measured: one change each, run against r11-align, r12 and r13 (194 tests), then restored and checked with cmp)

Mutant Killed by
a too-large run costs 0 (R68v survivor) BR220, BR220z, BR300, BR220t
a too-large run costs one side only (R68v survivor) BR220, BR220z, BR300, BR220t
a too-large run costs its full length (29bb7f6's score) BR220, BR220z, BR300, BR220t
no run() at a kept token (R68v survivor) ZN1B, ZN1Bz, M3B, ZN4B, ZN1L, LNG100, LNG100t
a one-sided run costs nothing ZN4L-U
runs never reset ZN1B, ZN1Bz, M3B, ZN4B, ZN1L, UB100, UB100z, LNG100, LNG100t
the last run isn't charged ZN4L
every run costs its full length UB100, UB100z, BR220, BR220z, BR300, BR220t, LNG100

The first run of the last mutant overlapped with a rebuild of dist, which overwrote the mutant, and reported a false "survives". I reran it on its own with nothing else touching dist; the row above is that rerun.

R68t's and R68v's probe files at head, 29bb7f6 and main 246e3f6 (measured). r68v-probe.test.ts holds r68t-probe.test.ts verbatim, plus the R68V probes, so I ran it once per tree, with pattern ^R68[RTV], R68T_R1=1, R68V_BR=150,200,210,220,230,260,300, R68V_ORD=60,195,200 and R68V_DD=220,250. That is 136 decisions, 134 distinct IDs, per tree.

  • Head and 29bb7f6 differ only on BR and BRSIM probes past the run limit. BR220, BR220z, BR230, BR230z, BR260, BR260z, BR300, BR300z and BRSIM300 went from merge to waiting with "too large to tell". So did the tested variants BR220t, BR230t, BR260t and BR300t.
  • BR150 to BR210, with the decoy in b.js or z.js, hold at head as before, and their tested variants BR150t to BR210t merge as before. Every BRnc control holds, and BRSIM150 holds.
  • No other probe changes:
    • ORD60 and ORD195 merge, and ORD200 waits.
    • DD220 merges, and DD250 waits.
    • LNG100, LNG200, LNG100c and LNG200c hold, LNG100t merges, and LNG200t waits.
    • TC120t and BND166 to BND180 (tested or not) wait.
    • WRAP, WRAP-z, WRAPc, JOIN and JOINc merge (the named limit), and O6 holds.
  • Main merges every probe listed above.
  • Behaviour change (measured, fail-closed): BR220t to BR300t wait where 29bb7f6 merged them. Each is a large statement whose whole edit is one run of more than about 2,000 characters a side, with a same-named contest in the same PR. They join ORD200, DD250, TC120t, BND166t to BND180t and LNG200t under the "too large to tell" wait that R68u and R68v ruled an accepted fail-closed limit.
  • Probe decision times (67 s at head, 47 s at 29bb7f6, 46 s at main) are not comparable. The head run overlapped with the full npm test.

Checks at 1c87263 (measured)

Not done (follow-ups)

  • Throw only when the unscorable pair can change the pick, so ORD200 (and BRnt) merge. It isn't small: the score of a too-large pair is only bounded, between −(|u|+|s|) and −abs(|u|−|s|), and in BR220 that range overlaps the decoy's score (inferred). Left as a follow-up, as R68u and R68v listed it.
  • The $ survivor from round 11 is unchanged.

This is the merge policy, in the critical set: it needs independent and adversarial head-pinned reviews of 1c87263.

…ure away (#1014)

Code moved in a way git doesn't pair as a rename or move read as added
code, so a test that ran the moved code passed the gate for the whole
PR (R1, R3, R4, M3, M4b, M5b, M5c, R3s, and a tested function moved
into an existing file: all auto: true before this commit).

The reader now measures each file a second way (movedCode): every
top-level statement whose code lines are all added is paired with one
whose code lines are all removed, from any app file at the base (a
deleted one too, sent as `elsewhere`), by its code first, then by its
declared name, and measured against it (codeRanges' `moved` mode:
parentheses, commas and semicolons are no change there). A file counts
only where it counts both ways, so the pairing can only take measure
away: the M1/D1/C1 must-holds keep holding, and nothing that held
before can merge.

Lines swapped inside a function that stays are in no moved unit, so a
reorder is still measured: the two round-9 "moved line inside a branch"
tests that round 13's unfinished attempt broke still hold and merge.

The todo limits become must-holds; M4b1, M5c and A6 are added, each
with a merging control. Headers, quickstart and changelog name what is
left: a statement renamed as it moved, or moved inside one that stays.

Test (fails without): test/merge-vacuity-r12.test.ts, test/merge-vacuity-r13.test.ts

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

PASS 18b48ab

Verdict: PASS. I found nothing that has to change. The fix does what #1014 asks, it can only hold more PRs than before, nothing loosens, and CI is green at this head. The engine's own weakening check holds the PR for the owner. That is expected here and is not a reason to fail it.

I reviewed in a fresh detached worktree at the head. The diff is against the merge base 32cd15e5; origin/main is now 04f83b51. I read #1014, #903, the PR body, docs/process.md and every changed line in the 8 files. The worktree has been removed: symlink first, then git worktree remove.

Fail-before (measured)

  • At head, r12 and r13 pass 39 of 39.
  • I reverted src/vacuity.ts, src/vacuity-read.ts and src/coordinator/merge-flow.ts to the merge base, rebuilt and re-ran: 39 tests, 29 pass, 10 fail.
  • The ten failures are exactly R1, R3, R4, M3, M4b, M4b1, M5b, M5c, A6 and R3s. Every control still merged.

Full vacuity set (measured)

  • All 23 files (merge-policy-vacuity, merge-vacuity-* and vacuity) at head: 280 tests, 271 pass, 0 fail, 9 todo. The 9 todos are the existing V8 limits.
  • npm run typecheck (root and web) exits 0.
  • push-gate --range origin/main..<sha> is clean.
  • The CI-parity set passes 184 of 184: change-files-renames, merge-policy, changelog, owner-terminal-removed, merge-policy-weakening, changelog-fragments, brand, docs-guardrails and engine-guard-locks.
  • check:denylist reports 1645 files clean.

The gate can't pass a PR it held before

  • Inferred, from reading the code: a file counts as run only if it counts both with and without the pairing. The original measure is computed exactly as before; the lexer only adds a new top output and leaves the others unchanged. So the set of files the run counts can only shrink.
    • The list of modules loaded on their own doesn't depend on that set.
    • The policy reads the set only as "did the tests run at least one changed app file", so a smaller set can only hold more.
    • Any new exception or timeout makes the PR wait.
    • The same both-ways check covers built code (dist/).
  • Measured: I built a mutant that keeps only the paired measure.
    • The existing L1 control in merge-vacuity-r11-align then merges when it should hold.
    • So does a case of my own (X1): an untaken arm edited in place, plus a dead copy of the old line in a new file and an unrelated k in a deleted file. The pairing then measures the line against the wrong source.
    • All of r12 and r13 still pass under this mutant.
    • So the rule matters, and an existing test catches its removal. The PR body's "no test fails if the both-ways rule is removed" is wrong, but in the safe direction.

Other mutants (measured)

  • With name pairing turned off, M3 fails, and so does my X2 (a tested function moved to a new file gaining export, next to an untested edit).
  • With the punctuation allowance turned off (MOVE_NOISE), M5b and R3s fail.
  • At head, X1, X2 and X3 (a tested class moved to a new file next to an untested edit) all hold.

The seven todo-to-must-hold conversions only tighten (measured, from the diff)

  • R1, R3 and R4 in r12, and M3, M4b, M5b and R3s in r13, each only drop { todo: MOVES } and the limit: prefix. Their assertion bodies are byte-identical.
  • Three new must-holds are added, each with a merging control: M4b1, M5c and A6.

Nothing loosens (inferred, from reading the diff)

  • No assertion is weakened, no test is skipped or removed, and no exception or allowlist entry is added.
  • The docs and the changelog describe more being held, not less.
  • One behaviour change is a tightening: a PR that only moves code now holds instead of merging.
  • The changelog fragment is present, and the brand and denylist checks pass.

CI (measured): gh pr checks 1068 at 18b48ab7: every check passed once the 8 test shards finished. The bootstrap and real-systemd jobs in the slow workflow were skipped, and nothing is red. Of the two scan checks, both are gitleaks secret scans.

Weakening scan (measured): this is the engine's merge-policy check, not a CI job, so I called weakeningOf on this change myself, at both the tip and the base. It returns two "may weaken a check" holds:

  • one for the changed test(...) lines in r12 and r13;
  • one for the changed guard lines in merge-flow.ts and the vacuity sources.

It flags any changed test or guard line. I checked those lines (above): the test changes only tighten and the guard changes only add measuring. This PR is in the critical set and is owner-merged anyway, so the hold just sends it to the owner, as intended.

Optional, not required

  • Correct the PR body's claim about the both-ways rule: the L1 control already catches its removal (measured).
  • A test aimed directly at the rule from the moved-code side, like X1, would make the reason for it obvious (guess, on its value).
  • Not a finding: one of my runs showed a not ok 1 - dist/test. That came from my own command passing the directory as a test file, not from the PR (measured).

@jayden1711

Copy link
Copy Markdown
Owner Author

Second review (adversarial): FAIL 18b48ab

The pairing never lets through a PR that main holds. But in two common shapes it silently falls back to the old vacuous measure, so a PR with an untested real change still merges. Those shapes aren't among the limits the docs name. Separately, the dist/ half of the both-ways rule survives mutation.

What I checked and ran (scratch detached worktree at the head, everything heavy through heavy.sh)

  • Build and new tests (measured): tsc is clean. r12 and r13 pass 39/39.
  • PR's full vacuity set (measured): every merge-vacuity-* file plus vacuity and merge-policy-vacuity: 280 tests, 271 pass, 0 fail, 9 todo.
  • Fail-before (measured): I checked out the merge base's versions of the four source files, rebuilt, and ran r12 and r13: 39 tests, 29 pass, 10 fail. These are the same ten the PR body lists.
  • CI parity (measured):
    • push-gate --range origin/main..18b48ab7 is clean.
    • change-files-renames, merge-policy, changelog and owner-terminal-removed pass 155/155.
    • gh pr checks 1068 at this head: every check passes or is skipped.
  • Probes: I wrote 12 fixtures through the real decided() merge flow, in a scratch test file that is now deleted.
  • Mutants: 15 source mutants, each run against all 291 tests (the 280 above plus my probes).
  • Cost: I timed measureTouched directly on inputs with many hunks.

Required changes

  1. A / inside brackets turns pairing off for the rest of the file (measured; not a named limit).

    • Root cause: in lexLines's "every / is a regex" reading, a line like return Math.round(u / 2); swallows its ). Bracket depth then stays at 1, so codeLines().top is false for every later line. Measured top output: [true,false,false,false,false,false,false] with /, against [true,false,false,true,…] with *.
    • P1a: M5c with export function half(u) { return Math.round(u / 2); } above g in the source file gives auto: true. The untested 'everything' arm in h merges.
    • P1b: the same function placed in the target file also gives auto: true.
    • P1ctl: the same fixture with u * 2 holds.
    • Division inside a call or an index is very common, yet the quickstart and changelog now say a whole top-level statement that keeps its name "is measured as if git had paired it".
    • Suggested fix: take top from the heuristic reading only, or make the regex reading reset or clamp its depth. Either is safe because pairing only removes measure (inferred). Add P1a and P1b as must-holds.
  2. A moved unit that can't be placed drops pairing for its whole file, so the PR merges (measured).

    • Cause: in movedCode, if (!m) continue; leaves the file measured as plain added code. That is the vacuous behaviour Vacuity gate: pair moved code git doesn't pair, without ever adding anything to measure (follow-up to #903) #1014 fixes.
    • P2: g (300 lines) is moved to c.js and rewritten untested into 280 lines, beside an unchanged h that the test calls. Result: auto: true.
    • P2ctl: the same with a small rewrite holds.
    • Elsewhere the gate holds a change it can't place by character (UNPLACEABLE); here it fails open, and also discards the file's units that could be placed.
    • Suggested fix: throw UNPLACEABLE, or at least hold. Add P2.
  3. The dist/ half of the both-ways rule has surviving mutants (measured).

    • Mutant B: hit(spansOf(src)) && removed. No PR test fails. My P3d kills it.
    • Mutant C: && hit(movedSpansOf(src)) removed. No PR test fails. My P5 kills it.
    • P3d is a multi-line g with a comment added on every line and its untaken arm edited, plus a removed same-named g in b.js that aligns more closely. P5 is R3 run through dist/ with identity maps.
    • Both are this policy's core safety rule, so tests like these are required.
  4. Correct the PR body's claim that "No test fails if the both-ways rule is removed" (measured: false).

    • The non-dist mutant (Mutant A: ranOn(…codeOf…) && removed) fails the existing control L1 in merge-vacuity-r11-align. It also fails my P3 and P3m.
    • L1 catches it only incidentally. I recommend adding P3m (or something like it) as a dedicated must-hold.

Optional

  • No real loosening found (inferred, supported by measurement). Every fixture's result is held under the PR whenever main holds it; the only new pass condition is the && in measureTouched.
    • I tried punctuation-only changes, same-name decoys, a deleted file's spans sent as elsewhere, and the dist/ path.
    • P1a, P1b and P2 also merge on main (measured: they fail under Mutant D, which removes the moved check).
  • Punctuation-only semantic change gives a false hold (measured, named in the PR). P4 rewrites return combine(alpha(u)), beta(u); in moved g; it is tested yet held.
  • A same-named decoy that aligns better wins the name pairing (inferred). For example, a longer removed g containing the new one. This can only add false holds, and the PR names it.
  • Cost grows quadratically with hunks (measured). At 64,000 one-line hunks, measureTouched takes 104.5 s, against 3.4 s with movedCode disabled. 32,000 hunks take 23.2 s.
  • A base file baseOf can't read is silently skipped as a source (inferred, not run). For example, a deleted file over 8 MB. It falls back to old behaviour rather than holding.
  • Mutants that survive but only ever remove measure (measured survivors; harmless by the AND rule, inferred):
    • Mutant I: no "wholly added or removed" requirement.
    • Mutant M: sources reused.
    • Mutant N: elsewhere includes measured files.
    • Mutant P: name pairing always takes the first candidate.
  • Killed mutants (measured):
Mutant Change Killed by
D moved check removed R1, R3, R4, M3, M4b, M4b1, M5b, M5c, A6, R3s
E quiet disabled M5b, R3s
F name pairing removed M3
G reader drops elsewhere R1, R4
J top ignores depth M3, M5b, R3s
K git's characters kept inside units the ten in D's row
L moved mode joins toggled hunks R3 and eight more
O merge-flow sends empty elsewhere R1, R4
  • Changelog fragment is present. No skipped or weakened assertions. No brand or denylist hits (push-gate clean).

The scratch worktree is removed: the node_modules symlink first, then git worktree remove --force.

…hrough (#1014)

From the adversarial review of 18b48ab, each measured there and here:

- A division inside brackets (`Math.round(u / 2)`) turned the pairing
  off for the rest of its file: codeLines' `top` needed both readings,
  and the every-`/`-is-a-regex one swallows the `)`, so no later line
  started outside the brackets. `top` now comes from the first reading
  when it ends cleanly (P1a, P1b: auto true at 18b48ab).
- A moved unit codeRanges couldn't place left its whole file measured
  as added code. It now throws UNPLACEABLE, as a hunk does (P2: a
  300-line g rewritten untested into 280 lines beside a tested h).

Must-holds P3m, P3d and P5 pin the both-ways rule in app code and
through dist/ source maps: each fails under the mutant that drops one
half of it.

Test (fails without): test/merge-vacuity-r13.test.ts (P1a, P1b, P2)

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Owner decision on the weakening scan, scoped to head 01163c9: "ok", not a weakening.

The engine's weakeningOf (base 32cd15e, tip 261eeb4) flagged:

  • test/merge-vacuity-r12.test.ts: 7 changed test( lines. They turn limit: … { todo: MOVES } cases into must hold: tests with no todo, so those cases now have to pass (measured from the diff).
  • src/coordinator/merge-flow.ts: changedHunks now also returns the removed spans of other app files, which the reader gets as elsewhere. Nothing is removed from the check (measured from the diff).

A new head needs a new scan and decision.

@jayden1711

Copy link
Copy Markdown
Owner Author

FAIL 01163c9

Summary. Every claim in the PR body's "Adversarial review fixes" section checks out. But the new UNPLACEABLE throw makes ordinary, fully tested PRs hold that merge on main and at 18b48ab. Neither the PR body, the changelog nor the quickstart mentions this. A smaller change I tried in a scratch build fixes it and keeps P2 held.

What I checked and ran

  • Setup (measured): fresh detached worktree at 01163c9, built with heavy.sh npx tsc -p tsconfig.json (clean). Current main is 261eeb4.
  • All vacuity tests at the head (measured): every dist/test/merge-vacuity-*.test.js plus vacuity and merge-policy-vacuity: 288 tests, 279 pass, 0 fail, 9 todo. This matches the PR body.
  • Fail-before (measured): I put back 18b48ab's src/vacuity.ts, rebuilt, and ran r13 and r11-align: 43 tests, 3 fail.
    • P1a, P1b and P2 each fail with auto: true.
    • P3m, P3d and P5 pass there. That is expected, because they target the both-ways rule, which is already in 18b48ab; the mutants below cover them.
  • CI parity (measured):
    • node scripts/push-gate.mjs --range origin/main..01163c9f: 2 commits and 1 tip, clean.
    • change-files-renames, merge-policy, changelog, owner-terminal-removed and merge-policy-weakening: 155/155 pass.
  • PR CI (measured, gh pr checks 1068): every completed check is green. "test shard 1/4 (ubuntu-latest, 24)" was still pending. I didn't wait for it, because the verdict is FAIL either way.

Mutants, applied to dist/src/vacuity.js and run against r13, r12 and r11-align (measured): all six were killed

Mutant What it changes Tests that fail
A Removes the unpaired half of the app-code check L1, P3m
B Removes the unpaired half of the dist/ check P3d
C Removes the paired half of the dist/ check P5
F (mine) top from the second reading first P1a, P1b
G (mine) Puts back the old top (both readings must agree) P1a, P1b
H (mine) continue instead of the UNPLACEABLE throw P2

Findings

  1. Required: the throw causes false holds (measured). I probed the same PRs at the head, at 18b48ab and at main:

    • FH1: a tested 150-line run() moved to a new file, with two of its lines replaced by unrelated lines (one near the top, one near the bottom). The head holds it with UNPLACEABLE. 18b48ab and main merge it.
    • FH1, done in place: the same two-line rewrite without the move merges at the head. So the source comment's "as for a hunk" is inaccurate: git splits the in-place edit into small hunks, but a moved unit is aligned against its source as one block.
    • FH2: src/old.js deleted, and a new, different 120-line main() with the same name added in src/new.js and tested. The head holds it with UNPLACEABLE. 18b48ab and main merge it. Replacing a module with a new implementation is an ordinary PR.
    • Control: the same tested move with one similar line edited merges at the head.
    • Why it happens (inferred): a unit paired only by name is aligned as one block. Past about 2,000 characters, alignHunk falls back to pairing lines, and returns null when a pair of lines keeps less than half its code.
    • The hold message is also misleading: "its coverage run failed: a change too large to align" for a two-line edit.
  2. Fix I tried (measured, scratch patch to dist/src/vacuity.js, not committed): in movedCode, when a name-only pairing can't be placed, drop just that unit's pairing and keep the others. Throw only if a pairing by identical code fails.

    • P2 still holds, as "ran none" rather than UNPLACEABLE.
    • FH1 and FH2 merge, as on main.
    • Everything else in r11-align, r12 and r13 passes. The one failure is P2's expected reason text.
    • It still can't pass a PR that main held, because of the both-ways rule (inferred).
    • Trade-off (inferred): a moved unit that is both rewritten past alignment and tested only partly would merge, as it does on main today.

    Required: either adopt this, change P2's expected reason to "ran none", and add FH1 and FH2 as tests that must merge; or keep the throw with your explicit sign-off, and disclose it in the PR body, changelog and quickstart with a test that pins it.

  3. Required: the docs leave this out (measured, read). The PR body's "Behaviour change" section, changelog.d/1014-vacuity-moved-code.md and docs/quickstart.md say nothing about moved code that is rewritten too much to align now holding a PR even when fully tested.

  4. Optional: a rarer way around the top fix remains (measured). BP1 is M5c with if (s) /\(/.test(s); above g in the file g moved to. It merges with auto: true at the head, at 18b48ab and at main. A regular expression right after ) misreads in the first reading, so the bracket count never returns to zero and pairing is off for the rest of the file. Like before Vacuity gate: pair moved code git doesn't pair, without ever adding anything to measure (follow-up to #903) #1014, that code reads as added, so this isn't a regression. A possible fix: prefer the reading whose bracket count ends at zero. Not blocking.

  5. Diff hygiene (measured, read): no weakened assertions, no skipped tests, no scope creep in 01163c9, and the changelog fragment is present. The new comments cite "Vacuity gate: pair moved code git doesn't pair, without ever adding anything to measure (follow-up to #903) #1014's adversarial review, P1/P2". That matches how the surrounding code cites #784 round N, so it's not blocking.

The scratch worktree is removed.

@jayden1711

Copy link
Copy Markdown
Owner Author

Second review (adversarial): FAIL 01163c9

The fixes for the previous review hold, but 7 of my 12 probes still merge with auto: true (measured). Each is a whole top-level statement that keeps its name. In each one, a test runs moved code, and that lets an untested change merge. docs/quickstart.md, the changelog fragment and the merge-policy-vacuity.ts comment say this case is now "measured as if git had paired it", so those claims are false too.

The probes are in /var/tmp/wl-coord/tmp/rev-r18d/r18d-probes.test.ts. They use the same decided() harness as merge-vacuity-r13. Measured with node --import ./dist/test/isolate.js --test dist/test/zz-r18d-probe.test.js: 5 pass, 7 fail.

Required changes

  1. A statement that spans lines with no bracket open is split into separate units (measured: E1, E1b merge). A line that starts with no bracket open reads as a new statement, so a formatter's arrow wrap breaks the unit in two:

    • E1: export const g = (u) => combine(alpha(u), beta(u)); is moved to c.js and wrapped as export const g = (u) => with combine(alpha(u), beta(u)); on the next line. An untested arm is edited in h. The first piece pairs by name; the body line has no name and no matching code, so it reads as added and the test of g counts it.
    • E1b: the same with alpha(u) + and beta(u) on separate lines.
    • Control E1c (the same move, not wrapped) holds.
    • A unit needs to run until the statement actually ends, not just until the next line that starts with no bracket open.
  2. A unit only pairs when every line of it is added or removed, so a shared } defeats it (measured: E2, E2d merge). With -w and the histogram algorithm, git keeps a common } as an unchanged line:

    • E2: g moves from s.js to c.js, and an untested new function q is written where g was. The closing } stays unchanged, so g's source isn't wholly removed, and the moved g reads as added.
    • E2d: the destination side. g moves into b.js in place of a removed old(), and the shared } means g isn't wholly added.
    • Control E2c (q added after h instead) holds.
    • Replacing a function in place while moving the old one out is an ordinary refactor.
  3. The name pattern doesn't recognise every statement that keeps its name (measured: E3 merges). A CommonJS exports.g = function (u) {…} is moved to a new file with an untaken arm edited (M3's shape). It gets no name, its code differs, and the pairing never finds it. export abstract class, export enum, export namespace and module.exports.x = are missed the same way (inferred from the name pattern, not run).

  4. The first lexer reading can end cleanly and still be wrong about brackets (measured: E4, E5 merge). The fix takes statement starts from the first reading whenever it ends cleanly, but that reading can be wrong:

    • E4: if (u) /\(/.test(u); sits above g in c.js. The first reading takes / after ) as a division, so the \( opens a bracket that never closes. No later line starts a statement, and g merges into the function above it.
    • E5: Math.floor(i++ / 2). After +, the first reading takes / as the start of a regular expression, which swallows the ).
    • Control E6 (a template with ${Math.round(a / 2)} and ${ {k: 1}.k }) holds.
    • These shapes are rarer than 1–3, but they show that "ends cleanly" doesn't mean the brackets were read right. Failing closed would avoid this: hold the PR when the two readings disagree about where statements start, when a moved unit is involved.
  5. Correct the docs, changelog and comment claims to match what is actually measured, once items 1–4 are fixed or listed as limits.

What I checked

  • New tests and CI-parity suites (measured, fresh detached worktree at the head, build clean):
Suite Result
merge-vacuity-r13 31/31 pass
merge-vacuity-r12 16/16 pass
change-files-renames 63/63 pass
merge-policy 57/57 pass
changelog 27/27 pass
owner-terminal-removed 8/8 pass
  • Push gate (measured): node scripts/push-gate.mjs --range origin/main..01163c9f reports 2 commits clean.
  • Fail-before (measured): with src/vacuity.ts reverted to the parent 18b48ab, P1a, P1b and P2 fail (28 pass, 3 fail).
  • Mutants against the fix (measured, each one caught by r13):
    • Statement starts always taken from the regex reading: P1a and P1b fail.
    • An unplaceable moved unit skipped instead of holding: P2 fails.
    • Built-code (dist/) lines counted from the moved reading only: P3d fails.
    • App code counted from the moved reading only: P3m fails.
    • The formatter-noise characters (),; not ignored: M5b and R3s fail.
  • False holds from the unplaceable throw (measured: none found):
    • F1: a 400-line function moved unchanged beside a tested new function merges.
    • F2: an unrelated 300-line main added and tested while the old file's main is deleted merges.
    • I didn't cover every shape; for example, a large function paired by name and genuinely rewritten holds (P2's case). That's the safe direction (inferred).
  • Diff hygiene (inferred, read the diff): no weakened assertions or skipped tests, changelog fragment present, the brand name isn't hardcoded, and the denylist CI check passed.
  • PR CI at this head (measured, from gh pr view): at the time of reading, test shard 1/4 (Node 22 and 24) and test shard 4/4 (Node 22) were still running, and one new-deps run was cancelled while another passed. The rest were green or skipped. I didn't wait for them because the verdict is FAIL either way.

The scratch worktree is removed.

#1014)

Round 3 of the moved-code pairing, after both reviews of 01163c9:

- A unit paired by name that can't be aligned with its source is
  measured as added code, as git paired it; the file's other pairings
  stay. A unit with its source's code is never aligned: nothing in it
  changed. The UNPLACEABLE throw from movedCode is gone (FH1, FH2 must
  merge; P2 holds as ran none; P2x).
- A top-level statement runs until its code has ended, across a line a
  formatter wrapped after an operator (E1, E1b, E1f, E1s).
- A closing line git keeps between a moved unit and the code written in
  its place doesn't stop the pairing, at either end (E2, E2d).
- Declared names also read from exports/module.exports assignments and
  TypeScript abstract classes, enums and namespaces (E3, E3m, E3a, E3n).
- Statement starts come from their own lexer reading (a regular
  expression after a condition's `)`, a division after a postfix
  increment), else the every-slash-a-regex reading, whichever balances
  its brackets; codeLines' mask is unchanged (E4, E4b, E5, E7). A file
  no reading balances pairs nothing, as before #1014: a todo names it.

Docs, changelog and comments name what still reads as added code.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Owner decision on the weakening scan, scoped to head e19bdc3: "ok", not a weakening.

The engine's weakeningOf (base 32cd15e, tip e91629f) flags:

  • the two items cleared at 01163c9: the r12 limit: … { todo } cases turned into must-hold tests, and merge-flow.ts passing the reader elsewhere;
  • a new { todo } limit test in test/merge-vacuity-r13.test.ts: a file whose brackets no lexer reading balances pairs nothing. The builder measured that the same shape merges on main, so it isn't a regression; the reviewers of this head are checking that;
  • P2's expected reason, changed from UNPLACEABLE to "ran none". P2 still holds. The throw was removed because it held fully tested PRs that main merges (R18c's FH1 and FH2).

A new head needs a new scan and decision.

@jayden1711

Copy link
Copy Markdown
Owner Author

FAIL e19bdc3

Summary. Every round-3 claim I re-checked holds. But round 3 opens a new way for an untested change to merge, and the docs don't name it. The shape is a TypeScript file written without semicolons, where a line ending in > or ! sits above the moved function, for example export type T = Array<number> or const k = globalThis.process!. At 01163c9 the PR held; at this head it merges with auto: true. Main merges it too, so it isn't a regression from main. It's the same kind of statement-boundary gap as R18d's E1, which was a required fix. CI also isn't complete-green.

What I checked and ran

All of this ran in fresh detached worktrees (r18e at the head, r18e-b as a scratch copy), built with heavy.sh npx tsc -p tsconfig.json and run through heavy.sh node --import ./dist/test/isolate.js --test. Main is e91629f. None of the four source files this PR changes differ between main and the PR's merge base 32cd15e, and the PR merges into main cleanly (measured).

  • Vacuity suites at the head (measured): every merge-vacuity-* plus vacuity and merge-policy-vacuity: 309 tests, 299 pass, 0 fail, 10 todo. This matches the PR body.
  • Fail-before (measured): with src/vacuity.ts and src/merge-policy-vacuity.ts put back to 01163c9, r11-align, r12 and r13 give 80 tests, 17 fail. Those are P2, P2x, FH1, FH2, E1, E1b, E1f, E2, E2d, E3, E3m, E3a, E3n, E4, E4b, E5 and E7, exactly the builder's list. E1s passes there, as claimed.
  • R18d's 12 probes (measured):
    • At this head all 12 behave as intended: the 10 must-holds hold and F1/F2 merge.
    • At 01163c9, 7 fail (E1, E1b, E2, E2d, E3, E4, E5), which reproduces R18d's result.
    • At main every probe merges.
  • R18c's FH1 and FH2 (measured): both merge at this head. At 01163c9 both hold with the "too large to align" reason (UNPLACEABLE). FH1 done in place merges at all three.
  • The { todo } limit (measured): g placed after Math.round(function () { return 4; } / 2) merges both at this head and at main. So it is not a regression, as claimed.
  • P2's reason change (measured):
    • P2 holds as "ran none", and that is correct: g is genuinely rewritten and never run.
    • My P2t (the same PR with g also tested) merges, which shows the unplaceable g is now measured as added code.
    • Main merges P2, per the builder's measurement at 18b48ab, so the head is stricter here and nothing regressed.
  • CI parity (measured):
    • node scripts/push-gate.mjs --range origin/main..e19bdc3b: 3 commits and 1 tip, clean.
    • change-files-renames, merge-policy, changelog, owner-terminal-removed and merge-policy-weakening: 155/155 pass.
    • All 3 commits are signed off.
  • PR CI at this head (measured, gh pr view 1068 --json statusCheckRollup): every check completed. Two are red: "test shard 2/4 (ubuntu-latest, 24)" and the "test (ubuntu-latest, 24)" check that rolls the shards up.
    • The shard's "install gitleaks" step failed with curl: (35) Recv failure: Connection reset by peer, a network failure unrelated to this change.
    • Because of that, "this shard's tests" was skipped. Shard 2/4 on Node 24 never ran, though the same shard on Node 22 passed.
    • It needs a rerun before any PASS.

Mutants (measured, applied to dist/src/vacuity.js, run against r11-align and r13)

All 10 of the builder's mutants I re-ran are killed:

Mutant Tests that fail
R1 E4b, E5
R2 E5
R3 E4b
R4 E7
R6 E1f
R7 E1s
R8 E2, E2d
R10 FH1, FH2
R11 P2x

R6 needed its text corrected for the compiled file (const goesOn = /).

All 4 of my own mutants survive (63/63 pass with each):

Mutant What it changes
O1 uses an unbalanced lexer reading instead of pairing nothing
O2 drops low >= 0 from balanced
O3 placed looks up ms.indexOf instead of named.indexOf
O4 closes also accepts lowercase letters

O1, O2 and O4 only change where measure is taken away, so the both-ways rule keeps them from merging anything main holds (inferred). O3 matters only in a file that has both an identical-code unit and an unplaceable name-paired unit, and no test has that combination.

Findings

  1. Required: a no-semicolon TypeScript line ending in > or ! stops the next statement from starting (measured).

    • The probes: each is M5c into src/c.ts, with an untested arm edited in h, and the extra line placed above g.
      • X2: export type T = Array<number> (no semicolon).
      • X2b: export type T = Map<string, number>.
      • X3: const k = globalThis.process!.
    • Results: all three merge (auto: true) at this head, hold as "ran none" at 01163c9, and merge at main. The X2 control with a semicolon holds. A JavaScript const k = 1 + 1 without a semicolon also holds.
    • Cause (inferred): topUnits' goesOn reads a trailing > or ! as an operator, so g is absorbed into the line above. That merged unit has no name and its code doesn't match g's source, so nothing pairs it and it reads as added code.
    • The source side (inferred, not run): the same can happen in the file g moved from.
    • A fix I tried (measured, scratch patch to dist/src/vacuity.js, not committed): a line that starts with a keyword that can never continue an expression always starts a statement, whatever ends the line before it. The keywords are export, const, let, var, enum, interface, declare, abstract, namespace and import (not followed by ().
      • With it, X2, X2b and X3 hold.
      • E1f still holds, because its continuation line starts with function.
      • r11-align, r12, r13, R18d's 12 and my probes give 107 tests, 106 pass, 0 fail, 1 todo.
    • Required: adopt that rule or an equivalent, and add X2, X2b and X3 as must-hold tests.
  2. Required: the docs (measured, read; X1 measured):

    • Where: docs/quickstart.md, the changelog fragment and the src/merge-policy-vacuity.ts header.
    • X2/X3: they say a whole top-level statement that keeps its name is measured as if git had paired it. X2 and X3 contradict that.
    • "Rewritten too far to align": this understates the limit.
      • My X1 moves a 150-line function and replaces just two lines inside branches the test never takes, beside an untested edit in another file. It merges at this head and at main, and held at 01163c9.
      • With only one line replaced (X1-one), or in a 20-line function, the PR holds.
      • So the docs should say that in a large moved unit, two replaced lines are enough. This is R18c's accepted trade-off and the same as main, so only the wording needs to change.
  3. Required before any PASS (measured): the Node 24 shard 2/4 has to be rerun and come back green.

  4. Optional: add tests that kill O3 (a file with an identical-code unit plus an unplaceable name-paired unit) and O4 (a unit with an unchanged non-closing line). O1 and O2 only take measure away, so they matter less.

  5. Diff hygiene (measured, read):

    • No weakened assertions or skipped tests: r12 and r13 todos became must-holds.
    • The changelog fragment is present and the product name isn't hardcoded.
    • The comments cite review ids in the same style as the existing #784 round N comments, so they're not noise.

My probe file is at /var/tmp/wl-coord/tmp/rev-r18e/r18e-probes.test.ts. Logs for the head, 01163c9 and main runs, the mutants and the fix attempt are in /var/tmp/wl-coord/tmp/rev-r18e/. Both scratch worktrees are removed.

@jayden1711

Copy link
Copy Markdown
Owner Author

Second review (adversarial): FAIL e19bdc3

This head makes an ordinary tested PR worse than main (measured). Take a one-line top-level function edited in place, with no move at all, where the fix only adds parentheses ((s) => s.open to (s) => s.open()) and a test runs it. Main merges that PR; this head holds it with "ran none". Several in-scope moved-unit shapes also still merge with auto: true, so the docs claim that such moves are "measured as if git had paired it" doesn't hold for them. None of those shapes is new: main merges each of them too.

The probes are in /var/tmp/wl-coord/tmp/rev-r18f/r18f-probes.test.ts, using the same decided() harness as merge-vacuity-r13. I ran them at the head and at main (e91629f). Logs: probe-r18f.log, probe-r18f-main.log, probeF-r18f.log, probeF-r18f-main.log.

Required changes

  1. A false hold on ordinary in-place edits, worse than main (bar a; measured at the head and at main). Each of these PRs has a tested fix and no moved code. Main merges each one; the head holds each one with "ran none":

    • Z-F6: export const isOpen = (s) => s.open; becomes s.open().
    • Z-F7: a * b + c becomes a * (b + c) in a one-line arrow.
    • Z-F9: the same call fix in a CommonJS exports.isOpen = (s) => s.open;.
    • Z-F10: the same call fix in export function ready(s) { return s.ok; }.
    • Control Z-F8: a real edit (+ c to - c) merges at both.
    • Cause (inferred from the code): a one-line top-level statement edited in place is a unit that is wholly removed and wholly added. movedCode pairs it with its own old line by its key, and the key drops (),; (MOVE_NOISE). The paired unit then reads as "changed nothing", and that read replaces git's measurement. For the AND to stay a pure take-away, a unit shouldn't pair with the removed side of the hunk git already paired it with. The same applies to a unit that is wholly changed in place.
    • Same root, moved case (measured): Z-F1 (a * b + c to a * (b + c)) and Z-F1b (!x instanceof Array to !(x instanceof Array)) are tested precedence fixes in a function moved to a new file. Main merges both; the head holds both. Either make parentheses that change precedence count, or name this as a limit. As shipped, the docs' "measured as if git had paired it" is false here, because git counts those parentheses.
  2. A same-named unit takes a moved unit's source (bar b: whole named top-level units, in scope; measured; main merges these too). In the by-name pass, the source goes to the first unit in path order with that name, not to the closest match. The real moved unit is then left unpaired and reads as added code:

    • Z-N1: M3 (g moved to c.js with its untaken arm edited) plus an untested export function g in a new b.js. Merges.
    • Control Z-N1z: the same decoy in z.js holds.
    • Z-N2: M3 where a new g is written in a.js in the old one's place. Merges. Replacing an implementation and moving the old one aside is an ordinary refactor.
    • Z-N3: E3 (exports.g = moved, untaken arm edited) plus exports.g = () => 0; in b.cjs. Merges.
    • Either pick the best-aligned destination for each source (the sources already get a best-aligned pick), or name this as a limit.
  3. A statement read as continuing past an operator that actually ends it (bar b; measured; main merges these too). In semicolon-free TypeScript, a line ending in > or ! makes goesOn true. The moved g then joins the unit above it and isn't a whole unit:

    • Z-A1: export type Pair = Array<number> above the moved g. Merges.
    • Control Z-A1c: the same line ending in ; holds.
    • Z-A2: export const env = globalThis.process! above the moved g. Merges.
    • Fix this, or name it as a limit.
  4. Docs, changelog and comment (bar c): once items 1–3 are fixed or listed as limits, make docs/quickstart.md, changelog.d/1014-vacuity-moved-code.md and the merge-policy-vacuity.ts comment match them.

Optional (outside the stated scope, or rare; main merges these too; measured)

  • Z-A4: a numeric literal ending in a dot (export const scale = 2.) above the moved g merges.
  • Z-B1: the fallback reading can balance and still be wrong. With export default /\)/; the statement reading can't balance, so the every-/-a-regex reading is used. That reading balances Math.round(7 / 2) against (8 / (4 / 2)) around g, so g isn't a unit start, and the PR merges. Control Z-B1c (without the default regex) holds.
  • Z-B2: of used as a variable before a division (Math.round(of / 2)) merges. of is in the list of keywords after which a / starts a regular expression.
  • The postfix exemption is untested: end !== '++' && end !== '--' has no test in -r13. With it removed, all 51 -r13 tests still pass; my Z-A3 (a semicolon-free n++ line above the moved g) catches it.

Held correctly at the head (main merges them all; measured)

  • Z-A3: a semicolon-free ++ line above the moved g.
  • Z-A5: a semicolon-free } line followed by a call statement.
  • Z-T1, Z-T2: declaration lines inside multi-line templates, including a ${} spanning lines with a nested backtick.
  • Z-N4: two sources named g, where the best-aligned one is chosen.
  • Z-F5: M5c with CRLF line endings.

Also measured: Z-F2 (a tested q written where g was, sharing }), Z-F3 (a tested in-place rewrite where git keeps only }) and Z-F4 (a tested edit moved into a CRLF file) merge at both the head and main.

What I checked

  • Build (measured): tsc is clean at the head and at main, each in a fresh detached worktree.
Suite at the head Result (measured)
merge-vacuity-r13 51 pass, 1 todo
merge-vacuity-r12 16/16 pass
change-files-renames 63/63 pass
merge-policy 57/57 pass
changelog 27/27 pass
owner-terminal-removed 8/8 pass
  • Push gate (measured): node scripts/push-gate.mjs --range origin/main..e19bdc3b reports 3 commits and 1 tip clean.
  • Fail-before (measured): with src/vacuity.ts reverted to 01163c9, -r13 gives 34 pass and 17 fail. The failures are P2, P2x, FH1, FH2, E1, E1b, E1f, E2, E2d, E3, E3m, E3a, E3n, E4, E4b, E5 and E7.
  • Mutants (measured, -r13):
Mutant Result
goesOn always false caught, but only by E1f (E1 and E1b still pass)
closer tolerance removed caught by E2 and E2d
postfix ++/-- exemption removed survives the PR's suite; caught by my Z-A3
  • Diff hygiene (inferred, from reading the diff): no weakened assertions or skipped tests, and the R1, R3 and R4 todos became must-holds. The changelog fragment is present and the brand name isn't hardcoded; the CI denylist check is green.
  • PR CI at this head (measured, from gh pr view --json statusCheckRollup): everything is complete. Every check is green or skipped except test shard 2/4 (ubuntu-latest, 24) and its test (ubuntu-latest, 24) rollup. That job's log shows 343 pass and 0 fail, then curl: (35) Recv failure: Connection reset by peer while downloading gitleaks. That's a network flake, unrelated to the change; a rerun is needed before merge.
  • Cleanup (measured): the dist checksum after the mutants and probes (08254150…) equals the pre-mutant one. Both scratch worktrees are removed.

…ound 4)

- A unit is never paired with the old text git already paired it with, so
  a statement edited in place is measured as git measured it.
- Only formatting is no change in moved code: semicolons, a comma before a
  closing bracket, and parentheses that can't change meaning. Parentheses
  that change precedence count as code.
- A column-0 export or import line always starts a statement; so does a
  line starting with a keyword no expression continues with.
- Same-named pairs are taken best-aligned first, so a same-named unit
  never takes another's source by path order. A source may keep its
  header where git keeps it for the code rewritten in its place, provided
  one of its lines was removed.
- Docs and limits updated: X1, and unbalanced files start statements only
  on export or import lines.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Owner decision on the weakening scan, scoped to head d76d96f: "ok", not a weakening.

The engine's weakeningOf (base 32cd15e, tip e91629f) flags:

  • the items cleared at e19bdc3: the r12 limit: cases turned into must-hold tests, and merge-flow.ts passing the reader elsewhere;
  • more limit: todo tests in test/merge-vacuity-r13.test.ts: X1 (a tested 150-line statement moved with two lines replaced reads as added code) and statements in a file no reading balances. The builder measured that main merges both shapes, so they record limits rather than relax a check. The reviewers of this head are re-checking that.

A new head needs a new scan and decision.

@jayden1711

Copy link
Copy Markdown
Owner Author

FAIL d76d96f

Summary. Every claim in round 4 that I re-checked holds, and CI at this head is complete and green. But I found a new false hold: a shape main merges and this head holds. The docs claim it can't happen. Two smaller test gaps also need fixing.

What I checked and ran

I worked in fresh detached worktrees at the head, at e19bdc3 and at main (e91629f), built each with heavy.sh npx tsc, and removed all three afterwards.

  • CI (measured, gh pr view 1068 --json statusCheckRollup and gh run view): every check completed. Runs 40163413980 and 40164971529 have headSha d76d96f and are green or skipped.
  • Vacuity set at the head (measured): all merge-vacuity-* plus vacuity and merge-policy-vacuity give 346 tests, 335 pass, 0 fail, 11 todo. This matches the PR body.
  • Probes (measured): I ran r18e's 15 probes, r18f's 27 and 12 of my own at all three commits.
    • Every r18e and r18f probe gives its intended result at the head.
    • The builder's table reproduces exactly:
      • Z-F1, Z-F1b, Z-F6, Z-F7, Z-F9 and Z-F10: pass / FAIL / pass.
      • Z-A1, Z-A2, Z-A4, Z-B1, Z-B2 and Z-N1 to Z-N3: pass / FAIL / FAIL.
      • X2, X2b, X3 and r18e's TODO-limit: hold / merge / merge.
      • X1: merges at all three.
    • No probe from r18e or r18f holds at the head where main merges. My G1 and G1b do (finding 1).
  • Fail-before (measured): head's r12 and r13 tests run against e19bdc3's source give 105 tests and 20 fail, exactly the builder's 20. Against main's source, 59 fail, including Z-A4c, E4c, Q-S, Q-S2, Q-C and Q-P, as claimed.
  • Mutants (measured): applied to the compiled vacuity.js at the head and run against r11-align, r12, r13 and my probes. The file's checksum was the same before and after.
    • 12 of the builder's mutants: 11 are killed (S1, S2, Q2, Q4, A1, A2, A3, N1, K1, K2, R3). A4, which lets an indented export start a statement, survives, as stated.
    • 3 of my own:
      • Dropping the parenthesis close-check (if (lone && after)) survives the PR's suite. Only my G2 and G2e catch it.
      • Treating parentheses that wrap { as formatting (lone = true) survives the PR's suite. Only my G5 catches it.
      • Matching any hunk in the git-paired check is killed by M4b and M4b1.
  • The A4 survivor (inferred): it's harmless. In valid code an indented export that isn't inside a string, template or comment occurs only in a TypeScript namespace or declare module block. Starting a statement there only splits that block, and the both-ways rule keeps it from merging anything main holds. The shipped code, which only anchors at column 0, is the stricter version.
  • The listed limits on main:
    • X1 (measured): merges at the head, at e19bdc3 and at main.
    • Unbalanced file (measured): the limit is real. My GU merges at the head and at main, and its balanced control holds at the head. But the PR's own todo test for it doesn't show this (finding 2).
  • CI-parity set (measured):
    • push-gate --range origin/main..d76d96f0: 4 commits and 1 tip clean.
    • check:denylist: 1645 files clean.
    • change-files-renames, merge-policy, changelog, owner-terminal-removed, changelog-fragments, brand and every test that imports merge-policy-weakening (except desktop-removed, the known local artifact): 242 pass, 0 fail.
    • adversarial.test.js was cancelled at 180 s, then passed alone: 196 pass, 0 fail, 10 todo.
    • All 4 commits are signed off.
  • Diff hygiene (measured, read):
    • No assertion was weakened. Round 4's only removed test is the ESM unbalanced-file todo, and that shape is now the must-hold E8.
    • The changelog fragment is present and the product name isn't hardcoded.
    • The comments cite test ids, in the style of earlier rounds.

Findings

  1. Required: a false hold where main merges (measured at all three). A tested one-line top-level edit made in place holds as "ran none" when the same PR removes an identical copy of the new line from another app file.
    • G1: export const g = (x) => x + 1; becomes x + 2 in a.js, the test runs g, and the PR deletes b.js, which held export const g = (x) => x + 2;.
    • G1b: the same, with b.js kept and only that line removed. This is an ordinary de-duplication.
    • Main merges both, and e19bdc3 held both too, so this isn't new in round 4.
    • G1c: the same shape in a 3-line function merges at the head, because git keeps its header line.
    • Cause (inferred): the check that a unit is never paired with the text git already paired it with only excludes that one source. The by-code pass then pairs the unit with the identical copy elsewhere, so the edit reads as "changed nothing".
    • This contradicts "A statement edited in place is measured as git measured it" in docs/quickstart.md, in the changelog fragment and in the merge-policy-vacuity.ts header.
    • Required: either don't pair at all a unit whose added lines git paired with removed code in the same unit's place, and add G1 and G1b as must-merges, or name this as a limit everywhere the docs make that claim.
  2. Required: the unbalanced-file limit's todo in r13 is vacuous (measured). At the head and at main it holds with "its coverage run failed … test failed".
    • The fixture's own test fails because its named import of the .cjs module fails (my GUn reproduces this). So the test never reaches the measurement it names and can never pass.
    • With a default import (import m from '../src/c.cjs', my GU) the limit does merge at the head and at main.
    • Required: fix the fixture so the todo actually exercises the limit.
  3. Required: pin the precedence rule the docs state (measured). Two plausible wrong implementations of "parentheses that change precedence count" pass the PR's suite:
    • Dropping the close-check: caught only by G2 (return a + b * c to return (a + b) * c, a moved function) and G2e (the same after =).
    • Treating ({ … }) after => as formatting: caught only by G5 (() => { a: 1 } to () => ({ a: 1 })).
    • Add G2, G2e and G5 as must-merge tests.
  4. Optional (inferred): ANCHOR also matches a column-0 export: or import: object key. That only splits a unit, so it takes measure away and can't merge anything main holds.

My probe file is /var/tmp/wl-coord/tmp/rev-r18g/r18g-probes.test.ts, the mutant script is mutants.sh beside it, and the logs (probes-*.log, fb-*.log, mutants.log, parity.log, adv.log, vac-head.log) are in the same folder.

@jayden1711

Copy link
Copy Markdown
Owner Author

Second review (adversarial): FAIL d76d96f

I'm failing this head for two reasons. First, one shape is worse than main: a tested one-line edit in place now holds, where main merges it. Second, the docs say redundant parentheses in moved code are no change, but they still count, so an untested change merges. Fail-before reproduces, the suites pass and CI is green at this head. My 28 probes of ordinary in-place edits all give the same result at the head and at main. The full verdict is in /var/tmp/wl-coord/v-1068-r18h.md, and the probes are in /var/tmp/wl-coord/tmp/rev-r18h/ (probes.test.ts, with head.jsonl and main.jsonl).

Required changes

  1. A false hold on a tested in-place edit (bar a; measured: the head holds, main merges).

    • D1: src/a.js changes export const f = (x) => x + 1; to x + 2 in place, and a test runs it. In the same PR, src/b.js deletes its own identical export const f = (x) => x + 2; (removing a duplicate).
    • D1d: the same, with b.js deleted outright. Same result.
    • D1m: the multi-line version merges at the head, because git keeps the header line. So whether the PR holds depends on whether the statement fits on one line.
    • Cause (inferred from the code): the new same rule only stops a statement pairing with the old text git already paired it with. The identical-code pass then pairs it with b.js's deleted copy and reads it as "changed nothing".
    • Docs: this contradicts the quickstart's "A statement edited in place is measured as git measured it."
    • Suggested fix: never pair a statement git paired in place, and add D1 and D1d as must-merges.
  2. Redundant parentheses in moved code still count (bars b and c; measured: the head merges, main merges too).

    • Each probe moves g to c.js with only its parentheses changed, adds an untested edit to h, and has a test that runs g. Each should hold, and each merges with auto: true:
      • B8: u.x && u.y || … to (u.x && u.y) || …. Prettier adds these on any re-format.
      • B8m: 5 % 3 * … to (5 % 3) * …, also a Prettier addition.
      • B18: (u.x) || ….
      • B19: u.a ? (alpha(u)) : (…). The PR body lists parentheses after ? as handled, but the closing parenthesis must be followed by ;, ,, ), ] or }, so ? (x) : never qualifies.
      • B30: (u.a) ? 1 : ….
      • B17c: an IIFE's })() written as }()).
      • B7p: a TypeScript return type (number | string) written as number | string.
    • Docs: the quickstart calls "parentheses that can't change what the code does" no change, and the changelog says only "parentheses that change precedence count". The code drops parentheses only in the listed positions.
    • Fix: either make the docs, the changelog and the merge-policy-vacuity.ts header describe exactly what's implemented and list the rest as a limit, or drop these parentheses in quiet.

Optional

  • Untested guards (measured on the r11-align, r12 and r13 suites, 115 tests): three of my six mutants of the compiled reader survive.
    • Removing the guard against parentheses that open with {: my B16b catches it. That's a tested change from ({ x }) to { x }, which becomes a false hold. Worth adding as a test.
    • Removing the template and string guard on the column-0 export anchor: no probe of mine catches it, C1 to C7 included. I infer it's low risk, because splitting a statement only takes measure away.
    • Removing ? and : from the positions where parentheses drop: nothing catches it.
  • Whitespace and semicolons (measured): B2s (whitespace inside a string) and B22 (;;), both tested and in moved code, hold at the head but merge at main. That matches how whitespace-only edits in place already behave, so I'm not requiring it.

What I checked

  • Build (measured): tsc through the slot wrapper is clean at the head.
  • Suites at the head (measured): change-files-renames, merge-policy, changelog, owner-terminal-removed, every merge-vacuity-*, vacuity* and merge-policy-vacuity*. That's 501 tests: 490 pass, 0 fail, 11 todo.
  • Push gate (measured): node scripts/push-gate.mjs --range origin/main..d76d96f0 reports 4 commits and 1 tip clean. All 4 commits are signed off.
  • Fail-before (measured): with src/vacuity.ts put back to round 3 (e19bdc3) and rebuilt, r12 and r13 give 105 tests, 20 failing. Those are exactly the 20 new tests the PR names.
  • Probes (measured): 90 at both the head and main, all run, plus two template probes (C7, C7s). Outside the findings above, the head differs from main only where it correctly holds.
    • Correct holds where main merges: B0, B5, B16, B18r, B6u, B6w, B7u, D3, C3 and C5.
    • Tested changes of meaning in moved code still merge: parentheses inside strings, regexes and ${}, (a, b) against a, b, ({}) against {}, (-2) **, ?.() and a[(b, c)].
  • Mutants (measured): the other three are killed.
    • In-place pairing always off: killed by Z-F11.
    • Same-named pairs taken in path order instead of best-aligned: killed by Z-N1, Z-N3 and Z-N4.
    • The return/throw rule off: killed by R3s.
    • The dist checksum (89b40e24…) is restored afterwards.
  • CI at this head (measured, gh pr view 1068 --json statusCheckRollup): 20 success, 7 skipped, nothing failing or pending.
  • Cleanup (measured): both scratch worktrees are removed.

- A statement git paired in place with a removed statement of its own
  name pairs with nothing, so a tested one-line edit is measured as git
  measured it even when the PR removes a copy of its new text elsewhere
  (G1, G1b, D1, D1d: must merge).
- Pin the parenthesis positions quiet treats as formatting (G2, G2e, G5,
  B16b must merge; Q-Pq, Q-Pc must hold) and the template guard on the
  column-0 anchor (Z-T3).
- The unbalanced-file limit's todo imports its CommonJS module whole, so
  it reaches the measurement it names; a balanced control holds.
- Docs name exactly which parentheses are formatting and list the rest
  as a limit.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Second review (adversarial): FAIL 7f79694

I'm failing this head for two reasons. First, round 5 makes one documented-held shape merge: a function moved into a file in place of a stale function of the same name (bar b, and the docs claim c). It held at d76d96f. Second, the changelog's "so parentheses that change precedence count" is false for parentheses after ??, so a tested precedence fix in moved code holds where main merges (bars a and c). Suites, typecheck and CI are green.

Bar applied: the owner's sibling-PR bar. FAIL only on (a) an overclaim in the docs, changelog or PR body, (b) a documented-held shape that merges, or (c) a clear false hold on an ordinary shape main merges. New surviving mutants and gaps outside the docs' claims are follow-ups.

Required

  1. The in-place rule matches any same-named statement, not "its old text" (bars b and c; measured at head, d76d96f and main cc34fd7).

    • N5: g moves unchanged from src/s.js into src/c.js, replacing a stale export function g() { return 'stale'; } there. An untested arm is edited in h in s.js, and the test runs c.js's g. Git's hunk in c.js removes the stale g and adds the new one, so the new skip applies, and g never pairs with its real source in s.js.
      • Head: merges (auto: true). d76d96f: holds. Main: merges.
      • Control N5x: the same with the stale function named old holds at head.
      • With the new skip line removed, N5 holds again (measured by mutant), so round 5 causes it.
    • N6: the same with a one-line stale export const g = () => 'stale';. It merges at head, at d76d96f and at main. I didn't trace why d76d96f also merges.
    • W1: a function moved within its file, where git puts its old and new text in one hunk. It merges at head, at d76d96f and at main.
      • src/w.js goes from g, h, z to h', g', z.
      • h' edits its untaken 'all' arm to 'everything'.
      • g' is g re-wrapped: the lone arrow parameter's parentheses and the ; dropped.
      • The test runs g. Git gives one hunk: -1,2 +1,2.
    • W2: the same with a one-line function declaration re-wrapped onto three lines. It merges at head, at d76d96f and at main.
    • Docs overclaim:
      • The quickstart says that code "moved within its file" or "re-wrapped" is measured as if git had paired it. Its only exception is "a statement edited in place (git pairs it with its old text under the same name)".
      • The changelog qualifies that exception as "(edited where it was)".
      • The code (sources.some((s) => s.name === u.name && same(u, s))) skips any unit that any hunk of its file pairs with any same-named removed statement. That covers a different statement with the same name (N5, N6) and a move inside one hunk (W1, W2).
    • Required: either make the skip apply only where the unit really is its source's own text edited in place (N5 and W1 must hold), or name both shapes as limits in the quickstart, the changelog and both headers, and pin them with tests.
  2. Parentheses after ?? are treated as formatting but change precedence (bars a and c; measured).

    • PQ2: g moves from s.js to c.js, with const v = u.a ?? (u.b ? 1 : 2); rewritten as const v = u.a ?? u.b ? 1 : 2;. The second form parses as (u.a ?? u.b) ? 1 : 2. The test asserts the new result, 2, for { a: 0, b: false }.
    • PQ3: the same in return, tested with { a: null, b: true }, so the right side of ?? runs.
    • Results:
Probe Head d76d96f Main In place at head
PQ2 holds ("ran none") holds merges not run
PQ3 holds ("ran none") holds merges merges
  • Cause (measured by mutant): with ? removed from quiet's positions, PQ2 and PQ3 merge.
  • Docs overclaim:
    • The changelog says "Every other parenthesis counts as code, so parentheses that change precedence count".
    • The quickstart and the merge-policy-vacuity.ts header call parentheses after ? "formatting".
    • A ternary inside ?? (…) is a listed position, yet its parentheses change precedence.
  • Required: either don't drop parentheses after ?? whose content holds a top-level ?:, and add PQ2 and PQ3 as must-merges, or correct the changelog sentence and name this as a limit.

What I ran

All of this ran in detached worktrees at head, d76d96f and main, with TMPDIR set to /var/tmp/r18j-fjbF (mode 700).

  • Head and build (measured): gh api …/pulls/1068 -q .head.sha gives 7f79694 at the start and at the end. tsc is clean at all three commits, and npm run typecheck exits 0 at head.
  • Vacuity set (measured): all 23 merge-vacuity-*, vacuity* and merge-policy-vacuity* files at head: 360 tests, 349 pass, 0 fail, 11 todo. This matches the PR body.
  • CI (measured, gh pr view 1068 --json statusCheckRollup): every check is complete, with 17 success and 8 skipped, nothing failing or pending.
  • Probes (measured): 21 fixtures through the real decided() merge flow, run at all three commits. Beyond the Required items:
    • U1: an unnamed in-place edit (globalThis.f = …) while another file removes a copy of its new text. It merges at head, as at main; it held at d76d96f, so round 5 fixed it.
    • F1 and F9: a tested real edit in a moved or reordered g merges at all three.
    • W1x and W3: both hold at head. W1x is W1 moved to another file; W3 is a multi-line g whose header git keeps.
    • RM1: a tested removal in moved code ([1, 2, 3] to [1, 2]) merges, as at main.
  • Mutants of round 5's code (measured): applied to dist/src/vacuity.js and run against r11-align, r12 and r13 (131 tests, baseline 129 pass, 2 todo). The file's checksum was restored after each.
Mutant Result
Skip removed killed by G1, G1b, D1, D1d
Skip for any name killed by M4b, M4b1, E2d
Unnamed units never skipped survives; my U1 catches it
? dropped from the parenthesis positions killed by Q-Pq

Follow-ups (not blocking)

  • No test for the unnamed half of the skip (measured): add U1 as a must-merge.
  • PQ1 holds, cause not traced (measured result; the cause is a guess): PQ1 is PQ3 with { a: 0, b: false }, so the right side of ?? never runs. It holds at head even with the ? position removed, and merges in place and at main.
  • Whitespace that changes meaning under automatic semicolon insertion (measured): RA1 rewrites moved return u.a; as return followed by u.a; on the next line, which returns undefined. It is tested, and it holds at head, where main merges.
  • F4, a likely correct hold (measured): a one-line f renamed to g in place and edited, while the PR deletes b.js, which held exactly the new g. Head holds and main merges. That's arguably a move from b.js, so I'm not counting it as a false hold.

Probes, the mutant script and logs are in /var/tmp/wl-coord/tmp/rev-r18j/. My three worktrees are removed, the symlink first each time. The vacuity run still going in /var/tmp/wl-coord/rev/r18i belongs to another reviewer, so I left it alone.

@jayden1711

Copy link
Copy Markdown
Owner Author

FAIL 7f79694

Final check R18i: PR #1068 at head 7f796943

Round 5 fixes what R18g and R18h found: G1, G1b, D1 and D1d now merge, and the four source files' docs name the two positions where parentheses are formatting. But one changelog claim is false (I have a measured counterexample), and that's the same subject the last two reviews failed on.

Required

R1. The changelog's precedence claim is false. changelog.d/1014-vacuity-moved-code.md says: "Every other parenthesis counts as code, so parentheses that change precedence count."

  • quiet() treats parentheses opened right after ? as formatting, and that includes ??. Around a conditional, dropping them changes precedence: u.a ?? (u.b ? 1 : 2) is not (u.a ?? u.b) ? 1 : 2.
  • Probe Y2 (measured): g moves to a new file. Its only change is the tested return u.a ?? (u.b ? 1 : 2) → return u.a ?? u.b ? 1 : 2.
    • At the head it holds ("ran none"). With main's source it merges.
    • Y2i is the same edit made in place. It holds at both the head and main.
  • This is a hold where the code is safe, not a wrong merge. The head measures the move the way main measures the same edit in place (inferred). Only the sentence is wrong.
  • Either fix is enough:
    • Wording: drop or qualify the clause, and name this hold next to the other new false holds.
    • Code: don't treat parentheses after ?? as formatting when they hold a top-level ?, = or =>. Add Y2 as a must-merge test with a mutant.

The quickstart and the headers of merge-policy-vacuity.ts and vacuity.ts don't make this claim and match the code (inferred, from reading).

The three weakening flags: none relaxes a check

  1. test/merge-vacuity-r12.test.ts: a strengthening. R1, R3 and R4 go from limit: todos to must-hold tests with the same assertions. The file is unchanged since d76d96f. All three pass at the head and fail with main's source (measured).
  2. src/coordinator/merge-flow.ts: a strengthening. It now also passes the removed line spans of other app files (elsewhere), and only the pairing uses them.
    • A file counts only when it counts both with and without the pairing, so pairing can only cause more holds (inferred, from the code).
    • The measure without pairing is a refactor of main's (inferred).
    • The file is unchanged since d76d96f (measured).
  3. The two limit: todos in test/merge-vacuity-r13.test.ts: recorded limits that main shares. The unbalanced-file exports.g case and X1 both merge at the head, at d76d96f and with main's source. Their controls hold at the head (measured).

What I ran (measured unless labelled)

  • PR head: still 7f796943 at the start and end of the review.
  • Build and npm run typecheck: clean.
  • Vacuity suites (merge-vacuity-*, vacuity, merge-policy-vacuity): 360 tests, 349 pass, 0 fail, 11 todo, matching the PR body.
    • Two merge-vacuity-r10-shutdown tests failed inside my sandbox. They fail the same way with main's source, the PR doesn't touch that file, and they pass outside the sandbox.
  • Other merge-policy* suites: 91/91 pass.
  • Fail-before: on d76d96f's source with the head's tests, exactly G1, G1b, D1 and D1d fail.
  • Mutant: dropping the same-name condition fails exactly M4b, M4b1 and E2d, as claimed. I didn't re-run the other six mutants the PR names.
  • Probe Y1: a tested g moved over a different, same-named g in an existing file, with an untaken branch edited, merges at the head, at d76d96f and on main. Round 5 didn't introduce it, and it matches D1's must-merge. Y1c, the same move into a file with no g, holds at the head.
  • Merge onto main: git merge-tree --write-tree cc34fd79 7f796943 is clean. On that merged tree, adversarial.test and merge-door.test give 242 tests, 218 pass, 0 fail, 24 todo.
  • CI at the head: every check that ran reports SUCCESS, including all eight test shards, test on Node 22 and 24, push-gate, denylist, dco and scan. The real-systemd and bootstrap jobs were skipped. That they aren't required is a guess; I didn't read the ruleset.
  • Not re-run locally: check:denylist and push-gate --range. CI's denylist and push-gate jobs passed.

Optional follow-ups

  • Mention Y1 in the docs' in-place sentence: a move onto a same-named statement in the destination reads as an in-place edit.
  • A multi-line return (\n x\n) with its parentheses dropped changes meaning through automatic semicolon insertion, but would read as formatting. That would cause a hold, not a merge (inferred, not probed).

My worktree is still at /var/tmp/wl-coord/rev/r18i, detached, with only an untracked node_modules symlink. Scratch copies are in /var/tmp/r18i-AZGG. Nothing is left running, and I made no commits, pushes or GitHub posts.

…kept, keep precedence parentheses after ?? (#1014)

A moved function that git pairs in place with a stale same-named one it
replaces (N5, N6) no longer skips the move pairing: the in-place skip now
needs a code-character similarity of at least 0.8, and the removals git
measured inside a placed unit go with it. Parentheses after ?? whose
content holds a lower-precedence operator count as code (PQ2, PQ3).
W1, W2 and a large in-place rewrite to a removed copy's text are pinned
as limit todos and documented.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

FAIL 51538c8

Final check R18k: PR #1068 at head 51538c8b

Round 6 fixes what R18i and R18j found. N5, N5x and N6 hold. G1, G1b, D1, D1d, U1, PQ2 and PQ3 merge. The ?? rule in the code matches its docs word for word. Suites, typecheck, fail-before, mutants, the merge onto main and CI are all green.

One gap is left. The docs state the in-place rule without its budget fallback, and I have a measured counterexample: a shape the docs say holds merges. That is the same subject the last two reviews failed on.

Required

R1. The in-place rule's docs leave out its fallback once the cell budget is spent (measured).

  • The code: edited() adds up nearCells across the whole PR. Once that passes MOVE_PAIR_CELLS (32M cells), it returns "kept": every later unit that git pairs with a same-named statement counts as edited in place, whatever its similarity.
    • Only the code comment says so ("past the cells, kept").
    • The quickstart, the changelog fragment and the merge-policy-vacuity.ts header say, with no condition, that one keeping under 0.8 "took that statement's place and pairs like any moved statement". The header also calls N5 and N6 "must hold".
  • Probe B3: N5, plus src/a.js with ten one-line tables of about 1,950 characters, each edited in place.
    • Head: merges (auto: true).
    • Main: merges.
  • Control B3c: the same with three tables, under the budget.
    • Head: holds ("ran none").
    • Main: merges.
  • Impact:
    • This is no worse than main, but it is a documented-held shape that merges, and it fails open. The gate's other budgets make the PR wait.
    • It is rare (inferred): every statement must be under the per-hunk alignment cap, or B2's "too large to align" hold fires first. So it takes about nine or more such in-place rewrites, or a few big fully rewritten statements, in the same PR as an N5 shape.
  • Either fix is enough:
    • name the fallback as a limit in the quickstart, the changelog and both headers;
    • or treat it as "not kept", so the unit pairs and the PR can only hold, and pin B3 as a must-hold.

Weakening flags (owner-delegated question for this head)

weakeningOf(base 32cd15e5, head 51538c8b, tip cc34fd79) reports the same three flags (measured).

  1. Changed test lines in test/merge-vacuity-r12.test.ts: a strengthening.
    • R1, R3 and R4 go from limit: todos to must-hold tests with the same assertions.
    • All three pass at head and fail with main's source (measured).
    • The file is unchanged since 7f79694.
  2. Changed guard lines in src/coordinator/merge-flow.ts: a strengthening.
    • The file also sends the removed line spans of other app files (elsewhere), and only the pairing reads them.
    • A file counts only where it counts both with and without pairing, so pairing can only add holds (inferred, from the code).
  3. Added limit: todos in test/merge-vacuity-r13.test.ts. All five fail as todos at head (measured):
Todo Asserts Head Main What it is
unbalanced-file exports.g hold merges merges limit main shares
X1 hold merges merges limit main shares
W1 hold merges merges limit main shares
W2 hold merges merges limit main shares
rewritten in place past EDITED_IN_PLACE merge holds ("ran none", src/a.js, src/b.js) merges new hold, fails closed

None of the five relaxes a check.

The new hold is a rare shape, not an ordinary one (measured probes, inferred judgement). It fires only when two things are both true:

  • A statement whose code lines are all new is rewritten in place below 0.8 similarity.
  • The same PR removes another copy elsewhere that matches its new text exactly, ignoring formatting.

The probes at head:

  • Merge, as at main:
    • O1: a one-line rewrite in place, tested.
    • O2: O1, with a deleted file holding an unrelated f.
    • O3: a multi-line rewrite whose header also changed.
    • O4: O3, with an unrelated same-named f deleted.
    • L1: the limit shape with a multi-line f whose header git keeps.
    • L2: the limit shape where the copy differs in tested code.
    • L3: the limit shape with the copy kept, not removed.
  • Holds: only L0, the one-line exact-copy de-duplication.

That shape can't be told apart from N5, a move over a stale same-named function, and it matches the PR's stated change that a PR which only moves code holds.

What I ran

Labels: measured unless marked inferred.

  • PR head: 51538c8b at the start and the end.
  • Merge base: 32cd15e5. The API's base.sha reads 04f83b51.
  • Build and npm run typecheck: clean.
  • Vacuity suites (merge-vacuity-*, vacuity, merge-policy-vacuity): 369 tests, 355 pass, 0 fail, 14 todo. This matches the PR body.
  • Other merge-policy* suites: 91/91 pass.
  • Fail-before: I ran the head's r11-align, r12 and r13 tests over each commit's source.
    • At 7f79694: exactly N5, N6, PQ2 and PQ3 fail.
    • At main cc34fd7: 66 fail, including N5, N5x and N6. PQ2 and PQ3 pass there.
    • At head: 135 pass, 0 fail, 5 todo.
  • Mutants of compiled vacuity.js: all killed, as claimed.
Mutant Killed by
threshold 2/3 N5
threshold 0.96 G1, G1b, D1, D1d, U1
loose never set PQ2, PQ3
removals not dropped N6
  • Rule statements: read against quiet and movedCode.
    • The ?? rule and the opener and closer lists match the code exactly (inferred).
    • p === '?' can only qualify after ??, because a ternary's ? needs a : before the close (inferred).
    • Probe P1 (a lone ?? (u.b) dropped) holds as formatting.
    • Probe P2 (?? (u.b == null) dropped) counts as code, as documented.
    • The in-place rule matches the code except for R1.
  • PR body similarity figures (0.95, ≥ 0.96, 0.67, 0.43): consistent with the mutant thresholds. I didn't compute them myself (inferred).
  • Merge onto main: git merge-tree --write-tree cc34fd79 51538c8b is clean (tree 575d0b33). On that tree, adversarial.test and merge-door.test give 242 tests, 218 pass, 0 fail, 24 todo.
  • CI at head: every check is complete, 20 success and 7 skipped (bootstrap and the real-systemd jobs). Nothing is failing or pending.
  • Sign-off: all 6 commits are signed off.
  • Not re-run locally: check:denylist and push-gate. CI's denylist and push-gate jobs passed.

Optional follow-ups

  • Changelog scope: its new Limit: sentence says "a statement rewritten in place". It applies only when every code line is new (L1 merges). That overstates when the hold happens, not when code merges, so it's harmless.
  • Already-disclosed holds: PQ1, PQ1r and RA1 still hold at head where main merges. Track them as the issues the body promises.

My probes are in /var/tmp/r18k-9sYd/zz-r18k-probes.test.ts, with probes-*.txt, fb-*.tap, mut-*/out.tap, vac-head.tap and adv.tap beside them. The detached worktree /var/tmp/wl-coord/rev/r18k is left in place, with an untracked node_modules symlink and probe file. Nothing is running, and I made no commits, pushes or GitHub posts.

@jayden1711

Copy link
Copy Markdown
Owner Author

Second review (adversarial): FAIL 51538c8

The EDITED_IN_PLACE check has a fallback the docs don't mention. When alignChars returns null, or a cell budget shared across the whole PR is spent, edited() returns true ("kept"). So N5, a moved g over a stale same-named g, merges at head in two measured ways. One of them held at d76d96f, so this is a regression (bars b and d). The sentences stating the rule also claim more than the code does (bar a). Nothing else failed: no false holds against main beyond those already listed, every earlier fix holds, suites and typecheck pass, and CI is green.

Bar applied: the owner's bar for this PR. FAIL only on (a) a docs, changelog or PR-body sentence that claims more than a case proves, (b) a shape the docs say is held that merges, (c) a clear false hold on an ordinary shape main merges, or (d) a shape that held at d76d96f or 7f79694, is documented as held, and now merges.

Required

  1. The shared budget lets N5 merge, and it held at d76d96f (bars b and d; measured).

    • The cause: nearCells is never reset between units, so it adds up across the whole PR. Once it passes MOVE_PAIR_CELLS (32M), every later same-named pair git paired in place is "kept", whatever its similarity.
    • The probes: all are N5 plus one large in-place change in a file that sorts before c.js.
    Probe The extra change Head d76d96f 7f79694 / main
    BUDf export function build(o) with 200 lines o.fieldN = o.fieldN ?? defaults.fieldN;, the parameter renamed o → options in place merges holds merge
    BUD120, BUD160 a 120- or 160-line export const TABLE = [...] switched from ' to " and wrapped in Object.freeze(...) merges holds merge
    • Controls:
      • BUD40, BUD80 and BUD110 hold at head, so the size threshold matches the budget.
      • BUDz, the same table in z.js, which sorts after c.js, holds, so path order matters.
      • BUDq (quotes flipped, header line unchanged, so the table is no unit) holds.
      • BUDc (an unrelated untested edit instead) holds.
    • Causal mutant: making the budget per pair (nearCells += → nearCells =) makes every BUD variant and BUDf hold. r11-align, r12 and r13 still pass under it (0 real failures, 5 existing todos), so no test pins this.
  2. One large pair does the same (bar b; measured).

    • BL1wb and BL2wb: a 110-line g is moved over a 110-line stale g(w) whose body differs on every line. The unit is wholly added and its similarity is below 0.8. But the changed middle is over ALIGN_CELLS, so alignChars returns null, which is read as "kept".
    • Both merge at head, d76d96f, 7f79694 and main. At d76d96f the route is N6's, so this is not a regression.
    • With the mutant ops === null || → ops !== null &&, both hold, and the suites still pass.
  3. The docs claim more than the code does (bar a; read). Three places say a statement that keeps less than 0.8 "took that statement's place and pairs like any [moved statement / other]": the quickstart, the changelog fragment, and the merge-policy-vacuity.ts header ("N5, N6: must hold"). Items 1 and 2 contradict this. Only a code comment says "past the cells, kept".

    • Fix (inferred): treat null, whether from the budget or from alignChars, as not kept. That only takes measure away, so the both-ways rule still holds. Then add BUDf and BL1wb as must-holds.
    • Or name both as limits everywhere the rule is stated.
  4. The changelog's own example overclaims (bar a; measured). It says "(a stale same-named function replaced by the one moved there) … pairs like any other". N5u is N5 with the stale function's signature export function g(u) { matching the moved g's.

    • Git keeps that header line, so the moved g is never a wholly added unit.
    • It merges at head, d76d96f, 7f79694 and main, so it isn't a regression. But it is the commonest form of the shape that example describes.
    • Name it as a limit, or narrow the sentence.

The weakening flags

None of them relaxes a check.

  • test/merge-vacuity-r12.test.ts: a strengthening. R1, R3 and R4 go from todo to must-hold with byte-identical assertions, and the file is unchanged since 7f79694 (measured, diff).
  • src/coordinator/merge-flow.ts: a strengthening. It only adds elsewhere, which only the pairing uses, and pairing only takes measure away (inferred). It is unchanged since 7f79694 (measured).
  • The r13 limit: todos: I ran head's r13 and r12 against main's source.
    • W1, W2, X1 and the unbalanced-file case merge at main too. These are limits main shares (measured).
    • The "rewritten in place past 0.8 to a removed copy's text" todo merges at main but holds at head. That is a stricter false hold, not a relaxation (measured).

What I ran

Head, main cc34fd7, d76d96f and 7f79694 each had their own detached worktree, built with tsc. All four worktrees are removed, the symlink first.

  • Head (measured): 51538c8 at the start and the end. The four PR source files are unchanged between the merge base and main.
  • Vacuity set at head (measured): 23 files, 369 tests: 355 pass, 0 fail, 14 todo. This matches the PR body.
  • Typecheck (measured): npm run typecheck exits 0 once my copied probe files are out of test/.
  • CI (measured, gh pr view --json statusCheckRollup): 20 success, 7 skipped, none failing.
  • Earlier probe sets (measured): r18d, r18e, r18f, r18g, r18h and r18j, copied unmodified, at all four commits.
    • Head merges where an earlier head held only for G1, G1b, D1, D1d, U1, PQ2 and PQ3. Each of those is meant to merge.
    • Head holds where an earlier head merged only for N5 and N6, which are meant to hold. So no earlier probe regressed.
    • Head against main: every difference is head holding where main merges. The ones meant to merge are PQ1, PQ1r, RA1 and F4, which the PR body already lists, and B2s and B22, which were "any". There is no new false hold.
  • New probes (measured): TN1, TN2, RP1 and RP2 (tiny or threshold in-place rewrites, and a parameter rename) merge. TN5, TN5b, N7u and BN5-mid hold. RN1 and RN2 (renamed as moved, a listed limit) merge.
  • Mutants of round 6 (measured): these survive the suites:
    • the budget made per pair, and null read as not kept (both described above);
    • inOne using some instead of every;
    • loose matching only ?, and loose without ?. Both are harmless, because ? and : each mark a ternary (inferred).

Follow-ups (not blocking)

  • N7 (measured): g moved over a twin in c.js that differs only by formatting merges at head, 7f79694 and main, and held at d76d96f. The stated ≥ 0.8 rule covers it, but the docs name only the within-file case (W1, W2).
  • Tests to add: something that kills the inOne some mutant (measured survivor), and pins for the null and budget paths.
  • Existing limits: PQ1, PQ1r, RA1 and F4 still hold where main merges.

Probes and logs are in /var/tmp/r18l-NQu0/:

  • Probe files: probes/zz-r18l*.test.js.
  • Probe logs: p-<worktree>-<set>.log.
  • Mutant logs and script: mut-*.log, mut.sh.
  • Vacuity run: vac-head.log.

I made no commits, pushes or posts, and nothing is left running.

…1014)

The in-place check aligned against a PR-wide cell budget and read both
a spent budget and an alignment too large to make as kept, so one big
in-place change anywhere earlier in the PR (BUDf, BUD120, B3), or a
big moved function over a stale one (BL1wb), skipped the move pairing
and merged. Each pair now aligns on its own, bounded as codeRanges
bounds each hunk, and one it can't align pairs like any moved unit.
D1p pins the per-pair choice; N5u is pinned as a limit todo, and the
docs state the fallback and when the rewrite limit applies.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Independent final check (R18m), posted by the coordinator.

FAIL ce7be5f

Final check R18m: PR #1068 at head ce7be5fa

Round 7 fixes what R18k and R18l found. edited() now aligns each pair on its own, and a pair too large to align is never counted as edited in place. BUDf, BUD120, B3 and BL1wb hold, and D1p and BIGIN merge. Suites, typecheck, fail-before, the claimed mutants, the merge onto main and CI are all green.

One gap is left, in the same class as the last two failures. The module's other PR-wide budget (MOVE_PAIR_CELLS, used to choose between same-named pairs) still fails open. Once it is spent, two documented must-holds merge.

Required

R1. The same-name scoring budget fails open, so Z-N1 and Z-N4 merge (bars a and b; measured).

  • The code: the loop that scores same-name candidates adds up cells across the whole PR. Past MOVE_PAIR_CELLS (32M), every later contested pair scores -Infinity, so candidates are taken in path order.
    • Only the code comment says so ("past it: path order").
    • The changelog says, with no condition, "the closest same-named pair first, so a same-named statement elsewhere never takes another's source". The quickstart and the merge-policy-vacuity.ts header say "the closest same-named pair first", and the header lists Z-N1 to Z-N4 as must-holds.
  • Probe ZN1spent(n): Z-N1, plus src/0.js with n moved functions of about 2,000 code characters each. Each has two same-named sources, in p.js and q.js, and differs from both by one constant. The PR changes nothing else.
Probe Head 51538c8 7f79694 d76d96f Main d5650da
n = 0, 1, 2, 3 holds holds holds holds merges
n = 5, 8 merges merges merges merges merges
ZN4spent, n = 5 merges merges merges merges merges
  • Causal mutant: with MOVE_PAIR_CELLS = Infinity, every variant holds. r11-align, r12 and r13 still pass under it (143 pass, 0 fail), so no test pins this.
  • Not a regression: it is the same at every head since d76d96f, and main merges all of these. The budget is also spent cheaply: it counts the full key product, while the near-identical pairs actually align in only a few cells.
  • Either fix is enough (inferred):
    • name the fallback as a limit in the quickstart, the changelog and both headers, and narrow "never takes another's source";
    • or make it fail closed (throw so the PR waits, or score each pair on its own as edited() now does, with the reader's 120 s limit as the backstop), and pin ZN1spent n=5 as a must-hold.

Weakening flags (owner-delegated question for this head)

weakeningOf(base 32cd15e5, head ce7be5fa, tip d5650da8), run with main's scanner, reports the same three flags (measured). None of them relaxes a check.

  1. Changed test lines in test/merge-vacuity-r12.test.ts: a strengthening.
    • R1, R3 and R4 go from limit: todos to must-holds with identical assertions (measured, diff).
    • All three pass at head and fail with main's source (measured).
  2. Changed guard lines in src/coordinator/merge-flow.ts: a strengthening.
    • The file only adds elsewhere: the removed line spans of app files the PR doesn't measure (measured, diff).
    • Only movedCode reads it. measureTouched counts a file only where both codeOf and movedOf count it, so pairing can only take measure away (inferred, from reading the code).
  3. Added limit: todos in test/merge-vacuity-r13.test.ts. Head's r13 todos, run against head's source and against main's (measured):
Todo Asserts Head Main What it is
unbalanced-file exports.g hold merges merges limit main shares
X1 hold merges merges limit main shares
W1 hold merges merges limit main shares
W2 hold merges merges limit main shares
N5u (new this round) hold merges merges limit main shares
rewritten in place past EDITED_IN_PLACE merge holds ("ran none", src/a.js, src/b.js) merges a stricter hold, which fails closed

What I ran

Labels: measured unless marked inferred.

  • PR head: ce7be5fa at the start and at the end. Main is d5650da8, and the merge base is 32cd15e5.
  • Build and npm run typecheck: clean.
  • Vacuity suites (merge-vacuity-*, vacuity, merge-policy-vacuity): 378 tests, 363 pass, 0 fail, 15 todo. This matches the PR body.
  • merge-policy* suites: 104/104 pass.
  • Fail-before: head's r11-align, r12 and r13 tests over 51538c8's compiled reader give 149 tests, 139 pass, 4 fail: exactly BUDf, BUD120, B3 and BL1wb. D1p and BIGIN pass both there and at head.
  • The PR's mutants:
    • "too large to align counts as kept" is killed by BL1wb only.
    • "PR-wide budget that fails closed" is killed by D1p only.
  • The inOne equivalence argument (inferred, from reading codeRanges, ranOn, innerAt and ranSegments): sound.
    • A removal's need counts only when both of its sides are in ran ranges outside the module's top level, and share an innermost V8 range in every process.
    • A need that straddles a unit boundary has its sides in two different top-level statements, so it can never count. Dropping it (some) or keeping it (every) gives the same verdict.
    • One caveat: this relies on topUnits cutting at real top-level statements, which is true for balanced readings.
  • Per-pair cost: I called measureTouched directly on N same-named functions, each rewritten in place, with about 3.97M cells per pair (just under ALIGN_CELLS):
Pairs File size Head 51538c8
200 0.58 MB 1.5 s 0.97 s
1,000 2.9 MB 8.4 s 7.6 s
2,000 5.9 MB 26.6 s 28.6 s
  • Per pair, the cost is about 10 ms at the limit. Pairs just over the limit return null at once, which is cheaper.
  • So it stays well under the reader's 120 s limit, and past that limit the read errors and the PR waits (inferred, from readTouchedInChild). The PR body's "costs about what the module already spends per hunk" is consistent with this.
  • Other budgets in the module:
    • MOVE_PAIR_CELLS in the scoring loop fails open (R1).
    • The source-map limits stop at the map files; past the source count it throws, so it holds.
    • Each alignChars null in codeRanges either holds (UNPLACEABLE) or, for moved units, is measured as git paired it, which the both-ways rule keeps from merging anything (inferred).
  • Docs:
    • The rule's wording matches the code: the 0.8 similarity, a pair too large to align never counted as edited in place, and "about 2,000 code characters a side" (√4M, of the middle).
    • N5u is named as a limit in the quickstart, the changelog and both headers.
    • The narrowed rewrite-limit sentence matches L1 from R18k.
    • The exception is R1.
    • I did not re-run the PR body's "Head vs 51538c8: no other difference" probe set.
  • Merge onto main: git merge-tree --write-tree d5650da8 ce7be5fa is clean (tree 78c9b9de). I exported that tree, built it, and ran:
    • adversarial/adversarial, adversarial/merge-door and merge-policy-tip-merged-in: 255 tests, 231 pass, 0 fail, 24 todo;
    • r12, r13 and tip-merged-in: 0 fail.
  • CI at head: every check is complete, 20 success and 7 skipped. Nothing is failing or pending.
  • Sign-off: all 7 commits are signed off.
  • Not re-run locally: check:denylist and push-gate.

Optional follow-ups

  • A test that pins the scoring budget's behaviour, whichever fix is taken.
  • The scoring budget counts full key products, not the cells actually aligned. If the budget stays, counting the middle (after the common start and end) would make it far harder to spend.
  • Already-disclosed holds at head where main merges (PQ1, PQ1r, RA1, F4) and the known limits (W1, W2, N5u, the B7 and B8 parenthesis shapes) are unchanged.

Every worktree I added is removed, and nothing is running. I made no commits, pushes or GitHub posts.

@jayden1711

Copy link
Copy Markdown
Owner Author

Independent adversarial review (R18n), posted by the coordinator.

Second review (adversarial): FAIL ce7be5f

Bar (owner decision, applied to this PR): FAIL only if one of these holds:

  • (a) a docs, changelog or PR-body sentence claims more than a case proves;
  • (b) a shape the docs say is held merges;
  • (c) a clear false hold on an ordinary PR shape that main merges;
  • (d) a regression of a documented hold;
  • or a crafted PR makes the check hang or crash in a way that merges.

Round 7's own fix is sound. One fail-open path is left in the same pairing code: the last budget shared across the whole PR.

Required

1. The by-name scoring budget lets a decoy take the source, so Z-N1 and Z-N4 merge (bars a and b; measured).

  • Cause: in movedCode, the loop that scores ambiguous same-named pairs adds cells across the whole PR. Past MOVE_PAIR_CELLS (32M) every score is -Infinity, so pairs are taken in path order. The code comment says so ("past it: path order"). The docs don't.
  • The docs' claims:
    • The changelog says "the closest same-named pair first, so a same-named statement elsewhere never takes another's source."
    • The quickstart and the merge-policy-vacuity.ts header say "the closest same-named pair first", and the header lists Z-N1 to Z-N4 as must-holds.
  • Probes: each spends the budget first with two ambiguous one-line statements named t, of about 4,500 code characters each. They are in new files src/01.js and src/02.js, against one t removed from src/0t.js. That is about 9 KB, well within the default 800 lines and 20 files.
Probe Shape Head 51538c8 d76d96f main
ZN1 Z-N1 (r13's fixture) hold hold hold merge
ZN1B Z-N1 plus the two t statements merge merge merge merge
ZN4B Z-N4 plus the two t statements merge merge merge merge
ZN1Bs (control) the t statements about 300 characters hold hold – merge
ZN1Bz (control) the decoy in z.js, sorting after hold hold – merge
M3B (control) M3 alone plus the big t statements hold hold – merge
  • Causal mutant: with cells > MOVE_PAIR_CELLS ? null changed to false ? null, ZN1B and ZN4B hold. With the budget removed, r11-align, r12 and r13 still all pass, so no test pins this path.
  • Not a regression (bar d): this has been there since d76d96f. It is the same class as R18k's B3, which was Required.
  • Either fix is enough:
    • make a spent budget fail closed (the PR waits) or drop the budget, and add ZN1B and ZN4B as must-holds;
    • or name the fallback as a limit in the quickstart, the changelog and both headers, and soften "never takes another's source".

Weakening flags (owner-delegated question)

None of them relaxes a check.

  • test/merge-vacuity-r12.test.ts: a strengthening. R1, R3 and R4 go from limit: todos to must-holds with the same assertions, and the file is unchanged since 7f79694 (measured, diff). All three fail with main's source (measured).
  • src/coordinator/merge-flow.ts: a strengthening. It only adds elsewhere, which only the pairing reads, and a file counts only where it counts both ways (inferred, from the code). It is unchanged since 7f79694 (measured).
  • The r13 limit: todos (measured: head's r13 run against each commit's source):
    • The unbalanced-file case, N5u, W1, W2 and X1 all assert a hold and merge (auto: true) at main and at 51538c8. These are limits main shares.
    • "Rewritten in place past EDITED_IN_PLACE to a removed copy's text" asserts a merge. It holds at head and 51538c8, and passes at main. That is a stricter hold that fails closed, not a relaxation.

What I ran (measured unless marked)

  • Head and CI:
    • gh api …/pulls/1068 -q .head.sha gave ce7be5f at the start and the end.
    • CI at head (statusCheckRollup): 18 success, 7 skipped, none failing or pending.
    • git merge-tree --write-tree d5650da8 HEAD is clean (tree 78c9b9de).
    • None of the PR's files changed on main since the merge base 32cd15e. So I didn't run the suites on the merged tree (inferred safe).
  • Head suites: the 23 merge-vacuity-*, vacuity* and merge-policy-vacuity* files give 378 tests: 363 pass, 0 fail, 15 todo. This matches the PR body. npm run typecheck exits 0.
  • Fail-before: head's r12 and r13 tests with 51538c8's source fail exactly BUDf, BUD120, B3 and BL1wb. With main's source, 72 fail.
  • Earlier probe sets: r18d, e, f, g, h, j, k, l, l2 and l3 (copied, unmodified) at head, 51538c8 and main.
    • Head against 51538c8: only BUD, BUD120, BUD160, BUDf, BL1wb, BL2wb and B3 differ, all going from merge to hold. That matches the PR body. C4 differs only in a timing string.
    • Head against main: 79 entries differ, and head never merges where main holds.
      • 29 assertion-style probes pass at head and fail at main.
      • The other 50 hold at head and merge at main, and every one is already named in the PR body: the wanted holds, B2s and B22 ("any"), the documented rewrite limit L0, and PQ1, PQ1r, RA1 and F4 (disclosed follow-ups).
    • No earlier documented hold regressed.
  • Performance (one heavy process at a time, under timeout and nice):
    • PERF1, 399 one-line same-named pairs git puts in place, each about 1,990 code characters a side (800 lines): head 13.3 s, main 10.3 s, both merge. So per-pair alignment adds about 30%, which supports the body's inferred cost claim. At 50 pairs: head 1.8 s, 51538c8 1.7 s, main 1.3 s.
    • PERF2, Q functions named t added in one file and Q removed from another, plus a tested real change. The by-name loop's named.filter grows much faster than linearly:
Q Head Main Head's result
50 0.4 s 0.6 s merge
150 3.3 s 0.5 s merge
399 (800 lines, default limits) 120.5 s 0.5 s holds: "the coverage directory took longer than 120 s to read"
  • So the timeout fails closed: the reader child is killed and the PR waits (measured). From the code, crashes and errors also come back as { error } (inferred).
  • Mutants of round 7's code (compiled vacuity.js, against r11-align, r12 and r13; checksum restored after each):
Mutant Result
null alignment counted as kept killed by BL1wb
PR-wide budget, failing closed killed by D1p
51538c8's rule killed by BUDf, BUD120, B3 and BL1wb
by-name scores always path order killed by Z-N1, Z-N3 and Z-N4
per-pair cap of 1M cells in edited() survives
by-name budget removed survives
  • Docs check (inferred, from reading):
    • The in-place rule is stated correctly in all four places: same name (or both unnamed), similarity of at least 0.8, too large to align means never edited in place, each pair on its own. The quickstart leaves out the unnamed case.
    • "About 2,000 code characters a side" is right for equal sides: alignChars returns null when (a+1)(r+1) > 4,000,000 after trimming the shared start and end. For unequal sides it is the product that counts. Where that matters, one side is below 2/3 of the other, so the similarity can't reach 0.8 anyway.
  • Cleanup: my four worktrees are removed (symlinks first), and nothing of mine is running. A vacuity-read process from another reviewer's r18m worktree was running; I left it alone. No commits, pushes or posts.

Follow-ups (not blocking)

  • M3S (measured): M3 with its source in a base file over 8 MiB (SCRIPT_BYTES) merges at head, because the file is skipped as a source. Its control holds. The PR body has disclosed this since round 2, but the docs don't. It needs a huge file already at the base, so a single PR can't craft it.
  • PERF2 (measured): a false hold on a crafted 800-line shape that main merges in 0.5 s. It fails closed. This is Vacuity gate: bound the cost of a crafted large hunk and cap coverage runs across PRs (follow-up to #903) #1016's subject.
  • Surviving mutants: pin the "about 2,000" threshold (the 1M-cap mutant) and the by-name budget path (fixing item 1 does this).
  • Quickstart wording: the in-place sentence leaves out "(or, unnamed, an unnamed one)".
  • Disclosed holds: PQ1, PQ1r, RA1 and F4 still hold where main merges.

The by-name scoring budget was shared across the PR: once big contested
pairs elsewhere spent it, every score tied and candidates paired in path
order, so a same-named unit could take another's source (Z-N1, Z-N4).

Each contested pair is now compared on its own: by character within the
alignment bound, else by its lines' text; when even the lines are too
many, the coverage read fails and the PR waits. Counting contested
pairs with a map also removes the quadratic filter over candidates.

Tests: ZN1spent, ZN1B, ZN4B and ZN1L must hold; ZN1Bs, ZN1Bz and M3B
stay holds; D1L (a large in-place edit) must merge.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Independent adversarial review (R68p), posted by the coordinator.

VERDICT: FAIL @35d11f5610d7771779e9acf4a6a2b85233f2ad8a (adversarial)

Required

R1. The line fallback scores raw lines, so formatting the docs call "no change" lets a small decoy take the source. ZN1L plus Q-S or a comment merges (bars a and b; measured).

  • Cause (from reading closeness in movedCode): the score by character uses key, which is code only: no comments, no formatting. The score by line uses u.lines, the raw text with only whitespace removed, so semicolons and comments still count there.
    • A 122-line g moved with its semicolons dropped, a no-change per the docs and Q-S, therefore differs from its source on every line. Its line score comes out at about −(both sizes).
    • A two-line decoy g is scored by character, at about −(the source's size), so it wins.
    • The real g is then left unpaired, reads as added code, and the test that runs it merges the PR.
  • Probes: each is ZN1L's frame: a.js loses a 122-line g, and c.js gets it back with return 1 changed to return 2 (an untaken branch) and grant changed to grantAll (an untaken arm). test/x.test.js is M3T, and the decoy is G_DECOY.
Probe Head ce7be5f main e80bac6
SEMI: semicolons dropped, decoy b.js merge merge merge
SEMI: decoy z.js, which sorts after merge merge merge
SEMI-half: semicolons dropped on half the lines merge merge merge
CMT: // checked on each line, decoy b.js merge merge merge
SEMI and CMT controls, no decoy hold hold merge
No formatting, small decoy hold merge merge
  • Why it fails the bar: the changelog says "the closest same-named pair first, so a same-named statement elsewhere never takes another's source… compared on its own, by character or, too large for that, by line". The docs also say semicolons and comments are no change in moved code. ZN1L and Q-S are must-holds. This is not a regression from ce7be5f or main; it is the shape this round claims to handle.
  • Causal fix (measured): score the lines by their code-only, whitespace-free characters, taken from the same code mask as key, instead of lines.replace(/\s/g,''). With that change all of the above hold, and r13's Z-N|ZN|D1L|M3B|Q-S tests pass 15/15. Pin SEMI and CMT as must-holds.

Checked, no finding

  • Head's new tests (measured): r13 Z-N|ZN|D1L|M3B gives 13/13 pass at head. npm run typecheck exits 0.
  • No path-order fallback is left (inferred, from reading): pairs that aren't contested keep score 0, and they never conflict. The +/- indices in the line walk match alignChars(a=u, r=s).
  • Head SHA: 35d11f5 at the start and at the end.
  • CI at head (gh pr checks): 11 pass, 6 pending, 7 skipping.

Follow-ups (not blocking)

  • Cost (measured, the COST probe): n one-line t statements of about 1,900 characters each, added against n removed (M3 base), give n² char alignments of about 3.6M cells each, with no total budget.
n Head ce7be5f main
5 1.6 s 1.6 s 0.8 s
10 2.6 s 2.1 s 0.8 s
20 6.2 s 2.1 s 0.8 s
  • Head holds at every n, correctly.
  • Extrapolating to about 200 or more such pairs within 800 lines, the read passes the reader's 120 s limit and fails closed (inferred). The shape is crafted; this is Vacuity gate: bound the cost of a crafted large hunk and cap coverage runs across PRs (follow-up to #903) #1016's subject.
  • The CoverageUnreadable throw: it needs two contested same-named statements of about 2,000 lines or more. That is past the default max_lines of 800, so default limits already make such a PR wait (inferred). It is disclosed. Not probed.
  • An exact untested copy of g: pairing by code comes first, so the copy takes the source, and the edited, tested g reads as added (inferred; main merges this too). It is a judgement call and predates this round.

What I ran

  • I built a detached worktree at head and added a probe file: a copy of r13 plus the R68P cases, run with --test-name-pattern=R68P.
  • I ran the same probes against ce7be5f's and origin/main's versions of the four source files, swapped in and rebuilt with tsc, then restored them.
  • I applied the line-key mutant to src/vacuity.ts, rebuilt, ran the probes and the r13 subset, and then restored the file.
  • TMPDIR was under /var/tmp. I made no GitHub writes.

@jayden1711

Copy link
Copy Markdown
Owner Author

Independent final check (R68o), posted by the coordinator.

VERDICT: FAIL @35d11f5610d7771779e9acf4a6a2b85233f2ad8a (final check)

Final check R68o: PR #1068, round 8

Round 8 fixes what R18m and R18n found: ZN1spent, ZN1B and ZN4B hold, CI is green, and fail-before is exact. One gap is left. The new line fallback counts comment text, so a decoy can still take another statement's source. That makes the changelog's unconditional claim false again.

Required

R1. When the by-line score is used, a decoy that keeps the source's comments takes the source (bars a and b; measured).

  • Cause (inferred, from the code): closeness falls back to u.lines and s.lines, which are the raw lines with only whitespace stripped. Comments are still in them. The by-character score uses key, which is code only. So when two pairs are too large to align by character, the "closest" pair is decided partly by comments, which the gate otherwise treats as no change.
  • Probe ZN1L-cmt: this is the PR's own ZN1L, with 300 comment lines added at the top of the source g. The decoy in src/b.js keeps those comments, and the real moved g in src/c.js drops them.
Probe Head ce7be5f main
ZN1L-ctl (ZN1L itself) hold merge merge
ZN1L-cmt merge ([true, []]) merge merge
ZN1L-cmtboth (the real g keeps the comments too) hold merge merge
  • Not a regression: this shape merges at ce7be5f and at main.
  • Why it fails anyway: the changelog still says "the closest same-named pair first, so a same-named statement elsewhere never takes another's source", and the header lists ZN1L as a must-hold. This is the same class of finding R18m and R18n failed.
  • Either fix is enough (inferred):
    • score the line fallback on code-only line text (the same mask key uses), and pin ZN1L-cmt as a must-hold;
    • or name comment-driven scoring as a limit and narrow "never takes another's source".

Checked and true (measured)

  • Head: 35d11f56 at the start and at the end (gh api …/pulls/1068). The merge onto main e80bac69 is clean (git merge-tree). Main hasn't touched any of the PR's files since the merge base 32cd15e5.
  • npm run typecheck: passes. check:denylist: 1645 files clean.
  • CI (gh pr checks 1068): 19 pass, 7 skipped.
  • Sign-off: all 8 commits are signed off.
  • r11-align, r12 and r13 at head: 157 tests, 151 pass, 0 fail, 6 todo.
  • Fail-before, with head's r13 run against each source:
    • With ce7be5f's src, exactly ZN1spent, ZN1B, ZN4B and ZN1L fail. The controls, Z-N1 to Z-N4 and D1L pass.
    • With main's src, 12 of the 13 fail. Only D1L passes.
  • R18m's extra probes: ZN1spent with n=8 and ZN4spent with n=5 both hold at head.
  • Mutants of compiled vacuity.js (run against r11-align, r12 and r13, with the checksum restored afterwards):
Mutant Result
PR-wide budget restored, fail-open killed by ZN1spent, ZN1B and ZN4B
no line fallback killed by ZN1L
line score sign flipped killed by ZN1L
no scoring, or `
line score counts only the source side survives
throw changed to -Infinity survives (disclosed: no test reaches the throw)
  • Logic: ways is equivalent to the old named.filter(...).length < 2 test (inferred).

Follow-ups

  • Surviving mutant: the one-sided line score survives (measured).
  • Cost: there is no longer any PR-wide cap. Many contested pairs of about 2,000 characters each cost about 10 ms per pair (R18m's figure) times units × sources. That could hit the 120 s limit, but the timeout fails closed (inferred, not run).
  • Not re-run: the full vacuity set (371 tests), the PR's PERF2 timings, and push-gate.

Cleanup: my worktree and its temporary files are removed. I made no GitHub writes.

…nes (#1014)

The by-line score for a contested same-named pair read raw lines, so
dropped semicolons or comments kept by a decoy let a two-line decoy take
the moved statement's source (SEMI, SEMI-z, SEMI-half, CMT, ZN1L-cmt).
Score each line by its code characters, as `key` reads them.

Every hunk is now also aligned code against code and comment against
comment, not only line pairs whose code matches: before, comments a
statement dropped could stand in for code its untaken branch changed
(CM1 to CM4, which also fail on main; ZN1L-cmt needs this too).

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…nk (#1014)

CM5 is CM3 with a comment on every line of g, the edited line's naming
its new arm: too large to align whole, its lines are paired by code and
the edited pair aligned on its own. Aligned without the code/comment
tags, as 35d11f5 aligned a pair whose code differs, the comment stands
in for the edited code and a test that runs g merges the PR. No test
caught that half of the previous commit before.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Independent final check (R68q), posted by the coordinator.

VERDICT: PASS @08b5e159cfab7b112a18088e80f35a8c63e210d0 (final check)

Final check R68q: PR #1068, round 9

Round 9 fixes both round-8 failures (R68o ZN1L-cmt, R68p SEMI/CMT). Each must-hold case is pinned, fails on 35d11f5 and passes at head. Required CI is green on the exact head.

Findings (all measured unless labelled)

  • Head: 08b5e159… at the start and the end (gh pr view --json headRefOid). Main is 028013d6.
  • Fail-before / pass-after: I used two detached worktrees, one at head and one at head with git checkout 35d11f56 -- src, built each with tsc, and ran --test-name-pattern='SEMI|CMT|ZN1L|ZN4L|CM[0-9]' on merge-vacuity-r13. With 35d11f5's src, 10 of 19 fail: SEMI, SEMI-z, SEMI-half, CMT, ZN1L-cmt and CM1 to CM5. Each fails with actual true. ZN1L, ZN4L, the SEMIc/SEMI-halfc/CMTc controls and CM1t/CM3t/CM4t/CM5t pass there. At head, 19/19 pass. This matches the body exactly. Only src/vacuity.ts and src/merge-policy-vacuity.ts differ from 35d11f5 in src.
  • Tests are meaningful: each must-hold asserts [auto, reasons] against a specific hold(...), not just auto (read).
  • Extra probes (mine): in ZN1L's frame, with the decoy in b.js except where noted:
    • PAREN (precedence-free parentheses after =), BLOCK (/* */ on each line) and MIXz (semicolons dropped plus comments, decoy in z.js) hold at head and merge on 35d11f5's src.
    • MIXc, the no-decoy control, holds at both.
    • So "formatting and comments never decide it" holds beyond the pinned cases.
  • No unrelated files: the diff from the merge base 32cd15e5 has 8 files. src/git-remote.ts and src/inbox.ts have 0 diff lines. The merge tree's copies of both have main's blob IDs.
  • Merge with main: git merge-tree --write-tree origin/main 08b5e159 exits 0 (tree c6fc5bde). Main has not touched any of the PR's files since the merge base.
  • Docs and changelog:
    • The quickstart says "code only against code and comment only against comment". alignHunk now tags the whole hunk, unequal-count runs and line pairs, so this is true (read).
    • The line score uses rows, built from quiet(text, lex.mask), the same mask as key. That mask is the formatting definition the changelog gives (read).
    • The changelog's "Before…" bullet is true against main: main's alignHunk aligned the whole hunk untagged (git show origin/main:src/vacuity.ts).
  • Body: the round-9 claims I re-ran are true.
    • Minor: check:denylist gives 1645 files clean here, not 1647. The script counts git ls-files -co, untracked files included, so the count depends on the worktree (inferred). Not material.
  • Checks: npm run typecheck exits 0. r11-align, r12 and r13 at head: 175 tests, 169 pass, 0 fail, 6 todo. All 10 commits are signed off.
  • CI (gh pr checks 1068): 19 pass, 7 skipped, 0 fail or pending. Runs 40306482575 (ci) and 40306482693 (ci slow) are on head_sha 08b5e15 with conclusion success (gh api).
  • Weak scan (weak-scan.mjs, 3 reasons), all benign:
    1. r12 lines changed: R1, R3 and R4 go from todo limits to must-holds, which strengthens them.
    2. Guard lines changed in merge-flow.ts: changedHunks also returns elsewhere, which is additive, and hunks is passed as before.
    3. Six todo tests added in r13: named limits that merge as on main (E3 no-balance, N5u, the rewrite past EDITED_IN_PLACE, W1, W2, X1). Each is documented, and none replaces an existing assertion.

Not re-run

The full 23-file vacuity set (404 tests), npm test (CI is authoritative), push-gate, PERF2/COST timings, and R68o's surviving one-sided-score mutant.

Follow-ups (not blocking, carried over)

Worktrees and TMPDIR removed. No GitHub writes.

…aks never decide (#1014)

Too large to align by character, a contested same-named pair was scored
by its line rows, so a moved g re-wrapped on half or more of its lines
counted every wrapped line as changed and a two-line decoy g took its
source (R68r: WRAP-1of2, WRAP-2of3, WRAP-3of4, before and after the
decoy in path order). Score the tokens of `key` instead (word runs and
punctuation runs, each weighing its characters): `key` holds no
whitespace, so a line break never changes them.

ZN4L-S, ZN4L-U and ZN4L-N pin each side's unkept tokens and their
weight. The quickstart and changelog name a statement re-wrapped on
every line, or with pairs of lines joined, under the too-far-to-align
limit.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Independent adversarial review (R68r), posted by the coordinator.

VERDICT: FAIL @08b5e159cfab7b112a18088e80f35a8c63e210d0 (adversarial)

Required

R1. Line breaks still decide the by-line score: a re-wrapped moved g loses its source to a two-line decoy, and the PR merges (bars a and b; measured).

  • Cause (inferred, from closeness): the fallback aligns rows, one string per source line. A formatter re-wrap is whitespace only, but it splits rows, so every wrapped line counts as fully changed. This is SEMI and CMT again, with line breaks in place of semicolons.
  • What it breaks: the quickstart says moved code "re-wrapped … is measured as if git had paired it … reading only code either way, so formatting and comments never decide it". The changelog says the same and "never takes another's source".
  • Probes: r13's zn1lMoved with const vN = lines wrapped after =, and G_DECOY as the decoy (<tmp>).
Probe Head 35d11f5 main 028013d
WRAP-1of2: every other line wrapped, decoy in b.js merge merge merge
WRAP-2of3, WRAP-3of4 merge merge merge
WRAP-2of3z: decoy in z.js merge merge merge
The same with no decoy (-1of2c, -2of3c, -3of4c, -halfc) hold hold merge
  • A fix that works (measured): scoring on code tokens of key (k.match(/[\w$]+|[^\w$;,()]/g)) in the compiled reader. Every partial-wrap probe then holds, and r13's ZN, SEMI, CMT, CM, Z-N, M3B and D1L tests pass 31/31. The file was restored and checked with cmp.

Follow-ups

  • Whole-statement re-wraps (measured): WRAP, WRAPc, JOIN and JOINc merge even with no decoy, at all three trees. These fall under the "too far to align" limit, but the quickstart's re-wrap sentence should name them.
  • Surviving mutants (measured, r11-align, r12 and r13):
    • empty rows kept in the line score;
    • the line score weighted by line count;
    • R68o's one-sided score.
    • Killed: a constant tag, by ZN1L-cmt and CM1 to CM5.
  • Cost (measured, r68r-perf2.mjs): 100 near-limit hunks take 1,233 ms at head, 775 ms at 35d11f5 and 719 ms at main. Tagging makes alignment about 1.6× slower. Peak RSS is 127, 125 and 117 MB.

Checked, no finding (measured)

  • No new false holds: 17 ordinary in-place edits give the same verdict at all three trees.
    • They cover comments, commented-out code, strings, regular expressions, templates, mixed lines, JSDoc, and large and unequal-count hunks.
    • The tested moves MT-wrap, MT-semi, MT-cmt and MT-blk merge everywhere.
    • INL (inline /* */) is newly held.
  • weak-scan: its 3 flags are benign: todos turned into must-holds, elsewhere plumbing, and listed limit: todos. Round 9 removes no test.
  • Tests and CI: head 08b5e15 at the start and at the end. CI: 19 pass, 7 skipping. r11-align, r12 and r13: 169 pass, 0 fail, 6 todo.

What I ran

  • Detached worktrees for the head, 35d11f5 and main, built with tsc, with TMPDIR under /var/tmp (0700).
  • Probes, mutants (restored) and codeRanges timings.
  • No GitHub writes. The worktrees are removed.

@jayden1711

Copy link
Copy Markdown
Owner Author

Independent final check (R68s), posted by the coordinator.

VERDICT: PASS @4869afd27f8b72a1ce65ddee4e65b7c335af8321 (final check)

Head confirmed at start and end (gh pr view 1068 --json headRefOid). Round 10's diff since 08b5e15 touches closeness/key in src/vacuity.ts, one header comment, quickstart, changelog and r13 tests.

Findings

  • Fail-before / pass-after (measured): head's r13 with 08b5e15's src, tsc -p tsconfig.json, node --import ./dist/test/isolate.js --test --test-name-pattern='WRAP|ZN4L' dist/test/merge-vacuity-r13.test.js. At 08b5e15: 13 tests, 7 pass, 6 fail (all six WRAP must-holds, actual true); the three WRAP controls and four ZN4L tests pass. At head: 13/13.
  • Mutant table (measured, compiled dist/src/vacuity.js, r11-align+r12+r13, 187 tests each, in copies of the tree): source-only score → ZN4L-S; unit-only → ZN4L-U, ZN4L-N; weight 1 → ZN4L-N; one token per character → 20 fail. All four killed, as claimed. The table lists 17 for the last one, and ZN4L-S/U/N also fail (an underclaim, which doesn't matter).
  • R68r's 43 probes (measured, head vs 08b5e15): they differ only on WRAP-half, -1of2, -2of3, -2of3z and -3of4, which went from merge to hold. WRAP, WRAP-z, WRAPc, JOIN and JOINc merge at both: that is the newly documented limit. O6 holds at both, and correctly, because f(1) never takes ? 2. This matches the body.
  • Behaviour change (measured, my own probe): the probe has two same-named added gs (one a two-line decoy) contesting one removed g, with every line rewritten. 100 lines (about 1,200 tokens) merge at head and at 08b5e15. 300 lines (about 3,600 tokens) hold at head with "two statements named g are too large to tell…" and merge at 08b5e15. So the fail-closed threshold sits between those sizes, consistent with "about 2,000 code tokens a side".
  • Docs (inferred, read against code): alignChars trims the common prefix and suffix and then caps at ALIGN_CELLS = 4M, so "what they don't share at their start and end … about 2,000 code tokens a side" is accurate. key strips all whitespace, so line breaks can't change the tokens, and "never decide which source it takes" holds. The quickstart and changelog name the re-wrapped-every-line and joined-pairs limit, and so do both headers.
  • Checks (measured): npm run typecheck exit 0. npm run check:denylist: 1645 files clean. The body says 1647; the script counts untracked files (git ls-files -co), so the number depends on the checkout. Every dist/test/*vacuity*.test.js at head: 416 tests, 401 pass, 0 fail, 15 todo, which matches the body. gh pr checks 1068: every required check passes (19 pass, 7 skipping, none failing). The commit is signed off.

Weakening scan flags

Ran weakeningOf (compiled head) against the worktree, tip origin/main.

  • merge-base..HEAD, 3 flags:
    1. r12 assertion lines changed: limit: todo tests R1, R3, R4 (and r13's M3, M4b, M5b, R3s) became must-holds. Strengthening.
    2. Guard lines in src/coordinator/merge-flow.ts: changedHunks also returns elsewhere, and the request carries it. These are plumbing for pairing that only takes measure away (the both-ways rule, pinned by P3m, P3d and P5). Strengthening (inferred).
    3. New { todo } lines in r13: the unbalanced-file case, N5u, rewrite-to-copy, W1, W2 and X1, each a documented limit. Limit, not an exception to an existing check.
  • 08b5e15..HEAD, 1 flag: the src/merge-policy-vacuity.ts header comment changed "by line" to "by code token" and added the WRAP must-holds. Strengthening (documentation).
  • No real weakening found.

What I ran

A detached worktree at <tmp>, TMPDIR <tmp>, and nice/ionice on every build and test. Only targeted tests; no full npm test. No writes to GitHub.

Follow-ups (not blocking)

  • A whole-statement re-wrap or join of a large moved statement still merges, at main as well. This is the documented limit.
  • Two contested same-named statements past about 2,000 unshared tokens now make the PR wait where 08b5e15 merged them (measured above). This is documented, fails closed and is unusual in practice (inferred). Consider pinning it with a test: none reaches the throw path.

@jayden1711

Copy link
Copy Markdown
Owner Author

Independent adversarial review (R68t), posted by the coordinator.

VERDICT: FAIL @4869afd27f8b72a1ce65ddee4e65b7c335af8321 (adversarial)

Required

R1. A two-line decoy still takes a large moved g's source, and the PR merges with an untested edit (bar a; measured).

  • The changelog says pairing goes "the closest same-named pair first, so a same-named statement elsewhere never takes another's source".
  • Probe UB100: a 105-line g moved into c.js, every word token on every line edited inside an if (u.never) block the test never enters (const v5 = alpha(u) + 5 * beta(u) becomes const w5 = alphz(u) + 6 * betz(u)). G_DECOY sits in b.js or z.js.
  • UB100 and UB100z merge. The no-decoy control holds, so the pair aligns and no documented limit covers it.
  • Cause (measured by logging closeness; dist restored, checked with cmp): the moved g scores −3402, the decoy −2818. Each edited word token counts its full length.
  • Same at 08b5e15; main merges every variant. This is an overclaim, not a regression.
  • Fix (inferred): narrow the sentence to path order and name this limit, or score a partly matching token by its characters.

Follow-ups (measured)

  • New false hold (ORD200). Two 200-line renders, each moved into another file with a tested edit, hold ("too large to tell…"). 08b5e15 and main merge it. This is the behaviour change the PR names. A row-score fallback before the throw would avoid it.
  • The ~2,000-token boundary is accurate. BND160 is scored; BND166 to BND180 hold whether or not the change is tested. I found no fail-open just under it.
  • Surviving mutants (r11-align, r12, r13: 187 tests):
    • the throw replaced by return -Infinity: nothing pins the fail-closed claim;
    • word-run-only tokens;
    • \w without $;
    • no \s filter in key (looks equivalent, inferred).
    • Killed: single-character punctuation, one token per character, weight 1, unit-only and source-only.

Checked, no finding (measured)

  • Formatting decoys don't win. Trailing commas (TC40 to TC120), parenthesised elements (PAR) and dropped semicolons (SD) all hold, decoys and controls alike; TCt merges. key comes from quiet(). ) ) ) gives the same key as ))) (inferred).
  • Fail-before and pass-after. Head's r13 on 08b5e15's src: the 6 WRAP must-holds fail, ZN4L and the controls pass. All 13 pass at head.
  • R68r's 43 probes match the PR body. The WRAP-* partial wraps moved from merge to hold. WRAP, WRAPc and JOIN merge at all three trees, under the documented limit.
  • Cost (r68r-perf2.mjs): 646 to 757 ms at head, 706 to 796 at 08b5e15, 345 to 503 at main.
  • Typecheck exits 0; CI is green.

Weakening scan flags

weak-scan.mjs with main's engine reports 3 flags:

  1. r12 assertion lines: limit: todos become must-holds. Strengthening.
  2. merge-flow lines: elsewhere plumbing. Strengthening.
  3. todo marks in r13: named limit: cases (N5u, lexer, X1, W1, W2). Limit.

No real weakening.

What I ran

  • Detached worktrees for the head, 08b5e15 and main c4c657e, now removed. TMPDIR under /var/tmp.
  • Probes in <tmp>.
  • No GitHub writes.

… a moved pair (#1014)

Scored by token, every edited name cost its full length on both sides,
so a 105-line g moved with every word of 100 lines edited inside a
block its test never enters scored worse than a two-line decoy g, which
took its source, and the PR merged (R68t: UB100, UB100z). Each run of
tokens between kept ones is now aligned by character and costs the
characters it doesn't keep (its full length when too large to align).

ZN1T pins the fail-closed path: two same-named statements too large to
tell apart by token make the PR wait, even with the change tested.
ZN4L-P pins punctuation tokens, and ZN4L-S now drops lines only, so a
source-side-only score loses it.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Independent final check (R68u), posted by the coordinator.

VERDICT: PASS @29bb7f6c865b8bdef4a15753f2b56673ee1193c2 (final check)

Head confirmed at start and end (gh pr view 1068 --json headRefOid). Signed off.

Findings

  • Fail-before / pass-after (measured): head's r13 on 4869afd's src, tsc -p tsconfig.json, node --import ./dist/test/isolate.js --test --test-name-pattern='UB100|ZN1T|ZN4L' dist/test/merge-vacuity-r13.test.js. At 4869afd: 8 of 10 pass; UB100 and UB100z fail with actual [true, []]. At head: 10/10. The body's "all 11" is a miscount.

  • Mutants (measured, compiled vacuity.js, r11-align+r12+r13, 193 tests): every row in the table is killed by exactly the tests listed:

    • throw→-Infinity: ZN1T;
    • word runs only: ZN1T, ZN4L-P;
    • full-length runs: UB100, UB100z;
    • moved side only: ZN4L-U, -N;
    • source side only: ZN4L-S;
    • score -= 1: ZN1L, ZN4L, -N, -P, -S.

    \w without $ survives, as disclosed.

  • Probes (measured, R68r+R68t file, 95 probes, head / 4869afd / main 246e3f6): head and 4869afd differ only on UB100 and UB100z, which went from merge to hold. WRAP, WRAP-z, WRAPc, JOIN and JOINc merge, as at main (the documented limit). O6 holds, as at main. ORD200, TC120t and BND166t to BND180t wait with "too large to tell" where main merges. Total time: 58.6 / 57.4 / 62.2 s.

  • Docs (inferred, read against run()): "an edited token costing only the characters it changes" is accurate. The docs leave out that a run too large to align by character costs its full length (fails closed).

  • Checks (measured): typecheck exit 0; check:denylist 1645 clean; all 8 required checks pass (one optional shard pending).

Ruling on ORD200 / TC120t / BND166-180: accepted fail-closed limit, not (c)

  • It holds rather than refuses: a named reason, and no wrong merge.
  • The trigger is narrow (inferred from code). One PR needs:
    • at least two wholly added or removed top-level statements with the same name;
    • a contested pair whose untrimmed middle is over about 2,000 tokens a side (about 165 dense lines);
    • a real edit, because an unedited move pairs by code first.
  • Main's merge here is the fail-open this PR closes. Main would merge ORD200 just as well with the edit untested (inferred). Waiting is the right default for a critical-set gate until the two cases can be told apart.
  • It is disclosed in the body, quickstart and changelog, and pinned by ZN1T. R68s and R68t both rated it a follow-up.
  • Counterweight: large same-named moves (render, handler) happen. In ORD200 the throw likely comes from the cross pair while the right pair aligns cheaply (inferred), so the hold is avoidable. That makes it a follow-up.

Weakening scan flags

weak-scan.mjs (main's and head's engine give the same result), base 32cd15e, 3 flags:

  1. r12 limit: todos became must-holds: strengthening.
  2. merge-flow.ts elsewhere plumbing for a pairing that only takes measure away: strengthening (inferred).
  3. New r13 { todo }s (unbalanced file, N5u, rewrite-to-copy, W1, W2, X1): documented limits.

The ZN4L-S rebuild changes inputs, not assertions, and the source-side mutant shows it got stronger. No real weakening.

Follow-ups

  • Throw only when the unscorable pair can change the pick, so ORD200 merges.
  • Document the full-length cost of a run too large to align.
  • The $ survivor.

What I ran

Detached worktree <tmp>, TMPDIR <tmp>, nice/ionice on builds and tests, targeted tests only, no GitHub writes. git fetch failed with a proxy 403; the head was already local and matched gh.

@jayden1711

Copy link
Copy Markdown
Owner Author

Independent adversarial review (R68v), posted by the coordinator.

VERDICT: FAIL @29bb7f6c865b8bdef4a15753f2b56673ee1193c2 (adversarial)

Required

R1. A two-line decoy still takes a fully edited large moved g's source once one run of edited tokens is too large to align by character (bar a/b; measured).

  • Claims: the changelog and quickstart say "an edited token costing only the characters it changes" and "a same-named statement elsewhere never takes another's source". The round-11 body says a fully edited large moved statement keeps its source over a small decoy.
  • Probe BRn: a moved g holds one long expression in an if (u.never) block, + alphaq<i> per line edited to - alphar<i>. No token is kept, so the whole edit is one run.
  • Results at head (R68V_BR=… node --test --test-name-pattern='^R68V' dist/test/r68v-probe.test.js):
    • BR150, BR200 and BR210 hold.
    • BR220, BR220z, BR230, BR260 and BR300 merge, with the decoy in b.js or z.js.
    • Every no-decoy control (BRnc) holds, so the pair can be placed. Every tested variant (BRnt) merges.
    • BRSIM300, a decoy built from the new names, also merges.
  • Cause (measured by logging closeness in compiled vacuity.js, then restored and checked with cmp):
    • BR210: the moved g scores −840 and the decoy −2035.
    • BR220: the moved g scores −4180, its full run length of 2,160 chars a side, and the decoy −2135.
    • The run is past ALIGN_CELLS (about 2,000 chars a side), so each edited token costs its full length. The body discloses this; the docs and changelog don't.
  • Main 246e3f6 merges every BR probe, controls included. So this is an overclaim, not a regression.
  • Fix (inferred): make a too-large run wait, as a too-large token alignment does, or split it before aligning. Otherwise, name the limit and narrow the "never" sentence.

Ruling on the disclosed behaviour change (ORD200, TC120t, BND166–180)

These are an accepted fail-closed limit, not (c).

  • Measured threshold:
    • ORD195 merges and ORD200 holds, at about 10 code tokens a line, so about 2,000 tokens.
    • DD220 merges and DD250 holds. DD is a dedupe of two diverged same-named copies into one shared module, with a tested edit.
  • The hold needs three things together (inferred): an unpaired move, a same-named contest in the same PR, and a contested pair that differs near both ends across more than about 2,000 code tokens a side.
  • Moving one function, or several with distinct names, never triggers it, and the PR waits with the reason named. That is uncommon, not ordinary use.
  • Main merges these only because it reads all moved code as added code.
  • Follow-up: a row-score fallback before the throw.

Mutants (r11-align, r12, r13 compiled tests; 187 pass at head; dist restored and checked with cmp)

  • Killed:
    • a one-sided run costs nothing (ZN4L-U);
    • runs never reset (UB100, UB100z);
    • the last run isn't charged (ZN4L);
    • every run costs its full length (UB100, UB100z).
  • Survive:
    • a too-large run costs 0;
    • a too-large run costs one side only;
    • no run() at a kept token.
    • No test has a run past ALIGN_CELLS, which is the R1 shape.

Checked, no finding (measured)

  • Fail-before and pass-after. Head's r13 test on 4869afd's source: UB100 and UB100z fail; ZN1T, ZN4L to ZN4L-P, UB100c and UB100t pass. All pass at head.
  • Many short runs. LNG100 and LNG200 use long similar renames with their punctuation kept. Decoys hold, controls hold, LNG100t merges.
  • The fail-closed boundary is not fail-open. LNG200t, a tested change with a decoy, waits with "too large to tell".
  • Checks. npm run typecheck exits 0. CI on the head: every check that has finished passes; three Node 22 and 24 shards were still pending.

Weakening scan flags

weak-scan.mjs with main's engine reports 3 flags:

  1. r12 assertion lines: R1, R3 and R4 go from limit: todos to must-holds. Strengthening.
  2. merge-flow guard lines: the elsewhere plumbing, which adds sources. Pairing only takes measure away. Strengthening.
  3. todo marks in r13: named limit: cases (lexer, N5u, EDITED_IN_PLACE, W1, W2, X1), each documented. Limit, no weakening.

What I ran

  • Detached worktrees of the head, 4869afd and main, now removed. TMPDIR under /var/tmp, with nice and ionice.
  • Probes: <tmp> (R68t's file plus R68V BR, LNG, BRSIM, ORD and DD).
  • Mutant script: <tmp>.
  • No GitHub writes.

… the PR wait (#1014)

A contested same-named pair scored by token weighed a run of edited
tokens past ALIGN_CELLS by its full length, so a two-line decoy took a
fully edited large moved statement's source and the PR merged (BR220).
Such a run now throws CoverageUnreadable, as a too-large token alignment
does: the PR waits with 'too large to tell'.

BR220, BR220z, BR300 and BR220t must hold with that reason; BR220c is
the no-decoy control. LNG100 and LNG100t pin that each short run is
aligned on its own.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.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.

Vacuity gate: pair moved code git doesn't pair, without ever adding anything to measure (follow-up to #903)

1 participant