mirror of
https://github.com/github/spec-kit.git
synced 2026-08-15 14:02:04 +02:00
fix(manifests): reject non-string requires.speckit_version (#3980)
`requires.speckit_version` was presence-checked but never type-checked in both the extension and preset manifest validators, so an unquoted YAML `speckit_version: 1.0` (a float) passed validation and reached `SpecifierSet(required)` in `check_compatibility()`. That call is guarded by `except InvalidSpecifier` alone, which a non-string escapes two different ways: - a float/int/bool/None raises `TypeError: 'float' object is not iterable` from the `SpecifierSet` constructor; - a list or dict is an *iterable*, so `SpecifierSet` accepts it and the failure surfaces much later as `AttributeError: 'str' object has no attribute 'filter'` from inside `.contains()`. Neither is a `CompatibilityError`/`PresetCompatibilityError`, so both bypass the CLI's "Compatibility Error" handler in `_commands.py` and exit 1 with a raw traceback that names no field, leaving the author with no hint which manifest key is wrong. Type-check the field in both validators, requiring a non-empty string, and additionally guard `check_compatibility()` in both managers since each is public and reachable with a hand-built or mutated manifest. This mirrors the sibling `IntegrationDescriptor`, which already requires a non-empty string for the same key, and completes the type-checking pass started in #3943 for the neighbouring `extension`/`preset` fields. Adds 33 regression tests across both modules covering every escape path; 26 of them fail without this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Assisted-by: Claude Code (model: Claude Opus 5, supervised)
This commit is contained in:
co-authored by
Claude Opus 5
parent
0fa86e8e9c
commit
cd996f74eb
@@ -410,6 +410,55 @@ class TestExtensionManifest:
|
||||
with pytest.raises(ValidationError, match="Invalid version"):
|
||||
ExtensionManifest(manifest_path)
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"bad",
|
||||
[
|
||||
1.0, # unquoted YAML float -- the likeliest authoring slip
|
||||
5, # unquoted int
|
||||
True, # YAML `yes`/`true`
|
||||
None, # `speckit_version:` written but left empty
|
||||
[">=0.1.0"], # iterable: slips past SpecifierSet() entirely
|
||||
{"min": "0.1"}, # iterable: same
|
||||
],
|
||||
)
|
||||
def test_non_string_speckit_version(self, temp_dir, valid_manifest_data, bad):
|
||||
"""A non-string requires.speckit_version must be a ValidationError.
|
||||
|
||||
It was presence-checked only, so it reached ``SpecifierSet(required)`` in
|
||||
check_compatibility(), which is guarded by ``except InvalidSpecifier``
|
||||
alone. A non-string escapes that guard two ways: scalars raise TypeError
|
||||
from the constructor, and a list/dict is iterable so SpecifierSet accepts
|
||||
it and the failure surfaces later as ``AttributeError: 'str' object has no
|
||||
attribute 'filter'`` from inside .contains().
|
||||
"""
|
||||
import yaml
|
||||
|
||||
valid_manifest_data["requires"]["speckit_version"] = 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="Invalid requires.speckit_version"
|
||||
):
|
||||
ExtensionManifest(manifest_path)
|
||||
|
||||
def test_empty_speckit_version(self, temp_dir, valid_manifest_data):
|
||||
"""A blank requires.speckit_version must be rejected, not treated as any."""
|
||||
import yaml
|
||||
|
||||
valid_manifest_data["requires"]["speckit_version"] = " "
|
||||
|
||||
manifest_path = temp_dir / "extension.yml"
|
||||
with open(manifest_path, 'w') as f:
|
||||
yaml.dump(valid_manifest_data, f)
|
||||
|
||||
with pytest.raises(
|
||||
ValidationError, match="Invalid requires.speckit_version"
|
||||
):
|
||||
ExtensionManifest(manifest_path)
|
||||
|
||||
def test_valid_category(self, temp_dir, valid_manifest_data):
|
||||
"""Test manifest with various category values (free-form string)."""
|
||||
import yaml
|
||||
@@ -1265,6 +1314,28 @@ class TestExtensionManager:
|
||||
with pytest.raises(CompatibilityError, match="Extension requires spec-kit"):
|
||||
manager.check_compatibility(manifest, "0.0.1")
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"bad",
|
||||
[1.0, 5, True, None, [">=0.1.0"], {"min": "0.1"}],
|
||||
)
|
||||
def test_check_compatibility_non_string_specifier(self, project_dir, bad):
|
||||
"""check_compatibility() must report a non-string as CompatibilityError.
|
||||
|
||||
Defense in depth for the validator check above: this method is public and
|
||||
reachable with a hand-built manifest, and ``except InvalidSpecifier`` does
|
||||
not cover a non-string. Without the guard, scalars raise a bare TypeError
|
||||
and iterables construct fine only to break inside .contains() -- neither
|
||||
is a CompatibilityError, so both bypass the CLI's "Compatibility Error"
|
||||
handler and exit 1 with a raw traceback naming no field.
|
||||
"""
|
||||
from types import SimpleNamespace
|
||||
|
||||
manager = ExtensionManager(project_dir)
|
||||
manifest = SimpleNamespace(requires_speckit_version=bad)
|
||||
|
||||
with pytest.raises(CompatibilityError, match="Invalid version specifier"):
|
||||
manager.check_compatibility(manifest, "0.15.2")
|
||||
|
||||
def test_check_compatibility_allows_prerelease_builds(self, extension_dir, project_dir):
|
||||
"""Prerelease spec-kit builds should satisfy compatible version ranges."""
|
||||
manager = ExtensionManager(project_dir)
|
||||
|
||||
@@ -406,6 +406,37 @@ provides:
|
||||
with pytest.raises(PresetValidationError, match="Missing requires.speckit_version"):
|
||||
PresetManifest(manifest_path)
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"bad",
|
||||
[
|
||||
1.0, # unquoted YAML float -- the likeliest authoring slip
|
||||
5, # unquoted int
|
||||
True, # YAML `yes`/`true`
|
||||
None, # `speckit_version:` written but left empty
|
||||
[">=0.1.0"], # iterable: slips past SpecifierSet() entirely
|
||||
{"min": "0.1"}, # iterable: same
|
||||
" ", # blank string must not mean "any version"
|
||||
],
|
||||
)
|
||||
def test_non_string_speckit_version(self, temp_dir, valid_pack_data, bad):
|
||||
"""A non-string requires.speckit_version must be a PresetValidationError.
|
||||
|
||||
It was presence-checked only, so it reached ``SpecifierSet(required)`` in
|
||||
check_compatibility(), which is guarded by ``except InvalidSpecifier``
|
||||
alone. A non-string escapes that guard two ways: scalars raise TypeError
|
||||
from the constructor, and a list/dict is iterable so SpecifierSet accepts
|
||||
it and the failure surfaces later as ``AttributeError: 'str' object has no
|
||||
attribute 'filter'`` from inside .contains().
|
||||
"""
|
||||
valid_pack_data["requires"]["speckit_version"] = bad
|
||||
manifest_path = temp_dir / "preset.yml"
|
||||
with open(manifest_path, 'w') as f:
|
||||
yaml.dump(valid_pack_data, f)
|
||||
with pytest.raises(
|
||||
PresetValidationError, match="Invalid requires.speckit_version"
|
||||
):
|
||||
PresetManifest(manifest_path)
|
||||
|
||||
def test_no_templates_provided(self, temp_dir, valid_pack_data):
|
||||
"""Test pack with no templates."""
|
||||
valid_pack_data["provides"]["templates"] = []
|
||||
@@ -964,6 +995,26 @@ class TestPresetManager:
|
||||
with pytest.raises(PresetCompatibilityError, match="Invalid version specifier"):
|
||||
manager.check_compatibility(manifest, "0.1.5")
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"bad",
|
||||
[1.0, 5, True, None, [">=0.1.0"], {"min": "0.1"}],
|
||||
)
|
||||
def test_check_compatibility_non_string_specifier(self, pack_dir, temp_dir, bad):
|
||||
"""check_compatibility() must report a non-string as a compatibility error.
|
||||
|
||||
Defense in depth for the validator check: this method is public and the
|
||||
specifier is read back out of mutable manifest data, and ``except
|
||||
InvalidSpecifier`` does not cover a non-string. Without the guard, scalars
|
||||
raise a bare TypeError and iterables construct fine only to break inside
|
||||
.contains() -- neither is a PresetCompatibilityError, so both bypass the
|
||||
CLI's "Compatibility Error" handler and exit 1 with a raw traceback.
|
||||
"""
|
||||
manager = PresetManager(temp_dir)
|
||||
manifest = PresetManifest(pack_dir / "preset.yml")
|
||||
manifest.data["requires"]["speckit_version"] = bad
|
||||
with pytest.raises(PresetCompatibilityError, match="Invalid version specifier"):
|
||||
manager.check_compatibility(manifest, "0.1.5")
|
||||
|
||||
def test_install_with_priority(self, project_dir, pack_dir):
|
||||
"""Test installing a pack with custom priority."""
|
||||
manager = PresetManager(project_dir)
|
||||
|
||||
Reference in New Issue
Block a user