Skip to content

test: a red needs a reason before it becomes a ledger row (#946) - #947

Merged
jdatcmd merged 5 commits into
mainfrom
fix/946-a-red-needs-a-reason
Sep 11, 2026
Merged

test: a red needs a reason before it becomes a ledger row (#946)#947
jdatcmd merged 5 commits into
mainfrom
fix/946-a-red-needs-a-reason

Conversation

@jdatcmd

@jdatcmd jdatcmd commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Fixes #946.

merge already refused a log that does not reconcile, and reconciliation is not the property that matters. Both logs that poisoned this ledger on the day it landed reconciled.

Two independent routes on day one, from the two people who knew the tool best. A third costs no imagination: a run against a stale .so. The rate is the argument, not either instance.

The change

An environment red and a real regression are identical in the log, so the tool cannot infer which and now makes the caller assert it — the same move check_ledger_budget.txt already makes when it names a census apart from a ceiling.

merge refuses a log carrying any FAIL unless the caller says which kind of red it is:

  • --mutation NAME — exists, means "I broke this deliberately"
  • --reds-are-real — new, "a genuine observation of the code under test"

Refused before any row is built, so a declined merge is never half-applied.

Not "refuse FAILs unless --mutation", which @OffgridwithJD proposed and we rejected together: a genuine CI red is the most valuable row this ledger holds and has no mutation to name, so that rule would refuse precisely the entry the ledger exists for. That is the deadlock the budget file already argues against for checks_never_observed_red.

A default refusal is affordable because nothing automated calls merge. grep -rn 'pgc_ledger.py merge' .github/ test/ finds one hit and it is the gate's own advice string; gate is the automated path. That was the cost I expected to weigh, and it does not exist.

Eight call sites updated in each harness, checked one at a time

Four pytest arms and ten shell checks broke. Every one used a red log as a vehicle for a different subject — duplicate detection, rename grouping, date monotonicity, mutation attribution. None asserts that a red merges without a reason, so passing the new flag keeps each testing what it tests. That is updating callers to a changed interface, not weakening tests.

One needed more. The control named "the same log merges without --mutation" stops being what it demonstrates, so it now says the refusal above is --mutation-across-two-checks and not the log. Both harnesses held their own copy and both broke independently, which is the independence rule working rather than a duplicated failure.

Checked rather than assumed: --date not-a-date still fails on the date, because that validation runs at the top of cmd_merge and this guard sits below it. A deny arm is evidence only if the call reached the code that denies it.

Proved by removal

guard neutered to `if False`     the new arm FAILS, alone
control                          an all-PASS log merges with no flag, rc=0

The artifacts, enumerated rather than discovered

#925 cost three rounds because I fixed whatever broke next instead of asking what a new check obliges. This time:

TESTS.md entry            added
NO_CLUSTER membership     already true
shell twin                added
parts.manifest            part 410 already listed, unchanged
check_ledger.tsv          +7 rows
check_ledger_budget.txt   census 826 -> 833

The change is its own demonstration: the regeneration log is 833 records with zero FAILs, so the guard added here does not refuse it. A red run would now be refused by the very thing being added — which is what #925's .git-less regeneration needed and did not have.

Gate

harness_selftest, PG17 assert   rc=0, 833 passed + 0 failed + 0 unrunnable + 0 skipped = 833
driver-free pytest              196 passed
ledger                          826 -> 833 rows, all 7 in 410, 0 removed or altered
census                          826 -> 833, derived from the ledger rather than typed
ceiling                         250, untouched

Based on main d05e3c3.

🤖 Generated with Claude Code

https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw

jdatcmd and others added 3 commits September 10, 2026 20:22
`merge` already refused a log that does not RECONCILE, and reconciliation is not
the property that matters. Both logs that poisoned this ledger on the day it
landed reconciled:

  - mine, on #925: 827 records against `checks run: 827`, fifteen checks red
    because I had copied the tree without `.git`. Flawless self-consistency,
    fifteen environment reds, and I caught it by noticing the failures rather
    than by the tool refusing them.
  - @OffgridwithJD's, on #944: one FAIL from an unfinished change, recording a
    red for part 350's doc-coverage check that the check never earned.

Two independent routes on day one, from the two people who knew the tool best. A
third costs no imagination: a run against a stale `.so`. THE RATE IS THE
ARGUMENT, not either instance.

AN ENVIRONMENT RED AND A REAL REGRESSION ARE IDENTICAL IN THE LOG. Nothing in a
RESULT record says which, so the tool cannot infer it and now makes the caller
assert it -- the same move `check_ledger_budget.txt` already makes when it names
a census apart from a ceiling.

`merge` refuses a log carrying any FAIL unless the caller says which kind of red
it is: `--mutation NAME` (exists, a deliberate break) or `--reds-are-real` (new,
a genuine observation of the code under test). Refused BEFORE any row is built,
so a declined merge is never half-applied.

NOT "refuse FAILs unless --mutation", which @OffgridwithJD proposed and we
rejected together. A genuine CI red is the most valuable row this ledger can hold
and has no mutation to name, so that rule would refuse precisely the entry the
ledger exists for -- the deadlock the budget file already argues against for
`checks_never_observed_red`.

A DEFAULT REFUSAL IS AFFORDABLE BECAUSE NOTHING AUTOMATED CALLS `merge`.
`grep -rn 'pgc_ledger.py merge' .github/ test/` finds one hit and it is the
gate's own advice string. `gate` is the automated path. That was the cost I
expected to have to weigh and it does not exist.

Both harnesses, independently implemented. The shell twin builds its own
fixtures and names no pytest file; the pytest twin drives the tool directly.

EIGHT EXISTING CALL SITES UPDATED, AND I CHECKED EACH RATHER THAN CHASING GREEN.
Four arms broke, all of them using a red log as a VEHICLE for a different subject
-- duplicate detection, rename grouping, date monotonicity, mutation attribution.
None asserts that a red merges without a reason, so passing the new flag keeps
each testing what it tests. One needed more than that: the control in
`test_a_mutation_names_one_check_not_every_casualty` was named "the same log
merges without --mutation", which stops being what it demonstrates. It now says
the refusal above is --mutation-across-two-checks and not the log.

Proved by removal, and by a control that must not move:

    guard neutered (`if False`)   the new arm FAILS, alone
    control                       an all-PASS log merges with no flag, rc=0
    driver-free pytest            196 passed

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw
The shell twin holds its own copies of the same call sites, and they broke
independently of the pytest ones -- which is the independence rule working rather
than a duplicate failure. Eight merge calls feed a red log as a VEHICLE for a
different subject (date monotonicity, duplicate naming, rename grouping, mutation
attribution); each now says why.

The control named 'the same log merges without --mutation' is renamed for the same
reason its pytest counterpart was: that is no longer what it demonstrates.

CHECKED, NOT ASSUMED: `--date not-a-date` still fails on the DATE, because the date
validation runs at the top of cmd_merge and this guard sits below it. A deny arm is
evidence only if the call reached the code that denies it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw
Seven new checks in part 410, so two tree-wide artifacts move with them. I
enumerated what a new check obliges rather than fixing whatever broke first,
which is the lesson #925 cost three rounds to learn:

    TESTS.md entry            done in the first commit
    NO_CLUSTER membership     already true, test_mutation_ledger.py is driver-free
    shell twin                done in the second commit
    parts.manifest            part 410 already listed, unchanged
    check_ledger.tsv          +7 rows, this commit
    check_ledger_budget.txt   census 826 -> 833, this commit

REGENERATED FROM A GREEN RUN, and this change is its own demonstration: the log is
833 records with zero FAILs, so the guard added here does not refuse it. A red run
would now be refused by the very thing being added, which is what #925's
`.git`-less regeneration needed and did not have.

    harness_selftest, PG17    rc=0, 833 passed + 0 failed + 0 unrunnable
                              + 0 skipped = 833
    ledger                    826 -> 833 rows, all 7 in 410-a-check-must-have-been-red
    rows removed or altered    0
    census                    826 -> 833, DERIVED from the ledger, not typed
    ceiling                   250, untouched -- adding checks to a covered suite
                              cannot move it, as the budget file predicts
    driver-free pytest        196 passed

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw
@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Reviewed at 34b45907 on pg18a (assert). Everything in the body checks out, and the implementation is right rather than merely correct. I went looking for one specific hole and it is not there.

The hole I expected to find

A guard over this property could read the human FAIL summary lines instead of the RESULT records. That would pass a log whose record is red while its summary is not — and it would look identical in every test you wrote, because your fixtures carry both.

This one reads runs, the parsed records, which is the same source the rows are built from. So:

red RESULT with NO human FAIL line      rc=2, refused      <- a ^FAIL grep would have merged this
human FAIL line with no red RECORD      rc=0, not refused  <- correct: that is reconcile's job

The second line is the right division of labour, not a gap: the ledger records records, and a record-less FAIL is a reconciliation failure rather than an unexplained red.

What I ran

All on a throwaway ledger, each from a fresh empty file:

input rc effect
green log, no flag 0 2 rows
red log, no flag 2 ledger byte-identical, 0 rows
red RESULT, no human FAIL line 2 refused
human FAIL line, no red record 0 1 row
--reds-are-real --date 2026-09-10 0 red recorded with its date
--mutation deliberate-break 0 red recorded with the name
--reds-are-real --date not-a-date 2 fails on the date
a log of only UNRUN and SKIP 0 2 rows — neither is a red
8 reds 2 6 listed, then ... and 2 more

"Refused before any row is built" I checked by md5, not by reading the position. The ledger is byte-identical after the refusal. Your shell arm "and nothing is written, so a refused merge is not half-applied" asserts the same thing, and with the guard neutered it reports got [51] want [0] — the strongest single arm in the change, because it is the difference between a refusal and a rollback.

The last row matters too: eight reds print six and then say how many were not listed. A silent truncation there would read as "these are the reds" when it is "these are some of them".

Removal proof, both harnesses

Guard neutered to if False and …, mutation asserted to have applied, file left well-formed, restored byte-exact:

pytest   1 failed, 12 passed   test_a_reconciling_log_with_a_red_is_not_evidence_on_its_own
shell    3 FAILs              the refusal, the naming, and the nothing-is-written arm

Small note for the body rather than the code: it says "the new arm FAILS, alone", which is exactly true of the pytest half and is three arms on the shell half. Both are sound removal proofs; the sentence reads as one arm in total.

Independence

Neither twin reaches across: 410 names nothing under test/pytest/, and test_mutation_ledger.py names no shell file. pgc_ledger.py lives in test/, so both harnesses driving it is two harnesses testing a shared tool rather than one driving the other.

The two counts in the body

I started writing these up as an inconsistency — "Eight call sites updated in each harness" against "Four pytest arms and ten shell checks broke" — and then measured instead of filing it. The removed side of the diff isolates updated sites from new ones: 9 removed merge-invoking lines per harness, of which one in each is the control's name line rather than a call site. Eight per harness, exactly as you wrote. The two numbers count different units, sites against arms-and-checks, and both are right.

Worth one clause saying so, because I am not the only reader who will try to reconcile them — and I nearly published the reconciliation as a defect.

Gate, matching yours

harness_selftest.sh (pg18a assert)  833 checks run, 0 FAIL
pgc_ledger.py gate                  rc=0, new this run=0, ceiling 250
driver-free pytest                  196 passed
full pytest corpus                  279 passed
reds in the run I generated         0

That last line is the claim I most wanted to check: the change does not refuse its own regeneration. It is the property #925's .git-less regeneration needed and did not have, and it is what makes this self-demonstrating rather than self-referential.

Not approving yet

CI is 11 of 13 with suites (PG 17) and suites (PG 18) still running. I approve on full green only, and I will re-check the head has not moved first.

The design decision I would call out as the right one: an environment red and a real regression are identical in the log, so the tool makes the caller assert which rather than inferring it. That is the same move check_ledger_budget.txt makes in naming a census apart from a ceiling, and it is why --reds-are-real is a better answer than the "refuse unless --mutation" I originally proposed — which would have refused the single most valuable row this ledger can hold.

@linuxhikerpm linuxhikerpm left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed exact head 34b4590729b2. Builds, shellcheck, and pytest-guards are green on this SHA; suites (PG 17/18) were still in flight at review time.

The patch matches #946. Reconciliation is not the property that matters, and this does not refuse FAILs unless --mutation: an all-PASS log still merges with no flag, --mutation still records a deliberate break, and --reds-are-real is the door for a genuine observation. The new check runs before any row is built. Independent twins cover the public merge seam from separately constructed logs (test/selftest/410-a-check-must-have-been-red.sh and test/pytest/test_mutation_ledger.py); neither reads the other.

Blocking: mergeable=CONFLICTING against current main (3d42c68210bb, #944). GitHub will not merge this SHA. #944 rewrote the ledger and TESTS.md and took checks_never_observed_red to 756. This branch's 833 is 826 + 7 from the pre-#944 baseline. Rebase onto current main and re-derive the census from a green run; do not keep 833.

Also drop the leftover ledger row when regenerating. The test renamed control: the same log merges without --mutation to control: the same log merges with a different reason, so the refusal above is --mutation-across-two-checks and not the log. Both names are in check_ledger.tsv on this SHA; only the new name is still a check. merge will not delete the old row.

Non-blocking: the existing --mutation two-FAIL error still says "Merge without --mutation". After this change that path also needs --reds-are-real. The refusal order is correct (--mutation two-FAIL still fires first); only the advice string is stale.

r and others added 2 commits September 10, 2026 21:23
…-reason

# Conflicts:
#	test/check_ledger_budget.txt
…946)

#944 landing took the census 826 -> 756 while this branch takes it to 833, so
neither number survives the compose. Arithmetic says 763. The rows are the source
and the number is derived, so it came from a run rather than from addition.

THE LEDGER AUTO-MERGED, WHICH IS THE DANGEROUS CASE, so I checked it against the
run rather than against its own count. It took main's deletions and this branch's
additions correctly:

    part 350  main 5   mine 53  -> 5      main's deletion kept
    part 370  main 0   mine 12  -> 0      main's deletion kept
    part 380  main 12  mine 21  -> 12     main's deletion kept
    part 410  main 111 mine 118 -> 118    this branch's addition kept

Verified by set comparison against a real run on the composed tree, not by count:
checks in the run absent from the ledger, 0.

AND ONE STALE ROW THAT WAS MINE TO CLEAN. Three ledger rows did not appear in the
run. Two are pre-existing conditional checks in part 330 that also sit in main's
ledger. The third is the check THIS BRANCH RENAMED: main's ledger carries it under
the old name, this branch adds it under the new one, and the merge kept both. The
rename-scan subcommand reports vanished=3 and cannot pair them, because the new
name was already present.

Dropped after checking it carried nothing: verdict "never", no mutation, so no
history was lost. A row with a date or a mutation would have had to travel with the
rename instead.

    selftest on the composed tree   763 checks, 762 passed + 1 failed
                                    the one failure was the census arm naming 763,
                                    which is the arm doing its job
    ledger                          763 -> 762 rows after the stale row
    census                          derived from the ledger, not typed
    ceiling                         250, untouched
    driver-free pytest              208 passed

AND A DEFECT IN THIS COMMIT'S OWN MESSAGE, fixed by amend: I wrote the first
version through an unquoted heredoc, so the backticked words rename-scan and never
were run as commands and left holes in the text. The message is written from a
file now. A commit message is the durable record and a hole in it is a hole in the
record.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw
@jdatcmd
jdatcmd force-pushed the fix/946-a-red-needs-a-reason branch from bbfadd9 to f80ca7d Compare September 11, 2026 03:26
@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Re-verified at f80ca7d0, the rebased head. The compose is right and the census reconciles exactly. Approving when CI settles.

The census

main                                756
this branch                         762     delta +6
part 410's ledger rows  111 -> 117           +6
checks added to 410                   7
checks removed (the renamed control)  1     net +6
selftest on this tree           763 checks  = main's 757 + 6

I went looking for a discrepancy here and briefly had one: I expected 763, because the PR's earlier version was +7. The seventh is the control you renamed"the same log merges without --mutation" became "…with a different reason, so the refusal above is --mutation-across-two-checks and not the log" — so one name leaves as another arrives and the net is 6. My arithmetic was wrong, not your number.

Worth noting because it is a property of this ledger that will keep coming up: a rename is a deletion and an insertion in the census, so "checks added" and "census delta" are different counts whenever a name changes. Yours reconcile on all three readings.

And the number came from a run on the composed tree, which was the whole reason to park the rebase. Good call.

Gate, pg18a (assert), at f80ca7d0

harness_selftest.sh     763 checks run, 0 FAIL
pgc_ledger.py gate      rc=0, new this run=0, ceiling 250
ledger                  762 rows, 762 never, 0 ever red
driver-free pytest      208 passed
full pytest corpus      293 passed

0 ever red is the one I check deliberately on this artifact now: the regeneration log was green, so the guard this PR adds does not refuse its own output, and no row carries a red it did not earn.

One note on reading the diff, for anyone who looks after me

git diff 34b45907..HEAD on this branch shows 15 files and all of #944's content, because the rebase moved the base past a merge. Against main it is the 6 files the PR actually touches. I nearly reported the wrong surface from the two-dot diff and mention it only because the same trap is one comment up in this repo's own history.

Everything from my earlier review still holds — the guard reads the RESULT verdict rather than a ^FAIL line, the refusal leaves the ledger byte-identical, and both harnesses' removal proofs discriminate. Nothing about the change moved; only its base.

@OffgridwithJD OffgridwithJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approving at f80ca7d0, the rebased head I verified. CI settled: 13 checks, 0 failures, none pending. Head re-checked before approving.

What the approval rests on:

harness_selftest.sh     763 checks run, 0 FAIL      = main's 757 + 6
pgc_ledger.py gate      rc=0, new this run=0, ceiling 250
ledger                  762 rows, 762 never, 0 ever red
driver-free pytest      208 passed
full pytest corpus      293 passed

The census reconciles on three independent readings — census 756 → 762, part 410's rows 111 → 117, and 7 checks added less the 1 renamed control — and it came from a run on the composed tree rather than arithmetic, which was the whole reason to park the rebase.

0 ever red is the line I check deliberately on this artifact now: the regeneration log was green, so the guard this PR adds does not refuse its own output, and no row carries a red it did not earn.

From the first review, all still true at this head: the guard reads the RESULT verdict rather than a ^FAIL line, so a log whose record is red while its summary is not is still refused; the refusal leaves the ledger byte-identical; --reds-are-real and --mutation both record correctly; a bad --date still fails on the date; UNRUN/SKIP are not reds; and eight reds print six with ... and 2 more rather than truncating silently.

The merge decision is @jdatcmd's, not mine.

@linuxhikerpm linuxhikerpm left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approving exact head f80ca7d053fb. MERGEABLE, CLEAN, 13/13 on this SHA.

The follow-up commit does both things the last review asked for. The renamed check control: the same log merges without --mutation is gone from the ledger (verdict was never, so no history moved). Census is 762 from a run on the composed tree, not 833 or 756+7. The #946 rule itself is unchanged: a reconciling FAIL still needs --mutation or --reds-are-real; an all-PASS log still merges with no flag.

@jdatcmd
jdatcmd merged commit d5c1f62 into main Sep 11, 2026
13 checks passed
jdatcmd added a commit to OffgridwithJD/pgcolumnar that referenced this pull request Sep 11, 2026
Merging main after commandprompt#942, commandprompt#947, commandprompt#949, commandprompt#950 and commandprompt#951 landed. Two conflicts and one
of them could not be resolved by reading the diff.

CHANGELOG.md -- union, both entries kept.

check_ledger_budget.txt -- THE NUMBER CAME FROM A RUN, not from arithmetic. This
branch carried 769 off main's old 756 baseline; commandprompt#947 has since landed 762; the
composed ledger takes both row sets and holds 775. None of 769, 762, or any sum
of deltas is the answer, because the rows are the source and the census is a
measurement of them.

    selftest on the composed tree, census left stale on purpose
        776 checks, 775 passed + 1 failed
        FAIL the committed census matches the committed ledger: got [762] want [775]

That arm naming 775 is the derivation; the confirmation is a second run:

    census set to 775, derived from the ledger
        776 checks, 776 passed + 0 failed

CHECKED BEFORE TRUSTING THE LOG, because a reconciling log can still be evidence
about the environment rather than the code (commandprompt#946):

    checks in the run absent from the ledger    0
    rows in the ledger absent from the run      2, both pre-existing conditional
                                                premises in part 330 that also sit
                                                in main's ledger
    FAIL records in the run                     1, the stale census arm itself

So the ledger's auto-merge was correct and the only thing wrong was the number
describing it. Ceiling untouched at 250: adding rows to an already-covered suite
cannot move it.

Third time this artifact has needed re-deriving on a merge. That is now a property
rather than an accident -- the census is a measurement of the tree, and every
merge invalidates it. Tracked in commandprompt#952.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw
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.

pgc_ledger.py merge records a red from any log that reconciles, and two of us poisoned the ledger that way on day one

3 participants