Skip to content

Commit 39c36c4

Browse files
jawwad-aliclaude
andauthored
fix(workflows): report a falsy non-mapping overlay manifest as a shape error (#3884)
* fix(workflows): report a falsy non-mapping overlay manifest as a shape error `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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 21fb1bb commit 39c36c4

2 files changed

Lines changed: 72 additions & 1 deletion

File tree

src/specify_cli/workflows/overlays/layer_sources.py

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,11 +152,27 @@ def collect(self, workflow_id: str, *, include_disabled: bool = False) -> list[L
152152
if path.is_symlink():
153153
raise OverlayLoadError(path, ["Symlinked overlay files are not allowed"])
154154
try:
155-
data = yaml.safe_load(path.read_text(encoding="utf-8")) or {}
155+
text = path.read_text(encoding="utf-8")
156+
# ``safe_load`` returns None for BOTH an empty document and an
157+
# explicit null scalar (``null``, ``~``, ``Null``, ``NULL``), so
158+
# it cannot tell them apart on its own. ``compose`` yields no
159+
# node only for a genuinely empty document.
160+
is_empty_document = yaml.compose(text) is None
161+
data = yaml.safe_load(text)
156162
except yaml.YAMLError as exc:
157163
raise OverlayLoadError(path, [f"Invalid YAML: {exc}"]) from exc
158164
except (OSError, UnicodeDecodeError) as exc:
159165
raise OverlayLoadError(path, [f"Cannot load overlay: {exc}"]) from exc
166+
# Only a genuinely EMPTY document becomes an empty mapping, so its
167+
# missing-field errors are reported. Every non-mapping document --
168+
# including an explicit ``null``/``~`` and the falsy shapes ``[]``,
169+
# ``false``, ``0``, ``''`` that the previous ``or {}`` masked -- must
170+
# reach ``validate_overlay_yaml`` unchanged so it reports the wrong
171+
# manifest shape, like the truthy twins (``- a``, ``hello``) already
172+
# do. The sibling reader for these same files, ``_read_overlay`` in
173+
# overlays/_commands.py, does not coerce either.
174+
if is_empty_document:
175+
data = {}
160176
if (
161177
not include_disabled
162178
and isinstance(data, dict)

tests/workflows/test_overlay_layer_sources.py

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,61 @@ def _write_overlay_file(project_dir: Path, workflow_id: str, overlay_id: str, da
3030
return path
3131

3232

33+
class TestProjectOverlaySourceManifestShape:
34+
"""A non-mapping overlay manifest is reported as a shape error."""
35+
36+
@pytest.mark.parametrize(
37+
"content", ["[]", "false", "0", "''", "null", "~", "NULL"]
38+
)
39+
def test_falsy_non_mapping_manifest_reports_shape_error(
40+
self, project_dir: Path, content: str
41+
) -> None:
42+
"""Every non-mapping document reports the mapping-shape error.
43+
44+
`validate_overlay_yaml` opens with an `isinstance(data, dict)` check, so a
45+
truthy non-mapping (`- a`, `hello`) correctly reports "Overlay manifest
46+
must be a mapping." Two things masked that for other documents:
47+
48+
* `yaml.safe_load(...) or {}` replaced the falsy shapes `[]`, `false`,
49+
`0` and `''` with an empty mapping.
50+
* `safe_load` returns `None` for an explicit null scalar (`null`, `~`,
51+
`NULL`) as well as for an empty document, so a `data is None` check
52+
swallowed those too.
53+
54+
Both now reach the validator unchanged; only a genuinely empty document
55+
is normalised to `{}` (pinned separately below), using `yaml.compose`,
56+
which yields no node only for an empty document.
57+
"""
58+
ov_dir = project_dir / ".specify" / "workflows" / "overlays" / "wf"
59+
ov_dir.mkdir(parents=True, exist_ok=True)
60+
(ov_dir / "ov.yml").write_text(content, encoding="utf-8")
61+
62+
source = ProjectOverlaySource(project_dir)
63+
with pytest.raises(OverlayLoadError) as exc_info:
64+
source.collect("wf")
65+
66+
assert exc_info.value.errors == ["Overlay manifest must be a mapping."], (
67+
exc_info.value.errors
68+
)
69+
70+
def test_empty_document_still_reports_missing_fields(
71+
self, project_dir: Path
72+
) -> None:
73+
"""An empty document is not a wrong shape — it is a mapping with no keys,
74+
so the missing-field errors must still be what is reported."""
75+
ov_dir = project_dir / ".specify" / "workflows" / "overlays" / "wf"
76+
ov_dir.mkdir(parents=True, exist_ok=True)
77+
(ov_dir / "ov.yml").write_text("", encoding="utf-8")
78+
79+
source = ProjectOverlaySource(project_dir)
80+
with pytest.raises(OverlayLoadError) as exc_info:
81+
source.collect("wf")
82+
83+
assert any("is required" in err for err in exc_info.value.errors), (
84+
exc_info.value.errors
85+
)
86+
87+
3388
class TestProjectOverlaySourceFileReadErrors:
3489
"""File-read errors must be wrapped in OverlayLoadError, not leaked as raw tracebacks."""
3590

0 commit comments

Comments
 (0)