From 7546a7a15f63f628803c2b7690ee16af83250cd5 Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Sat, 14 Mar 2026 17:54:31 +0100 Subject: [PATCH 01/12] feat(extensions,presets): add priority-based resolution ordering Add priority field to extension and preset registries for deterministic template resolution when multiple sources provide the same template. Extensions: - Add `list_by_priority()` method to ExtensionRegistry - Add `--priority` option to `extension add` command - Add `extension set-priority` command - Show priority in `extension list` and `extension info` - Preserve priority during `extension update` - Update RFC documentation Presets: - Add `preset set-priority` command - Show priority in `preset info` output - Use priority ordering in PresetResolver for extensions Both systems: - Lower priority number = higher precedence (default: 10) - Backwards compatible with legacy entries (missing priority defaults to 10) - Comprehensive test coverage including backwards compatibility Closes #1845 Closes #1854 Co-Authored-By: Claude Opus 4.5 --- extensions/RFC-EXTENSION-SYSTEM.md | 33 ++- src/specify_cli/__init__.py | 115 +++++++++- src/specify_cli/extensions.py | 27 ++- src/specify_cli/presets.py | 35 ++- tests/test_extensions.py | 327 +++++++++++++++++++++++++++++ tests/test_presets.py | 178 +++++++++++++++- 6 files changed, 697 insertions(+), 18 deletions(-) diff --git a/extensions/RFC-EXTENSION-SYSTEM.md b/extensions/RFC-EXTENSION-SYSTEM.md index a0f6034e5c..d7d89511d8 100644 --- a/extensions/RFC-EXTENSION-SYSTEM.md +++ b/extensions/RFC-EXTENSION-SYSTEM.md @@ -359,12 +359,15 @@ specify extension add jira "installed_at": "2026-01-28T14:30:00Z", "source": "catalog", "manifest_hash": "sha256:abc123...", - "enabled": true + "enabled": true, + "priority": 10 } } } ``` +**Priority Field**: Extensions are ordered by `priority` (lower = higher precedence). Default is 10. Used for template resolution when multiple extensions provide the same template. + ### 3. Configuration ```bash @@ -1085,10 +1088,10 @@ $ specify extension list Installed Extensions: ✓ jira (v1.0.0) - Jira Integration - Commands: 3 | Hooks: 2 | Status: Enabled + Commands: 3 | Hooks: 2 | Priority: 10 | Status: Enabled ✓ linear (v0.9.0) - Linear Integration - Commands: 1 | Hooks: 1 | Status: Enabled + Commands: 1 | Hooks: 1 | Priority: 10 | Status: Enabled ``` **Options:** @@ -1200,6 +1203,7 @@ Next steps: - `--version VERSION`: Install specific version - `--dev PATH`: Install from local path (development mode) - `--no-register`: Skip command registration (manual setup) +- `--priority NUMBER`: Set resolution priority (lower = higher precedence, default 10) #### `specify extension remove NAME` @@ -1280,6 +1284,29 @@ $ specify extension disable jira To re-enable: specify extension enable jira ``` +#### `specify extension set-priority NAME PRIORITY` + +Change the resolution priority of an installed extension. + +```bash +$ specify extension set-priority jira 5 + +✓ Extension 'Jira Integration' priority changed: 10 → 5 + +Lower priority = higher precedence in template resolution +``` + +**Priority Values:** + +- Lower numbers = higher precedence (checked first in resolution) +- Default priority is 10 +- Must be a positive integer (1 or higher) + +**Use Cases:** + +- Ensure a critical extension's templates take precedence +- Override default resolution order when multiple extensions provide similar templates + --- ## Compatibility & Versioning diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index 8509db7efe..5718e9a3ad 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -2210,6 +2210,10 @@ def preset_info( if license_val: console.print(f" License: {license_val}") console.print("\n [green]Status: installed[/green]") + # Get priority from registry + pack_metadata = manager.registry.get(pack_id) + priority = pack_metadata.get("priority", 10) if pack_metadata else 10 + console.print(f" [dim]Priority:[/dim] {priority}") console.print() return @@ -2241,6 +2245,53 @@ def preset_info( console.print() +@preset_app.command("set-priority") +def preset_set_priority( + pack_id: str = typer.Argument(help="Preset ID"), + priority: int = typer.Argument(help="New priority (lower = higher precedence)"), +): + """Set the resolution priority of an installed preset.""" + from .presets import PresetManager + + project_root = Path.cwd() + + # Check if we're in a spec-kit project + specify_dir = project_root / ".specify" + if not specify_dir.exists(): + console.print("[red]Error:[/red] Not a spec-kit project (no .specify/ directory)") + console.print("Run this command from a spec-kit project root") + raise typer.Exit(1) + + # Validate priority + if priority < 1: + console.print("[red]Error:[/red] Priority must be a positive integer (1 or higher)") + raise typer.Exit(1) + + manager = PresetManager(project_root) + + # Check if preset is installed + if not manager.registry.is_installed(pack_id): + console.print(f"[red]Error:[/red] Preset '{pack_id}' is not installed") + raise typer.Exit(1) + + # Get current metadata + metadata = manager.registry.get(pack_id) + if metadata is None: + console.print(f"[red]Error:[/red] Preset '{pack_id}' not found in registry (corrupted state)") + raise typer.Exit(1) + + old_priority = metadata.get("priority", 10) + if old_priority == priority: + console.print(f"[yellow]Preset '{pack_id}' already has priority {priority}[/yellow]") + raise typer.Exit(0) + + # Update priority + manager.registry.update(pack_id, {"priority": priority}) + + console.print(f"[green]✓[/green] Preset '{pack_id}' priority changed: {old_priority} → {priority}") + console.print("\n[dim]Lower priority = higher precedence in template resolution[/dim]") + + # ===== Preset Catalog Commands ===== @@ -2578,7 +2629,7 @@ def extension_list( console.print(f" [{status_color}]{status_icon}[/{status_color}] [bold]{ext['name']}[/bold] (v{ext['version']})") console.print(f" [dim]{ext['id']}[/dim]") console.print(f" {ext['description']}") - console.print(f" Commands: {ext['command_count']} | Hooks: {ext['hook_count']} | Status: {'Enabled' if ext['enabled'] else 'Disabled'}") + console.print(f" Commands: {ext['command_count']} | Hooks: {ext['hook_count']} | Priority: {ext['priority']} | Status: {'Enabled' if ext['enabled'] else 'Disabled'}") console.print() if available or all_extensions: @@ -2766,6 +2817,7 @@ def extension_add( extension: str = typer.Argument(help="Extension name or path"), dev: bool = typer.Option(False, "--dev", help="Install from local directory"), from_url: Optional[str] = typer.Option(None, "--from", help="Install from custom URL"), + priority: int = typer.Option(10, "--priority", help="Resolution priority (lower = higher precedence, default 10)"), ): """Install an extension.""" from .extensions import ExtensionManager, ExtensionCatalog, ExtensionError, ValidationError, CompatibilityError @@ -2795,7 +2847,7 @@ def extension_add( console.print(f"[red]Error:[/red] No extension.yml found in {source_path}") raise typer.Exit(1) - manifest = manager.install_from_directory(source_path, speckit_version) + manifest = manager.install_from_directory(source_path, speckit_version, priority=priority) elif from_url: # Install from URL (ZIP file) @@ -2828,7 +2880,7 @@ def extension_add( zip_path.write_bytes(zip_data) # Install from downloaded ZIP - manifest = manager.install_from_zip(zip_path, speckit_version) + manifest = manager.install_from_zip(zip_path, speckit_version, priority=priority) except urllib.error.URLError as e: console.print(f"[red]Error:[/red] Failed to download from {from_url}: {e}") raise typer.Exit(1) @@ -2872,7 +2924,7 @@ def extension_add( try: # Install from downloaded ZIP - manifest = manager.install_from_zip(zip_path, speckit_version) + manifest = manager.install_from_zip(zip_path, speckit_version, priority=priority) finally: # Clean up downloaded ZIP if zip_path.exists(): @@ -3114,6 +3166,8 @@ def extension_info( console.print() console.print("[green]✓ Installed[/green]") + priority = metadata.get("priority", 10) + console.print(f"[dim]Priority:[/dim] {priority}") console.print(f"\nTo remove: specify extension remove {resolved_installed_id}") return @@ -3207,6 +3261,9 @@ def _print_extension_info(ext_info: dict, manager): install_allowed = ext_info.get("_install_allowed", True) if is_installed: console.print("[green]✓ Installed[/green]") + metadata = manager.registry.get(ext_info['id']) + priority = metadata.get("priority", 10) if metadata else 10 + console.print(f"[dim]Priority:[/dim] {priority}") console.print(f"\nTo remove: specify extension remove {ext_info['id']}") elif install_allowed: console.print("[yellow]Not installed[/yellow]") @@ -3471,6 +3528,10 @@ def extension_update( if "installed_at" in backup_registry_entry: new_metadata["installed_at"] = backup_registry_entry["installed_at"] + # Preserve the original priority + if "priority" in backup_registry_entry: + new_metadata["priority"] = backup_registry_entry["priority"] + # If extension was disabled before update, disable it again if not backup_registry_entry.get("enabled", True): new_metadata["enabled"] = False @@ -3717,6 +3778,52 @@ def extension_disable( console.print(f"To re-enable: specify extension enable {extension_id}") +@extension_app.command("set-priority") +def extension_set_priority( + extension: str = typer.Argument(help="Extension ID or name"), + priority: int = typer.Argument(help="New priority (lower = higher precedence)"), +): + """Set the resolution priority of an installed extension.""" + from .extensions import ExtensionManager + + project_root = Path.cwd() + + # Check if we're in a spec-kit project + specify_dir = project_root / ".specify" + if not specify_dir.exists(): + console.print("[red]Error:[/red] Not a spec-kit project (no .specify/ directory)") + console.print("Run this command from a spec-kit project root") + raise typer.Exit(1) + + # Validate priority + if priority < 1: + console.print("[red]Error:[/red] Priority must be a positive integer (1 or higher)") + raise typer.Exit(1) + + manager = ExtensionManager(project_root) + + # Resolve extension ID from argument (handles ambiguous names) + installed = manager.list_installed() + extension_id, display_name = _resolve_installed_extension(extension, installed, "set-priority") + + # Get current metadata + metadata = manager.registry.get(extension_id) + if metadata is None: + console.print(f"[red]Error:[/red] Extension '{extension_id}' not found in registry (corrupted state)") + raise typer.Exit(1) + + old_priority = metadata.get("priority", 10) + if old_priority == priority: + console.print(f"[yellow]Extension '{display_name}' already has priority {priority}[/yellow]") + raise typer.Exit(0) + + # Update priority + manager.registry.update(extension_id, {"priority": priority}) + + console.print(f"[green]✓[/green] Extension '{display_name}' priority changed: {old_priority} → {priority}") + console.print("\n[dim]Lower priority = higher precedence in template resolution[/dim]") + + def main(): app() diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index 0dfd40b7cd..a3df56f520 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -324,6 +324,20 @@ def is_installed(self, extension_id: str) -> bool: """ return extension_id in self.data["extensions"] + def list_by_priority(self) -> List[tuple]: + """Get all installed extensions sorted by priority. + + Lower priority number = higher precedence (checked first). + + Returns: + List of (extension_id, metadata) tuples sorted by priority + """ + extensions = self.data["extensions"] + return sorted( + extensions.items(), + key=lambda item: item[1].get("priority", 10), + ) + class ExtensionManager: """Manages extension lifecycle: installation, removal, updates.""" @@ -440,7 +454,8 @@ def install_from_directory( self, source_dir: Path, speckit_version: str, - register_commands: bool = True + register_commands: bool = True, + priority: int = 10, ) -> ExtensionManifest: """Install extension from a local directory. @@ -448,6 +463,7 @@ def install_from_directory( source_dir: Path to extension directory speckit_version: Current spec-kit version register_commands: If True, register commands with AI agents + priority: Resolution priority (lower = higher precedence, default 10) Returns: Installed extension manifest @@ -497,6 +513,7 @@ def install_from_directory( "source": "local", "manifest_hash": manifest.get_hash(), "enabled": True, + "priority": priority, "registered_commands": registered_commands }) @@ -505,13 +522,15 @@ def install_from_directory( def install_from_zip( self, zip_path: Path, - speckit_version: str + speckit_version: str, + priority: int = 10, ) -> ExtensionManifest: """Install extension from ZIP file. Args: zip_path: Path to extension ZIP file speckit_version: Current spec-kit version + priority: Resolution priority (lower = higher precedence, default 10) Returns: Installed extension manifest @@ -554,7 +573,7 @@ def install_from_zip( raise ValidationError("No extension.yml found in ZIP file") # Install from extracted directory - return self.install_from_directory(extension_dir, speckit_version) + return self.install_from_directory(extension_dir, speckit_version, priority=priority) def remove(self, extension_id: str, keep_config: bool = False) -> bool: """Remove an installed extension. @@ -643,6 +662,7 @@ def list_installed(self) -> List[Dict[str, Any]]: "version": metadata.get("version", "unknown"), "description": manifest.description, "enabled": metadata.get("enabled", True), + "priority": metadata.get("priority", 10), "installed_at": metadata.get("installed_at"), "command_count": len(manifest.commands), "hook_count": len(manifest.hooks) @@ -655,6 +675,7 @@ def list_installed(self) -> List[Dict[str, Any]]: "version": metadata.get("version", "unknown"), "description": "⚠️ Corrupted extension", "enabled": False, + "priority": metadata.get("priority", 10), "installed_at": metadata.get("installed_at"), "command_count": 0, "hook_count": 0 diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 1519633791..36904c1fbe 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -23,6 +23,8 @@ from packaging import version as pkg_version from packaging.specifiers import SpecifierSet, InvalidSpecifier +from .extensions import ExtensionRegistry + @dataclass class PresetCatalogEntry: @@ -271,6 +273,21 @@ def remove(self, pack_id: str): del self.data["presets"][pack_id] self._save() + def update(self, pack_id: str, updates: dict): + """Update preset metadata in registry. + + Args: + pack_id: Preset ID + updates: Partial metadata to merge into existing metadata + + Raises: + KeyError: If preset is not installed + """ + if pack_id not in self.data["presets"]: + raise KeyError(f"Preset '{pack_id}' not found in registry") + self.data["presets"][pack_id].update(updates) + self._save() + def get(self, pack_id: str) -> Optional[dict]: """Get preset metadata from registry. @@ -729,6 +746,7 @@ def install_from_zip( Args: zip_path: Path to preset ZIP file speckit_version: Current spec-kit version + priority: Resolution priority (lower = higher precedence, default 10) Returns: Installed preset manifest @@ -1445,10 +1463,12 @@ def resolve( if candidate.exists(): return candidate - # Priority 3: Extension-provided templates + # Priority 3: Extension-provided templates (sorted by priority — lower number wins) if self.extensions_dir.exists(): - for ext_dir in sorted(self.extensions_dir.iterdir()): - if not ext_dir.is_dir() or ext_dir.name.startswith("."): + registry = ExtensionRegistry(self.extensions_dir) + for ext_id, _metadata in registry.list_by_priority(): + ext_dir = self.extensions_dir / ext_id + if not ext_dir.is_dir(): continue for subdir in subdirs: if subdir: @@ -1515,14 +1535,17 @@ def resolve_with_source( continue if self.extensions_dir.exists(): - for ext_dir in sorted(self.extensions_dir.iterdir()): - if not ext_dir.is_dir() or ext_dir.name.startswith("."): + ext_registry = ExtensionRegistry(self.extensions_dir) + for ext_id, ext_meta in ext_registry.list_by_priority(): + ext_dir = self.extensions_dir / ext_id + if not ext_dir.is_dir(): continue try: resolved.relative_to(ext_dir) + version = ext_meta.get("version", "?") if ext_meta else "?" return { "path": resolved_str, - "source": f"extension:{ext_dir.name}", + "source": f"extension:{ext_id} v{version}", } except ValueError: continue diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 61a3e1c987..ac9d4d611d 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -2363,3 +2363,330 @@ def test_list_shows_extension_id(self, extension_dir, project_dir): # Verify name and version are also shown assert "Test Extension" in result.output assert "1.0.0" in result.output + + +class TestExtensionPriority: + """Test extension priority-based resolution.""" + + def test_list_by_priority_empty(self, temp_dir): + """Test list_by_priority on empty registry.""" + extensions_dir = temp_dir / "extensions" + extensions_dir.mkdir() + + registry = ExtensionRegistry(extensions_dir) + result = registry.list_by_priority() + + assert result == [] + + def test_list_by_priority_single(self, temp_dir): + """Test list_by_priority with single extension.""" + extensions_dir = temp_dir / "extensions" + extensions_dir.mkdir() + + registry = ExtensionRegistry(extensions_dir) + registry.add("test-ext", {"version": "1.0.0", "priority": 5}) + + result = registry.list_by_priority() + + assert len(result) == 1 + assert result[0][0] == "test-ext" + assert result[0][1]["priority"] == 5 + + def test_list_by_priority_ordering(self, temp_dir): + """Test list_by_priority returns extensions sorted by priority.""" + extensions_dir = temp_dir / "extensions" + extensions_dir.mkdir() + + registry = ExtensionRegistry(extensions_dir) + # Add in non-priority order + registry.add("ext-low", {"version": "1.0.0", "priority": 20}) + registry.add("ext-high", {"version": "1.0.0", "priority": 1}) + registry.add("ext-mid", {"version": "1.0.0", "priority": 10}) + + result = registry.list_by_priority() + + assert len(result) == 3 + # Lower priority number = higher precedence (first) + assert result[0][0] == "ext-high" + assert result[1][0] == "ext-mid" + assert result[2][0] == "ext-low" + + def test_list_by_priority_default(self, temp_dir): + """Test list_by_priority uses default priority of 10.""" + extensions_dir = temp_dir / "extensions" + extensions_dir.mkdir() + + registry = ExtensionRegistry(extensions_dir) + # Add without explicit priority + registry.add("ext-default", {"version": "1.0.0"}) + registry.add("ext-high", {"version": "1.0.0", "priority": 1}) + registry.add("ext-low", {"version": "1.0.0", "priority": 20}) + + result = registry.list_by_priority() + + assert len(result) == 3 + # ext-high (1), ext-default (10), ext-low (20) + assert result[0][0] == "ext-high" + assert result[1][0] == "ext-default" + assert result[2][0] == "ext-low" + + def test_install_with_priority(self, extension_dir, project_dir): + """Test that install_from_directory stores priority.""" + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False, priority=5) + + metadata = manager.registry.get("test-ext") + assert metadata["priority"] == 5 + + def test_install_default_priority(self, extension_dir, project_dir): + """Test that install_from_directory uses default priority of 10.""" + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + metadata = manager.registry.get("test-ext") + assert metadata["priority"] == 10 + + def test_list_installed_includes_priority(self, extension_dir, project_dir): + """Test that list_installed includes priority in returned data.""" + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False, priority=3) + + installed = manager.list_installed() + + assert len(installed) == 1 + assert installed[0]["priority"] == 3 + + def test_priority_preserved_on_update(self, temp_dir): + """Test that registry update preserves priority.""" + extensions_dir = temp_dir / "extensions" + extensions_dir.mkdir() + + registry = ExtensionRegistry(extensions_dir) + registry.add("test-ext", {"version": "1.0.0", "priority": 5, "enabled": True}) + + # Update with new metadata (no priority specified) + registry.update("test-ext", {"enabled": False}) + + updated = registry.get("test-ext") + assert updated["priority"] == 5 # Preserved + assert updated["enabled"] is False # Updated + + +class TestExtensionPriorityCLI: + """Test extension priority CLI integration.""" + + def test_add_with_priority_option(self, extension_dir, project_dir): + """Test extension add command with --priority option.""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + runner = CliRunner() + + with patch.object(Path, "cwd", return_value=project_dir): + result = runner.invoke(app, [ + "extension", "add", str(extension_dir), "--dev", "--priority", "3" + ]) + + assert result.exit_code == 0, result.output + + manager = ExtensionManager(project_dir) + metadata = manager.registry.get("test-ext") + assert metadata["priority"] == 3 + + def test_list_shows_priority(self, extension_dir, project_dir): + """Test extension list shows priority.""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + runner = CliRunner() + + # Install extension with priority + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False, priority=7) + + with patch.object(Path, "cwd", return_value=project_dir): + result = runner.invoke(app, ["extension", "list"]) + + assert result.exit_code == 0, result.output + assert "Priority: 7" in result.output + + def test_set_priority_changes_priority(self, extension_dir, project_dir): + """Test set-priority command changes extension priority.""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + runner = CliRunner() + + # Install extension with default priority + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + # Verify default priority + assert manager.registry.get("test-ext")["priority"] == 10 + + with patch.object(Path, "cwd", return_value=project_dir): + result = runner.invoke(app, ["extension", "set-priority", "test-ext", "5"]) + + assert result.exit_code == 0, result.output + assert "priority changed: 10 → 5" in result.output + + # Reload registry to see updated value + manager2 = ExtensionManager(project_dir) + assert manager2.registry.get("test-ext")["priority"] == 5 + + def test_set_priority_same_value_no_change(self, extension_dir, project_dir): + """Test set-priority with same value shows already set message.""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + runner = CliRunner() + + # Install extension with priority 5 + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False, priority=5) + + with patch.object(Path, "cwd", return_value=project_dir): + result = runner.invoke(app, ["extension", "set-priority", "test-ext", "5"]) + + assert result.exit_code == 0, result.output + assert "already has priority 5" in result.output + + def test_set_priority_invalid_value(self, extension_dir, project_dir): + """Test set-priority rejects invalid priority values.""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + runner = CliRunner() + + # Install extension + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + with patch.object(Path, "cwd", return_value=project_dir): + result = runner.invoke(app, ["extension", "set-priority", "test-ext", "0"]) + + assert result.exit_code == 1, result.output + assert "Priority must be a positive integer" in result.output + + def test_set_priority_not_installed(self, project_dir): + """Test set-priority fails for non-installed extension.""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + runner = CliRunner() + + # Ensure .specify exists + (project_dir / ".specify").mkdir(parents=True, exist_ok=True) + + with patch.object(Path, "cwd", return_value=project_dir): + result = runner.invoke(app, ["extension", "set-priority", "nonexistent", "5"]) + + assert result.exit_code == 1, result.output + assert "not installed" in result.output.lower() or "no extensions installed" in result.output.lower() + + def test_set_priority_by_display_name(self, extension_dir, project_dir): + """Test set-priority works with extension display name.""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + runner = CliRunner() + + # Install extension + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + # Use display name "Test Extension" instead of ID "test-ext" + with patch.object(Path, "cwd", return_value=project_dir): + result = runner.invoke(app, ["extension", "set-priority", "Test Extension", "3"]) + + assert result.exit_code == 0, result.output + assert "priority changed" in result.output + + # Reload registry to see updated value + manager2 = ExtensionManager(project_dir) + assert manager2.registry.get("test-ext")["priority"] == 3 + + +class TestExtensionPriorityBackwardsCompatibility: + """Test backwards compatibility for extensions installed before priority feature.""" + + def test_legacy_extension_without_priority_field(self, temp_dir): + """Extensions installed before priority feature should default to 10.""" + extensions_dir = temp_dir / "extensions" + extensions_dir.mkdir() + + # Simulate legacy registry entry without priority field + registry = ExtensionRegistry(extensions_dir) + registry.data["extensions"]["legacy-ext"] = { + "version": "1.0.0", + "source": "local", + "enabled": True, + "installed_at": "2025-01-01T00:00:00Z", + # No "priority" field - simulates pre-feature extension + } + registry._save() + + # Reload registry + registry2 = ExtensionRegistry(extensions_dir) + + # list_by_priority should use default of 10 + result = registry2.list_by_priority() + assert len(result) == 1 + assert result[0][0] == "legacy-ext" + # Priority defaults to 10 in sorting + + def test_legacy_extension_in_list_installed(self, extension_dir, project_dir): + """list_installed returns priority=10 for legacy extensions without priority field.""" + manager = ExtensionManager(project_dir) + + # Install extension normally + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + # Manually remove priority to simulate legacy extension + ext_data = manager.registry.data["extensions"]["test-ext"] + del ext_data["priority"] + manager.registry._save() + + # list_installed should still return priority=10 + installed = manager.list_installed() + assert len(installed) == 1 + assert installed[0]["priority"] == 10 + + def test_mixed_legacy_and_new_extensions_ordering(self, temp_dir): + """Legacy extensions (no priority) sort with default=10 among prioritized extensions.""" + extensions_dir = temp_dir / "extensions" + extensions_dir.mkdir() + + registry = ExtensionRegistry(extensions_dir) + + # Add extension with explicit priority=5 + registry.add("ext-with-priority", {"version": "1.0.0", "priority": 5}) + + # Add legacy extension without priority (manually) + registry.data["extensions"]["legacy-ext"] = { + "version": "1.0.0", + "source": "local", + "enabled": True, + # No priority field + } + registry._save() + + # Add extension with priority=15 + registry.add("ext-low-priority", {"version": "1.0.0", "priority": 15}) + + # Reload and check ordering + registry2 = ExtensionRegistry(extensions_dir) + result = registry2.list_by_priority() + + assert len(result) == 3 + # Order: ext-with-priority (5), legacy-ext (defaults to 10), ext-low-priority (15) + assert result[0][0] == "ext-with-priority" + assert result[1][0] == "legacy-ext" + assert result[2][0] == "ext-low-priority" diff --git a/tests/test_presets.py b/tests/test_presets.py index 3ad70c6d14..ac68ee265d 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -32,6 +32,7 @@ PresetCompatibilityError, VALID_PRESET_TEMPLATE_TYPES, ) +from specify_cli.extensions import ExtensionRegistry # ===== Fixtures ===== @@ -678,6 +679,11 @@ def test_resolve_extension_provided_templates(self, project_dir): ext_template = ext_templates_dir / "custom-template.md" ext_template.write_text("# Extension Custom Template\n") + # Register extension in registry + extensions_dir = project_dir / ".specify" / "extensions" + ext_registry = ExtensionRegistry(extensions_dir) + ext_registry.add("my-ext", {"version": "1.0.0", "priority": 10}) + resolver = PresetResolver(project_dir) result = resolver.resolve("custom-template") assert result is not None @@ -741,10 +747,15 @@ def test_resolve_with_source_extension(self, project_dir): ext_template = ext_templates_dir / "unique-template.md" ext_template.write_text("# Unique\n") + # Register extension in registry + extensions_dir = project_dir / ".specify" / "extensions" + ext_registry = ExtensionRegistry(extensions_dir) + ext_registry.add("my-ext", {"version": "1.0.0", "priority": 10}) + resolver = PresetResolver(project_dir) result = resolver.resolve_with_source("unique-template") assert result is not None - assert result["source"] == "extension:my-ext" + assert "extension:my-ext" in result["source"] def test_resolve_with_source_not_found(self, project_dir): """Test resolve_with_source for nonexistent template.""" @@ -979,8 +990,13 @@ def test_override_beats_pack_beats_extension_beats_core(self, project_dir, pack_ ext_templates_dir.mkdir(parents=True) (ext_templates_dir / "spec-template.md").write_text("# Extension\n") + # Register extension in registry + extensions_dir = project_dir / ".specify" / "extensions" + ext_registry = ExtensionRegistry(extensions_dir) + ext_registry.add("my-ext", {"version": "1.0.0", "priority": 10}) + result = resolver.resolve_with_source("spec-template") - assert result["source"] == "extension:my-ext" + assert "extension:my-ext" in result["source"] # Install pack — should win over extension manager = PresetManager(project_dir) @@ -1710,3 +1726,161 @@ def test_no_skills_registered_when_no_skill_dir_exists(self, project_dir, temp_d metadata = manager.registry.get("self-test") assert metadata.get("registered_skills", []) == [] + + +class TestPresetSetPriority: + """Test preset set-priority CLI command.""" + + def test_set_priority_changes_priority(self, project_dir, pack_dir): + """Test set-priority command changes preset priority.""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + runner = CliRunner() + + # Install preset with default priority + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + + # Verify default priority + assert manager.registry.get("test-pack")["priority"] == 10 + + with patch.object(Path, "cwd", return_value=project_dir): + result = runner.invoke(app, ["preset", "set-priority", "test-pack", "5"]) + + assert result.exit_code == 0, result.output + assert "priority changed: 10 → 5" in result.output + + # Reload registry to see updated value + manager2 = PresetManager(project_dir) + assert manager2.registry.get("test-pack")["priority"] == 5 + + def test_set_priority_same_value_no_change(self, project_dir, pack_dir): + """Test set-priority with same value shows already set message.""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + runner = CliRunner() + + # Install preset with priority 5 + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5", priority=5) + + with patch.object(Path, "cwd", return_value=project_dir): + result = runner.invoke(app, ["preset", "set-priority", "test-pack", "5"]) + + assert result.exit_code == 0, result.output + assert "already has priority 5" in result.output + + def test_set_priority_invalid_value(self, project_dir, pack_dir): + """Test set-priority rejects invalid priority values.""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + runner = CliRunner() + + # Install preset + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + + with patch.object(Path, "cwd", return_value=project_dir): + result = runner.invoke(app, ["preset", "set-priority", "test-pack", "0"]) + + assert result.exit_code == 1, result.output + assert "Priority must be a positive integer" in result.output + + def test_set_priority_not_installed(self, project_dir): + """Test set-priority fails for non-installed preset.""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + runner = CliRunner() + + with patch.object(Path, "cwd", return_value=project_dir): + result = runner.invoke(app, ["preset", "set-priority", "nonexistent", "5"]) + + assert result.exit_code == 1, result.output + assert "not installed" in result.output.lower() + + +class TestPresetPriorityBackwardsCompatibility: + """Test backwards compatibility for presets installed before priority feature.""" + + def test_legacy_preset_without_priority_field(self, temp_dir): + """Presets installed before priority feature should default to 10.""" + presets_dir = temp_dir / ".specify" / "presets" + presets_dir.mkdir(parents=True) + + # Simulate legacy registry entry without priority field + registry = PresetRegistry(presets_dir) + registry.data["presets"]["legacy-pack"] = { + "version": "1.0.0", + "source": "local", + "enabled": True, + "installed_at": "2025-01-01T00:00:00Z", + # No "priority" field - simulates pre-feature preset + } + registry._save() + + # Reload registry + registry2 = PresetRegistry(presets_dir) + + # list_by_priority should use default of 10 + result = registry2.list_by_priority() + assert len(result) == 1 + assert result[0][0] == "legacy-pack" + # Priority defaults to 10 in sorting + + def test_legacy_preset_in_list_installed(self, project_dir, pack_dir): + """list_installed returns priority=10 for legacy presets without priority field.""" + manager = PresetManager(project_dir) + + # Install preset normally + manager.install_from_directory(pack_dir, "0.1.5") + + # Manually remove priority to simulate legacy preset + pack_data = manager.registry.data["presets"]["test-pack"] + del pack_data["priority"] + manager.registry._save() + + # list_installed should still return priority=10 + installed = manager.list_installed() + assert len(installed) == 1 + assert installed[0]["priority"] == 10 + + def test_mixed_legacy_and_new_presets_ordering(self, temp_dir): + """Legacy presets (no priority) sort with default=10 among prioritized presets.""" + presets_dir = temp_dir / ".specify" / "presets" + presets_dir.mkdir(parents=True) + + registry = PresetRegistry(presets_dir) + + # Add preset with explicit priority=5 + registry.add("pack-with-priority", {"version": "1.0.0", "priority": 5}) + + # Add legacy preset without priority (manually) + registry.data["presets"]["legacy-pack"] = { + "version": "1.0.0", + "source": "local", + "enabled": True, + # No priority field + } + + # Add another preset with priority=15 + registry.add("low-priority-pack", {"version": "1.0.0", "priority": 15}) + registry._save() + + # Reload and check ordering + registry2 = PresetRegistry(presets_dir) + sorted_presets = registry2.list_by_priority() + + # Should be: pack-with-priority (5), legacy-pack (default 10), low-priority-pack (15) + assert [p[0] for p in sorted_presets] == [ + "pack-with-priority", + "legacy-pack", + "low-priority-pack", + ] From 9f1218886a09f11c6523f07d79c9205357ab16dd Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Sat, 14 Mar 2026 18:04:15 +0100 Subject: [PATCH 02/12] fix: address code review feedback - list_by_priority(): add secondary sort by ID for deterministic ordering, return deep copies to prevent mutation - install_from_directory/zip: validate priority >= 1 early - extension add CLI: validate --priority >= 1 before install - PresetRegistry.update(): preserve installed_at timestamp - Test assertions: use exact source string instead of substring match Co-Authored-By: Claude Opus 4.5 --- src/specify_cli/__init__.py | 10 ++++++++++ src/specify_cli/extensions.py | 21 +++++++++++++++----- src/specify_cli/presets.py | 36 ++++++++++++++++++++++++++++------- tests/test_presets.py | 4 ++-- 4 files changed, 57 insertions(+), 14 deletions(-) diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index 5718e9a3ad..4ca67c67db 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -2000,6 +2000,11 @@ def preset_add( console.print("Run this command from a spec-kit project root") raise typer.Exit(1) + # Validate priority + if priority < 1: + console.print("[red]Error:[/red] Priority must be a positive integer (1 or higher)") + raise typer.Exit(1) + manager = PresetManager(project_root) speckit_version = get_speckit_version() @@ -2831,6 +2836,11 @@ def extension_add( console.print("Run this command from a spec-kit project root") raise typer.Exit(1) + # Validate priority + if priority < 1: + console.print("[red]Error:[/red] Priority must be a positive integer (1 or higher)") + raise typer.Exit(1) + manager = ExtensionManager(project_root) speckit_version = get_speckit_version() diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index a3df56f520..33d5112c38 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -328,14 +328,17 @@ def list_by_priority(self) -> List[tuple]: """Get all installed extensions sorted by priority. Lower priority number = higher precedence (checked first). + Extensions with equal priority are sorted alphabetically by ID + for deterministic ordering. Returns: - List of (extension_id, metadata) tuples sorted by priority + List of (extension_id, metadata_copy) tuples sorted by priority. + Metadata is deep-copied to prevent accidental mutation. """ extensions = self.data["extensions"] return sorted( - extensions.items(), - key=lambda item: item[1].get("priority", 10), + [(ext_id, copy.deepcopy(meta)) for ext_id, meta in extensions.items()], + key=lambda item: (item[1].get("priority", 10), item[0]), ) @@ -469,9 +472,13 @@ def install_from_directory( Installed extension manifest Raises: - ValidationError: If manifest is invalid + ValidationError: If manifest is invalid or priority is invalid CompatibilityError: If extension is incompatible """ + # Validate priority + if priority < 1: + raise ValidationError("Priority must be a positive integer (1 or higher)") + # Load and validate manifest manifest_path = source_dir / "extension.yml" manifest = ExtensionManifest(manifest_path) @@ -536,9 +543,13 @@ def install_from_zip( Installed extension manifest Raises: - ValidationError: If manifest is invalid + ValidationError: If manifest is invalid or priority is invalid CompatibilityError: If extension is incompatible """ + # Validate priority early + if priority < 1: + raise ValidationError("Priority must be a positive integer (1 or higher)") + with tempfile.TemporaryDirectory() as tmpdir: temp_path = Path(tmpdir) diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 36904c1fbe..edf509cdb5 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -276,6 +276,10 @@ def remove(self, pack_id: str): def update(self, pack_id: str, updates: dict): """Update preset metadata in registry. + Merges the provided updates with the existing entry, preserving any + fields not specified. The installed_at timestamp is always preserved + from the original entry. + Args: pack_id: Preset ID updates: Partial metadata to merge into existing metadata @@ -285,7 +289,13 @@ def update(self, pack_id: str, updates: dict): """ if pack_id not in self.data["presets"]: raise KeyError(f"Preset '{pack_id}' not found in registry") - self.data["presets"][pack_id].update(updates) + existing = self.data["presets"][pack_id] + # Merge: existing fields preserved, new fields override + merged = {**existing, **updates} + # Always preserve original installed_at + if "installed_at" in existing: + merged["installed_at"] = existing["installed_at"] + self.data["presets"][pack_id] = merged self._save() def get(self, pack_id: str) -> Optional[dict]: @@ -311,14 +321,18 @@ def list_by_priority(self) -> List[tuple]: """Get all installed presets sorted by priority. Lower priority number = higher precedence (checked first). + Presets with equal priority are sorted alphabetically by ID + for deterministic ordering. Returns: - List of (pack_id, metadata) tuples sorted by priority + List of (pack_id, metadata_copy) tuples sorted by priority. + Metadata is deep-copied to prevent accidental mutation. """ + import copy packs = self.data["presets"] return sorted( - packs.items(), - key=lambda item: item[1].get("priority", 10), + [(pack_id, copy.deepcopy(meta)) for pack_id, meta in packs.items()], + key=lambda item: (item[1].get("priority", 10), item[0]), ) def is_installed(self, pack_id: str) -> bool: @@ -697,9 +711,13 @@ def install_from_directory( Installed preset manifest Raises: - PresetValidationError: If manifest is invalid + PresetValidationError: If manifest is invalid or priority is invalid PresetCompatibilityError: If pack is incompatible """ + # Validate priority + if priority < 1: + raise PresetValidationError("Priority must be a positive integer (1 or higher)") + manifest_path = source_dir / "preset.yml" manifest = PresetManifest(manifest_path) @@ -752,9 +770,13 @@ def install_from_zip( Installed preset manifest Raises: - PresetValidationError: If manifest is invalid + PresetValidationError: If manifest is invalid or priority is invalid PresetCompatibilityError: If pack is incompatible """ + # Validate priority early + if priority < 1: + raise PresetValidationError("Priority must be a positive integer (1 or higher)") + with tempfile.TemporaryDirectory() as tmpdir: temp_path = Path(tmpdir) @@ -1474,7 +1496,7 @@ def resolve( if subdir: candidate = ext_dir / subdir / f"{template_name}{ext}" else: - candidate = ext_dir / "templates" / f"{template_name}{ext}" + candidate = ext_dir / f"{template_name}{ext}" if candidate.exists(): return candidate diff --git a/tests/test_presets.py b/tests/test_presets.py index ac68ee265d..9ed2fd5f00 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -755,7 +755,7 @@ def test_resolve_with_source_extension(self, project_dir): resolver = PresetResolver(project_dir) result = resolver.resolve_with_source("unique-template") assert result is not None - assert "extension:my-ext" in result["source"] + assert result["source"] == "extension:my-ext v1.0.0" def test_resolve_with_source_not_found(self, project_dir): """Test resolve_with_source for nonexistent template.""" @@ -996,7 +996,7 @@ def test_override_beats_pack_beats_extension_beats_core(self, project_dir, pack_ ext_registry.add("my-ext", {"version": "1.0.0", "priority": 10}) result = resolver.resolve_with_source("spec-template") - assert "extension:my-ext" in result["source"] + assert result["source"] == "extension:my-ext v1.0.0" # Install pack — should win over extension manager = PresetManager(project_dir) From e5b1a1af314e0206ac621800e3d2675df21e2a61 Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Mon, 16 Mar 2026 20:02:34 +0100 Subject: [PATCH 03/12] fix: address additional review feedback - PresetResolver: add fallback to directory scanning when registry is empty/corrupted for robustness and backwards compatibility - PresetRegistry.update(): add guard to prevent injecting installed_at when absent in existing entry (mirrors ExtensionRegistry behavior) - RFC: update extension list example to match actual CLI output format Co-Authored-By: Claude Opus 4.5 --- extensions/RFC-EXTENSION-SYSTEM.md | 14 +++-- src/specify_cli/presets.py | 89 +++++++++++++++++++++--------- 2 files changed, 73 insertions(+), 30 deletions(-) diff --git a/extensions/RFC-EXTENSION-SYSTEM.md b/extensions/RFC-EXTENSION-SYSTEM.md index d7d89511d8..7a7cc8c99a 100644 --- a/extensions/RFC-EXTENSION-SYSTEM.md +++ b/extensions/RFC-EXTENSION-SYSTEM.md @@ -1087,11 +1087,15 @@ List installed extensions in current project. $ specify extension list Installed Extensions: - ✓ jira (v1.0.0) - Jira Integration - Commands: 3 | Hooks: 2 | Priority: 10 | Status: Enabled - - ✓ linear (v0.9.0) - Linear Integration - Commands: 1 | Hooks: 1 | Priority: 10 | Status: Enabled + ✓ Jira Integration (v1.0.0) + jira + Create Jira issues from spec-kit artifacts + Commands: 3 | Hooks: 2 | Priority: 10 | Status: Enabled + + ✓ Linear Integration (v0.9.0) + linear + Create Linear issues from spec-kit artifacts + Commands: 1 | Hooks: 1 | Priority: 10 | Status: Enabled ``` **Options:** diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index edf509cdb5..d056b585cd 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -292,9 +292,13 @@ def update(self, pack_id: str, updates: dict): existing = self.data["presets"][pack_id] # Merge: existing fields preserved, new fields override merged = {**existing, **updates} - # Always preserve original installed_at + # Always preserve original installed_at based on key existence, not truthiness, + # to handle cases where the field exists but may be falsy (legacy/corruption) if "installed_at" in existing: merged["installed_at"] = existing["installed_at"] + else: + # If not present in existing, explicitly remove from merged if caller provided it + merged.pop("installed_at", None) self.data["presets"][pack_id] = merged self._save() @@ -1488,17 +1492,35 @@ def resolve( # Priority 3: Extension-provided templates (sorted by priority — lower number wins) if self.extensions_dir.exists(): registry = ExtensionRegistry(self.extensions_dir) - for ext_id, _metadata in registry.list_by_priority(): - ext_dir = self.extensions_dir / ext_id - if not ext_dir.is_dir(): - continue - for subdir in subdirs: - if subdir: - candidate = ext_dir / subdir / f"{template_name}{ext}" - else: - candidate = ext_dir / f"{template_name}{ext}" - if candidate.exists(): - return candidate + registered_extensions = registry.list_by_priority() + + # If registry is empty but extension directories exist, fall back to + # directory scanning for robustness (handles corrupted/missing registry) + if not registered_extensions: + # Scan directories alphabetically with implicit priority=10 + for ext_dir in sorted(self.extensions_dir.iterdir()): + if not ext_dir.is_dir() or ext_dir.name.startswith("."): + continue + for subdir in subdirs: + if subdir: + candidate = ext_dir / subdir / f"{template_name}{ext}" + else: + candidate = ext_dir / f"{template_name}{ext}" + if candidate.exists(): + return candidate + else: + # Use registry-based resolution with priority ordering + for ext_id, _metadata in registered_extensions: + ext_dir = self.extensions_dir / ext_id + if not ext_dir.is_dir(): + continue + for subdir in subdirs: + if subdir: + candidate = ext_dir / subdir / f"{template_name}{ext}" + else: + candidate = ext_dir / f"{template_name}{ext}" + if candidate.exists(): + return candidate # Priority 4: Core templates if template_type == "template": @@ -1558,18 +1580,35 @@ def resolve_with_source( if self.extensions_dir.exists(): ext_registry = ExtensionRegistry(self.extensions_dir) - for ext_id, ext_meta in ext_registry.list_by_priority(): - ext_dir = self.extensions_dir / ext_id - if not ext_dir.is_dir(): - continue - try: - resolved.relative_to(ext_dir) - version = ext_meta.get("version", "?") if ext_meta else "?" - return { - "path": resolved_str, - "source": f"extension:{ext_id} v{version}", - } - except ValueError: - continue + registered_extensions = ext_registry.list_by_priority() + + if registered_extensions: + # Use registry-based attribution + for ext_id, ext_meta in registered_extensions: + ext_dir = self.extensions_dir / ext_id + if not ext_dir.is_dir(): + continue + try: + resolved.relative_to(ext_dir) + version = ext_meta.get("version", "?") if ext_meta else "?" + return { + "path": resolved_str, + "source": f"extension:{ext_id} v{version}", + } + except ValueError: + continue + else: + # Fallback: scan directories when registry is empty/corrupted + for ext_dir in sorted(self.extensions_dir.iterdir()): + 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} (unregistered)", + } + except ValueError: + continue return {"path": resolved_str, "source": "core"} From e1341fce3946c0086dcbb4dbc2a5e0057c208592 Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Mon, 16 Mar 2026 21:34:57 +0100 Subject: [PATCH 04/12] fix: restore defensive code and RFC descriptions lost in rebase - Restore defensive code in list_by_priority() with .get() and isinstance check - Restore detailed --from URL and --dev option descriptions in RFC Co-Authored-By: Claude Opus 4.5 --- extensions/RFC-EXTENSION-SYSTEM.md | 6 ++---- src/specify_cli/extensions.py | 4 +++- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/extensions/RFC-EXTENSION-SYSTEM.md b/extensions/RFC-EXTENSION-SYSTEM.md index 7a7cc8c99a..5d3d8e9cb2 100644 --- a/extensions/RFC-EXTENSION-SYSTEM.md +++ b/extensions/RFC-EXTENSION-SYSTEM.md @@ -1203,10 +1203,8 @@ Next steps: **Options:** -- `--from URL`: Install from custom URL or Git repo -- `--version VERSION`: Install specific version -- `--dev PATH`: Install from local path (development mode) -- `--no-register`: Skip command registration (manual setup) +- `--from URL`: Install from a remote URL (archive). Does not accept Git repositories directly. +- `--dev`: Install from a local path in development mode (the PATH is the positional `extension` argument). - `--priority NUMBER`: Set resolution priority (lower = higher precedence, default 10) #### `specify extension remove NAME` diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index 33d5112c38..c103a14bb5 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -335,7 +335,9 @@ def list_by_priority(self) -> List[tuple]: List of (extension_id, metadata_copy) tuples sorted by priority. Metadata is deep-copied to prevent accidental mutation. """ - extensions = self.data["extensions"] + extensions = self.data.get("extensions", {}) or {} + if not isinstance(extensions, dict): + extensions = {} return sorted( [(ext_id, copy.deepcopy(meta)) for ext_id, meta in extensions.items()], key=lambda item: (item[1].get("priority", 10), item[0]), From be7edcb54abb8fc3e29ef07186640976f371be43 Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Mon, 16 Mar 2026 21:36:57 +0100 Subject: [PATCH 05/12] fix: add defensive code to presets list_by_priority() - Add .get() and isinstance check for corrupted/empty registry - Move copy import to module level (remove local import) - Matches defensive pattern used in extensions.py Co-Authored-By: Claude Opus 4.5 --- src/specify_cli/presets.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index d056b585cd..75ed4c54ac 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -7,6 +7,7 @@ customize the Spec-Driven Development workflow. """ +import copy import json import hashlib import os @@ -332,8 +333,9 @@ def list_by_priority(self) -> List[tuple]: List of (pack_id, metadata_copy) tuples sorted by priority. Metadata is deep-copied to prevent accidental mutation. """ - import copy - packs = self.data["presets"] + packs = self.data.get("presets", {}) or {} + if not isinstance(packs, dict): + packs = {} return sorted( [(pack_id, copy.deepcopy(meta)) for pack_id, meta in packs.items()], key=lambda item: (item[1].get("priority", 10), item[0]), From 3ed54d3ff4e7cb8df96dd826a9abbcbb18ebe2ba Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Mon, 16 Mar 2026 23:09:04 +0100 Subject: [PATCH 06/12] fix: address reviewer feedback on priority resolution - Rename _normalize_priority to normalize_priority (public API) - Add comprehensive tests for normalize_priority function (9 tests) - Filter non-dict metadata entries in list_by_priority() methods - Fix extension priority resolution to merge registered and unregistered extensions into unified sorted list (unregistered get implicit priority 10) - Add tests for extension priority resolution ordering (4 tests) The key fix ensures unregistered extensions with implicit priority 10 correctly beat registered extensions with priority > 10, and vice versa. Co-Authored-By: Claude Opus 4.5 --- src/specify_cli/extensions.py | 29 ++++++++- src/specify_cli/presets.py | 119 ++++++++++++++++++++-------------- tests/test_extensions.py | 99 ++++++++++++++++++++++++++++ tests/test_presets.py | 116 +++++++++++++++++++++++++++++++++ 4 files changed, 312 insertions(+), 51 deletions(-) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index c103a14bb5..7d40e337d9 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -41,6 +41,26 @@ class CompatibilityError(ExtensionError): pass +def normalize_priority(value: Any, default: int = 10) -> int: + """Normalize a stored priority value for sorting and display. + + Corrupted registry data may contain missing, non-numeric, or non-positive + values. In those cases, fall back to the default priority. + + Args: + value: Priority value to normalize (may be int, str, None, etc.) + default: Default priority to use for invalid values (default: 10) + + Returns: + Normalized priority as positive integer (>= 1) + """ + try: + priority = int(value) + except (TypeError, ValueError): + return default + return priority if priority >= 1 else default + + @dataclass class CatalogEntry: """Represents a single catalog entry in the catalog stack.""" @@ -338,8 +358,15 @@ def list_by_priority(self) -> List[tuple]: extensions = self.data.get("extensions", {}) or {} if not isinstance(extensions, dict): extensions = {} + sortable_extensions = [] + for ext_id, meta in extensions.items(): + if not isinstance(meta, dict): + continue + metadata_copy = copy.deepcopy(meta) + metadata_copy["priority"] = normalize_priority(metadata_copy.get("priority", 10)) + sortable_extensions.append((ext_id, metadata_copy)) return sorted( - [(ext_id, copy.deepcopy(meta)) for ext_id, meta in extensions.items()], + sortable_extensions, key=lambda item: (item[1].get("priority", 10), item[0]), ) diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 75ed4c54ac..8bf7040a12 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -24,7 +24,7 @@ from packaging import version as pkg_version from packaging.specifiers import SpecifierSet, InvalidSpecifier -from .extensions import ExtensionRegistry +from .extensions import ExtensionRegistry, normalize_priority @dataclass @@ -336,8 +336,15 @@ def list_by_priority(self) -> List[tuple]: packs = self.data.get("presets", {}) or {} if not isinstance(packs, dict): packs = {} + sortable_packs = [] + for pack_id, meta in packs.items(): + if not isinstance(meta, dict): + continue + metadata_copy = copy.deepcopy(meta) + metadata_copy["priority"] = normalize_priority(metadata_copy.get("priority", 10)) + sortable_packs.append((pack_id, metadata_copy)) return sorted( - [(pack_id, copy.deepcopy(meta)) for pack_id, meta in packs.items()], + sortable_packs, key=lambda item: (item[1].get("priority", 10), item[0]), ) @@ -1495,34 +1502,37 @@ def resolve( if self.extensions_dir.exists(): registry = ExtensionRegistry(self.extensions_dir) registered_extensions = registry.list_by_priority() + registered_extension_ids = {ext_id for ext_id, _ in registered_extensions} - # If registry is empty but extension directories exist, fall back to - # directory scanning for robustness (handles corrupted/missing registry) - if not registered_extensions: - # Scan directories alphabetically with implicit priority=10 - for ext_dir in sorted(self.extensions_dir.iterdir()): - if not ext_dir.is_dir() or ext_dir.name.startswith("."): - continue - for subdir in subdirs: - if subdir: - candidate = ext_dir / subdir / f"{template_name}{ext}" - else: - candidate = ext_dir / f"{template_name}{ext}" - if candidate.exists(): - return candidate - else: - # Use registry-based resolution with priority ordering - for ext_id, _metadata in registered_extensions: - ext_dir = self.extensions_dir / ext_id - if not ext_dir.is_dir(): - continue - for subdir in subdirs: - if subdir: - candidate = ext_dir / subdir / f"{template_name}{ext}" - else: - candidate = ext_dir / f"{template_name}{ext}" - if candidate.exists(): - return candidate + # Build unified list: (priority, ext_id, metadata_or_none) + # Registered extensions use their stored priority; unregistered get implicit 10. + all_extensions: list[tuple[int, str, dict | None]] = [] + + for ext_id, metadata in registered_extensions: + priority = metadata.get("priority", 10) if metadata else 10 + all_extensions.append((priority, ext_id, metadata)) + + # Add unregistered directories with implicit priority=10 + for ext_dir in self.extensions_dir.iterdir(): + if not ext_dir.is_dir() or ext_dir.name.startswith("."): + continue + if ext_dir.name not in registered_extension_ids: + all_extensions.append((10, ext_dir.name, None)) + + # Sort by (priority, ext_id) for deterministic ordering + all_extensions.sort(key=lambda x: (x[0], x[1])) + + for _priority, ext_id, _metadata in all_extensions: + ext_dir = self.extensions_dir / ext_id + if not ext_dir.is_dir(): + continue + for subdir in subdirs: + if subdir: + candidate = ext_dir / subdir / f"{template_name}{ext}" + else: + candidate = ext_dir / f"{template_name}{ext}" + if candidate.exists(): + return candidate # Priority 4: Core templates if template_type == "template": @@ -1583,34 +1593,43 @@ def resolve_with_source( if self.extensions_dir.exists(): ext_registry = ExtensionRegistry(self.extensions_dir) registered_extensions = ext_registry.list_by_priority() + registered_extension_ids = {ext_id for ext_id, _ in registered_extensions} - if registered_extensions: - # Use registry-based attribution - for ext_id, ext_meta in registered_extensions: - ext_dir = self.extensions_dir / ext_id - if not ext_dir.is_dir(): - continue - try: - resolved.relative_to(ext_dir) - version = ext_meta.get("version", "?") if ext_meta else "?" + # Build unified list: (priority, ext_id, metadata_or_none) + all_extensions: list[tuple[int, str, dict | None]] = [] + + for ext_id, ext_meta in registered_extensions: + priority = ext_meta.get("priority", 10) if ext_meta else 10 + all_extensions.append((priority, ext_id, ext_meta)) + + # Add unregistered directories with implicit priority=10 + for ext_dir in self.extensions_dir.iterdir(): + if not ext_dir.is_dir() or ext_dir.name.startswith("."): + continue + if ext_dir.name not in registered_extension_ids: + all_extensions.append((10, ext_dir.name, None)) + + # Sort by (priority, ext_id) for deterministic ordering + all_extensions.sort(key=lambda x: (x[0], x[1])) + + for _priority, ext_id, ext_meta in all_extensions: + ext_dir = self.extensions_dir / ext_id + if not ext_dir.is_dir(): + continue + try: + resolved.relative_to(ext_dir) + if ext_meta: + version = ext_meta.get("version", "?") return { "path": resolved_str, "source": f"extension:{ext_id} v{version}", } - except ValueError: - continue - else: - # Fallback: scan directories when registry is empty/corrupted - for ext_dir in sorted(self.extensions_dir.iterdir()): - if not ext_dir.is_dir() or ext_dir.name.startswith("."): - continue - try: - resolved.relative_to(ext_dir) + else: return { "path": resolved_str, - "source": f"extension:{ext_dir.name} (unregistered)", + "source": f"extension:{ext_id} (unregistered)", } - except ValueError: - continue + except ValueError: + continue return {"path": resolved_str, "source": "core"} diff --git a/tests/test_extensions.py b/tests/test_extensions.py index ac9d4d611d..abe5697e2b 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -26,6 +26,7 @@ ExtensionError, ValidationError, CompatibilityError, + normalize_priority, version_satisfies, ) @@ -121,6 +122,57 @@ def project_dir(temp_dir): return proj_dir +# ===== normalize_priority Tests ===== + +class TestNormalizePriority: + """Test normalize_priority helper function.""" + + def test_valid_integer(self): + """Test with valid integer priority.""" + assert normalize_priority(5) == 5 + assert normalize_priority(1) == 1 + assert normalize_priority(100) == 100 + + def test_valid_string_number(self): + """Test with string that can be converted to int.""" + assert normalize_priority("5") == 5 + assert normalize_priority("10") == 10 + + def test_zero_returns_default(self): + """Test that zero priority returns default.""" + assert normalize_priority(0) == 10 + assert normalize_priority(0, default=5) == 5 + + def test_negative_returns_default(self): + """Test that negative priority returns default.""" + assert normalize_priority(-1) == 10 + assert normalize_priority(-100, default=5) == 5 + + def test_none_returns_default(self): + """Test that None returns default.""" + assert normalize_priority(None) == 10 + assert normalize_priority(None, default=5) == 5 + + def test_invalid_string_returns_default(self): + """Test that non-numeric string returns default.""" + assert normalize_priority("invalid") == 10 + assert normalize_priority("abc", default=5) == 5 + + def test_float_truncates(self): + """Test that float is truncated to int.""" + assert normalize_priority(5.9) == 5 + assert normalize_priority(3.1) == 3 + + def test_empty_string_returns_default(self): + """Test that empty string returns default.""" + assert normalize_priority("") == 10 + + def test_custom_default(self): + """Test custom default value.""" + assert normalize_priority(None, default=20) == 20 + assert normalize_priority("invalid", default=1) == 1 + + # ===== ExtensionManifest Tests ===== class TestExtensionManifest: @@ -2430,6 +2482,24 @@ def test_list_by_priority_default(self, temp_dir): assert result[1][0] == "ext-default" assert result[2][0] == "ext-low" + def test_list_by_priority_invalid_priority_defaults(self, temp_dir): + """Malformed priority values fall back to the default priority.""" + extensions_dir = temp_dir / "extensions" + extensions_dir.mkdir() + + registry = ExtensionRegistry(extensions_dir) + registry.add("ext-high", {"version": "1.0.0", "priority": 1}) + registry.data["extensions"]["ext-invalid"] = { + "version": "1.0.0", + "priority": "high", + } + registry._save() + + result = registry.list_by_priority() + + assert [item[0] for item in result] == ["ext-high", "ext-invalid"] + assert result[1][1]["priority"] == 10 + def test_install_with_priority(self, extension_dir, project_dir): """Test that install_from_directory stores priority.""" manager = ExtensionManager(project_dir) @@ -2471,6 +2541,35 @@ def test_priority_preserved_on_update(self, temp_dir): assert updated["priority"] == 5 # Preserved assert updated["enabled"] is False # Updated + def test_resolve_uses_unregistered_extension_dirs_when_registry_partially_corrupted(self, project_dir): + """Resolution scans unregistered extension dirs after valid registry entries.""" + extensions_dir = project_dir / ".specify" / "extensions" + + valid_dir = extensions_dir / "valid-ext" / "templates" + valid_dir.mkdir(parents=True) + (valid_dir / "other-template.md").write_text("# Valid\n") + + broken_dir = extensions_dir / "broken-ext" / "templates" + broken_dir.mkdir(parents=True) + (broken_dir / "target-template.md").write_text("# Broken Target\n") + + registry = ExtensionRegistry(extensions_dir) + registry.add("valid-ext", {"version": "1.0.0", "priority": 10}) + registry.data["extensions"]["broken-ext"] = "corrupted" + registry._save() + + from specify_cli.presets import PresetResolver + + resolver = PresetResolver(project_dir) + resolved = resolver.resolve("target-template") + sourced = resolver.resolve_with_source("target-template") + + assert resolved is not None + assert resolved.name == "target-template.md" + assert "Broken Target" in resolved.read_text() + assert sourced is not None + assert sourced["source"] == "extension:broken-ext (unregistered)" + class TestExtensionPriorityCLI: """Test extension priority CLI integration.""" diff --git a/tests/test_presets.py b/tests/test_presets.py index 9ed2fd5f00..74bb6ab8ec 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -574,6 +574,24 @@ def test_list_by_priority_default(self, temp_dir): assert sorted_packs[0][0] == "pack-b" assert sorted_packs[1][0] == "pack-a" + def test_list_by_priority_invalid_priority_defaults(self, temp_dir): + """Malformed priority values fall back to the default priority.""" + packs_dir = temp_dir / "packs" + packs_dir.mkdir() + registry = PresetRegistry(packs_dir) + + registry.add("pack-high", {"version": "1.0.0", "priority": 1}) + registry.data["presets"]["pack-invalid"] = { + "version": "1.0.0", + "priority": "high", + } + registry._save() + + sorted_packs = registry.list_by_priority() + + assert [item[0] for item in sorted_packs] == ["pack-high", "pack-invalid"] + assert sorted_packs[1][1]["priority"] == 10 + # ===== PresetResolver Tests ===== @@ -776,6 +794,104 @@ def test_resolve_skips_hidden_extension_dirs(self, project_dir): assert result is None +class TestExtensionPriorityResolution: + """Test extension priority resolution with registered and unregistered extensions.""" + + def test_unregistered_beats_registered_with_lower_precedence(self, project_dir): + """Unregistered extension (implicit priority 10) beats registered with priority 20.""" + extensions_dir = project_dir / ".specify" / "extensions" + extensions_dir.mkdir(parents=True, exist_ok=True) + + # Create registered extension with priority 20 (lower precedence than 10) + registered_dir = extensions_dir / "registered-ext" + (registered_dir / "templates").mkdir(parents=True) + (registered_dir / "templates" / "test-template.md").write_text("# From Registered\n") + + ext_registry = ExtensionRegistry(extensions_dir) + ext_registry.add("registered-ext", {"version": "1.0.0", "priority": 20}) + + # Create unregistered extension directory (implicit priority 10) + unregistered_dir = extensions_dir / "unregistered-ext" + (unregistered_dir / "templates").mkdir(parents=True) + (unregistered_dir / "templates" / "test-template.md").write_text("# From Unregistered\n") + + # Unregistered (priority 10) should beat registered (priority 20) + resolver = PresetResolver(project_dir) + result = resolver.resolve("test-template") + assert result is not None + assert "From Unregistered" in result.read_text() + + def test_registered_with_higher_precedence_beats_unregistered(self, project_dir): + """Registered extension with priority 5 beats unregistered (implicit priority 10).""" + extensions_dir = project_dir / ".specify" / "extensions" + extensions_dir.mkdir(parents=True, exist_ok=True) + + # Create registered extension with priority 5 (higher precedence than 10) + registered_dir = extensions_dir / "registered-ext" + (registered_dir / "templates").mkdir(parents=True) + (registered_dir / "templates" / "test-template.md").write_text("# From Registered\n") + + ext_registry = ExtensionRegistry(extensions_dir) + ext_registry.add("registered-ext", {"version": "1.0.0", "priority": 5}) + + # Create unregistered extension directory (implicit priority 10) + unregistered_dir = extensions_dir / "unregistered-ext" + (unregistered_dir / "templates").mkdir(parents=True) + (unregistered_dir / "templates" / "test-template.md").write_text("# From Unregistered\n") + + # Registered (priority 5) should beat unregistered (priority 10) + resolver = PresetResolver(project_dir) + result = resolver.resolve("test-template") + assert result is not None + assert "From Registered" in result.read_text() + + def test_unregistered_attribution_with_priority_ordering(self, project_dir): + """Test resolve_with_source correctly attributes unregistered extension.""" + extensions_dir = project_dir / ".specify" / "extensions" + extensions_dir.mkdir(parents=True, exist_ok=True) + + # Create registered extension with priority 20 + registered_dir = extensions_dir / "registered-ext" + (registered_dir / "templates").mkdir(parents=True) + (registered_dir / "templates" / "test-template.md").write_text("# From Registered\n") + + ext_registry = ExtensionRegistry(extensions_dir) + ext_registry.add("registered-ext", {"version": "1.0.0", "priority": 20}) + + # Create unregistered extension (implicit priority 10) + unregistered_dir = extensions_dir / "unregistered-ext" + (unregistered_dir / "templates").mkdir(parents=True) + (unregistered_dir / "templates" / "test-template.md").write_text("# From Unregistered\n") + + # Attribution should show unregistered extension + resolver = PresetResolver(project_dir) + result = resolver.resolve_with_source("test-template") + assert result is not None + assert "unregistered-ext" in result["source"] + assert "(unregistered)" in result["source"] + + def test_same_priority_sorted_alphabetically(self, project_dir): + """Extensions with same priority are sorted alphabetically by ID.""" + extensions_dir = project_dir / ".specify" / "extensions" + extensions_dir.mkdir(parents=True, exist_ok=True) + + # Create two unregistered extensions (both implicit priority 10) + # "aaa-ext" should come before "zzz-ext" alphabetically + zzz_dir = extensions_dir / "zzz-ext" + (zzz_dir / "templates").mkdir(parents=True) + (zzz_dir / "templates" / "test-template.md").write_text("# From ZZZ\n") + + aaa_dir = extensions_dir / "aaa-ext" + (aaa_dir / "templates").mkdir(parents=True) + (aaa_dir / "templates" / "test-template.md").write_text("# From AAA\n") + + # AAA should win due to alphabetical ordering at same priority + resolver = PresetResolver(project_dir) + result = resolver.resolve("test-template") + assert result is not None + assert "From AAA" in result.read_text() + + # ===== PresetCatalog Tests ===== From a6e1d244241092cb6b075cb2c18c95bbdc456406 Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Mon, 16 Mar 2026 23:23:13 +0100 Subject: [PATCH 07/12] fix: DRY refactor and strengthen test assertions - Extract _get_all_extensions_by_priority() helper in PresetResolver to eliminate duplicated extension list construction - Add priority=10 assertion to test_legacy_extension_without_priority_field - Add priority=10 assertion to test_legacy_preset_without_priority_field Co-Authored-By: Claude Opus 4.5 --- src/specify_cli/presets.py | 139 +++++++++++++++++-------------------- tests/test_extensions.py | 3 +- tests/test_presets.py | 3 +- 3 files changed, 68 insertions(+), 77 deletions(-) diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 8bf7040a12..bfc5c77528 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -1446,6 +1446,40 @@ def __init__(self, project_root: Path): self.overrides_dir = self.templates_dir / "overrides" self.extensions_dir = project_root / ".specify" / "extensions" + def _get_all_extensions_by_priority(self) -> list[tuple[int, str, dict | None]]: + """Build unified list of registered and unregistered extensions sorted by priority. + + Registered extensions use their stored priority; unregistered directories + get implicit priority=10. Results are sorted by (priority, ext_id) for + deterministic ordering. + + Returns: + List of (priority, ext_id, metadata_or_none) tuples sorted by priority. + """ + if not self.extensions_dir.exists(): + return [] + + registry = ExtensionRegistry(self.extensions_dir) + registered_extensions = registry.list_by_priority() + registered_extension_ids = {ext_id for ext_id, _ in registered_extensions} + + all_extensions: list[tuple[int, str, dict | None]] = [] + + for ext_id, metadata in registered_extensions: + priority = metadata.get("priority", 10) if metadata else 10 + all_extensions.append((priority, ext_id, metadata)) + + # Add unregistered directories with implicit priority=10 + for ext_dir in self.extensions_dir.iterdir(): + if not ext_dir.is_dir() or ext_dir.name.startswith("."): + continue + if ext_dir.name not in registered_extension_ids: + all_extensions.append((10, ext_dir.name, None)) + + # Sort by (priority, ext_id) for deterministic ordering + all_extensions.sort(key=lambda x: (x[0], x[1])) + return all_extensions + def resolve( self, template_name: str, @@ -1499,40 +1533,17 @@ def resolve( return candidate # Priority 3: Extension-provided templates (sorted by priority — lower number wins) - if self.extensions_dir.exists(): - registry = ExtensionRegistry(self.extensions_dir) - registered_extensions = registry.list_by_priority() - registered_extension_ids = {ext_id for ext_id, _ in registered_extensions} - - # Build unified list: (priority, ext_id, metadata_or_none) - # Registered extensions use their stored priority; unregistered get implicit 10. - all_extensions: list[tuple[int, str, dict | None]] = [] - - for ext_id, metadata in registered_extensions: - priority = metadata.get("priority", 10) if metadata else 10 - all_extensions.append((priority, ext_id, metadata)) - - # Add unregistered directories with implicit priority=10 - for ext_dir in self.extensions_dir.iterdir(): - if not ext_dir.is_dir() or ext_dir.name.startswith("."): - continue - if ext_dir.name not in registered_extension_ids: - all_extensions.append((10, ext_dir.name, None)) - - # Sort by (priority, ext_id) for deterministic ordering - all_extensions.sort(key=lambda x: (x[0], x[1])) - - for _priority, ext_id, _metadata in all_extensions: - ext_dir = self.extensions_dir / ext_id - if not ext_dir.is_dir(): - continue - for subdir in subdirs: - if subdir: - candidate = ext_dir / subdir / f"{template_name}{ext}" - else: - candidate = ext_dir / f"{template_name}{ext}" - if candidate.exists(): - return candidate + for _priority, ext_id, _metadata in self._get_all_extensions_by_priority(): + ext_dir = self.extensions_dir / ext_id + if not ext_dir.is_dir(): + continue + for subdir in subdirs: + if subdir: + candidate = ext_dir / subdir / f"{template_name}{ext}" + else: + candidate = ext_dir / f"{template_name}{ext}" + if candidate.exists(): + return candidate # Priority 4: Core templates if template_type == "template": @@ -1590,46 +1601,24 @@ def resolve_with_source( except ValueError: continue - if self.extensions_dir.exists(): - ext_registry = ExtensionRegistry(self.extensions_dir) - registered_extensions = ext_registry.list_by_priority() - registered_extension_ids = {ext_id for ext_id, _ in registered_extensions} - - # Build unified list: (priority, ext_id, metadata_or_none) - all_extensions: list[tuple[int, str, dict | None]] = [] - - for ext_id, ext_meta in registered_extensions: - priority = ext_meta.get("priority", 10) if ext_meta else 10 - all_extensions.append((priority, ext_id, ext_meta)) - - # Add unregistered directories with implicit priority=10 - for ext_dir in self.extensions_dir.iterdir(): - if not ext_dir.is_dir() or ext_dir.name.startswith("."): - continue - if ext_dir.name not in registered_extension_ids: - all_extensions.append((10, ext_dir.name, None)) - - # Sort by (priority, ext_id) for deterministic ordering - all_extensions.sort(key=lambda x: (x[0], x[1])) - - for _priority, ext_id, ext_meta in all_extensions: - ext_dir = self.extensions_dir / ext_id - if not ext_dir.is_dir(): - continue - try: - resolved.relative_to(ext_dir) - if ext_meta: - version = ext_meta.get("version", "?") - return { - "path": resolved_str, - "source": f"extension:{ext_id} v{version}", - } - else: - return { - "path": resolved_str, - "source": f"extension:{ext_id} (unregistered)", - } - except ValueError: - continue + for _priority, ext_id, ext_meta in self._get_all_extensions_by_priority(): + ext_dir = self.extensions_dir / ext_id + if not ext_dir.is_dir(): + continue + try: + resolved.relative_to(ext_dir) + if ext_meta: + version = ext_meta.get("version", "?") + return { + "path": resolved_str, + "source": f"extension:{ext_id} v{version}", + } + else: + return { + "path": resolved_str, + "source": f"extension:{ext_id} (unregistered)", + } + except ValueError: + continue return {"path": resolved_str, "source": "core"} diff --git a/tests/test_extensions.py b/tests/test_extensions.py index abe5697e2b..c87ba5b533 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -2739,7 +2739,8 @@ def test_legacy_extension_without_priority_field(self, temp_dir): result = registry2.list_by_priority() assert len(result) == 1 assert result[0][0] == "legacy-ext" - # Priority defaults to 10 in sorting + # Priority defaults to 10 and is normalized in returned metadata + assert result[0][1]["priority"] == 10 def test_legacy_extension_in_list_installed(self, extension_dir, project_dir): """list_installed returns priority=10 for legacy extensions without priority field.""" diff --git a/tests/test_presets.py b/tests/test_presets.py index 74bb6ab8ec..b6fe81d5ba 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -1949,7 +1949,8 @@ def test_legacy_preset_without_priority_field(self, temp_dir): result = registry2.list_by_priority() assert len(result) == 1 assert result[0][0] == "legacy-pack" - # Priority defaults to 10 in sorting + # Priority defaults to 10 and is normalized in returned metadata + assert result[0][1]["priority"] == 10 def test_legacy_preset_in_list_installed(self, project_dir, pack_dir): """list_installed returns priority=10 for legacy presets without priority field.""" From 0823ed47b392815d773b4c7371fe44bb22492fc1 Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Mon, 16 Mar 2026 23:51:52 +0100 Subject: [PATCH 08/12] fix: add isinstance(dict) checks for corrupted registry entries Add defensive checks throughout CLI commands and manager methods to handle cases where registry entries may be corrupted (non-dict values). This prevents AttributeError when calling .get() on non-dict metadata. Locations fixed: - __init__.py: preset/extension info, set-priority, enable/disable, upgrade commands - extensions.py: list_installed() - presets.py: list_installed() Co-Authored-By: Claude Opus 4.5 --- src/specify_cli/__init__.py | 37 +++++++++++++++++++++-------------- src/specify_cli/extensions.py | 3 +++ src/specify_cli/presets.py | 3 +++ 3 files changed, 28 insertions(+), 15 deletions(-) diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index 4ca67c67db..98b133c8f8 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -2217,7 +2217,7 @@ def preset_info( console.print("\n [green]Status: installed[/green]") # Get priority from registry pack_metadata = manager.registry.get(pack_id) - priority = pack_metadata.get("priority", 10) if pack_metadata else 10 + priority = pack_metadata.get("priority", 10) if isinstance(pack_metadata, dict) else 10 console.print(f" [dim]Priority:[/dim] {priority}") console.print() return @@ -2281,7 +2281,7 @@ def preset_set_priority( # Get current metadata metadata = manager.registry.get(pack_id) - if metadata is None: + if metadata is None or not isinstance(metadata, dict): console.print(f"[red]Error:[/red] Preset '{pack_id}' not found in registry (corrupted state)") raise typer.Exit(1) @@ -3147,8 +3147,15 @@ def extension_info( # Get local manifest info ext_manifest = manager.get_extension(resolved_installed_id) metadata = manager.registry.get(resolved_installed_id) + metadata_is_dict = isinstance(metadata, dict) + if not metadata_is_dict: + console.print( + "[yellow]Warning:[/yellow] Extension metadata appears to be corrupted; " + "some information may be unavailable." + ) + version = metadata.get("version", "unknown") if metadata_is_dict else "unknown" - console.print(f"\n[bold]{resolved_installed_name}[/bold] (v{metadata.get('version', 'unknown')})") + console.print(f"\n[bold]{resolved_installed_name}[/bold] (v{version})") console.print(f"ID: {resolved_installed_id}") console.print() @@ -3176,7 +3183,7 @@ def extension_info( console.print() console.print("[green]✓ Installed[/green]") - priority = metadata.get("priority", 10) + priority = metadata.get("priority", 10) if metadata_is_dict else 10 console.print(f"[dim]Priority:[/dim] {priority}") console.print(f"\nTo remove: specify extension remove {resolved_installed_id}") return @@ -3272,7 +3279,7 @@ def _print_extension_info(ext_info: dict, manager): if is_installed: console.print("[green]✓ Installed[/green]") metadata = manager.registry.get(ext_info['id']) - priority = metadata.get("priority", 10) if metadata else 10 + priority = metadata.get("priority", 10) if isinstance(metadata, dict) else 10 console.print(f"[dim]Priority:[/dim] {priority}") console.print(f"\nTo remove: specify extension remove {ext_info['id']}") elif install_allowed: @@ -3339,7 +3346,7 @@ def extension_update( for ext_id in extensions_to_update: # Get installed version metadata = manager.registry.get(ext_id) - if metadata is None or "version" not in metadata: + if metadata is None or not isinstance(metadata, dict) or "version" not in metadata: console.print(f"⚠ {ext_id}: Registry entry corrupted or missing (skipping)") continue try: @@ -3524,13 +3531,13 @@ def extension_update( shutil.copy2(cfg_file, new_extension_dir / cfg_file.name) # 9. Restore metadata from backup (installed_at, enabled state) - if backup_registry_entry: + if backup_registry_entry and isinstance(backup_registry_entry, dict): # Copy current registry entry to avoid mutating internal # registry state before explicit restore(). current_metadata = manager.registry.get(extension_id) - if current_metadata is None: + if current_metadata is None or not isinstance(current_metadata, dict): raise RuntimeError( - f"Registry entry for '{extension_id}' missing after install — update incomplete" + f"Registry entry for '{extension_id}' missing or corrupted after install — update incomplete" ) new_metadata = dict(current_metadata) @@ -3595,7 +3602,7 @@ def extension_update( # (files that weren't in the original backup) try: new_registry_entry = manager.registry.get(extension_id) - if new_registry_entry is None: + if new_registry_entry is None or not isinstance(new_registry_entry, dict): new_registered_commands = {} else: new_registered_commands = new_registry_entry.get("registered_commands", {}) @@ -3715,10 +3722,10 @@ def extension_enable( # Update registry metadata = manager.registry.get(extension_id) - if metadata is None: + if metadata is None or not isinstance(metadata, dict): console.print(f"[red]Error:[/red] Extension '{extension_id}' not found in registry (corrupted state)") raise typer.Exit(1) - + if metadata.get("enabled", True): console.print(f"[yellow]Extension '{display_name}' is already enabled[/yellow]") raise typer.Exit(0) @@ -3763,10 +3770,10 @@ def extension_disable( # Update registry metadata = manager.registry.get(extension_id) - if metadata is None: + if metadata is None or not isinstance(metadata, dict): console.print(f"[red]Error:[/red] Extension '{extension_id}' not found in registry (corrupted state)") raise typer.Exit(1) - + if not metadata.get("enabled", True): console.print(f"[yellow]Extension '{display_name}' is already disabled[/yellow]") raise typer.Exit(0) @@ -3818,7 +3825,7 @@ def extension_set_priority( # Get current metadata metadata = manager.registry.get(extension_id) - if metadata is None: + if metadata is None or not isinstance(metadata, dict): console.print(f"[red]Error:[/red] Extension '{extension_id}' not found in registry (corrupted state)") raise typer.Exit(1) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index 7d40e337d9..f2ab3f7187 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -691,6 +691,9 @@ def list_installed(self) -> List[Dict[str, Any]]: result = [] for ext_id, metadata in self.registry.list().items(): + # Ensure metadata is a dictionary to avoid AttributeError when using .get() + if not isinstance(metadata, dict): + metadata = {} ext_dir = self.extensions_dir / ext_id manifest_path = ext_dir / "extension.yml" diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index bfc5c77528..455983ee6c 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -861,6 +861,9 @@ def list_installed(self) -> List[Dict[str, Any]]: result = [] for pack_id, metadata in self.registry.list().items(): + # Ensure metadata is a dictionary to avoid AttributeError when using .get() + if not isinstance(metadata, dict): + metadata = {} pack_dir = self.presets_dir / pack_id manifest_path = pack_dir / "preset.yml" From adb3e59eb9fc145f50f69bf91d76d80c593d6bfb Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Tue, 17 Mar 2026 08:42:32 +0100 Subject: [PATCH 09/12] fix: normalize priority display to match resolution behavior Use normalize_priority() for all priority display in CLI commands to ensure displayed values match actual resolution behavior when registry data is corrupted/hand-edited. Locations fixed: - extensions.py: list_installed() - presets.py: list_installed(), PresetResolver - __init__.py: preset info, extension info, set-priority commands Also added GraphQL query for unresolved PR comments to CLAUDE.md. Co-Authored-By: Claude Opus 4.5 --- CLAUDE.md | 126 ++++++++++++++++++++++++++++++++++ src/specify_cli/__init__.py | 17 +++-- src/specify_cli/extensions.py | 6 +- src/specify_cli/presets.py | 8 +-- 4 files changed, 144 insertions(+), 13 deletions(-) create mode 100644 CLAUDE.md diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000000..a9a8f788fb --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,126 @@ +# CLAUDE.md + +This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. + +## Project Overview + +**Specify CLI** is a Python CLI tool that bootstraps projects for Spec-Driven Development (SDD). It sets up directory structures, templates, and AI agent integrations for multiple coding assistants (Claude Code, Copilot, Cursor, Gemini, etc.). + +## Development Commands + +```bash +# Install dependencies +uv sync + +# Run CLI locally +uv run specify --help +uv run specify init --ai claude +uv run specify check + +# Run tests +uv run pytest # All tests +uv run pytest tests/test_extensions.py # Single file +uv run pytest -k "test_name" # Single test by name + +# Test with coverage +uv run pytest --cov=src --cov-report=term-missing + +# Generate release packages locally (for testing template changes) +./.github/workflows/scripts/create-release-packages.sh v1.0.0 +``` + +## Architecture + +### Source Structure + +- `src/specify_cli/__init__.py` - Main CLI implementation (typer-based) +- `src/specify_cli/agents.py` - CommandRegistrar for agent-specific command formats +- `src/specify_cli/extensions.py` - Extension manager for modular packages +- `src/specify_cli/presets.py` - Preset manager for versioned template collections + +### Key Components + +**AGENT_CONFIG** (in `__init__.py`): Single source of truth for all supported AI agents. Dictionary key must match the actual CLI tool name (e.g., `"cursor-agent"` not `"cursor"`). Fields: + +- `name`: Display name +- `folder`: Agent directory (e.g., `.claude/`) +- `commands_subdir`: Where commands live (e.g., `commands`, `workflows`, `prompts`) +- `requires_cli`: Whether agent needs CLI tool check + +**CommandRegistrar** (in `agents.py`): Handles writing command files to agent directories. Has its own `AGENT_CONFIGS` dict with format-specific details (file extension, argument placeholder style). Must stay in sync with `AGENT_CONFIG` when adding new agents. + +**Extension System** (in `extensions.py`): + +- `ExtensionManifest`: Validates `extension.yml` files +- `ExtensionManager`: Handles install/remove lifecycle +- `ExtensionCatalog`: Fetches from remote catalog with caching +- `HookExecutor`: Manages extension hooks + +**Preset System** (in `presets.py`): + +- `PresetManifest`: Validates `preset.yml` files +- `PresetManager`: Handles preset install/remove lifecycle +- `PresetCatalog`: Fetches presets from remote catalogs with priority stacking + +### Templates + +- `templates/commands/` - Command templates (Markdown format with frontmatter) +- `templates/*.md` - Spec, plan, tasks, and constitution templates + +## Version Management + +Changes to `__init__.py` require: + +1. Version bump in `pyproject.toml` +2. Entry in `CHANGELOG.md` + +## Adding New Agent Support + +1. Add to `AGENT_CONFIG` in `__init__.py` using actual CLI tool name as key +2. Update `--ai` help text in `init()` command +3. Update README.md supported agents table +4. Update release scripts in `.github/workflows/scripts/` +5. Update context scripts in `scripts/bash/` and `scripts/powershell/` + +See [AGENTS.md](AGENTS.md) for detailed guide. + +## Testing Guidelines + +- Tests are in `tests/` using pytest +- `test_extensions.py` - Extension system tests +- `test_presets.py` - Preset system tests +- `test_ai_skills.py` - AI skills/command generation tests +- `test_agent_config_consistency.py` - Validates AGENT_CONFIG consistency +- `test_cursor_frontmatter.py` - Cursor-specific frontmatter handling + +## GitHub PR Reviews + +The GraphQL API with `reviewThreads` and `isResolved` filter is the reliable way to get unresolved Copilot review comments. The REST API does not expose resolution status. + +```bash +# Get unresolved review comments on a PR +gh api graphql -f query=' +query { + repository(owner: "github", name: "spec-kit") { + pullRequest(number: PR_NUMBER) { + reviewThreads(first: 50) { + nodes { + isResolved + isOutdated + path + line + comments(first: 3) { + nodes { + body + author { login } + createdAt + } + } + } + } + } + } +}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | {path, line, outdated: .isOutdated, comment: .comments.nodes[0].body[0:200]}' +``` + +Replace `PR_NUMBER` with the actual PR number. This returns only unresolved threads with their file path, line number, and whether the comment is outdated (code changed since comment). diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index 98b133c8f8..3e1cc09f3d 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -2182,6 +2182,7 @@ def preset_info( pack_id: str = typer.Argument(..., help="Preset ID to get info about"), ): """Show detailed information about a preset.""" + from .extensions import normalize_priority from .presets import PresetCatalog, PresetManager, PresetError project_root = Path.cwd() @@ -2217,7 +2218,7 @@ def preset_info( console.print("\n [green]Status: installed[/green]") # Get priority from registry pack_metadata = manager.registry.get(pack_id) - priority = pack_metadata.get("priority", 10) if isinstance(pack_metadata, dict) else 10 + priority = normalize_priority(pack_metadata.get("priority") if isinstance(pack_metadata, dict) else None) console.print(f" [dim]Priority:[/dim] {priority}") console.print() return @@ -2285,7 +2286,8 @@ def preset_set_priority( console.print(f"[red]Error:[/red] Preset '{pack_id}' not found in registry (corrupted state)") raise typer.Exit(1) - old_priority = metadata.get("priority", 10) + from .extensions import normalize_priority + old_priority = normalize_priority(metadata.get("priority")) if old_priority == priority: console.print(f"[yellow]Preset '{pack_id}' already has priority {priority}[/yellow]") raise typer.Exit(0) @@ -3110,7 +3112,7 @@ def extension_info( extension: str = typer.Argument(help="Extension ID or name"), ): """Show detailed information about an extension.""" - from .extensions import ExtensionCatalog, ExtensionManager + from .extensions import ExtensionCatalog, ExtensionManager, normalize_priority project_root = Path.cwd() @@ -3183,7 +3185,7 @@ def extension_info( console.print() console.print("[green]✓ Installed[/green]") - priority = metadata.get("priority", 10) if metadata_is_dict else 10 + priority = normalize_priority(metadata.get("priority") if metadata_is_dict else None) console.print(f"[dim]Priority:[/dim] {priority}") console.print(f"\nTo remove: specify extension remove {resolved_installed_id}") return @@ -3200,6 +3202,8 @@ def extension_info( def _print_extension_info(ext_info: dict, manager): """Print formatted extension info from catalog data.""" + from .extensions import normalize_priority + # Header verified_badge = " [green]✓ Verified[/green]" if ext_info.get("verified") else "" console.print(f"\n[bold]{ext_info['name']}[/bold] (v{ext_info['version']}){verified_badge}") @@ -3279,7 +3283,7 @@ def _print_extension_info(ext_info: dict, manager): if is_installed: console.print("[green]✓ Installed[/green]") metadata = manager.registry.get(ext_info['id']) - priority = metadata.get("priority", 10) if isinstance(metadata, dict) else 10 + priority = normalize_priority(metadata.get("priority") if isinstance(metadata, dict) else None) console.print(f"[dim]Priority:[/dim] {priority}") console.print(f"\nTo remove: specify extension remove {ext_info['id']}") elif install_allowed: @@ -3829,7 +3833,8 @@ def extension_set_priority( console.print(f"[red]Error:[/red] Extension '{extension_id}' not found in registry (corrupted state)") raise typer.Exit(1) - old_priority = metadata.get("priority", 10) + from .extensions import normalize_priority + old_priority = normalize_priority(metadata.get("priority")) if old_priority == priority: console.print(f"[yellow]Extension '{display_name}' already has priority {priority}[/yellow]") raise typer.Exit(0) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index f2ab3f7187..d7d29b7912 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -367,7 +367,7 @@ def list_by_priority(self) -> List[tuple]: sortable_extensions.append((ext_id, metadata_copy)) return sorted( sortable_extensions, - key=lambda item: (item[1].get("priority", 10), item[0]), + key=lambda item: (item[1]["priority"], item[0]), ) @@ -705,7 +705,7 @@ def list_installed(self) -> List[Dict[str, Any]]: "version": metadata.get("version", "unknown"), "description": manifest.description, "enabled": metadata.get("enabled", True), - "priority": metadata.get("priority", 10), + "priority": normalize_priority(metadata.get("priority")), "installed_at": metadata.get("installed_at"), "command_count": len(manifest.commands), "hook_count": len(manifest.hooks) @@ -718,7 +718,7 @@ def list_installed(self) -> List[Dict[str, Any]]: "version": metadata.get("version", "unknown"), "description": "⚠️ Corrupted extension", "enabled": False, - "priority": metadata.get("priority", 10), + "priority": normalize_priority(metadata.get("priority")), "installed_at": metadata.get("installed_at"), "command_count": 0, "hook_count": 0 diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 455983ee6c..3a3b6d9d7c 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -345,7 +345,7 @@ def list_by_priority(self) -> List[tuple]: sortable_packs.append((pack_id, metadata_copy)) return sorted( sortable_packs, - key=lambda item: (item[1].get("priority", 10), item[0]), + key=lambda item: (item[1]["priority"], item[0]), ) def is_installed(self, pack_id: str) -> bool: @@ -878,7 +878,7 @@ def list_installed(self) -> List[Dict[str, Any]]: "installed_at": metadata.get("installed_at"), "template_count": len(manifest.templates), "tags": manifest.tags, - "priority": metadata.get("priority", 10), + "priority": normalize_priority(metadata.get("priority")), }) except PresetValidationError: result.append({ @@ -890,7 +890,7 @@ def list_installed(self) -> List[Dict[str, Any]]: "installed_at": metadata.get("installed_at"), "template_count": 0, "tags": [], - "priority": metadata.get("priority", 10), + "priority": normalize_priority(metadata.get("priority")), }) return result @@ -1469,7 +1469,7 @@ def _get_all_extensions_by_priority(self) -> list[tuple[int, str, dict | None]]: all_extensions: list[tuple[int, str, dict | None]] = [] for ext_id, metadata in registered_extensions: - priority = metadata.get("priority", 10) if metadata else 10 + priority = normalize_priority(metadata.get("priority") if metadata else None) all_extensions.append((priority, ext_id, metadata)) # Add unregistered directories with implicit priority=10 From 9340b25bed060a8572ee88d41087ab4a9a0d714c Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Tue, 17 Mar 2026 09:09:02 +0100 Subject: [PATCH 10/12] fix: repair corrupted priority values in set-priority commands Changed set-priority commands to check if the raw stored value is already a valid int equal to the requested priority before skipping. This ensures corrupted values (e.g., "high") get repaired even when setting to the default priority (10). Also removed CLAUDE.md that was accidentally added to the repo. Co-Authored-By: Claude Opus 4.5 --- CLAUDE.md | 126 ------------------------------------ src/specify_cli/__init__.py | 16 +++-- 2 files changed, 12 insertions(+), 130 deletions(-) delete mode 100644 CLAUDE.md diff --git a/CLAUDE.md b/CLAUDE.md deleted file mode 100644 index a9a8f788fb..0000000000 --- a/CLAUDE.md +++ /dev/null @@ -1,126 +0,0 @@ -# CLAUDE.md - -This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. - -## Project Overview - -**Specify CLI** is a Python CLI tool that bootstraps projects for Spec-Driven Development (SDD). It sets up directory structures, templates, and AI agent integrations for multiple coding assistants (Claude Code, Copilot, Cursor, Gemini, etc.). - -## Development Commands - -```bash -# Install dependencies -uv sync - -# Run CLI locally -uv run specify --help -uv run specify init --ai claude -uv run specify check - -# Run tests -uv run pytest # All tests -uv run pytest tests/test_extensions.py # Single file -uv run pytest -k "test_name" # Single test by name - -# Test with coverage -uv run pytest --cov=src --cov-report=term-missing - -# Generate release packages locally (for testing template changes) -./.github/workflows/scripts/create-release-packages.sh v1.0.0 -``` - -## Architecture - -### Source Structure - -- `src/specify_cli/__init__.py` - Main CLI implementation (typer-based) -- `src/specify_cli/agents.py` - CommandRegistrar for agent-specific command formats -- `src/specify_cli/extensions.py` - Extension manager for modular packages -- `src/specify_cli/presets.py` - Preset manager for versioned template collections - -### Key Components - -**AGENT_CONFIG** (in `__init__.py`): Single source of truth for all supported AI agents. Dictionary key must match the actual CLI tool name (e.g., `"cursor-agent"` not `"cursor"`). Fields: - -- `name`: Display name -- `folder`: Agent directory (e.g., `.claude/`) -- `commands_subdir`: Where commands live (e.g., `commands`, `workflows`, `prompts`) -- `requires_cli`: Whether agent needs CLI tool check - -**CommandRegistrar** (in `agents.py`): Handles writing command files to agent directories. Has its own `AGENT_CONFIGS` dict with format-specific details (file extension, argument placeholder style). Must stay in sync with `AGENT_CONFIG` when adding new agents. - -**Extension System** (in `extensions.py`): - -- `ExtensionManifest`: Validates `extension.yml` files -- `ExtensionManager`: Handles install/remove lifecycle -- `ExtensionCatalog`: Fetches from remote catalog with caching -- `HookExecutor`: Manages extension hooks - -**Preset System** (in `presets.py`): - -- `PresetManifest`: Validates `preset.yml` files -- `PresetManager`: Handles preset install/remove lifecycle -- `PresetCatalog`: Fetches presets from remote catalogs with priority stacking - -### Templates - -- `templates/commands/` - Command templates (Markdown format with frontmatter) -- `templates/*.md` - Spec, plan, tasks, and constitution templates - -## Version Management - -Changes to `__init__.py` require: - -1. Version bump in `pyproject.toml` -2. Entry in `CHANGELOG.md` - -## Adding New Agent Support - -1. Add to `AGENT_CONFIG` in `__init__.py` using actual CLI tool name as key -2. Update `--ai` help text in `init()` command -3. Update README.md supported agents table -4. Update release scripts in `.github/workflows/scripts/` -5. Update context scripts in `scripts/bash/` and `scripts/powershell/` - -See [AGENTS.md](AGENTS.md) for detailed guide. - -## Testing Guidelines - -- Tests are in `tests/` using pytest -- `test_extensions.py` - Extension system tests -- `test_presets.py` - Preset system tests -- `test_ai_skills.py` - AI skills/command generation tests -- `test_agent_config_consistency.py` - Validates AGENT_CONFIG consistency -- `test_cursor_frontmatter.py` - Cursor-specific frontmatter handling - -## GitHub PR Reviews - -The GraphQL API with `reviewThreads` and `isResolved` filter is the reliable way to get unresolved Copilot review comments. The REST API does not expose resolution status. - -```bash -# Get unresolved review comments on a PR -gh api graphql -f query=' -query { - repository(owner: "github", name: "spec-kit") { - pullRequest(number: PR_NUMBER) { - reviewThreads(first: 50) { - nodes { - isResolved - isOutdated - path - line - comments(first: 3) { - nodes { - body - author { login } - createdAt - } - } - } - } - } - } -}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | {path, line, outdated: .isOutdated, comment: .comments.nodes[0].body[0:200]}' -``` - -Replace `PR_NUMBER` with the actual PR number. This returns only unresolved threads with their file path, line number, and whether the comment is outdated (code changed since comment). diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index 3e1cc09f3d..3ee2c4a003 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -2287,11 +2287,15 @@ def preset_set_priority( raise typer.Exit(1) from .extensions import normalize_priority - old_priority = normalize_priority(metadata.get("priority")) - if old_priority == priority: + raw_priority = metadata.get("priority") + # Only skip if the stored value is already a valid int equal to requested priority + # This ensures corrupted values (e.g., "high") get repaired even when setting to default (10) + if isinstance(raw_priority, int) and raw_priority == priority: console.print(f"[yellow]Preset '{pack_id}' already has priority {priority}[/yellow]") raise typer.Exit(0) + old_priority = normalize_priority(raw_priority) + # Update priority manager.registry.update(pack_id, {"priority": priority}) @@ -3834,11 +3838,15 @@ def extension_set_priority( raise typer.Exit(1) from .extensions import normalize_priority - old_priority = normalize_priority(metadata.get("priority")) - if old_priority == priority: + raw_priority = metadata.get("priority") + # Only skip if the stored value is already a valid int equal to requested priority + # This ensures corrupted values (e.g., "high") get repaired even when setting to default (10) + if isinstance(raw_priority, int) and raw_priority == priority: console.print(f"[yellow]Extension '{display_name}' already has priority {priority}[/yellow]") raise typer.Exit(0) + old_priority = normalize_priority(raw_priority) + # Update priority manager.registry.update(extension_id, {"priority": priority}) From a040036305162dbedc2a5be82c6fd7fbf39313a6 Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Tue, 17 Mar 2026 10:05:57 +0100 Subject: [PATCH 11/12] fix: harden registry update methods against corrupted entries - Normalize priority when restoring during extension update to prevent propagating corrupted values (e.g., "high", 0, negative) - Add isinstance(dict) checks in ExtensionRegistry.update() and PresetRegistry.update() to handle corrupted entries (string/list) that would cause TypeError on merge Co-Authored-By: Claude Opus 4.5 --- src/specify_cli/__init__.py | 5 +++-- src/specify_cli/extensions.py | 3 +++ src/specify_cli/presets.py | 3 +++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index 3ee2c4a003..ec1ccf978e 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -3315,6 +3315,7 @@ def extension_update( ValidationError, CommandRegistrar, HookExecutor, + normalize_priority, ) from packaging import version as pkg_version import shutil @@ -3553,9 +3554,9 @@ def extension_update( if "installed_at" in backup_registry_entry: new_metadata["installed_at"] = backup_registry_entry["installed_at"] - # Preserve the original priority + # Preserve the original priority (normalized to handle corruption) if "priority" in backup_registry_entry: - new_metadata["priority"] = backup_registry_entry["priority"] + new_metadata["priority"] = normalize_priority(backup_registry_entry["priority"]) # If extension was disabled before update, disable it again if not backup_registry_entry.get("enabled", True): diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index d7d29b7912..984ca83d64 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -271,6 +271,9 @@ def update(self, extension_id: str, metadata: dict): raise KeyError(f"Extension '{extension_id}' is not installed") # Merge new metadata with existing, preserving original installed_at existing = self.data["extensions"][extension_id] + # Handle corrupted registry entries (e.g., string/list instead of dict) + if not isinstance(existing, dict): + existing = {} # Merge: existing fields preserved, new fields override merged = {**existing, **metadata} # Always preserve original installed_at based on key existence, not truthiness, diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 3a3b6d9d7c..6d334de845 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -291,6 +291,9 @@ def update(self, pack_id: str, updates: dict): if pack_id not in self.data["presets"]: raise KeyError(f"Preset '{pack_id}' not found in registry") existing = self.data["presets"][pack_id] + # Handle corrupted registry entries (e.g., string/list instead of dict) + if not isinstance(existing, dict): + existing = {} # Merge: existing fields preserved, new fields override merged = {**existing, **updates} # Always preserve original installed_at based on key existence, not truthiness, From c5bf302173bbffc12b92138c801dccdd74b3d4e4 Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Tue, 17 Mar 2026 12:05:19 +0100 Subject: [PATCH 12/12] fix: use safe fallback for version in list_installed() When registry entry is corrupted (non-dict), metadata becomes {} after the isinstance check. Use metadata.get("version", manifest.version) instead of metadata["version"] to avoid KeyError. Co-Authored-By: Claude Opus 4.5 --- src/specify_cli/presets.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 6d334de845..121d596178 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -875,7 +875,7 @@ def list_installed(self) -> List[Dict[str, Any]]: result.append({ "id": pack_id, "name": manifest.name, - "version": metadata["version"], + "version": metadata.get("version", manifest.version), "description": manifest.description, "enabled": metadata.get("enabled", True), "installed_at": metadata.get("installed_at"),