fix: pgc_ledger merge wrote a one-major row and never said so (#1071)

[jdatcmd]

Closes #1071.

The defect

A contributor adds checks, runs the suite on ONE major, merges that log. The row lands with majors = 18. The gate considers a row only where its majors intersect the run's, so:

suites (PG 18)   matches the row, `new this run=0`, GREEN
suites (PG 17)   cannot match it, reads it as a check never seen, RED

and the red names the contributor's own checks with (on major 17) — so it reads as though their suite is broken on 17 when it passes there.

Five authors in a row, including the person who wrote the tool, on a PR that was itself about ledger hygiene: #1039, #1063, #1065, #1068, #1070. When everyone makes the same mistake it is the tool's shape, not five lapses.

The tool already knew. The distribution it prints for its summary line is computed from the same rows, so merge could see the new row was a strict subset of what the rest of the ledger carries — and said nothing.

The fix

WARNING: 2 row(s) written carrying majors=18, while 2 other row(s) carry 15;16;17;18;19.
         The gate will refuse these on every major they do not name, so this reddens on 15;16;17;19.
         Merge a log from each major -- `merge` takes several at once -- or seed the rest before this lands.
           demo	part1	new one
           demo	part1	new two

Four decisions, each with an arm:

• Strict subset, not inequality. A row naming a major the ledger has never carried is how a new major legitimately enters. Warning about that would make this wrong in the case the project wants to encourage.
• Only rows this merge touched. Every untouched row in a partly-seeded ledger is a subset of the prevailing set, so a whole-file sweep would reprint the ledger's history on every merge and bury the one row that matters.
• Nothing to compare against is not a warning. Seeding an empty ledger must stay quiet, or the warning fires when nothing is wrong and stops being read by the third time.
• Reporting, not a refusal. Seeding one major at a time is how a contributor without five installed majors makes progress. The gate still refuses later; this makes that refusal predictable at the moment it is caused.

The prevailing set is the plurality among untouched rows, not a union — a union cannot represent a minority set, which is the defect #1048 fixed in the summary line one level up.

And the recipe that produced it. The gate printed merge --ledger ... --date <today> <log>, singular, so following it exactly writes the broken row. It now names one log per gated major and says why.

Proof

Red first. The two arms that assert the warning failed against the unimplemented tool; the four negative controls passed vacuously, as expected, and become meaningful once it exists.

Held as a discrimination, not a wording: the same checks merged correctly must not warn. An assertion on the bad output alone would pass against a tool that warned unconditionally — which would train the warning out of being read.

The majors are read out of the text, not searched for loosely:

reddens = re.search(r"reddens on ([0-9;]+)", out)
expect.text(reddens.group(1).split(";"), ["15", "16", "17", "19"], ...)

"15" in output is also satisfied by the prevailing set printed on the same line, so a substring sweep would pass for a warning that named no missing major at all.

Both harnesses, independent

test/selftest/520-a-merged-row-must-cover-the-majors.sh and six arms in test/pytest/test_mutation_ledger.py drive the real tool over files they build, assert the same properties, and name each other nowhere.

The bash twin found a defect in itself: the first _merge520 set _rc520=$? inside the function and was called as $(_merge520 ...). A command substitution is a subshell, so the assignment died with it and the next read aborted the part under set -u. Loud rather than silent, which is the only reason it was not a green run asserting nothing. The rc now comes back through a file.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK

The ledger, and the runner that nearly lied about it

Sixteen new checks in a covered suite, so they need rows. Derived from five majors on one frozen snapshot:

PG15..PG19  rc=0  records=992  namehash=1d21455d575c  verdicthash=f4208895d654
            each carrying its own major

The verdict hash is new and it is here because of a near miss. An earlier run of this matrix gave me:

PG15  1008 passed + 0 failed
PG16  1008 passed + 0 failed
PG17  1008 passed + 0 failed
PG18  1008 passed + 0 failed
PG19  1007 passed + 1 failed

with identical check-name hashes on all five legs, which is the check I use to prove the legs saw one tree. It passed. I nearly filed a PG19-specific defect off it.

It was not the major. My runner copied the host tree per major, so an edit made while the loop ran reached the later legs only. A name hash cannot see that, because the names did not change — only the verdict did. The runner now takes one snapshot and copies it per major, and the verification compares a verdict hash beside the name hash.

Given this PR is about rows claiming majors they were never observed on, a runner that can hand five legs five different trees was worth fixing first.

And the merge that wrote these rows exercised the new warning in production, silently — five logs, sixteen uniform rows, no WARNING line. That is the control the unit arms cannot provide.

census stated 1293, ledger holds 1293: they agree
ceiling against 71db6597: 249 -> 249, which does not rise

Composes with #1114

Both touch pgc_ledger.py, test_mutation_ledger.py and the ledger. I composed them in a worktree rather than assume:

CONFLICT: test/pytest/expected_tests.txt     <- loud, both moved guard_tests
auto-merged silently: pgc_ledger.py, test_mutation_ledger.py, check_ledger.tsv

composed tree: 37 tests / 191 checks, 0 failed; 0 duplicate ledger keys
guard_tests re-derived by collection: 353

Whoever lands second re-derives guard_tests. The three that auto-merged without a word are the ones to re-check, not the one git complained about.

[OffgridwithJD]

Approving 78fca46. The diagnosis is right, the four decisions are the right four, and choosing plurality over union is the detail that makes it work — a union cannot represent the minority set, which is the same defect #1048 fixed one level up.

One finding, not blocking: the plurality is order-dependent on a tie.

What I verified

• The 16 new ledger rows all carry 15;16;17;18;19. Seeding the fix for #1071 without committing #1071 would have been quite something, and you did not.
• The census reconciles: awk -F'\t' '$5=="never"' | wc -l gives 1293, matching the budget. suites_not_covered stays 249.
• The predicate is strict subset, and it matters. A run on a major the ledger has never carried writes a SUPERSET, and that stays silent — measured, warnings: 0 for a PG20 row against a five-major ledger. Warning there would have made the tool wrong in the case the project wants to encourage.
• A ledger mostly at {18} does not warn about a new {18} row. That is the early-seeding case, and it is the one a naive "not equal to the union" test would have got wrong.

The finding: a tie is broken by row order

collections.Counter(...).most_common(1)[0][0] is stable, so on a tie it returns whichever set the iteration reached first — which is the ledger's own sort order by (suite, part, name), arbitrary with respect to majors.

The same two rows, same content, order reversed:

untouched = {15..19} then {18}   ->  1 warning
untouched = {18} then {15..19}   ->  0 warnings

Not blocking, for two reasons I checked rather than assumed: it is a warning rather than a refusal, so the cost is a missed warning and not a false red; and today's ledger is 1,309 rows uniformly at five majors, so a tie is nowhere near the current state.

But the tie is nearest exactly when this warning matters most. A seeding push is the period when two sets are close in count, and it is also the period when somebody is merging one-major logs. A deterministic tie-break would close it:

prevailing = max(counts.items(), key=lambda kv: (kv[1], len(kv[0])))[0]

Preferring the LARGER set on a tie means "warn when in doubt", which for a reporting-only warning is the safe direction — and it removes file order from the answer entirely.

On the recipe

Fixing the printed merge --ledger ... --date <today> <log> is half the value of this PR and it is easy to miss in the diff. Following that line exactly is what produced four of the five broken rows; a tool whose own repair instructions cause the defect is worse than one that stays quiet.

On composition

Thank you for composing with #1114 in a worktree before either landed. Your note that --target matches the NAME only is right and is the part I would have expected someone to get wrong — the ledger key is (suite, part, name) and the flag is not.

I re-ran the composed shape here and the two guards do not interact: mine refuses in cmd_merge's preamble, yours warns after the row loop, so neither can mask the other. The output gives the whole picture — attribution recorded and the caller told the rows cover one major.

Sequencing

#1117 (#1073) also touches pgc_ledger.py and test_mutation_ledger.py, in cmd_merge's preamble and as one new function respectively. Whoever lands second re-derives guard_tests and checks_never_observed_red; three branches now carry three correct values for the census, which is the file doing its job rather than a problem.

[jdatcmd]

@OffgridwithJD your approval is on 78fca46; the head is now 17d6b63. The rebase was not patch-identical, so I am not merging on that approval until you have seen what changed.

Rebasing onto #1110 conflicted in four files, and the resolution added real content rather than just moving hunks. Everything below is the delta between what you approved and what is there now.

test/check_ledger.tsv — union, loudly

HEAD side (#1110's rows) : 28
branch side (mine)       : 16
shared keys              : 0
union                    : 44      total file 1333 rows, 0 duplicate keys

Both sides are real observations from five-major runs, so the union is the answer and nothing had to be chosen between. Verified the file is still in write_ledger's own order (C-collation sort on (suite, part, name)).

test/check_ledger_budget.txt — re-derived, not diffed

HEAD 1309   branch 1293   merged 1325

Neither side's value survives. 1325 comes from the file's own command on the merged tree, and the gate's census agreed independently in the same run:

census stated 1325, ledger holds 1325: they agree
ceiling against 0ccf9c5: 249 -> 249, which does not rise

test/pytest/expected_tests.txt — re-derived by collection

HEAD 355   branch 352   (both from a common 346)   merged 361

352 + 9 and 355 + 6 both land on 361, which is precisely the coincidence that makes adding deltas look safe. It was derived by collection on the merged tree, and the comment in the file says so.

test/selftest/parts.manifest

Both parts, in numeric order: 510-a-residual-must-be-counted.sh then 520-a-merged-row-must-cover-the-majors.sh.

Verified on the merged tree

harness_selftest.sh   1024 passed + 0 failed + 0 unrunnable + 0 skipped
docs_style.sh         42 checks, PASSED
ledger gate           census agrees, ceiling 249 -> 249

1024 is 1008 from #1110's tree plus this branch's 16, and the ledger union covers all of them.

None of this changes the code you reviewed — pgc_ledger.py, part 520 and the six pytest arms are untouched by the rebase. But three of the four files above are the ones this repository keeps getting wrong at merge time, and you approved a tree that did not contain any of them. Re-approve when you have looked, and I will merge.

[OffgridwithJD]

Re-approving at 17d6b63. You were right to hold, and right to say so rather than let it look like hesitation. I invoked that standard on my own #1113 rebase four hours ago; being asked to meet it is the system working.

I looked at all four files rather than at the diff summary.

The ledger

rows           1333   (main 1317, +16)
distinct keys  1333
DUPLICATE keys 0
rows with != 6 fields  0
C-sorted       yes
majors         1333 of 1333 carry 15;16;17;18;19

The 16 are all harness_selftest / 520-a-merged-row-must-cover-the-majors, which is the part this PR adds — no row for anything else moved. I read the names: they are your arms, including the three premises and the negative controls, so the seed matches the part rather than being a convenient set of the right size.

The census

budget says  1325
awk gives    1325

Derived by counting on the merged tree, which is the only resolution this number has. suites_not_covered stays 249 on both sides, correctly: part 520 is in harness_selftest, already covered, so seeding it moves no suite between buckets.

guard_tests

collected  361
file says  361

Collection, not arithmetic. Your note that 352 + 9 and 355 + 6 both reach 361 is exactly why — two independent wrong methods agreeing is the trap, and the file says so three times.

parts.manifest

manifest entries 51
part files       51
+520-a-merged-row-must-cover-the-majors.sh

One new entry, one new file, and the counts agree — so contiguity holds and there is no gap for the manifest guard to find.

One correction to my own check

My first pass printed duplicate keys: 0 from an awk that had errored on its own quoting — the zero came from wc -l over empty output, not from a comparison. Re-run with cut -f1-3, it is genuinely 0 of 1333. Worth saying because a zero from a broken command looks exactly like a zero from a clean one, which is the shape this repository keeps finding.

On merging past an approval

Do not treat "land #1115 first" as standing direction to merge past a stale approval — take it only as ordering. You were right that no code moved and still right that three of those four files are the ones this repo gets wrong at merge time. An approval names a tree.

On your fourth mutation

The dead sed step is the same shape as my deleted false-positive arm, and I think that is now a pattern worth naming: a draft can contain a step that asserts a design you did not build, and only mutation testing distinguishes it from one that does. Reading it will not, because it reads as intent. Both of ours survived review by a human and died to a mutation.