mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
fix(extensions): guard the required manifest sections so one bad extension cannot break extension list (#3797)
ExtensionManifest.REQUIRED_FIELDS only checks key PRESENCE, so a section that is
written but left empty (`provides:` -> None) or given the wrong shape
(`provides: []`) passes it and then fails on first use:
extension: null -> TypeError: argument of type 'NoneType' is not iterable
requires: null -> TypeError: argument of type 'NoneType' is not iterable
provides: null -> AttributeError: 'NoneType' object has no attribute 'get'
provides: [] -> AttributeError: 'list' object has no attribute 'get'
Neither is a ValidationError, so both escape the callers that already handle
malformed manifests. list_installed() catches ValidationError only and has a
deliberate "Corrupted extension" fallback, so a single bad extension took down
the whole command -- reproduced end-to-end:
before: specify extension list -> exit 1, raw AttributeError, no output
after: specify extension list -> exit 0, the good extension listed, the
bad one shown as "Corrupted extension"
Add an isinstance guard for each required section, mirroring the nested guards
already in this function ("Invalid provides.commands: expected a list", "Invalid
hooks: expected a mapping") and _load_yaml's document-root check. Only the three
REQUIRED sections lacked one.
`provides: {}` is unaffected: it is a well-shaped mapping, so an extension that
provides only hooks still validates, and with no hooks it keeps the pre-existing
"must provide at least one command or hook" message. Both are locked by tests.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -263,8 +263,25 @@ class ExtensionManifest:
|
||||
f"(expected {self.SCHEMA_VERSION})"
|
||||
)
|
||||
|
||||
# The REQUIRED_FIELDS loop above only checks key PRESENCE, so a section
|
||||
# that is written but left empty (``provides:`` -> None) or given the
|
||||
# wrong shape (``provides: []``) passes it and then fails on first use:
|
||||
# ``field not in None`` raises TypeError and ``None.get(...)`` raises
|
||||
# AttributeError. Neither is a ValidationError, so both escape the
|
||||
# callers that already handle malformed manifests -- list_installed()'s
|
||||
# "Corrupted extension" fallback catches ValidationError only, so one bad
|
||||
# extension made ``specify extension list`` exit 1 with a raw
|
||||
# AttributeError instead of listing the rest. Guard each required
|
||||
# section's shape, mirroring the nested guards below ("Invalid
|
||||
# provides.commands: expected a list", "Invalid hooks: expected a
|
||||
# mapping") and _load_yaml's document-root check.
|
||||
|
||||
# Validate extension metadata
|
||||
ext = self.data["extension"]
|
||||
if not isinstance(ext, dict):
|
||||
raise ValidationError(
|
||||
f"Invalid extension: expected a mapping, got {type(ext).__name__}"
|
||||
)
|
||||
for field in ["id", "name", "version", "description"]:
|
||||
if field not in ext:
|
||||
raise ValidationError(f"Missing extension.{field}")
|
||||
@@ -299,11 +316,19 @@ class ExtensionManifest:
|
||||
|
||||
# Validate requires section
|
||||
requires = self.data["requires"]
|
||||
if not isinstance(requires, dict):
|
||||
raise ValidationError(
|
||||
f"Invalid requires: expected a mapping, got {type(requires).__name__}"
|
||||
)
|
||||
if "speckit_version" not in requires:
|
||||
raise ValidationError("Missing requires.speckit_version")
|
||||
|
||||
# Validate provides section
|
||||
provides = self.data["provides"]
|
||||
if not isinstance(provides, dict):
|
||||
raise ValidationError(
|
||||
f"Invalid provides: expected a mapping, got {type(provides).__name__}"
|
||||
)
|
||||
commands = provides.get("commands", [])
|
||||
hooks = self.data.get("hooks")
|
||||
|
||||
|
||||
@@ -614,6 +614,64 @@ class TestExtensionManifest:
|
||||
with pytest.raises(ValidationError, match="Invalid provides.commands"):
|
||||
ExtensionManifest(manifest_path)
|
||||
|
||||
@pytest.mark.parametrize("section", ["extension", "requires", "provides"])
|
||||
@pytest.mark.parametrize("bad", [None, [], "text"])
|
||||
def test_required_section_not_mapping_rejected(
|
||||
self, temp_dir, valid_manifest_data, section, bad
|
||||
):
|
||||
"""A required section that is written but empty or wrongly shaped must
|
||||
raise ValidationError, not a raw TypeError/AttributeError.
|
||||
|
||||
REQUIRED_FIELDS only checks key presence, so `provides:` with no value
|
||||
passed it and then hit `None.get(...)`. That AttributeError escaped
|
||||
list_installed()'s ValidationError-only "Corrupted extension" fallback,
|
||||
so one bad extension made `specify extension list` exit 1 instead of
|
||||
listing the others.
|
||||
"""
|
||||
import yaml
|
||||
|
||||
valid_manifest_data[section] = bad
|
||||
|
||||
manifest_path = temp_dir / "extension.yml"
|
||||
with open(manifest_path, 'w') as f:
|
||||
yaml.dump(valid_manifest_data, f)
|
||||
|
||||
with pytest.raises(ValidationError, match=f"Invalid {section}"):
|
||||
ExtensionManifest(manifest_path)
|
||||
|
||||
def test_empty_provides_mapping_is_still_accepted_with_hooks(
|
||||
self, temp_dir, valid_manifest_data
|
||||
):
|
||||
"""Regression guard: `provides: {}` is a well-SHAPED mapping, so the new
|
||||
shape check must not reject it — an extension may provide only hooks."""
|
||||
import yaml
|
||||
|
||||
valid_manifest_data["provides"] = {}
|
||||
assert valid_manifest_data.get("hooks"), "fixture is expected to define hooks"
|
||||
|
||||
manifest_path = temp_dir / "extension.yml"
|
||||
with open(manifest_path, 'w') as f:
|
||||
yaml.dump(valid_manifest_data, f)
|
||||
|
||||
ExtensionManifest(manifest_path) # must not raise
|
||||
|
||||
def test_empty_provides_and_no_hooks_keeps_its_own_message(
|
||||
self, temp_dir, valid_manifest_data
|
||||
):
|
||||
"""...and with no hooks either, it keeps the pre-existing message rather
|
||||
than the new shape error."""
|
||||
import yaml
|
||||
|
||||
valid_manifest_data["provides"] = {}
|
||||
valid_manifest_data.pop("hooks", None)
|
||||
|
||||
manifest_path = temp_dir / "extension.yml"
|
||||
with open(manifest_path, 'w') as f:
|
||||
yaml.dump(valid_manifest_data, f)
|
||||
|
||||
with pytest.raises(ValidationError, match="at least one command or hook"):
|
||||
ExtensionManifest(manifest_path)
|
||||
|
||||
def test_hooks_not_dict_rejected(self, temp_dir, valid_manifest_data):
|
||||
"""Test manifest with hooks as a list is rejected."""
|
||||
import yaml
|
||||
|
||||
Reference in New Issue
Block a user