From 4a85076de71966bf1de64140fa3f099117ccedde Mon Sep 17 00:00:00 2001 From: chelsealong Date: Fri, 7 Aug 2026 15:47:18 +0000 Subject: [PATCH 1/4] feat(extensions): accept provides.templates and provides.scripts in manifest Extensions could only formally declare commands under `provides` (plus config/hooks/events); templates and scripts shipped by an extension were picked up purely by filename convention, with no id, description, or metadata. Add optional `provides.templates` and `provides.scripts` sections to the extension manifest schema, mirroring the preset template shape minus an authorable `strategy` (extension artifacts always resolve as replace, so a present `strategy` key is now a validation error rather than a silently accepted no-op). ExtensionManifest gains `templates`/`scripts` properties so tooling can enumerate an extension's declared artifacts directly from the manifest. An extension may now satisfy the "must provide something" rule with only a template or script, not just a command/hook/event. Addresses the manifest-schema portion of #4010; resolver authoritative-vs-convention precedence for these new sections is left for a follow-up. --- extensions/EXTENSION-API-REFERENCE.md | 40 ++++- src/specify_cli/extensions/__init__.py | 99 +++++++++- tests/test_extensions.py | 239 +++++++++++++++++++++++++ 3 files changed, 375 insertions(+), 3 deletions(-) diff --git a/extensions/EXTENSION-API-REFERENCE.md b/extensions/EXTENSION-API-REFERENCE.md index bf85d18826..a7bece0b89 100644 --- a/extensions/EXTENSION-API-REFERENCE.md +++ b/extensions/EXTENSION-API-REFERENCE.md @@ -40,12 +40,25 @@ requires: required: boolean # Optional, default: false provides: - commands: # Required, at least one command + commands: # At least one of commands/templates/scripts/hooks/events required - name: string # Required, pattern: ^speckit\.[a-z0-9-]+\.[a-z0-9-]+$ file: string # Required, relative path to command file description: string # Required aliases: [string] # Optional, same pattern as name; namespace must match extension.id and must not shadow core or installed extension commands + templates: # Optional, array of declared templates. Always resolve + # as "replace" -- 'strategy' is not an authorable field here. + - name: string # Required, pattern: ^[a-z0-9-]+$ + file: string # Required, relative path to template file + description: string # Optional + + scripts: # Optional, array of declared scripts. Always resolve + # as "replace" -- 'strategy' is not an authorable field here. + - name: string # Required, pattern: ^[a-z0-9-]+$ + file: string # Required, relative path to script file + description: string # Optional + runtimes: [string] # Optional, subset of: bash, powershell, python + config: # Optional, array of config files - name: string # Config file name template: string # Template file path @@ -111,6 +124,29 @@ defaults: # Optional, default configuration values - **Examples**: `speckit.jira.specstoissues`, `speckit.linear.sync` - **Invalid**: `jira.specstoissues`, `speckit.command`, `speckit.jira.CreateIssues` +#### `provides.templates[].name` / `provides.scripts[].name` + +- **Type**: string +- **Pattern**: `^[a-z0-9-]+$` +- **Description**: Unlike commands, templates and scripts are not invoked by + name, so they use the same plain slug pattern as `extension.id` rather than + the namespaced command pattern. +- **Examples**: `myext-template`, `myext-collect` + +#### `provides.templates[].strategy` / `provides.scripts[].strategy` + +- Not an authorable field. Extension-contributed templates and scripts are + always resolved as `replace`; a manifest that includes a `strategy` key on + one of these entries is rejected with a `ValidationError`. Composable + strategies (`wrap`/`prepend`/`append`) are preset-only. + +#### `provides.scripts[].runtimes` + +- **Type**: array of strings +- **Values**: `bash`, `powershell`, `python` +- **Description**: Declares which runtimes the script supports. Purely + informational metadata — it is not used to select or invoke the script. + #### `hooks` - **Type**: object @@ -143,6 +179,8 @@ manifest.version # str: Version manifest.description # str: Description manifest.requires_speckit_version # str: Required spec-kit version manifest.commands # List[Dict]: Command definitions +manifest.templates # List[Dict]: Declared template definitions +manifest.scripts # List[Dict]: Declared script definitions manifest.hooks # Dict: Hook definitions ``` diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 9fa44d3809..2becae9bc1 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -61,6 +61,13 @@ ) EXTENSION_COMMAND_NAME_PATTERN = re.compile(r"^speckit\.([a-z0-9-]+)\.([a-z0-9-]+)$") +# Naming pattern for provides.templates / provides.scripts entries. Unlike +# commands, these are not namespaced (they aren't invoked via a command +# name), so they follow the same plain slug pattern as extension.id. +VALID_EXTENSION_ARTIFACT_NAME_PATTERN = re.compile(r"^[a-z0-9-]+$") + +VALID_SCRIPT_RUNTIMES = frozenset({"bash", "powershell", "python"}) + VALID_EFFECTS = frozenset({"read-only", "read-write"}) DEFAULT_HOOK_PRIORITY = 10 @@ -368,11 +375,17 @@ def _validate(self): f"Invalid provides: expected a mapping, got {type(provides).__name__}" ) commands = provides.get("commands", []) + templates = provides.get("templates", []) + scripts = provides.get("scripts", []) hooks = self.data.get("hooks") events = self.data.get("events") if "commands" in provides and not isinstance(commands, list): raise ValidationError("Invalid provides.commands: expected a list") + if "templates" in provides and not isinstance(templates, list): + raise ValidationError("Invalid provides.templates: expected a list") + if "scripts" in provides and not isinstance(scripts, list): + raise ValidationError("Invalid provides.scripts: expected a list") if "hooks" in self.data and not isinstance(hooks, dict): raise ValidationError("Invalid hooks: expected a mapping") if "events" in self.data: @@ -382,9 +395,17 @@ def _validate(self): has_commands = bool(commands) has_hooks = bool(hooks) has_events = bool(events) + has_templates = bool(templates) + has_scripts = bool(scripts) + + if not has_commands and not has_hooks and not has_events and not has_templates and not has_scripts: + raise ValidationError( + "Extension must provide at least one command, hook, or event " + "(or a declared template/script)" + ) - if not has_commands and not has_hooks and not has_events: - raise ValidationError("Extension must provide at least one command, hook, or event") + self._validate_provided_artifacts(templates, section="templates", singular="template") + self._validate_provided_artifacts(scripts, section="scripts", singular="script") # Validate hook values (if present). # Each event is a single mapping or a list of mappings. @@ -545,6 +566,70 @@ def _validate(self): f"The extension author should update the manifest." ) + @staticmethod + def _validate_provided_artifacts(entries: List[Any], section: str, singular: str) -> None: + """Validate provides.templates / provides.scripts entries. + + Mirrors the shape/path-safety checks PresetManifest applies to its + non-command templates, minus 'type' (the section name already + distinguishes template vs script) and 'strategy' (extension-provided + artifacts are always 'replace' -- see the forced-replace resolver + behavior for extension layers in presets/__init__.py). A present + 'strategy' key is rejected rather than silently ignored, so an author + who copies a preset-style entry gets a clear error instead of a + silently-dropped field. + """ + for entry in entries: + if not isinstance(entry, dict): + raise ValidationError( + f"Each entry in 'provides.{section}' must be a mapping" + ) + if "name" not in entry or "file" not in entry: + raise ValidationError(f"{singular.capitalize()} missing 'name' or 'file'") + + name = entry["name"] + if not isinstance(name, str): + raise ValidationError( + f"Invalid {singular} name: expected a string, got {type(name).__name__}" + ) + if not VALID_EXTENSION_ARTIFACT_NAME_PATTERN.match(name): + raise ValidationError( + f"Invalid {singular} name '{name}': " + "must be lowercase alphanumeric with hyphens only" + ) + + file_value = entry["file"] + reason = relative_extension_path_violation(file_value) + if reason: + label = repr(file_value) if isinstance(file_value, str) else f"for {singular} '{name}'" + raise ValidationError(f"Invalid {singular} 'file' {label}: {reason}") + + if "description" in entry and not isinstance(entry["description"], str): + raise ValidationError( + f"Invalid {singular} description for '{name}': expected a string" + ) + + if "strategy" in entry: + raise ValidationError( + f"Invalid {singular} entry '{name}': 'strategy' is not authorable for " + "extension-provided artifacts, which always use 'replace' semantics" + ) + + if section == "scripts" and "runtimes" in entry: + runtimes = entry["runtimes"] + if not isinstance(runtimes, list) or not all( + isinstance(r, str) for r in runtimes + ): + raise ValidationError( + f"Invalid runtimes for script '{name}': expected a list of strings" + ) + invalid = sorted(set(runtimes) - VALID_SCRIPT_RUNTIMES) + if invalid: + raise ValidationError( + f"Invalid runtimes {invalid} for script '{name}': " + f"must be one of {sorted(VALID_SCRIPT_RUNTIMES)}" + ) + @staticmethod def _try_correct_command_name(name: str, ext_id: str) -> Optional[str]: """Try to auto-correct a non-conforming command name to the required pattern. @@ -615,6 +700,16 @@ def config(self) -> List[Dict[str, Any]]: return [] return raw + @property + def templates(self) -> List[Dict[str, Any]]: + """Get list of declared templates (provides.templates).""" + return self.data.get("provides", {}).get("templates", []) + + @property + def scripts(self) -> List[Dict[str, Any]]: + """Get list of declared scripts (provides.scripts).""" + return self.data.get("provides", {}).get("scripts", []) + @property def hooks(self) -> Dict[str, Any]: """Get hook definitions.""" diff --git a/tests/test_extensions.py b/tests/test_extensions.py index d668019087..6508826dc9 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -1020,6 +1020,245 @@ def test_manifest_hash(self, extension_dir): assert len(hash_value) > 10 +class TestExtensionManifestTemplatesAndScripts: + """Tests for the optional provides.templates / provides.scripts sections.""" + + def test_templates_and_scripts_declared(self, temp_dir, valid_manifest_data): + """A manifest declaring templates and scripts exposes them via properties.""" + import yaml + + valid_manifest_data["provides"]["templates"] = [ + { + "name": "myext-template", + "file": "templates/myext-template.md", + "description": "Report scaffold contributed by myext", + } + ] + valid_manifest_data["provides"]["scripts"] = [ + { + "name": "myext-collect", + "file": "scripts/bash/myext-collect.sh", + "description": "Data-collection helper", + "runtimes": ["bash", "python"], + } + ] + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + + manifest = ExtensionManifest(manifest_path) + + assert manifest.templates == valid_manifest_data["provides"]["templates"] + assert manifest.scripts == valid_manifest_data["provides"]["scripts"] + assert manifest.warnings == [] + + def test_templates_only_extension_is_valid(self, temp_dir, valid_manifest_data): + """An extension with only a declared template (no commands/hooks/events) is valid.""" + import yaml + + valid_manifest_data["provides"]["commands"] = [] + valid_manifest_data.pop("hooks", None) + valid_manifest_data["provides"]["templates"] = [ + {"name": "myext-template", "file": "templates/myext-template.md"} + ] + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + + manifest = ExtensionManifest(manifest_path) + assert len(manifest.templates) == 1 + assert len(manifest.commands) == 0 + + def test_scripts_only_extension_is_valid(self, temp_dir, valid_manifest_data): + """An extension with only a declared script (no commands/hooks/events) is valid.""" + import yaml + + valid_manifest_data["provides"]["commands"] = [] + valid_manifest_data.pop("hooks", None) + valid_manifest_data["provides"]["scripts"] = [ + {"name": "myext-collect", "file": "scripts/bash/myext-collect.sh"} + ] + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + + manifest = ExtensionManifest(manifest_path) + assert len(manifest.scripts) == 1 + + def test_no_provides_at_all_still_rejected(self, temp_dir, valid_manifest_data): + """Without commands, hooks, events, templates, or scripts the manifest is + still rejected — the relaxed rule only widens what counts, it doesn't + drop the requirement that an extension provide *something*.""" + import yaml + + valid_manifest_data["provides"]["commands"] = [] + valid_manifest_data.pop("hooks", None) + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + + with pytest.raises(ValidationError, match="must provide at least one command, hook, or event"): + ExtensionManifest(manifest_path) + + @pytest.mark.parametrize("section", ["templates", "scripts"]) + def test_provides_section_must_be_a_list(self, temp_dir, valid_manifest_data, section): + """provides.templates / provides.scripts must be a list, not e.g. a mapping.""" + import yaml + + valid_manifest_data["provides"][section] = {"not": "a list"} + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + + with pytest.raises(ValidationError, match=f"Invalid provides.{section}: expected a list"): + ExtensionManifest(manifest_path) + + @pytest.mark.parametrize("section", ["templates", "scripts"]) + def test_provides_entry_must_be_a_mapping(self, temp_dir, valid_manifest_data, section): + """Each provides.templates / provides.scripts entry must be a mapping.""" + import yaml + + valid_manifest_data["provides"][section] = ["not-a-mapping"] + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + + with pytest.raises(ValidationError, match=f"Each entry in 'provides.{section}' must be a mapping"): + ExtensionManifest(manifest_path) + + @pytest.mark.parametrize("section", ["templates", "scripts"]) + def test_provides_entry_missing_name_or_file(self, temp_dir, valid_manifest_data, section): + """Each entry requires both 'name' and 'file'.""" + import yaml + + valid_manifest_data["provides"][section] = [{"name": "only-a-name"}] + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + + with pytest.raises(ValidationError, match="missing 'name' or 'file'"): + ExtensionManifest(manifest_path) + + @pytest.mark.parametrize("section", ["templates", "scripts"]) + def test_provides_entry_invalid_name_format(self, temp_dir, valid_manifest_data, section): + """Names must be lowercase alphanumeric with hyphens only.""" + import yaml + + valid_manifest_data["provides"][section] = [ + {"name": "Bad_Name", "file": f"{section}/bad.txt"} + ] + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + + with pytest.raises(ValidationError, match="must be lowercase alphanumeric with hyphens only"): + ExtensionManifest(manifest_path) + + @pytest.mark.parametrize("section", ["templates", "scripts"]) + def test_provides_entry_path_traversal_rejected(self, temp_dir, valid_manifest_data, section): + """The 'file' field is checked with the same path-safety policy as commands.""" + import yaml + + valid_manifest_data["provides"][section] = [ + {"name": "escape", "file": "../evil"} + ] + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + + with pytest.raises(ValidationError, match="relative path within the extension directory"): + ExtensionManifest(manifest_path) + + @pytest.mark.parametrize("section", ["templates", "scripts"]) + def test_provides_entry_strategy_rejected(self, temp_dir, valid_manifest_data, section): + """'strategy' is preset-only; extension-provided artifacts are always 'replace'.""" + import yaml + + valid_manifest_data["provides"][section] = [ + {"name": "has-strategy", "file": f"{section}/x.txt", "strategy": "replace"} + ] + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + + with pytest.raises(ValidationError, match="'strategy' is not authorable"): + ExtensionManifest(manifest_path) + + def test_script_runtimes_accepted(self, temp_dir, valid_manifest_data): + """A valid 'runtimes' list on a script entry is accepted as-is.""" + import yaml + + valid_manifest_data["provides"]["scripts"] = [ + { + "name": "myext-collect", + "file": "scripts/bash/myext-collect.sh", + "runtimes": ["bash", "powershell", "python"], + } + ] + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + + manifest = ExtensionManifest(manifest_path) + assert manifest.scripts[0]["runtimes"] == ["bash", "powershell", "python"] + + def test_script_runtimes_must_be_a_list_of_strings(self, temp_dir, valid_manifest_data): + """A non-list 'runtimes' value is rejected.""" + import yaml + + valid_manifest_data["provides"]["scripts"] = [ + {"name": "myext-collect", "file": "scripts/bash/myext-collect.sh", "runtimes": "bash"} + ] + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + + with pytest.raises(ValidationError, match="expected a list of strings"): + ExtensionManifest(manifest_path) + + def test_script_runtimes_rejects_unknown_runtime(self, temp_dir, valid_manifest_data): + """An unrecognized runtime name is rejected with the valid set in the message.""" + import yaml + + valid_manifest_data["provides"]["scripts"] = [ + {"name": "myext-collect", "file": "scripts/bash/myext-collect.sh", "runtimes": ["ruby"]} + ] + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + + with pytest.raises(ValidationError, match="Invalid runtimes.*must be one of"): + ExtensionManifest(manifest_path) + + def test_provides_entry_description_must_be_a_string(self, temp_dir, valid_manifest_data): + """An optional 'description' field must be a string when present.""" + import yaml + + valid_manifest_data["provides"]["templates"] = [ + {"name": "myext-template", "file": "templates/myext-template.md", "description": 123} + ] + + manifest_path = temp_dir / "extension.yml" + with open(manifest_path, 'w', encoding="utf-8") as f: + yaml.dump(valid_manifest_data, f) + + with pytest.raises(ValidationError, match="expected a string"): + ExtensionManifest(manifest_path) + + # ===== ExtensionRegistry Tests ===== class TestExtensionRegistry: From f0732151a8aa6c020ff60284a755524238f0e712 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Fri, 7 Aug 2026 16:05:58 +0000 Subject: [PATCH 2/4] fix(presets): wire extension-declared templates/scripts into resolver collect_all_layers only consulted ExtensionManifest for command resolution, leaving provides.templates/.scripts purely decorative -- a declared entry whose file didn't sit at the conventional path was validated but never resolved. Extend the existing manifest-fallback branch to cover template_type "template" and "script" the same way it already does "command": convention lookup first, manifest lookup as fallback so undeclared on-disk files keep resolving unchanged. --- src/specify_cli/presets/__init__.py | 21 +++++--- tests/test_presets.py | 78 +++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 7 deletions(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 157bac6c46..8ca0acdc7b 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -5426,18 +5426,25 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: continue # Try convention-based lookup first candidate = _find_in_subdirs(ext_dir) - # If not found and this is a command, check extension manifest - if candidate is None and template_type == "command": + # If not found, check the extension manifest for a declared + # command/template/script entry with this name. + if candidate is None and template_type in ("command", "template", "script"): ext_manifest_path = ext_dir / "extension.yml" if ext_manifest_path.exists(): try: from ..extensions import ExtensionManifest, ValidationError as ExtValidationError ext_manifest = ExtensionManifest(ext_manifest_path) - for cmd in ext_manifest.commands: - if cmd.get("name") == template_name: - cmd_file = cmd.get("file") - if cmd_file: - c = ext_dir / cmd_file + if template_type == "command": + entries = ext_manifest.commands + elif template_type == "template": + entries = ext_manifest.templates + else: + entries = ext_manifest.scripts + for entry in entries: + if entry.get("name") == template_name: + entry_file = entry.get("file") + if entry_file: + c = ext_dir / entry_file if c.exists(): candidate = c break diff --git a/tests/test_presets.py b/tests/test_presets.py index 80f2ddab58..1c7e8b11d9 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -11220,6 +11220,84 @@ def test_extension_command_resolves_via_manifest_when_filename_differs(self, pro assert "# Selftest Core" in result assert "{CORE_TEMPLATE}" not in result + def test_extension_template_resolves_via_manifest_when_filename_differs(self, project_dir): + """provides.templates entries resolve via extension.yml when the file + doesn't sit at the conventional path. + + Regression coverage for #4010: manifest-declared templates/scripts + must actually be consulted by the resolver, not just accepted by + manifest validation. + """ + ext_dir = project_dir / ".specify" / "extensions" / "reportext" + tmpl_dir = ext_dir / "templates" / "nested" + tmpl_dir.mkdir(parents=True, exist_ok=True) + + # File lives at a path convention-based lookup (templates/.md) + # would never find. + (tmpl_dir / "actual.md").write_text("# Report Scaffold\n") + (ext_dir / "extension.yml").write_text( + "schema_version: '1.0'\n" + "extension:\n id: reportext\n name: Report Ext\n version: 1.0.0\n" + " description: test\n author: test\n repository: https://example.com\n" + " license: MIT\n" + "requires:\n speckit_version: '>=0.2.0'\n" + "provides:\n" + " templates:\n" + " - name: report-scaffold\n" + " file: templates/nested/actual.md\n" + " description: Report scaffold\n" + ) + + resolver = PresetResolver(project_dir) + layers = resolver.collect_all_layers("report-scaffold", "template") + assert layers, "expected the manifest-declared template to resolve" + assert layers[0]["path"] == tmpl_dir / "actual.md" + assert layers[0]["strategy"] == "replace" + + def test_extension_script_resolves_via_manifest_when_filename_differs(self, project_dir): + """provides.scripts entries resolve via extension.yml when the file + doesn't sit at the conventional path.""" + ext_dir = project_dir / ".specify" / "extensions" / "collectext" + script_dir = ext_dir / "scripts" / "bash" + script_dir.mkdir(parents=True, exist_ok=True) + + # File is under scripts/bash/, not directly under scripts/, so + # convention-based lookup (scripts/.sh) would never find it. + (script_dir / "collect.sh").write_text("#!/usr/bin/env bash\necho collect\n") + (ext_dir / "extension.yml").write_text( + "schema_version: '1.0'\n" + "extension:\n id: collectext\n name: Collect Ext\n version: 1.0.0\n" + " description: test\n author: test\n repository: https://example.com\n" + " license: MIT\n" + "requires:\n speckit_version: '>=0.2.0'\n" + "provides:\n" + " scripts:\n" + " - name: myext-collect\n" + " file: scripts/bash/collect.sh\n" + " description: Data-collection helper\n" + " runtimes: [bash]\n" + ) + + resolver = PresetResolver(project_dir) + layers = resolver.collect_all_layers("myext-collect", "script") + assert layers, "expected the manifest-declared script to resolve" + assert layers[0]["path"] == script_dir / "collect.sh" + assert layers[0]["strategy"] == "replace" + + def test_extension_template_convention_lookup_unaffected_when_undeclared(self, project_dir): + """An extension template with no manifest entry still resolves via + the pre-existing filename convention (no regression).""" + ext_dir = project_dir / ".specify" / "extensions" / "conventionext" + tmpl_dir = ext_dir / "templates" + tmpl_dir.mkdir(parents=True, exist_ok=True) + (tmpl_dir / "legacy-template.md").write_text("# Legacy Template\n") + # No extension.yml at all -- purely convention-based, unregistered extension. + + resolver = PresetResolver(project_dir) + layers = resolver.collect_all_layers("legacy-template", "template") + assert layers, "expected convention-based lookup to still find the template" + assert layers[0]["path"] == tmpl_dir / "legacy-template.md" + # ===== _replay_wraps_for_command Tests ===== From 0070d25b59c933056b18a684e15e29c4666317df Mon Sep 17 00:00:00 2001 From: chelsealong Date: Fri, 7 Aug 2026 17:06:58 +0000 Subject: [PATCH 3/4] fix(presets): make extension manifest lookup authoritative over convention Copilot review on #4012 found the manifest-declared template/script lookup was gated on convention lookup missing first, so a stale conventional file could shadow a declared entry at a non-conventional path, and resolve() never consulted the manifest at all (only collect_all_layers() did). Add a shared _extension_manifest_declared_template() helper and check it before convention-based lookup in both resolve() and collect_all_layers(), mirroring the preset manifest precedence. Also update EXTENSION-DEVELOPMENT-GUIDE.md, which still claimed provides only supports commands and required a command or hook. --- extensions/EXTENSION-DEVELOPMENT-GUIDE.md | 21 ++++- src/specify_cli/presets/__init__.py | 101 ++++++++++++++++------ tests/test_presets.py | 97 +++++++++++++++++++++ 3 files changed, 189 insertions(+), 30 deletions(-) diff --git a/extensions/EXTENSION-DEVELOPMENT-GUIDE.md b/extensions/EXTENSION-DEVELOPMENT-GUIDE.md index 5da95c9d54..5030565b14 100644 --- a/extensions/EXTENSION-DEVELOPMENT-GUIDE.md +++ b/extensions/EXTENSION-DEVELOPMENT-GUIDE.md @@ -177,9 +177,11 @@ Compatibility requirements. What the extension provides. -**Optional sub-fields**: +**Optional sub-fields** (at least one of `commands`, `templates`, `scripts`, `hooks`, or `events` is required): -- `commands`: Array of command objects (at least one command or hook is required) +- `commands`: Array of command objects +- `templates`: Array of template objects +- `scripts`: Array of script objects **Command object**: @@ -188,6 +190,21 @@ What the extension provides. - `description`: Command description (optional) - `aliases`: Alternative command names (optional, array; each must match `speckit.{ext-id}.{command}`) +**Template object**: + +- `name`: Template name (lowercase, alphanumeric, hyphens — e.g. `myext-template`) +- `file`: Path to template file (relative to extension root) +- `description`: Template description (optional) + +**Script object**: + +- `name`: Script name (lowercase, alphanumeric, hyphens — e.g. `myext-collect`) +- `file`: Path to script file (relative to extension root) +- `description`: Script description (optional) +- `runtimes`: Runtimes the script supports (optional, array; subset of `bash`, `powershell`, `python` — informational only, not used to select or invoke the script) + +Extension-provided templates and scripts always resolve as `replace`; a manifest that includes a `strategy` key on one of these entries is rejected with a `ValidationError`. Composable strategies (`wrap`/`prepend`/`append`) are preset-only. + ### Optional Fields #### `hooks` diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 8ca0acdc7b..8ed3093e5e 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -4979,6 +4979,60 @@ def _manifest_declared_template( return tmpl, None return None, None + def _extension_manifest_declared_template( + self, ext_dir: Path, template_name: str, template_type: str + ) -> tuple[dict | None, Path | None]: + """Resolve an extension's manifest-declared command/template/script entry and usable file. + + Mirrors ``_manifest_declared_template`` (for presets): returns ``(entry, candidate)`` + where ``entry`` is the matching ``provides.`` mapping, or ``None`` if the + extension has no (valid) manifest or doesn't declare this ``(name, type)``. + ``candidate`` is the declared ``file:`` resolved under ``ext_dir`` IFF it is a + regular file that stays within ``ext_dir`` (guards against path traversal via a + malformed manifest, mirroring ``resolve_extension_command_via_manifest``); + ``None`` otherwise. + + The manifest is authoritative: when ``entry`` is not ``None`` but ``candidate`` is + ``None``, callers must NOT fall back to convention-based lookup — that would mask + a typo or pick up an undeclared file. Shared by ``resolve()`` and + ``collect_all_layers()`` so their manifest-first resolution cannot silently + diverge (the divergence flagged in review on #4012). + """ + if template_type not in ("command", "template", "script"): + return None, None + ext_manifest_path = ext_dir / "extension.yml" + if not ext_manifest_path.exists(): + return None, None + from ..extensions import ExtensionManifest, ValidationError as ExtValidationError + + try: + ext_manifest = ExtensionManifest(ext_manifest_path) + except (ExtValidationError, yaml.YAMLError, OSError, TypeError, AttributeError): + return None, None + if template_type == "command": + entries = ext_manifest.commands + elif template_type == "template": + entries = ext_manifest.templates + else: + entries = ext_manifest.scripts + for entry in entries: + if entry.get("name") != template_name: + continue + file_rel = entry.get("file") + if not file_rel: + return entry, None + rel_path = Path(file_rel) + if rel_path.is_absolute(): + return entry, None + try: + ext_root = ext_dir.resolve() + candidate = (ext_root / rel_path).resolve() + candidate.relative_to(ext_root) # raises ValueError if outside + except (OSError, ValueError): + return entry, None + return entry, (candidate if candidate.is_file() else None) + return None, None + def _get_all_extensions_by_priority(self) -> list[tuple[int, str, dict | None]]: """Build unified list of registered and unregistered extensions sorted by priority. @@ -5115,6 +5169,16 @@ def resolve( ext_dir = self.extensions_dir / ext_id if not ext_dir.is_dir(): continue + # The extension manifest is authoritative, same as preset manifests + # above: check it before convention-based lookup so a declared entry + # at a non-conventional path wins over a stale conventional file. + entry, manifest_candidate = self._extension_manifest_declared_template( + ext_dir, template_name, template_type + ) + if manifest_candidate is not None: + return manifest_candidate + if entry is not None: + continue for subdir in subdirs: if subdir: candidate = ext_dir / subdir / f"{template_name}{ext}" @@ -5424,34 +5488,15 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: ext_dir = self.extensions_dir / ext_id if not ext_dir.is_dir(): continue - # Try convention-based lookup first - candidate = _find_in_subdirs(ext_dir) - # If not found, check the extension manifest for a declared - # command/template/script entry with this name. - if candidate is None and template_type in ("command", "template", "script"): - ext_manifest_path = ext_dir / "extension.yml" - if ext_manifest_path.exists(): - try: - from ..extensions import ExtensionManifest, ValidationError as ExtValidationError - ext_manifest = ExtensionManifest(ext_manifest_path) - if template_type == "command": - entries = ext_manifest.commands - elif template_type == "template": - entries = ext_manifest.templates - else: - entries = ext_manifest.scripts - for entry in entries: - if entry.get("name") == template_name: - entry_file = entry.get("file") - if entry_file: - c = ext_dir / entry_file - if c.exists(): - candidate = c - break - except (ExtValidationError, yaml.YAMLError): - # Invalid extension manifest — fall back to - # convention-based lookup (already attempted above). - pass + # The extension manifest is authoritative, same as preset manifests + # above: check it before convention-based lookup so a declared entry + # at a non-conventional path wins over a stale conventional file, and + # a declared-but-missing file isn't silently masked by convention. + entry, candidate = self._extension_manifest_declared_template( + ext_dir, template_name, template_type + ) + if entry is None: + candidate = _find_in_subdirs(ext_dir) if candidate: if ext_meta: version = ext_meta.get("version", "?") diff --git a/tests/test_presets.py b/tests/test_presets.py index 1c7e8b11d9..fe300584bf 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -11298,6 +11298,103 @@ def test_extension_template_convention_lookup_unaffected_when_undeclared(self, p assert layers, "expected convention-based lookup to still find the template" assert layers[0]["path"] == tmpl_dir / "legacy-template.md" + def test_extension_manifest_wins_over_stale_conventional_file(self, project_dir): + """A declared entry is authoritative even when a stale file also sits at + the conventional path (templates/.md) — the manifest must win, + not the convention lookup, per #4010's acceptance criteria.""" + ext_dir = project_dir / ".specify" / "extensions" / "bothpathsext" + (ext_dir / "templates").mkdir(parents=True, exist_ok=True) + (ext_dir / "custom").mkdir(parents=True, exist_ok=True) + + # Stale file at the conventional path -- must NOT win. + (ext_dir / "templates" / "report-scaffold.md").write_text("# Stale\n") + # Declared file at a non-conventional path -- must win. + (ext_dir / "custom" / "bar.md").write_text("# Actual\n") + (ext_dir / "extension.yml").write_text( + "schema_version: '1.0'\n" + "extension:\n id: bothpathsext\n name: Both Paths Ext\n version: 1.0.0\n" + " description: test\n author: test\n repository: https://example.com\n" + " license: MIT\n" + "requires:\n speckit_version: '>=0.2.0'\n" + "provides:\n" + " templates:\n" + " - name: report-scaffold\n" + " file: custom/bar.md\n" + " description: Report scaffold\n" + ) + + resolver = PresetResolver(project_dir) + + layers = resolver.collect_all_layers("report-scaffold", "template") + assert layers, "expected the manifest-declared template to resolve" + assert layers[0]["path"] == ext_dir / "custom" / "bar.md" + + resolved = resolver.resolve("report-scaffold", "template") + assert resolved == ext_dir / "custom" / "bar.md" + + with_source = resolver.resolve_with_source("report-scaffold", "template") + assert with_source["path"] == str(ext_dir / "custom" / "bar.md") + + def test_extension_manifest_declared_but_missing_file_does_not_fall_back(self, project_dir): + """A declared entry whose file is missing is authoritative -- the + resolver must not silently mask the typo by falling back to a + conventional file that happens to also exist.""" + ext_dir = project_dir / ".specify" / "extensions" / "missingfileext" + (ext_dir / "scripts").mkdir(parents=True, exist_ok=True) + + # A conventional file exists, but the manifest declares a different, + # non-existent file for the same name. + (ext_dir / "scripts" / "myext-collect.sh").write_text("#!/usr/bin/env bash\necho legacy\n") + (ext_dir / "extension.yml").write_text( + "schema_version: '1.0'\n" + "extension:\n id: missingfileext\n name: Missing File Ext\n version: 1.0.0\n" + " description: test\n author: test\n repository: https://example.com\n" + " license: MIT\n" + "requires:\n speckit_version: '>=0.2.0'\n" + "provides:\n" + " scripts:\n" + " - name: myext-collect\n" + " file: scripts/does-not-exist.sh\n" + " description: Data-collection helper\n" + ) + + resolver = PresetResolver(project_dir) + + assert resolver.collect_all_layers("myext-collect", "script") == [] + assert resolver.resolve("myext-collect", "script") is None + + def test_extension_script_resolve_and_resolve_with_source_parity(self, project_dir): + """resolve() and resolve_with_source() must find a manifest-declared + script at a non-conventional path, matching collect_all_layers().""" + ext_dir = project_dir / ".specify" / "extensions" / "collectext2" + script_dir = ext_dir / "scripts" / "bash" + script_dir.mkdir(parents=True, exist_ok=True) + + (script_dir / "collect.sh").write_text("#!/usr/bin/env bash\necho collect\n") + (ext_dir / "extension.yml").write_text( + "schema_version: '1.0'\n" + "extension:\n id: collectext2\n name: Collect Ext 2\n version: 1.0.0\n" + " description: test\n author: test\n repository: https://example.com\n" + " license: MIT\n" + "requires:\n speckit_version: '>=0.2.0'\n" + "provides:\n" + " scripts:\n" + " - name: myext-collect2\n" + " file: scripts/bash/collect.sh\n" + " description: Data-collection helper\n" + " runtimes: [bash]\n" + ) + + resolver = PresetResolver(project_dir) + + resolved = resolver.resolve("myext-collect2", "script") + assert resolved == script_dir / "collect.sh" + + with_source = resolver.resolve_with_source("myext-collect2", "script") + assert with_source is not None + assert with_source["path"] == str(script_dir / "collect.sh") + assert with_source["source"] == "extension:collectext2 (unregistered)" + # ===== _replay_wraps_for_command Tests ===== From 467c0f7fab713a1dad5b8b3028d4c3f4d02fe5ab Mon Sep 17 00:00:00 2001 From: chelsealong Date: Fri, 7 Aug 2026 17:35:13 +0000 Subject: [PATCH 4/4] fix(presets): stop resolving symlinks in extension manifest candidate path _extension_manifest_declared_template() resolved ext_dir/rel_path before returning it, which follows symlinks in ext_dir's ancestors (e.g. macOS's symlinked tmp dir) and diverges from the unresolved paths convention-based lookup returns for the same directory. Resolve only for the traversal containment check; return the unresolved candidate. Fixes the 4 CI test failures across all OS/Python matrix jobs on #4012. --- src/specify_cli/presets/__init__.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 8ed3093e5e..fac44d59c2 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -5024,10 +5024,14 @@ def _extension_manifest_declared_template( rel_path = Path(file_rel) if rel_path.is_absolute(): return entry, None + candidate = ext_dir / rel_path try: - ext_root = ext_dir.resolve() - candidate = (ext_root / rel_path).resolve() - candidate.relative_to(ext_root) # raises ValueError if outside + # Resolve only for the containment check, not for the + # returned path -- resolving the returned path would follow + # symlinks in ext_dir's ancestors (e.g. a symlinked tmp dir + # on macOS) and diverge from the unresolved paths convention + # lookup returns for the same directory. + candidate.resolve().relative_to(ext_dir.resolve()) # raises ValueError if outside except (OSError, ValueError): return entry, None return entry, (candidate if candidate.is_file() else None)