Skip to content

Integrity receipt entries are dict[str, str | int], so four use sites coerce and one of them forgets #489

Description

@kstonekuan

Integrity receipt entries travel as dict[str, str | int] (src/hflow/snapshot.py:129, :138, :170, :180, :885). Every field is a union, so every read coerces, and the coercions disagree with each other:

src/hflow/snapshot.py:153   key=lambda entry: str(entry["path"])       # coerced
src/hflow/snapshot.py:913   relative_path = str(entry["path"])         # coerced again
src/hflow/snapshot.py:928   receipt_size = int(entry["size_bytes"])    # coerced
src/hflow/snapshot.py:942   if delivered_sha256 != entry["sha256"]:    # NOT coerced

:942 compares a str against str | int. That comparison is well-typed and always False for the int branch, so a receipt whose sha256 arrived as a number would silently report a content mismatch rather than refusing a malformed receipt. Nothing constructs that today, because _file_integrity_record writes a hex string, but the type says it is possible and the code at :942 does not act like it.

Three fields, three known types, one union covering all of them. The receipt is parsed from external JSON at :861 and then re-interrogated at every use.

What to do

One frozen dataclass, constructed at the boundary where the marker is read, and used by type everywhere after:

@dataclass(frozen=True)
class FileIntegrityRecord:
    path: str
    size_bytes: int
    sha256: str

Parse it once, next to the content_id gate that #483 just added, and refuse a receipt entry whose fields are the wrong type there rather than coercing at four separate use sites.

The constraint that makes this non-trivial

_inventory_content_id (:138) hashes json.dumps(normalized, sort_keys=True, separators=(",", ":")) over these dicts, and #483 made every verify compare that digest against the stored content_id. The serialized form must not change by one byte, or every snapshot ever exported fails verification.

So the dataclass needs an explicit conversion back to the same dict shape for hashing, and the digest must be computed over that, not over the dataclass. This is the whole risk of the change and the reason it is labelled advanced.

Definition of done

  1. FileIntegrityRecord is constructed once per entry, at the marker-reading boundary, and the per-file loop takes records rather than dicts.
  2. A receipt entry with a non-string path or sha256, or a non-int size_bytes, is refused with a message naming the field, not coerced.
  3. content_id digests are byte-identical. Prove it: export a snapshot on current main, keep the format.json, then verify that same directory with the change applied. It must report OK. A test asserting a known digest over a fixed entry set is the durable version.
  4. The exporter writes the same format.json as before. Compare a full marker before and after.
  5. Mutation: change one field's type in the parse step and confirm a test goes red.

If the digest cannot be preserved

Say so on the issue and stop. Keeping the receipt format stable is worth more than the type cleanup, and a serialization change here would need its own decision about format versioning.

Validation

uv sync --locked --all-extras
uv run ruff check
uv run ruff format --check
uv run ty check
uv run pytest -q

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

advancedNeeds codebase familiarity; not a starter issueenhancementNew feature or requesthelp wantedExtra attention is needed

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions