diff --git a/presets/PUBLISHING.md b/presets/PUBLISHING.md index f71c1f45d8..439fd54300 100644 --- a/presets/PUBLISHING.md +++ b/presets/PUBLISHING.md @@ -68,6 +68,8 @@ preset: requires: speckit_version: ">=0.1.0" # Required spec-kit version + extensions: # Optional: extensions this preset needs + - "companion-extension" provides: templates: @@ -93,6 +95,34 @@ tags: # 2-5 relevant tags - ✅ Command names use dot notation (e.g. `speckit.specify`) - ✅ Tags are lowercase and descriptive +#### Declaring extension dependencies + +If your preset overrides commands that call into an extension, declare it in +`requires.extensions`. Without the extension the preset still installs and the +overrides fall through to the core workflow, so nothing errors — the feature +just silently does less than the user expects. Declaring the dependency makes +`specify preset add` say so, and name the command that fixes it. + +Use a bare id, or a mapping when you need a version floor or an optional +dependency: + +```yaml +requires: + speckit_version: ">=0.9.0" + extensions: + - "companion-extension" # required, any version + - id: "other-extension" + version: ">=1.2.0" # optional PEP 440 specifier + required: false # optional, defaults to true +``` + +Notes: + +- The field is optional. A preset that declares nothing behaves exactly as before. +- A missing or version-unsatisfied dependency produces a **warning, not a failure** — the install still succeeds. +- `required: false` documents an enhancing-but-optional extension and is never warned about. +- Declare it in `preset.yml`, not only in your catalog entry. The catalog is not consulted for `--dev` and `--from ` installs, so the manifest is the only copy present on every install path. + ### 3. Test Locally ```bash diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 54dc5d2845..2006587bc0 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -381,6 +381,14 @@ def _validate(self): f"got {type(requires['speckit_version']).__name__}" ) + # Validate the optional extension dependency list. A preset that + # overrides commands calling into an extension is inert without it, and + # until now the only place that could be said was the README -- see + # issue #4231. Absent means "no dependencies", so every existing preset + # stays valid. + if "extensions" in requires: + self._validate_requires_extensions(requires["extensions"]) + # Validate provides section provides = self.data["provides"] if "templates" not in provides: @@ -524,11 +532,116 @@ def author(self) -> str: """Get preset author.""" return self.data["preset"].get("author", "") + @staticmethod + def _validate_requires_extensions(declared: Any) -> None: + """Validate the optional ``requires.extensions`` list. + + Accepts either a bare extension id or a mapping carrying an optional + version specifier and an optional ``required`` flag: + + .. code-block:: yaml + + requires: + extensions: + - speckit-inventory + - id: other-ext + version: ">=1.2.0" + required: false + + Raises: + PresetValidationError: If the list or any entry is malformed. + """ + if not isinstance(declared, list): + raise PresetValidationError( + "Invalid requires.extensions: expected a list, " + f"got {type(declared).__name__}" + ) + + for index, entry in enumerate(declared): + label = f"requires.extensions[{index}]" + + if isinstance(entry, str): + entry = {"id": entry} + elif not isinstance(entry, dict): + raise PresetValidationError( + f"Invalid {label}: expected a string or a mapping, " + f"got {type(entry).__name__}" + ) + + if "id" not in entry: + raise PresetValidationError(f"Missing {label}.id") + extension_id = entry["id"] + if not isinstance(extension_id, str): + raise PresetValidationError( + f"Invalid {label}.id: expected a string, " + f"got {type(extension_id).__name__}" + ) + # Same id shape the extension loader enforces, so a dependency can + # never name something that could not be installed in the first place. + if not re.match(r'^[a-z0-9-]+$', extension_id): + raise PresetValidationError( + f"Invalid {label}.id '{extension_id}': " + "must be lowercase alphanumeric with hyphens only" + ) + + if "version" in entry: + constraint = entry["version"] + # Mirrors the requires.speckit_version reasoning: a non-string + # escapes InvalidSpecifier two ways -- scalars raise TypeError + # from the constructor, and a list/dict is iterable so it + # constructs and only fails later inside .contains(). + if not isinstance(constraint, str) or not constraint.strip(): + raise PresetValidationError( + f"Invalid {label}.version: expected a non-empty string, " + f"got {type(constraint).__name__}" + ) + try: + SpecifierSet(constraint) + except InvalidSpecifier: + raise PresetValidationError( + f"Invalid {label}.version '{constraint}': " + "not a valid version specifier" + ) + + if "required" in entry and not isinstance(entry["required"], bool): + raise PresetValidationError( + f"Invalid {label}.required: expected a boolean, " + f"got {type(entry['required']).__name__}" + ) + @property def requires_speckit_version(self) -> str: """Get required spec-kit version range.""" return self.data["requires"]["speckit_version"] + @property + def requires_extensions(self) -> List[Dict[str, Any]]: + """Get declared extension dependencies, normalized to mappings. + + Returns: + One entry per dependency with ``id``, ``version`` (``None`` when + unconstrained), and ``required`` (defaulting to ``True``). Empty + when the manifest declares no dependencies. + """ + declared = self.data["requires"].get("extensions") + if not isinstance(declared, list): + return [] + + normalized: List[Dict[str, Any]] = [] + for entry in declared: + if isinstance(entry, str): + entry = {"id": entry} + if not isinstance(entry, dict) or not isinstance(entry.get("id"), str): + continue + normalized.append( + { + "id": entry["id"], + "version": entry.get("version"), + "required": entry.get("required", True), + } + ) + return normalized + @property def templates(self) -> List[Dict[str, Any]]: """Get list of provided templates.""" @@ -838,6 +951,82 @@ def check_compatibility( return True + def find_unmet_extension_dependencies( + self, + manifest: PresetManifest + ) -> List[Dict[str, Any]]: + """Find declared extension dependencies that are not satisfied. + + Reports rather than raises. A preset whose overrides call into an + extension is written to degrade safely -- without the extension the + core workflow still runs -- so a missing dependency is a warning, not + an install failure. See issue #4231. + + Args: + manifest: Preset manifest to inspect + + Returns: + One entry per unsatisfied dependency, each with ``id``, the + requested ``version`` specifier (``None`` when unconstrained), the + ``installed`` version (``None`` when absent), and a ``reason`` of + ``"missing"``, ``"disabled"``, or ``"version"``. Optional + dependencies (``required: false``) are never reported. + """ + # Defense in depth, mirroring check_compatibility(): this method is + # public and also reachable with a hand-built manifest object that + # predates this field. A manifest without it declares nothing. + candidates = getattr(manifest, "requires_extensions", None) + if not isinstance(candidates, list): + return [] + + declared = [ + dep for dep in candidates + if isinstance(dep, dict) and dep.get("required", True) + ] + if not declared: + return [] + + registry = ExtensionRegistry(self.project_root / ".specify" / "extensions") + unmet: List[Dict[str, Any]] = [] + + for dep in declared: + metadata = registry.get(dep["id"]) + if metadata is None: + unmet.append({**dep, "installed": None, "reason": "missing"}) + continue + + installed_version = metadata.get("version") + installed_version = ( + installed_version if isinstance(installed_version, str) else None + ) + + # A disabled extension is registered but contributes nothing: + # resolution skips it (see _collect_extension_layers), so the + # preset is just as inert as if it were absent. Report it before + # any version check -- enabling it is the prerequisite, and the + # version may well be fine once it is. + if not metadata.get("enabled", True): + unmet.append( + {**dep, "installed": installed_version, "reason": "disabled"} + ) + continue + + constraint = dep["version"] + if not constraint: + continue + + if installed_version is None: + # A registry entry without a usable version cannot be compared. + # Treat it as satisfied rather than inventing a failure, since + # the extension is demonstrably installed and enabled. + continue + if not version_satisfies(installed_version, constraint): + unmet.append( + {**dep, "installed": installed_version, "reason": "version"} + ) + + return unmet + def _register_commands( self, manifest: PresetManifest, diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index 48d5c9f14f..5594273c7a 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -40,6 +40,53 @@ preset_app.add_typer(preset_catalog_app, name="catalog") +def _warn_unmet_extension_dependencies(manager, manifest) -> None: + """Warn when a preset's declared extension dependencies are unsatisfied. + + A preset whose command overrides call into an extension is inert without + it, but the overrides still fall through to the core workflow, so nothing + breaks -- it just silently does less than the user expects. Naming the + missing extension and the command that installs it turns that silence into + something actionable. See issue #4231. + """ + unmet = manager.find_unmet_extension_dependencies(manifest) + if not unmet: + return + + console.print() + console.print("[yellow]![/yellow] This preset depends on extensions that are not satisfied:") + for dep in unmet: + extension_id = _escape_markup(dep["id"]) + reason = dep["reason"] + # The remediation has to match the reason. `extension add` refuses an + # already-installed extension without --force, and `extension update` + # only moves forward to the catalog release. A general PEP 440 + # constraint may require an exact version, an upper bound, or a + # downgrade, so do not promise that update will satisfy it. + if reason == "missing": + console.print(f" [yellow]{extension_id}[/yellow] is not installed") + remedy = f"specify extension add {extension_id}" + elif reason == "disabled": + console.print(f" [yellow]{extension_id}[/yellow] is installed but disabled") + remedy = f"specify extension enable {extension_id}" + else: + console.print( + f" [yellow]{extension_id}[/yellow] " + f"{_escape_markup(dep['installed'])} does not satisfy " + f"{_escape_markup(dep['version'])}" + ) + remedy = ( + "install a release of " + f"{extension_id} satisfying {_escape_markup(dep['version'])}" + ) + console.print(f" Fix with: {remedy}") + console.print() + console.print( + "[dim]The preset is installed and safe to use; the parts that rely on " + "these extensions will do nothing until this is resolved.[/dim]" + ) + + # ===== Preset Commands ===== @@ -283,6 +330,12 @@ def _validate_download_redirect(old_url, new_url): console.print("[red]Error:[/red] Specify a preset ID, --from URL, or --dev path") raise typer.Exit(1) + # Every install path above binds `manifest` and the no-source branch + # exits, so one call here covers --dev, --from, and catalog installs + # alike. Warns rather than fails: the preset is installed and its + # overrides fall through to the core workflow without the extension. + _warn_unmet_extension_dependencies(manager, manifest) + except PresetCompatibilityError as e: console.print(f"[red]Compatibility Error:[/red] {_escape_markup(str(e))}") raise typer.Exit(1) diff --git a/tests/test_presets.py b/tests/test_presets.py index 660a26d1b1..c1e85b691e 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -40,6 +40,8 @@ VALID_PRESET_TEMPLATE_TYPES, ) from specify_cli.extensions import ExtensionRegistry +from specify_cli._console import console +from specify_cli.presets._commands import _warn_unmet_extension_dependencies # ===== Fixtures ===== @@ -535,6 +537,61 @@ def test_same_name_different_type_templates_allowed( manifest = PresetManifest(manifest_path) assert len(manifest.templates) == 2 + def test_requires_extensions_absent_is_valid(self, temp_dir, valid_pack_data): + """A preset with no declared dependencies stays valid and reports none.""" + manifest_path = temp_dir / "preset.yml" + with open(manifest_path, 'w') as f: + yaml.dump(valid_pack_data, f) + assert PresetManifest(manifest_path).requires_extensions == [] + + def test_requires_extensions_accepts_both_forms(self, temp_dir, valid_pack_data): + """Bare ids and mappings normalize to the same shape.""" + valid_pack_data["requires"]["extensions"] = [ + "speckit-inventory", + {"id": "other-ext", "version": ">=1.2.0", "required": False}, + ] + manifest_path = temp_dir / "preset.yml" + with open(manifest_path, 'w') as f: + yaml.dump(valid_pack_data, f) + + assert PresetManifest(manifest_path).requires_extensions == [ + {"id": "speckit-inventory", "version": None, "required": True}, + {"id": "other-ext", "version": ">=1.2.0", "required": False}, + ] + + @pytest.mark.parametrize( + "bad, expected", + [ + ("speckit-inventory", "Invalid requires.extensions"), # str, not list + ({"id": "x"}, "Invalid requires.extensions"), # mapping, not list + ([123], r"Invalid requires\.extensions\[0\]"), # member not str/mapping + ([None], r"Invalid requires\.extensions\[0\]"), + ([{"version": ">=1"}], r"Missing requires\.extensions\[0\]\.id"), + ([{"id": 5}], r"Invalid requires\.extensions\[0\]\.id"), + ([{"id": "Bad_ID"}], r"Invalid requires\.extensions\[0\]\.id"), + (["Bad_ID"], r"Invalid requires\.extensions\[0\]\.id"), + ([{"id": "x", "version": 1.0}], r"Invalid requires\.extensions\[0\]\.version"), + ([{"id": "x", "version": " "}], r"Invalid requires\.extensions\[0\]\.version"), + ([{"id": "x", "version": "nonsense"}], r"Invalid requires\.extensions\[0\]\.version"), + ([{"id": "x", "required": "yes"}], r"Invalid requires\.extensions\[0\]\.required"), + ], + ) + def test_requires_extensions_rejects_malformed( + self, temp_dir, valid_pack_data, bad, expected + ): + """Malformed dependency declarations fail as PresetValidationError. + + Same reasoning as requires.speckit_version: an unvalidated value reaches + ``SpecifierSet`` or ``re.match`` later and surfaces as a bare TypeError + that no caller handles as a malformed manifest. + """ + valid_pack_data["requires"]["extensions"] = 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=expected): + PresetManifest(manifest_path) + # ===== PresetRegistry Tests ===== @@ -1099,6 +1156,205 @@ def test_list_installed_includes_priority(self, project_dir, pack_dir): assert installed[0]["priority"] == 3 +class TestPresetExtensionDependencies: + """Test find_unmet_extension_dependencies (issue #4231).""" + + @staticmethod + def _install_extension(project_dir, extension_id, version, enabled=True): + """Register an installed extension the way the extension installer does.""" + extensions_dir = project_dir / ".specify" / "extensions" + extensions_dir.mkdir(parents=True, exist_ok=True) + registry_path = extensions_dir / ".registry" + data = {"schema_version": "1.0", "extensions": {}} + if registry_path.exists(): + data = json.loads(registry_path.read_text(encoding="utf-8")) + data["extensions"][extension_id] = {"version": version, "enabled": enabled} + registry_path.write_text(json.dumps(data), encoding="utf-8") + + @staticmethod + def _manifest(temp_dir, valid_pack_data, declared): + valid_pack_data["requires"]["extensions"] = declared + manifest_path = temp_dir / "dep-preset.yml" + with open(manifest_path, 'w') as f: + yaml.dump(valid_pack_data, f) + return PresetManifest(manifest_path) + + def test_no_declared_dependencies_is_satisfied( + self, project_dir, temp_dir, valid_pack_data + ): + """A preset declaring nothing never reports an unmet dependency.""" + manifest_path = temp_dir / "plain-preset.yml" + with open(manifest_path, 'w') as f: + yaml.dump(valid_pack_data, f) + + manager = PresetManager(project_dir) + assert manager.find_unmet_extension_dependencies( + PresetManifest(manifest_path) + ) == [] + + def test_missing_dependency_is_reported( + self, project_dir, temp_dir, valid_pack_data + ): + """An uninstalled required extension is reported as missing.""" + manifest = self._manifest(temp_dir, valid_pack_data, ["speckit-inventory"]) + + unmet = PresetManager(project_dir).find_unmet_extension_dependencies(manifest) + + assert len(unmet) == 1 + assert unmet[0]["id"] == "speckit-inventory" + assert unmet[0]["reason"] == "missing" + assert unmet[0]["installed"] is None + + def test_installed_dependency_is_satisfied( + self, project_dir, temp_dir, valid_pack_data + ): + """An installed extension with no version constraint is satisfied.""" + self._install_extension(project_dir, "speckit-inventory", "0.1.0") + manifest = self._manifest(temp_dir, valid_pack_data, ["speckit-inventory"]) + + assert PresetManager(project_dir).find_unmet_extension_dependencies( + manifest + ) == [] + + def test_satisfied_version_constraint( + self, project_dir, temp_dir, valid_pack_data + ): + """A satisfied version constraint reports nothing.""" + self._install_extension(project_dir, "speckit-inventory", "1.5.0") + manifest = self._manifest( + temp_dir, valid_pack_data, + [{"id": "speckit-inventory", "version": ">=1.2.0"}], + ) + + assert PresetManager(project_dir).find_unmet_extension_dependencies( + manifest + ) == [] + + def test_unsatisfied_version_constraint_reports_both_versions( + self, project_dir, temp_dir, valid_pack_data + ): + """A version mismatch reports the installed version alongside the constraint.""" + self._install_extension(project_dir, "speckit-inventory", "0.1.0") + manifest = self._manifest( + temp_dir, valid_pack_data, + [{"id": "speckit-inventory", "version": ">=9.0.0"}], + ) + + unmet = PresetManager(project_dir).find_unmet_extension_dependencies(manifest) + + assert len(unmet) == 1 + assert unmet[0]["reason"] == "version" + assert unmet[0]["installed"] == "0.1.0" + assert unmet[0]["version"] == ">=9.0.0" + + def test_version_warning_does_not_promise_update_satisfies_constraint(self): + """Version remediation must handle constraints update cannot guarantee.""" + manager = MagicMock() + manager.find_unmet_extension_dependencies.return_value = [ + { + "id": "speckit-inventory", + "reason": "version", + "installed": "3.0.0", + "version": "<2", + } + ] + + with console.capture() as capture: + _warn_unmet_extension_dependencies(manager, MagicMock()) + + output = strip_ansi(capture.get()) + assert "install a release of speckit-inventory satisfying <2" in output + assert "specify extension update" not in output + + def test_optional_dependency_is_never_reported( + self, project_dir, temp_dir, valid_pack_data + ): + """`required: false` opts out of the warning even when absent.""" + manifest = self._manifest( + temp_dir, valid_pack_data, + [{"id": "speckit-inventory", "required": False}], + ) + + assert PresetManager(project_dir).find_unmet_extension_dependencies( + manifest + ) == [] + + def test_registry_entry_without_version_is_not_a_failure( + self, project_dir, temp_dir, valid_pack_data + ): + """An unusable registry version cannot be compared, so it is not invented + into a mismatch -- the extension is demonstrably installed.""" + self._install_extension(project_dir, "speckit-inventory", "0.1.0") + registry_path = project_dir / ".specify" / "extensions" / ".registry" + data = json.loads(registry_path.read_text(encoding="utf-8")) + data["extensions"]["speckit-inventory"]["version"] = None + registry_path.write_text(json.dumps(data), encoding="utf-8") + + manifest = self._manifest( + temp_dir, valid_pack_data, + [{"id": "speckit-inventory", "version": ">=9.0.0"}], + ) + + assert PresetManager(project_dir).find_unmet_extension_dependencies( + manifest + ) == [] + + def test_disabled_dependency_is_reported( + self, project_dir, temp_dir, valid_pack_data + ): + """A disabled extension contributes nothing, so it counts as unmet. + + Resolution skips disabled extensions, leaving the preset just as inert + as if the extension were absent -- but the registry entry exists, so a + presence-only check would call it satisfied and stay silent. + """ + self._install_extension(project_dir, "speckit-inventory", "0.1.0", enabled=False) + manifest = self._manifest(temp_dir, valid_pack_data, ["speckit-inventory"]) + + unmet = PresetManager(project_dir).find_unmet_extension_dependencies(manifest) + + assert len(unmet) == 1 + assert unmet[0]["reason"] == "disabled" + assert unmet[0]["installed"] == "0.1.0" + + def test_disabled_is_reported_ahead_of_version_mismatch( + self, project_dir, temp_dir, valid_pack_data + ): + """Enabling is the prerequisite, so it is reported before the version.""" + self._install_extension(project_dir, "speckit-inventory", "0.1.0", enabled=False) + manifest = self._manifest( + temp_dir, valid_pack_data, + [{"id": "speckit-inventory", "version": ">=9.0.0"}], + ) + + unmet = PresetManager(project_dir).find_unmet_extension_dependencies(manifest) + + assert [dep["reason"] for dep in unmet] == ["disabled"] + + def test_multiple_dependencies_report_independently( + self, project_dir, temp_dir, valid_pack_data + ): + """Each declared dependency is evaluated on its own.""" + self._install_extension(project_dir, "present-ext", "1.0.0") + self._install_extension(project_dir, "off-ext", "1.0.0", enabled=False) + manifest = self._manifest( + temp_dir, valid_pack_data, + [ + "present-ext", + "absent-ext", + "off-ext", + {"id": "opt-ext", "required": False}, + ], + ) + + unmet = PresetManager(project_dir).find_unmet_extension_dependencies(manifest) + + assert [(dep["id"], dep["reason"]) for dep in unmet] == [ + ("absent-ext", "missing"), + ("off-ext", "disabled"), + ] + + class TestRegistryPriority: """Test registry priority sorting."""