Files
spec-kit/tests/workflows/test_overlay_layer_sources.py
Ali jawwadandClaude Opus 5 39c36c4144 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>
2026-08-17 07:39:27 -05:00

328 lines
13 KiB
Python

"""Tests for ProjectOverlaySource and BaseWorkflowSource."""
from __future__ import annotations
from pathlib import Path
from unittest.mock import patch
import pytest
import yaml
from specify_cli.workflows.overlays.layer_sources import (
BaseWorkflowSource,
OverlayLoadError,
ProjectOverlaySource,
)
@pytest.fixture
def project_dir(tmp_path: Path) -> Path:
workflows_dir = tmp_path / ".specify" / "workflows"
workflows_dir.mkdir(parents=True, exist_ok=True)
return tmp_path
def _write_overlay_file(project_dir: Path, workflow_id: str, overlay_id: str, data: dict) -> Path:
ov_dir = project_dir / ".specify" / "workflows" / "overlays" / workflow_id
ov_dir.mkdir(parents=True, exist_ok=True)
path = ov_dir / f"{overlay_id}.yml"
path.write_text(yaml.safe_dump(data), encoding="utf-8")
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."""
def test_oserror_raises_overlay_load_error(self, project_dir: Path) -> None:
"""An OSError from read_text (e.g. permission denied) is wrapped in OverlayLoadError."""
_write_overlay_file(
project_dir,
"wf",
"ov1",
{"id": "ov1", "extends": "wf", "priority": 5, "edits": []},
)
source = ProjectOverlaySource(project_dir)
with patch.object(Path, "read_text", side_effect=OSError("Permission denied")):
with pytest.raises(OverlayLoadError) as exc_info:
source.collect("wf")
assert exc_info.value.errors, "OverlayLoadError must carry a non-empty errors list"
def test_unicode_error_raises_overlay_load_error(self, project_dir: Path) -> None:
"""A file containing non-UTF-8 bytes raises OverlayLoadError, not UnicodeDecodeError."""
ov_dir = project_dir / ".specify" / "workflows" / "overlays" / "wf"
ov_dir.mkdir(parents=True, exist_ok=True)
# Write raw invalid UTF-8 bytes directly so read_text(encoding="utf-8") fails.
bad_file = ov_dir / "bad.yml"
bad_file.write_bytes(b"\xff\xfe invalid utf-8")
source = ProjectOverlaySource(project_dir)
with pytest.raises(OverlayLoadError) as exc_info:
source.collect("wf")
assert exc_info.value.errors, "OverlayLoadError must carry a non-empty errors list"
_UNSAFE_IDS = [
"../outside",
"../../escape",
"nested/workflow",
"wf\n",
"overlays",
"runs",
"steps",
"",
"/absolute",
"UPPER",
"has space",
]
class TestProjectOverlaySourceIdValidation:
"""ProjectOverlaySource.collect() must reject unsafe IDs before path construction."""
@pytest.mark.parametrize("workflow_id", _UNSAFE_IDS)
def test_rejects_unsafe_id(self, project_dir: Path, workflow_id: str) -> None:
source = ProjectOverlaySource(project_dir)
with pytest.raises(OverlayLoadError, match="Invalid workflow ID"):
source.collect(workflow_id)
@pytest.mark.parametrize("workflow_id", _UNSAFE_IDS)
def test_does_not_access_filesystem_for_unsafe_id(
self, project_dir: Path, workflow_id: str
) -> None:
"""No directory walk or file read should happen for an invalid ID."""
source = ProjectOverlaySource(project_dir)
with patch.object(Path, "iterdir", side_effect=AssertionError("iterdir called")):
with pytest.raises(OverlayLoadError, match="Invalid workflow ID"):
source.collect(workflow_id)
class TestProjectOverlaySourceContainment:
"""ProjectOverlaySource.collect() must enforce containment of the workflow overlay dir."""
def test_rejects_symlinked_workflow_overlay_dir(self, project_dir: Path, tmp_path: Path) -> None:
"""A symlinked per-workflow overlay directory must be rejected."""
real_dir = tmp_path / "real-overlay"
real_dir.mkdir()
overlay_root = project_dir / ".specify" / "workflows" / "overlays"
overlay_root.mkdir(parents=True, exist_ok=True)
link = overlay_root / "wf"
link.symlink_to(real_dir)
source = ProjectOverlaySource(project_dir)
with pytest.raises(OverlayLoadError, match="Symlinked overlay directories are not allowed"):
source.collect("wf")
def test_rejects_workflow_overlay_dir_escaping_root(
self, project_dir: Path, tmp_path: Path
) -> None:
"""A workflow overlay dir that resolves outside the overlay root must be rejected.
This requires the ID itself to pass validation but the resolved path to escape —
which is possible if the overlay root itself is a junction/mount that resolves
outside the project root; or in edge cases on case-insensitive file systems.
We simulate it by patching Path.resolve to return an outside path.
"""
overlay_root = project_dir / ".specify" / "workflows" / "overlays"
overlay_root.mkdir(parents=True, exist_ok=True)
workflow_overlay_dir = overlay_root / "wf"
workflow_overlay_dir.mkdir()
outside = tmp_path / "outside" / "wf"
outside.mkdir(parents=True)
original_resolve = Path.resolve
def fake_resolve(self: Path, **kwargs: object) -> Path:
if self == workflow_overlay_dir:
return outside
return original_resolve(self, **kwargs)
source = ProjectOverlaySource(project_dir)
with patch.object(Path, "resolve", fake_resolve):
with pytest.raises(OverlayLoadError, match="Path traversal detected"):
source.collect("wf")
class TestProjectOverlaySourceDisabledFiltering:
"""ProjectOverlaySource.collect() should expose disabled entries only on opt-in."""
def test_skips_disabled_by_default(self, project_dir: Path) -> None:
_write_overlay_file(
project_dir,
"wf",
"ov1",
{
"id": "ov1",
"extends": "wf",
"priority": 5,
"enabled": False,
"edits": [{"remove": "a"}],
},
)
source = ProjectOverlaySource(project_dir)
assert source.collect("wf") == []
def test_can_include_disabled_for_management_views(self, project_dir: Path) -> None:
_write_overlay_file(
project_dir,
"wf",
"ov1",
{
"id": "ov1",
"extends": "wf",
"priority": 5,
"enabled": False,
"edits": [{"remove": "a"}],
},
)
source = ProjectOverlaySource(project_dir)
layers = source.collect("wf", include_disabled=True)
assert [layer.content.id for layer in layers] == ["ov1"]
assert layers[0].content.enabled is False
def test_skips_invalid_disabled_overlay_during_resolution(self, project_dir: Path) -> None:
_write_overlay_file(
project_dir,
"wf",
"disabled",
{
"id": "disabled",
"extends": "wf",
"enabled": False,
"edits": "not-a-list",
},
)
source = ProjectOverlaySource(project_dir)
assert source.collect("wf") == []
with pytest.raises(OverlayLoadError, match="edits"):
source.collect("wf", include_disabled=True)
def test_rejects_duplicate_manifest_ids(self, project_dir: Path) -> None:
data = {
"id": "duplicate",
"extends": "wf",
"edits": [{"remove": "a"}],
}
_write_overlay_file(project_dir, "wf", "first", data)
_write_overlay_file(project_dir, "wf", "second", data)
with pytest.raises(OverlayLoadError, match="Duplicate overlay id"):
ProjectOverlaySource(project_dir).collect("wf")
class TestBaseWorkflowSourceIdValidation:
"""BaseWorkflowSource.collect() must reject unsafe IDs before path construction."""
@pytest.mark.parametrize("workflow_id", _UNSAFE_IDS)
def test_rejects_unsafe_id(self, project_dir: Path, workflow_id: str) -> None:
source = BaseWorkflowSource(project_dir)
with pytest.raises(OverlayLoadError, match="Invalid workflow ID"):
source.collect(workflow_id)
class TestBaseWorkflowSourceContainment:
"""BaseWorkflowSource.collect() must enforce the same checks as _safe_workflow_id_dir."""
def test_rejects_symlinked_workflow_dir(self, project_dir: Path, tmp_path: Path) -> None:
"""A symlinked workflow directory must be rejected."""
real_dir = tmp_path / "real-wf"
real_dir.mkdir()
(real_dir / "workflow.yml").write_text("schema_version: '1.0'\n", encoding="utf-8")
workflows_dir = project_dir / ".specify" / "workflows"
workflows_dir.mkdir(parents=True, exist_ok=True)
link = workflows_dir / "wf"
link.symlink_to(real_dir)
source = BaseWorkflowSource(project_dir)
with pytest.raises(OverlayLoadError, match="Symlinked overlay directories are not allowed"):
source.collect("wf")
def test_rejects_symlinked_workflow_yml(self, project_dir: Path, tmp_path: Path) -> None:
"""A symlinked workflow.yml must be rejected even if the directory is real."""
real_yml = tmp_path / "workflow.yml"
real_yml.write_text("schema_version: '1.0'\n", encoding="utf-8")
workflows_dir = project_dir / ".specify" / "workflows"
wf_dir = workflows_dir / "wf"
wf_dir.mkdir(parents=True, exist_ok=True)
link = wf_dir / "workflow.yml"
link.symlink_to(real_yml)
source = BaseWorkflowSource(project_dir)
with pytest.raises(OverlayLoadError, match="Symlinked workflow files are not allowed"):
source.collect("wf")
def test_missing_workflow_returns_empty(self, project_dir: Path) -> None:
"""A workflow directory that does not exist returns an empty layer list."""
source = BaseWorkflowSource(project_dir)
assert source.collect("no-such-wf") == []
def test_rejects_symlinked_workflows_root(self, project_dir: Path, tmp_path: Path) -> None:
outside = tmp_path / "outside"
outside.mkdir()
workflows_dir = project_dir / ".specify" / "workflows"
workflows_dir.rmdir()
workflows_dir.symlink_to(outside)
with pytest.raises(OverlayLoadError, match="Symlinked workflow directories"):
BaseWorkflowSource(project_dir).collect("wf")