mirror of
https://github.com/github/spec-kit.git
synced 2026-08-15 14:02:04 +02:00
feat(extensions): accept provides.templates and provides.scripts in manifest (#4012)
* 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. * 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. * 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. * 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.
This commit is contained in:
chelsealong
@@ -1020,6 +1020,245 @@ provides:
|
||||
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:
|
||||
|
||||
@@ -11249,6 +11249,181 @@ class TestWrapStrategy:
|
||||
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/<name>.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/<name>.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"
|
||||
|
||||
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/<name>.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 =====
|
||||
|
||||
|
||||
Reference in New Issue
Block a user