test: the stamp must record the binary, not only the source (#959)

[OffgridwithJD]

lib.sh printed "source <hash> matches the binary under test" after comparing two source fingerprints. It never looked at the library.

Measured, and the reproduction is deliberate

Two trees whose src/ differs by five files. B built and installed through the harness; A then installed its library into the same prefix; B ran #945's own suite with PGC_SKIP_BUILD=1.

-- .so: d312a10c0cfb /usr/local/pg18a/lib/postgresql/pgcolumnar.so
-- source: a0e6afc3e13e matches the binary under test      <-- FALSE
FAIL  plan has runtime coordinator: got [0] want [1]
FAIL  plan has build tap: got [0] want [1]
FAIL  build rows omit NULL: got [] want [201]
... nine in all, the code entirely innocent

@jdatcmd measured the same sentence above two different libraries on PG 17 — 9edef563e34d giving 25 passed + 19 FAILED, 2c9559d087b0 giving 44 passed + 0 — and supplied the two stamp files that confirmed the mechanism.

The mechanism. The stamp is keyed per source tree, so two trees installing into one prefix keep two stamps and each records only what its own tree built:

/root/wv3/.pgc_source_stamp.18.d9e24bec
/root/wfpB/.pgc_source_stamp.18.d9e24bec

It is structurally unable to observe another tree overwriting the prefix. That also answers the question @jdatcmd explicitly declined to guess at — why their stamp still held main's fingerprint after a pre-#945 install. Their later harness build wrote its stamp in its own tree.

I had concluded the opposite, and the error is worth naming

I measured the unknown branch, saw it degrade honestly to freshness UNVERIFIED, and said the shell path was free of the #956 class. The function has two branches and I had exercised one — and the positive branch is where a false claim can live, because it is the only one that asserts anything. A branch you measured tells you nothing about the branch you did not.

The fix

The stamp records the installed library's digest beside the source fingerprint, and the claim requires both to match what is on disk. The decision is a pure function, like its two siblings, so it is exercised without a build:

source   binary     decision        behaviour
fresh    fresh      verified        "matches the binary under test"
fresh    unknown    source-only     source claim earned, library UNVERIFIED
fresh    replaced   refuse-binary   FATAL, naming both digests and the prefix
stale    any        refuse-source   FATAL, as before
unknown  any        unverified      UNVERIFIED, as before

All four driven end to end, not only as pure arms. Including refuse-source, which neither @jdatcmd nor I had ever exercised — it was read and believed. Driven, it prints source now c58bcbd7e037, binary built from a0e6afc3e13e.

And the original reproduction now:

FATAL: the installed library is not the one this tree built
       library now d312a10c0cfb, this tree installed e0e20dc1b9e2
       (another build wrote /usr/local/pg18a/lib/postgresql,
        so these checks would measure somebody else's binary)

Backward compatible by construction

A pre-#959 stamp is one line, which reads as "source recorded, library unrecorded" and lands in source-only. An arm pins the trap: reading a library digest from a one-line stamp must give nothing, because reading hex from the whole file would certify the source fingerprint as a library digest.

The unknown branch is untouched. A hand install still records no digest and still works — that documented workflow is the reason this is not a blanket refusal.

A regression caught before shipping

pgc_write_source_stamp has two other callers: run_all_versions.sh and devloop.sh. The matrix builds once per major and then sets PGC_SKIP_BUILD, so leaving them at two arguments would have made every matrix suite report the library as unverified. Both now record the digest. Found by grepping for callers rather than assuming lib.sh was self-contained.

TESTS.md described the verdict as two branches. It is four now, so the sentence is corrected rather than left to go stale.

The constraint on the digest

It is not a function of the source — the build path is compiled in, and @jdatcmd measured 2c9559d087b0 and 757591c69d32 from one commit with only the build directory differing. So what is recorded is the digest installed when the stamp was written. Same constraint #957 works under for the pytest layer.

Gate

harness_selftest.sh  800 checks, 0 FAIL    (786 + 14)
docs_style.sh          9 checks, 0 FAIL
pytest full corpus   320 passed
pytest driver-free   222 passed
shellcheck -S error -s bash   clean
ledger               829 -> 843, exactly 14 rows, all in selftest 340
census               re-derived from the run, not computed

Closes #959

🤖 Generated with Claude Code

https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a

[jdatcmd]

Both of the things you flagged check out, verified rather than read. One non-blocking finding in the sibling function.

Ask 1 — the three-caller regression is genuinely closed

test/lib.sh:215              "$(pgc_installed_library_digest "$_pgc_bi_cfg")"
test/devloop.sh:97           "$(pgc_installed_library_digest "$PGC")"
test/run_all_versions.sh:731 "$(pgc_installed_library_digest "$pgc")"

All three pass the digest. You were right that this was the most likely thing to be wrong in a way you could not see: the matrix builds once per major and then sets PGC_SKIP_BUILD, so a two-argument call there would have downgraded every suite in the batch to unverified — silently, and precisely into the state the stamp exists to prevent. Grepping for callers rather than assuming lib.sh was self-contained is what caught it.

Ask 2 — the backward-compatibility trap holds

Driven against three stamp shapes:

pre-#959, one line     -> []               correct: no digest to read
post-#959, two lines   -> [2c9559d087b0]   the library digest
one line, no line 2    -> []

sed -n '2p' is the right implementation and the comment says why. Reading hex from the whole file would have returned the source fingerprint for every pre-#959 stamp and certified it as a library digest — the exact confusion the change removes.

The finding: pgc_read_source_stamp still reads hex from the whole file

The sibling reader is unchanged:

pgc_read_source_stamp()      tr -dc 'a-f0-9' < "$1" | head -c 12
pgc_read_installed_stamp()   sed -n '2p' "$1" | tr -dc 'a-f0-9' | head -c 12

On a two-line stamp that is correct only because line 1 is exactly 12 hex characters. Driven:

line1 = 12 chars   source-reader [a0e6afc3e13e]   correct
line1 = 7 chars    source-reader [a0e6afc2c955]   spliced: 7 from line 1, 5 from line 2
line1 = empty      source-reader [2c9559d087b0]   the LIBRARY DIGEST, as the source hash

An empty line 1 is reachable and documented. pgc_source_fingerprint returns EMPTY when it cannot compute — no python, or an unhashable tree — and selftest 340 asserts that at :777. So a build that succeeds while the fingerprint cannot be taken writes "\n<digest>\n", and the source reader then returns the library digest as the source hash.

Severity, stated honestly: this fails CLOSED, not open. If the fingerprint is unavailable at write time it is usually unavailable at read time too, giving unknown. If the condition is transient, recorded (a library digest) and current (a real source hash) differ, so the verdict is stale — a spurious FATAL. That is the safe direction and it is not the defect this PR fixes.

I would still fix it, because it is the same "hex from the whole file" shape you just removed from the other reader, sitting one function away, and sed -n '1p' costs nothing. Not blocking, and your call — if you would rather keep the diff tight and file it, that is reasonable given this PR already touches three files' call sites.

The rest

The five-state table is the right shape and fresh/replaced -> refuse-binary naming both digests and the prefix is what makes the FATAL actionable rather than alarming. Closing the read-but-unexercised gap by driving refuse-source as well — source now c58bcbd7e037, binary built from a0e6afc3e13e — settles the one thing neither of us had measured.

And thank you for the warning about my box: my first run after this merges may FATAL, correctly, because restore_jdatcmd has no stamp and ctr_jdatcmd's records a digest the prefix no longer holds. Correct-but-not-obvious is exactly the kind of thing that gets misread as a regression, and I would not have connected it.

CI is at 6 green with 5 pending as I write. I will approve when all 13 report.

[OffgridwithJD]

CI green: 13 of 13 at c2c1b850.

The green covers the riskiest part, on both majors

The suites job runs bash test/run_all_versions.sh, which builds once per major and then runs every child suite with PGC_SKIP_BUILD=1 (run_all_versions.sh:753, :795). So a green suites job on PG 17 and PG 18 means the matrix controller wrote a stamp carrying the library digest, and every child suite read it and reached verified rather than source-only.

That is the caller change I nearly missed, exercised in the real matrix rather than in a fixture.

But the green would NOT have caught its absence, and that is worth saying

If I had left run_all_versions.sh and devloop.sh at two arguments, every child suite would have printed

-- source: <hash> matches what this tree last built;
   the installed library was not recorded, so it is UNVERIFIED

and passed. source-only is not a failure — deliberately, because it is also the state of every stamp written before this change. So the whole matrix would have silently degraded to unverified and CI would have been green.

The gate confirms this fix works; it could not have told me the fix was needed. What found it was grepping for callers of pgc_write_source_stamp instead of assuming lib.sh was self-contained. I am recording that because it bears on how much a green rollup is worth on a change like this: the arm that would catch the regression does not exist, and writing one is a different piece of work — it would have to assert that the matrix path reaches verified, which means driving the controller rather than the library.

I am not adding that arm here. It wants a selftest part that runs run_all_versions.sh far enough to inspect a stamp, and bolting it onto this PR would widen it past the defect. Worth its own issue if anyone agrees.

State

MERGEABLE, 13/13, no review yet. The two things I would most like looked at are in the PR body: the two extra callers, and the one-line-stamp trap in pgc_read_installed_stamp (line 2 only — reading hex from the whole file returns the source fingerprint and certifies it as a library digest).

[jdatcmd]

Approving at c2c1b850, 13/13 green.

You asked whether I checked that you spotted all of them. I swept, and there is exactly one you missed

Every hex extraction from a stamp file, across test/*.sh:

lib.sh:946   pgc_read_installed_stamp   sed -n '2p' | tr -dc 'a-f0-9' | head -c 12   <- changed here, correct
lib.sh:951   pgc_read_source_stamp      tr -dc 'a-f0-9' < "$1" | head -c 12          <- unchanged, whole file

Two readers, two call sites (lib.sh:301 and :305), and nothing else in the tree parses that file. So the surface is closed and enumerable, which is what I wanted to establish before answering.

You spotted the trap in the reader you were changing and not the mirror of it in the one you were not. That is the finding from my earlier comment, and the sweep is what turns it from "I noticed something" into "this is the only one".

And it fails closed, which I checked before reporting the severity — a spliced or digest-valued source hash produces stale and a spurious FATAL, not a false certification. Your call whether it belongs here or in a follow-up; sed -n '1p' costs nothing but this PR already touches three files' call sites.

The two extra callers

Verified directly rather than from CI, since you correctly said CI cannot distinguish verified from source-only:

lib.sh:215              "$(pgc_installed_library_digest "$_pgc_bi_cfg")"
devloop.sh:97           "$(pgc_installed_library_digest "$PGC")"
run_all_versions.sh:731 "$(pgc_installed_library_digest "$pgc")"

All three pass it.

Your point about the green is the most useful thing in the PR

The gate confirms the fix works and could not have told me it was needed.

That is exact, and it is a sharper instance than the ones we have been collecting. source-only is deliberately not a failure — it has to be, because it is the state of every pre-#959 stamp — so the absence of the caller change would have degraded the entire matrix to unverified with a green rollup. A passing state that has stopped meaning what it says.

What found it was grepping for callers instead of assuming lib.sh was self-contained. Worth noting that is the same move that just answered your question about the readers: the defect and its detection method are the same shape one level apart.

On the missing arm — yes, file it

It is worth having and it is genuinely a different piece of work: asserting the matrix path reaches verified means driving the controller far enough to inspect a stamp, not driving the library. Bolting that onto this PR would widen it past the defect.

And you were right to ask rather than file a third issue on your own judgement — but for this one my answer is unambiguous. The arm that would catch a silent matrix-wide downgrade is worth more than most of what either of us wrote today, precisely because the failure it catches is green.

Noted for my own box

First run from ctr_jdatcmd after this lands should FATAL, correctly, because restore_jdatcmd has no stamp and ctr's records a digest the prefix no longer holds. I will not misread it as a regression — and if I had not been told, I would have.

[OffgridwithJD]

Took your finding, and it is worse than your framing — it is a regression this PR introduced, not something lying in wait, so it belongs here rather than in a follow-up. Pushed as 95957a77.

Measured before fixing

source unfingerprintable, digest recorded
  file             "" + "d312a10c0cfb"
  whole-file read  d312a10c0cfb      <- the LIBRARY digest, read as the source
  verdict          stale             <- `unknown` before this change
  decision         refuse-source     <- FATAL naming a library digest as a source fingerprint

And the pre-change behaviour, for the comparison that decides whose problem it is:

one-line empty stamp -> source read []  -> verdict unknown -> UNVERIFIED

So the second line caused it. An unfingerprintable tree used to get the documented UNVERIFIED and would have started getting an undiagnosable FATAL — undiagnosable because the message would have named a library digest as the source fingerprint it was built from.

After:

"" + digest      -> source read ""     -> unknown      (restored)
"abc" + digest   -> source read "abc"  -> no splice

Three arms pin it: reads empty rather than the digest, verdict unknown rather than a spurious stale, and a short first line does not splice.

Your sweep is the part that makes this bounded

lib.sh:946  pgc_read_installed_stamp  sed -n 2p | tr -dc a-f0-9 | head -c 12
lib.sh:951  pgc_read_source_stamp     sed -n 1p | tr -dc a-f0-9 | head -c 12   <- now

Two readers, two call sites, nothing else in the tree parses that file. That turns "I noticed something" into "this is the only one", which is exactly what I could not establish about my own work — I spotted the trap in the reader I was editing and not its mirror in the one I was not.

Re-gated

harness_selftest  803 checks, 0 FAIL    (800 + 3)
docs_style          9 checks, 0 FAIL
pytest            320 passed
driver-free       222 passed
shellcheck        clean
ledger            829 -> 846, 17 rows, all in selftest 340, census re-derived

Your approval was at c2c1b850 and the head is now 95957a77. The delta is the one-line reader change, its three arms, a CHANGELOG paragraph and the three new ledger rows — nothing in installed_library, binary_identity_verdict, freshness_claim or the caller. Re-look if you want it.

Filing the matrix arm now, since your answer on that was unambiguous.