[guijacquemet]

Fixes the five closures an independent review of 8db1f4a found premature (issues reopened by the owner). Binding spec: design v0.4 §1, §5, §6, §9; contract §17; plan question 59.

Fixes #80, #87, #88, #92, #96

(#96: the correctness part, mixed metadata epochs, is fixed. See "#96: header reads that remain" below for the performance part. If the owner wants that part kept open, change Fixes #96 to Refs #96 before merging.)

Summary (by module)

• Move only, first commit (f8cb39a): conversion.py (587/600 lines) split into conversion_plan (plan_conversion, PlannedCopy; orchestration), conversion_reuse (compare with an earlier copy tree; io) and conversion_write (.partial writing; io). Helpers used across modules lose their leading underscore. Tests now patch write_named_copy/os.replace in conversion_write.

• [Sad Catapella B6][Performance] TIFF-to-Zarr conversion reopens headers to read physical calibration already inspected #96, one metadata epoch: preflight(bind_headers=True) takes each image's fingerprint before and after reading its header (Preflight.header_fingerprints). plan_conversion refuses any image whose two fingerprints differ or that went missing (source_changed_during_check, an error naming the files, nothing written). That bound fingerprint is recorded as the source's and checked before and after the copy is written, as before. The copy's pixel type now comes from the same header: write_named_copy(..., dtype=) no longer calls plane_layout to reopen the header. A plane whose type differs from the header's is refused (CopyWriteError, so conversion_failed). It is never cast.

• [Sad Catapella B5][Scientific integrity] Reused TIFF-converted Zarr copies are not validated against their stored pixels #87, the copies' SHA-256 (owner decision): new content_hash.content_sha256. A file is hashed as is. An OME-Zarr folder is hashed from its files' relative paths and SHA-256s in path order, with labels left out (as for fingerprints). Each copy is hashed in one pass after it is written and moved into place, and the hash is recorded as copy_sha256. Three places check it:

  • reuse (conversion_reuse): needs the plan, the fingerprint and the SHA-256 to match;
  • review_conversion (check/run on a copy tree): copy_changed_after_conversion also covers a same-size change with the time put back;
  • RunInputs.open (new conversion_check): checks the record SHA-256, the record version, and the fingerprint and SHA-256 of each copy the run recorded.

  A copy that cannot be read counts as changed. Sources keep size+mtime (design §1).

• [Sad Catapella B5][Correctness] Channel-label copy freshness check accepts missing or truncated OMERO metadata #88: CopyEntry.channel_names and significant_bits hold what the copy contains: as written for converted images, from the header for copied ones. same_plan() compares both.

• [Sad Catapella B6][Scientific calibration] Reused OME-Zarr copies are not checked for numeric pixel-scale correctness #92: same_plan() compares unknown_sizes in full (roles and reasons). The copy hash also covers the calibration written into the copy.

• [Sad Catapella B3][Validation] ConversionSettings accepts noninteger numeric inputs that fail downstream #80: cli_run.parse_size is exact (Fraction, ASCII digits only). The result must be a whole number of bytes above 0: 1.5GiB gives 1610612736, while 1.5, 0.5B, 0.5 and 1.0000000001KiB are refused. Previously 9007199254740993 lost precision through float. --memory-limit is only on run and convert (check and masks don't take it). argparse calls parse_size while reading the command line, so a bad size exits with 2 before any preflight or I/O. A test proves this for both commands.

Decisions

• Conversion record format change: record_version goes from "1" to "2". The new per-entry fields are copy_sha256, channel_names and significant_bits, and an entry without copy/copy_sha256 is invalid. A version 1 record is refused everywhere it is read (convert, review_conversion → conversion_record_unreadable, RunInputs.open), and the message says to convert the originals again into a new folder. It is never silently accepted. This is documented in contract §17 and the user guide (copies made by v0.4.0rc3 or earlier).
• What RunInputs costs: it hashes once, at open, the copies of every image in manifest.files, which is one full read of those copies. Excluded images' copies are not read, since RunInputs never reads them. I rejected hashing on every read because it would read a copy whole once per plane. Reads after open keep the per-read fingerprint check. Measured on this machine: 256 MiB hashed in 0.38 s with a warm cache, so a cold read is bounded by disk speed. check/run on a copy tree pay the same one extra read (in review_conversion). This is documented in contract §9.1 and §17 and the user guide.
• New error code source_changed_during_check. I did not reuse conversion_source_changed, which is the run's warning about sources changed after the conversion.
• Recorded in plan §12 question 59 (16) (copy SHA-256), (17) (epoch binding) and (18) (exact sizes), and in plan §10 (PP-11c).

#96: header reads that remain

• Removed: write_named_copy's plane_layout(source, …), which reopened the header only to get the pixel type.
• Still read: the bound inspection in preflight (once per image). The calibration and layout come from it, through copy_layout, which is pure and does not read the file.
• Still read: check_planes at plan time, once per image to convert. This is the run's own memory and block check in planes.py. Its block layout needs the TIFF page and strip structure, which TiffMetadata does not hold, and planes.py is far over its size baseline. If the file changed, the write-time fingerprint check refuses it.
• Still read: the reader (iter_planes) opens the file and parses its header to read the pixels. This is unavoidable, and it happens between the two checks against the bound fingerprint.
• Still read: the CLI convert runs one preflight of its own (for --axes checking) before convert_experiment's bound preflight.
• No benchmark of open counts or elapsed time at 100/1k/10k files has been run, so this PR makes no performance claim.

§9 figures (lines, origin/main → this branch)

conversion 587 → 289; conversion_plan new 185; conversion_reuse new 76; conversion_write new 141; content_hash new 87; conversion_check new 67; conversion_record 386 → 463; conversion_issues 136 → 148; run_inputs 575 → 575; ome_tiff_copy 152 → 158; cli_run 452 → 464; run/preflight 370 → 399. tools/code_rules_baseline.json is unchanged and the # noqa count is unchanged (no noqa lines in the diff). The new modules are declared in pyproject.toml layers.

Notes for review

• Tests that fail on 8db1f4a: 23 of the 28 new tests in tests/test_convert_integrity.py fail when run against main's source (checked with PYTHONPATH=<main>/src). The 5 that pass on main are the valid-size parse cases that main also handled and two refusals that main already refused. Failures on main, by issue:
  • [Sad Catapella B6][Performance] TIFF-to-Zarr conversion reopens headers to read physical calibration already inspected #96: the conversion is written although the source changed after its header was read; the writer reads the header again; the dtype keyword is missing.
  • [Sad Catapella B5][Scientific integrity] Reused TIFF-converted Zarr copies are not validated against their stored pixels #87: record_version is "1"; a same-size, mtime-restored copy change is reused; it is not warned about by the run; RunInputs.open does not raise; an OME-Zarr chunk changed in place is reused; there is no version 1 refusal.
  • [Sad Catapella B5][Correctness] Channel-label copy freshness check accepts missing or truncated OMERO metadata #88: channel names and bits are not recorded or compared (4 cases).
  • [Sad Catapella B6][Scientific calibration] Reused OME-Zarr copies are not checked for numeric pixel-scale correctness #92: different unknown-size reasons are reused.
  • [Sad Catapella B3][Validation] ConversionSettings accepts noninteger numeric inputs that fail downstream #80: 1.5, 0.5B, 0.5, 1.0000000001KiB and fullwidth １ are accepted; 2**53+1 is rounded; the commands run preflight with --memory-limit 1.5.
• The epoch test monkeypatches metadata.inspect_tiff to rewrite the source right after its header is read.
• PP-11b (claude/pp-11b-convert-card) has not merged yet. The public convert_experiment signature is unchanged.
• Not verified: Windows and macOS locally (CI covers them). On the first run the macOS jobs were not picked up by a runner (capacity), not a test failure.

Independent review (418bbbd)

• Folder links in an OME-Zarr copy: os.walk does not enter a link to a folder, so the files behind one were left out of the copy's SHA-256 (a change there kept the hash). content_sha256 now raises OSError for such a folder, and the copy counts as changed. The conversion itself writes no links.
• copy_changed_after_conversion: the run continues with this warning, but RunInputs now refuses a run on a changed or added copy. The warning no longer suggests analysing such a copy: it says the bricks will refuse it and to convert again (contract §17, user guide).
• Two regression tests, both failing on 792a6c1. Full gate: 1780 passed, 1 skipped (test_discovery.py:665, root).

Tests

ruff check ., ruff format --check ., python tools/check.py --rules: pass. pytest with .[dev,notebook,zarr]: 1778 passed, 1 skipped (at 792a6c1; 1780 after the review) (the skip: tests/test_discovery.py:665, running as root ignores file modes, expected), 1 warning (the existing reading array from closed file in test_planes).

https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc

---

Generated by Claude Code

[guijacquemet]

Independent review: merged as 6ac81ad, a merge commit with head 418bbbd. The branch was auto-deleted and has been restored on 418bbbd.

What I checked in the code, with my own scripts as well as the tests

• [Sad Catapella B6][Performance] TIFF-to-Zarr conversion reopens headers to read physical calibration already inspected #96 race. I wrote a script that rewrites the source right after its header is read (new pixels, PhysicalSizeZ 2.0 → 5.0).
  • On main 8db1f4a the copy is written with the new pixels and the old header's Z size of 2.0, so the copy mixes two versions of the file.
  • On the branch it is refused with source_changed_during_check and nothing is written.
  • A rewrite after planning is refused on both, as conversion_failed at the write-time check.
  • The fingerprint taken with the header is the one recorded as the source's, and it is checked before and after the copy is written.
  • The pixel type comes from that header. A big-endian >u2 source, read whole and read in blocks, converts bit for bit. A plane of another type is refused, never cast.
• [Sad Catapella B5][Scientific integrity] Reused TIFF-converted Zarr copies are not validated against their stored pixels #87 copy SHA-256.
  • The hash is taken from the copy on disk after it is moved into place, not from a re-encode.
  • It is checked at reuse, in review_conversion (check/run) and in RunInputs.open.
  • RunInputs.open checks every image in manifest.files. Excluded images are not in files, and _checked() only resolves those entries, so RunInputs never reads an excluded copy.
  • labels is left out of the OME-Zarr hash on purpose. Label images are masks, each with its own fingerprint (contract §15, the same as source_fingerprint), and a brick may add one without changing the image.
• Record version 1 is refused by convert, by run (conversion_record_unreadable, checked with a script) and by RunInputs, each with the message to convert again.
• [Sad Catapella B5][Correctness] Channel-label copy freshness check accepts missing or truncated OMERO metadata #88 / [Sad Catapella B6][Scientific calibration] Reused OME-Zarr copies are not checked for numeric pixel-scale correctness #92. same_plan compares the channel names, the significant bits and unknown_sizes including the reasons.
• [Sad Catapella B3][Validation] ConversionSettings accepts noninteger numeric inputs that fail downstream #80. parse_size uses Fraction with ASCII digits only. It is the argparse type= of --memory-limit, which only run and convert take, so a bad size exits with 2 before any preflight.

Findings, fixed in 418bbbd (both regression tests fail on 792a6c1)

1. An OME-Zarr copy containing a link to a folder was hashed without the files behind the link, because os.walk does not follow it. I reproduced this: the hash stayed the same after a change behind the link. Such a copy now raises OSError and counts as changed.
2. copy_changed_after_conversion is a warning and the run goes on, but RunInputs now refuses that run. I reproduced this with an added copy. The warning still said "check that these images are the ones you want analysed". It now says the bricks will refuse the run and to convert again. Contract §17 and the user guide are updated.

Gate (own worktree, Python 3.12, .[dev,notebook,zarr]): ruff check and ruff format are clean, and tools/check.py --rules passes. pytest gives 1780 passed, 1 skipped (test_discovery.py:665, running as root) and 1 warning (an existing one). CI is green on all 7 jobs.

§9 figures (files touched in src)

• Largest file: run_inputs.py, 575 lines.
• Largest function: run_command, 51 executable lines (unchanged).
• Highest complexity: run_command 10; the highest new one is write_all at 8.
• Most parameters: convert_experiment 7 (unchanged); the most in a new or changed function is preflight at 6.
• Most statements: 30.
• The baseline is unchanged and the # noqa count in src is 49 on both main and the branch. Layers are declared for content_hash and conversion_check (io).
• The split commit f8cb39a only moves code (checked with --color-moved).

Issues: only #80 closed automatically, because GitHub links just the first number after "Fixes". I am closing #87, #88, #92 and #96 by hand, naming this merge.

---

Generated by Claude Code