From 463c5b01836bcbe762b7c02493cf6668060f0a45 Mon Sep 17 00:00:00 2001 From: Matt McKay Date: Mon, 10 Aug 2026 08:54:21 +1000 Subject: [PATCH 1/2] Guardrail: hash a file whenever its manifest records a hash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `consumers: []` short-circuited the check before it reached the sha256 comparison, and this is the repo's only byte-integrity gate. Manifests land ahead of their repoints by convention, so a dataset arrives here with an empty consumer list and is flipped to `repointed` by a later PR in another repo — which made the one PR that introduces new bytes the one PR that never verifies them. `mpd2020.xlsx.yml` landed exactly that way in #38: hash recorded, consumers empty, never hashed until #41 filled them in. The hash check now keys on `integrity.sha256` being recorded rather than on `consumers` being non-empty. The consumer-specific errors are unchanged, so a live lecture whose file goes missing still says so in those terms. No behaviour change on `main`: all 18 manifests carry consumers and a hash today, so the run is identical bar the summary wording. Verified against a scratch tree for the four cases that matter — unconsumed + correct hash passes; unconsumed + drifted bytes fails; unconsumed + hash recorded but file absent fails; unconsumed with no hash stays out of scope. Lands before the `high_dim_data` fold, which introduces six datasets at `consumers: []` — QuantEcon/workspace-lectures#23 step 3, PR B. Co-Authored-By: Claude Opus 5 (1M context) --- .github/scripts/check_consumed_files.py | 37 +++++++++++++++++-------- 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/.github/scripts/check_consumed_files.py b/.github/scripts/check_consumed_files.py index 2091292..bf2dea6 100644 --- a/.github/scripts/check_consumed_files.py +++ b/.github/scripts/check_consumed_files.py @@ -1,5 +1,6 @@ #!/usr/bin/env python3 -"""Go-live guardrail: a PR must not break a file a live lecture consumes. +"""Go-live guardrail: a PR must not break a file a live lecture consumes, and +must not land bytes that do not match what the manifest says they are. For every manifest sidecar lectures/.yml: @@ -7,11 +8,19 @@ - if `consumers` is non-empty (a lecture reads this file in production): * the data file must exist * `integrity.sha256` must be recorded + - if `integrity.sha256` is recorded, with or without consumers: + * the data file must exist * the committed bytes must hash to it -Files with no manifest yet (Phase 6 backfill pending) or an empty `consumers` -list are out of scope here — the full validation suite (schema, dtypes, -invariants) is PLAN Phase 5 and will subsume this check. +The second clause exists because manifests land *ahead* of their repoints by +convention, so a dataset arrives with `consumers: []` and is flipped by a +later PR in another repo. Keying the hash check on `consumers` alone meant +the one PR that introduces new bytes was the one PR that never verified +them — and this is the repo's only byte-integrity gate. + +Files with no manifest yet (Phase 6 backfill pending), and manifests that +record no hash, are out of scope here — the full validation suite (schema, +dtypes, invariants) is PLAN Phase 5 and will subsume this check. """ from __future__ import annotations @@ -60,8 +69,13 @@ def main() -> int: continue consumers = manifest.get("consumers") or [] - if not consumers: - continue # nothing live reads it — out of scope for this guardrail + integrity = manifest.get("integrity") + # A non-dict integrity (e.g. a stray string) is treated as missing, so + # it lands in the "not recorded" error below instead of crashing here. + recorded = integrity.get("sha256") if isinstance(integrity, dict) else None + + if not consumers and not recorded: + continue # nothing reads it, nothing to verify — out of scope checked += 1 data_path = LECTURES / declared @@ -69,13 +83,12 @@ def main() -> int: errors.append( f"{declared}: consumed by {len(consumers)} lecture(s) but the " f"data file is missing — this would break a live lecture build" + if consumers else + f"{declared}: integrity.sha256 is recorded but the data file " + f"is missing — a manifest describes a file this repo publishes" ) continue - integrity = manifest.get("integrity") - # A non-dict integrity (e.g. a stray string) is treated as missing, so - # it lands in the "not recorded" error below instead of crashing here. - recorded = integrity.get("sha256") if isinstance(integrity, dict) else None if not recorded: errors.append( f"{declared}: consumed but integrity.sha256 is not recorded — " @@ -96,8 +109,8 @@ def main() -> int: for e in errors: print(f"::error::{e}") print( - f"{len(manifests)} manifest(s) found, {checked} consumed file(s) " - f"checked, {len(errors)} error(s)" + f"{len(manifests)} manifest(s) found, {checked} file(s) hash-checked, " + f"{len(errors)} error(s)" ) return 1 if errors else 0 From 184d66aa601a61e5a2bf9a5d9403a2523152e280 Mon Sep 17 00:00:00 2001 From: Matt McKay Date: Mon, 10 Aug 2026 10:04:09 +1000 Subject: [PATCH 2/2] Guardrail: fail on a malformed integrity block, and count only real hashes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes from Copilot's review, both confirmed by reproduction. A present-but-non-mapping `integrity` (e.g. `integrity: "sha256: …"`, one missing indent level) read as "no hash recorded", so a manifest with `consumers: []` short-circuited before the byte check and the run was silently green — 0 errors, 0 files hashed, exit 0. That is a hole this PR opened: with a consumer present the same manifest still errored, so it was only the new unconsumed path, which is exactly the shape every new dataset lands in. It is now a hard error either way, and the comment claiming a non-dict integrity "lands in the not recorded error below" goes with it, because after the keying change it did not. `checked` was also incremented before the existence and hash-recorded checks, so the summary could report a file as hash-checked when sha256() was never called. Accurate under the old "consumed file(s) checked" label; wrong under this PR's rename. The counter moves to where the hash is computed. Verified: the four cases in the PR body still behave, plus the two new ones — a string integrity with no consumers now exits 1, and a consumed manifest with no recorded hash reports 0 hash-checked rather than 1. The real tree is unchanged: 18 manifests, 18 hash-checked, exit 0. Co-Authored-By: Claude Opus 5 (1M context) --- .github/scripts/check_consumed_files.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/.github/scripts/check_consumed_files.py b/.github/scripts/check_consumed_files.py index bf2dea6..b1516aa 100644 --- a/.github/scripts/check_consumed_files.py +++ b/.github/scripts/check_consumed_files.py @@ -70,14 +70,23 @@ def main() -> int: consumers = manifest.get("consumers") or [] integrity = manifest.get("integrity") - # A non-dict integrity (e.g. a stray string) is treated as missing, so - # it lands in the "not recorded" error below instead of crashing here. - recorded = integrity.get("sha256") if isinstance(integrity, dict) else None + # A present-but-malformed integrity block must fail loudly. Read as + # "no hash recorded" it would skip the byte check entirely for a + # manifest that has no consumers yet — which is how every new dataset + # lands here, since manifests precede their repoints. + if integrity is not None and not isinstance(integrity, dict): + errors.append( + f"{declared}: `integrity` must be a mapping, got " + f"{type(integrity).__name__} — check the indentation under " + f"`integrity:`; as written the sha256 is unreadable and the " + f"byte check would be skipped silently" + ) + continue + recorded = (integrity or {}).get("sha256") if not consumers and not recorded: continue # nothing reads it, nothing to verify — out of scope - checked += 1 data_path = LECTURES / declared if not data_path.exists(): errors.append( @@ -97,6 +106,7 @@ def main() -> int: continue actual = sha256(data_path) + checked += 1 # counted where the hash is actually computed if actual != recorded: errors.append( f"{declared}: bytes do not match the manifest (sha256 {actual} "