fix(snapshot): refuse markers whose format identity the exporter would refuse - #486
Conversation
The exporter's replace guard and the new verifier gate asked the same two-part question in two places. If the verifier were ever the looser of the two, exit 0 would start meaning 'some directory with an integrity-shaped key matched'. Shared predicate, separate messages: each caller says what it was trying to do.
Two mutations went unnoticed. Dropping the format-name comparison left the suite green, because the foreign-marker test uses a marker carrying neither field and the version check alone refuses it. Disabling the exporter's replace guard also left it green: that guard had no test anywhere, which matters more now the predicate is shared, since it could be loosened from one side and noticed by neither.
kstonekuan
left a comment
There was a problem hiding this comment.
LGTM. Merging.
The probe back to 402688e is what makes this safe to land, and it is the question I would have asked: if any exporter revision had ever omitted those fields, this gate would refuse snapshots we ourselves produced. It never did.
I pushed two commits.
One predicate, two callers. The gate repeated the exporter's replace guard at :617-619 verbatim. Two copies of "is this ours" can drift, and if the verifier were ever the looser of the two, exit 0 would start meaning "some directory with an integrity-shaped key matched". Now both call _marker_identifies_dataset_snapshot and each keeps its own message, since one is refusing a destination and the other is refusing input.
Then the tests that extraction requires. Two mutations went unnoticed:
predicate stops checking the format name 37 passed (nothing noticed)
exporter replace guard never fires 37 passed (nothing noticed)
The first is inside your change. test_foreign_marker_is_refused_at_the_boundary uses a marker carrying neither field, so the version check alone refuses it and the name comparison never fires. Added a marker claiming version 1 of somebody else's format.
The second is older than this PR: the exporter's replace guard had no test anywhere in the tree. That was tolerable while it was its own copy and is not once the predicate is shared, because it could then be loosened from one side and noticed by neither. Added three cases, asserting the directory still has its contents afterwards, since a refused overwrite must not have deleted anything first.
After both, all five mutations bite from both sides:
verifier gate never fires 2 failed
exporter replace guard never fires 3 failed
predicate stops checking the version 3 failed
predicate stops checking the format name 2 failed
version compared loosely (JSON int 1 accepted) 2 failed
Validated on the rebased result: ruff, ruff format, ty, 1743 passed / 6 skipped.
That is #472 and #473 both in, and #469 stays with VARUN3WARE as you noted. Three of us have now been over this one function's boundary, so if you want the fourth thing there: #489 turns the receipt entries into a real type, and its hard constraint is that the content_id digest your #483 gate now enforces must not move by a byte. Labelled advanced, and it is yours if you want it.
fix(snapshot): refuse markers whose format identity the exporter would refuse
Closes #472.
What changed
verify_dataset_snapshotnow pins the marker's format identity at the boundary, immediately after the marker is parsed and before any receipt logic runs. The comparison is deliberately identical to the exporter's replace guard (snapshot.py:618-625) and the writer (:772-773):formatmust equalhflow-dataset-snapshotandformat_versionmust equal"1", so there is exactly one answer to "is this an HFlow snapshot".A marker without that identity, or with a future version, raises through the same
ValueErrorthe unparsable-marker path uses, so the CLI maps it to exit 2 with no new exit code. A foreign directory that happens to carry an integrity-shaped key can no longer be certified; on this base the repro from the issue returnedok: Truefor exactly that input.The maintainer's two notes, applied
"format_version": 1as a JSON int, and the writer records the string"1". The comparison inherits the export guard's strictness, so the int is refused, and per the request the error message says exactly why: it names the found values, the expected values, and states that the version's type is checked because the writer records it as a string.1is the only version there has ever been; refusing anything else at the same boundary sets the version-2 precedent (a future verifier decides what a newer marker can partly read, and that choice gets made then, deliberately).Pre-#401 probe result
The very first exporter (402688e, #132) already wrote both fields unconditionally (writer lines :645-646 of that revision), and every revision since has kept them. So no marker the exporter has ever produced lacks the identity, no valid historical input changes answer, and the pre-#401 no-integrity marker (identity present,
integrityabsent) still returns theno-receiptUNVERIFIABLE path, not a raise.Boundary family and sequencing
These three sit on one function's input boundary, and the maintainer asked for one change rather than three where possible:
content_idgate, built to the maintainer's spec on the issue; this branch is rebased onto it and both gates pass the same snapshots.Gates
15 passed in tests/test_snapshot_verify.py (13 existing + the foreign-marker test + the version test covering both the future-version and int-1 refusals), 31 passed across the verify and dataset-snapshot suites, ruff check / ruff format / ty clean. Mutation run: with the identity gate disabled, exactly the two new raise tests go red while the clean, pre-#401, and moved-root tests stay green; restored, all green. Zero non-ASCII and zero em dashes in changed files.