fix(manifests): reject non-string requires.speckit_version - #3980
Merged
Conversation
`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 github#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)
Contributor
There was a problem hiding this comment.
Pull request overview
Adds robust type validation for requires.speckit_version across extension and preset manifests.
Changes:
- Rejects non-string and blank version requirements during validation.
- Adds defensive compatibility checks for mutated or hand-built manifests.
- Adds regression coverage for scalar, iterable, null, and blank values.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/extensions/__init__.py |
Hardens extension validation and compatibility checks. |
src/specify_cli/presets/__init__.py |
Hardens preset validation and compatibility checks. |
tests/test_extensions.py |
Covers invalid extension version requirements. |
tests/test_presets.py |
Covers invalid preset version requirements. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
Collaborator
|
Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
requires.speckit_versionis checked for presence but never for type in both the extension and preset manifest validators. An unquoted YAMLspeckit_version: 1.0— an easy authoring slip, since YAML parses it as a float — passes validation and reachesSpecifierSet(required)incheck_compatibility().That call is guarded by
except InvalidSpecifieralone, which a non-string escapes two different ways:1.0,5,True,NoneTypeError: 'float' object is not iterable— raised by theSpecifierSetconstructor, notInvalidSpecifier[">=0.1.0"],{"min": "0.1"}SpecifierSetaccepts it; fails much later asAttributeError: 'str' object has no attribute 'filter'from inside.contains()Neither is a
CompatibilityError/PresetCompatibilityError, so both bypass theCompatibility Errorhandlers inextensions/_commands.py:1101andpresets/_commands.py:275, and exit 1 with a raw traceback that names no field — leaving the author no hint which manifest key is wrong. The list/dict case is the nastier one: the error surfaces deep insidepackagingwith a message that mentions neither the manifest nor the key.Reproduced on
mainwith an otherwise fully valid manifest:Fix
ValidationError/PresetValidationErrornaming the field.check_compatibility()methods additionally reject a non-string up front. These are public and reachable with a hand-built or mutated manifest (test_check_compatibility_invalidalready mutatesmanifest.datathis way), so the guard belongs at both layers.This mirrors the sibling
IntegrationDescriptor, which already requires a non-empty string for the very same key, and completes the type-checking pass started in #3943 for the neighbouringextension/presetfields —id/name/version/descriptionare now type-checked whilerequires.speckit_versionwas still presence-only.Blank strings are rejected too, so
speckit_version: " "can't silently mean "any version".Testing
33 regression tests added across both modules, covering every escape path (scalars, iterables, and blank strings) at both the validator and manager layer. 26 fail without the source change (the rest are existing-behaviour assertions that pass either way).
Full
tests/test_extensions.py+tests/test_presets.py:7 failed, 952 passed, 78 errorsboth with and without this change — identical counts. Those pre-existing failures are Windows-localPermissionError (WinError 5)from symlink/tmpdir restrictions in my environment, unrelated to this change.🤖 Generated with Claude Code