Skip to content

Fix the rc4 external review: mount points, hard links, commit fsync, content verification - #140

Merged
guijacquemet merged 4 commits into
mainfrom
claude/fix-chatgpt-rc4-review
Nov 3, 2026
Merged

guijacquemet merged 4 commits into
mainfrom
claude/fix-chatgpt-rc4-review

Conversation

@guijacquemet

@guijacquemet guijacquemet commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

This fixes the findings of the external rc4 review. The owner's decisions are final. Do not merge before the independent review.

B1: mount points below the experiment or masks folder are refused

  • Added mounts.MountCheck. LinkGuard.real() now refuses any folder (or file) below the walked root that is a mount point, with the error mounted_folder naming it. A path counts as a mount point when:
    • it is listed in /proc/self/mountinfo (octal escapes undone), or
    • it is a folder whose st_dev differs from the root's.
  • mounted_folder is in LINK_CODES, so discovery, OME-Zarr stores (check_store), the masks folder, check, run, convert and RunInputs all refuse it the same way. run writes no table or manifest.
  • The selected root itself may be on any mount.
  • I tested a real bind mount in this container. os.path.ismount() returns False and st_dev is the same as the parent's (only the inode differs). That is why the mountinfo check is needed.
  • Tests in tests/test_mounts.py:
    • a real mount --bind (skipped by name without root or permission). Here it ran and passed;
    • a replaced mount table, so the checks always run: experiment, masks, a link onto a mount, an OME-Zarr store, check/run/RunInputs, and the st_dev rule;
    • parsing of mountinfo.
  • Remaining limits (documented in contract §1 and mounts.py):
    • where /proc/self/mountinfo cannot be read (macOS, BSD, Windows), only a mount on another device is seen, so a BSD nullfs mount of the same device is missed;
    • macOS bindfs (FUSE) and Windows mounted folders have another device, and a Windows mounted folder is also a mount-point reparse point, which the junction/link rule already handles;
    • Btrfs subvolumes have their own st_dev and are refused too;
    • mounts are read once per walk, so a mount made after the walk is not seen;
    • each folder of the walk now costs one stat, including the folders inside OME-Zarr stores (their files are not stat'ed).

I1: hard-linked images and masks get a warning

  • claim() uses the stat it already takes. A file with st_nlink > 1 is the warning hard_linked_file, naming each file and its count. Reading and fingerprints are unchanged.
  • Warnings do not change exit codes: check still exits 0 (tested).
  • The files inside OME-Zarr stores are not counted.

I2: the output folder is fsynced after the commit

  • _commit() now calls _flush_folder(output) right after the manifest's os.replace. Before, only _finish_committed flushed.
  • The flush a recovering lock holder needs moved to finish_publication, so a normal run flushes once.
  • A test records the order: staged-folder flush → manifest rename → output-folder flush → moves. It fails without the fix.
  • Contract §9.4 now describes the commit flush and the Windows limit (folders cannot be flushed there).

I3: identity rule stated; opt-in --verify-content

  • (a) The contract (§9 row reuse, §9.1 RunInputs, §9.4 resume), the user guide, AGENTS.md, CHANGELOG and docstrings now state the identity rule: the metadata fingerprint (size + mtime_ns; for OME-Zarr, the inventory). They also state its blind spot: a rewrite of the same size with the modification time set back. A test shows the blind spot in the default mode.
  • (b) How strict mode is turned on:
    • RunMode(name, verify_content=True) passed as run_experiment(mode=...);
    • CLI --verify-content / --no-verify-content;
    • the notebook's "Verify each image's content (SHA-256) before reusing its rows" box, filled from the earlier run and shown in the plan.
  • What strict mode does:
    • records files[].content_sha256 (TIFF bytes; OME-Zarr: per-file SHA-256s over the files the inventory covers) and settings.verify_content;
    • adds the content SHA-256 to RowKey and the staging key, so earlier and staged rows are reused only for the same content;
    • checks that an image read still has its SHA-256 after the read (else source_changed_during_run);
    • reports an image that cannot be hashed as unreadable_planes;
    • makes RunInputs.open of such a run check every image's content once (it already checked fingerprints).
  • Changing the mode between runs:
    • verify_content is a recorded input: a run that does not give it keeps the recorded choice;
    • turning it on or off recomputes every row, with the reason "content verification (verify_content) was turned on/off";
    • it appears as the input change verify_content;
    • staged rows of the other mode are discarded with that reason.
  • The default mode's behaviour and speed are unchanged. In the default mode the staging key is unchanged too.
  • Deviation: the flag rides on mode= (RunMode) instead of a new parameter, because run_experiment is at its shrink-only baseline of 15 parameters.
  • To keep manifest.py under 600 lines, package_versions moved to versions.py (still re-exported from manifest).
  • New modules: source_content.py, versions.py, mounts.py, plus inventory.store_files. All have declared layers.
  • Strict-mode limits:
    • masks keep the fingerprint rule;
    • a change with metadata kept between the header read and the hashing, or after a reused image was hashed, is not seen;
    • cost: one extra whole read of each image per run, one more for each image read, and one per RunInputs.open.

Also

Opened #141: "Conversion reopens TIFF headers for pixel reads and memory checks (performance follow-up of #96)".

Gate (Python 3.13, .[dev,notebook,zarr])

  • ruff check .: pass. ruff check --ignore-noqa . reports the same diagnostics on main and on this branch: no new suppressed violations.
  • ruff format --check .: pass.
  • python tools/check.py --rules: pass. The baseline is unchanged; tools/code_rules.py SHA starts with cc04a420.
  • pytest: 2090 passed, 4 skipped, 0 failed. The skips are three case-sensitive file-system tests and one permissions-as-root test. The real bind-mount tests ran (not skipped).

🤖 Generated with Claude Code

https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc

…t hard-linked files (B1, I1)

A Linux bind mount below the experiment is an ordinary folder to the walk, so
discovery read files from outside the selected folder. LinkGuard now refuses
any folder (or file) below the walked root that is a mount point, with the
error mounted_folder naming it: a path listed in /proc/self/mountinfo (the only
way to see a same-device bind mount; os.path.ismount and st_dev report an
ordinary folder), or a folder on another device than the root. The rule holds
inside OME-Zarr stores, the masks folder, check, run, convert and RunInputs
(mounted_folder is one of LINK_CODES). The root itself may be on any mount.

An image or mask whose link count is above 1 is the warning hard_linked_file
(from the stat claim() already takes); reading and fingerprints are unchanged.

Tests: a real bind mount (skipped by name without mount --bind as root) and a
replaced mount table, which always runs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
_commit() replaced the manifest without an fsync of the output folder; the
flush came only from _finish_committed. The folder is now flushed in _commit,
right after os.replace, so the commit is on the disk when it returns. The flush
a recovering holder of the lock needs moved to finish_publication. A test
records the order of renames and folder flushes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
…fication (I3)

(a) The contract, user guide, AGENTS.md, CHANGELOG and docstrings (manifest,
fingerprint, inventory, run_experiment, RunInputs) now name the identity
rule for reuse, resume and RunInputs: the metadata fingerprint (size and
mtime_ns; for OME-Zarr the inventory). They also name its known blind spot,
a same-size rewrite with the modification time set back.

(b) Opt-in strict reuse: RunMode(name, verify_content=True) for
run_experiment(mode=...), CLI --verify-content / --no-verify-content, and
the notebook's "Verify each image's content" box. Each image's content
SHA-256 (TIFF bytes; OME-Zarr: per-file SHA-256s over the files its
inventory covers) is recorded in files[].content_sha256 and joins the row
key, so earlier and staged rows are reused only for the same content. An
image read must keep it after the read (else source_changed_during_run).
RunInputs.open of such a run checks it once. settings.verify_content is a
recorded input: turning it on or off recomputes every row, is reported as
an input change, and discards staged rows of the other mode with the reason.
The default run is unchanged.

RunMode carries the flag through mode=, because run_experiment's parameter
count is at its shrink-only baseline. package_versions moved to versions.py
(re-exported) to keep manifest.py under 600 lines. New code: mounts-free
source_content.py and inventory.store_files.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
@guijacquemet
guijacquemet marked this pull request as ready for review November 3, 2026 12:00
…ape (review of #140)

- mounts.MountCheck read st_dev from os.DirEntry.stat(), which is 0 on Windows,
  so every folder of every experiment was refused there (637 failures on the
  Windows CI jobs). It now uses os.stat(entry.path); regression test with a
  Windows-like entry.
- settings.verify_content is written only when true, so a default run's
  manifest keeps the v0.4.0rc4 keys (contract and changelog say so).
- Tests for a bind-mounted file inside an OME-Zarr store and for a strict-run
  image that cannot be hashed (never read), which mutations showed untested.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
@guijacquemet
guijacquemet merged commit 544fa45 into main Nov 3, 2026
7 checks passed
@guijacquemet
guijacquemet deleted the claude/fix-chatgpt-rc4-review branch November 3, 2026 12:22

Copy link
Copy Markdown
Contributor Author

Independent review, merged as 544fa45 (merge commit). Fixes are in 4d0726c.

Blocking, fixed: MountCheck.is_mount read st_dev from os.DirEntry.stat(). On Windows that call always reports 0, so every folder was refused as mounted_folder (both Windows jobs failed with 637 failures at 14aa0c8). It now uses os.stat(entry.path), and a regression test covers it.

Minor, fixed: settings.verify_content was written as false in every manifest. It is now written only when true, so a default run's manifest keeps the rc4 keys. A run written by rc4 opens with RunInputs and reruns with nothing read and no change listed; I checked this against a run produced by e24f334. I also added tests for a bind-mounted file inside an OME-Zarr store and for a strict-run image that cannot be hashed. Mutation runs showed neither case was tested.

I checked these and found no defect:

  • The B1 rule covers discovery, the masks walk, check_store, convert and RunInputs. The root is exempt.
  • Real bind mounts behave correctly: the root under a bind mount, the root as a mount point, \040 spaces in paths, and a file bind mount.
  • I3 strict reuse holds: reuse, resume, the after-read check, the recorded mode and RunInputs gating.
  • I2 ordering is correct. Neither rc4 nor this PR flushes after the final moves, so there is no regression.

Gate:

  • ruff check and ruff format --check are clean. ruff --ignore-noqa shows the same 66 findings as base.
  • tools/check.py --rules passes, and code_rules.py is unchanged (cc04a420).
  • Local pytest: 2093 passed, 4 skipped. The real bind-mount tests ran as root.
  • CI: all 7 jobs are green.

Generated by Claude Code

@guijacquemet
guijacquemet restored the claude/fix-chatgpt-rc4-review branch November 3, 2026 12:22
guijacquemet pushed a commit that referenced this pull request Nov 3, 2026
Q66 confirmed by the owner (3 November 2026); Q73's follow-up decided: neither a
stated reshape nor a reorder in v0.4, kept in issue #144; Q74 records the
tie-break and the consumers checked. The changelog gains a section for rc5 (PRs
#139, #140, #142, #143) with migration notes: mounts refused, position_id
required in v2, micrometre conversion within 1 ULP, --verify-content, and the
tie-break. The migration guide gets the same.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
@guijacquemet
guijacquemet deleted the claude/fix-chatgpt-rc4-review branch November 14, 2026 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants