github/github-spec-kit
1
0
Fork 0
mirror of https://github.com/github/spec-kit.git synced 2026-08-03 06:26:30 +08:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ali jawwad
2026-07-23 21:15:14 +05:00
committed by GitHub
parent 9b3546f437
commit cf0abe28f7
5 changed files with 90 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
src/specify_cli/bundler/commands_impl/catalog_config.py
View File

@@ -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 "

28
src/specify_cli/bundler/lib/yamlio.py
View File

@@ -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:

12
src/specify_cli/bundler/models/catalog.py
View File

@@ -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):

40
tests/contract/test_catalog_schema.py
View File

@@ -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.

14
tests/unit/test_bundler_catalog_config.py
View File

@@ -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)