From e269c0267e5b251eb11c89d469513cd282904160 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 31 Jul 2026 11:33:33 +0500 Subject: [PATCH 1/2] fix(workflows): report a falsy non-mapping overlay manifest as a shape error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ProjectOverlaySource.collect` did `yaml.safe_load(...) or {}`. `validate_overlay_yaml` opens with an `isinstance(data, dict)` check, so a truthy non-mapping is reported correctly — but `or {}` replaced the falsy non-mappings with an empty mapping first, so those files were reported as three bogus missing-field errors instead of the wrong shape: '- a' -> ['Overlay manifest must be a mapping.'] 'hello' -> ['Overlay manifest must be a mapping.'] '[]' -> ["Overlay 'id' is required...", "'extends' is required...", "'edits' is required..."] 'false' -> same three '0' -> same three "''" -> same three The sibling reader for these same files in the same package, `_read_overlay` in overlays/_commands.py, does not coerce. Only an empty document (None) now becomes an empty mapping, so a genuinely empty overlay still reports its missing fields. Co-Authored-By: Claude Opus 5 (1M context) --- .../workflows/overlays/layer_sources.py | 10 +++- tests/workflows/test_overlay_layer_sources.py | 47 +++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/overlays/layer_sources.py b/src/specify_cli/workflows/overlays/layer_sources.py index e51aaf70dd..b38d050f3e 100644 --- a/src/specify_cli/workflows/overlays/layer_sources.py +++ b/src/specify_cli/workflows/overlays/layer_sources.py @@ -152,11 +152,19 @@ def collect(self, workflow_id: str, *, include_disabled: bool = False) -> list[L if path.is_symlink(): raise OverlayLoadError(path, ["Symlinked overlay files are not allowed"]) try: - data = yaml.safe_load(path.read_text(encoding="utf-8")) or {} + data = yaml.safe_load(path.read_text(encoding="utf-8")) except yaml.YAMLError as exc: raise OverlayLoadError(path, [f"Invalid YAML: {exc}"]) from exc except (OSError, UnicodeDecodeError) as exc: raise OverlayLoadError(path, [f"Cannot load overlay: {exc}"]) from exc + # Only an empty document (``None``) becomes an empty mapping, so the + # missing-field errors are reported. ``or {}`` also masked the FALSY + # non-mappings (``[]``, ``false``, ``0``, ``''``), which must be + # reported as the wrong manifest shape like their truthy twins + # (``- a``, ``hello``) already are. The sibling reader for these same + # files, ``_read_overlay`` in overlays/_commands.py, does not coerce. + if data is None: + data = {} if ( not include_disabled and isinstance(data, dict) diff --git a/tests/workflows/test_overlay_layer_sources.py b/tests/workflows/test_overlay_layer_sources.py index fc6e30ef3f..c597915d94 100644 --- a/tests/workflows/test_overlay_layer_sources.py +++ b/tests/workflows/test_overlay_layer_sources.py @@ -30,6 +30,53 @@ def _write_overlay_file(project_dir: Path, workflow_id: str, overlay_id: str, da return path +class TestProjectOverlaySourceManifestShape: + """A non-mapping overlay manifest is reported as a shape error.""" + + @pytest.mark.parametrize("content", ["[]", "false", "0", "''"]) + def test_falsy_non_mapping_manifest_reports_shape_error( + self, project_dir: Path, content: str + ) -> None: + """`or {}` masked the falsy non-mappings. + + `validate_overlay_yaml` opens with a `isinstance(data, dict)` check, so a + truthy non-mapping (`- a`, `hello`) correctly reports "Overlay manifest + must be a mapping." But `yaml.safe_load(...) or {}` replaced `[]`, + `false`, `0` and `''` with an empty mapping first, so those files were + reported as three bogus missing-field errors instead of the wrong shape. + The sibling reader for these same files, `_read_overlay` in + `overlays/_commands.py`, does not coerce. + """ + ov_dir = project_dir / ".specify" / "workflows" / "overlays" / "wf" + ov_dir.mkdir(parents=True, exist_ok=True) + (ov_dir / "ov.yml").write_text(content, encoding="utf-8") + + source = ProjectOverlaySource(project_dir) + with pytest.raises(OverlayLoadError) as exc_info: + source.collect("wf") + + assert exc_info.value.errors == ["Overlay manifest must be a mapping."], ( + exc_info.value.errors + ) + + def test_empty_document_still_reports_missing_fields( + self, project_dir: Path + ) -> None: + """An empty document is not a wrong shape — it is a mapping with no keys, + so the missing-field errors must still be what is reported.""" + ov_dir = project_dir / ".specify" / "workflows" / "overlays" / "wf" + ov_dir.mkdir(parents=True, exist_ok=True) + (ov_dir / "ov.yml").write_text("", encoding="utf-8") + + source = ProjectOverlaySource(project_dir) + with pytest.raises(OverlayLoadError) as exc_info: + source.collect("wf") + + assert any("is required" in err for err in exc_info.value.errors), ( + exc_info.value.errors + ) + + class TestProjectOverlaySourceFileReadErrors: """File-read errors must be wrapped in OverlayLoadError, not leaked as raw tracebacks.""" From 31d69e7f8a307a945e5976af8b7d53e2109deaf7 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 31 Jul 2026 22:45:43 +0500 Subject: [PATCH 2/2] fix(workflows): distinguish an empty document from an explicit YAML null Review catch: `safe_load` returns None for an explicit null scalar (`null`, `~`, `Null`, `NULL`) as well as for an empty document, so the `data is None` normalization still converted those manifests to `{}` and they still received missing-field errors instead of the mapping-shape error. Use `yaml.compose`, which yields no node only for a genuinely empty document, to tell the two apart. Measured: empty doc -> missing-field (correct) explicit null -> SHAPE explicit ~ -> SHAPE NULL -> SHAPE [] false 0 '' -> SHAPE - a / hello -> SHAPE Extends the parametrized cases with null/~/NULL, and corrects the article before `isinstance` in the docstring. Co-Authored-By: Claude Opus 5 (1M context) --- .../workflows/overlays/layer_sources.py | 24 ++++++++++++------- tests/workflows/test_overlay_layer_sources.py | 24 ++++++++++++------- 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/src/specify_cli/workflows/overlays/layer_sources.py b/src/specify_cli/workflows/overlays/layer_sources.py index b38d050f3e..a62cef9340 100644 --- a/src/specify_cli/workflows/overlays/layer_sources.py +++ b/src/specify_cli/workflows/overlays/layer_sources.py @@ -152,18 +152,26 @@ def collect(self, workflow_id: str, *, include_disabled: bool = False) -> list[L if path.is_symlink(): raise OverlayLoadError(path, ["Symlinked overlay files are not allowed"]) try: - data = yaml.safe_load(path.read_text(encoding="utf-8")) + text = path.read_text(encoding="utf-8") + # ``safe_load`` returns None for BOTH an empty document and an + # explicit null scalar (``null``, ``~``, ``Null``, ``NULL``), so + # it cannot tell them apart on its own. ``compose`` yields no + # node only for a genuinely empty document. + is_empty_document = yaml.compose(text) is None + data = yaml.safe_load(text) except yaml.YAMLError as exc: raise OverlayLoadError(path, [f"Invalid YAML: {exc}"]) from exc except (OSError, UnicodeDecodeError) as exc: raise OverlayLoadError(path, [f"Cannot load overlay: {exc}"]) from exc - # Only an empty document (``None``) becomes an empty mapping, so the - # missing-field errors are reported. ``or {}`` also masked the FALSY - # non-mappings (``[]``, ``false``, ``0``, ``''``), which must be - # reported as the wrong manifest shape like their truthy twins - # (``- a``, ``hello``) already are. The sibling reader for these same - # files, ``_read_overlay`` in overlays/_commands.py, does not coerce. - if data is None: + # Only a genuinely EMPTY document becomes an empty mapping, so its + # missing-field errors are reported. Every non-mapping document -- + # including an explicit ``null``/``~`` and the falsy shapes ``[]``, + # ``false``, ``0``, ``''`` that the previous ``or {}`` masked -- must + # reach ``validate_overlay_yaml`` unchanged so it reports the wrong + # manifest shape, like the truthy twins (``- a``, ``hello``) already + # do. The sibling reader for these same files, ``_read_overlay`` in + # overlays/_commands.py, does not coerce either. + if is_empty_document: data = {} if ( not include_disabled diff --git a/tests/workflows/test_overlay_layer_sources.py b/tests/workflows/test_overlay_layer_sources.py index c597915d94..d852cb7622 100644 --- a/tests/workflows/test_overlay_layer_sources.py +++ b/tests/workflows/test_overlay_layer_sources.py @@ -33,19 +33,27 @@ def _write_overlay_file(project_dir: Path, workflow_id: str, overlay_id: str, da class TestProjectOverlaySourceManifestShape: """A non-mapping overlay manifest is reported as a shape error.""" - @pytest.mark.parametrize("content", ["[]", "false", "0", "''"]) + @pytest.mark.parametrize( + "content", ["[]", "false", "0", "''", "null", "~", "NULL"] + ) def test_falsy_non_mapping_manifest_reports_shape_error( self, project_dir: Path, content: str ) -> None: - """`or {}` masked the falsy non-mappings. + """Every non-mapping document reports the mapping-shape error. - `validate_overlay_yaml` opens with a `isinstance(data, dict)` check, so a + `validate_overlay_yaml` opens with an `isinstance(data, dict)` check, so a truthy non-mapping (`- a`, `hello`) correctly reports "Overlay manifest - must be a mapping." But `yaml.safe_load(...) or {}` replaced `[]`, - `false`, `0` and `''` with an empty mapping first, so those files were - reported as three bogus missing-field errors instead of the wrong shape. - The sibling reader for these same files, `_read_overlay` in - `overlays/_commands.py`, does not coerce. + must be a mapping." Two things masked that for other documents: + + * `yaml.safe_load(...) or {}` replaced the falsy shapes `[]`, `false`, + `0` and `''` with an empty mapping. + * `safe_load` returns `None` for an explicit null scalar (`null`, `~`, + `NULL`) as well as for an empty document, so a `data is None` check + swallowed those too. + + Both now reach the validator unchanged; only a genuinely empty document + is normalised to `{}` (pinned separately below), using `yaml.compose`, + which yields no node only for an empty document. """ ov_dir = project_dir / ".specify" / "workflows" / "overlays" / "wf" ov_dir.mkdir(parents=True, exist_ok=True)