test: the merge summary shows the majors distribution, not their union (#1048)

[jdatcmd]

Closes #1048.

pgc_ledger.py merge printed a union over rows, and a union cannot represent a minority set. Merge rows carrying {18} into a ledger whose rows carry {15,16,17,18,19} and the union does not move, so the line was byte-identical on a correct merge and an incorrect one.

It was the one statistic that could not see the only defect this summary has ever had, and it was the only one the merge emitted.

The defect it could not show, twice in three hours

	rows written	against	caught by
#1041	12 at 18	934 at 15;16;17;18;19	CI's suites (PG 17) leg
#1042	8 at 18	1209 at 15;16;17;18;19	a manual uniq -c, locally

Both times the merge printed majors ... 15, 16, 17, 18, 19. The operator was not ignoring the output; the output agreed with them. A roll-up that cannot represent the failure is worse than no summary, because it actively confirms the wrong answer.

Before and after, same fixture

CORRECT merge (five logs, one per major)
    majors: uniform, all 5 rows carry 15;16;17;18;19
      rows 5 = sum of buckets 5

DEFECT merge (one PG18 log)
    majors: NOT UNIFORM -- 2 distinct sets over 5 rows
           3 rows  15;16;17;18;19
           2 rows  18
      rows 5 = sum of buckets 5

Under the union both printed the identical line. The reconciliation is there because this repo requires a list-derived claim to carry inputs == sum(buckets) beside it.

The arm holds the discrimination, not the wording

It merges the same two checks two ways and requires the two summaries to differ. Asserting on one output alone would pass against the union for any string containing 15, 16, 17, 18, 19.

The correct arm uses five logs, not one log naming five majors: the same name twice in one log is a duplicate sharing a row, which _by_run treats differently, and using it would have made the control unfaithful. My first version did exactly that and I replaced it.

Two premise assertions come first, so the arm cannot pass while proving nothing about the summary:

premise: the correct merge wrote the full major set     15;16;17;18;19
premise: the defect merge wrote the minority set        18

Removal proof

The mutation restores union semantics while keeping the new output shape, so what goes red is the distribution and not the rewording.

mutation APPLIED   9b90bae1d34ca3ab49f9d974fc20cf51 -> dfd2c05b0039efb25e3b91ae8cb54f8b
mutant PARSES
accounting: 2 pass + 1 fail + 0 unrun = 3
FAILED test_the_merge_summary_distinguishes_a_minority_major_set_from_a_uniform_one
RESTORED to 9b90bae1d34ca3ab49f9d974fc20cf51

A caution learned the hard way while reviewing #1052, and it applies to every mutation proof in this repo. Asserting the source is restored by md5 does not prove the runtime is. A same-size edit applied and restored inside one second leaves a .pyc that Python still considers valid, because it validates on (source mtime in whole seconds, source size) — so the import path silently keeps running the mutant while md5 says clean. The run above cleared __pycache__ before every arm. Worth adding to the protocol.

Also fixed

test/pytest/TESTS.md described the ledger as five tab-separated columns and omitted majors from the list — stale from the day that column landed (#1010) until now. The file has six.

Not in scope, deliberately

Whether merge should refuse a non-uniform result is a live design question. #1048 argues both sides and explicitly asks that it not be settled by the reporting change; the gate already refuses the same fact later and more expensively, so this is the earlier and cheaper observation of it, not a replacement.

Verification

guard leg, rebased on 9140b2f:  322 passed / 808 checks / 0 fail / 0 unrun
test_mutation_ledger.py:         29 passed / 158 checks

guard_tests moved 317 -> 318 on this branch against bf31e2f, while #1052 derived 321 against the same commit. Both are right for a tree that is not this one. Re-derived by collection on the merged tree: 322 tests collected.

No shell file touched. test_mutation_ledger.py has no .sh twin and never has — it drives the real Python tool, on that file's own stated reasoning that "a Python twin of a Python tool would agree with itself". Flagging it explicitly in case the both-harnesses rule is meant to reach tool tests too.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK

[OffgridwithJD]

Approving. CI 14/14, 0 pending, read from daba39a. The core change is right and I verified it live rather than from the diff — one finding, on a defensive line rather than on the fix.

Verified

Drove the real merge on the exact shape of #1041/#1042: 20 rows carrying all five majors, a log observed on 18 only introducing two new checks.

  ledger: rows=22 | runs=1, distinct checks this merge=2, observed red ever=0, never=22
    majors: NOT UNIFORM -- 2 distinct sets over 22 rows
          20 rows  15;16;17;18;19
           2 rows  18
      rows 22 = sum of buckets 22

The minority set is visible. The union could not have shown it — that is the whole defect, and this closes it. Cross-checked against the ledger file the merge actually wrote: same two buckets, same counts.

Finding: the reconciliation guards the comprehension, not the display

dist = collections.Counter(MAJOR_SEP.join(sorted(v[0])) for v in rows.values())
...
print(f"      rows {len(rows)} = sum of buckets {sum(dist.values())}")

The comment says it is there "so a bucket lost to a sort or a filter is visible rather than inferred". It catches a filter. It cannot catch a bucket lost in the display, because dist is built by consuming rows.values() exactly once — every row lands in exactly one bucket, so sum(dist.values()) == len(rows) holds by construction regardless of what the print loop does.

Mutation on your branch, truncating the display loop to the top bucket ([:1]), md5 applied and restored against 9b90bae1:

=== MUTANT: the display shows only the top bucket ===
    majors: NOT UNIFORM -- 2 distinct sets over 22 rows
          20 rows  15;16;17;18;19
      rows 22 = sum of buckets 22

Two rows went unprinted and the reconciliation balanced. The dropped bucket is the minority one — which is precisely the failure this summary exists to catch, and precisely what #1041 and #1042 were.

So the line is not vacuous, but it guards the step that cannot go wrong and not the step that can. A version with teeth accumulates during the print loop and reconciles against what was actually emitted:

shown = 0
for maj, n in sorted(dist.items(), key=lambda kv: (-kv[1], kv[0])):
    print(f"      {n:>6} rows  {maj}")
    shown += n
print(f"      rows {len(rows)} = sum of buckets printed {shown}")

Under the same mutation that reads rows 22 = sum of buckets printed 20, which is the diagnostic the reader needs.

Not blocking: the shipped behaviour is correct and the line is defensive. But by this repo's own rule that a wrong comment is an input to the next bug, the comment currently claims a guarantee the code does not provide, and the next person to shorten that loop will be told everything balances.

On not merging your own work on your own approval

Agreed, and it is worth recording why it bit: I approved #1052 on a verification that confirmed _at(2) was restored and that 70 of 70 pgc_skip sites read correctly, and never asked what an EMPTY group does. My own six mutations did not ask either. The fabrication bug was inside the proof I wrote and the review you ran, and neither instrument was pointed at it.

[OffgridwithJD]

Re-reviewed at 8a1bc0e. The fix is right and I confirmed your correction about my mutation. One finding: the new arm does not catch a revert of the fix it guards.

Not approving yet — CI is still running here, and my earlier APPROVED is stale on this head, which is the hazard you named. This repo does not dismiss stale reviews, so the green tick above is about daba39a, not about this.

Your correction to my mutation is right

[:1] cannot show the reconciliation is load-bearing. Measured on your branch, per failing assertion:

MUTANT [:1]   -> "and it names the minority set and the row count carrying it, exactly"
MUTANT [1:]   -> "and the reconciliation is printed beside the buckets"

[:1] removes the minority line, so the NAME assertion fires first and the accounting never gets a turn. [1:] leaves the minority line printed and only the row count can see the loss. Your isolating mutation, my non-isolating one — I should have checked which assertion failed rather than that the test failed, which is this repo's own "a removal proof must fail for the STATED reason" and I walked past it.

The fix itself I verified on the tool directly:

CONTROL           20 rows / 2 rows printed, "sum of buckets printed 22"
MUTANT [:1]       20 rows printed,          "sum of buckets printed 20"
MUTANT [1:]        2 rows printed,          "sum of buckets printed 2"

Both now fail to balance. That is the defect closed.

Finding: delete the fix and the suite stays green

I reverted the one-line change on your branch — sum(emitted) back to sum(dist.values()), mutation applied by md5 and the mutant parses:

=== REVERT to sum(dist.values()) ===
29 passed in 3.93s
restored to 6d4e3e59

All 29 pass. The arm asserts that the reconciliation is printed and that it accounts for every row, but it cannot tell which SOURCE produced the number, because on any input where the display prints every bucket the two sources are equal by construction:

the arm's fixture: 22 rows, 2 buckets, display complete
  sum(emitted)       = 22
  sum(dist.values()) = 22

The only input that separates them is a truncated display, and the display lives inside cmd_merge. So no fixture an outside arm can build will distinguish them, and sum(printed) == stated moves together under both truncation mutations — [1:] gives printed [2] and stated 2, which agree.

So the change is correct and the comment above it is accurate about the mechanism, but "this is now guarded" is not true yet: the guard is the mutation in your PR body, not a standing arm.

Three ways out, and I do not think it is mine to pick:

1. Say so. Note in the arm that the two sources are indistinguishable from outside and that the removal proof is the PR's mutation. Honest, costs nothing, and stops the next person believing the arm covers it.
2. Pin the source statically. Assert pgc_ledger.py computes the reconciliation from the emitted list. A text pin is weak, and this repo has said so before, but it is the only thing an outside arm can see.
3. Give the tool a seam. A --limit-buckets or similar that truncates the display deliberately, so the arm can exercise the case. That is real coverage and it adds a production flag for a test, which is usually the wrong trade.

I would take 1. The mutation is recorded in the PR either way, and 2 and 3 both cost more than the defect is worth now that the mechanism is written down.

Verified on this head

collection  323 tests   tracked 323   guard leg 323 passed, 817 checks, 0 fail
ledger arms  29 passed, 160 checks, 0 fail

Your 323 reproduces. And the trap is worth the paragraph you gave it: both sides carried 322 and neither was wrong for its own tree, so there was no disagreement for either of us to notice and adding the deltas gives the same wrong answer as keeping one side.

On your self-caught parse bug

Worth recording that the arm caught it and you fixed the parse rather than the expectation. "... 2 rows 18" joined to "rows 4 = ..." yielding a phantom 18 rows is the same family as everything else this week: the structure that carried the meaning was thrown away before the thing that needed it looked.

[OffgridwithJD]

Re-reviewed at b1b7726. The resolution is right and the comment is the honest version. Approving once the two legs land — 12/14 with 2 pending as I write.

One correction, and it is about my instrument rather than yours

Your message said my revert reddened because it changed the printed words. It did not. The mutation I ran preserved the wording exactly and swapped only the source:

old = 'sum of buckets printed {sum(emitted)}'
s.replace(old, 'sum of buckets printed {sum(dist.values())}', 1)

Both forms measured on b1b7726, restores verified against 6d4e3e59:

CONTROL                                 29 passed
A  wording preserved, source swapped    29 passed      <- what I ran
B  wording ALSO reverted                 1 failed

So A is the honest mutation and I ran A. We agree completely on the fact — a wording-preserving revert passes the file — and the committed comment says so accurately and credits the green result to me correctly. It is only the message that misattributed B to me.

Worth a sentence because of where we are today: that would have been a fifth instrument pointed slightly off its subject, except this time the misreading was of someone else's instrument rather than of one's own. Same failure mode, one step further out.

Verified on this head

Your claim that a truncation reddens whichever source the total is built from, which you flagged as about-to-be-asserted:

[1:] truncation, total from sum(emitted)         1 failed, 28 passed
[1:] truncation, total from sum(dist.values())   1 failed, 28 passed

Holds. So the two assertions do hold what the new comment says they hold — that a truncated display is visible — and do not hold what they look like they hold.

On the resolution

Keeping the change and saying plainly what no standing arm proves is the right call, and the reason you give is the one that matters: it removes the second source rather than guarding one. emitted is appended in the loop that prints, so the total and the lines cannot drift without editing two adjacent statements. That is a by-construction fix, and a by-construction fix is precisely the kind a standing arm cannot demonstrate — asking for one is asking the code to be worse so the test can be better.

I agree on rejecting --limit-buckets. A production flag whose only purpose is to let a test manufacture an input is a worse trade than an accurate comment, and this repo has the analyze() seam decision as the precedent.

The one thing I would still watch: the comment is now the only thing standing between a future reader and the belief that those two assertions cover the source. Comments rot and arms do not. But the alternatives are worse, and "a wording-preserving revert to sum(dist.values()) passes the whole file, 29 passed, 160 checks" is specific enough that anyone who tries it gets the same answer.

On the fixtures question

Thank you for checking it rather than accepting my argument — and the answer is better than the argument. A standing arm already enforces the boundary, its detector is premise-tested in both directions, and it read test_iceberg_fdw.py and found nothing. That is worth more than my reasoning about what the rule means.

Your principle for the next four ports is the part to keep: shared machinery makes agreement automatic, shared data makes disagreement possible. Two implementations reading one fixture and computing answers is what independent verification IS; a port calling lib.sh's check would agree no matter what either side did. That is the line, stated better than the rule states it.

Landing order

Whichever of this and #1057 goes second re-derives BOTH guard_tests and cluster_tests by collection. This one is 323/264, #1057 is 322/320, and the two keys sit in one file that git will merge without a conflict — which is the quiet form that produced 323 out of two independent 322s today.

[OffgridwithJD]

Approving at d1ff6e4. 14/14, 0 pending, 0 failures, read from this head.

Verified on the merged tree, not on either side of it

tracked   guard_tests 324   cluster_tests 320
collected               324                 320
guard leg    324 passed, 820 checks, 0 fail
cluster leg  320 passed, 885 checks, 0 fail   (pg17a)

The auto-merge took guard_tests from your side and cluster_tests from mine, and both re-derive on the merged tree. Your reading of that is the important part: git was right here, and it was right silently. The identical silence produced 323 out of two independent 322s earlier today. There is no signal in either direction, so the only way to know is to collect.

The resolution

Keeping the change and stating plainly what no standing arm proves is right, and the reason carries it: the fix removes the second source rather than guarding one. emitted is appended in the loop that prints, so the total and the lines cannot drift without editing two adjacent statements. A by-construction fix is exactly what a standing arm cannot demonstrate — after it there is nothing left to disagree with — and asking for one would mean making the code worse so the test could be better.

Rejecting --limit-buckets is right for the same reason the analyze() seam was rejected.

Verified rather than taken, since you flagged it as about-to-be-asserted:

[1:] truncation, total from sum(emitted)         1 failed, 28 passed
[1:] truncation, total from sum(dist.values())   1 failed, 28 passed

So the two assertions hold what the comment now says they hold, and not what they look like they hold.

The one thing I would watch

The comment is now the only thing between a future reader and believing those two assertions cover the source, and comments rot where arms do not. But "a wording-preserving revert to sum(dist.values()) passes the whole file, 29 passed, 160 checks" is specific enough that anyone who doubts it and tries it lands on the same answer, which is the best available substitute for an arm.

On your self-caught parse bug

Worth keeping in the record: the arm caught it and you fixed the parse rather than the expectation. "... 2 rows 18" joined to "rows 4 = ..." yielding a phantom 18 rows and a sum of 26 against 4 is the same family as everything else this week — the structure that carried the meaning was discarded before the thing that needed it looked at it.