From cf0abe28f7ee875448f9e4dbd8cd2b533797a1cb Mon Sep 17 00:00:00 2001 From: Ali jawwad <33836051+jawwad-ali@users.noreply.github.com> Date: Thu, 23 Jul 2026 21:15:14 +0500 Subject: [PATCH] fix(bundler): reject a top-level non-mapping bundle-catalogs.yml in _merge_config (#3659) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(bundler): reject a top-level non-mapping bundle-catalogs.yml in _merge_config _merge_config silently ignored a top-level non-mapping document (a YAML list or scalar) — `data.get("catalogs") if isinstance(data, dict) else None` made it fall through to the built-in default stack — while the sibling reader of the SAME file (commands_impl/catalog_config._read) raises "expected a mapping at the top level". #3623 already made the inner non-list `catalogs` value agree between the two readers; this closes the remaining top-level-shape gap so both readers reject the same malformed documents. An empty file (load_yaml coerces to {}), absent `catalogs`, and `catalogs: []` all remain no-ops. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) * fix(bundler): reject FALSY non-mapping catalog configs (parse raw, not load_yaml) Address review (Copilot on #3659): the top-level guard used the shared load_yaml, whose `yaml.safe_load(...) or {}` coerces a FALSY top-level document ([], false, 0, '') to {} BEFORE the isinstance check — so those malformed configs silently fell back to the built-in defaults instead of raising. Only truthy non-mappings ([a,b], 42) were caught. Parse the raw document in both readers of bundle-catalogs.yml (models/catalog._merge_config AND commands_impl/catalog_config._read): an empty document (None) stays a no-op, but every non-mapping top level — falsy or truthy — now raises "expected a mapping at the top level". This keeps the two readers genuinely consistent. Tests cover the falsy cases for both. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) * fix(bundler): correct load_yaml so only empty documents become {} (not falsy non-mappings) Address review (Copilot re-review of #3659): the previous fix duplicated YAML parsing + exception wrapping inline in two readers, bypassing the centralized yamlio helper. Instead, correct the root cause in load_yaml. load_yaml did `yaml.safe_load(...) or {}`, which coerced ANY falsy result (None empty-doc, but also [], false, 0, '') to {} — contradicting its own docstring ("{} for an empty document") and hiding malformed non-mapping configs from callers' shape guards. Change to `{} if data is None else data` so only an empty document becomes {}; a non-mapping top level is returned as-parsed. Revert the inline raw-parse in models/catalog._merge_config and commands_impl/catalog_config._read back to the centralized load_yaml; their existing `isinstance(data, dict)` guards now correctly reject falsy non-mappings too. All three load_yaml callers (these two + manifest.from_dict) already guard the top-level shape, so none regresses. Falsy-case tests for both readers retained. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) * fix(bundler): distinguish an empty YAML document from an explicit null in load_yaml Address review (Copilot on #3659): yaml.safe_load returns None for BOTH an empty document AND an explicit null scalar (`null`/`~`), so mapping None to {} still let a top-level null bundle-catalogs.yml fall back to defaults instead of being rejected by the mapping guard. Use yaml.compose (which yields a node only for a non-empty document) to tell the two apart: a truly empty document becomes {}, while an explicit null is returned as None so the callers' isinstance(dict) guard rejects it like any other non-mapping. Drop the now-incorrect `if data is None: return []` short-circuit in catalog_config._read so an explicit null reaches that guard. Tests cover null/~ for both readers plus empty/comment-only no-op. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .../bundler/commands_impl/catalog_config.py | 7 +++- src/specify_cli/bundler/lib/yamlio.py | 28 ++++++++++--- src/specify_cli/bundler/models/catalog.py | 12 +++++- tests/contract/test_catalog_schema.py | 40 +++++++++++++++++-- tests/unit/test_bundler_catalog_config.py | 14 +++++++ 5 files changed, 90 insertions(+), 11 deletions(-) diff --git a/src/specify_cli/bundler/commands_impl/catalog_config.py b/src/specify_cli/bundler/commands_impl/catalog_config.py index dadc7e967..37e45bf99 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 b5fb0e569..5143a1f68 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 848ec789d..9621b5a33 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 64cd47db6..97600d21e 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 5ad6ecbb9..46c333700 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)