mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix(presets): guard non-list/non-mapping provides.templates in PresetManifest (#3712)
* fix(presets): guard non-list/non-mapping provides.templates in PresetManifest PresetManifest._validate iterated provides["templates"] with no shape guards, unlike the sibling ExtensionManifest. A malformed third-party preset.yml crashed with a raw TypeError that escapes the install handler's PresetValidationError/PresetError catch and dumps an unhandled traceback: templates: 5 -> "'int' object is not iterable" templates: [null] -> "argument of type 'NoneType' is not iterable" templates: [5] -> "argument of type 'int' is not iterable" (and a string/list entry raised the misleading "Template missing 'type', 'name', or 'file'"). Add a container list-guard and a per-entry mapping-guard that raise a clean PresetValidationError, mirroring ExtensionManifest's provides.commands guards. Valid manifests (list of mappings) are unaffected. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(presets): check provides.templates type before emptiness Address review feedback: the new shape guard sat behind the existing truthiness check, so a FALSY non-list (templates: 0/false/null/''/{}) still reported the misleading "Preset must provide at least one template" instead of the type error. Only truthy non-lists (5, "oops", {"a": 1}) reached the guard, which is why the original test (templates: 5) passed. Split the checks: presence -> container type -> emptiness. A falsy non-list now reports "expected a list"; an EMPTY LIST keeps the "at least one template" message, since that genuinely is a well-typed container with no templates. Parametrize the non-list test over truthy AND falsy values, and add a regression guard for the empty-list message. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(presets): drop the redundant empty-list templates test Address review feedback: the added test duplicated the pre-existing test_no_templates_provided -- both set provides.templates to [] and assert the same "must provide at least one template" error. That test already guards the empty-list result of the type-before-emptiness ordering, so keeping mine only added maintenance. Left a pointer comment where it was. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -322,13 +322,37 @@ class PresetManifest:
|
||||
|
||||
# Validate provides section
|
||||
provides = self.data["provides"]
|
||||
if "templates" not in provides or not provides["templates"]:
|
||||
if "templates" not in provides:
|
||||
raise PresetValidationError(
|
||||
"Preset must provide at least one template"
|
||||
)
|
||||
|
||||
# Validate templates
|
||||
for tmpl in provides["templates"]:
|
||||
# Validate templates. Guard the container and each entry's shape so a
|
||||
# malformed third-party preset.yml (e.g. ``templates: 5`` or
|
||||
# ``templates: [null]``) raises a clean PresetValidationError the
|
||||
# install handler already catches, instead of a raw TypeError
|
||||
# ('int'/'NoneType' object is not iterable) that escapes to an
|
||||
# unhandled traceback. Mirrors the sibling ExtensionManifest guards.
|
||||
#
|
||||
# Order matters: the container's TYPE is checked before its emptiness,
|
||||
# so a FALSY non-list (``templates: 0``/``false``/``null``/``''``/``{}``)
|
||||
# reports the accurate type error rather than the misleading "must
|
||||
# provide at least one template". An empty list still reports the
|
||||
# latter, since that genuinely is a list with no templates.
|
||||
templates = provides["templates"]
|
||||
if not isinstance(templates, list):
|
||||
raise PresetValidationError(
|
||||
"Invalid provides.templates: expected a list"
|
||||
)
|
||||
if not templates:
|
||||
raise PresetValidationError(
|
||||
"Preset must provide at least one template"
|
||||
)
|
||||
for tmpl in templates:
|
||||
if not isinstance(tmpl, dict):
|
||||
raise PresetValidationError(
|
||||
"Each template entry in 'provides.templates' must be a mapping"
|
||||
)
|
||||
if "type" not in tmpl or "name" not in tmpl or "file" not in tmpl:
|
||||
raise PresetValidationError(
|
||||
"Template missing 'type', 'name', or 'file'"
|
||||
|
||||
@@ -197,6 +197,45 @@ class TestPresetManifest:
|
||||
with pytest.raises(PresetValidationError, match="YAML mapping"):
|
||||
PresetManifest(manifest_path)
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"bad",
|
||||
[
|
||||
5, "oops", {"a": 1}, # truthy non-lists
|
||||
0, False, None, "", {}, # FALSY non-lists: must not fall through to
|
||||
# the misleading "at least one template"
|
||||
],
|
||||
)
|
||||
def test_non_list_templates_raises_validation_error(
|
||||
self, temp_dir, valid_pack_data, bad
|
||||
):
|
||||
"""A non-list provides.templates raises the accurate type error, not a raw
|
||||
'int object is not iterable' TypeError and not the misleading "must provide
|
||||
at least one template" (which a falsy non-list hit while the type check
|
||||
sat behind the emptiness check) — mirrors ExtensionManifest."""
|
||||
valid_pack_data["provides"]["templates"] = bad
|
||||
manifest_path = temp_dir / "preset.yml"
|
||||
manifest_path.write_text(yaml.dump(valid_pack_data), encoding="utf-8")
|
||||
with pytest.raises(PresetValidationError, match="templates.*expected a list"):
|
||||
PresetManifest(manifest_path)
|
||||
|
||||
# NOTE: the empty-list case (a well-typed container with no templates, which
|
||||
# must keep the "must provide at least one template" message after the
|
||||
# type-before-emptiness reordering) is already covered by
|
||||
# test_no_templates_provided below.
|
||||
|
||||
@pytest.mark.parametrize("bad_entry", [None, 5, "oops", ["nested"]])
|
||||
def test_non_mapping_template_entry_raises_validation_error(
|
||||
self, temp_dir, valid_pack_data, bad_entry
|
||||
):
|
||||
"""A non-mapping template entry (null/scalar/list) raises PresetValidationError,
|
||||
not a raw 'argument of type ... is not iterable' TypeError from the
|
||||
`"type" not in tmpl` membership test — mirrors ExtensionManifest."""
|
||||
valid_pack_data["provides"]["templates"] = [bad_entry]
|
||||
manifest_path = temp_dir / "preset.yml"
|
||||
manifest_path.write_text(yaml.dump(valid_pack_data), encoding="utf-8")
|
||||
with pytest.raises(PresetValidationError, match="must be a mapping"):
|
||||
PresetManifest(manifest_path)
|
||||
|
||||
def test_missing_schema_version(self, temp_dir, valid_pack_data):
|
||||
"""Test missing schema_version field."""
|
||||
del valid_pack_data["schema_version"]
|
||||
|
||||
Reference in New Issue
Block a user