diff --git a/CLAUDE.md b/CLAUDE.md index e018612..22ad809 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1528,6 +1528,16 @@ library bump breaking one profile should leave a legible blocker plus testable artifacts for the profiles that still work — not an empty `dist/` and an aborted make. +**The gate cannot pass vacuously.** Both disk checks derive the set of +images that MUST exist from the build record rather than iterating +whatever `*.d64` happens to be in `dist/`, so an absent image is a +failed check instead of a check nobody ran. An empty `dist/` used to +print `0/0 checks passed / RELEASE ARTIFACTS VERIFIED` and exit 0 — a +green light over a release containing nothing. A run that records zero +checks now fails, and the word `VERIFIED` is reserved for a run where +every section executed: any `SKIP_*` downgrades the verdict to +`PARTIAL VERIFICATION`. + `make package-verify` is the acceptance gate (`tools/package/ verify_release.py`): rebuilds every variant and compares **PRG** hashes (object hashes are not evidence — ca65 stamps build time into every diff --git a/Makefile b/Makefile index b977b57..4c4db56 100644 --- a/Makefile +++ b/Makefile @@ -484,5 +484,17 @@ package: # and compare PRG hashes, boot every D64 in VICE and assert the banner, and run # the built listener end to end against a Python ssl client. Measures; does not # assert. Run it after `make package`. +# +# The gate's own verdict logic is unit-tested first, and that ordering is the +# point: verify_release.py shipped with a bug where it reported RELEASE +# ARTIFACTS VERIFIED having checked nothing, so "the gate said yes" is only +# worth something if the gate's yes still means what it should. The tests need +# no VICE and no build; they cost milliseconds. A failure here stops the run +# rather than letting a broken verdict bless a release. +# +# tools/run_all_tests.py deliberately does not carry these: it allocates a VICE +# instance per suite and dispatches `run_tests(transport, labels, seed)`, a +# shape a pure-logic test does not fit. The gate is the right home for them. package-verify: + $(PACKAGE_PYTHON) tools/test_package_verify.py $(PACKAGE_PYTHON) tools/package/verify_release.py diff --git a/tools/package/build_prgs.sh b/tools/package/build_prgs.sh index 2e1cddc..aa73b8d 100755 --- a/tools/package/build_prgs.sh +++ b/tools/package/build_prgs.sh @@ -120,7 +120,10 @@ for line in "${PACKAGE_VARIANTS[@]}"; do rm -f "$log" bytes="$(wc -c < "$DIST/$prg" | tr -d ' ')" sha="$(sha256_of "$DIST/$prg")" - echo "variant=$key prg=$prg args=$args result=OK bytes=$bytes sha256=$sha" \ + # backend= lets verify_release.py derive which disk images MUST exist + # without re-parsing this matrix, keeping _common.sh the only place a + # variant is declared. + echo "variant=$key prg=$prg args=$args result=OK bytes=$bytes sha256=$sha backend=$(variant_field "$line" 5)" \ >> "$BUILD_INFO" printf '[package] wrote dist/%s %s bytes %s\n' "$prg" "$bytes" "$sha" done diff --git a/tools/package/verify_release.py b/tools/package/verify_release.py index e771033..843446f 100755 --- a/tools/package/verify_release.py +++ b/tools/package/verify_release.py @@ -120,6 +120,29 @@ def d64_images() -> list[Path]: return sorted(DIST.glob("*.d64")) +def expected_d64_images(variants: list[dict]) -> list[Path]: + """The disk images that MUST exist, given which variants built. + + This is the antidote to a vacuous pass. Both disk checks used to iterate + whatever `*.d64` happened to be in dist/, so an empty dist/ meant zero + checks ran, zero failed, and the run reported RELEASE ARTIFACTS VERIFIED — + a green light over a release with no disks in it at all. Deriving the + expected set from the build record instead means an absent image is a + failed check rather than a check nobody ran. + """ + ok = [r for r in variants if r.get("result") == "OK"] + images = [DIST / f"c64-https-{r['key']}.d64" for r in ok] + backends: list[str] = [] + for r in ok: + # backend= is written by build_prgs.sh; older build-info files predate + # it, so fall back to the key's prefix rather than crashing. + b = r.get("backend") or r["key"].split("-")[0] + if b not in backends: + backends.append(b) + images += [DIST / f"c64-https-{b}.d64" for b in backends] + return sorted(set(images)) + + def check_d64_contents(variants: list[dict]) -> None: """Read each PRG back out of each disk and byte-compare it.""" print("\n=== 2a. D64 contents (c1541 read-back, byte-compare) ===") @@ -127,8 +150,24 @@ def check_d64_contents(variants: list[dict]) -> None: if not shutil.which(c1541): record("c1541 available", False, "not on PATH") return + expected = expected_d64_images(variants) + if not expected: + record("disk images expected", False, + "no variant built, so no disk image could be expected — " + "nothing here was verified") + return + present = set(d64_images()) + for image in expected: + if image not in present: + record(f"{image.name} exists", False, + "expected from the build record but absent from dist/ — " + "did build_d64.sh run?") + stray = sorted(p.name for p in present - set(expected)) + if stray: + record("no unexpected disk images", False, + f"dist/ carries images no variant accounts for: {stray}") by_prg = {r["prg"]: r for r in variants} - for image in d64_images(): + for image in [i for i in expected if i in present]: listing = subprocess.run([c1541, "-attach", str(image), "-list"], capture_output=True, text=True).stdout names = [ln.split('"')[1] for ln in listing.splitlines() @@ -160,7 +199,7 @@ def check_d64_contents(variants: list[dict]) -> None: record(f"{image.name} carries the built PRGs", ok, "; ".join(detail)) -def check_d64_boots() -> None: +def check_d64_boots(variants: list[dict]) -> None: """Autostart every disk image in VICE and assert the boot banner. Two flags are load-bearing, and both were found the hard way: @@ -191,7 +230,17 @@ def check_d64_boots() -> None: return timeout = float(os.environ.get("VICE_BOOT_TIMEOUT", "240")) import time - for image in d64_images(): + expected = expected_d64_images(variants) + present = set(d64_images()) + if not expected: + record("disk images to boot", False, + "no variant built, so nothing was booted — " + "nothing here was verified") + return + for image in expected: + if image not in present: + record(f"{image.name} bootable", False, "image absent from dist/") + for image in [i for i in expected if i in present]: # Backend is in the filename by construction (see _common.sh); the # per-backend disks autostart their first file, which is that # backend's REU profile. @@ -274,6 +323,53 @@ def check_listener() -> None: f"found {leftovers}" if leftovers else "clean") +def summarize(results: list, missing: int, skipped: list) -> tuple: + """Turn the recorded checks into a verdict. Pure — see test_package_verify.py. + + Split out of main() precisely because this is where the pressure to say + something reassuring lands. The ordering below is the whole contract: + + 1. zero checks -> failure. A gate that ran nothing is not a gate that + passed, and this is not hypothetical: the glob-driven + disk checks used to record nothing on an empty dist/ + and the run reported RELEASE ARTIFACTS VERIFIED. + 2. any failure -> failure. + 3. any missing -> RELEASE INCOMPLETE. What is present may verify fine; + the matrix is still not releasable. + 4. any skip -> PARTIAL VERIFICATION, exit 0 so SKIP_* stays usable + for narrowing, but never the word VERIFIED — a run + that skipped sections is evidence about what ran, not + about the release. + 5. otherwise -> RELEASE ARTIFACTS VERIFIED. + + Rule 1 is checked before rule 4 on purpose: skipping every section must + not launder an empty run into a cheerful PARTIAL. + """ + failed = [n for n, ok, _ in results if not ok] + lines = ["\n" + "=" * 60, + f"{len(results) - len(failed)}/{len(results)} checks passed"] + if not results: + lines.append("NOTHING WAS VERIFIED — no check ran. This is a failure, " + "not a pass.") + return 1, lines + if failed: + lines.append("FAILED:") + lines += [f" - {name}" for name in failed] + return 1, lines + if missing: + lines.append(f"Everything present verifies, but {missing} variant(s) " + f"are MISSING — see the blocker above.") + lines.append("RELEASE INCOMPLETE") + return 1, lines + if skipped: + lines.append(f"PARTIAL VERIFICATION — everything that ran passed, but " + f"these were SKIPPED: {', '.join(skipped)}.") + lines.append("Not a release gate. Re-run without SKIP_* before tagging.") + return 0, lines + lines.append("RELEASE ARTIFACTS VERIFIED") + return 0, lines + + def report_missing_variants(variants: list[dict]) -> int: """Surface variants that never built, with the toolchain's own reason. @@ -308,37 +404,30 @@ def main() -> int: f"{len(d64_images())} disk images in {DIST}") missing = report_missing_variants(variants) + skipped: list[str] = [] if os.environ.get("SKIP_REBUILD") != "1": check_reproducible(variants) else: print("\n=== 1. PRG reproducibility SKIPPED (SKIP_REBUILD=1) ===") + skipped.append("reproducibility") check_d64_contents(variants) if os.environ.get("SKIP_VICE") != "1": - check_d64_boots() + check_d64_boots(variants) else: print("\n=== 2b. VICE boots SKIPPED (SKIP_VICE=1) ===") + skipped.append("VICE boots") if os.environ.get("SKIP_LISTENER") != "1": check_listener() else: print("\n=== 3. Listener SKIPPED (SKIP_LISTENER=1) ===") + skipped.append("listener") - failed = [n for n, ok, _ in results if not ok] - print(f"\n{'=' * 60}") - print(f"{len(results) - len(failed)}/{len(results)} checks passed") - if failed: - print("FAILED:") - for name in failed: - print(f" - {name}") - return 1 - if missing: - print(f"Everything present verifies, but {missing} variant(s) are " - f"MISSING — see the blocker above.") - print("RELEASE INCOMPLETE") - return 1 - print("RELEASE ARTIFACTS VERIFIED") - return 0 + code, lines = summarize(results, missing, skipped) + for line in lines: + print(line) + return code if __name__ == "__main__": diff --git a/tools/test_package_verify.py b/tools/test_package_verify.py new file mode 100644 index 0000000..d6becd4 --- /dev/null +++ b/tools/test_package_verify.py @@ -0,0 +1,236 @@ +#!/usr/bin/env python3 +"""test_package_verify.py — regression tests for the release gate's verdict. + +No VICE, no builds, no hardware: this exercises the pure logic in +tools/package/verify_release.py that decides whether a run may call itself +verified. Runs in milliseconds. + +WHY THIS FILE EXISTS. The gate shipped with a bug where it could pass having +checked nothing: both disk checks iterated `dist/*.d64`, so an empty dist/ +produced zero records, zero failures, and the cheerful verdict "0/0 checks +passed / RELEASE ARTIFACTS VERIFIED" with exit 0. A green light over a release +containing no disks at all. + +The durable fix has two halves, and both are pinned here: + + * iterate over what MUST exist (derived from the build record), not over + what happens to be on disk — an empty iteration is then a failed check + rather than a check nobody ran (test_expected_images_*); + * never let an empty or partial run reach a reassuring verdict + (test_verdict_*). + +The SKIP_* case is the one to watch: it is where the pressure to just say +VERIFIED will come back, because it is the invocation people actually use +while iterating. It must stay exit 0 (so it remains usable) while never +producing the word VERIFIED. + +Usage: python3 tools/test_package_verify.py +""" +from __future__ import annotations + +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent / "package")) + +import verify_release as vr # noqa: E402 + +PASSED = 0 +FAILED = 0 + + +def check(name: str, got, want) -> None: + global PASSED, FAILED + if got == want: + PASSED += 1 + print(f" [PASS] {name}") + else: + FAILED += 1 + print(f" [FAIL] {name}\n got {got!r}\n want {want!r}") + + +def verdict(results, missing=0, skipped=()): + """Return (exit_code, joined text) for a set of recorded checks.""" + code, lines = vr.summarize(list(results), missing, list(skipped)) + return code, "\n".join(lines) + + +OK = ("some check", True, "") +BAD = ("some check", False, "boom") + + +# --------------------------------------------------------------------------- +# Verdict logic +# --------------------------------------------------------------------------- + +def test_verdict_empty_run_is_a_failure() -> None: + """The original bug, pinned: zero checks must never read as success.""" + print("\n-- an empty run is a failure, not a pass --") + code, text = verdict([]) + check("exit code is 1", code, 1) + check("says NOTHING WAS VERIFIED", "NOTHING WAS VERIFIED" in text, True) + check("never says VERIFIED alone", "RELEASE ARTIFACTS VERIFIED" in text, False) + + +def test_verdict_empty_run_not_rescued_by_skips() -> None: + """Skipping every section must not launder an empty run into PARTIAL. + + This is the ordering that matters most: the skip branch is friendlier and + exits 0, so if it were checked first, `SKIP_REBUILD=1 SKIP_VICE=1 + SKIP_LISTENER=1` on an empty dist/ would report a cheerful partial pass — + which is exactly the shape of the original bug wearing a different hat. + """ + print("\n-- an empty run stays a failure even when everything was skipped --") + code, text = verdict([], skipped=["reproducibility", "VICE boots", "listener"]) + check("exit code is 1", code, 1) + check("says NOTHING WAS VERIFIED", "NOTHING WAS VERIFIED" in text, True) + check("does not claim PARTIAL VERIFICATION", + "PARTIAL VERIFICATION" in text, False) + + +def test_verdict_clean_full_run() -> None: + print("\n-- a complete, all-passing run is the only VERIFIED --") + code, text = verdict([OK, OK, OK]) + check("exit code is 0", code, 0) + check("says RELEASE ARTIFACTS VERIFIED", + "RELEASE ARTIFACTS VERIFIED" in text, True) + + +def test_verdict_skips_never_say_verified() -> None: + """Case D. Usable (exit 0), but never the word VERIFIED.""" + print("\n-- a skipped section downgrades the verdict but stays usable --") + code, text = verdict([OK, OK], skipped=["VICE boots"]) + check("exit code is 0 (SKIP_* stays usable)", code, 0) + check("says PARTIAL VERIFICATION", "PARTIAL VERIFICATION" in text, True) + check("does NOT say RELEASE ARTIFACTS VERIFIED", + "RELEASE ARTIFACTS VERIFIED" in text, False) + check("names what was skipped", "VICE boots" in text, True) + check("says it is not a release gate", "Not a release gate" in text, True) + + +def test_verdict_failures_win() -> None: + print("\n-- a failed check fails the run --") + code, text = verdict([OK, BAD]) + check("exit code is 1", code, 1) + check("lists the failure", "some check" in text, True) + check("does not say VERIFIED", "RELEASE ARTIFACTS VERIFIED" in text, False) + + +def test_verdict_missing_variants_block() -> None: + print("\n-- present artifacts verifying does not excuse a missing variant --") + code, text = verdict([OK, OK], missing=2) + check("exit code is 1", code, 1) + check("says RELEASE INCOMPLETE", "RELEASE INCOMPLETE" in text, True) + check("does not say VERIFIED", "RELEASE ARTIFACTS VERIFIED" in text, False) + + +def test_verdict_failure_outranks_missing() -> None: + print("\n-- a real failure is reported ahead of the missing-variant note --") + code, text = verdict([BAD], missing=1) + check("exit code is 1", code, 1) + check("reports the failure", "FAILED:" in text, True) + + +# --------------------------------------------------------------------------- +# Coverage derivation — iterate over what MUST exist +# --------------------------------------------------------------------------- + +def test_expected_images_from_build_record() -> None: + print("\n-- expected disk images come from the build record --") + variants = [ + {"key": "uci-reu", "prg": "a.prg", "result": "OK", "backend": "uci"}, + {"key": "uci-onchip", "prg": "b.prg", "result": "OK", "backend": "uci"}, + {"key": "ip65-reu", "prg": "c.prg", "result": "OK", "backend": "ip65"}, + {"key": "ip65-onchip", "prg": "d.prg", "result": "OK", "backend": "ip65"}, + ] + names = sorted(p.name for p in vr.expected_d64_images(variants)) + check("four singles plus two per-backend images", names, [ + "c64-https-ip65-onchip.d64", + "c64-https-ip65-reu.d64", + "c64-https-ip65.d64", + "c64-https-uci-onchip.d64", + "c64-https-uci-reu.d64", + "c64-https-uci.d64", + ]) + + +def test_expected_images_skip_failed_variants() -> None: + """A variant that did not build must not be expected to have a disk.""" + print("\n-- a variant that failed to build is not expected on disk --") + variants = [ + {"key": "uci-reu", "prg": "a.prg", "result": "OK", "backend": "uci"}, + {"key": "uci-onchip", "prg": "b.prg", "result": "FAILED", "backend": "uci"}, + ] + names = sorted(p.name for p in vr.expected_d64_images(variants)) + check("only the built variant's disk plus its backend disk", names, + ["c64-https-uci-reu.d64", "c64-https-uci.d64"]) + + +def test_expected_images_empty_when_nothing_built() -> None: + """Empty here is what makes the disk checks record an explicit failure.""" + print("\n-- nothing built means nothing expected (checks then fail loudly) --") + variants = [{"key": "uci-reu", "prg": "a.prg", "result": "FAILED", + "backend": "uci"}] + check("no images expected", vr.expected_d64_images(variants), []) + + +def test_expected_images_tolerate_old_build_info() -> None: + """backend= was added late; a build-info without it must not crash.""" + print("\n-- a build record predating backend= still derives correctly --") + variants = [{"key": "ip65-onchip", "prg": "d.prg", "result": "OK"}] + names = sorted(p.name for p in vr.expected_d64_images(variants)) + check("backend inferred from the key prefix", names, + ["c64-https-ip65-onchip.d64", "c64-https-ip65.d64"]) + + +# --------------------------------------------------------------------------- +# build-info parsing +# --------------------------------------------------------------------------- + +def test_parse_build_info_records(tmp_lines: list[str]) -> None: + print("\n-- build-info records parse, including args with spaces --") + import tempfile + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "build-info.txt" + path.write_text("\n".join(tmp_lines) + "\n") + saved = vr.BUILD_INFO + vr.BUILD_INFO = path + try: + recs = vr.parse_build_info() + finally: + vr.BUILD_INFO = saved + check("two records", len(recs), 2) + check("OK record result", recs[0]["result"], "OK") + check("multi-word args survive", recs[0]["args"], + "BACKEND=uci USE_NISTCURVES_ONCHIP=1") + check("sha captured", recs[0]["sha256"], "abc123") + check("backend captured", recs[0]["backend"], "uci") + check("FAILED record result", recs[1]["result"], "FAILED") + + +def main() -> int: + print("=== release-gate verdict regression tests ===") + test_verdict_empty_run_is_a_failure() + test_verdict_empty_run_not_rescued_by_skips() + test_verdict_clean_full_run() + test_verdict_skips_never_say_verified() + test_verdict_failures_win() + test_verdict_missing_variants_block() + test_verdict_failure_outranks_missing() + test_expected_images_from_build_record() + test_expected_images_skip_failed_variants() + test_expected_images_empty_when_nothing_built() + test_expected_images_tolerate_old_build_info() + test_parse_build_info_records([ + "variant=uci-onchip prg=x.prg args=BACKEND=uci USE_NISTCURVES_ONCHIP=1" + " result=OK bytes=62977 sha256=abc123 backend=uci", + "variant=ip65-onchip prg=y.prg args=BACKEND=ip65 result=FAILED" + " log=build-ip65-onchip.log", + ]) + print(f"\n{'=' * 60}") + print(f"{PASSED} passed, {FAILED} failed") + return 1 if FAILED else 0 + + +if __name__ == "__main__": + sys.exit(main())