From dcedcf2aa775916e798b2ea91699114621415b64 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 9 Sep 2026 15:38:58 -0600 Subject: [PATCH 1/5] test: a fingerprint that could not be computed must not look like one that was `pgc_source_fingerprint` could return a confident WRONG hash, silently, with status 0. Three defects, one function, all closed here with arms in both harnesses. ## 1. A failed digest was substituted as an empty one printf '%s %s\n' "${_pgc_fp_f#"$dir"/}" \ "$(md5sum < "$_pgc_fp_f" 2>/dev/null | cut -d' ' -f1)" `2>/dev/null` inside a command substitution turns a FAILED md5sum into an EMPTY digest rather than an error. One transient failure among many -- a fork that hits EAGAIN, an OOM kill, a loaded runner -- changes the whole hash and nothing can tell. Modelled with a stub md5sum that fails on its Nth call and is otherwise the real binary: baseline (real md5sum) c8e6b23db1c9 one digest empty (call #2) 22897add806e one digest empty (call #3) 58c76fdab962 exit status 0 Three different confident answers over ONE unchanged tree. **On the READ side that costs one suite at random.** #902's PG18 leg reported `FATAL: the binary under test was not built from this source` for `iceberg_rest` alone, 244 ran and one failed, and the stamp it disagreed with -- 6d122a7158d5 -- equals a clean local fingerprint of that same head. So the WRITE was right and one READ was not. **On the WRITE side it is worse and deterministic.** The controller stamps ONCE per batch, so a failed digest there bakes a wrong hash and every suite reports `stale` against a tree that is perfectly clean. Measured by @OffgridwithJD, stub failing only during the write, all five reads healthy: the tree's TRUE fingerprint 059c5c2f3cca the stamp the controller wrote 79742b3335ab (rc=0, nothing warned) suite 1..5 -> stale `stale` is the FATAL. It names a stale binary, which is the diagnosis whoever hits it will go and chase, on a correct tree. The fix is not that the computation cannot fail. It is that a failure is REPORTED as one: empty, which `pgc_freshness_verdict` already turns into `unknown` and the controller already prints as `freshness UNVERIFIED` and deliberately does not fail. **The asymmetry is the whole argument.** A false UNVERIFIED costs a line of output. A false FATAL costs a matrix and teaches people to re-run past a freshness check, which is the failure this controller exists to prevent. ## 2. One tree hashed three ways, depending on how the path was spelled @OffgridwithJD's finding, reproduced and widened here. `${f#"$dir"/}` strips a prefix that must match character for character: plain 92410d0598d6 trailing slash bf101efc7c10 differs dot segment /./ 774152fff929 differs (not in the original report) via symlink 3f3c0e36905a differs dot-dot /src/.. 92410d0598d6 relative . 92410d0598d6 `/./` is the one worth keeping: it is what a `$(dirname X)/./` composition produces and it reads as harmless. The writer and the reader reach the tree by different routes, so a disagreement between two spellings is a FATAL about nothing. Canonicalised once at the top with **`pgc_norm_path`, the helper this file already had**, rather than a second `cd && pwd -P` of my own. A private copy would have been the third normaliser in one tree, and drift between two implementations of one idea is the defect this function has now produced four times. ## 3. The fix's own trap, caught before it shipped Detecting a failed digest means capturing the per-file lines to inspect them, and `$(...)` STRIPS THE TRAILING NEWLINE that the old straight pipe into md5sum included. Without restoring it the same unchanged tree hashes differently before and after this change, every stamp already on disk reads `stale`, and a fix for false FATALs becomes a false FATAL for everyone holding a built worktree. @OffgridwithJD hit it and warned me before I wrote it. **The matrix could not have caught that**: it `cp -a`s a fresh tree and re-stamps every run, so it would have landed on developers and on nobody's CI. Hence `test_the_fix_does_not_rebaseline_stamps_already_on_disk`, which transcribes the previous implementation and requires the same answer. That is a COMPATIBILITY assertion, not a tidiness one. ## The arms, and the proof that each can fail Written first and run RED before the fix existed: 6 failures, all of them the new arms, each for the intended reason. Then GREEN, and then each fix reverted separately with the mutation asserted applied: revert .sh arms red pytest red the digest swallow 3 2 the canonicalisation 3 1 the trailing newline 1 1 Two controls, because the set is vacuous without them. `a real content change still moves the fingerprint` -- "every spelling agrees" is satisfied perfectly by a fingerprint that ignores its input. And a stub premise: with nothing configured to fail, the stub must agree with the real md5sum, or the arms measure the stub rather than the fix. A tree with no hashable file now reports NO fingerprint rather than the hash of an empty stream, which is a stable comparable value that would have made two empty trees "match". ## Verified harness_selftest.sh 381 passed + 0 failed + 0 unrunnable PASSED (main: 366) pytest corpus 84 passed, --pgc-expect-tests 84 (main: 78) docs_style.sh 9 checks PASSED TESTS.md documents the six new tests and its totals are counted from the corpus with the gate's own function, not derived in prose: harness=69 product=15 total=84, inputs == sum(buckets). **WHAT THIS DOES NOT EXPLAIN, corrected after this branch's own CI refuted me.** An earlier version of this message said the mechanism was sufficient to explain #902's PG18 leg. That was wrong. This branch's PG17 leg then failed at a head carrying the whole fix, with the IDENTICAL hash pair #902 produced: #902 PG18 iceberg_rest source now a735c673b129, binary built from 6d122a7158d5 #909 PG17 iceberg_rest_server source now a735c673b129, binary built from 6d122a7158d5 A stochastic digest failure gives a DIFFERENT wrong hash every time -- my own stub gave 22897add806e, 58c76fdab962 and 1d7c00654866 -- so an identical value twice is a deterministic state. And PG17 and PG18 use different build directories, so a path-derived hash would have differed between them, which rules out the spelling defect as the cause too. The three defects fixed here are real and measured; they are not the cause of the iceberg_rest* failures, which remain UNEXPLAINED. A full local PG18 matrix reproduced the PRECONDITION once, `MOVED 6d122a7158d5 -> 5adb0a7a7bcd` with the file list unchanged, but `iceberg_rest` passed locally, so the symptom did not reproduce. The watcher that caught the movement printed "a file's CONTENT changed" on the strength of the list being unchanged, which is equally true of a failed digest; it could not tell the two apart and that conclusion is withdrawn. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK --- test/lib.sh | 69 ++++++- test/pytest/TESTS.md | 48 ++++- test/pytest/test_build_refusal.py | 168 ++++++++++++++++ .../340-the-binary-must-be-built-from.sh | 190 ++++++++++++++++++ 4 files changed, 468 insertions(+), 7 deletions(-) diff --git a/test/lib.sh b/test/lib.sh index 9fc7baf3..fb63e313 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -645,10 +645,29 @@ pgc_source_build_dirs() { # pgc_source_build_dirs DIR -> dirs -printf '%h\n' 2>/dev/null | grep -v "^$dir/src$" || true } -pgc_source_fingerprint() { # pgc_source_fingerprint DIR -> hash +# A value no md5sum digest can be, so it cannot be confused with one. +_pgc_fp_failed='PGC_FINGERPRINT_DIGEST_FAILED' + +pgc_source_fingerprint() { # pgc_source_fingerprint DIR -> hash, or EMPTY if it could not be computed local dir="${1:-.}" - local d - { + local d out + # ONE TREE HASHES ONE WAY, HOWEVER THE PATH IS SPELLED. `${f#"$dir"/}` below + # strips a prefix that has to match character for character, so `$dir` with a + # trailing slash, or with a `/./` segment, or reached through a symlink, put + # the full ABSOLUTE path into the digest instead of the tree-relative one and + # the same tree hashed three different ways (@OffgridwithJD). Canonicalised + # ONCE here rather than defended at each call site, so a future caller cannot + # reintroduce it: the writer and the reader reach the tree by different routes + # and a disagreement between them is a FATAL about nothing. + # + # pgc_norm_path rather than a second `cd && pwd -P` of my own. This file has + # spent the day proving that two implementations of one idea drift, and a + # private copy here would be the third normaliser in one tree. Its fallback + # hands back the raw path for a directory that cannot be entered, which needs + # no special case: nothing is found under it, `out` is empty, and the guard + # below returns no fingerprint. + dir="$(pgc_norm_path "$dir")" + out="$({ while IFS= read -r d; do [ -n "$d" ] || continue find "$d" -maxdepth 1 -type f \( -name '*.c' -o -name '*.h' \ @@ -674,9 +693,47 @@ pgc_source_fingerprint() { # pgc_source_fingerprint DIR -> hash # source that cannot produce any binary at all. The path makes the # partition part of the input, and the per-file digest is an # unambiguous boundary between one file's bytes and the next's. - printf '%s %s\n' "${_pgc_fp_f#"$dir"/}" \ - "$(md5sum < "$_pgc_fp_f" 2>/dev/null | cut -d' ' -f1)" - done | md5sum | cut -c1-12 + # + # A DIGEST THAT FAILED MUST NOT LOOK LIKE ONE THAT SUCCEEDED. This was + # `$(md5sum < "$f" 2>/dev/null | cut -d' ' -f1)` inline, so a failed + # md5sum -- a fork that hits EAGAIN, an OOM kill, a loaded runner -- + # contributed an EMPTY digest and the function returned a confident + # WRONG hash with status 0. One stubbed failure among many, on one + # unchanged tree, gave three different answers: + # + # baseline c8e6b23db1c9 + # one digest empty (call 2) 22897add806e + # one digest empty (call 3) 58c76fdab962 + # + # On the READ side that costs one suite at random, which is what #902's + # PG18 leg showed. On the WRITE side it is worse and deterministic: a + # failed digest while the controller stamps bakes a wrong hash, and every + # suite in the batch then reports `stale` -- a FATAL naming a stale binary + # -- against a tree that is perfectly clean (@OffgridwithJD, measured: + # 5 of 5 suites stale on a correct tree). + _pgc_fp_h="$(md5sum < "$_pgc_fp_f" 2>/dev/null | cut -d' ' -f1)" + [ -n "$_pgc_fp_h" ] || { printf '%s\n' "$_pgc_fp_failed"; break; } + printf '%s %s\n' "${_pgc_fp_f#"$dir"/}" "$_pgc_fp_h" + done)" + # Empty rather than a hash, which pgc_freshness_verdict turns into `unknown` + # and the controller prints as "freshness UNVERIFIED" -- already designed, and + # already deliberately not a failure. The asymmetry is the whole argument: a + # false UNVERIFIED costs a line of output, a false FATAL costs a matrix AND + # teaches people to re-run past a freshness check, which is the failure this + # controller exists to prevent. + case "$out" in *"$_pgc_fp_failed"*) printf ''; return 0 ;; esac + # A tree with no hashable file cannot be verified, so it gets no fingerprint + # either. This also keeps the line below off an empty `out`, whose trailing + # newline would otherwise be the ONLY input to md5sum. + [ -n "$out" ] || { printf ''; return 0; } + # THE TRAILING NEWLINE IS LOAD-BEARING. The old code piped the loop straight + # into md5sum, so the stream md5sum saw ended with one; `$(...)` strips it. + # Without `printf '%s\n'` here the same unchanged tree hashes differently + # before and after this change, every stamp already on disk reads `stale`, and + # a fix for false FATALs becomes a false FATAL for everyone holding a built + # worktree (@OffgridwithJD, caught before it shipped). The matrix could not + # have caught it: it copies a fresh tree and re-stamps every run. + printf '%s\n' "$out" | md5sum | cut -c1-12 } # fresh the binary was built from this source diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 1b455763..1cd12e5b 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -4,7 +4,7 @@ Reference for anyone reading, running, or adding to `test/pytest/`. The design a the decisions behind the harness are in `design/ISSUE_432_PYTEST_HARNESS.md`. This file covers the tests themselves. -**78 tests in 6 files.** Sixty-three of them test the harness rather than the +**84 tests in 6 files.** Sixty-nine of them test the harness rather than the product, and they come first, because a harness that can report a false green makes every other result in this directory worthless. @@ -429,6 +429,52 @@ an unreadable postmaster start time, or a non-numeric epoch all produce. The boundary arm is separate on purpose: mtime resolution is one second, so a run fast enough to install and start within the same second must not refuse itself. +### The fingerprint's own integrity, and why it needed six more arms + +Six tests, added with the fix that closed three defects in `pgc_source_fingerprint` +itself. The subject is the instrument every other arm in this section depends on: +if the fingerprint can be wrong, `never report on source you did not build` reports +on nothing. + +`test_a_failed_digest_yields_no_fingerprint_rather_than_a_wrong_one` drives the +real shell function with a **stub `md5sum` that is the real one except on its Nth +call**, where it fails with no output — a fork that hits `EAGAIN`, an OOM kill, a +loaded runner. The old code substituted that empty digest into the hash and +returned status 0, so one unchanged tree produced three different confident +answers. The premise arm requires the stub to agree with the real `md5sum` when +nothing is configured to fail, or the test would be measuring the stub. + +`test_a_failed_digest_gives_unverified_and_never_a_false_stale` is the property +that matters. `stale` is the FATAL; `unknown` prints `freshness UNVERIFIED` and +runs the suites. The asymmetry is the whole argument for the change: a false +UNVERIFIED costs a line of output, a false FATAL costs a matrix **and** teaches +people to re-run past a freshness check, which is the failure this controller +exists to prevent. + +`test_one_tree_hashes_one_way_however_the_path_is_spelled` pins five spellings — +trailing slash, `/./`, `/src/..`, a symlink, and a relative `.` — against the plain +path. Three of them disagreed before the fix, because `${f#"$dir"/}` strips a +prefix that has to match character for character. + +`test_the_fix_does_not_rebaseline_stamps_already_on_disk` is a **compatibility** +assertion rather than a tidiness one, and it is the arm that would have caught the +worst version of this change. Detecting a failed digest means capturing the +per-file lines to inspect them, and `$(...)` strips the trailing newline that the +old straight pipe into `md5sum` included. Without restoring it, the same unchanged +tree hashes differently before and after the fix, every stamp already on disk reads +`stale`, and a fix for false FATALs becomes a false FATAL for everyone holding a +built worktree. The matrix cannot catch that: it copies a fresh tree and re-stamps +every run, so it lands on developers and on nobody's CI. The arm transcribes the +previous implementation and requires the same answer. + +`test_the_fingerprint_still_moves_on_a_real_change` is the control without which +the spelling arms are vacuous — "every spelling agrees" is satisfied perfectly by a +fingerprint that ignores its input. + +`test_a_tree_with_nothing_hashable_reports_no_fingerprint` closes the last one: the +hash of an empty stream is a stable, comparable value, so two trees with no source +would have *matched*. + ## 6. test_docs_cover_the_corpus.py: this document, checked The file you are reading is checked mechanically, because it went stale inside a diff --git a/test/pytest/test_build_refusal.py b/test/pytest/test_build_refusal.py index 71cc9773..b1ed5d18 100644 --- a/test/pytest/test_build_refusal.py +++ b/test/pytest/test_build_refusal.py @@ -20,6 +20,7 @@ cannot reach and which is where a wrong quote would hide. """ +import os import pathlib import subprocess @@ -455,3 +456,170 @@ def test_the_two_fingerprint_implementations_cover_the_same_inputs(tmp_path, exp path.write_text(old) expect.text(f"{sh_after != sh_before} {py_after != py_before}", "True True", f"editing {edit} moves both fingerprints") + + +# --------------------------------------------------------------------------- +# THE TWIN of test/selftest/340's fingerprint-integrity arms. Two observers, one +# implementation: these drive the real shell functions through bash rather than +# reimplementing them, for the reason four fingerprint defects in one day proved +# — a second implementation of one idea drifts, and the drift is invisible until +# someone diffs the two. + + +def _sh_fp(expr, path_env=None, env=None): + """Evaluate a lib.sh expression, optionally with a stubbed PATH.""" + script = f'. "{SRCDIR}/test/lib.sh" || exit 1; {expr}' + e = dict(os.environ) + if env: + e.update(env) + if path_env: + e["PATH"] = path_env + ":" + e.get("PATH", "") + p = subprocess.run(["bash", "-c", script], capture_output=True, text=True, env=e) + return p.stdout.strip(), p.returncode + + +def _fp_tree(tmp_path, name="t"): + t = tmp_path / name + (t / "src").mkdir(parents=True, exist_ok=True) + (t / "objstore").mkdir(parents=True, exist_ok=True) + (t / "src" / "a.c").write_text("int a;\n") + (t / "src" / "b.c").write_text("int b;\n") + (t / "src" / "c.c").write_text("int c;\n") + (t / "objstore" / "m.c").write_text("int m;\n") + (t / "objstore" / "Makefile").write_text("all:\n\ttrue\n") + (t / "Makefile").write_text("all:\n\ttrue\n") + (t / "pgcolumnar.control").write_text("x\n") + return t + + +def _md5_stub(tmp_path, fail_on): + """A stub md5sum: real, except that its Nth call fails with no output. + + Models one transient failure -- a fork that hits EAGAIN, an OOM kill, a + loaded runner -- rather than a permanently broken md5sum, because the + permanent case is not the one that produced a wrong answer in CI. + """ + b = tmp_path / f"bin{fail_on}" + b.mkdir(exist_ok=True) + counter = tmp_path / f"count{fail_on}" + counter.write_text("0") + (b / "md5sum").write_text( + "#!/bin/bash\n" + f'_n=$(( $(cat "{counter}" 2>/dev/null || echo 0) + 1 ))\n' + f'echo "$_n" > "{counter}"\n' + f'[ "$_n" = "{fail_on}" ] && exit 1\n' + 'exec /usr/bin/md5sum "$@"\n' + ) + (b / "md5sum").chmod(0o755) + return str(b) + + +def test_a_failed_digest_yields_no_fingerprint_rather_than_a_wrong_one(tmp_path, expect): + """`$(md5sum ... 2>/dev/null)` substituted an EMPTY digest and returned rc=0. + + One failed invocation among many silently changed the whole hash, so the + function gave three different confident answers for one unchanged tree. + """ + t = _fp_tree(tmp_path) + base, rc = _sh_fp(f'pgc_source_fingerprint "{t}"') + expect.at_least(len(base), 12, "premise: the tree fingerprints at all") + + # Premise for the stub: with nothing configured to fail it must agree with + # the real md5sum, or the arms below measure the stub and not the fix. + quiet = _md5_stub(tmp_path, 0) + got, _ = _sh_fp(f'pgc_source_fingerprint "{t}"', path_env=quiet) + expect.text(got, base, "premise: the stub agrees with md5sum when nothing fails") + + for n in (2, 3): + stub = _md5_stub(tmp_path, n) + got, _ = _sh_fp(f'pgc_source_fingerprint "{t}"', path_env=stub) + expect.text(got or "empty", "empty", + f"a failed digest on file {n} yields no fingerprint") + + +def test_a_failed_digest_gives_unverified_and_never_a_false_stale(tmp_path, expect): + """The property that matters. `stale` is the FATAL; `unknown` is UNVERIFIED. + + A false UNVERIFIED costs a line of output. A false FATAL costs a matrix and + teaches people to re-run past a freshness check. + """ + t = _fp_tree(tmp_path, "v") + base, _ = _sh_fp(f'pgc_source_fingerprint "{t}"') + stub = _md5_stub(tmp_path, 2) + verdict, _ = _sh_fp( + f'pgc_freshness_verdict "{base}" "$(pgc_source_fingerprint "{t}")"', + path_env=stub) + expect.text(verdict, "unknown", "a failed digest reads as unknown, not stale") + healthy, _ = _sh_fp(f'pgc_freshness_verdict "{base}" "$(pgc_source_fingerprint "{t}")"') + expect.text(healthy, "fresh", "control: an unstubbed run still reads fresh") + + +def test_one_tree_hashes_one_way_however_the_path_is_spelled(tmp_path, expect): + """`${f#"$dir"/}` strips a prefix that must match character for character.""" + t = _fp_tree(tmp_path, "s") + link = tmp_path / "s_link" + link.symlink_to(t) + plain, _ = _sh_fp(f'pgc_source_fingerprint "{t}"') + expect.at_least(len(plain), 12, "premise: the fixture fingerprints at all") + for label, spelling in ( + ("a trailing slash", f"{t}/"), + ("a /./ segment", f"{t}/./"), + ("a /src/.. segment", f"{t}/src/.."), + ("a symlink", str(link)), + ): + got, _ = _sh_fp(f'pgc_source_fingerprint "{spelling}"') + expect.text(got, plain, f"{label} hashes the same tree the same way") + rel, _ = _sh_fp(f'cd "{t}" && pgc_source_fingerprint .') + expect.text(rel, plain, "a relative path hashes the same tree the same way") + + +def test_the_fix_does_not_rebaseline_stamps_already_on_disk(tmp_path, expect): + """A COMPATIBILITY assertion, not a tidiness one. + + Capturing the per-file lines to inspect them strips the trailing newline the + old straight-pipe into md5sum included. Without restoring it the same + unchanged tree hashes differently before and after the fix, every stamp on + disk reads `stale`, and a fix for false FATALs becomes a false FATAL for + everyone holding a built worktree. The matrix cannot catch this: it copies a + fresh tree and re-stamps every run. + """ + t = _fp_tree(tmp_path, "c") + previous = r''' +_prev() { local dir="$1" d + { while IFS= read -r d; do [ -n "$d" ] || continue + find "$d" -maxdepth 1 -type f \( -name '*.c' -o -name '*.h' -o -name 'Makefile' \) -print0 2>/dev/null + done < <(pgc_source_build_dirs "$dir") + find "$dir" -maxdepth 1 -type f \( -name 'Makefile' -o -name '*.control' -o -name '*.sql' \) -print0 2>/dev/null + } | sort -z | while IFS= read -r -d '' _f; do + printf '%s %s\n' "${_f#"$dir"/}" "$(md5sum < "$_f" 2>/dev/null | cut -d' ' -f1)" + done | md5sum | cut -c1-12; } +''' + now, _ = _sh_fp(f'pgc_source_fingerprint "{t}"') + before, _ = _sh_fp(previous + f'_prev "{t}"') + expect.text(now, before, + "the fixed fingerprint equals what the previous implementation produced") + + +def test_the_fingerprint_still_moves_on_a_real_change(tmp_path, expect): + """Without this the spelling arms are vacuous. + + "Every spelling agrees" is satisfied perfectly by a fingerprint that ignores + its input, so the set needs one arm proving the hash still moves. + """ + t = _fp_tree(tmp_path, "m") + before, _ = _sh_fp(f'pgc_source_fingerprint "{t}"') + (t / "src" / "a.c").write_text("int a = 2;\n") + after, _ = _sh_fp(f'pgc_source_fingerprint "{t}"') + expect.text(str(after != before), "True", "a real content change moves the fingerprint") + (t / "src" / "a.c").write_text("int a;\n") + restored, _ = _sh_fp(f'pgc_source_fingerprint "{t}"') + expect.text(restored, before, "and restoring the content restores it") + + +def test_a_tree_with_nothing_hashable_reports_no_fingerprint(tmp_path, expect): + """The hash of an empty stream is a stable, comparable value: two empty + trees would have "matched".""" + empty = tmp_path / "hollow" + empty.mkdir() + got, _ = _sh_fp(f'pgc_source_fingerprint "{empty}"') + expect.text(got or "empty", "empty", "an unhashable tree yields no fingerprint") diff --git a/test/selftest/340-the-binary-must-be-built-from.sh b/test/selftest/340-the-binary-must-be-built-from.sh index 14366f49..059ed472 100644 --- a/test/selftest/340-the-binary-must-be-built-from.sh +++ b/test/selftest/340-the-binary-must-be-built-from.sh @@ -435,3 +435,193 @@ check "so the verdict is fresh, not unknown" \ "$(pgc_source_fingerprint "$_wr")")" "fresh" unset _wr _wr_rc _wr_written _wr_expected _wr_f + +# --------------------------------------------------------------------------- +# A FINGERPRINT THAT COULD NOT BE COMPUTED MUST NOT LOOK LIKE ONE THAT WAS. +# +# `$(md5sum < "$f" 2>/dev/null | cut -d' ' -f1)` substitutes an EMPTY digest +# when md5sum fails, so one transient failure -- a fork that hits EAGAIN, an +# OOM kill, a loaded runner -- silently changes the whole hash and the function +# still returns 0. Modelled with a stub md5sum that fails on its Nth call and is +# otherwise the real one, three different confident answers over ONE unchanged +# tree: +# +# baseline c8e6b23db1c9 +# one digest empty (call 2) 22897add806e +# one digest empty (call 3) 58c76fdab962 +# exit status 0 +# +# On #902's PG18 leg a suite reported FATAL "the binary under test was not built +# from this source" against a tree that was correct, and the stamp it disagreed +# with equalled a clean local fingerprint of the same head. Whatever moved, the +# WRITE was right and one READ was not. +# +# The requirement is not that the computation cannot fail. It is that a failure +# is reported as one: EMPTY, which pgc_freshness_verdict already turns into +# `unknown` and the controller already prints as "freshness UNVERIFIED" and +# deliberately does not fail. A false UNVERIFIED costs a line of output. A false +# FATAL costs a red matrix and teaches people to re-run past the check. + +_fp="$(mktemp -d "${TMPDIR:-/tmp}/pgc-fpfail.XXXXXX")" +mkdir -p "$_fp/tree/src" "$_fp/bin" +printf 'int a;\n' > "$_fp/tree/src/a.c" +printf 'int b;\n' > "$_fp/tree/src/b.c" +printf 'int c;\n' > "$_fp/tree/src/c.c" +printf 'all:\n\ttrue\n' > "$_fp/tree/Makefile" +printf 'x\n' > "$_fp/tree/pgcolumnar.control" + +# A stub that behaves exactly like md5sum except on its Nth invocation, where it +# fails the way a fork failure or an OOM kill does: no output, non-zero status. +cat > "$_fp/bin/md5sum" <<'STUB' +#!/bin/bash +_n=$(( $(cat "$PGC_FP_COUNT" 2>/dev/null || echo 0) + 1 )) +echo "$_n" > "$PGC_FP_COUNT" +[ "$_n" = "${PGC_FP_FAIL_ON:-0}" ] && exit 1 +exec /usr/bin/md5sum "$@" +STUB +chmod +x "$_fp/bin/md5sum" + +_fp_base="$(pgc_source_fingerprint "$_fp/tree")" +check "premise: the tree fingerprints to something on an unstubbed run" \ + "$([ -n "$_fp_base" ] && echo yes || echo empty)" "yes" + +# Premise for the stub itself: with no failure configured it must agree with the +# real thing, or the arms below would be measuring the stub rather than the fix. +PGC_FP_COUNT="$_fp/count"; export PGC_FP_COUNT +PGC_FP_FAIL_ON=0; export PGC_FP_FAIL_ON +echo 0 > "$PGC_FP_COUNT" +check "premise: the stub agrees with the real md5sum when nothing fails" \ + "$(PATH="$_fp/bin:$PATH" pgc_source_fingerprint "$_fp/tree")" "$_fp_base" + +# THE ARM. One failed digest, and the answer must be EMPTY rather than a hash. +for _fp_n in 2 3; do + PGC_FP_FAIL_ON="$_fp_n"; export PGC_FP_FAIL_ON + echo 0 > "$PGC_FP_COUNT" + _fp_got="$(PATH="$_fp/bin:$PATH" pgc_source_fingerprint "$_fp/tree")" + check "a failed digest on file $_fp_n yields no fingerprint, not a wrong one" \ + "$([ -z "$_fp_got" ] && echo empty || echo "$_fp_got")" "empty" +done + +# And the verdict that follows from it, which is the property that matters: the +# controller must say UNVERIFIED, never `stale`. `stale` is the FATAL. +PGC_FP_FAIL_ON=2; export PGC_FP_FAIL_ON +echo 0 > "$PGC_FP_COUNT" +check "so the verdict is unknown -- UNVERIFIED -- and never stale" \ + "$(pgc_freshness_verdict "$_fp_base" \ + "$(PATH="$_fp/bin:$PATH" pgc_source_fingerprint "$_fp/tree")")" "unknown" + +unset PGC_FP_FAIL_ON PGC_FP_COUNT + +# --------------------------------------------------------------------------- +# ONE TREE HASHES ONE WAY, HOWEVER THE PATH TO IT IS SPELLED. +# +# @OffgridwithJD's finding, reproduced and widened here. `${f#"$dir"/}` strips a +# prefix that must match CHARACTER FOR CHARACTER, so `$dir` with a trailing +# slash, or reached through a symlink, puts the FULL ABSOLUTE PATH into the +# digest instead of the tree-relative one: +# +# plain 92410d0598d6 +# trailing slash bf101efc7c10 differs +# dot segment /./ 774152fff929 differs +# via symlink 3f3c0e36905a differs +# dot-dot /src/.. 92410d0598d6 +# relative . 92410d0598d6 +# +# The `/./` case is the one to keep: it is what a `$(dirname X)/./` composition +# produces and it reads as harmless. Two spellings of one tree must not be able +# to disagree, because the writer and the reader reach the tree by different +# routes and a disagreement there is a FATAL about nothing. + +_sp="$(mktemp -d "${TMPDIR:-/tmp}/pgc-spell.XXXXXX")" +mkdir -p "$_sp/tree/src" "$_sp/tree/objstore" +printf 'int a;\n' > "$_sp/tree/src/a.c" +printf 'int b;\n' > "$_sp/tree/objstore/b.c" +printf 'all:\n\ttrue\n' > "$_sp/tree/objstore/Makefile" +printf 'all:\n\ttrue\n' > "$_sp/tree/Makefile" +printf 'x\n' > "$_sp/tree/pgcolumnar.control" +ln -s "$_sp/tree" "$_sp/link" + +_sp_plain="$(pgc_source_fingerprint "$_sp/tree")" +check "premise: the spelling fixture fingerprints at all" \ + "$([ -n "$_sp_plain" ] && echo yes || echo empty)" "yes" + +check "a trailing slash hashes the same tree the same way" \ + "$(pgc_source_fingerprint "$_sp/tree/")" "$_sp_plain" +check "a /./ segment hashes the same tree the same way" \ + "$(pgc_source_fingerprint "$_sp/tree/./")" "$_sp_plain" +check "a /src/.. segment hashes the same tree the same way" \ + "$(pgc_source_fingerprint "$_sp/tree/src/..")" "$_sp_plain" +check "a symlink to the tree hashes it the same way" \ + "$(pgc_source_fingerprint "$_sp/link")" "$_sp_plain" +check "a relative path hashes the same tree the same way" \ + "$(cd "$_sp/tree" && pgc_source_fingerprint .)" "$_sp_plain" + +unset _fp _fp_base _fp_got _fp_n _sp _sp_plain + +# --------------------------------------------------------------------------- +# THE FIX MUST NOT RE-BASELINE EVERY STAMP ALREADY ON DISK. +# +# Detecting a failed digest means capturing the per-file lines into a variable to +# inspect them, and `$(...)` STRIPS THE TRAILING NEWLINE that the old code's +# straight pipe into md5sum included. The same unchanged tree then hashes +# differently before and after the change, every stamp on disk reads `stale`, and +# a fix for false FATALs becomes a false FATAL for everyone holding a built +# worktree (@OffgridwithJD, caught before it shipped). The matrix cannot catch +# this: it copies a fresh tree and re-stamps every run, so it lands on developers +# and on nobody's CI. +# +# This arm is a COMPATIBILITY assertion, not a tidiness one. It recomputes the +# tree the way the previous implementation did and requires the same answer. + +_bc="$(mktemp -d "${TMPDIR:-/tmp}/pgc-compat.XXXXXX")" +mkdir -p "$_bc/src" "$_bc/objstore" +printf 'int a;\n' > "$_bc/src/a.c" +printf 'int b;\n' > "$_bc/src/b.c" +printf 'void h(void);\n' > "$_bc/src/h.h" +printf 'int m;\n' > "$_bc/objstore/m.c" +printf 'all:\n\ttrue\n' > "$_bc/objstore/Makefile" +printf 'all:\n\ttrue\n' > "$_bc/Makefile" +printf 'x\n' > "$_bc/pgcolumnar.control" +printf 'SELECT 1;\n' > "$_bc/pgcolumnar--9.9.sql" + +# The PREVIOUS implementation, transcribed: the same input set and the same +# per-file `path digest` lines, piped straight into md5sum as it was. +_pgc_fp_previous() { + local dir="${1:-.}" d + { + while IFS= read -r d; do + [ -n "$d" ] || continue + find "$d" -maxdepth 1 -type f \( -name '*.c' -o -name '*.h' \ + -o -name 'Makefile' \) -print0 2>/dev/null + done < <(pgc_source_build_dirs "$dir") + find "$dir" -maxdepth 1 -type f \( -name 'Makefile' -o -name '*.control' \ + -o -name '*.sql' \) -print0 2>/dev/null + } | sort -z | while IFS= read -r -d '' _f; do + printf '%s %s\n' "${_f#"$dir"/}" \ + "$(md5sum < "$_f" 2>/dev/null | cut -d' ' -f1)" + done | md5sum | cut -c1-12 +} + +check "the fixed fingerprint equals what the previous implementation produced" \ + "$(pgc_source_fingerprint "$_bc")" "$(_pgc_fp_previous "$_bc")" + +# THE CONTROL WITHOUT WHICH THE SPELLING ARMS ARE VACUOUS. "Every spelling +# agrees" is satisfied perfectly by a fingerprint that ignores its input, so the +# set needs one arm proving the hash still MOVES on a real change. +_bc_before="$(pgc_source_fingerprint "$_bc")" +printf 'int a = 2;\n' > "$_bc/src/a.c" +check "control: a real content change still moves the fingerprint" \ + "$([ "$(pgc_source_fingerprint "$_bc")" != "$_bc_before" ] && echo moved || echo SAME)" \ + "moved" +printf 'int a;\n' > "$_bc/src/a.c" +check "control: and restoring the content restores the fingerprint" \ + "$(pgc_source_fingerprint "$_bc")" "$_bc_before" + +# A tree with nothing hashable cannot be verified, so it reports no fingerprint +# rather than the hash of an empty stream -- which is a stable, comparable value +# and would have made two empty trees "match". +_bc_empty="$(mktemp -d "${TMPDIR:-/tmp}/pgc-empty.XXXXXX")" +check "a tree with no hashable file yields no fingerprint" \ + "$([ -z "$(pgc_source_fingerprint "$_bc_empty")" ] && echo empty || echo hashed)" "empty" + +unset _bc _bc_before _bc_empty From d9547faafbfe6fa75ad3de7f606558487a45bdb7 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 9 Sep 2026 15:38:58 -0600 Subject: [PATCH 2/5] docs: the empty-manifest arm has an observation behind it, not a model The arm shipped as "a legitimate empty tree". @OffgridwithJD then observed the whole manifest coming back empty under process pressure, on a read-only bind mount where content was excluded by construction rather than by argument, and the deviant value was md5 of the empty string. Reproduced here as an A/B with the real md5sum and no stub, 20 samples per cell: at `ulimit -u 40` the old function returns a confident answer about nothing in 11 runs of 20, and this branch in none of 40. The guard was already in the change. What was missing was the reason, and a reader deciding whether the arm earns its place needs the observation rather than my hypothesis. Also names what is NOT guarded -- a truncated manifest, which would give a plausible wrong hash that neither guard can see -- because the boundary of this change is "the three observed variants are closed" and not "the function is now infallible". Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK --- test/pytest/TESTS.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 1cd12e5b..a5d52859 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -475,6 +475,33 @@ fingerprint that ignores its input. hash of an empty stream is a stable, comparable value, so two trees with no source would have *matched*. +**That last arm is the only one in this set with a real observation behind it +rather than a model, and it was not the case it was written for.** It shipped as +"a legitimate empty tree". @OffgridwithJD then observed the WHOLE manifest coming +back empty under process pressure, on a read-only bind mount where content was +excluded by construction: two distinct fingerprints over a tree incapable of +changing, and the deviant value was `d41d8cd98f00`, which is md5 of the empty +string — not a corrupted manifest but *no* manifest, hashed confidently. Measured +here as an A/B with the real `md5sum` and no stub, 20 samples per cell: + + true fingerprint = eebe35d6eaed ; md5("") = d41d8cd98f00 + + OLD ulimit -u 45 correct=19 md5("")=1 refused=0 other=0 | sum=20 of 20 + OLD ulimit -u 40 correct=8 md5("")=11 refused=0 other=1 | sum=20 of 20 + NEW ulimit -u 45 correct=20 md5("")=0 refused=0 other=0 | sum=20 of 20 + NEW ulimit -u 40 correct=20 md5("")=0 refused=0 other=0 | sum=20 of 20 + +At `ulimit -u 40` the old function returns a confident answer about nothing in 11 +runs of 20. The guard covers it structurally rather than statistically: a +non-empty `out` has at least one line, so `md5("")` is not a reachable return +value. + +**What is still not guarded**, named here rather than left for someone to find: a +TRUNCATED manifest — `find` returning fewer files rather than none — would produce +a plausible wrong hash that neither the per-file sentinel nor the empty-manifest +guard can see. It has not been observed. The boundary of this change is "the three +observed variants are closed", not "the function is now infallible". + ## 6. test_docs_cover_the_corpus.py: this document, checked The file you are reading is checked mechanically, because it went stale inside a From a94b6d52ad96655136ac60d1103985009a30edd7 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 9 Sep 2026 15:51:34 -0600 Subject: [PATCH 3/5] test: when it refuses, it must say what it hashed Two CI failures reported the same pair of hashes and nothing else: #902 PG18 iceberg_rest source now a735c673b129, binary built from 6d122a7158d5 #909 PG17 iceberg_rest_server source now a735c673b129, binary built from 6d122a7158d5 Identical, across two branches, two majors and two build directories, with the fingerprint fix present in the second. **That made the second occurrence another sample rather than an answer**, and it is why this commit exists: twelve hex characters cannot name a file. ## What the repeated value rules out * NOT a transient digest failure. That gives a DIFFERENT wrong hash every time -- the stub in this suite gives 22897add806e, 58c76fdab962, 1d7c00654866. * NOT the path-spelling defect. PG17 and PG18 build in different directories, so a path-derived hash would differ between them. It did not. * NOT anything this branch already fixes: #909's failing head carried the whole fix, verified at 2c19b4e rather than assumed. So `a735c673b129` is a deterministic, CONTENT-derived state that CI reaches and neither agent has reproduced locally. ## The shape it most likely is, and the arm aimed at it An ADDED file is the only class that explains ONE deviant value from TWO different build directories, because the manifest carries the path RELATIVE to the tree: the same file appearing under `matrix-17` and `matrix-18` contributes the same line and therefore the same hash. @OffgridwithJD found the gap in my own ruled-out list -- I had tested three specific additions, and an addition's contribution depends on its CONTENT, so three samples rule out nothing. `test_an_added_file_is_named_rather_than_merely_changing_the_hash` requires the diff of two manifests to NAME the file rather than report that something changed. ## The change `pgc_source_manifest` is now a function and `pgc_source_fingerprint` is defined as its hash, so the two cannot drift apart -- one arm asserts exactly that. The FATAL path prints the manifest through `pgc_freshness_report`. The report is a FUNCTION rather than three lines inline so that an arm can drive it. The alternative was asserting that the source calls it, which is the shape this suite refuses everywhere else, and a dump nobody can run is a dump nobody knows is empty. Its empty case says `(empty -- nothing under DIR)` rather than printing nothing, because a silent empty dump reads as "the manifest was fine", which is the failure the report exists to end. ## Arms, written first and proved able to fail RED before the function existed: 5 failures, all of them the new arms. Then each piece reverted separately with the mutation asserted applied: revert arms red the report prints nothing 4 the manifest emits absolute paths 5 (incl. the compatibility arm) The absolute-path revert reddening the compatibility arm is the useful part: it says the manifest's relative paths are the same property the previous implementation had, not a new convention introduced here. harness_selftest.sh 392 passed + 0 failed + 0 unrunnable PASSED pytest corpus 89 passed, --pgc-expect-tests 89 docs_style.sh 9 checks PASSED TESTS.md's totals counted from the corpus with the gate's own function: 89 in 6, harness 74, product 15. The gate caught three test names I had not documented and one wrong harness count before this landed, which is the gate working. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK --- test/lib.sh | 49 +++++++++-- test/pytest/TESTS.md | 34 +++++++- test/pytest/test_build_refusal.py | 79 +++++++++++++++++ .../340-the-binary-must-be-built-from.sh | 84 +++++++++++++++++++ 4 files changed, 239 insertions(+), 7 deletions(-) diff --git a/test/lib.sh b/test/lib.sh index fb63e313..8b3b8349 100755 --- a/test/lib.sh +++ b/test/lib.sh @@ -294,6 +294,13 @@ pgc_setup() { echo "FATAL: the binary under test was not built from this source" >&2 echo " source now $_pgc_fresh_current, binary built from $_pgc_fresh_recorded" >&2 echo " (refusing to report checks about code that is not installed)" >&2 + # WHAT IT HASHED, not just what it hashed TO. Two CI failures reported + # this pair of hashes and nothing else, identically, across two + # branches and two majors -- which made the second occurrence another + # sample rather than an answer. Twelve characters cannot name a file; + # the manifest can, and a diff against the next occurrence's manifest + # says whether something appeared, vanished or changed. + pgc_freshness_report "$PGC_SRCDIR" >&2 exit 1 ;; unknown) @@ -648,9 +655,32 @@ pgc_source_build_dirs() { # pgc_source_build_dirs DIR -> dirs # A value no md5sum digest can be, so it cannot be confused with one. _pgc_fp_failed='PGC_FINGERPRINT_DIGEST_FAILED' -pgc_source_fingerprint() { # pgc_source_fingerprint DIR -> hash, or EMPTY if it could not be computed +# What the fingerprint was taken over, for the FATAL path. A FUNCTION rather than +# three lines inline, so an arm can drive it: a dump nobody can run is a dump +# nobody knows is empty. Two CI failures printed only the pair of hashes, and the +# second occurrence was therefore another sample rather than an answer. +pgc_freshness_report() { # pgc_freshness_report DIR -> the manifest, annotated + local dir="${1:-.}" n + n="$(pgc_source_manifest "$dir" | wc -l)" + echo " the manifest this fingerprint was taken over:" + if [ "$n" -eq 0 ]; then + echo " | (empty -- nothing under $dir matched, which is itself the finding)" + else + pgc_source_manifest "$dir" | sed 's/^/ | /' + fi + echo " ($n files, under $dir)" +} + +pgc_source_manifest() { # pgc_source_manifest DIR -> "relpath digest" per file, or EMPTY local dir="${1:-.}" - local d out + local d + # THE MANIFEST IS THE THING; THE FINGERPRINT IS ITS HASH. + # + # Twelve hex characters cannot say which file moved. Two CI failures reported + # `source now a735c673b129, binary built from 6d122a7158d5` and nothing else, + # identically, across two branches and two majors -- so a bare hash gave us a + # second sample of a mystery rather than an answer. The controller's FATAL + # path prints this, and a diff of two manifests names the file. # ONE TREE HASHES ONE WAY, HOWEVER THE PATH IS SPELLED. `${f#"$dir"/}` below # strips a prefix that has to match character for character, so `$dir` with a # trailing slash, or with a `/./` segment, or reached through a symlink, put @@ -667,7 +697,7 @@ pgc_source_fingerprint() { # pgc_source_fingerprint DIR -> hash, or EMPTY if it # no special case: nothing is found under it, `out` is empty, and the guard # below returns no fingerprint. dir="$(pgc_norm_path "$dir")" - out="$({ + { while IFS= read -r d; do [ -n "$d" ] || continue find "$d" -maxdepth 1 -type f \( -name '*.c' -o -name '*.h' \ @@ -714,7 +744,12 @@ pgc_source_fingerprint() { # pgc_source_fingerprint DIR -> hash, or EMPTY if it _pgc_fp_h="$(md5sum < "$_pgc_fp_f" 2>/dev/null | cut -d' ' -f1)" [ -n "$_pgc_fp_h" ] || { printf '%s\n' "$_pgc_fp_failed"; break; } printf '%s %s\n' "${_pgc_fp_f#"$dir"/}" "$_pgc_fp_h" - done)" + done +} + +pgc_source_fingerprint() { # pgc_source_fingerprint DIR -> hash, or EMPTY if it could not be computed + local out + out="$(pgc_source_manifest "${1:-.}")" # Empty rather than a hash, which pgc_freshness_verdict turns into `unknown` # and the controller prints as "freshness UNVERIFIED" -- already designed, and # already deliberately not a failure. The asymmetry is the whole argument: a @@ -723,8 +758,10 @@ pgc_source_fingerprint() { # pgc_source_fingerprint DIR -> hash, or EMPTY if it # controller exists to prevent. case "$out" in *"$_pgc_fp_failed"*) printf ''; return 0 ;; esac # A tree with no hashable file cannot be verified, so it gets no fingerprint - # either. This also keeps the line below off an empty `out`, whose trailing - # newline would otherwise be the ONLY input to md5sum. + # either -- and md5 of an empty stream is a stable, comparable value that + # would have made two empty trees "match". Observed rather than hypothetical: + # under process pressure the whole manifest came back empty and the old code + # returned md5("") = d41d8cd98f00 in 11 runs of 20 (@OffgridwithJD). [ -n "$out" ] || { printf ''; return 0; } # THE TRAILING NEWLINE IS LOAD-BEARING. The old code piped the loop straight # into md5sum, so the stream md5sum saw ended with one; `$(...)` strips it. diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index a5d52859..18f81005 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -4,7 +4,7 @@ Reference for anyone reading, running, or adding to `test/pytest/`. The design a the decisions behind the harness are in `design/ISSUE_432_PYTEST_HARNESS.md`. This file covers the tests themselves. -**84 tests in 6 files.** Sixty-nine of them test the harness rather than the +**89 tests in 6 files.** Seventy-four of them test the harness rather than the product, and they come first, because a harness that can report a false green makes every other result in this directory worthless. @@ -496,6 +496,38 @@ runs of 20. The guard covers it structurally rather than statistically: a non-empty `out` has at least one line, so `md5("")` is not a reachable return value. +### When it refuses, it says what it hashed + +Six more tests, added after two CI failures reported *the same pair of hashes and +nothing else* — `source now a735c673b129, binary built from 6d122a7158d5`, +identically, across two branches, two majors and two build directories, with the +fingerprint fix present in one of them. **A bare hash made the second occurrence +another sample rather than an answer.** + +So the manifest is a function in its own right, `pgc_source_fingerprint` is +defined as its hash — the two cannot drift apart, and one test asserts exactly +that — and the FATAL path prints it through `pgc_freshness_report`. + +`test_an_added_file_is_named_rather_than_merely_changing_the_hash` is the arm +aimed at the open question. An addition is the only class that explains one +deviant value from two different build directories, because the manifest carries +the path RELATIVE to the tree: the same file appearing under `matrix-17` and +`matrix-18` contributes the same line and therefore the same hash. The test +requires the diff to name the file rather than report that something changed. + +`test_the_manifest_names_what_the_fingerprint_hashed` pins the shape of each line +— a tree-relative path and a 32-character digest, never an absolute path, because +an absolute path in the digest is the spelling defect returning by another route. +`test_the_fingerprint_is_the_hash_of_the_manifest` is the arm that keeps the two +from drifting, and `test_an_empty_manifest_is_reported_as_empty_not_as_silence` +covers the case the report exists for. + +`test_the_fatal_report_can_be_run_rather_than_grepped_for` exists because the +alternative was asserting that the source calls the function, which is the shape +this suite refuses everywhere else. The report is a function so an arm can drive +it, and the empty case says `(empty -- nothing under ...)` rather than printing +nothing, because a silent empty dump reads as *the manifest was fine*. + **What is still not guarded**, named here rather than left for someone to find: a TRUNCATED manifest — `find` returning fewer files rather than none — would produce a plausible wrong hash that neither the per-file sentinel nor the empty-manifest diff --git a/test/pytest/test_build_refusal.py b/test/pytest/test_build_refusal.py index b1ed5d18..16264337 100644 --- a/test/pytest/test_build_refusal.py +++ b/test/pytest/test_build_refusal.py @@ -22,6 +22,7 @@ import os import pathlib +import re import subprocess import pytest @@ -623,3 +624,81 @@ def test_a_tree_with_nothing_hashable_reports_no_fingerprint(tmp_path, expect): empty.mkdir() got, _ = _sh_fp(f'pgc_source_fingerprint "{empty}"') expect.text(got or "empty", "empty", "an unhashable tree yields no fingerprint") + + +def _mf_tree(tmp_path, name): + t = tmp_path / name + (t / "src").mkdir(parents=True, exist_ok=True) + (t / "objstore").mkdir(parents=True, exist_ok=True) + (t / "src" / "a.c").write_text("int a;\n") + (t / "src" / "h.h").write_text("void h(void);\n") + (t / "objstore" / "m.c").write_text("int m;\n") + (t / "objstore" / "Makefile").write_text("all:\n\ttrue\n") + (t / "Makefile").write_text("all:\n\ttrue\n") + (t / "pgcolumnar.control").write_text("x\n") + return t + + +def test_the_manifest_names_what_the_fingerprint_hashed(tmp_path, expect): + """Twelve hex characters cannot say which file moved. + + Two CI failures reported `source now a735c673b129, binary built from + 6d122a7158d5` and nothing else -- identically, across two branches, two + majors and two build directories, with the fingerprint fix present in one of + them. A bare hash made the second occurrence another sample rather than an + answer. + """ + t = _mf_tree(tmp_path, "mf") + out, _ = _sh_fp(f'pgc_source_manifest "{t}"') + lines = [l for l in out.splitlines() if l.strip()] + expect.num(len(lines), 6, "the manifest names every file the fingerprint hashes") + shaped = [l for l in lines if re.fullmatch(r"[A-Za-z0-9_./-]+ [0-9a-f]{32}", l)] + expect.num(len(shaped), 6, "each line is a tree-relative path and a digest") + expect.num(len([l for l in lines if l.startswith("/")]), 0, + "the manifest is tree-relative, never absolute") + + +def test_the_fingerprint_is_the_hash_of_the_manifest(tmp_path, expect): + """One is defined as the other, so the two cannot drift apart.""" + t = _mf_tree(tmp_path, "mh") + fp, _ = _sh_fp(f'pgc_source_fingerprint "{t}"') + via, _ = _sh_fp(f'pgc_source_manifest "{t}" | md5sum | cut -c1-12') + expect.text(fp, via, "the fingerprint is the hash of the manifest") + + +def test_an_added_file_is_named_rather_than_merely_changing_the_hash(tmp_path, expect): + """The class the two CI failures fall into, and the one a hash cannot report. + + An addition is the only class that explains one deviant value from two + different build directories: the manifest carries the path RELATIVE to the + tree, so the same file added under matrix-17 and matrix-18 contributes the + same line. + """ + t = _mf_tree(tmp_path, "add") + before, _ = _sh_fp(f'pgc_source_manifest "{t}"') + (t / "src" / "zz_appeared.c").write_text("int zz;\n") + after, _ = _sh_fp(f'pgc_source_manifest "{t}"') + gained = set(after.splitlines()) - set(before.splitlines()) + expect.num(len(gained), 1, "exactly one manifest line appears") + expect.text(sorted(gained)[0].split()[0], "src/zz_appeared.c", + "and it names the file that appeared") + + +def test_the_fatal_report_can_be_run_rather_than_grepped_for(tmp_path, expect): + """A dump nobody can run is a dump nobody knows is empty.""" + t = _mf_tree(tmp_path, "rep") + out, _ = _sh_fp(f'pgc_freshness_report "{t}"') + expect.num(len([l for l in out.splitlines() if l.strip().startswith("| src/a.c ")]), 1, + "the report names each hashed file") + expect.num(len([l for l in out.splitlines() if "(6 files, under" in l]), 1, + "the report states how many files it hashed") + + +def test_an_empty_manifest_is_reported_as_empty_not_as_silence(tmp_path, expect): + """A silent empty dump reads as "the manifest was fine", which is the failure + this report exists to end.""" + hollow = tmp_path / "hollow_report" + hollow.mkdir() + out, _ = _sh_fp(f'pgc_freshness_report "{hollow}"') + expect.num(len([l for l in out.splitlines() if "empty -- nothing under" in l]), 1, + "an empty manifest says so") diff --git a/test/selftest/340-the-binary-must-be-built-from.sh b/test/selftest/340-the-binary-must-be-built-from.sh index 059ed472..f51dc32a 100644 --- a/test/selftest/340-the-binary-must-be-built-from.sh +++ b/test/selftest/340-the-binary-must-be-built-from.sh @@ -625,3 +625,87 @@ check "a tree with no hashable file yields no fingerprint" \ "$([ -z "$(pgc_source_fingerprint "$_bc_empty")" ] && echo empty || echo hashed)" "empty" unset _bc _bc_before _bc_empty + +# --------------------------------------------------------------------------- +# WHEN IT REFUSES, IT MUST SAY WHAT IT HASHED. +# +# Two CI failures reported the same pair of hashes and nothing else: +# +# source now a735c673b129, binary built from 6d122a7158d5 +# +# Identical across #902/PG18/iceberg_rest and #909/PG17/iceberg_rest_server -- +# two branches, two majors, two build directories, and one of the two runs +# carried the fingerprint fix. That rules out a transient digest failure (which +# gives a different wrong hash every time) and a path-dependent one (two build +# directories would give two values). It is a deterministic, content-derived +# state, and neither agent has reproduced it locally. +# +# Twelve characters cannot say which file moved. A manifest can, and the +# difference between "the hash changed" and "objstore/x.c appeared" is the +# difference between another sample and an answer. So the manifest is a function +# in its own right, the fingerprint is its hash, and the FATAL path prints it. + +_mf="$(mktemp -d "${TMPDIR:-/tmp}/pgc-manifest.XXXXXX")" +mkdir -p "$_mf/src" "$_mf/objstore" +printf 'int a;\n' > "$_mf/src/a.c" +printf 'void h(void);\n'> "$_mf/src/h.h" +printf 'int m;\n' > "$_mf/objstore/m.c" +printf 'all:\n\ttrue\n' > "$_mf/objstore/Makefile" +printf 'all:\n\ttrue\n' > "$_mf/Makefile" +printf 'x\n' > "$_mf/pgcolumnar.control" + +_mf_lines="$(pgc_source_manifest "$_mf" | wc -l)" +check "the manifest names every file the fingerprint hashes" "$_mf_lines" "6" + +check "each manifest line is a tree-relative path and a digest" \ + "$(pgc_source_manifest "$_mf" | grep -cE '^[A-Za-z0-9_./-]+ [0-9a-f]{32}$')" "6" + +check "the manifest is tree-relative, never absolute" \ + "$(pgc_source_manifest "$_mf" | grep -c '^/')" "0" + +# The fingerprint IS the manifest's hash, so the two cannot drift apart. +check "the fingerprint is the hash of the manifest" \ + "$(pgc_source_fingerprint "$_mf")" \ + "$(pgc_source_manifest "$_mf" | md5sum | cut -c1-12)" + +# THE ARM THAT MATTERS: an ADDED file is named, not merely counted. This is the +# class the two CI failures fall into and the one a bare hash cannot report. +printf 'int zz;\n' > "$_mf/src/zz_appeared.c" +check "an added file appears in the manifest by name" \ + "$(pgc_source_manifest "$_mf" | grep -c '^src/zz_appeared.c ')" "1" +check "and comparing two manifests names it rather than saying 'changed'" \ + "$(diff <(pgc_source_manifest "$_mf" | grep -v zz_appeared) \ + <(pgc_source_manifest "$_mf") | grep -oE 'src/zz_appeared\.c')" \ + "src/zz_appeared.c" +rm -f "$_mf/src/zz_appeared.c" + +# A manifest for a tree that cannot be hashed is empty, matching the fingerprint's +# own refusal rather than inventing a second convention. +_mf_hollow="$(mktemp -d "${TMPDIR:-/tmp}/pgc-hollow.XXXXXX")" +check "an unhashable tree has an empty manifest" \ + "$(pgc_source_manifest "$_mf_hollow" | wc -l)" "0" + +unset _mf _mf_lines _mf_hollow + +# The FATAL path's dump, driven rather than grepped for. A report nobody can run +# is a report nobody knows is empty, and "the source says it calls it" is the kind +# of claim this suite exists to refuse. +_fr="$(mktemp -d "${TMPDIR:-/tmp}/pgc-report.XXXXXX")" +mkdir -p "$_fr/src" +printf 'int a;\n' > "$_fr/src/a.c" +printf 'all:\n\ttrue\n' > "$_fr/Makefile" +printf 'x\n' > "$_fr/pgcolumnar.control" + +check "the report names each hashed file" \ + "$(pgc_freshness_report "$_fr" | grep -c '^ | src/a.c ')" "1" +check "the report states how many files it hashed" \ + "$(pgc_freshness_report "$_fr" | grep -cE '^ \(3 files, under ')" "1" +check "an added file shows up in the report" \ + "$(printf 'int z;\n' > "$_fr/src/z.c"; pgc_freshness_report "$_fr" | grep -c '^ | src/z.c ')" "1" +rm -f "$_fr/src/z.c" +# The empty case says so rather than printing nothing, because a silent empty +# dump reads as "the manifest was fine" -- the failure this report exists to end. +_fr_hollow="$(mktemp -d "${TMPDIR:-/tmp}/pgc-rhollow.XXXXXX")" +check "an empty manifest is reported as empty, not as silence" \ + "$(pgc_freshness_report "$_fr_hollow" | grep -c 'empty -- nothing under')" "1" +unset _fr _fr_hollow From b8da10aa79b0f0725dc9e060a8232e733fd5dee8 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 9 Sep 2026 16:15:36 -0600 Subject: [PATCH 4/5] test: the empty-result arms had no premise, so they passed over a broken harness @OffgridwithJD found that `expect.refusal` matched its pattern against pytest's printed SOURCE rather than the raised message, so 13 merged arms asserted nothing (#905, b327a0a2). Neither of my branches uses `expect.refusal` -- checked rather than assumed, `git show : | grep -c expect.refusal` is 0 on both -- but the CLASS is what matters, and auditing my own arms against it found one of mine. ## The shape: a failure that produces exactly the value the test expects `test_a_tree_with_nothing_hashable_reports_no_fingerprint` asserts an EMPTY result. Empty is also what a harness that cannot run at all produces. Driving the helper against a `lib.sh` that does not exist: stdout when the whole harness is broken: '' the arm asserts: 'empty' == "empty" -> True **Green over a completely broken tree.** The `.sh` half had the same hole, for the same reason. Both now take a premise first: the SAME function, over a real tree, must return a fingerprint. Proved to catch it -- with `pgc_source_fingerprint` neutered to `return 0`, the arm now FAILS where it previously passed: FAILED test_a_tree_with_nothing_hashable_reports_no_fingerprint 9 failed, 24 passed harness_selftest.sh 393 passed + 0 failed + 0 unrunnable PASSED pytest corpus 89 passed TESTS.md's totals are unchanged: the pytest side gained an expectation, not a test function, and the corpus is still 89 in 6. **This is the second arm of mine this session that could not fail**, after the "premise" in my #904 probe suite that compared an expression to itself. Both were found by auditing after someone else found the same shape in their own work, which is the argument for two agents better than any of the review rules. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK --- test/pytest/test_build_refusal.py | 17 ++++++++++++++++- .../340-the-binary-must-be-built-from.sh | 7 +++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/test/pytest/test_build_refusal.py b/test/pytest/test_build_refusal.py index 16264337..8fb6f90f 100644 --- a/test/pytest/test_build_refusal.py +++ b/test/pytest/test_build_refusal.py @@ -619,7 +619,22 @@ def test_the_fingerprint_still_moves_on_a_real_change(tmp_path, expect): def test_a_tree_with_nothing_hashable_reports_no_fingerprint(tmp_path, expect): """The hash of an empty stream is a stable, comparable value: two empty - trees would have "matched".""" + trees would have "matched". + + THE PREMISE IS LOAD-BEARING AND WAS MISSING. This arm asserts an EMPTY + result, and an empty result is also what a harness that cannot run at all + produces: point the helper at a `lib.sh` that does not exist and stdout is + `''`, so `got or "empty"` is `"empty"` and the arm passes green over a + completely broken tree. That is the same shape as the `expect.refusal` + defect @OffgridwithJD found on main -- a failure that produces exactly the + value the test expects. The populated-tree premise below fails first when + the harness is broken, which is what makes the empty assertion mean + anything. + """ + live = _fp_tree(tmp_path, "hollow_premise") + base, _ = _sh_fp(f'pgc_source_fingerprint "{live}"') + expect.at_least(len(base), 12, + "premise: the same helper returns a fingerprint for a real tree") empty = tmp_path / "hollow" empty.mkdir() got, _ = _sh_fp(f'pgc_source_fingerprint "{empty}"') diff --git a/test/selftest/340-the-binary-must-be-built-from.sh b/test/selftest/340-the-binary-must-be-built-from.sh index f51dc32a..f58f952e 100644 --- a/test/selftest/340-the-binary-must-be-built-from.sh +++ b/test/selftest/340-the-binary-must-be-built-from.sh @@ -620,7 +620,14 @@ check "control: and restoring the content restores the fingerprint" \ # A tree with nothing hashable cannot be verified, so it reports no fingerprint # rather than the hash of an empty stream -- which is a stable, comparable value # and would have made two empty trees "match". +# THE PREMISE IS LOAD-BEARING. This arm asserts an EMPTY result, and empty is +# also what a harness that cannot run produces -- so without a premise that the +# SAME function returns something for a real tree, it passes green over a broken +# one. Same shape as the expect.refusal defect @OffgridwithJD found on main: a +# failure that produces exactly the value the test expects. _bc_empty="$(mktemp -d "${TMPDIR:-/tmp}/pgc-empty.XXXXXX")" +check "premise: the same function returns a fingerprint for a real tree" \ + "$([ -n "$(pgc_source_fingerprint "$_bc")" ] && echo yes || echo empty)" "yes" check "a tree with no hashable file yields no fingerprint" \ "$([ -z "$(pgc_source_fingerprint "$_bc_empty")" ] && echo empty || echo hashed)" "empty" From bf4f5091231059e68cd60f64e6ae2380daca98f4 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 9 Sep 2026 16:40:58 -0600 Subject: [PATCH 5/5] test: the selftest wrote into the tree the other suites were reading `test/selftest/340` wrote `objstore/.pgc_fingerprint_probe.c` into `$PGC_SRCDIR` to prove that a new file under a recursed directory moves the fingerprint. `harness_selftest` runs IN the matrix, so at `PGC_JOBS=4` it created that file in the shared build directory while up to three sibling suites fingerprinted concurrently, and whichever sampled inside the window reported FATAL: the binary under test was not built from this source source now a735c673b129, binary built from 6d122a7158d5 against a tree that was correct. Reproduced exactly: clean tree 6d122a7158d5 with objstore/.pgc_fingerprint_probe.c a735c673b129 <- what CI reported after removal 6d122a7158d5 The path is tree-RELATIVE and the content fixed, so the deviant value was IDENTICAL across majors, build directories and branches. That is what made it look deterministic enough to be a real staleness, and it is what sent @OffgridwithJD and me chasing a transient md5sum failure and then a path-spelling defect. Four pull requests carried the red. **@linuxhikerpm found it by reading the suite** (#910) and correctly declined to fold the fix into an unrelated change. The merge of #903 was mine, so the fix is mine. ## The arm's intent survives It exists because the REAL tree's `objstore/` was not being read, and a hand-built fixture could not have caught that -- so it now probes a HARDLINKED COPY of the real tree rather than a fixture, and a PREMISE requires the copy to discover the same build directories as the real tree. **That premise immediately earned itself.** My first fix used `cp -al SRC DST || cp -a SRC DST`. `/tmp` is a different filesystem from the tree here, so the hardlink copy failed AFTER creating DST, and `cp -a` then copied the tree INSIDE it -- the copy's build dirs came out as `bfix src` instead of `objstore src`. Copying entry by entry fixes it, and skips `.git` for free. ## Two observers, and the first version of the second one was vacuous `.sh`: the concrete probe path must be outside the live tree. Reverted, it reddens `got [INSIDE /root/bfix] want [outside]`. pytest: no part directs a write at the live tree. **A BEFORE/AFTER RUN CANNOT CATCH THIS AND I WROTE ONE FIRST.** Fingerprint the tree, run the suite, fingerprint again -- the probe is created and `rm -f`'d inside the same suite, so the tree is byte-identical when the run ends and the comparison passes. The damage is done to whoever samples DURING the window; an after-the-fact observer is blind to it by construction, and sampling concurrently would only make the arm racy. **The second version was vacuous too.** It looked for the tree root inside a redirection target, and the defect is written in two steps -- `_bd_probe= "$_bd_root/..."` then `> "$_bd_probe"` -- so it PASSED against the reverted defect. It now follows one level of indirection, and reverting names the line: got '340-the-binary-must-be-built-from.sh:184' want 'none' Its limit is stated in the test rather than left to be discovered: it recognises a redirection whose target derives from the tree-root variables the parts use, and a write reaching the tree another way would evade it. It carries three premises -- the direct shape, the indirect shape, and a control that a write into a COPY is NOT flagged -- because a pattern matching nothing would otherwise pass silently. ## Verified harness_selftest.sh 395 passed + 0 failed + 0 unrunnable PASSED pytest corpus 90 passed, --pgc-expect-tests 90 docs_style.sh 9 checks PASSED live tree after a full selftest run: 0 probe files, fingerprint unchanged TESTS.md counted from the corpus with the gate's own function: 90 in 6, harness 75, product 15. Not fixed here and not made worse: `pgc_source_build_dirs` called DIRECTLY with a symlinked path still returns `src` alone and drops `objstore`. Every caller goes through the manifest, which canonicalises first -- measured, real and symlinked both give 6d122a7158d5 on this branch where the symlink gave cbc6688e0ac9 before it -- so it is latent rather than live, and an arm on the raw helper would pin an interface nobody uses that way (@OffgridwithJD, who found it and then walked back the ask). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Unuuvh3fRR67SceiGpfeeK --- test/pytest/TESTS.md | 43 ++++++++++- test/pytest/test_build_refusal.py | 73 +++++++++++++++++++ .../340-the-binary-must-be-built-from.sh | 62 ++++++++++++++-- 3 files changed, 171 insertions(+), 7 deletions(-) diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 18f81005..a3a4f811 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -4,7 +4,7 @@ Reference for anyone reading, running, or adding to `test/pytest/`. The design a the decisions behind the harness are in `design/ISSUE_432_PYTEST_HARNESS.md`. This file covers the tests themselves. -**89 tests in 6 files.** Seventy-four of them test the harness rather than the +**90 tests in 6 files.** Seventy-five of them test the harness rather than the product, and they come first, because a harness that can report a false green makes every other result in this directory worthless. @@ -528,6 +528,47 @@ this suite refuses everywhere else. The report is a function so an arm can drive it, and the empty case says `(empty -- nothing under ...)` rather than printing nothing, because a silent empty dump reads as *the manifest was fine*. +### The suite that wrote into the tree the other suites were reading + +`test_no_selftest_part_writes_into_the_live_source_tree` scans the parts for a +redirection aimed at the live tree. + +`test/selftest/340` used to write `objstore/.pgc_fingerprint_probe.c` into +`$PGC_SRCDIR`, to prove that a new file under a recursed directory moves the +fingerprint. `harness_selftest` runs IN the matrix, so at `PGC_JOBS=4` it created +that file in the shared build directory while sibling suites fingerprinted +concurrently, and whichever sampled inside that window reported `FATAL: the binary +under test was not built from this source` against a tree that was correct. The +path is tree-relative and the content fixed, so the deviant value was *identical* +across majors, build directories and branches — which is what made it look like a +real staleness. It cost four pull requests and two wrong diagnoses before +@linuxhikerpm found it by reading the suite. + +The arm now probes a hardlinked COPY of the tree. The intent survives, because the +defect it was written for was that the *real* tree's `objstore/` was not being +read, and a hand-built fixture could not have caught that — so a premise requires +the copy to discover the same build directories as the real tree. **That premise +immediately earned itself**: the first fix hardlinked across a filesystem +boundary, `cp -al` failed after creating the destination, `cp -a` then copied the +tree *inside* it, and the copy's build directories came out as `bfix src` rather +than `objstore src`. + +**A BEFORE/AFTER RUN CANNOT CATCH THIS, and that is worth recording because it +was my first attempt.** Fingerprint the tree, run the suite, fingerprint again: +the probe was created and `rm -f`'d inside the same suite, so the tree is +byte-identical by the time the run ends and the comparison passes. The damage is +done to whoever samples DURING the window, and an after-the-fact observer is blind +to it by construction. Sampling concurrently instead would make the arm racy — it +would pass whenever the timing missed. So the observable property is the one in +the source: no part directs a write at the live tree. + +Its limit is stated in the test: it recognises a redirection whose target mentions +the tree-root variables the parts actually use, and a write reaching the tree by +another route would evade it. It carries three premises of its own — that the scan +recognises a write at `$PGC_SRCDIR`, that it recognises one through `$_bd_root`, +and that a write into a COPY is *not* flagged — because a pattern that matches +nothing would otherwise pass this arm silently. + **What is still not guarded**, named here rather than left for someone to find: a TRUNCATED manifest — `find` returning fewer files rather than none — would produce a plausible wrong hash that neither the per-file sentinel nor the empty-manifest diff --git a/test/pytest/test_build_refusal.py b/test/pytest/test_build_refusal.py index 8fb6f90f..f0d976b5 100644 --- a/test/pytest/test_build_refusal.py +++ b/test/pytest/test_build_refusal.py @@ -717,3 +717,76 @@ def test_an_empty_manifest_is_reported_as_empty_not_as_silence(tmp_path, expect) out, _ = _sh_fp(f'pgc_freshness_report "{hollow}"') expect.num(len([l for l in out.splitlines() if "empty -- nothing under" in l]), 1, "an empty manifest says so") + + +def test_no_selftest_part_writes_into_the_live_source_tree(expect): + """A suite that runs beside others must not write into the tree they read. + + `test/selftest/340` used to write `objstore/.pgc_fingerprint_probe.c` into + $PGC_SRCDIR to prove that a new file under a recursed directory moves the + fingerprint. `harness_selftest` runs IN the matrix, so at PGC_JOBS=4 it + created that file in the shared build directory while sibling suites + fingerprinted concurrently, and whichever sampled inside the window reported + + FATAL: the binary under test was not built from this source + source now a735c673b129, binary built from 6d122a7158d5 + + against a tree that was correct. Four pull requests and two wrong diagnoses + before @linuxhikerpm found it by reading the suite. + + WHY THIS IS A STATIC SCAN AND NOT A BEFORE/AFTER RUN. I wrote the behavioural + version first: fingerprint the tree, run the suite, fingerprint again. It + CANNOT CATCH THIS DEFECT. The probe was created and `rm -f`'d inside the same + suite, so the tree is byte-identical by the time the run ends and the + comparison passes. The damage is done to whoever samples DURING the window, + and an after-the-fact observer is blind to it by construction. Sampling + concurrently instead would make the arm racy -- it would pass whenever the + timing missed. So the observable property is the one in the source: no part + directs a write at the live tree. + + ITS LIMIT, stated because I have spent today objecting to guards that catch + one spelling: this recognises a redirection whose target mentions the tree + root variables the parts actually use. A write reaching the tree by some other + route -- a `cp` destination, a path assembled elsewhere -- would evade it. The + `.sh` half asserts the concrete probe path is outside the tree and reddens + with `got [INSIDE ...]`; between them they cover this instance and the obvious + generalisations of it, not every conceivable one. + """ + parts = sorted((SRCDIR / "test" / "selftest").glob("*.sh")) + expect.at_least(len(parts), 5, "premise: the selftest parts were found") + + def offenders_in(text): + """Redirections that land in the live tree, following one indirection. + + The real defect is written in two steps -- `_bd_probe="$_bd_root/..."` + and then `printf ... > "$_bd_probe"` -- so a scan that only looks for the + tree root INSIDE a redirection target misses it entirely. That was this + arm's first version, and it passed against the reverted defect. + """ + root = r"(?:PGC_SRCDIR|_bd_root)" + tainted = set(re.findall(r'^\s*([A-Za-z_][A-Za-z0-9_]*)=\"?\$\{?' + root + r'\b', + text, re.M)) + names = "|".join([root] + sorted(re.escape(t) for t in tainted)) + redirect = re.compile(r'>\s*"?\$\{?(?:' + names + r')\b') + found = [] + for n, line in enumerate(text.splitlines(), 1): + if line.lstrip().startswith("#"): + continue + if redirect.search(line): + found.append(n) + return found + + bad = [] + for part in parts: + bad += [f"{part.name}:{n}" for n in offenders_in(part.read_text())] + expect.text(", ".join(bad) or "none", "none", + "no selftest part directs a write at the live source tree") + + # PREMISES: the scan must be able to see each shape it claims to cover, or it + # passes on a pattern that matches nothing -- the shape it exists to refuse. + expect.num(len(offenders_in('printf x > "$PGC_SRCDIR/objstore/p.c"\n')), 1, + "premise: a direct write at the tree root is seen") + expect.num(len(offenders_in('_p="$_bd_root/objstore/p.c"\nprintf x > "$_p"\n')), 1, + "premise: and the INDIRECT form, which is the defect's own shape") + expect.num(len(offenders_in('_p="$_bd_copy/objstore/p.c"\nprintf x > "$_p"\n')), 0, + "control: a write into a COPY is not an offender") diff --git a/test/selftest/340-the-binary-must-be-built-from.sh b/test/selftest/340-the-binary-must-be-built-from.sh index f58f952e..b272dc49 100644 --- a/test/selftest/340-the-binary-must-be-built-from.sh +++ b/test/selftest/340-the-binary-must-be-built-from.sh @@ -128,17 +128,67 @@ check "every directory the Makefile builds from is in the fingerprint" \ "$([ -z "$_bd_missing" ] && echo none || echo "missing:$_bd_missing")" "none" # And the consequence, end to end: a change under a recursed directory must move -# the fingerprint. Asserted against the real tree rather than a fixture, because -# the defect was that the real tree's objstore/ was not being read. -_bd_probe="$_bd_root/objstore/.pgc_fingerprint_probe.c" -_bd_before="$(pgc_source_fingerprint "$_bd_root")" +# the fingerprint. Against a tree with the REAL tree's SHAPE -- because the defect +# was that the real tree's objstore/ was not being read, and a hand-built fixture +# could not have caught it -- but NOT against the real tree itself. +# +# THIS AROSE FROM WRITING INTO THE LIVE SOURCE TREE, AND IT COST FOUR PULL +# REQUESTS. This arm used to write `objstore/.pgc_fingerprint_probe.c` into +# $PGC_SRCDIR. harness_selftest runs IN the matrix, so at PGC_JOBS=4 it created +# that file in the shared build directory while up to three sibling suites +# fingerprinted concurrently, and whichever sampled inside the window reported +# +# FATAL: the binary under test was not built from this source +# source now a735c673b129, binary built from 6d122a7158d5 +# +# on a tree that was correct. The path is tree-RELATIVE and the content fixed, so +# the deviant value was IDENTICAL across majors, build directories and branches -- +# which is what made it look deterministic enough to be a real staleness, and what +# sent two agents chasing a transient md5sum failure and then a path-spelling +# defect. @linuxhikerpm found it by reading, and it reproduces exactly: +# +# clean tree 6d122a7158d5 +# with objstore/.pgc_fingerprint_probe.c a735c673b129 <- what CI reported +# +# A hardlinked copy costs no data and keeps the structure exact. Writing a NEW +# file into it cannot touch the original, because only existing files are shared. +# Entry by entry, and hardlink-first. `cp -al SRC DST` is NOT safe as a fallback +# pair: /tmp is a different filesystem from the tree here, so the hardlink copy +# failed AFTER creating DST, and `cp -a` then copied the tree INSIDE it -- the +# copy's build dirs came out as `bfix src` instead of `objstore src`. The PREMISE +# below caught that, which is the entire reason it is written as a premise rather +# than assumed. `*` also skips `.git`, which no part of this question needs. +_bd_copy="$(mktemp -d "${TMPDIR:-/tmp}/pgc-bdcopy.XXXXXX")/tree" +mkdir -p "$_bd_copy" +for _bd_entry in "$_bd_root"/*; do + [ -e "$_bd_entry" ] || continue + cp -al "$_bd_entry" "$_bd_copy/" 2>/dev/null || cp -a "$_bd_entry" "$_bd_copy/" +done +_bd_probe="$_bd_copy/objstore/.pgc_fingerprint_probe.c" + +# THE ARM THAT WOULD HAVE CAUGHT THE ORIGINAL. A suite that runs beside others +# must not write into the tree they are reading. +check "the probe is written outside the live source tree" \ + "$(case "$_bd_probe" in "$_bd_root"/*) echo "INSIDE $_bd_root" ;; *) echo outside ;; esac)" \ + "outside" + +# PREMISE: the copy is equivalent for the question being asked. If the copy did +# not carry objstore/, "a new file under objstore moves the fingerprint" would be +# measuring a directory the fingerprint never covered, and would pass for the +# wrong reason. +check "PREMISE the copy discovers the same build directories as the real tree" \ + "$(pgc_source_build_dirs "$_bd_copy" | sed "s|^$_bd_copy/||" | sort | tr '\n' ' ')" \ + "$(pgc_source_build_dirs "$_bd_root" | sed "s|^$_bd_root/||" | sort | tr '\n' ' ')" + +_bd_before="$(pgc_source_fingerprint "$_bd_copy")" printf 'int pgc_fingerprint_probe;\n' > "$_bd_probe" check "a new source file under objstore moves the fingerprint" \ - "$([ "$(pgc_source_fingerprint "$_bd_root")" != "$_bd_before" ] && echo moved || echo same)" \ + "$([ "$(pgc_source_fingerprint "$_bd_copy")" != "$_bd_before" ] && echo moved || echo same)" \ "moved" rm -f "$_bd_probe" check "and removing it restores the fingerprint" \ - "$(pgc_source_fingerprint "$_bd_root")" "$_bd_before" + "$(pgc_source_fingerprint "$_bd_copy")" "$_bd_before" +rm -rf "${_bd_copy%/tree}" # ---- the postmaster arm must be able to FIRE, not just to compute ----------- #