From d53c007cd8a84a66611fde50b51d72fa6066a2cf Mon Sep 17 00:00:00 2001 From: Sagar Kharal Date: Thu, 10 Sep 2026 10:29:03 +0530 Subject: [PATCH 1/2] fix(snapshot): enforce the receipt inventory content_id at verify time --- src/hflow/snapshot.py | 29 ++++++++++++++++++++++- tests/test_snapshot_verify.py | 43 +++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/src/hflow/snapshot.py b/src/hflow/snapshot.py index bab40e6d..9ca38326 100644 --- a/src/hflow/snapshot.py +++ b/src/hflow/snapshot.py @@ -828,7 +828,11 @@ def verify_dataset_snapshot( Unreadable input (a missing directory, a missing or unparsable ``format.json``) raises instead: that is not a finding about a delivered - snapshot, it is the wrong input entirely. + snapshot, it is the wrong input entirely. The same applies to a receipt + that is internally inconsistent with itself: a missing or malformed + ``content_id``, or a recomputed inventory hash that disagrees with the + stored one, means the receipt no longer describes the delivered set + (tampering or a truncated write), which is exit 2, not damaged bytes. Extra files under ``assets/`` that the receipt does not name are ignored: the receipt covers what was exported, not everything a recipient may add. @@ -882,6 +886,29 @@ def verify_dataset_snapshot( *integrity.get("tables", {}).values(), *integrity.get("assets", []), ] + + # The deleted-member gate (#473): when a receipt entry and its file are + # both gone, the surviving entries are self-consistent and every per-file + # check passes; only the stored inventory hash, computed over the original + # set, differs from the hash of what remains. Recompute it exactly as the + # exporter did (:194) and refuse a receipt that no longer describes the + # delivered set. Internal inconsistency is unreadable input, not damage, + # so it raises to exit 2 like an unparsable marker rather than reporting + # findings. + stored_content_id = integrity.get("content_id") + if not isinstance(stored_content_id, str) or not stored_content_id: + raise ValueError( + "format.json integrity receipt carries no usable content_id; " + "the delivered member set cannot be checked against the receipt" + ) + recomputed_content_id = _inventory_content_id(receipt_entries) + if recomputed_content_id != stored_content_id: + raise ValueError( + "format.json integrity receipt is internally inconsistent: recomputed " + f"inventory content_id {recomputed_content_id!r} != stored " + f"{stored_content_id!r}; the receipt no longer describes the delivered set" + ) + for entry in receipt_entries: relative_path = str(entry["path"]) delivered_path = resolved_directory / relative_path diff --git a/tests/test_snapshot_verify.py b/tests/test_snapshot_verify.py index 5adda878..c17d80da 100644 --- a/tests/test_snapshot_verify.py +++ b/tests/test_snapshot_verify.py @@ -89,6 +89,49 @@ def test_missing_file_reports_missing_alone(tmp_path: Path) -> None: assert marker["integrity"]["tables"]["samples"]["path"] in report.findings[0].uri +def test_removed_receipt_entry_and_file_raise_inventory_mismatch(tmp_path: Path) -> None: + """#473's deleted-member case: when a receipt entry and its file are both + gone, the surviving entries agree with each other and every per-file + check passes; only the stored inventory content_id, computed over the + original set, can witness the loss. The marker is internally + inconsistent, so verify raises (CLI exit 2) instead of certifying.""" + + def strip_measurements(output_directory: Path) -> str: + marker_path = output_directory / "format.json" + marker = json.loads(marker_path.read_text()) + entry = marker["integrity"]["tables"].pop("measurements") + marker_path.write_text(json.dumps(marker, indent=2, sort_keys=True) + "\n") + (output_directory / entry["path"]).unlink() + return entry["path"] + + output_directory, _ = _export_two_episode_snapshot(tmp_path, "references") + removed = strip_measurements(output_directory) + + with pytest.raises(ValueError, match="content_id"): + verify_dataset_snapshot(output_directory) + + # A fresh export for the CLI path: the raise must map to exit 2, the + # unreadable-input code, not to a findings-based exit. + output_directory, _ = _export_two_episode_snapshot(tmp_path / "cli", "references") + strip_measurements(output_directory) + assert cli_main(["verify", "snapshot", str(output_directory)]) == 2 + assert removed + + +def test_deleted_file_with_intact_receipt_reports_missing(tmp_path: Path) -> None: + """Negative control for #473: delete the file but keep its receipt entry. + This is the ordinary ``missing`` path and must keep reporting DAMAGED + with or without the inventory gate; it exercises the per-file loop, not + the gate.""" + output_directory, marker = _export_two_episode_snapshot(tmp_path, "references") + (output_directory / marker["integrity"]["tables"]["measurements"]["path"]).unlink() + + report = verify_dataset_snapshot(output_directory) + + assert not report.ok + assert [f.reason for f in report.findings] == ["missing"] + + def test_truncated_file_reports_size_mismatch_and_skips_the_hash( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: From df77266134efd332b83be5e0e265d420f3910dc2 Mon Sep 17 00:00:00 2001 From: Kingston Date: Wed, 9 Sep 2026 23:17:23 -0700 Subject: [PATCH 2/2] test(snapshot): pin the missing-content_id half of the #473 gate The gate is two refusals. The mismatch branch is held by the pinning test; deleting the missing-or-malformed branch left the whole suite green. Four shapes: absent, empty, non-string, wrong type, each through the API and the CLI exit code. --- tests/test_snapshot_verify.py | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/test_snapshot_verify.py b/tests/test_snapshot_verify.py index c17d80da..1c6978fe 100644 --- a/tests/test_snapshot_verify.py +++ b/tests/test_snapshot_verify.py @@ -132,6 +132,38 @@ def test_deleted_file_with_intact_receipt_reports_missing(tmp_path: Path) -> Non assert [f.reason for f in report.findings] == ["missing"] +@pytest.mark.parametrize( + ("replacement", "label"), + [(None, "absent"), ("", "empty"), (0, "not-a-string"), ([], "wrong-type")], + ids=["absent", "empty", "not-a-string", "wrong-type"], +) +def test_receipt_without_a_usable_content_id_is_refused( + tmp_path: Path, replacement: object, label: str +) -> None: + """The other half of the #473 gate, which the mismatch test cannot reach. + + A receipt whose ``content_id`` is missing or unusable cannot witness a + deleted member at all, so certifying it would be certifying that the + check ran. Deleting this branch left the whole suite green, so it needs + its own case. An ``integrity`` block with no ``content_id`` is not + something hflow writes (both arrived in #401), which is exactly why a + marker carrying one is unreadable input rather than damaged bytes. + """ + output_directory, _ = _export_two_episode_snapshot(tmp_path, "references") + marker_path = output_directory / "format.json" + marker = json.loads(marker_path.read_text()) + if replacement is None: + marker["integrity"].pop("content_id") + else: + marker["integrity"]["content_id"] = replacement + marker_path.write_text(json.dumps(marker, indent=2, sort_keys=True) + "\n") + + with pytest.raises(ValueError, match="no usable content_id"): + verify_dataset_snapshot(output_directory) + + assert cli_main(["verify", "snapshot", str(output_directory)]) == 2, label + + def test_truncated_file_reports_size_mismatch_and_skips_the_hash( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: