fix(bundler): reject a top-level non-mapping bundle-catalogs.yml in _merge_config

[jawwad-ali]

What

_merge_config silently ignored a top-level non-mapping bundle-catalogs.yml (a YAML list or scalar): data.get("catalogs") if isinstance(data, dict) else None made a list/scalar document fall through to None → early return → the built-in default stack, with no error.

The sibling reader of the same file — commands_impl/catalog_config._read — raises "expected a mapping at the top level" for exactly those documents. Merged PR #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 input.

Fix

Add an explicit top-level mapping guard mirroring _read:

if not isinstance(data, dict):
    raise BundlerError(f"Malformed catalog config at {config_path}: expected a mapping at the top level, got {type(data).__name__}.")

An empty file (load_yaml coerces to {}), absent catalogs, and catalogs: [] all remain no-ops.

Tests

tests/contract/test_catalog_schema.py::test_toplevel_non_mapping_raises (parametrized over a top-level list and a scalar) — both raise after the fix, silently return the default stack before it. Regression tests confirm absent/empty catalogs still yield the built-in defaults. ruff clean; all 23 contract tests pass.

---

AI-assisted: authored with Claude Code. Verified the message/behavior matches the _read sibling and confirmed fail-before/pass-after.

[Copilot]

Pull request overview

Aligns bundle catalog parsing with the sibling reader by rejecting malformed top-level YAML structures.

Changes:

• Adds a top-level mapping validation guard.
• Adds regression tests for list and scalar documents.
• Remaining issue: falsy non-mappings are still coerced to {} by load_yaml.

Show a summary per file

File	Description
src/specify_cli/bundler/models/catalog.py	Validates the catalog configuration’s top-level shape.
tests/contract/test_catalog_schema.py	Tests rejection of truthy non-mapping documents.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Medium

[mnriem]

Please address Copilot feedback

[jawwad-ali]

Good catch — fixed in 7285066. You're right: load_yaml() does yaml.safe_load(...) or {}, so a falsy top-level ([], false, 0, '') was coerced to {} before the guard and silently fell back to defaults.

Both readers of bundle-catalogs.yml now parse the raw document instead of going through the coercing load_yaml: 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. I fixed _read (commands_impl/catalog_config.py) the same way so the two readers stay genuinely consistent (its if data is None was likewise dead code under load_yaml). Added falsy-case tests for both.

[Copilot]

Review details

Comments suppressed due to low confidence (1)

src/specify_cli/bundler/models/catalog.py:265

• Keep this read behind lib/yamlio.load_yaml rather than duplicating its parser and error translation. That module explicitly centralizes bundler reads (src/specify_cli/bundler/lib/yamlio.py:3-6); the actual defect is its safe_load(...) or {} implementation. Changing the helper to return {} only when the parsed value is None, then reusing it here, preserves empty-document behavior while exposing falsy non-mappings and prevents duplicated readers from drifting.

    try:
        data = yaml.safe_load(config_path.read_text(encoding="utf-8"))
    except yaml.YAMLError as exc:
        raise BundlerError(f"Invalid YAML in {config_path}: {exc}") from exc
    except OSError as exc:

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Medium

[mnriem]

Please address Copilot feedback

[jawwad-ali]

You're right — reverted the duplicated inline parsing and fixed the root cause in the centralized helper instead. load_yaml now does {} if data is None else data (only an empty document becomes {}, matching its docstring) rather than ... or {} which coerced falsy non-mappings too. Both readers (_merge_config and _read) go back through load_yaml; their existing isinstance(data, dict) guards now reject []/false/0/'' correctly. I audited all three load_yaml callers (these two + manifest.from_dict) — each already guards the top-level shape, so none regresses. Fix in the latest commit; falsy-case tests kept for both readers.

[Copilot]

Review details

Comments suppressed due to low confidence (1)

src/specify_cli/bundler/commands_impl/catalog_config.py:46

• The existing if data is None: return [] immediately below still accepts an explicit top-level YAML null. Once load_yaml preserves explicit nulls, _merge_config will reject this value but _read will continue silently treating it as empty, leaving the two readers inconsistent. Truly empty documents already become {}, so remove the None special case.

    # ``load_yaml`` returns ``{}`` only for an empty document and the raw parse
    # otherwise, so a falsy non-mapping (``[]``/``false``/``0``/``''``) 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).

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Medium

[mnriem]

Please address Copilot feedback

[jawwad-ali]

Good point — fixed. yaml.safe_load returns None for both an empty document and an explicit null (null/~), so load_yaml now uses yaml.compose (which yields a node only for a non-empty document) to distinguish them: a truly empty/comment-only document becomes {}, while an explicit null returns None and is rejected by the callers' isinstance(dict) guard like any other non-mapping. Also dropped the now-incorrect if data is None: return [] short-circuit in _read so an explicit null reaches that guard. Added null/~ cases for both readers plus an empty/comment-only no-op test. (Rebased onto main to clear the docs-table conflict too.)

[Copilot]

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Medium

[mnriem]

Thank you!