mirror of
https://github.com/github/spec-kit.git
synced 2026-03-20 04:13:08 +00:00
fix: address Copilot PR review comments (round 2)
- Fix init --preset error masking: distinguish "not found" from real errors - Fix bash resolve_template: skip hidden dirs in extensions (match Python/PS) - Fix temp dir leaks in tests: use temp_dir fixture instead of mkdtemp - Fix self-test catalog entry: add note that it's local-only (no download_url) - Fix Windows path issue in resolve_with_source: use Path.relative_to() - Fix skill restore path: use project's .specify/templates/commands/ not source tree - Add encoding="utf-8" to all file read/write in agents.py - Update test to set up core command templates for skill restoration
This commit is contained in:
@@ -5,11 +5,12 @@
|
|||||||
"presets": {
|
"presets": {
|
||||||
"self-test": {
|
"self-test": {
|
||||||
"name": "Self-Test Preset",
|
"name": "Self-Test Preset",
|
||||||
"description": "A preset that overrides all core templates for testing purposes",
|
"description": "A preset that overrides all core templates for testing purposes. Install with --dev from the repo.",
|
||||||
"author": "github",
|
"author": "github",
|
||||||
"version": "1.0.0",
|
"version": "1.0.0",
|
||||||
"repository": "https://github.com/github/spec-kit",
|
"repository": "https://github.com/github/spec-kit",
|
||||||
"license": "MIT",
|
"license": "MIT",
|
||||||
|
"note": "This preset is bundled with the repo and intended for local testing only. Install via: specify preset add --dev presets/self-test",
|
||||||
"requires": {
|
"requires": {
|
||||||
"speckit_version": ">=0.1.0"
|
"speckit_version": ">=0.1.0"
|
||||||
},
|
},
|
||||||
|
|||||||
@@ -213,6 +213,8 @@ except Exception:
|
|||||||
if [ -d "$ext_dir" ]; then
|
if [ -d "$ext_dir" ]; then
|
||||||
for ext in "$ext_dir"/*/; do
|
for ext in "$ext_dir"/*/; do
|
||||||
[ -d "$ext" ] || continue
|
[ -d "$ext" ] || continue
|
||||||
|
# Skip hidden directories (e.g. .backup, .cache)
|
||||||
|
case "$(basename "$ext")" in .*) continue;; esac
|
||||||
local candidate="$ext/templates/${template_name}.md"
|
local candidate="$ext/templates/${template_name}.md"
|
||||||
[ -f "$candidate" ] && echo "$candidate" && return 0
|
[ -f "$candidate" ] && echo "$candidate" && return 0
|
||||||
done
|
done
|
||||||
|
|||||||
@@ -1600,17 +1600,21 @@ def init(
|
|||||||
preset_manager.install_from_directory(local_path, speckit_ver)
|
preset_manager.install_from_directory(local_path, speckit_ver)
|
||||||
else:
|
else:
|
||||||
preset_catalog = PresetCatalog(project_path)
|
preset_catalog = PresetCatalog(project_path)
|
||||||
try:
|
pack_info = preset_catalog.get_pack_info(preset)
|
||||||
zip_path = preset_catalog.download_pack(preset)
|
if not pack_info:
|
||||||
preset_manager.install_from_zip(zip_path, speckit_ver)
|
|
||||||
# Clean up downloaded ZIP to avoid cache accumulation
|
|
||||||
try:
|
|
||||||
zip_path.unlink(missing_ok=True)
|
|
||||||
except OSError:
|
|
||||||
# Best-effort cleanup; failure to delete is non-fatal
|
|
||||||
pass
|
|
||||||
except PresetError:
|
|
||||||
console.print(f"[yellow]Warning:[/yellow] Preset '{preset}' not found in catalog. Skipping.")
|
console.print(f"[yellow]Warning:[/yellow] Preset '{preset}' not found in catalog. Skipping.")
|
||||||
|
else:
|
||||||
|
try:
|
||||||
|
zip_path = preset_catalog.download_pack(preset)
|
||||||
|
preset_manager.install_from_zip(zip_path, speckit_ver)
|
||||||
|
# Clean up downloaded ZIP to avoid cache accumulation
|
||||||
|
try:
|
||||||
|
zip_path.unlink(missing_ok=True)
|
||||||
|
except OSError:
|
||||||
|
# Best-effort cleanup; failure to delete is non-fatal
|
||||||
|
pass
|
||||||
|
except PresetError as preset_err:
|
||||||
|
console.print(f"[yellow]Warning:[/yellow] Failed to install preset '{preset}': {preset_err}")
|
||||||
except Exception as preset_err:
|
except Exception as preset_err:
|
||||||
console.print(f"[yellow]Warning:[/yellow] Failed to install preset: {preset_err}")
|
console.print(f"[yellow]Warning:[/yellow] Failed to install preset: {preset_err}")
|
||||||
|
|
||||||
|
|||||||
@@ -301,7 +301,7 @@ class CommandRegistrar:
|
|||||||
if not source_file.exists():
|
if not source_file.exists():
|
||||||
continue
|
continue
|
||||||
|
|
||||||
content = source_file.read_text()
|
content = source_file.read_text(encoding="utf-8")
|
||||||
frontmatter, body = self.parse_frontmatter(content)
|
frontmatter, body = self.parse_frontmatter(content)
|
||||||
|
|
||||||
frontmatter = self._adjust_script_paths(frontmatter)
|
frontmatter = self._adjust_script_paths(frontmatter)
|
||||||
@@ -318,7 +318,7 @@ class CommandRegistrar:
|
|||||||
raise ValueError(f"Unsupported format: {agent_config['format']}")
|
raise ValueError(f"Unsupported format: {agent_config['format']}")
|
||||||
|
|
||||||
dest_file = commands_dir / f"{cmd_name}{agent_config['extension']}"
|
dest_file = commands_dir / f"{cmd_name}{agent_config['extension']}"
|
||||||
dest_file.write_text(output)
|
dest_file.write_text(output, encoding="utf-8")
|
||||||
|
|
||||||
if agent_name == "copilot":
|
if agent_name == "copilot":
|
||||||
self.write_copilot_prompt(project_root, cmd_name)
|
self.write_copilot_prompt(project_root, cmd_name)
|
||||||
@@ -327,7 +327,7 @@ class CommandRegistrar:
|
|||||||
|
|
||||||
for alias in cmd_info.get("aliases", []):
|
for alias in cmd_info.get("aliases", []):
|
||||||
alias_file = commands_dir / f"{alias}{agent_config['extension']}"
|
alias_file = commands_dir / f"{alias}{agent_config['extension']}"
|
||||||
alias_file.write_text(output)
|
alias_file.write_text(output, encoding="utf-8")
|
||||||
if agent_name == "copilot":
|
if agent_name == "copilot":
|
||||||
self.write_copilot_prompt(project_root, alias)
|
self.write_copilot_prompt(project_root, alias)
|
||||||
registered.append(alias)
|
registered.append(alias)
|
||||||
@@ -345,7 +345,7 @@ class CommandRegistrar:
|
|||||||
prompts_dir = project_root / ".github" / "prompts"
|
prompts_dir = project_root / ".github" / "prompts"
|
||||||
prompts_dir.mkdir(parents=True, exist_ok=True)
|
prompts_dir.mkdir(parents=True, exist_ok=True)
|
||||||
prompt_file = prompts_dir / f"{cmd_name}.prompt.md"
|
prompt_file = prompts_dir / f"{cmd_name}.prompt.md"
|
||||||
prompt_file.write_text(f"---\nagent: {cmd_name}\n---\n")
|
prompt_file.write_text(f"---\nagent: {cmd_name}\n---\n", encoding="utf-8")
|
||||||
|
|
||||||
def register_commands_for_all_agents(
|
def register_commands_for_all_agents(
|
||||||
self,
|
self,
|
||||||
|
|||||||
@@ -565,9 +565,8 @@ class PresetManager:
|
|||||||
|
|
||||||
from . import SKILL_DESCRIPTIONS
|
from . import SKILL_DESCRIPTIONS
|
||||||
|
|
||||||
# Locate core command templates
|
# Locate core command templates from the project's installed templates
|
||||||
script_dir = Path(__file__).parent.parent.parent # up from src/specify_cli/
|
core_templates_dir = self.project_root / ".specify" / "templates" / "commands"
|
||||||
core_templates_dir = script_dir / "templates" / "commands"
|
|
||||||
|
|
||||||
for skill_name in skill_names:
|
for skill_name in skill_names:
|
||||||
# Derive command name from skill name (speckit-specify -> specify)
|
# Derive command name from skill name (speckit-specify -> specify)
|
||||||
@@ -1458,20 +1457,29 @@ class PresetResolver:
|
|||||||
if str(self.presets_dir) in resolved_str and self.presets_dir.exists():
|
if str(self.presets_dir) in resolved_str and self.presets_dir.exists():
|
||||||
registry = PresetRegistry(self.presets_dir)
|
registry = PresetRegistry(self.presets_dir)
|
||||||
for pack_id, _metadata in registry.list_by_priority():
|
for pack_id, _metadata in registry.list_by_priority():
|
||||||
if f"/{pack_id}/" in resolved_str:
|
pack_dir = self.presets_dir / pack_id
|
||||||
|
try:
|
||||||
|
resolved.relative_to(pack_dir)
|
||||||
meta = registry.get(pack_id)
|
meta = registry.get(pack_id)
|
||||||
version = meta.get("version", "?") if meta else "?"
|
version = meta.get("version", "?") if meta else "?"
|
||||||
return {
|
return {
|
||||||
"path": resolved_str,
|
"path": resolved_str,
|
||||||
"source": f"{pack_id} v{version}",
|
"source": f"{pack_id} v{version}",
|
||||||
}
|
}
|
||||||
|
except ValueError:
|
||||||
|
continue
|
||||||
|
|
||||||
if str(self.extensions_dir) in resolved_str and self.extensions_dir.exists():
|
if self.extensions_dir.exists():
|
||||||
for ext_dir in sorted(self.extensions_dir.iterdir()):
|
for ext_dir in sorted(self.extensions_dir.iterdir()):
|
||||||
if ext_dir.is_dir() and f"/{ext_dir.name}/" in resolved_str:
|
if not ext_dir.is_dir() or ext_dir.name.startswith("."):
|
||||||
|
continue
|
||||||
|
try:
|
||||||
|
resolved.relative_to(ext_dir)
|
||||||
return {
|
return {
|
||||||
"path": resolved_str,
|
"path": resolved_str,
|
||||||
"source": f"extension:{ext_dir.name}",
|
"source": f"extension:{ext_dir.name}",
|
||||||
}
|
}
|
||||||
|
except ValueError:
|
||||||
|
continue
|
||||||
|
|
||||||
return {"path": resolved_str, "source": "core"}
|
return {"path": resolved_str, "source": "core"}
|
||||||
|
|||||||
@@ -499,15 +499,15 @@ class TestPresetManager:
|
|||||||
manager = PresetManager(project_dir)
|
manager = PresetManager(project_dir)
|
||||||
assert manager.get_pack("nonexistent") is None
|
assert manager.get_pack("nonexistent") is None
|
||||||
|
|
||||||
def test_check_compatibility_valid(self, pack_dir):
|
def test_check_compatibility_valid(self, pack_dir, temp_dir):
|
||||||
"""Test compatibility check with valid version."""
|
"""Test compatibility check with valid version."""
|
||||||
manager = PresetManager(Path(tempfile.mkdtemp()))
|
manager = PresetManager(temp_dir)
|
||||||
manifest = PresetManifest(pack_dir / "preset.yml")
|
manifest = PresetManifest(pack_dir / "preset.yml")
|
||||||
assert manager.check_compatibility(manifest, "0.1.5") is True
|
assert manager.check_compatibility(manifest, "0.1.5") is True
|
||||||
|
|
||||||
def test_check_compatibility_invalid(self, pack_dir):
|
def test_check_compatibility_invalid(self, pack_dir, temp_dir):
|
||||||
"""Test compatibility check with invalid specifier."""
|
"""Test compatibility check with invalid specifier."""
|
||||||
manager = PresetManager(Path(tempfile.mkdtemp()))
|
manager = PresetManager(temp_dir)
|
||||||
manifest = PresetManifest(pack_dir / "preset.yml")
|
manifest = PresetManifest(pack_dir / "preset.yml")
|
||||||
manifest.data["requires"]["speckit_version"] = "not-a-specifier"
|
manifest.data["requires"]["speckit_version"] = "not-a-specifier"
|
||||||
with pytest.raises(PresetCompatibilityError, match="Invalid version specifier"):
|
with pytest.raises(PresetCompatibilityError, match="Invalid version specifier"):
|
||||||
@@ -1678,6 +1678,11 @@ class TestPresetSkills:
|
|||||||
|
|
||||||
(project_dir / ".claude" / "commands").mkdir(parents=True, exist_ok=True)
|
(project_dir / ".claude" / "commands").mkdir(parents=True, exist_ok=True)
|
||||||
|
|
||||||
|
# Set up core command template in the project so restoration works
|
||||||
|
core_cmds = project_dir / ".specify" / "templates" / "commands"
|
||||||
|
core_cmds.mkdir(parents=True, exist_ok=True)
|
||||||
|
(core_cmds / "specify.md").write_text("---\ndescription: Core specify command\n---\n\nCore specify body\n")
|
||||||
|
|
||||||
manager = PresetManager(project_dir)
|
manager = PresetManager(project_dir)
|
||||||
SELF_TEST_DIR = Path(__file__).parent.parent / "presets" / "self-test"
|
SELF_TEST_DIR = Path(__file__).parent.parent / "presets" / "self-test"
|
||||||
manager.install_from_directory(SELF_TEST_DIR, "0.1.5")
|
manager.install_from_directory(SELF_TEST_DIR, "0.1.5")
|
||||||
|
|||||||
Reference in New Issue
Block a user