From ceb55efde54dbb12c736e2acdb11ac94ef0a402a Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Sat, 14 Mar 2026 17:54:31 +0100 Subject: [PATCH 1/2] 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 | 116 +++++++++- 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, 698 insertions(+), 18 deletions(-) diff --git a/extensions/RFC-EXTENSION-SYSTEM.md b/extensions/RFC-EXTENSION-SYSTEM.md index a0f6034e5..d7d89511d 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 a45535aee..5718e9a3a 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 ===== @@ -2576,8 +2627,9 @@ def extension_list( status_color = "green" if ext["enabled"] else "red" 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: @@ -2765,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 @@ -2794,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) @@ -2827,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) @@ -2871,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(): @@ -3113,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 @@ -3206,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]") @@ -3470,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 @@ -3716,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 0dfd40b7c..a3df56f52 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 151963379..36904c1fb 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 6299abbb8..3efab56f6 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -2337,3 +2337,330 @@ def test_update_failure_rolls_back_registry_hooks_and_commands(self, tmp_path): for cmd_file in command_files: assert cmd_file.exists(), f"Expected command file to be restored after rollback: {cmd_file}" + + +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 3ad70c6d1..ac68ee265 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 13a78ae5ef24f84c5aea4876711061687a40293f Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Sat, 14 Mar 2026 18:04:15 +0100 Subject: [PATCH 2/2] 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 5718e9a3a..4ca67c67d 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 a3df56f52..33d5112c3 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 36904c1fb..edf509cdb 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 ac68ee265..9ed2fd5f0 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)