Skip to content

Links stay inside the experiment and masks folders; --log is checked before it opens (#81, #99) - #128

Merged
guijacquemet merged 11 commits into
mainfrom
claude/fix-symlinks-log
Nov 2, 2026
Merged

guijacquemet merged 11 commits into
mainfrom
claude/fix-symlinks-log

Conversation

@guijacquemet

@guijacquemet guijacquemet commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #81, fixes #99

Before-audit fixes from the 2 November triage (design v0.4 §1 "sources are never touched", §2, §9).

Summary

#81: links stay inside the folder walked (owner policy: contain)

  • links.py (new, io): LinkGuard follows a link (file or folder) only when its real path (os.path.realpath) is inside the real path of the folder walked. Three errors:
    • link_outside_folder: the link leads outside. It names each link and its target (ctrl/rep1/b.tif -> /data/other/far.tif), broken links included. The link is not followed.
    • link_loop: the link leads to a folder on its own walk path. It is not followed.
    • linked_twice: one file is reached by two paths: a link and its target, two links, a linked folder and its target, or two hard links (the same device and inode, from the one stat taken per image or mask). Both paths are named.
  • What counts as a link: whatever os.DirEntry.is_symlink() reports, plus, on Windows, directory junctions (mount-point reparse tag).
  • One helper serves every walk:
    • discovery._Walker.visit: images and folders, plus the links inside each OME-Zarr store (check_store);
    • masks/files._folder_masks: replaces rglob;
    • unrecorded.unrecorded_files: raises LinkRefusedError, which RunInputs.open turns into a RunInputsError;
    • convert: it plans from discovery.
  • Hidden and system names are skipped before any link is looked at, so a hidden link is never followed. A contained link to a hidden folder is followed: the hidden folder itself is never walked, so its files are found once, under the name of the link the user made.
  • A broken link that stays inside is kept and reported by its reader (unreadable_tiff, unreadable_mask), never skipped.

#99: --log is checked before it is opened

  • log_target.py (new, io) refuses a log:
    • inside the experiment or masks folder, given or recorded by the earlier run in the output folder;
    • inside convert's --to;
    • as, or inside, a protected output: plane_histograms.csv, run_manifest.json, the report files, experiment_design.csv, calibration.csv, mask_relationships.json, the lock, .morphobricks-conversion.json, or the .run- staging folders;
    • as the design file given with run --design (an input of the run; review);
    • as a folder, or in a folder that does not exist;
    • as an existing file that has a second name (a hard link).
  • Paths are compared resolved. Folders are also compared by device and inode, so another case on macOS or Windows is caught. Output names are compared ignoring case.
  • cli.main checks --mask-axes and the recorded experiment first, then the log path, and only then opens the log. An invalid command writes nothing. This applies to every command with --log: run, masks and convert (check, design and calibrate have none).

Owner decisions (2 November 2026)

Review (independent reviewers)

First review: f4381fb, 65e963c (merge of main: #126, #127, #129), 9b372da.

  • Owner decision implemented: hard links → linked_twice, both paths named. Detected in the experiment folder (discovery and run) and in the masks folder. 2 regression tests, both failing on fbe6829.
  • Finding (fixed): a broken link in the masks folder was skipped, so its image only showed mask_missing. It is now unreadable_mask, naming the link. Regression test fails on fbe6829.
  • Finding (fixed): --log compared resolved paths only (case on macOS and Windows; a hard link to a source outside every folder). Regression tests: the hard-link and output-case tests fail on fbe6829; the folder-case test runs only where the file system ignores case (macOS and Windows CI) and is skipped on Linux.
  • Contract §8 corrected for OME-Zarr axes and calibration: nothing guessed, nothing silent (#72, #73, #94) #129's 0.1/0.2 refusal (9b372da).

Second review: 610aba6, 2aebb97 (merge of main: #130), 795faab.

  • Finding (fixed, 610aba6): run --design my_design.csv --log my_design.csv, with the design outside the output folder, appended the log to the confirmed design and the run went on (exit 0, 207 → 315 bytes). The design file given is now refused (resolved, or the same device and inode). Regression test fails before the fix. Contract §8, user guide §5 and plan question 65 updated.
  • §9 (fixed, 795faab): the rewritten _Walker.visit (36 lines) had no docstring; __init__'s # noqa: ANN204 replaced by its annotation.
  • Verified by probes (discovery, masks, RunInputs.open, convert, masks command): containment by real path for files and folders; links inside OME-Zarr stores (outside, loop to the store, a link up to an ancestor folder); hard links across leaves; broken links; hidden links and links to hidden folders; the root named through a link; the log checked before it opens and before any write for run, masks and convert; valid logs still written. No other walk of the experiment or masks tree (grep for rglob, os.walk, iterdir, glob, scandir, listdir).
  • Not changed: a link inside the experiment to an output folder placed inside it is followed, but that layout is already the error output_inside_experiment.
  • Windows junctions: detected in code; not exercised by a test (no junction can be made on Linux CI).

Store scan cost (measurement only, not a performance claim)

discover_experiment, best of 7, on 500 TIFFs plus two stores of 10,000 files each (first review, this container): main (7f55ba1) 34 to 37 ms, this branch 50 to 57 ms, about 20 ms per 20,000 store files plus one stat per image; the implementer measured 84 ms on its machine. Results are identical. The owner accepted this cost.

Tests and gate

  • tests/test_links.py: 20 tests. tests/test_cli_log.py: 30 cases.
  • Full gate on 795faab (uv venv, Python 3.11, .[dev,notebook,zarr], root): ruff check clean; ruff format: 187 files already formatted; pytest 1901 passed, 2 skipped (test_discovery.py:665, permissions ignored as root; test_cli_log.py:164, a file system that tells case apart, runs on macOS and Windows); tools/check.py --rules passed.

Code standards (§9), measured

  • Largest file touched: discovery.py, 609 lines (baselined at 610 on main; 609 here). New links.py 247, log_target.py 182.
  • Largest function: _folder_masks, 39 executable lines. Then LinkGuard.issues 37, visit 36 and cli.main 32.
  • Highest complexity: visit 12, main 11, _folder_masks 10, log_refusal 8.
  • Parameters: at most 5 (run_log_refusal).
  • # noqa baseline: ANN 53 → 52; nothing grows; no new exemption.

https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc

🤖 Generated with Claude Code

Owner policy (2 November 2026): contain. One helper, links.LinkGuard, walks the
experiment (discovery, OME-Zarr stores included) and the masks folder: a link
whose real path leaves the folder is not followed (link_outside_folder, naming
each link and its target), a link back to a folder above it is link_loop, and
two paths of one file are linked_twice, naming both. The unrecorded-files
check of RunInputs raises on the same links; conversion plans from discovery,
so it refuses them before anything is copied.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
The log is refused inside the experiment or masks folder (given or recorded
by the earlier run), inside convert's folder of copies, and as or inside an
output artefact of the run (table, manifest, reports, design, calibration,
mask_relationships.json, the lock, the conversion record, staging folders);
any other name in the output folder is fine. A folder, or a file in a missing
folder, is refused too. The log is opened only once the arguments are checked,
so an invalid command writes nothing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
…n Q60-61

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
A store's files are only listed; folders and links are followed up. On 20,000
store files plus 500 TIFFs discovery took 312 ms with the first version and
84 ms now (main: 28 ms, without the check). Adds a test of a link inside a store
back to the store (convert's copy would never end).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
…ed through a link (#81)

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 2, 2026 16:30
…log check

Owner decision (2 November 2026): one device and inode reached by two names
is linked_twice, naming both. LinkGuard.claim keys each image or mask by the
device and inode of the one stat it takes (its real path when it has none).

Review findings:
- a broken link in the masks folder was skipped, so its image only looked
  as if it had no mask (mask_missing); it is now kept and named by its
  reader (unreadable_mask), as discovery does for images;
- --log compared resolved paths only: on macOS and Windows another case of
  the experiment folder or of run_manifest.json got past the check, and a
  hard link to a source image outside every folder was appended to. Folders
  are now also compared by device and inode, output names ignoring case, and
  an existing file with a second name is refused.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
guijacquemet pushed a commit that referenced this pull request Nov 2, 2026
No conflicts; plan questions on main end at 59, so 62-64 are renumbered once #128
and #129 land.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
docs/plan.md: main's PP-11b and PP-11c items first, then this PR's links
item; this PR's questions 60 and 61 become 61 and 62, after main's question
60 (OME-Zarr axes and calibration, #129). docs/contract.md: both additions to
the error row kept (main's OME-Zarr codes, this PR's link codes).

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

The error row still said a store without axes is refused when its version
does not fix them; since the owner's decision in #129 no version fixes them.

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

The confirmed design given with --design outside the output folder was
appended to by the log, and the run went on. It is an input of the run, so
the log is refused there (resolved, or the same device and inode), like the
sources. Regression test fails before the fix.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
Plan §12: main's questions 61-63 (#130) kept; this PR's link and log
questions renumbered 64 and 65 (owner, 2 November 2026), and the plan's
done-list entry with them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
guijacquemet pushed a commit that referenced this pull request Nov 2, 2026
Conflicts: plan questions (main has 60 to 63; this branch's questions
become 66 and 67, 64 and 65 being taken by #128) and the code-rule
layers of pyproject.toml (size_checks and ngff_scale, both pure).

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

visit was rewritten by the links fix (36 lines) without a docstring; the
noqa ANN204 on __init__ is replaced by its return annotation (noqa ANN
baseline 53 -> 52). discovery.py stays at 609 lines.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
@guijacquemet
guijacquemet merged commit 950f642 into main Nov 2, 2026
7 checks passed
@guijacquemet
guijacquemet deleted the claude/fix-symlinks-log branch November 2, 2026 20:34
@guijacquemet
guijacquemet restored the claude/fix-symlinks-log branch November 2, 2026 20:35

Copy link
Copy Markdown
Contributor Author

Independent review (second pass, after the first reviewer was cut off): merged as 950f642 (merge commit; parents feec007 and 795faab). The branch was deleted automatically and has been restored at 795faab.

Findings, fixed on the branch

  • 610aba6: run --design FILE --log FILE, with the design outside the output folder, appended the log to the confirmed design and the run went on (exit 0; the file grew from 207 to 315 bytes). The design file given is now refused, compared resolved or by device and inode. Regression test test_a_log_over_the_design_given_leaves_it_unchanged fails before the fix. Contract §8, user guide §5 and plan question 65 are updated.
  • 795faab (§9): the rewritten _Walker.visit (36 lines) had no docstring, so I added one. __init__'s # noqa: ANN204 is replaced by a return annotation, so the noqa ANN baseline goes from 53 to 52.
  • 2aebb97: merge of main (Experiment design identity and counts: excluded folders, duplicate columns, output folder (#82, #83, #98) #130). Plan §12 keeps main's questions 60 to 63. This PR's questions are renumbered 64 (links) and 65 (--log), and the plan's done-list entry with them. No other file cites them.

Verified (probes and code)

  • Each "Fixes" claim checked in the code:
  • Links inside OME-Zarr stores behave as follows:
    • a link that leads outside is link_outside_folder;
    • a link to the store itself, or to a folder above it, is link_loop.
  • Other link cases behave as follows:
    • hard links across leaves are linked_twice, naming both;
    • broken links are reported, never skipped;
    • hidden links are skipped before they are looked at;
    • a link to a hidden folder is found once;
    • an experiment folder named through a link works.
  • Not changed: a link to an output folder placed inside the experiment is followed. That layout is already the error output_inside_experiment.
  • Windows junctions are detected in code but not tested.

Gate on 795faab (own worktree, uv venv, Python 3.11, .[dev,notebook,zarr], root)

  • ruff check: clean.
  • ruff format: 187 files already formatted.
  • pytest: 1901 passed, 2 skipped. The skips are test_discovery.py:665, where file permissions are ignored as root, and test_cli_log.py:164, which runs only on a case-insensitive file system.
  • tools/check.py --rules: passed.
  • CI: all 7 checks green (lint; ubuntu, macOS and Windows on Python 3.11 and 3.13).

§9 figures

  • Largest file touched: discovery.py, 609 lines. It was baselined at 610 on main. New files: links.py 247 lines, log_target.py 182.
  • Largest function: _folder_masks, 39 executable lines.
  • Highest complexity: visit, 12. Next are main 11, _folder_masks 10 and log_refusal 8.
  • Most parameters: 5 (run_log_refusal).
  • # noqa and the baseline only shrink (ANN 53 → 52). No new exemption.

Store scan cost (first review's measurement, accepted by the owner): about 20 ms per 20,000 store files on this container, against the implementer's 84 ms. Results are identical.


Generated by Claude Code

guijacquemet pushed a commit that referenced this pull request Nov 2, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
guijacquemet pushed a commit that referenced this pull request Nov 2, 2026
Conflicts: contract §8 (main's link errors kept; copy_changed_after_conversion
moved to the errors, source_changed_during_run added), plan §10 (main's
entry kept, PP-12's after it) and §12 (main's questions 64 and 65 kept,
PP-12's question renumbered 66, its references updated), run/publish.py
(PP-12's: the .run- staging folders are gone). The --log check (#99) now
protects .publish and .staging instead of the .run- folders.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
guijacquemet pushed a commit that referenced this pull request Nov 2, 2026
Plan questions 64 and 65 are now #128's; this PR's become 66 and 67.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc
@guijacquemet
guijacquemet deleted the claude/fix-symlinks-log 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

2 participants