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>
This commit is contained in:
@@ -152,11 +152,27 @@ class ProjectOverlaySource:
|
||||
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 {}
|
||||
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 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
|
||||
and isinstance(data, dict)
|
||||
|
||||
@@ -30,6 +30,61 @@ 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", "''", "null", "~", "NULL"]
|
||||
)
|
||||
def test_falsy_non_mapping_manifest_reports_shape_error(
|
||||
self, project_dir: Path, content: str
|
||||
) -> None:
|
||||
"""Every non-mapping document reports the mapping-shape error.
|
||||
|
||||
`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." 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)
|
||||
(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."""
|
||||
|
||||
|
||||
Reference in New Issue
Block a user