From 923e4461cfcca885f9eae712ec45af5ba60832e9 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Wed, 22 Jul 2026 10:06:37 +0500 Subject: [PATCH 1/2] fix(bundler): reject non-list 'catalogs' in bundle-catalogs.yml with a clean error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _merge_config guarded only 'if not catalogs: return', so a non-empty scalar (catalogs: 5) passed through and raised a raw 'TypeError: int object is not iterable' from the loop below — escaping the module's BundlerError error contract. The sibling reader of the same file (commands_impl/catalog_config.py, used by 'bundle catalog list') already raises an actionable BundlerError for the identical mis-shape. Add the same isinstance(list) guard so both readers of bundle-catalogs.yml agree. Test: 'catalogs: 5' raises BundlerError('...must be a list...') (fails before: raw TypeError). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/specify_cli/bundler/models/catalog.py | 10 ++++++++++ tests/contract/test_catalog_schema.py | 12 ++++++++++++ 2 files changed, 22 insertions(+) diff --git a/src/specify_cli/bundler/models/catalog.py b/src/specify_cli/bundler/models/catalog.py index a1744f9948..1870dda70d 100644 --- a/src/specify_cli/bundler/models/catalog.py +++ b/src/specify_cli/bundler/models/catalog.py @@ -253,6 +253,16 @@ def _merge_config(by_id: dict[str, CatalogSource], config_path: Path, scope: Sco catalogs = data.get("catalogs") if isinstance(data, dict) else None if not catalogs: return + if not isinstance(catalogs, list): + # A non-empty scalar (``catalogs: 5``) passes the falsy check above and + # would raise a raw ``TypeError: 'int' object is not iterable`` from the + # loop below. Report the same actionable BundlerError the sibling reader + # of this file raises (commands_impl/catalog_config.py) so both readers + # of bundle-catalogs.yml agree. + raise BundlerError( + f"Malformed catalog config at {config_path}: 'catalogs' must be a " + f"list, got {type(catalogs).__name__}." + ) for raw in catalogs: src = CatalogSource.from_dict(raw, scope) by_id[src.id] = src diff --git a/tests/contract/test_catalog_schema.py b/tests/contract/test_catalog_schema.py index a04218f82b..e427b7b583 100644 --- a/tests/contract/test_catalog_schema.py +++ b/tests/contract/test_catalog_schema.py @@ -43,6 +43,18 @@ def test_builtin_default_stack_when_no_config(tmp_path: Path): assert all(s.scope is Scope.BUILTIN for s in sources) +def test_non_list_catalogs_raises_actionable_error(tmp_path: Path): + """A scalar ``catalogs:`` value raises a clean BundlerError, not a raw + 'int object is not iterable' TypeError — matching what the sibling reader + (bundle catalog list) already reports for the same file.""" + make_project(tmp_path) + (tmp_path / ".specify" / "bundle-catalogs.yml").write_text( + "catalogs: 5\n", encoding="utf-8" + ) + with pytest.raises(BundlerError, match="must be a list"): + load_source_stack(tmp_path) + + def test_project_config_overrides_same_id(tmp_path: Path): make_project(tmp_path) config = { From 094f1bfda19e455f47abae28e6681961bb6f63e3 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Wed, 22 Jul 2026 18:35:10 +0500 Subject: [PATCH 2/2] fix(bundler): reject falsy non-list 'catalogs' too (is None, not falsy) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review: the isinstance guard sat after `if not catalogs: return`, so falsy non-list values (`catalogs: false`, `0`, `''`, `{}`) hit the early return and were silently accepted instead of raising the promised BundlerError. Only an absent/None value means "nothing to merge". Change the early return to `if catalogs is None`, mirroring the sibling reader (commands_impl/catalog_config._read). An empty list stays valid (the merge loop is a no-op). Add parametrized tests for the falsy non-list cases and for the absent/empty-list no-op. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) --- src/specify_cli/bundler/models/catalog.py | 16 +++++++++------ tests/contract/test_catalog_schema.py | 24 +++++++++++++++++++++++ 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/bundler/models/catalog.py b/src/specify_cli/bundler/models/catalog.py index 1870dda70d..293d31c69b 100644 --- a/src/specify_cli/bundler/models/catalog.py +++ b/src/specify_cli/bundler/models/catalog.py @@ -251,14 +251,18 @@ def _merge_config(by_id: dict[str, CatalogSource], config_path: Path, scope: Sco return data = load_yaml(config_path) catalogs = data.get("catalogs") if isinstance(data, dict) else None - if not catalogs: + if catalogs is None: return if not isinstance(catalogs, list): - # A non-empty scalar (``catalogs: 5``) passes the falsy check above and - # would raise a raw ``TypeError: 'int' object is not iterable`` from the - # loop below. Report the same actionable BundlerError the sibling reader - # of this file raises (commands_impl/catalog_config.py) so both readers - # of bundle-catalogs.yml agree. + # Treat only an absent/``None`` ``catalogs`` as "nothing to merge"; any + # other non-list value (``catalogs: 5``, ``false``, ``0``, ``''``, + # ``{}``) is a malformed config and must raise, not be silently skipped + # by a falsy check. Otherwise a truthy scalar would raise a raw + # ``TypeError: 'int' object is not iterable`` from the loop below, while + # falsy non-lists would be swallowed. Report the same actionable + # BundlerError the sibling reader of this file raises + # (commands_impl/catalog_config.py) so both readers of + # bundle-catalogs.yml agree. An empty list stays valid (loop is a no-op). raise BundlerError( f"Malformed catalog config at {config_path}: 'catalogs' must be a " f"list, got {type(catalogs).__name__}." diff --git a/tests/contract/test_catalog_schema.py b/tests/contract/test_catalog_schema.py index e427b7b583..bfa289d497 100644 --- a/tests/contract/test_catalog_schema.py +++ b/tests/contract/test_catalog_schema.py @@ -55,6 +55,30 @@ def test_non_list_catalogs_raises_actionable_error(tmp_path: Path): load_source_stack(tmp_path) +@pytest.mark.parametrize("value", ["false", "0", "''", "{}"]) +def test_falsy_non_list_catalogs_still_raises(tmp_path: Path, value: str): + """A *falsy* non-list ``catalogs:`` value (false/0/''/{}) must also raise — + only an absent/``None`` value means "nothing to merge". A plain falsy check + would silently swallow these, diverging from the sibling reader.""" + make_project(tmp_path) + (tmp_path / ".specify" / "bundle-catalogs.yml").write_text( + f"catalogs: {value}\n", encoding="utf-8" + ) + with pytest.raises(BundlerError, match="must be a list"): + load_source_stack(tmp_path) + + +@pytest.mark.parametrize("body", ["catalogs:\n", "catalogs: []\n"]) +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.""" + 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. + sources = load_source_stack(tmp_path) + assert len(sources) > 0 + + def test_project_config_overrides_same_id(tmp_path: Path): make_project(tmp_path) config = {