diff --git a/AGENTS.md b/AGENTS.md index dde83760..a26ef1b4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -64,6 +64,8 @@ src/morphobricks_preprocessing/ discovery.py folder walk, leaf folders, labels (condition / biological_repeat / technical_repeat), position_id per image and the duplicate check; FOV_KEY, check_identity_text, check_plane_index, check_identity_case (the shared identity rules, contract §16) + links.py LinkGuard: links followed only inside the folder walked (experiment, OME-Zarr + stores, masks folder); link_outside_folder, link_loop, linked_twice (#81) metadata.py TIFF header inspection (tifffile, no pixel reads), dtype/dimension/series checks, OME channel names + significant bits, physical sizes (TiffMetadata.physical_sizes); header consistency warnings (width/height, plane counts, pixel sizes); @@ -138,7 +140,7 @@ src/morphobricks_preprocessing/ manifest SHA-256, what was found, reused, recomputed and replaced cli.py, __main__ `morphobricks-preprocessing check|run|masks|calibrate|design|convert` (also `python -m morphobricks_preprocessing`); cli_run.py holds `run`'s options and output; cli_calibrate.py `calibrate`; - cli_convert.py `convert` + cli_convert.py `convert`; log_target.py where `--log` may write (#99) masks/ masks (Q32; a package since PP-5: model, files, header, layout, read, checks, labels): find_masks pairs every image with its masks (masks folder with one folder per type mirroring the experiment, or _.tif in the diff --git a/docs/contract.md b/docs/contract.md index fe541740..c4bfcda9 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -27,6 +27,7 @@ whole experiment; any change to them goes through the normalization brick. | Uniqueness | Folder names need only be unique among siblings, ignoring case. Two folder names of one level (condition, biological repeat or technical repeat) that differ only in case are the error `case_conflict`, whether siblings or anywhere in the experiment (`ctrl/Rep1` and `treated/rep1`; decision D-e, §16; siblings only before D86). | | Names | Every label and `position_id` passes `check_identity_text` (§16): no leading or trailing whitespace, and not `""`, `NA`, `nan`, `none`, `null`, `n/a` or `` in any case, which table readers turn into a missing value. A folder or image name that fails is the error `invalid_name` (decision D-e; accepted before D86). | | Depth | At most three folder levels below the root. A folder below the technical-repeat level is an error. | +| Links | Contain (issue #81, owner 2 November 2026): a symbolic link to a file or folder (on Windows also a directory junction, which Python 3.11 does not report as a link) is followed only when its real path (`os.path.realpath`) is inside the experiment folder, itself resolved. A link that leaves it is not followed and is the error `link_outside_folder`, naming each link and its target (a broken link too); a link to a folder on its own path (the folder holding it, or one above) is `link_loop`; one file 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; owner, 2 November 2026) is `linked_twice`, naming both. A broken link that stays inside is kept and reported by its reader (`unreadable_tiff`, `unreadable_mask`), never skipped. Hidden and system names (§1) are skipped before any link is looked at, so a hidden link is never followed; a contained link *to* a hidden folder is followed, because that folder is itself never walked, so its files are found once, under the link's name, which the user chose. The links inside an OME-Zarr store are held to the same rule, though the store is not entered as a folder level: its folders are listed and only its links resolved, about 84 ms per 20,000 store files on each discovery (accepted by the owner, 2 November 2026). The masks folder follows the same rule (§15), with `RunInputs` (§9.1) and `convert` (§17) refusing the same links; one helper, `links.LinkGuard`, walks both folders. | | System files | Dot-files and dot-folders (`.DS_Store`, `._*`, `.ipynb_checkpoints`), `Thumbs.db`, `desktop.ini` and `__MACOSX` are ignored without a warning. | | Scope | A group is identified by its full label path, e.g. `(control, bio_rep1)`. | | Positions | Every image is exactly one position (field of view): files with several positions, scenes, series or tiles are refused (§2). Its `position_id` is the image name without its extension (`.tif`, `.tiff` or `.zarr`, any case, with a preceding `.ome`): `img_01.ome.tif` gives `img_01`; a name that is only an extension (`.tif`, `.ome.zarr`) is refused. `discovery.position_id` is the one function every MorphoBricks brick names positions with (cross-brick decision D85, 29 October 2026), so it also removes the other image extensions bricks read (`.png`, `.jpg`, `.jpeg`, `.bmp`, `.gif`, `.npy`; naming only: this brick still analyses TIFF and Zarr images only). A position is identified by `condition`, `biological_repeat`, `technical_repeat` and `position_id` (cross-brick decision D02, 26 October 2026), so equal names in two technical repeats are two positions. Two images in one leaf folder with the same `position_id`, ignoring case (`img_01.tif` and `img_01.ome.zarr`), are an error (`duplicate_position_id`): the brick does not guess which is which. | @@ -121,11 +122,22 @@ method is an offset and a scale, `(pixels − offset) / scale`. | Severity | Situations | Effect | |---|---|---| -| Error | Experiment design not confirmed (`design_not_confirmed`) or a repeat group holding two folders of one condition (`repeat_group_conflict`), when a design is used (§12); image type RGB for images that are not three channels of an integer type (`rgb_not_possible`, §13); mixed dtypes; images with different numbers of channels (`different_channel_counts`, §2) or different channel names at the same channel number (`channel_names_differ`, §2); unreadable TIFF (header or pixel data) or OME-Zarr folder in a leaf folder that is not excluded; OME-Zarr images without the `zarr` extra; OME-Zarr images whose declared axes do not fit the experiment's roles or that the confirmed roles contradict (`ome_zarr_axes_mismatch`, §2); OME-Zarr images that do not name their axes when their OME-NGFF version does not fix them (`unreadable_ome_zarr`, §2); every image excluded; more than one image series in a file; unsupported sample format or axis; missing/duplicate X or Y, duplicate roles, unassigned non-singleton axis, inconsistent mapping; an image whose confirmed roles swap Y and X, contradict its header or put X before Y (`axes_yx_swapped`, `axes_contradict_header`, `axes_x_before_y`, §2); plane that cannot be read within the memory limit, not even in blocks (for example a plane stored compressed in one piece); folder named `single` in another case, or beside other folders (§1); folders of one level differing only in case, anywhere in the experiment (`case_conflict`); folder or image names with outer whitespace or reading as a missing value (`invalid_name`, §1); images in one leaf folder with the same position identifier (§1); folder below the technical-repeat level; images in a folder that also holds subfolders (`tiff_outside_leaf`, §1); output folder inside the experiment or inside the masks folder (`output_inside_masks`, §15); images named as masks of other images (`image_named_as_mask`, §1); no usable planes; no axis roles given or recorded (`axes_not_confirmed`, §9.2); another run holding the output folder's lock (`output_locked`, §9.2); a copy tree whose conversion record cannot be read (`conversion_record_unreadable`, §17); a recorded `experiment_design.csv` that cannot be read (`design_unreadable`); a `calibration.csv` that cannot be read (`calibration_unreadable`) or that states sizes for a target that is neither a folder nor an image of the experiment (`calibration_target_not_found`, §9.3); no experiment folder given or recorded (`experiment_not_recorded`, §9.2); when masks are given, a masks folder missing or inside the experiment, mask names differing only in case, two masks of one type for an image, a mask whose name could belong to two images or types, or a mask that is unreadable, has axes other than Y, X and named or stated Z/T (`mask_not_2d`), has stated axes that do not fit it or contradict the axes its metadata names (`invalid_mask_axes`, `mask_axes_contradict_header`), is stored with X before Y (its header's or its stated axes; refused by `check` and `run` too, never recorded with empty axes) or with its image's Y and X sizes swapped (`mask_layout`; never transposed), is of another size than its image (in `run`, its image's Y and X under the confirmed roles; not reported for an image with an axis error), is not of integers or booleans, has other depth slices or time points than the image's other masks or, in `run`, than the image (§15) | Processing is blocked | +| Error | Experiment design not confirmed (`design_not_confirmed`) or a repeat group holding two folders of one condition (`repeat_group_conflict`), when a design is used (§12); image type RGB for images that are not three channels of an integer type (`rgb_not_possible`, §13); mixed dtypes; images with different numbers of channels (`different_channel_counts`, §2) or different channel names at the same channel number (`channel_names_differ`, §2); unreadable TIFF (header or pixel data) or OME-Zarr folder in a leaf folder that is not excluded; OME-Zarr images without the `zarr` extra; OME-Zarr images whose declared axes do not fit the experiment's roles or that the confirmed roles contradict (`ome_zarr_axes_mismatch`, §2); OME-Zarr images that do not name their axes, and every OME-NGFF 0.1 or 0.2 store (`unreadable_ome_zarr`, §2); every image excluded; more than one image series in a file; unsupported sample format or axis; missing/duplicate X or Y, duplicate roles, unassigned non-singleton axis, inconsistent mapping; an image whose confirmed roles swap Y and X, contradict its header or put X before Y (`axes_yx_swapped`, `axes_contradict_header`, `axes_x_before_y`, §2); plane that cannot be read within the memory limit, not even in blocks (for example a plane stored compressed in one piece); folder named `single` in another case, or beside other folders (§1); folders of one level differing only in case, anywhere in the experiment (`case_conflict`); folder or image names with outer whitespace or reading as a missing value (`invalid_name`, §1); images in one leaf folder with the same position identifier (§1); links that leave the experiment or masks folder, loop, or reach one file twice (`link_outside_folder`, `link_loop`, `linked_twice`, §1); folder below the technical-repeat level; images in a folder that also holds subfolders (`tiff_outside_leaf`, §1); output folder inside the experiment or inside the masks folder (`output_inside_masks`, §15); images named as masks of other images (`image_named_as_mask`, §1); no usable planes; no axis roles given or recorded (`axes_not_confirmed`, §9.2); another run holding the output folder's lock (`output_locked`, §9.2); a copy tree whose conversion record cannot be read (`conversion_record_unreadable`, §17); a recorded `experiment_design.csv` that cannot be read (`design_unreadable`); a `calibration.csv` that cannot be read (`calibration_unreadable`) or that states sizes for a target that is neither a folder nor an image of the experiment (`calibration_target_not_found`, §9.3); no experiment folder given or recorded (`experiment_not_recorded`, §9.2); when masks are given, a masks folder missing or inside the experiment, mask names differing only in case, two masks of one type for an image, a mask whose name could belong to two images or types, or a mask that is unreadable, has axes other than Y, X and named or stated Z/T (`mask_not_2d`), has stated axes that do not fit it or contradict the axes its metadata names (`invalid_mask_axes`, `mask_axes_contradict_header`), is stored with X before Y (its header's or its stated axes; refused by `check` and `run` too, never recorded with empty axes) or with its image's Y and X sizes swapped (`mask_layout`; never transposed), is of another size than its image (in `run`, its image's Y and X under the confirmed roles; not reported for an image with an axis error), is not of integers or booleans, has other depth slices or time points than the image's other masks or, in `run`, than the image (§15) | Processing is blocked | | Warning | Images whose metadata does not name their axes, read with the confirmed roles (`axes_not_named`, §2); in a tree of named-axis copies, sources changed or gone since the conversion (`conversion_source_changed`) and copies changed, gone or added since (`copy_changed_after_conversion`, §17); folders named like a masks folder holding images named as masks (`mask_folder_in_experiment`, §1); intensity checks of the written table (`image_intensity_outlier`, `repeat_intensity_shift`, `saturated_image`, `no_intensity_spread`, §14), reported apart from the run's issues; files that are not images; empty folders; images excluded by the user, or excluded paths that match no image; non-finite pixels; different image widths or heights; different numbers of depth slices, time points or other planes; physical pixel sizes that differ or are known for only some images; a stated pixel size that differs from the header (`calibration_differs_from_header`, §9.3; the stated size is used); unbalanced numbers of images; biological repeats missing from some conditions (§11); planes without finite pixels while others remain; channels whose central intensity range covers too few histogram bins (`narrow_histogram_range`, §4); planes over the memory limit that are read in blocks; images without a mask of a type other images have, masks matching no image, masks of excluded images, files outside the mask-type folders (§15) | Processing continues; the item is reported | Every message states what happened in plain language, lists the affected paths, and -suggests an action. Technical details go to the diagnostic log. +suggests an action. Technical details go to the diagnostic log. The command line's log +(`--log FILE`, issue #99) is checked before it is opened, and opened only after the other +arguments: it is refused inside the experiment or masks folder (given, or recorded by the +earlier run in the output folder), inside `convert`'s folder of copies, as the design file +given with `--design` (an input of the run), as a folder or in a +folder that does not exist, and as or inside an output artefact (`plane_histograms.csv`, +`run_manifest.json`, the report files, `experiment_design.csv`, `calibration.csv`, +`mask_relationships.json`, the lock, `.morphobricks-conversion.json`, the `.run-` staging +folders; `log_target.PROTECTED_OUTPUTS`). Paths are compared resolved and, for the folders that exist, by +device and inode (another spelling of a folder on a file system that ignores case); output +names ignore case; an existing file with a second name (a hard link) is refused. Any other +name in the output folder is allowed. In notebook step 1, message details start collapsed with orange warning and red error counts visible outside the dropdown. Collapsing details does not change severity or diff --git a/docs/plan.md b/docs/plan.md index fe3904b6..8be74205 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -557,6 +557,10 @@ API, CLI). The notebook and CLI only call the API. header naming a column twice or a column without a name, and `RunInputs.design()` reads with `read_confirmed_design`; changing the notebook's output folder reloads the design from the new folder and asks again. +- **Before the audit: links and the log (issues #81 and #99)** *(done; questions 64 and + 65)*: `links.LinkGuard` follows a link only inside the folder walked (discovery, OME-Zarr + stores, the masks folder; `RunInputs` and `convert` refuse the same links), and the CLI + checks `--log` (`log_target`) before opening it, after the other arguments. - **Release v0.4.0 (PP-13) checklist**, for every tag of v0.4.0 including release candidates, in the release PR before the tag is pushed: - set `version` in `pyproject.toml`; @@ -1589,3 +1593,30 @@ Raised during review; to be decided before the milestones that need them: answer (the folder may still be being typed), and the design card says which. The run is gated as before, so it runs only on the new folder's confirmed design. No copy action: the user answers again, or copies the file. Contract §12; user guide §4. +64. Are symbolic links in the experiment or masks folder followed (issue #81)? *Decided + (owner, 2 November 2026): contain.* A link (file or folder; on Windows also a directory + junction) is followed only when its real path stays inside the experiment folder, or the + masks folder for masks, both resolved; a link that leaves it is refused + (`link_outside_folder`, naming each link and its target, a broken link too). One file + reached by two paths is refused, naming both (`linked_twice`); hard links too (the same + device and inode, from the one `stat` taken per image or mask; owner, 2 November 2026, + in review). A link back to a folder on its own path is `link_loop`. A broken link that + stays inside is reported by its reader, never skipped (review: masks skipped it). The + links inside an OME-Zarr store are held to the same rule (reading and copying a store + follow them); the cost, about 84 ms per 20,000 store files on each discovery, is + accepted (owner, 2 November 2026). One helper (`links.LinkGuard`) walks the experiment + and the masks folder for discovery, the masks pairing, the unrecorded-files check of + `RunInputs` (raised as `RunInputsError`) and, through discovery, `convert`. Contract §1, + §8. +65. Where may the command line's `--log` write (issue #99)? + *Decided (owner, 2 November 2026):* the log path is checked first and the file opened + only after the other arguments are checked. It 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 a protected output artefact (`plane_histograms.csv`, `run_manifest.json`, + the reports, the design, `calibration.csv`, `mask_relationships.json`, the lock, the + conversion record, the staging folders). Any other name in the output folder is fine. + In review: folders are also compared by device and inode and output names ignoring + case (macOS and Windows), and an existing file with a second name (a hard link) is + refused. Also in review: the design file given with `--design` (an input) is refused + (it was appended to). Contract §8; user guide §5. + diff --git a/docs/user_guide.md b/docs/user_guide.md index 61fa0e15..26aaa705 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -147,6 +147,13 @@ experiment/ images is dropped silently. - Other files (`.txt`, `.csv`, ...) are reported and skipped. System files such as `.DS_Store`, `Thumbs.db`, `desktop.ini` and `__MACOSX` are ignored without a message. +- Links (symbolic links, and junctions on Windows) are followed only when they stay inside + the experiment folder; the same holds for the masks folder. A link to a file or folder + outside it is an error (`link_outside_folder`) naming the link and its target: nothing + outside the folder you selected is read, so copy the data in or select a folder that + holds it. A link back to a folder above it is an error (`link_loop`), and so is one file + reached by two paths, a link and its target or two hard links (`linked_twice`): it would + count twice. To change a label, rename the folder and check again. @@ -607,7 +614,7 @@ report names the change and its effect. Other options: | `--no-exclude` | Clear the exclusions an earlier run recorded, so every image is analysed again (not together with `--exclude`) | | `--remove-stale-lock` | Remove the lock left by a run that was stopped or whose computer restarted (`check`, `run`, `masks`, `design` and `calibrate`); give it only when no other run uses the output folder | | `--outlier-limit N`, `--saturation-limit FRACTION`, `--min-images N` | Limits of the intensity checks printed after the run (default 3, 0.01 and 5) | -| `--log FILE` | Write technical details to a file | +| `--log FILE` | Write technical details to a file (`run`, `masks`, `convert`). It is checked before anything is written and opened only once the other options are checked: a file inside the experiment or masks folder (given or recorded by the earlier run), inside the folder of the copies (`convert`), the design file given with `--design`, or named as one of the run's files in the output folder (`plane_histograms.csv`, `run_manifest.json`, the reports, `experiment_design.csv`, `calibration.csv`, `mask_relationships.json`, the lock) is refused and nothing is written; so is a file that is a hard link (it has another name). Any other name in the output folder is fine, for example `run.log` | | `--masks FOLDER` | The masks folder (§2). `check` lists the mask types found and checks every mask from its header; `run` stops on a mask error and records the masks in `run_manifest.json` | | `--mask-axes TYPE=ROLES` | The axes of a mask type whose files do not name them, one role (Y, X, Z or T) per axis as stored: `Cells=TYX` for plain stacks of time points, `Cells=YX` for one 2D mask shared by every plane of its image (repeatable; `check`, `run`, `masks`). Masks are never reordered. | @@ -908,6 +915,9 @@ continues. |---|---|---| | `experiment_not_found` | Error | The folder does not exist. Select an existing folder. | | `unreadable_folder` | Error | A folder could not be opened. Check permissions. | +| `link_outside_folder` | Error | A link in the experiment or masks folder leads outside it (the message names each link and its target), and nothing outside the selected folder is read. Copy or move the files in, or select a folder that holds them, then remove the link. | +| `link_loop` | Error | A link leads back to a folder that holds it. Remove it. | +| `linked_twice` | Error | One file is reached by two paths (a link and its target, two links, or two hard links), so it would count twice under two names; both are named. Keep one path to each file. | | `reserved_folder_name` | Error | A folder is named `single` in another case (`Single`), or a folder named `single` has other folders beside it. Rename it to `single` in lower case, or give it a name of its own. | | `case_conflict` | Error | Folders of one level have names that differ only in case, next to each other or anywhere in the experiment (`control/Rep1` and `treated/rep1`). Spell each name one way, or rename or merge them. | | `invalid_name` | Error | A folder or image name starts or ends with a space, or is `NA`, `nan`, `none`, `null`, `n/a` or `` (any case), which tables read as a missing value. Rename it. | diff --git a/pyproject.toml b/pyproject.toml index 26d9d115..ee829c11 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -174,6 +174,8 @@ baseline = "tools/code_rules_baseline.json" "morphobricks_preprocessing.design_values" = "pure" "morphobricks_preprocessing.design_edit" = "pure" "morphobricks_preprocessing.discovery" = "io" +"morphobricks_preprocessing.links" = "io" +"morphobricks_preprocessing.log_target" = "io" "morphobricks_preprocessing.image_type" = "pure" "morphobricks_preprocessing.intensity" = "io" "morphobricks_preprocessing.manifest" = "io" diff --git a/src/morphobricks_preprocessing/cli.py b/src/morphobricks_preprocessing/cli.py index 3e00fc38..d4271308 100644 --- a/src/morphobricks_preprocessing/cli.py +++ b/src/morphobricks_preprocessing/cli.py @@ -35,7 +35,13 @@ from .cli_convert import add_convert_parser, convert_command from .cli_design import add_design_parser, design_command, design_instructions from .cli_masks import add_masks_arguments, masks_command -from .cli_run import add_run_arguments, command_preflight, parse_size, run_command +from .cli_run import ( + add_run_arguments, + command_preflight, + parse_size, + run_command, + use_recorded_experiment, +) from .design import ( DESIGN_FILENAME, DesignCheckError, @@ -43,6 +49,7 @@ ) from .discovery import default_output_folder from .image_type import review_image_type +from .log_target import log_refusal, run_log_refusal from .mask_text import mask_summary from .masks import mask_axes_option from .report import ( @@ -106,15 +113,24 @@ def main(argv: Sequence[str] | None = None) -> int: return {"calibrate": calibrate_command, "design": design_command}[args.command](args) if getattr(args, "no_exclude", False) and args.exclude: parser.error("--exclude and --no-exclude cannot be given together") - handler = _attach_log(args.log) if getattr(args, "log", None) else None - try: - if args.command == "convert": - return convert_command(args) if _remove_stale_lock(args) else 1 + if args.command != "convert": try: args.mask_axes = mask_axes_option(args.mask_axes) except ValueError as exc: print(exc) return 1 + if args.command in ("run", "masks") and not use_recorded_experiment(args): + return 1 + # The log is opened only now, once the arguments are checked, and never inside the + # experiment, the masks or the copies, or over an output file (issue #99). + refusal = _log_refusal(args) if getattr(args, "log", None) else None + if refusal is not None: + print(refusal) + return 1 + handler = _attach_log(args.log) if getattr(args, "log", None) else None + try: + if args.command == "convert": + return convert_command(args) if _remove_stale_lock(args) else 1 if args.remove_stale_lock and not _remove_stale_lock(args): return 1 return {"check": _check, "run": run_command, "masks": masks_command}[args.command](args) @@ -278,6 +294,17 @@ def _print_design(args: argparse.Namespace, pre: Preflight) -> None: ) +def _log_refusal(args: argparse.Namespace) -> str | None: + """Return why ``--log`` may not be written for this command, or None.""" + if args.command == "convert": + folders = {"experiment folder": args.experiment, "folder of the copies": args.to} + return log_refusal(args.log, folders) + experiment = Path(args.experiment).resolve() + output = Path(args.output) if args.output else default_output_folder(experiment) + design = getattr(args, "design", None) + return run_log_refusal(args.log, experiment, output, args.masks, design) + + def _attach_log(path: str) -> logging.Handler: handler = logging.FileHandler(path, encoding="utf-8") handler.setLevel(logging.DEBUG) diff --git a/src/morphobricks_preprocessing/discovery.py b/src/morphobricks_preprocessing/discovery.py index 508babd8..bf1c5676 100644 --- a/src/morphobricks_preprocessing/discovery.py +++ b/src/morphobricks_preprocessing/discovery.py @@ -17,7 +17,8 @@ and its ``position_id``, the image name without its extension (contract §1). Only folder and file names are read here; image headers are inspected by -:mod:`morphobricks_preprocessing.metadata`. +:mod:`morphobricks_preprocessing.metadata`. Links are followed only inside the +experiment folder (:mod:`.links`, issue #81). """ from __future__ import annotations @@ -32,6 +33,7 @@ from morphobricks_normalization.records import LEVELS, SINGLE from .image_names import position_id +from .links import LinkGuard from .mask_names import mask_name_issues from .validation import ValidationIssue, agree, error, has_errors, plural, warning from .zarr_input import is_zarr_name @@ -348,22 +350,22 @@ def discover_experiment( walker.issues.extend(check_output_folder(root, output_folder)) walker.excluded = Path(output_folder).resolve() walker.visit(root, ()) - issues = (*walker.finish(), *mask_name_issues(walker.leaves)) + issues = (*walker.finish(), *walker.links.issues(), *mask_name_issues(walker.leaves)) return DiscoveryResult(root=root, leaves=tuple(walker.leaves), issues=issues) class _Walker: """Collects leaf folders and problem paths during one walk.""" - def __init__(self, root: Path): # noqa: ANN204 + def __init__(self, root: Path) -> None: self.root = root + self.links = LinkGuard(root, "experiment folder") self.excluded: Path | None = None self.leaves: list[LeafFolder] = [] self.issues: list[ValidationIssue] = [] self.reserved: list[str] = [] self.single_with_siblings: list[str] = [] - # Every spelling of each level's folder names, by its case-folded name: names that - # differ only in case clash anywhere in the experiment (decision D-e). + # Each level's name spellings by case-folded name: case-only clashes (decision D-e). self.spellings: list[dict[str, dict[str, list[str]]]] = [{} for _ in LEVELS] self.invalid_names: list[str] = [] self.too_deep: list[str] = [] @@ -377,6 +379,7 @@ def relative(self, path: Path) -> str: return path.relative_to(self.root).as_posix() or ROOT def visit(self, folder: Path, names: tuple[str, ...]) -> None: + """Sort the entries of ``folder`` (labels ``names``), following links by the guard.""" try: entries = sorted(os.scandir(folder), key=lambda entry: entry.name) except OSError: @@ -390,13 +393,13 @@ def visit(self, folder: Path, names: tuple[str, ...]) -> None: path = Path(entry.path) if is_system_name(entry.name): logger.debug("Skipping system file %s", path) - elif is_ome_zarr_folder(entry): - tiffs.append(self.relative(path)) + elif (real := self.links.real(entry, folder)) is None: + logger.debug("Link %s not followed", path) # reported by discover_experiment + elif is_ome_zarr_folder(entry) or not entry.is_dir() and is_tiff_name(entry.name): + tiffs.append(self.links.claim(path, real)) elif entry.is_dir(): if path != self.excluded: subfolders.append(path) - elif is_tiff_name(entry.name): - tiffs.append(self.relative(path)) else: self.non_tiff.append(self.relative(path)) @@ -412,18 +415,16 @@ def visit(self, folder: Path, names: tuple[str, ...]) -> None: self.outside_leaf.extend(tiffs) self._check_names(subfolders, len(names)) if len(names) == len(LEVELS): - # Stop here: the folders below a technical repeat have no level to map - # to, and listing them is enough for the error message. + # Stop: no level below a technical repeat; listing the folders is enough. self.too_deep.extend(self.relative(path) for path in subfolders) return for path in subfolders: self.visit(path, (*names, path.name)) def _check_names(self, subfolders: list[Path], level: int) -> None: - # Names are compared ignoring case, across the whole experiment (decision D-e): - # Windows and macOS would merge "Ctrl" and "ctrl" when the experiment is copied - # there, and later bricks would key "Rep1" and "rep1" as two biological repeats - # where design pairing and other tools take them as one. + # Names are compared ignoring case, experiment-wide (decision D-e): Windows and + # macOS would merge "Ctrl" and "ctrl" in a copy, and later bricks would key "Rep1" + # and "rep1" as two biological repeats where design pairing takes them as one. for path in subfolders: if level < len(LEVELS): spellings = self.spellings[level].setdefault(path.name.casefold(), {}) @@ -445,9 +446,8 @@ def _check_names(self, subfolders: list[Path], level: int) -> None: self.single_with_siblings.append(self.relative(path)) def _check_positions(self, images: list[str]) -> None: - # Two images of one technical repeat with the same position identifier would - # be the same field of view to every later brick, so this is refused rather - # than guessed. Case is ignored for the same reason as for folder names. + # Two images of one technical repeat with one position identifier (ignoring case, + # as for folder names) would be one field of view to every brick: refused. by_key: dict[str, list[str]] = {} for image in images: by_key.setdefault(position_id(image).casefold(), []).append(image) @@ -581,9 +581,8 @@ def finish(self) -> tuple[ValidationIssue, ...]: ) if self.outside_leaf: issues.append( - # An error, not a warning (owner decision, October 2026): a folder holding - # both images and subfolders has no level for its images, and every brick - # refuses it, so they are never dropped silently. + # An error (owner, October 2026): a folder holding images and subfolders + # has no level for its images; every brick refuses it, never dropping them. error( "tiff_outside_leaf", f"{plural(len(self.outside_leaf), 'image')} " diff --git a/src/morphobricks_preprocessing/links.py b/src/morphobricks_preprocessing/links.py new file mode 100644 index 00000000..1d593895 --- /dev/null +++ b/src/morphobricks_preprocessing/links.py @@ -0,0 +1,247 @@ +"""Symbolic links in the experiment and masks folders (issue #81; owner, 2 November 2026). + +The rule is **contain**: a link (to a file or a folder) is followed only when its real path +(every link resolved) stays inside the folder that is walked, the experiment folder or the +masks folder, itself resolved. Nothing outside the folder the user selected is read: + +- a link whose real path leaves the folder is not followed, and is the error + ``link_outside_folder`` naming each link and its target; +- a link to a folder on the walk's own path (the folder holding it or one above) is not + followed, and is the error ``link_loop``; +- two paths of one file (a link and its target, two links to it, or two hard links: the + same device and inode) would count one image or mask twice under two names: the error + ``linked_twice`` names both (hard links: owner, 2 November 2026). + +Inside an OME-Zarr store, which discovery never enters as a folder level, every link is +held to the same rule (:meth:`LinkGuard.check_store`), since reading and copying the store +follow its links. + +A link is what Python reports as one: a symbolic link (``os.DirEntry.is_symlink``) and, on +Windows, also a directory junction (a mount-point reparse point), which Python 3.11 does +not report as a link. Real paths come from :func:`os.path.realpath`. One +:class:`LinkGuard` serves one walk of one folder. +""" + +from __future__ import annotations + +import logging +import os +import stat +from collections.abc import Callable, Iterator +from pathlib import Path + +from .validation import ValidationIssue, agree, error, plural + +logger = logging.getLogger(__name__) + +LINK_OUTSIDE = "link_outside_folder" +LINK_LOOP = "link_loop" +LINKED_TWICE = "linked_twice" +LINK_CODES = frozenset({LINK_OUTSIDE, LINK_LOOP, LINKED_TWICE}) +"""The codes of the errors about links (all errors).""" + +_JUNCTION = getattr(stat, "IO_REPARSE_TAG_MOUNT_POINT", None) + + +class LinkRefusedError(ValueError): + """A folder holds links that are refused; the message names them.""" + + +def is_link(entry: os.DirEntry) -> bool: + """Return True for a symbolic link, or a directory junction on Windows. + + Parameters + ---------- + entry + A folder entry from :func:`os.scandir`. + + Returns + ------- + bool + """ + if entry.is_symlink(): + return True + if os.name != "nt" or _JUNCTION is None: + return False + try: + return getattr(entry.stat(follow_symlinks=False), "st_reparse_tag", 0) == _JUNCTION + except OSError: + return False + + +def is_inside(path: Path, folder: Path) -> bool: + """Return True when ``path`` is ``folder`` or below it (both resolved).""" + return path == folder or folder in path.parents + + +class LinkGuard: + """Follow the links of one walk of ``root`` only when they stay inside it. + + Parameters + ---------- + root + The folder walked, as the walk names it; links are held to its real path. + what + What it is, for the messages (``"experiment folder"``, ``"masks folder"``). + """ + + def __init__(self, root: Path, what: str) -> None: + self.root = root + self.real_root = Path(os.path.realpath(root)) + self.what = what + # The real paths of the folders above each folder entered, the folder's own last. + self._stacks: dict[Path, tuple[Path, ...]] = {root: (self.real_root,)} + self._owners: dict[Path | tuple[int, int], str] = {} + self.outside: list[str] = [] + self.loops: list[str] = [] + self.twice: list[str] = [] + + def relative(self, path: Path) -> str: + """Return a path of the walk relative to the root, with ``/`` separators.""" + return path.relative_to(self.root).as_posix() or "." + + def real(self, entry: os.DirEntry, folder: Path) -> Path | None: + """Return the real path of an entry of ``folder``, or None for a refused link. + + ``folder`` is the root or a folder this guard let through. A folder let through can + be walked next. + """ + stack = self._stacks[folder] + if not is_link(entry): + target = stack[-1] / entry.name + else: + target = Path(os.path.realpath(entry.path)) + shown = f"{self.relative(Path(entry.path))} -> {target}" + if not is_inside(target, self.real_root): + self.outside.append(shown) + return None + if target in stack: + self.loops.append(shown) + return None + if entry.is_dir(): + self._stacks[Path(entry.path)] = (*stack, target) + return target + + def claim(self, path: Path, real: Path) -> str: + """Record that ``path``, an image or mask of the walk, is ``real``; return its name. + + A file is known by its device and inode (from the one ``stat`` this takes), so a + hard link to an image already found is caught like a symbolic one; by its real + path when it cannot be stat'ed (a broken link) or has no inode. An OME-Zarr store + (a folder) also has its links checked (:meth:`check_store`). + """ + try: + found = os.stat(path) + except OSError: # a broken link: refused or reported unreadable by its reader + found = None + if found is not None and stat.S_ISDIR(found.st_mode): + self.check_store(path) + key = (found.st_dev, found.st_ino) if found is not None and found.st_ino else real + name = self.relative(path) + first = self._owners.setdefault(key, name) + if first != name: + self.twice.append(f"{name} (the same file as {first})") + return name + + def files(self, folder: Path, skip: Callable[[str], bool]) -> Iterator[tuple[Path, Path]]: + """Yield ``(path, real path)`` of every file below ``folder``, sorted by path. + + Folders and files whose name ``skip`` accepts are left out; links are followed under + this guard's rule, and a folder that cannot be listed is left out (logged). + """ + try: + entries = sorted(os.scandir(folder), key=lambda entry: entry.name) + except OSError: + logger.debug("Cannot list %s", folder, exc_info=True) + return + for entry in entries: + if skip(entry.name): + continue + real = self.real(entry, folder) + if real is None: + continue + if entry.is_dir(): + yield from self.files(Path(entry.path), skip) + else: + yield Path(entry.path), real + + def check_store(self, store: Path) -> None: + """Hold every link inside an OME-Zarr store (a folder let through) to the rule. + + A store can hold millions of chunk files: only its folders and its links are + followed up, so a file costs no more than its listing. + """ + pending = [store] + while pending: + folder = pending.pop() + try: + entries = list(os.scandir(folder)) + except OSError: + logger.debug("Cannot list %s", folder, exc_info=True) + continue + for entry in entries: + if is_link(entry): + if self.real(entry, folder) is not None and entry.is_dir(): + pending.append(Path(entry.path)) + elif entry.is_dir(follow_symlinks=False): + stack = self._stacks[folder] + self._stacks[Path(entry.path)] = (*stack, stack[-1] / entry.name) + pending.append(Path(entry.path)) + + def issues(self) -> list[ValidationIssue]: + """Return the errors about the links found so far.""" + issues = [] + if self.outside: + issues.append( + error( + LINK_OUTSIDE, + f"{plural(len(self.outside), 'link')} in the {self.what} " + f"{agree(self.outside, 'leads', 'lead')} outside it (link -> target), " + "and nothing outside the selected folder is read: not followed.", + self.outside, + f"Copy or move the files into the {self.what}, or select a folder that " + "holds them, then remove the links.", + ) + ) + if self.loops: + issues.append( + error( + LINK_LOOP, + f"{plural(len(self.loops), 'link')} in the {self.what} " + f"{agree(self.loops, 'leads', 'lead')} back to a folder that " + "holds it, so the folder would be read again and again; not followed.", + self.loops, + "Remove these links.", + ) + ) + if self.twice: + issues.append( + error( + LINKED_TWICE, + f"{plural(len(self.twice), 'path')} in the {self.what} " + f"{agree(self.twice, 'is', 'are')} the same file as another path (through a " + "link, or a hard link), so one file would count twice, under two names.", + self.twice, + "Keep one path to each file: remove the link (or the hard link), or " + "replace it with a copy if both are really meant.", + ) + ) + return issues + + +def refuse_links(issues: list[ValidationIssue] | tuple[ValidationIssue, ...]) -> None: + """Raise :class:`LinkRefusedError` when the issues hold an error about links. + + Parameters + ---------- + issues + Issues of a walk (discovery's, or the masks folder's). + + Raises + ------ + LinkRefusedError + Naming every refused link. + """ + found = [issue for issue in issues if issue.code in LINK_CODES] + if found: + raise LinkRefusedError("\n".join(str(issue) for issue in found)) diff --git a/src/morphobricks_preprocessing/log_target.py b/src/morphobricks_preprocessing/log_target.py new file mode 100644 index 00000000..fb925ec8 --- /dev/null +++ b/src/morphobricks_preprocessing/log_target.py @@ -0,0 +1,182 @@ +"""Where the command line's ``--log`` may write (issue #99; owner, 2 November 2026). + +The log is checked before it is opened, and it is opened only once the command's arguments +are checked, so an invalid command writes nothing (design v0.4 §1: sources are never +touched). A log is refused: + +- inside the experiment folder or the masks folder, given or recorded by the earlier run in + the output folder (it would become a source, or change one); +- inside the folder of named-axis copies of ``convert`` (every file there is a copy, and + the conversion refuses files it did not write); +- as, or inside, a file or folder the run writes or keeps in its output folder + (:data:`PROTECTED_OUTPUTS`, the lock and the staging folders). Any other name in the + output folder is fine; +- as the design file given with ``--design`` (an input of the run); +- when it is a folder, or its folder does not exist. + +Paths are compared resolved, so a link to a protected file is refused as the file, and by +device and inode where they exist, so another spelling of a folder (case on macOS and +Windows) is refused as the folder; output names are compared ignoring case. An existing file +with another name (a hard link) is refused: it may be a source or an output under that name. +""" + +from __future__ import annotations + +import os +from collections.abc import Mapping +from pathlib import Path + +from morphobricks_normalization.csv_io import OUTPUT_FILENAME + +from .calibration import CALIBRATION_FILENAME +from .conversion_record import RECORD_FILENAME +from .design import DESIGN_FILENAME +from .discovery_report import REPORT_FILENAME, REPORT_TEXT_FILENAME +from .links import is_inside +from .manifest import MANIFEST_FILENAME +from .mask_scan import MASK_REPORT_FILENAME +from .run.lock import LOCK_FILENAME +from .run.publish import STAGING_PREFIX +from .run.recorded import recorded_masks_folder, usable +from .run.reuse import read_previous + +PROTECTED_OUTPUTS = frozenset( + { + OUTPUT_FILENAME, + MANIFEST_FILENAME, + REPORT_FILENAME, + REPORT_TEXT_FILENAME, + DESIGN_FILENAME, + CALIBRATION_FILENAME, + MASK_REPORT_FILENAME, + LOCK_FILENAME, + RECORD_FILENAME, + } +) +"""Names in an output folder a log may not be written to, or inside.""" + + +def log_refusal( + log: str | os.PathLike, + folders: Mapping[str, str | os.PathLike | None], + output: str | os.PathLike | None = None, +) -> str | None: + """Return why a log cannot be written at ``log``, or None when it can. + + Parameters + ---------- + log + The log file asked for. + folders + Folders the log may not be in, by what they are (``"experiment folder"``); None + values are left out. + output + The command's output folder, where only :data:`PROTECTED_OUTPUTS` and the staging + folders are refused; None when the command has none. + + Returns + ------- + str or None + The message for the user, naming the log and what it would touch. + """ + target = Path(log).resolve() + for what, folder in folders.items(): + if folder is not None and _parts_below(target, Path(folder)) is not None: + return _refused(log, f"it is inside the {what} ({Path(folder).resolve()})") + parts = None if output is None else _parts_below(target, Path(output)) + if parts is not None: + first = parts[0].casefold() if parts else "" + if not parts or first in _PROTECTED_FOLDED or first.startswith(STAGING_PREFIX): + return _refused(log, f"it would write over the run's {parts[0] if parts else 'folder'}") + if target.is_dir(): + return _refused(log, "it is a folder") + if not target.parent.is_dir(): + return _refused(log, f"its folder {target.parent} does not exist") + if target.is_file() and target.stat().st_nlink > 1: + return _refused(log, "it is a file that has another name too (a hard link)") + return None + + +_PROTECTED_FOLDED = frozenset(name.casefold() for name in PROTECTED_OUTPUTS) + + +def _parts_below(target: Path, folder: Path) -> tuple[str, ...] | None: + """Return the names of ``target`` (resolved) below ``folder``; None when it is not inside. + + Compared by resolved path, then, for each existing folder above ``target``, by device and + inode, which catches another spelling of ``folder`` on a file system that ignores case. + """ + real = folder.resolve() + if is_inside(target, real): + return target.relative_to(real).parts + try: + wanted = os.stat(real) + except OSError: # a folder that does not exist holds nothing + return None + chain = [target, *target.parents] + for depth, above in enumerate(chain): + try: + if os.path.samestat(os.stat(above), wanted): + return tuple(path.name for path in reversed(chain[:depth])) + except OSError: # not there (yet): nothing to compare + continue + return None + + +def run_log_refusal( + log: str | os.PathLike, + experiment: str | os.PathLike, + output: str | os.PathLike, + masks: str | os.PathLike | None, + design: str | os.PathLike | None = None, +) -> str | None: + """Return why the log of ``run`` or ``masks`` cannot be written, or None. + + The experiment and masks folders recorded by the earlier run in ``output`` are refused + too, as the given ones (the manifest is only read), and so is the design file given. + + Parameters + ---------- + log + The log file asked for. + experiment + The experiment folder given (or the recorded one). + output + The output folder. + masks + The masks folder given, or None. + design + The design file given with ``--design``, or None. + + Returns + ------- + str or None + As :func:`log_refusal`. + """ + if design is not None and _same_file(Path(log), Path(design)): + given = Path(design).resolve() + return _refused(log, f"it is the design file given with --design ({given})") + recorded = usable(read_previous(Path(output).resolve())[0]) + folders = {"experiment folder": experiment, "masks folder": masks} + if recorded is not None: + folders["experiment folder of the earlier run"] = recorded.experiment_root + folders["masks folder of the earlier run"] = recorded_masks_folder(recorded) + return log_refusal(log, folders, output) + + +def _same_file(one: Path, other: Path) -> bool: + """Return True when two paths name one file: resolved, or by device and inode.""" + if one.resolve() == other.resolve(): + return True + try: + return os.path.samefile(one, other) + except OSError: # one of them does not exist: not the same file + return False + + +def _refused(log: str | os.PathLike, why: str) -> str: + """Say that the log is not opened, why, and what to do.""" + return ( + f"The log {log} was not opened: {why}. Nothing was written. Give --log another file, " + "outside the experiment and masks folders, for example run.log in the output folder." + ) diff --git a/src/morphobricks_preprocessing/masks/files.py b/src/morphobricks_preprocessing/masks/files.py index a9fa0602..50bdcd21 100644 --- a/src/morphobricks_preprocessing/masks/files.py +++ b/src/morphobricks_preprocessing/masks/files.py @@ -12,6 +12,7 @@ from ..discovery import DiscoveryResult, is_system_name, is_tiff_name from ..image_names import position_id +from ..links import LinkGuard from ..metadata import TiffMetadata from ..validation import ValidationIssue, agree, error, has_errors, plural, warning from ..zarr_input import ( @@ -138,7 +139,7 @@ def _folder_masks( A mask matching no image is the warning ``mask_without_image``, and a mask of an excluded image the warning ``mask_of_excluded_image``: neither is used (design v0.4 §3 keeps the severity of every existing check). ``images`` are the analysed images' paths (excluded - images left out). + images left out). Links are followed only inside the masks folder (:mod:`..links`). """ by_key, by_leaf = {}, {} for leaf in discovery.leaves: @@ -149,14 +150,13 @@ def _folder_masks( by_key[(prefix + stem).casefold()] = image by_leaf.setdefault(prefix.casefold(), []).append((stem, image)) files, unpaired, loose, ambiguous, excluded = [], [], [], [], [] - for path in sorted(folder.rglob("*")): - relative = path.relative_to(folder) - if any(is_system_name(part) for part in relative.parts) or not path.is_file(): - continue - mask_path = relative.as_posix() + links = LinkGuard(folder, "masks folder") + for path, real in links.files(folder, is_system_name): + relative = path.relative_to(folder) # a broken link is kept: its reader names it if not is_tiff_name(path.name): - (loose if len(relative.parts) == 1 else unpaired).append(mask_path) + (loose if len(relative.parts) == 1 else unpaired).append(relative.as_posix()) continue + mask_path = links.claim(path, real) found = _suffix_matches(relative, by_leaf) if len(relative.parts) > 1: key = Path(*relative.parts[1:]).with_name(position_id(path.name)).as_posix() @@ -168,7 +168,7 @@ def _folder_masks( files.append(MaskFile(found[0][0], found[0][1], mask_path, FOLDER)) else: (ambiguous if found else unpaired).append(mask_path) - issues = [*_layout_issues(loose, unpaired), *_excluded_issues(excluded)] + issues = [*links.issues(), *_layout_issues(loose, unpaired), *_excluded_issues(excluded)] if ambiguous: issues.append( error( diff --git a/src/morphobricks_preprocessing/run/publish.py b/src/morphobricks_preprocessing/run/publish.py index 1a8faa87..6475b2ca 100644 --- a/src/morphobricks_preprocessing/run/publish.py +++ b/src/morphobricks_preprocessing/run/publish.py @@ -31,6 +31,9 @@ logger = logging.getLogger(__name__) +STAGING_PREFIX = ".run-" +"""The name prefix of a run's staging folders in the output folder (removed after the run).""" + class _OutputRollbackError(RuntimeError): """Raised when an output update fails and its earlier files cannot be restored.""" @@ -65,7 +68,7 @@ def _staged_outputs( The outputs are written to a temporary folder inside ``output``; ``with_design`` publishes ``experiment_design.csv`` too, and ``with_report`` the report files. """ - staged = Path(tempfile.mkdtemp(prefix=".run-", dir=output)) + staged = Path(tempfile.mkdtemp(prefix=STAGING_PREFIX, dir=output)) keep_backups = False try: yield staged diff --git a/src/morphobricks_preprocessing/run_inputs.py b/src/morphobricks_preprocessing/run_inputs.py index 8b6fa497..814cbc9a 100644 --- a/src/morphobricks_preprocessing/run_inputs.py +++ b/src/morphobricks_preprocessing/run_inputs.py @@ -39,6 +39,7 @@ from .conversion_check import conversion_problem from .design import DESIGN_FILENAME, DesignCheckError, ExperimentDesign, read_confirmed_design from .fingerprint import Fingerprint +from .links import LinkRefusedError from .manifest import ( LEGACY_MANIFEST_VERSION, MANIFEST_FILENAME, @@ -362,6 +363,8 @@ def _check_files(self) -> None: raise RunInputsError(_ZARR_MISSING) from exc except NgffError as exc: raise RunInputsError(_labels_unreadable_text(exc)) from exc + except LinkRefusedError as exc: + raise RunInputsError(f"The run cannot be checked:\n{exc}") from exc if unrecorded: raise UnrecordedFilesError(unrecorded) diff --git a/src/morphobricks_preprocessing/unrecorded.py b/src/morphobricks_preprocessing/unrecorded.py index 5ca98119..8b9357d6 100644 --- a/src/morphobricks_preprocessing/unrecorded.py +++ b/src/morphobricks_preprocessing/unrecorded.py @@ -13,7 +13,10 @@ no image are left out, as the run leaves them out), and the label images of the recorded OME-Zarr images, less the masks the run recorded. Label images are never skipped: without the ``zarr`` extra, or with a ``labels`` group that cannot be read, the check raises - instead of passing (owner, 2 November 2026). + instead of passing (owner, 2 November 2026); +- links: a link that leaves the experiment or masks folder, leads back to a folder holding + it, or is a second path to a file is never passed over: the check raises + :class:`.links.LinkRefusedError` (issue #81), as the run refuses it. """ from __future__ import annotations @@ -23,6 +26,7 @@ from pathlib import Path from .discovery import DiscoveryResult, discover_experiment +from .links import refuse_links from .manifest import RunManifest from .masks import FOLDER from .masks.files import _folder_masks @@ -84,9 +88,13 @@ def unrecorded_files( NgffError When the ``labels`` group of a recorded OME-Zarr image cannot be read (the message names the image). + LinkRefusedError + When the experiment or masks folder holds a link the run refuses (the message names + each link). """ recorded = {entry.relative_path for entry in manifest.files} discovery = discover_experiment(experiment_root, output_folder) + refuse_links(discovery.issues) images = set(discovery.images) - recorded - set(manifest.settings.exclude) recorded_masks = {m["path"] for m in (manifest.masks or {}).get("files", ())} masks = _folder_mask_paths(masks_root, discovery, recorded) | _label_paths( @@ -101,7 +109,8 @@ def _folder_mask_paths( """Return the masks-folder files that pair with a recorded image, by the run's rules.""" if masks_root is None or not masks_root.is_dir(): return set() - found, _ = _folder_masks(masks_root, discovery, recorded) + found, issues = _folder_masks(masks_root, discovery, recorded) + refuse_links(issues) return {mask.path for mask in found if mask.source == FOLDER} diff --git a/tests/test_cli_log.py b/tests/test_cli_log.py new file mode 100644 index 00000000..811da1c7 --- /dev/null +++ b/tests/test_cli_log.py @@ -0,0 +1,177 @@ +"""``--log`` is checked before it is opened, and opened only after the arguments (issue #99). + +Before the fix the log was opened, created or appended to, before anything was checked: in +the experiment folder, over a source image, or over an output file of the run. +""" + +import os + +import pytest + +from morphobricks_preprocessing.cli import main +from morphobricks_preprocessing.testing import make_clean_run + + +@pytest.fixture +def clean(tmp_path): + return make_clean_run( + tmp_path / "run", axes="YX", shape=(16, 20), mask_types=("Cells",), design=True + ) + + +def _refused(capsys, path, before=None): + out = capsys.readouterr().out + assert "was not opened" in out and "Nothing was written" in out + if before is None: + assert not path.exists() + else: + assert path.read_bytes() == before + return out + + +def _run(clean, *more): + return ["run", str(clean.experiment), "--output", str(clean.output), *more] + + +def _masks(clean, *more): + args = ["masks", str(clean.experiment), "--output", str(clean.output)] + return [*args, "--masks", str(clean.masks), *more] + + +@pytest.mark.parametrize("command", [_run, _masks]) +def test_a_log_in_the_experiment_folder_is_refused(clean, capsys, command): + log = clean.experiment / "notes.txt" + assert main(command(clean, "--log", str(log))) == 1 + assert "inside the experiment folder" in _refused(capsys, log) + + +@pytest.mark.parametrize("command", [_run, _masks]) +def test_a_log_over_a_source_image_leaves_it_unchanged(clean, capsys, command): + image = clean.experiment / "ctrl/rep1/a.ome.tif" + before = image.read_bytes() + assert main(command(clean, "--log", str(image))) == 1 + _refused(capsys, image, before) + + +@pytest.mark.parametrize("command", [_run, _masks]) +def test_a_log_in_the_masks_folder_is_refused(clean, capsys, command): + log = clean.masks / "Cells/notes.txt" + assert main(command(clean, "--log", str(log))) == 1 + assert "inside the masks folder" in _refused(capsys, log) + + +@pytest.mark.parametrize("command", [_run, _masks]) +@pytest.mark.parametrize( + "name", + ["plane_histograms.csv", "run_manifest.json", "discovery_report.txt", "experiment_design.csv"], +) +def test_a_log_over_an_output_file_leaves_it_unchanged(clean, capsys, command, name): + target = clean.output / name + before = target.read_bytes() + assert main(command(clean, "--log", str(target))) == 1 + assert f"the run's {name}" in _refused(capsys, target, before) + + +@pytest.mark.parametrize( + "name", + [ + "calibration.csv", + "mask_relationships.json", + ".morphobricks-preprocessing.lock", + ".run-x/log.txt", + ], +) +def test_a_log_named_as_another_output_is_refused(clean, capsys, name): + log = clean.output / name + assert main(_run(clean, "--log", str(log))) == 1 + _refused(capsys, log) + + +def test_the_recorded_experiment_and_masks_folders_are_refused(clean, capsys): + # Only --output given: the run uses the recorded experiment and masks folders. + for log, where in [ + (clean.experiment / "notes.txt", "experiment folder"), + (clean.masks / "notes.txt", "masks folder of the earlier run"), + ]: + assert main(["run", "--output", str(clean.output), "--log", str(log)]) == 1 + assert f"inside the {where}" in _refused(capsys, log) + + +def test_a_log_is_not_opened_when_the_arguments_are_refused(clean, capsys, tmp_path): + log = tmp_path / "run.log" + assert main(_run(clean, "--mask-axes", "Cells", "--log", str(log))) == 1 + capsys.readouterr() + assert not log.exists() + + +@pytest.mark.parametrize("command", [_run, _masks]) +def test_a_log_elsewhere_in_the_output_folder_is_written(clean, capsys, command): + log = clean.output / "run.log" + status = main(command(clean, "--log", str(log))) + assert "was not opened" not in capsys.readouterr().out + assert status == 0 and log.is_file() + + +def test_convert_refuses_a_log_in_the_experiment_or_the_copies(clean, capsys, tmp_path): + copies = tmp_path / "copies" + for log, where in [ + (clean.experiment / "notes.txt", "experiment folder"), + (copies / "notes.txt", "folder of the copies"), + ]: + copies.mkdir(exist_ok=True) + args = ["convert", str(clean.experiment), "--to", str(copies), "--axes", "YX"] + assert main([*args, "--log", str(log)]) == 1 + assert f"inside the {where}" in _refused(capsys, log) + assert list(copies.iterdir()) == [] + + +def test_convert_writes_a_log_outside(clean, capsys, tmp_path): + log = tmp_path / "convert.log" + args = ["convert", str(clean.experiment), "--to", str(tmp_path / "copies"), "--axes", "YX"] + assert main([*args, "--log", str(log)]) == 0 + assert log.read_text(encoding="utf-8") + + +def test_a_log_in_a_missing_folder_or_naming_a_folder_is_refused(clean, capsys, tmp_path): + for log in (tmp_path / "missing/run.log", tmp_path): + assert main(_run(clean, "--log", str(log))) == 1 + assert "was not opened" in capsys.readouterr().out + + +@pytest.mark.parametrize("command", [_run, _masks]) +def test_a_log_hard_linked_to_a_source_image_leaves_it_unchanged(clean, capsys, tmp_path, command): + image = clean.experiment / "ctrl/rep1/a.ome.tif" + before = image.read_bytes() + log = tmp_path / "elsewhere.log" + try: + os.link(image, log) + except (OSError, NotImplementedError): + pytest.skip("hard links cannot be made here") + assert main(command(clean, "--log", str(log))) == 1 + assert "hard link" in _refused(capsys, image, before) + + +def test_an_output_named_in_another_case_is_refused(clean, capsys): + before = (clean.output / "run_manifest.json").read_bytes() + assert main(_run(clean, "--log", str(clean.output / "Run_Manifest.JSON"))) == 1 + _refused(capsys, clean.output / "run_manifest.json", before) + + +def test_the_experiment_folder_in_another_case_is_refused(clean, capsys): + # On a file system that ignores case (macOS, Windows) it is the same folder. + other = clean.experiment.with_name(clean.experiment.name.swapcase()) + if not other.is_dir(): + pytest.skip("this file system tells names apart by case") + log = other / "notes.txt" + assert main(_run(clean, "--log", str(log))) == 1 + assert "inside the experiment folder" in _refused(capsys, clean.experiment / "notes.txt") + + +def test_a_log_over_the_design_given_leaves_it_unchanged(clean, capsys, tmp_path): + # Review of #128: the confirmed design given with --design, outside the output folder, + # was appended to (and the run went on). + design = tmp_path / "my_design.csv" + design.write_bytes((clean.output / "experiment_design.csv").read_bytes()) + before = design.read_bytes() + assert main(_run(clean, "--design", str(design), "--log", str(design))) == 1 + assert "design file given with --design" in _refused(capsys, design, before) diff --git a/tests/test_links.py b/tests/test_links.py new file mode 100644 index 00000000..5abb49c3 --- /dev/null +++ b/tests/test_links.py @@ -0,0 +1,293 @@ +"""Symbolic links stay inside the experiment and masks folders (issue #81, owner policy "contain"). + +Every test here failed before the fix: links were followed wherever they led, two paths of +one file were two images, and a linked folder in the masks folder was skipped silently. +""" + +import os + +import numpy as np +import pytest + +from morphobricks_preprocessing import ( + RunInputs, + RunInputsError, + UnrecordedFilesError, + convert_experiment, + prefill_mapping, + run_experiment, +) +from morphobricks_preprocessing.discovery import discover_experiment +from morphobricks_preprocessing.run import preflight +from morphobricks_preprocessing.testing import make_clean_run +from tiff_tree import write_data + + +def _can_link(folder): + try: + os.symlink(folder, folder / "probe") + except (OSError, NotImplementedError): + return False + (folder / "probe").unlink() + return True + + +@pytest.fixture +def tree(tmp_path): + """An experiment of two leaf folders, and a folder outside it.""" + if not _can_link(tmp_path): + pytest.skip("symbolic links cannot be made here (Windows without the privilege)") + root, outside = tmp_path / "exp", tmp_path / "outside" + for rel in ("ctrl/rep1/a.tif", "drug/rep1/a.tif"): + write_data(root / rel, np.zeros((4, 4), np.uint8), "YX") + write_data(outside / "far.tif", np.ones((4, 4), np.uint8), "YX") + return root, outside + + +def _codes(issues): + return sorted(issue.code for issue in issues if issue.is_error) + + +def _issue(issues, code): + return next(issue for issue in issues if issue.code == code) + + +def test_a_file_link_out_of_the_experiment_is_refused_and_not_listed(tree): + root, outside = tree + (root / "ctrl/rep1/b.tif").symlink_to(outside / "far.tif") + found = discover_experiment(root) + assert _codes(found.issues) == ["link_outside_folder"] + issue = _issue(found.issues, "link_outside_folder") + assert issue.paths == (f"ctrl/rep1/b.tif -> {(outside / 'far.tif').resolve()}",) + assert "ctrl/rep1/b.tif" not in found.images + + +def test_a_folder_link_out_of_the_experiment_is_refused_and_not_entered(tree): + root, outside = tree + (root / "ext").symlink_to(outside, target_is_directory=True) + found = discover_experiment(root) + assert _codes(found.issues) == ["link_outside_folder"] + assert _issue(found.issues, "link_outside_folder").paths == (f"ext -> {outside.resolve()}",) + assert found.images == ("ctrl/rep1/a.tif", "drug/rep1/a.tif") + + +def test_a_broken_link_out_of_the_experiment_is_refused(tree): + root, outside = tree + (root / "ctrl/rep1/b.tif").symlink_to(outside / "gone.tif") + assert _codes(discover_experiment(root).issues) == ["link_outside_folder"] + + +def test_links_that_stay_inside_are_followed(tree): + root, _ = tree + write_data(root / ".raw/b.tif", np.zeros((4, 4), np.uint8), "YX") # a hidden folder + write_data(root / ".raw/rep2/c.tif", np.zeros((4, 4), np.uint8), "YX") + (root / "ctrl/rep1/b.tif").symlink_to(root / ".raw/b.tif") + (root / "drug/rep2").symlink_to(root / ".raw/rep2", target_is_directory=True) + found = discover_experiment(root) + assert found.ok and not found.issues + assert found.images == ( + "ctrl/rep1/a.tif", + "ctrl/rep1/b.tif", + "drug/rep1/a.tif", + "drug/rep2/c.tif", + ) + + +def test_an_image_found_through_a_link_and_its_target_is_refused_naming_both(tree): + root, _ = tree + (root / "drug/rep1/b.tif").symlink_to(root / "ctrl/rep1/a.tif") + found = discover_experiment(root) + assert _codes(found.issues) == ["linked_twice"] + assert _issue(found.issues, "linked_twice").paths == ( + "drug/rep1/b.tif (the same file as ctrl/rep1/a.tif)", + ) + + +def test_a_linked_folder_holding_the_same_images_is_refused(tree): + root, _ = tree + (root / "drug/rep2").symlink_to(root / "ctrl/rep1", target_is_directory=True) + found = discover_experiment(root) + assert _codes(found.issues) == ["linked_twice"] + assert "drug/rep2/a.tif (the same file as ctrl/rep1/a.tif)" in found.issues[0].paths + + +def test_a_link_back_to_a_folder_above_is_a_loop(tree): + root, _ = tree + (root / "ctrl/loop").symlink_to(root, target_is_directory=True) + found = discover_experiment(root) + assert "link_loop" in _codes(found.issues) + assert _issue(found.issues, "link_loop").paths == (f"ctrl/loop -> {root.resolve()}",) + + +def test_a_run_with_a_link_out_is_refused_before_any_pixel_is_read(tree, tmp_path): + root, outside = tree + (root / "ctrl/rep1/b.tif").symlink_to(outside / "far.tif") + result = run_experiment(root, prefill_mapping("YX"), output_folder=tmp_path / "out") + assert not result.written + assert "link_outside_folder" in _codes(result.issues) + + +def test_a_conversion_with_a_link_out_is_refused(tree, tmp_path): + root, outside = tree + (root / "ctrl/rep1/b.tif").symlink_to(outside / "far.tif") + result = convert_experiment(root, prefill_mapping("YX"), tmp_path / "copies") + assert not result.written + assert "link_outside_folder" in _codes(result.issues) + assert not (tmp_path / "copies" / "ctrl").exists() + + +def test_a_link_out_of_an_ome_zarr_store_is_refused(tree): + pytest.importorskip("zarr") + from zarr_tree import write_ngff + + root, outside = tree + store = write_ngff(root / "ctrl/rep1/z.ome.zarr", np.zeros((1, 1, 4, 4), np.uint8)) + (store / "extra").symlink_to(outside, target_is_directory=True) + found = discover_experiment(root) + assert _codes(found.issues) == ["link_outside_folder"] + assert _issue(found.issues, "link_outside_folder").paths == ( + f"ctrl/rep1/z.ome.zarr/extra -> {outside.resolve()}", + ) + + +def test_a_linked_ome_zarr_store_out_of_the_experiment_is_refused(tree): + pytest.importorskip("zarr") + from zarr_tree import write_ngff + + root, outside = tree + write_ngff(outside / "z.ome.zarr", np.zeros((1, 1, 4, 4), np.uint8)) + (root / "ctrl/rep1/z.ome.zarr").symlink_to(outside / "z.ome.zarr", target_is_directory=True) + found = discover_experiment(root) + assert _codes(found.issues) == ["link_outside_folder"] + assert "ctrl/rep1/z.ome.zarr" not in found.images + + +@pytest.fixture +def clean(tmp_path): + if not _can_link(tmp_path): + pytest.skip("symbolic links cannot be made here (Windows without the privilege)") + return make_clean_run(tmp_path / "run", axes="YX", shape=(16, 20), mask_types=("Cells",)) + + +def test_a_mask_linked_from_outside_the_masks_folder_is_refused(clean): + mask = clean.masks / "Cells/ctrl/rep1/a.tif" + real = clean.masks.parent / "elsewhere.tif" + mask.rename(real) + mask.symlink_to(real) + pre = preflight(clean.experiment, clean.output, masks_folder=clean.masks) + issue = _issue(pre.masks.issues, "link_outside_folder") + assert issue.paths == (f"Cells/ctrl/rep1/a.tif -> {real.resolve()}",) + assert "masks folder" in issue.message + + +def test_a_linked_folder_inside_the_masks_folder_is_followed(clean): + # Before the fix the masks below a linked folder were skipped without a word. + folder = clean.masks / "Cells/drug" + folder.rename(clean.masks / ".drug") + folder.symlink_to(clean.masks / ".drug", target_is_directory=True) + pre = preflight(clean.experiment, clean.output, masks_folder=clean.masks) + paths = sorted(mask.path for mask in pre.masks.files) + assert "Cells/drug/rep1/a.tif" in paths and "Cells/drug/rep1/b.tif" in paths + assert not [issue for issue in pre.masks.issues if issue.is_error] + + +def test_run_inputs_refuse_a_link_out_added_after_the_run(clean, tmp_path): + outside = tmp_path / "far.tif" + write_data(outside, np.zeros((16, 20), np.uint16), "YX") + (clean.experiment / "ctrl/rep1/c.tif").symlink_to(outside) + with pytest.raises(RunInputsError) as raised: + RunInputs.open(clean.output) + assert not isinstance(raised.value, UnrecordedFilesError) + assert f"ctrl/rep1/c.tif -> {outside.resolve()}" in str(raised.value) + + +def test_run_inputs_refuse_a_mask_link_out_added_after_the_run(clean, tmp_path): + outside = tmp_path / "far.tif" + (clean.masks / "Cells/ctrl/rep1/a.tif").rename(outside) + (clean.masks / "Cells/ctrl/rep1/a.tif").symlink_to(outside) + with pytest.raises(RunInputsError, match="link"): + RunInputs.open(clean.output) + + +def test_a_link_back_inside_an_ome_zarr_store_is_a_loop(tree): + # Copying the store (convert) follows its links: this one would never end. + pytest.importorskip("zarr") + from zarr_tree import write_ngff + + root, _ = tree + store = write_ngff(root / "ctrl/rep1/z.ome.zarr", np.zeros((1, 1, 4, 4), np.uint8)) + (store / "0" / "again").symlink_to(store, target_is_directory=True) + found = discover_experiment(root) + assert _codes(found.issues) == ["link_loop"] + assert _issue(found.issues, "link_loop").paths == ( + f"ctrl/rep1/z.ome.zarr/0/again -> {store.resolve()}", + ) + + +def test_a_masks_folder_given_through_a_link_keeps_its_links_inside(clean, tmp_path): + # The masks folder as RunInputs gives it may not be resolved: links are held to its real path. + from morphobricks_preprocessing.masks.files import _folder_masks + + (clean.masks / "Cells/drug").rename(clean.masks / ".drug") + (clean.masks / "Cells/drug").symlink_to(clean.masks / ".drug", target_is_directory=True) + alias = tmp_path / "alias" + alias.symlink_to(clean.masks, target_is_directory=True) + discovery = discover_experiment(clean.experiment) + found, issues = _folder_masks(alias, discovery, discovery.images) + assert not [issue for issue in issues if issue.is_error] + assert "Cells/drug/rep1/a.tif" in {mask.path for mask in found} + + +def _can_hard_link(folder): + (folder / "probe-source").write_bytes(b"") + try: + os.link(folder / "probe-source", folder / "probe-link") + except (OSError, NotImplementedError): + return False + finally: + for name in ("probe-source", "probe-link"): + (folder / name).unlink(missing_ok=True) + return True + + +def test_a_hard_link_to_an_image_is_refused_naming_both(tmp_path): + # Owner, 2 November 2026: one device and inode reached by two names is one file. + if not _can_hard_link(tmp_path): + pytest.skip("hard links cannot be made here") + root = tmp_path / "exp" + for rel in ("ctrl/rep1/a.tif", "drug/rep1/a.tif"): + write_data(root / rel, np.zeros((4, 4), np.uint8), "YX") + os.link(root / "ctrl/rep1/a.tif", root / "drug/rep1/b.tif") + found = discover_experiment(root) + assert _codes(found.issues) == ["linked_twice"] + assert _issue(found.issues, "linked_twice").paths == ( + "drug/rep1/b.tif (the same file as ctrl/rep1/a.tif)", + ) + result = run_experiment(root, prefill_mapping("YX"), output_folder=tmp_path / "out") + assert not result.written + assert "linked_twice" in _codes(result.issues) + + +def test_a_hard_link_between_masks_is_refused(tmp_path): + if not _can_hard_link(tmp_path): + pytest.skip("hard links cannot be made here") + clean = make_clean_run(tmp_path / "run", axes="YX", shape=(16, 20), mask_types=("Cells",)) + mask = clean.masks / "Cells/ctrl/rep1/b.tif" + mask.unlink() + os.link(clean.masks / "Cells/ctrl/rep1/a.tif", mask) + pre = preflight(clean.experiment, clean.output, masks_folder=clean.masks) + assert _issue(pre.masks.issues, "linked_twice").paths == ( + "Cells/ctrl/rep1/b.tif (the same file as Cells/ctrl/rep1/a.tif)", + ) + with pytest.raises(RunInputsError): # refused: the mask changed since the run + RunInputs.open(clean.output) + + +def test_a_broken_link_in_the_masks_folder_is_named(clean): + # Before, it was skipped: its image only looked as if it had no mask (mask_missing). + mask = clean.masks / "Cells/ctrl/rep1/a.tif" + mask.unlink() + mask.symlink_to(clean.masks / "Cells/ctrl/rep1/gone.tif") + pre = preflight(clean.experiment, clean.output, masks_folder=clean.masks) + issue = _issue(pre.masks.issues, "unreadable_mask") + assert issue.is_error and issue.paths == ("Cells/ctrl/rep1/a.tif",) diff --git a/tools/code_rules_baseline.json b/tools/code_rules_baseline.json index 98f7a0fc..f79d873d 100644 --- a/tools/code_rules_baseline.json +++ b/tools/code_rules_baseline.json @@ -1,6 +1,6 @@ { "noqa": { - "ANN": 53, + "ANN": 52, "C901": 0, "PLR0913": 3, "PLR0915": 0 @@ -18,7 +18,7 @@ "src/morphobricks_preprocessing/design.py: # lines (max #)": 926, "src/morphobricks_preprocessing/discovery.py _check_names: private function of # lines without a docstring": 14, "src/morphobricks_preprocessing/discovery.py finish: # executable lines (max #)": 145, - "src/morphobricks_preprocessing/discovery.py: # lines (max #)": 610, + "src/morphobricks_preprocessing/discovery.py: # lines (max #)": 609, "src/morphobricks_preprocessing/image_type.py _rgb_problem: private function of # lines without a docstring": 12, "src/morphobricks_preprocessing/manifest.py file_entry_for: # parameters (max #)": 9, "src/morphobricks_preprocessing/masks/files.py _duplicate_issues: private function of # lines without a docstring": 19,