diff --git a/src/specify_cli/bundler/commands_impl/catalog_config.py b/src/specify_cli/bundler/commands_impl/catalog_config.py index dadc7e9672..37e45bf990 100644 --- a/src/specify_cli/bundler/commands_impl/catalog_config.py +++ b/src/specify_cli/bundler/commands_impl/catalog_config.py @@ -40,9 +40,12 @@ def _read(project_root: Path) -> list[dict]: path = ensure_within(project_root, _config_path(project_root)) if not path.exists(): return [] + # ``load_yaml`` returns ``{}`` only for an empty document and the raw parse + # otherwise, so a non-mapping top level — a falsy ``[]``/``false``/``0``/``''`` + # or an explicit null (``load_yaml`` -> ``None``) — is caught by the isinstance + # guard below and raised like a truthy one, staying consistent with the other + # reader of this file (models/catalog._merge_config). data = load_yaml(path) - if data is None: - return [] if not isinstance(data, dict): raise BundlerError( f"Malformed catalog config at {path}: expected a mapping at the top " diff --git a/src/specify_cli/bundler/lib/yamlio.py b/src/specify_cli/bundler/lib/yamlio.py index b5fb0e5691..5143a1f689 100644 --- a/src/specify_cli/bundler/lib/yamlio.py +++ b/src/specify_cli/bundler/lib/yamlio.py @@ -39,17 +39,35 @@ def ensure_within(root: Path, candidate: Path) -> Path: def load_yaml(path: Path) -> Any: - """Parse a YAML file, returning ``{}`` for an empty document.""" + """Parse a YAML file, returning ``{}`` only for an *empty* document. + + A non-empty document is returned exactly as parsed — including a + non-mapping such as ``[]``, ``false``, ``0``, ``''``, or an explicit null + (``null``/``~``) — so callers can validate the top-level shape (e.g. reject + a non-mapping config) instead of having it silently coerced to an empty + mapping. + + ``yaml.safe_load`` returns ``None`` for *both* an empty document and an + explicit null scalar, so ``yaml.compose`` (which yields no node only for a + truly empty document) is used to tell them apart: an empty document becomes + ``{}`` while an explicit ``null``/``~`` is returned as ``None`` for the + caller to reject. + """ path = Path(path) if not path.exists(): raise BundlerError(f"File not found: {path}") try: - with path.open("r", encoding="utf-8") as handle: - return yaml.safe_load(handle) or {} - except yaml.YAMLError as exc: - raise BundlerError(f"Invalid YAML in {path}: {exc}") from exc + text = path.read_text(encoding="utf-8") except OSError as exc: raise BundlerError(f"Could not read {path}: {exc}") from exc + try: + has_node = yaml.compose(text) is not None + data = yaml.safe_load(text) + except yaml.YAMLError as exc: + raise BundlerError(f"Invalid YAML in {path}: {exc}") from exc + if data is None and not has_node: + return {} + return data def dump_yaml(path: Path, data: Any, *, within: Path | None = None) -> Path: diff --git a/src/specify_cli/bundler/models/catalog.py b/src/specify_cli/bundler/models/catalog.py index 848ec789da..9621b5a334 100644 --- a/src/specify_cli/bundler/models/catalog.py +++ b/src/specify_cli/bundler/models/catalog.py @@ -256,8 +256,18 @@ def load_source_stack(project_root: Path, user_config_dir: Path | None = None) - def _merge_config(by_id: dict[str, CatalogSource], config_path: Path, scope: Scope) -> None: if not config_path.exists(): return + # ``load_yaml`` returns ``{}`` only for an empty document and the raw parse + # otherwise, so a non-mapping top level (a YAML list or scalar, including + # the falsy ``[]``/``false``/``0``/``''``) is caught here and raised — + # matching the sibling reader commands_impl/catalog_config._read. #3623 + # aligned the inner non-list ``catalogs`` value between the two readers. data = load_yaml(config_path) - catalogs = data.get("catalogs") if isinstance(data, dict) else None + if not isinstance(data, dict): + raise BundlerError( + f"Malformed catalog config at {config_path}: expected a mapping at " + f"the top level, got {type(data).__name__}." + ) + catalogs = data.get("catalogs") if catalogs is None: return if not isinstance(catalogs, list): diff --git a/tests/contract/test_catalog_schema.py b/tests/contract/test_catalog_schema.py index 64cd47db6e..97600d21ef 100644 --- a/tests/contract/test_catalog_schema.py +++ b/tests/contract/test_catalog_schema.py @@ -68,10 +68,44 @@ def test_falsy_non_list_catalogs_still_raises(tmp_path: Path, value: str): load_source_stack(tmp_path) -@pytest.mark.parametrize("body", ["catalogs:\n", "catalogs: []\n"]) +@pytest.mark.parametrize( + "body", + [ + "- a\n- b\n", # truthy list + "42\n", # truthy scalar + "[]\n", # falsy list + "false\n", # falsy bool + "0\n", # falsy int + "''\n", # falsy empty string + "null\n", # explicit null scalar (safe_load -> None, but a real node) + "~\n", # explicit null scalar (alt spelling) + ], +) +def test_toplevel_non_mapping_raises(tmp_path: Path, body: str): + """A top-level non-mapping bundle-catalogs.yml (list/scalar/null) must raise, + matching the sibling reader (catalog_config._read) — not silently fall back + to the built-in default stack. This includes FALSY non-mappings ([], false, + 0, '') and an explicit null (null/~); the shared load_yaml would coerce those + to {} and hide them, so it distinguishes them from a truly empty document.""" + make_project(tmp_path) + (tmp_path / ".specify" / "bundle-catalogs.yml").write_text(body, encoding="utf-8") + with pytest.raises(BundlerError, match="expected a mapping at the top level"): + load_source_stack(tmp_path) + + +@pytest.mark.parametrize( + "body", + [ + "catalogs:\n", # present key, null value + "catalogs: []\n", # present key, empty list + "", # truly empty document + "# only a comment\n", # comment-only == empty document + ], +) def test_absent_or_empty_catalogs_is_noop(tmp_path: Path, body: str): - """An absent (``None``) or empty-list ``catalogs:`` is valid: it contributes - no project sources and falls back to the built-in default stack.""" + """An empty document, comment-only file, or absent/empty-list ``catalogs:`` + is valid: it contributes no project sources and falls back to the built-in + default stack (must not be confused with an explicit top-level null).""" make_project(tmp_path) (tmp_path / ".specify" / "bundle-catalogs.yml").write_text(body, encoding="utf-8") # Does not raise; still yields the built-in defaults. diff --git a/tests/unit/test_bundler_catalog_config.py b/tests/unit/test_bundler_catalog_config.py index 5ad6ecbb93..46c333700a 100644 --- a/tests/unit/test_bundler_catalog_config.py +++ b/tests/unit/test_bundler_catalog_config.py @@ -154,6 +154,20 @@ def test_read_rejects_non_mapping_top_level(tmp_path: Path): cc._read(project) +@pytest.mark.parametrize("body", ["[]\n", "false\n", "0\n", "''\n", "null\n", "~\n"]) +def test_read_rejects_falsy_non_mapping_top_level(tmp_path: Path, body: str): + # A FALSY non-mapping top level ([], false, 0, '') OR an explicit null + # (null/~) must raise like a truthy one. safe_load coerces these to + # None/{}, so load_yaml distinguishes them from a truly empty document — + # staying consistent with models/catalog._merge_config. + project = tmp_path / "proj" + (project / ".specify").mkdir(parents=True) + cc._config_path(project).write_text(body, encoding="utf-8") + + with pytest.raises(BundlerError, match="expected a mapping at the top level"): + cc._read(project) + + def test_read_rejects_unknown_schema_version(tmp_path: Path): project = tmp_path / "proj" (project / ".specify").mkdir(parents=True)