fix(py,ts,mcp): make validation mean the same thing at every version and in both ports

mcp/ngff_zarr_mcp/tools.py

@@ -8,26 +8,24 @@
 
 from ngff_zarr import (  # type: ignore[import-untyped]
     Methods,
+    ValidationError,
     cli_input_to_ngff_image,
     detect_cli_io_backend,
     from_ome_zarr,
     to_multiscales,
     to_ome_zarr,
+    validate_structural,
 )
 
-# Import validation function if available
+# The schema pass needs the ngff-zarr[validate] extra. Where it is missing the
+# tool says the pass did not run, rather than calling a store valid on the
+# strength of a check it never made.
 try:
     from ngff_zarr import validate as validate_ngff
 except ImportError:
-    # Fallback if ngff-zarr schema validation is unavailable (no-op stub whose
-    # signature mirrors ngff_zarr.validate).
-    def validate_ngff(
-        ngff_dict: dict,
-        version: str = "0.4",
-        model: str = "image",
-        strict: bool = False,
-    ) -> None:
-        pass
+    # Rebinding the name to None is what the call site tests for; mypy sees the
+    # function type from the import above.
+    validate_ngff = None  # type: ignore[assignment]
 
 
 from .models import (
@@ -319,6 +317,12 @@ async def inspect_ome_zarr(store_path: str) -> StoreInfo:
         raise ValueError(f"Failed to inspect store: {str(e)}")
 
 
+_SCHEMA_PASS_SKIPPED = (
+    "Schema validation did not run: install the ngff-zarr[validate] extra to "
+    "check the metadata document against its JSON Schema."
+)
+
+
 async def validate_ome_zarr(store_path: str) -> ValidationResult:
     """Validate an OME-Zarr store."""
 
@@ -378,12 +382,40 @@ async def validate_ome_zarr(store_path: str) -> ValidationResult:
             except Exception:
                 version = "0.4"  # Default assumption
 
-            # Try ngff-zarr schema validation if available. ngff_zarr.validate
-            # expects the parsed NGFF metadata dict, not a store path.
+            # ngff_zarr.validate expects the parsed NGFF metadata dict, not a
+            # store path. A document the schema rejects is invalid, so the
+            # verdict belongs in errors and reaches the caller through `valid`.
+            if validate_ngff is None:
+                warnings.append(_SCHEMA_PASS_SKIPPED)
+            else:
+                try:
+                    validate_ngff(root_attrs, version=version or "0.4")
+                except ImportError:
+                    warnings.append(_SCHEMA_PASS_SKIPPED)
+                except ValueError as no_schema:
+                    # A version ngff-zarr bundles no schema tree for. That says
+                    # nothing about the document, so it is a pass that did not
+                    # run rather than a failure.
+                    warnings.append(f"Schema validation did not run: {no_schema}")
+                except Exception as validation_error:
+                    errors.append(f"Schema validation failed: {validation_error}")
+
+            # The structural rules carry the spec MUSTs no JSON Schema states,
+            # among them the finest-to-coarsest dataset order. They read the
+            # parsed model, so they run on the multiscales loaded above. The
+            # detected version is passed on: several rules are gated on it, and
+            # without it a 0.6 array coordinate system or an RFC-3 axis model is
+            # measured against the v0.4 caps and reported as a false failure.
             try:
-                validate_ngff(root_attrs, version=version or "0.4")
-            except Exception as validation_error:
-                warnings.append(f"NGFF validation warning: {str(validation_error)}")
+                validate_structural(
+                    multiscales.metadata,  # type: ignore[arg-type]
+                    version=version,
+                )
+            except ValidationError as structural_error:
+                errors.append(f"Structural validation failed: {structural_error}")
+            except ImportError:
+                # One rule, the RFC 4 orientation check, reaches for jsonschema.
+                warnings.append(_SCHEMA_PASS_SKIPPED)
 
         except Exception as e:
             errors.append(f"Failed to load as NGFF: {str(e)}")

mcp/tests/test_validate_ome_zarr.py

@@ -0,0 +1,87 @@
+# SPDX-FileCopyrightText: Copyright (c) Fideus Labs LLC
+# SPDX-License-Identifier: MIT
+"""``validate_ome_zarr`` reports a store the spec refuses as invalid.
+
+The tool runs two passes over a store: the JSON Schema of the store's own
+version, and the structural rules that carry the spec MUSTs no schema states.
+A failure in either is an error, so it reaches the caller through ``valid``.
+"""
+
+import json
+from pathlib import Path
+
+import numpy as np
+import pytest
+from ngff_zarr import to_multiscales, to_ngff_image, to_ome_zarr
+
+from ngff_zarr_mcp.tools import validate_ome_zarr
+
+VERSIONS = ["0.4", "0.5", "0.6"]
+
+
+def _write_store(path: Path, version: str) -> Path:
+    image = to_ngff_image(
+        np.zeros((32, 32), dtype=np.uint8), dims=["y", "x"], scale={"y": 1.0, "x": 1.0}
+    )
+    multiscales = to_multiscales(image, scale_factors=[2])
+    to_ome_zarr(str(path), multiscales, version=version)
+    return path
+
+
+def _root_document(store: Path) -> tuple[Path, dict]:
+    """The root attributes document and its path, whichever layout is on disk."""
+    v3 = store / "zarr.json"
+    if v3.exists():
+        return v3, json.loads(v3.read_text())
+    v2 = store / ".zattrs"
+    return v2, json.loads(v2.read_text())
+
+
+def _edit_multiscales(store: Path, mutate) -> None:
+    path, document = _root_document(store)
+    if "attributes" in document:
+        multiscales = document["attributes"]["ome"]["multiscales"]
+    else:
+        multiscales = document["multiscales"]
+    mutate(multiscales[0])
+    path.write_text(json.dumps(document))
+    consolidated = store / "zarr.json"
+    if consolidated.exists() and path == consolidated:
+        return
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("version", VERSIONS)
+async def test_a_valid_store_is_valid_at_every_version(tmp_path, version):
+    """Zarr v3 stores, which is every 0.5 and 0.6 store, get past the store check."""
+    store = _write_store(tmp_path / f"valid-{version}.zarr", version)
+
+    result = await validate_ome_zarr(str(store))
+
+    assert result.valid, result.errors
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("version", VERSIONS)
+async def test_datasets_ordered_coarsest_to_finest_are_refused(tmp_path, version):
+    """`dataset-order-highest-to-lowest`, a rule no JSON Schema states."""
+    store = _write_store(tmp_path / f"reversed-{version}.zarr", version)
+    _edit_multiscales(store, lambda ms: ms["datasets"].reverse())
+
+    result = await validate_ome_zarr(str(store))
+
+    assert not result.valid
+    assert any("dataset" in error.lower() for error in result.errors), result.errors
+
+
+@pytest.mark.asyncio
+async def test_a_document_the_schema_rejects_is_an_error_not_a_warning(tmp_path):
+    """A schema failure used to be demoted to a warning, leaving `valid` True."""
+    pytest.importorskip("jsonschema")
+    store = _write_store(tmp_path / "no-axes.zarr", "0.4")
+    _edit_multiscales(store, lambda ms: ms.pop("axes"))
+
+    result = await validate_ome_zarr(str(store))
+
+    assert not result.valid
+    assert result.errors

py/ngff_zarr/hcs.py

@@ -162,7 +162,11 @@ def get_well(self, row_name: str, column_name: str) -> Optional["HCSWell"]:
         # Cache wells to avoid reloading - using bounded cache now
         if well_path not in self._wells:
             hcs_well = HCSWell.from_store(
-                self.store, well_path, well_meta, self.image_cache_size
+                self.store,
+                well_path,
+                well_meta,
+                self.image_cache_size,
+                validate=self._validate,
             )
             if self._validate:
                 # Strict structural validation of the well's own metadata, in
@@ -215,11 +219,13 @@ def __init__(
         well_metadata: PlateWell,
         well_group_metadata: Well,
         image_cache_size: int | None = None,
+        validate: bool = False,
     ):
         self.store = store
         self.path = well_path
         self.plate_metadata = well_metadata
         self.metadata = well_group_metadata
+        self._validate = validate
 
         # Use bounded cache for images to prevent memory issues
         from .config import config
@@ -234,8 +240,14 @@ def from_store(
         well_path: str,
         well_metadata: PlateWell,
         image_cache_size: int | None = None,
+        validate: bool = False,
     ) -> "HCSWell":
-        """Load a well from a zarr store."""
+        """Load a well from a zarr store.
+
+        ``validate`` reaches the images this well reads: ``from_hcs_zarr
+        (validate=True)`` checks the plate and the well metadata, and the
+        images are part of what it was asked to check.
+        """
         # Dispatches on store type: local paths read through the compat
         # layer, bytes mappings (including ZipReadStore for .ozx) through
         # the store reader, other store objects through zarr-python.
@@ -288,7 +300,12 @@ def from_store(
         well_group_metadata = Well(images=images, version=version)
 
         return cls(
-            store, well_path, well_metadata, well_group_metadata, image_cache_size
+            store,
+            well_path,
+            well_metadata,
+            well_group_metadata,
+            image_cache_size,
+            validate=validate,
         )
 
     @property
@@ -317,18 +334,20 @@ def get_image(self, field_index: int = 0) -> NgffMultiscales | None:
                 # A view of the same remote store narrowed to this field's
                 # sub-hierarchy, sharing the obstore client.
                 self._images[image_path] = from_ome_zarr(
-                    self.store.with_prefix(image_path)
+                    self.store.with_prefix(image_path), validate=self._validate
                 )
             elif isinstance(self.store, ZipReadStore):
                 # A view of the same archive narrowed to this field's
                 # sub-hierarchy; from_ome_zarr reads it as a mapping store.
                 self._images[image_path] = from_ome_zarr(
-                    self.store.with_prefix(image_path)
+                    self.store.with_prefix(image_path), validate=self._validate
                 )
             elif isinstance(self.store, (str, Path)):
                 # If store is a path string, append the image path
                 full_image_path = Path(self.store) / self.path / image_meta.path
-                self._images[image_path] = from_ome_zarr(str(full_image_path))
+                self._images[image_path] = from_ome_zarr(
+                    str(full_image_path), validate=self._validate
+                )
             else:
                 raise TypeError(
                     "HCS plates are read from local directory paths, remote "