feat(verify): check LeRobot prepared-manifest deliveries against receipts

[VARUN3WARE]

Summary

• Add shared VerificationReport / VerificationFinding / VerificationStatus in src/hflow/verification.py for the hflow verify family ([Feature]: LeRobot prepared-manifest receipts are written but nothing verifies a delivered import #432; contract with Snapshot integrity receipts are written but nothing reads them: no way to verify a delivered snapshot #428).
• Implement verify_lerobot_import and CLI hflow verify lerobot-import to check schema-3 prepared-manifest receipts (uri, content_id, size_bytes) without re-converting.
• Exit codes: 0 clean, 1 damaged, 2 unreadable, 3 unverifiable; unlisted landing/ extras ignored.
• Docs: import-lerobot-v3.md names the real verify path; FORMAT.md documents verify exit 3.

Test plan

• uv run ruff check / ruff format --check / ty check
• uv run pytest tests/test_verification.py -q
• Full uv run pytest -q (1594 passed, 6 skipped)
• Confirm CI green
• @Sagar-024: please review — this lands verification.py first for Snapshot integrity receipts are written but nothing reads them: no way to verify a delivered snapshot #428 to import

[Sagar-024]

Approved. The part I like most is exit_code_for: 0, 1, and 3 live in one function, 2 is raised for unreadable input before a report exists, and FORMAT.md documents the split so CI can tell an unverifiable delivery from a damaged one. That split is easy to get wrong per command.

Checked against what the #432 contract needs: VerificationReport with .ok and .findings, each finding carrying .uri, .reason, and .detail, and the module importing from catalog and storage only, never from snapshot.py or lerobot.py. Extras ignored, nothing rewritten, nothing re-converted. Holds.

One coordination note for the snapshot verifier (#428): it will consume these same types, so its reasons need to live beside REASON_MISSING and friends rather than inventing a second vocabulary, and once two verifiers share this module it may be worth separating the shared types from the per-delivery verifiers. Neither blocks this PR.

[VARUN3WARE]

Thanks @Sagar-024 , agreed on both notes for #428.

I'll keep snapshot reason strings beside the existing REASON_* constants in verification.py (same vocabulary, no second set of names). Happy to split shared types from the per-delivery verifiers in a follow-up once both verifiers are in-tree; not blocking this PR.

CI is green here; ready for maintainer merge whenever @kstonekuan has a slot.

[kstonekuan]

The contract is right and I want to say that first, because the thing I am about to raise is not about the shape. VerificationStatus with ok/damaged/unverifiable, exit_code_for mapping 0/1/3 with 2 raised before a report exists, findings carrying uri plus a reason code, unlisted files ignored, bool rejected on size_bytes, parsing the manifest at the boundary and raising rather than returning a report when the input is unreadable. That is the contract you and @Sagar-024 agreed on #432, implemented as agreed. Gate clean, 1616 passed.

The problem is path resolution, and it defeats the use case the issue was filed for.

_findings_for_episode_receipt resolves each episode with fetch_uri(uri), and uri is what storage.uri_for recorded at publish time, which is absolute: a resolved filesystem path for a local root, a full URL for a bucket. So verify_lerobot_import(data_root) reads the manifest from data_root and then verifies whatever the receipt's absolute path points at, which need not be inside data_root at all.

I built a delivery, copied it the way a recipient would, and asked:

original verify           -> ok
copy, original present    -> ok  findings=0
copy damaged, original ok  -> ok  findings=[]
copy alone, original gone  -> damaged  findings=['missing']

Read the third line carefully. I replaced the copy's episode with damaged bytes of the same length, and verify reported the copy clean, because it hashed the original. A verifier that certifies a damaged delivery as intact is worse than no verifier, because someone will trust it.

The fourth line is the ordinary case: a recipient on another machine, where the publishing path does not exist. An intact delivery reports every episode missing.

So the two outcomes are a false pass and a false failure, and #432 opened with exactly this: "a recipient of the delivery (landing MCAPs + prepared-manifest.json) cannot ask whether that delivery is still intact." As written, data_root is used to find the manifest and then ignored.

Your own test suite cannot see this, and it is worth understanding why: every case builds the delivery and verifies it in place, so the recorded absolute path and the root under verification are always the same directory. The bug lives exactly in the gap between them.

The fix

Resolve every episode relative to the root being verified, and treat the receipt's uri as provenance rather than as a lookup key: it records where the delivery was published, which is worth keeping in findings, and is not where it now lives.

The basename gets you there without a schema change: the importer owns the landing layout (_landing_relative_key is landing/lerobot_episode_{index + 1:04d}.mcap), so landing/<basename of uri> under the verified root is well defined for both local and bucket roots, and storage.exists / storage.fetch already take that shape. Say in the PR that you are relying on that layout, since it means the importer and the verifier now share a convention that is not written down in the manifest.

If you would rather record the relative key in the receipt, that is a schema-4 change and #432 puts it out of scope, so raise it separately rather than folding it in here.

Definition of done for the next round

1. A test that copies a delivery to a second root and verifies the copy, with the original still present and intact. It must read the copy.
2. A test that damages only the copy. That is the false pass above and it must go red before your fix.
3. A test that deletes the original entirely and verifies an intact copy. It must report ok.
4. The bucket path gets the same treatment. If you cannot test it without credentials, say what you reasoned instead of leaving it implied.

One smaller thing

episodes: [] currently returns unverifiable, and I am not sure that is right. A manifest with an empty episode list has a receipt in a format we understand and claims nothing, which reads more like ok than like "we could not tell". Exit 3 says a human should go and look, and there is nothing to look at. Say which you meant.

@Sagar-024 this one is worth reading before #428 lands. A snapshot's integrity.tables entries are paths relative to the export root rather than absolute, so you may already be safe by construction, but the trap is the same shape: verify what is under the root you were handed, never what a receipt says about where it used to live. Worth a test that moves the snapshot either way.

[VARUN3WARE]

@kstonekuan Fixed: episodes resolve as landing/<basename> under the verified root; receipt uri is provenance only. Empty episodes → ok. Added your three copy-root cases plus a bucket relative-fetch test. Ready for another look.

[Sagar-024]

Re-reviewed the fix commit (25a69e3). Stale approval replaced, this still holds the #432 contract checklist:

• CLI verb: hflow verify lerobot-import <root> present.
• Report: .ok derived from status, findings in one list, no raise on first mismatch.
• Per-finding uri / reason / detail: present, reasons from the shared constants.
• Shared types: single hflow.verification module, no verifier-local report shapes.
• Exit codes: 0 clean, 1 damaged, 2 unreadable input (raises), 3 unverifiable, via exit_code_for.
• Extras ignored: unlisted files under landing/ produce no finding.

The fix itself, verified in a scratch worktree (test_verification.py: 12 passed):

• Episodes now resolve as landing/<basename> under the root being verified; the receipt uri is provenance, never a lookup path.
• Both directions are pinned by mutation: mutating only the original keeps the copy green, damaging only the copy goes red.
• The deleted-original case and the bucket-root case are covered, plus the empty-claim receipt flipping to clean (the right call: nothing claimed, nothing damaged).
• Publish-time absolute uris still parse after a copy, so no schema bump is needed.

One note, non-blocking: .zcode shows a stray pr454.diff in my scratch from the review run; it is mine, not part of the PR.

Re-approving on the fix commit.

[Sagar-024]

@VARUN3WARE Kingston unblocked the order, so I am landing #428 first with your type shape verbatim: VerificationStatus, VerificationFinding, VerificationReport with the ok property, your three REASON constants and exit_code_for, plus one REASON_NO_RECEIPT beside yours, hyphenated to match your convention. When you rebase #454, drop your copy of verification.py and import from the landed one. Your fix stays in the import-verifier half (the landing resolution and the empty-claim case), so nothing is lost. Ping me after the rebase and I will review again.

[Sagar-024]

@kstonekuan carried into #428: the moved-root test exports a snapshot to one root, copies it to a second, deletes the first entirely, then verifies the copy clean and then damaged, plus a variant where the original stays alive and clean while the copy is damaged, so damage is never masked by the export root. PR #457 is up with it, and the shared types land in VARUN's exact shape so #454 re-lands as an importer.

[VARUN3WARE]

@Sagar-024 Thanks for the re-review. Ack on order: once #457 lands I’ll rebase #454, import the shared types from your verification.py, and keep only the lerobot-import resolution / empty-claim / CLI side. Will ping you after the rebase.

[kstonekuan]

Fix confirmed with the same probe that caught it. Before, the third line read ok; now:

original verify           -> ok
copy, original present    -> ok       findings=0
copy damaged, original ok  -> damaged  findings=['content-id-mismatch']
copy alone, original gone  -> damaged  findings=['content-id-mismatch']

The third line is the one that mattered: damage in the copy is found even with an intact original sitting next to it, so the verifier is reading what it was handed. The fourth is my probe still holding the damaged copy from the step before, so damaged is the right answer there too.

All four cases from the review are in, and test_verify_lerobot_import_resolves_bucket_deliveries_under_the_verified_root goes past what I asked for. Reasoning the bucket case rather than testing it would have been acceptable and testing it is better.

You also changed episodes: [] from unverifiable to ok, which I agree with. A manifest in a format we understand that claims nothing has been verified against everything it claims. Exit 3 should mean "go and look", and there is nothing to look at.

Not approving yet only because main moved underneath you: #457 merged and landed src/hflow/verification.py, so this now has an add/add conflict there plus one in __init__.py. That is the rebase you and @Sagar-024 planned for, in the order you agreed.

When you do it: take the merged module as the base and add to it rather than reconciling two copies. VerificationStatus, VerificationFinding, VerificationReport, exit_code_for and the three shared reason strings are already there, and REASON_NO_RECEIPT arrived with the snapshot verifier. Your verify_lerobot_import and _landing_relative_key_from_receipt_uri are the new parts. If you find yourself editing the shared types to fit, say what does not fit instead, because that is a contract question rather than a merge.

The basename mapping is worth one line in the PR body when you push: it means the verifier and the importer share the landing layout without the manifest recording it. That is fine, and it is the sort of coupling that should be stated rather than discovered.

Ping me when it is clean and I will merge.

[VARUN3WARE]

@kstonekuan Rebased onto main after #457. Dropped our duplicate verification.py types and import yours; LeRobot half lives in importers/lerobot_verify.py (root-relative landing/<basename>, empty claim → ok, copy-root tests). Ready for another look when you have a minute.

[kstonekuan]

Clean rebase. The verifier moved to src/hflow/importers/lerobot_verify.py, the shared types are imported rather than reconciled, and the only edit to verification.py is a docstring now naming both concrete verifiers. That is the right shape: one contract, two implementations, neither owning the other.

Validated on the rebased branch: full gate clean, 1632 passed / 6 skipped. The path fix stays fixed, which I checked with the probe from the first round rather than trusting the rebase.

Merging. Between this and #457 the verify family is real: same report, same reason strings, same exit codes, two receipt formats.

Worth writing down somewhere eventually, though not in this PR: the basename mapping means the verifier and the importer share the landing layout without the manifest recording it. You noted it, and the next person to change _landing_relative_key needs to know a verifier depends on it. If a schema-4 receipt ever happens, carrying the relative key is the thing that removes the coupling.

Good work on both rounds, and on settling the contract with @Sagar-024 in writing before either of you wrote code. That is why these merged in two days instead of arguing in review.