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)

Author: Noor-ul-ain001

src/specify_cli/extensions/__init__.py

@@ -341,6 +341,25 @@ def _validate(self):
             )
         if "speckit_version" not in requires:
             raise ValidationError("Missing requires.speckit_version")
+        # Presence alone is not enough: check_compatibility() feeds this value to
+        # ``SpecifierSet(required)``, guarded only by ``except InvalidSpecifier``,
+        # which a non-string escapes two different ways. A float/int/bool/None
+        # raises TypeError from the constructor, while 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, so both bypass the
+        # CLI's "Compatibility Error" handler and exit 1 with a raw traceback
+        # naming no field. An unquoted ``speckit_version: 1.0`` is an easy YAML
+        # slip. Mirrors the sibling IntegrationDescriptor, which already requires
+        # a non-empty string here.
+        if (
+            not isinstance(requires["speckit_version"], str)
+            or not requires["speckit_version"].strip()
+        ):
+            raise ValidationError(
+                "Invalid requires.speckit_version: expected a non-empty string, "
+                f"got {type(requires['speckit_version']).__name__}"
+            )
 
         # Validate provides section
         provides = self.data["provides"]
@@ -1851,6 +1870,17 @@ def check_compatibility(
         required = manifest.requires_speckit_version
 
         # Parse version specifier (e.g., ">=0.1.0,<2.0.0")
+        # Defense in depth: the manifest validator now rejects a non-string
+        # requires.speckit_version, but this method is public and also reachable
+        # with a hand-built manifest object. ``InvalidSpecifier`` alone does not
+        # cover a non-string -- scalars raise TypeError from the constructor, and
+        # a list/dict is iterable so it constructs here and only breaks inside
+        # .contains(). Reject up front so this always reports a CompatibilityError.
+        if not isinstance(required, str):
+            raise CompatibilityError(
+                "Invalid version specifier: expected a string, got "
+                f"{type(required).__name__} ({required!r})"
+            )
         try:
             SpecifierSet(required)  # Just to validate
         except InvalidSpecifier:

src/specify_cli/presets/__init__.py

@@ -344,6 +344,25 @@ def _validate(self):
         requires = self.data["requires"]
         if "speckit_version" not in requires:
             raise PresetValidationError("Missing requires.speckit_version")
+        # Presence alone is not enough: check_compatibility() feeds this value to
+        # ``SpecifierSet(required)``, guarded only by ``except InvalidSpecifier``,
+        # which a non-string escapes two different ways. A float/int/bool/None
+        # raises TypeError from the constructor, while 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 PresetCompatibilityError, so both
+        # bypass the CLI's "Compatibility Error" handler and exit 1 with a raw
+        # traceback naming no field. An unquoted ``speckit_version: 1.0`` is an
+        # easy YAML slip. Mirrors the sibling IntegrationDescriptor, which already
+        # requires a non-empty string here.
+        if (
+            not isinstance(requires["speckit_version"], str)
+            or not requires["speckit_version"].strip()
+        ):
+            raise PresetValidationError(
+                "Invalid requires.speckit_version: expected a non-empty string, "
+                f"got {type(requires['speckit_version']).__name__}"
+            )
 
         # Validate provides section
         provides = self.data["provides"]
@@ -756,6 +775,18 @@ def check_compatibility(
             PresetCompatibilityError: If pack is incompatible
         """
         required = manifest.requires_speckit_version
+        # Defense in depth: the manifest validator now rejects a non-string
+        # requires.speckit_version, but this method is public and also reachable
+        # with a hand-built manifest object. ``InvalidSpecifier`` alone does not
+        # cover a non-string -- scalars raise TypeError from the constructor, and
+        # a list/dict is iterable so it constructs here and only breaks inside
+        # .contains(). Reject up front so this always reports a
+        # PresetCompatibilityError.
+        if not isinstance(required, str):
+            raise PresetCompatibilityError(
+                "Invalid version specifier: expected a string, got "
+                f"{type(required).__name__} ({required!r})"
+            )
         try:
             SpecifierSet(required)  # Just to validate
         except InvalidSpecifier:

tests/test_extensions.py

@@ -410,6 +410,55 @@ def test_invalid_version(self, temp_dir, valid_manifest_data):
         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 @@ def test_check_compatibility_invalid(self, extension_dir, project_dir):
         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)

tests/test_presets.py

@@ -406,6 +406,37 @@ def test_missing_speckit_version(self, temp_dir, valid_pack_data):
         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 @@ def test_check_compatibility_invalid(self, pack_dir, temp_dir):
         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)