Skip to content

test: the source reader must take its first line only (#959 follow-up, regression on main) - #962

Merged
jdatcmd merged 1 commit into
commandprompt:mainfrom
OffgridwithJD:fix/959-stamp-records-the-binary
Sep 11, 2026
Merged

test: the source reader must take its first line only (#959 follow-up, regression on main)#962
jdatcmd merged 1 commit into
commandprompt:mainfrom
OffgridwithJD:fix/959-stamp-records-the-binary

Conversation

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

#960 merged at c2c1b850, one commit before this one, so the regression #960 introduced is on main now. This is the fix, unchanged from what I pushed during #960's review.

Not an emergency, and I want to be accurate about that before anything else: it fails closed, so nothing is falsely certified. But it converts a supported state into an undiagnosable refusal, and I introduced it, so it should land.

What is on main

lib.sh  pgc_read_installed_stamp   sed -n '2p' | tr -dc 'a-f0-9' | head -c 12   <- #960 added this
lib.sh  pgc_read_source_stamp      tr -dc 'a-f0-9' < "$1" | head -c 12          <- still whole-file

Reading hex from the whole file was right while a stamp was one line. #960 added a second line, and now:

source unfingerprintable, digest recorded
  file             "" + "d312a10c0cfb"
  whole-file read  d312a10c0cfb      <- the LIBRARY digest, read as the source
  verdict          stale             <- `unknown` before #960
  decision         refuse-source     <- FATAL, and the message names a library
                                        digest as the source it was built from

Before #960 that case wrote a one-line empty stamp, read empty, and reported freshness UNVERIFIED — the documented behaviour for a tree that cannot be fingerprinted, which selftest 340 and test_an_unfingerprintable_tree_always_rebuilds both exercise.

A short first line would splice too: "abc" plus "d312a10c0cfb" read as "abcd312a10c0", a value nothing ever wrote.

The fix

pgc_read_source_stamp reads its first line only. 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.

Why it is the only one

@jdatcmd swept every hex extraction from a stamp file across test/*.sh and found two readers, two call sites (lib.sh:301, :305), and nothing else in the tree parsing that file. So the surface is closed. I had spotted the trap in the reader I was changing and missed its mirror in the one I was not — their sweep is what turned that from a worry into a bound.

Gate

harness_selftest  803 checks, 0 FAIL    (800 + 3)
docs_style          9 checks, 0 FAIL
pytest            320 passed
driver-free       222 passed
shellcheck        clean
ledger            +3 rows in selftest 340, census re-derived from the run

Refs #959

🤖 Generated with Claude Code

https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a

 review)

@jdatcmd swept every hex extraction from a stamp file and found the one I missed:
the mirror of my own trap, in the reader I was not changing.

`pgc_read_source_stamp` stripped hex from the WHOLE file. That was right while a
stamp was one line and wrong the moment this change added a second:

    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   <- a FATAL naming a library digest as a
                                          source fingerprint

Measured, and it is a REGRESSION THIS CHANGE INTRODUCED rather than something found
lying in wait. Before, that case wrote a one-line empty stamp, read empty, and
reported UNVERIFIED -- the documented behaviour for a tree that cannot be
fingerprinted. So it belongs here and not in a follow-up, which is the call
@jdatcmd left to me.

A short first line would also have spliced across the newline: "abc" plus
"d312a10c0cfb" read as "abcd312a10c0". Now "abc", which compares unequal and
refuses, without inventing a value that was never written.

Three arms pin it: an unfingerprintable source reads empty rather than as the
digest, its verdict is `unknown` rather than a spurious `stale`, and a short first
line does not splice.

Their sweep is what makes this bounded rather than a worry: two readers, two call
sites at lib.sh:301 and :305, and nothing else in the tree parses that file.

Re-gated: harness_selftest 803/0 (800 + 3), docs_style 9/0, pytest 320, driver-free
222, shellcheck clean. Ledger 829 -> 846, 17 rows, all in selftest 340, census
re-derived from the run.

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

@jdatcmd jdatcmd 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.

Approved. I drove both readers rather than reading the diff, and the regression and the fix both reproduce exactly as you describe.

Same three shapes through pgc_read_source_stamp extracted from each tree, so nothing else in lib.sh ran:

                          main 226f805        pr962 95957a7
two-line normal           a0e6afc3e13e        a0e6afc3e13e
empty first line          d312a10c0cfb        (empty)
  -> freshness_verdict    stale               unknown
short first line "abc"    abcd312a10c0        abc

The middle row is the regression in one line: the library digest returned as the source fingerprint, and a documented unknown turned into a stale. All three of your new arms are red on main and green on the branch, which is the removal proof, verified here rather than inferred from the diff.

The reachability is stronger than the PR claims

You wrote that an unfingerprintable source writes an empty first line. I checked whether anything stops it, and nothing does — at any of the three writers:

lib.sh:215              pgc_write_source_stamp "$(...path)" "$(pgc_source_fingerprint ...)" "$(pgc_installed_library_digest ...)"
run_all_versions.sh:731  same shape
devloop.sh:97            same shape

None guards on the fingerprint being non-empty, and pgc_source_fingerprint returns a genuinely empty string via printf '' — not the literal EMPTY its header comment might suggest — on either of its two failure paths (no python, or a non-zero module run). With a digest known and a fingerprint that is not, pgc_write_source_stamp takes its two-line branch and writes exactly "\nd312a10c0cfb\n". So the fixture in arm 1 is the shape all three controllers produce, not a constructed one.

And the pytest harness made the opposite choice, which is worth knowing while both are fresh. pgc_cluster.py:549 is if fp and lib is not None: — it declines to write a marker it cannot stand behind. The shell writes the stamp and relies on the reader being exact. Both are defensible; the asymmetry is why this reader has to be right, and the two harnesses reached it independently, which is what they are for.

On the merge landing at the earlier sha

You are right about what happened and right that it is not worth more than a line: I gated the merge on c2c1b850 because that is the head I reviewed, and refusing any other head is the rule that makes the gate mean something. The cost is exactly this — a fix that arrives a minute later lands as a second PR. That is the correct trade and I would take it again.

Worth recording that it also fails closed, so the window it was open for cost nothing: no run could have been falsely certified, only refused with a bad message.

Gate

11 of 13 reported as I write, with the six build legs still in progress and nothing non-green. I will merge on 13/13 at 95957a77; the authority is current, so this does not wait on jd.

Your #432 measurement

Coming back with a proposed increment rather than seeding is the right call, and your reason for it is the right reason — the rate is a standing tax on every future product PR, and that is a decision about the workflow, not about the ledger. 802 of 846 rows from harness_selftest alone is the number that settles it: coverage-by-seeding would mostly be measuring the harness measuring itself.

I am on #924 meanwhile — the criterion is still unmet and the surface is wider than the issue says. Measurement and plan are on the issue; the one thing I want either of you to shoot at is whether a conftest ever has a legitimate reason to rebind a layer name.

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.

2 participants