From da6e7d22837f829fd445497a28fcf2d95f59f118 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 10 Mar 2026 16:35:17 -0500 Subject: [PATCH] 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 --- presets/catalog.json | 3 ++- scripts/bash/common.sh | 2 ++ src/specify_cli/__init__.py | 24 ++++++++++++++---------- src/specify_cli/agents.py | 8 ++++---- src/specify_cli/presets.py | 20 ++++++++++++++------ tests/test_presets.py | 13 +++++++++---- 6 files changed, 45 insertions(+), 25 deletions(-) diff --git a/presets/catalog.json b/presets/catalog.json index 93781f22..db5ae643 100644 --- a/presets/catalog.json +++ b/presets/catalog.json @@ -5,11 +5,12 @@ "presets": { "self-test": { "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", "version": "1.0.0", "repository": "https://github.com/github/spec-kit", "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": { "speckit_version": ">=0.1.0" }, diff --git a/scripts/bash/common.sh b/scripts/bash/common.sh index 00e00f77..735b8bd3 100644 --- a/scripts/bash/common.sh +++ b/scripts/bash/common.sh @@ -213,6 +213,8 @@ except Exception: if [ -d "$ext_dir" ]; then for ext in "$ext_dir"/*/; do [ -d "$ext" ] || continue + # Skip hidden directories (e.g. .backup, .cache) + case "$(basename "$ext")" in .*) continue;; esac local candidate="$ext/templates/${template_name}.md" [ -f "$candidate" ] && echo "$candidate" && return 0 done diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index 41220226..07a06b9e 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -1600,17 +1600,21 @@ def init( preset_manager.install_from_directory(local_path, speckit_ver) else: preset_catalog = PresetCatalog(project_path) - 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: + pack_info = preset_catalog.get_pack_info(preset) + if not pack_info: 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: console.print(f"[yellow]Warning:[/yellow] Failed to install preset: {preset_err}") diff --git a/src/specify_cli/agents.py b/src/specify_cli/agents.py index 5184f0f6..4c90a154 100644 --- a/src/specify_cli/agents.py +++ b/src/specify_cli/agents.py @@ -301,7 +301,7 @@ class CommandRegistrar: if not source_file.exists(): continue - content = source_file.read_text() + content = source_file.read_text(encoding="utf-8") frontmatter, body = self.parse_frontmatter(content) frontmatter = self._adjust_script_paths(frontmatter) @@ -318,7 +318,7 @@ class CommandRegistrar: raise ValueError(f"Unsupported format: {agent_config['format']}") 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": self.write_copilot_prompt(project_root, cmd_name) @@ -327,7 +327,7 @@ class CommandRegistrar: for alias in cmd_info.get("aliases", []): 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": self.write_copilot_prompt(project_root, alias) registered.append(alias) @@ -345,7 +345,7 @@ class CommandRegistrar: prompts_dir = project_root / ".github" / "prompts" prompts_dir.mkdir(parents=True, exist_ok=True) 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( self, diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 474df563..818f9951 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -565,9 +565,8 @@ class PresetManager: from . import SKILL_DESCRIPTIONS - # Locate core command templates - script_dir = Path(__file__).parent.parent.parent # up from src/specify_cli/ - core_templates_dir = script_dir / "templates" / "commands" + # Locate core command templates from the project's installed templates + core_templates_dir = self.project_root / ".specify" / "templates" / "commands" for skill_name in skill_names: # 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(): registry = PresetRegistry(self.presets_dir) 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) version = meta.get("version", "?") if meta else "?" return { "path": resolved_str, "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()): - 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 { "path": resolved_str, "source": f"extension:{ext_dir.name}", } + except ValueError: + continue return {"path": resolved_str, "source": "core"} diff --git a/tests/test_presets.py b/tests/test_presets.py index 57b9c5d0..6ce8912d 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -499,15 +499,15 @@ class TestPresetManager: manager = PresetManager(project_dir) 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.""" - manager = PresetManager(Path(tempfile.mkdtemp())) + manager = PresetManager(temp_dir) manifest = PresetManifest(pack_dir / "preset.yml") 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.""" - manager = PresetManager(Path(tempfile.mkdtemp())) + manager = PresetManager(temp_dir) manifest = PresetManifest(pack_dir / "preset.yml") manifest.data["requires"]["speckit_version"] = "not-a-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) + # 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) SELF_TEST_DIR = Path(__file__).parent.parent / "presets" / "self-test" manager.install_from_directory(SELF_TEST_DIR, "0.1.5")