mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix(bundler): reject non-list 'catalogs' in bundle-catalogs.yml with a clean error (#3623)
* fix(bundler): reject non-list 'catalogs' in bundle-catalogs.yml with a clean error
_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) <noreply@anthropic.com>
* fix(bundler): reject falsy non-list 'catalogs' too (is None, not falsy)
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) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -251,8 +251,22 @@ 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):
|
||||
# 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__}."
|
||||
)
|
||||
for raw in catalogs:
|
||||
src = CatalogSource.from_dict(raw, scope)
|
||||
by_id[src.id] = src
|
||||
|
||||
@@ -43,6 +43,42 @@ 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)
|
||||
|
||||
|
||||
@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 = {
|
||||
|
||||
Reference in New Issue
Block a user