mirror of
https://github.com/github/spec-kit.git
synced 2026-03-20 04:13:08 +00:00
feat(presets): add enable/disable toggle and update semantics (#1891)
* feat(presets): add enable/disable toggle and update semantics Add preset enable/disable CLI commands and update semantics to match the extension system capabilities. Changes: - Add `preset enable` and `preset disable` CLI commands - Add `restore()` method to PresetRegistry for rollback scenarios - Update `get()` and `list()` to return deep copies (prevents mutation) - Update `list_by_priority()` to filter disabled presets by default - Add input validation to `restore()` for defensive programming - Add 16 new tests covering all functionality and edge cases Closes #1851 Closes #1852 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address PR review - deep copy and error message accuracy - Fix error message in restore() to match actual validation ("dict" not "non-empty dict") - Use copy.deepcopy() in restore() to prevent caller mutation - Apply same fixes to ExtensionRegistry for parity - Add /defensive-check command for pre-PR validation - Add tests for restore() validation and deep copy behavior Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * revert: remove defensive-check command from PR * fix: address PR review - clarify messaging and add parity - Add note to enable/disable output clarifying commands/skills remain active - Add include_disabled parameter to ExtensionRegistry.list_by_priority for parity - Add tests for extension disabled filtering Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address PR review - disabled extension resolution and corrupted entries - Fix _get_all_extensions_by_priority to use include_disabled=True for tracking registered IDs, preventing disabled extensions from being picked up as unregistered directories - Add corrupted entry handling to get() - returns None for non-dict entries - Add integration tests for disabled extension template resolution - Add tests for get() corrupted entry handling in both registries Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: handle corrupted registry in list() methods - Add defensive handling to list() when presets/extensions is not a dict - Return empty dict instead of crashing on corrupted registry - Apply same fix to both PresetRegistry and ExtensionRegistry for parity - Add tests for corrupted registry handling Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: validate top-level registry structure in get() and restore() - get() now validates self.data["presets/extensions"] is a dict before accessing - restore() ensures presets/extensions dict exists before writing - Prevents crashes when registry JSON is parseable but has corrupted structure - Applied same fixes to both PresetRegistry and ExtensionRegistry for parity Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: validate root-level JSON structure in _load() and is_installed() - _load() now validates json.load() result is a dict before returning - is_installed() validates presets/extensions is a dict before checking membership - Prevents crashes when registry file is valid JSON but wrong type (e.g., array) - Applied same fixes to both registries for parity Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: normalize presets/extensions field in _load() - _load() now normalizes the presets/extensions field to {} if not a dict - Makes corrupted registries recoverable for add/update/remove operations - Applied same fix to both registries for parity Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: use raw registry keys to track corrupted extensions - Use registry.list().keys() instead of list_by_priority() for tracking - Corrupted entries are now treated as tracked, not picked up as unregistered - Tighten test assertion for disabled preset resolution - Update test to match new expected behavior for corrupted entries Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: handle None metadata in ExtensionManager.remove() - Add defensive check for corrupted metadata in remove() - Match existing pattern in PresetManager.remove() Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: add keys() method and filter corrupted entries in list() - Add lightweight keys() method that returns IDs without deep copy - Update list() to filter out non-dict entries (match type contract) - Use keys() instead of list().keys() for performance - Fix comment to reflect actual behavior Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address defensive-check findings - deep copy, corruption guards, parity - Extension enable/disable: use delta pattern matching presets - add(): use copy.deepcopy(metadata) in both registries - remove(): guard outer field for corruption in both registries - update(): guard outer field for corruption in both registries Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: deep copy updates in update() to prevent caller mutation Both PresetRegistry.update() and ExtensionRegistry.update() now deep copy the input updates/metadata dict to prevent callers from mutating nested objects after the call. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: iamaeroplane <michal.bachorik@gmail.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Michal Bachorik
@@ -369,6 +369,172 @@ class TestPresetRegistry:
|
||||
registry = PresetRegistry(packs_dir)
|
||||
assert registry.get("nonexistent") is None
|
||||
|
||||
def test_restore(self, temp_dir):
|
||||
"""Test restore() preserves timestamps exactly."""
|
||||
packs_dir = temp_dir / "packs"
|
||||
packs_dir.mkdir()
|
||||
registry = PresetRegistry(packs_dir)
|
||||
|
||||
# Create original entry with a specific timestamp
|
||||
original_metadata = {
|
||||
"version": "1.0.0",
|
||||
"source": "local",
|
||||
"installed_at": "2025-01-15T10:30:00+00:00",
|
||||
"enabled": True,
|
||||
}
|
||||
registry.restore("test-pack", original_metadata)
|
||||
|
||||
# Verify exact restoration
|
||||
restored = registry.get("test-pack")
|
||||
assert restored["installed_at"] == "2025-01-15T10:30:00+00:00"
|
||||
assert restored["version"] == "1.0.0"
|
||||
assert restored["enabled"] is True
|
||||
|
||||
def test_restore_rejects_none_metadata(self, temp_dir):
|
||||
"""Test restore() raises ValueError for None metadata."""
|
||||
packs_dir = temp_dir / "packs"
|
||||
packs_dir.mkdir()
|
||||
registry = PresetRegistry(packs_dir)
|
||||
|
||||
with pytest.raises(ValueError, match="metadata must be a dict"):
|
||||
registry.restore("test-pack", None)
|
||||
|
||||
def test_restore_rejects_non_dict_metadata(self, temp_dir):
|
||||
"""Test restore() raises ValueError for non-dict metadata."""
|
||||
packs_dir = temp_dir / "packs"
|
||||
packs_dir.mkdir()
|
||||
registry = PresetRegistry(packs_dir)
|
||||
|
||||
with pytest.raises(ValueError, match="metadata must be a dict"):
|
||||
registry.restore("test-pack", "not-a-dict")
|
||||
|
||||
with pytest.raises(ValueError, match="metadata must be a dict"):
|
||||
registry.restore("test-pack", ["list", "not", "dict"])
|
||||
|
||||
def test_restore_uses_deep_copy(self, temp_dir):
|
||||
"""Test restore() deep copies metadata to prevent mutation."""
|
||||
packs_dir = temp_dir / "packs"
|
||||
packs_dir.mkdir()
|
||||
registry = PresetRegistry(packs_dir)
|
||||
|
||||
original_metadata = {
|
||||
"version": "1.0.0",
|
||||
"nested": {"key": "original"},
|
||||
}
|
||||
registry.restore("test-pack", original_metadata)
|
||||
|
||||
# Mutate the original metadata after restore
|
||||
original_metadata["version"] = "MUTATED"
|
||||
original_metadata["nested"]["key"] = "MUTATED"
|
||||
|
||||
# Registry should have the original values
|
||||
stored = registry.get("test-pack")
|
||||
assert stored["version"] == "1.0.0"
|
||||
assert stored["nested"]["key"] == "original"
|
||||
|
||||
def test_get_returns_deep_copy(self, temp_dir):
|
||||
"""Test that get() returns a deep copy to prevent mutation."""
|
||||
packs_dir = temp_dir / "packs"
|
||||
packs_dir.mkdir()
|
||||
registry = PresetRegistry(packs_dir)
|
||||
|
||||
registry.add("test-pack", {"version": "1.0.0", "nested": {"key": "original"}})
|
||||
|
||||
# Get and mutate the returned copy
|
||||
metadata = registry.get("test-pack")
|
||||
metadata["version"] = "MUTATED"
|
||||
metadata["nested"]["key"] = "MUTATED"
|
||||
|
||||
# Original should be unchanged
|
||||
fresh = registry.get("test-pack")
|
||||
assert fresh["version"] == "1.0.0"
|
||||
assert fresh["nested"]["key"] == "original"
|
||||
|
||||
def test_get_returns_none_for_corrupted_entry(self, temp_dir):
|
||||
"""Test that get() returns None for corrupted (non-dict) entries."""
|
||||
packs_dir = temp_dir / "packs"
|
||||
packs_dir.mkdir()
|
||||
registry = PresetRegistry(packs_dir)
|
||||
|
||||
# Directly corrupt the registry with non-dict entries
|
||||
registry.data["presets"]["corrupted-string"] = "not a dict"
|
||||
registry.data["presets"]["corrupted-list"] = ["not", "a", "dict"]
|
||||
registry.data["presets"]["corrupted-int"] = 42
|
||||
registry._save()
|
||||
|
||||
# All corrupted entries should return None
|
||||
assert registry.get("corrupted-string") is None
|
||||
assert registry.get("corrupted-list") is None
|
||||
assert registry.get("corrupted-int") is None
|
||||
# Non-existent should also return None
|
||||
assert registry.get("nonexistent") is None
|
||||
|
||||
def test_list_returns_deep_copy(self, temp_dir):
|
||||
"""Test that list() returns deep copies to prevent mutation."""
|
||||
packs_dir = temp_dir / "packs"
|
||||
packs_dir.mkdir()
|
||||
registry = PresetRegistry(packs_dir)
|
||||
|
||||
registry.add("test-pack", {"version": "1.0.0", "nested": {"key": "original"}})
|
||||
|
||||
# Get list and mutate
|
||||
all_packs = registry.list()
|
||||
all_packs["test-pack"]["version"] = "MUTATED"
|
||||
all_packs["test-pack"]["nested"]["key"] = "MUTATED"
|
||||
|
||||
# Original should be unchanged
|
||||
fresh = registry.get("test-pack")
|
||||
assert fresh["version"] == "1.0.0"
|
||||
assert fresh["nested"]["key"] == "original"
|
||||
|
||||
def test_list_returns_empty_dict_for_corrupted_registry(self, temp_dir):
|
||||
"""Test that list() returns empty dict when presets is not a dict."""
|
||||
packs_dir = temp_dir / "packs"
|
||||
packs_dir.mkdir()
|
||||
registry = PresetRegistry(packs_dir)
|
||||
|
||||
# Corrupt the registry - presets is a list instead of dict
|
||||
registry.data["presets"] = ["not", "a", "dict"]
|
||||
registry._save()
|
||||
|
||||
# list() should return empty dict, not crash
|
||||
result = registry.list()
|
||||
assert result == {}
|
||||
|
||||
def test_list_by_priority_excludes_disabled(self, temp_dir):
|
||||
"""Test that list_by_priority excludes disabled presets by default."""
|
||||
packs_dir = temp_dir / "packs"
|
||||
packs_dir.mkdir()
|
||||
registry = PresetRegistry(packs_dir)
|
||||
|
||||
registry.add("pack-enabled", {"version": "1.0.0", "enabled": True, "priority": 5})
|
||||
registry.add("pack-disabled", {"version": "1.0.0", "enabled": False, "priority": 1})
|
||||
registry.add("pack-default", {"version": "1.0.0", "priority": 10}) # no enabled field = True
|
||||
|
||||
# Default: exclude disabled
|
||||
by_priority = registry.list_by_priority()
|
||||
pack_ids = [p[0] for p in by_priority]
|
||||
assert "pack-enabled" in pack_ids
|
||||
assert "pack-default" in pack_ids
|
||||
assert "pack-disabled" not in pack_ids
|
||||
|
||||
def test_list_by_priority_includes_disabled_when_requested(self, temp_dir):
|
||||
"""Test that list_by_priority includes disabled presets when requested."""
|
||||
packs_dir = temp_dir / "packs"
|
||||
packs_dir.mkdir()
|
||||
registry = PresetRegistry(packs_dir)
|
||||
|
||||
registry.add("pack-enabled", {"version": "1.0.0", "enabled": True, "priority": 5})
|
||||
registry.add("pack-disabled", {"version": "1.0.0", "enabled": False, "priority": 1})
|
||||
|
||||
# Include disabled
|
||||
by_priority = registry.list_by_priority(include_disabled=True)
|
||||
pack_ids = [p[0] for p in by_priority]
|
||||
assert "pack-enabled" in pack_ids
|
||||
assert "pack-disabled" in pack_ids
|
||||
# Disabled pack has lower priority number, so it comes first when included
|
||||
assert pack_ids[0] == "pack-disabled"
|
||||
|
||||
|
||||
# ===== PresetManager Tests =====
|
||||
|
||||
@@ -707,6 +873,44 @@ class TestPresetResolver:
|
||||
assert result is not None
|
||||
assert "Extension Custom Template" in result.read_text()
|
||||
|
||||
def test_resolve_disabled_extension_templates_skipped(self, project_dir):
|
||||
"""Test that disabled extension templates are not resolved."""
|
||||
# Create extension with templates
|
||||
ext_dir = project_dir / ".specify" / "extensions" / "disabled-ext"
|
||||
ext_templates_dir = ext_dir / "templates"
|
||||
ext_templates_dir.mkdir(parents=True)
|
||||
ext_template = ext_templates_dir / "disabled-template.md"
|
||||
ext_template.write_text("# Disabled Extension Template\n")
|
||||
|
||||
# Register extension as disabled
|
||||
extensions_dir = project_dir / ".specify" / "extensions"
|
||||
ext_registry = ExtensionRegistry(extensions_dir)
|
||||
ext_registry.add("disabled-ext", {"version": "1.0.0", "priority": 1, "enabled": False})
|
||||
|
||||
# Template should NOT be resolved because extension is disabled
|
||||
resolver = PresetResolver(project_dir)
|
||||
result = resolver.resolve("disabled-template")
|
||||
assert result is None, "Disabled extension template should not be resolved"
|
||||
|
||||
def test_resolve_disabled_extension_not_picked_up_as_unregistered(self, project_dir):
|
||||
"""Test that disabled extensions are not picked up via unregistered dir scan."""
|
||||
# Create extension directory with templates
|
||||
ext_dir = project_dir / ".specify" / "extensions" / "test-disabled-ext"
|
||||
ext_templates_dir = ext_dir / "templates"
|
||||
ext_templates_dir.mkdir(parents=True)
|
||||
ext_template = ext_templates_dir / "unique-disabled-template.md"
|
||||
ext_template.write_text("# Should Not Resolve\n")
|
||||
|
||||
# Register the extension but disable it
|
||||
extensions_dir = project_dir / ".specify" / "extensions"
|
||||
ext_registry = ExtensionRegistry(extensions_dir)
|
||||
ext_registry.add("test-disabled-ext", {"version": "1.0.0", "enabled": False})
|
||||
|
||||
# Verify the template is NOT resolved (even though the directory exists)
|
||||
resolver = PresetResolver(project_dir)
|
||||
result = resolver.resolve("unique-disabled-template")
|
||||
assert result is None, "Disabled extension should not be picked up as unregistered"
|
||||
|
||||
def test_resolve_pack_over_extension(self, project_dir, pack_dir, temp_dir, valid_pack_data):
|
||||
"""Test that pack templates take priority over extension templates."""
|
||||
# Create extension with templates
|
||||
@@ -2001,3 +2205,189 @@ class TestPresetPriorityBackwardsCompatibility:
|
||||
"legacy-pack",
|
||||
"low-priority-pack",
|
||||
]
|
||||
|
||||
|
||||
class TestPresetEnableDisable:
|
||||
"""Test preset enable/disable CLI commands."""
|
||||
|
||||
def test_disable_preset(self, project_dir, pack_dir):
|
||||
"""Test disable command sets enabled=False."""
|
||||
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")
|
||||
|
||||
# Verify initially enabled
|
||||
assert manager.registry.get("test-pack").get("enabled", True) is True
|
||||
|
||||
with patch.object(Path, "cwd", return_value=project_dir):
|
||||
result = runner.invoke(app, ["preset", "disable", "test-pack"])
|
||||
|
||||
assert result.exit_code == 0, result.output
|
||||
assert "disabled" in result.output.lower()
|
||||
|
||||
# Reload registry to see updated value
|
||||
manager2 = PresetManager(project_dir)
|
||||
assert manager2.registry.get("test-pack")["enabled"] is False
|
||||
|
||||
def test_enable_preset(self, project_dir, pack_dir):
|
||||
"""Test enable command sets enabled=True."""
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
runner = CliRunner()
|
||||
|
||||
# Install preset and disable it
|
||||
manager = PresetManager(project_dir)
|
||||
manager.install_from_directory(pack_dir, "0.1.5")
|
||||
manager.registry.update("test-pack", {"enabled": False})
|
||||
|
||||
# Verify disabled
|
||||
assert manager.registry.get("test-pack")["enabled"] is False
|
||||
|
||||
with patch.object(Path, "cwd", return_value=project_dir):
|
||||
result = runner.invoke(app, ["preset", "enable", "test-pack"])
|
||||
|
||||
assert result.exit_code == 0, result.output
|
||||
assert "enabled" in result.output.lower()
|
||||
|
||||
# Reload registry to see updated value
|
||||
manager2 = PresetManager(project_dir)
|
||||
assert manager2.registry.get("test-pack")["enabled"] is True
|
||||
|
||||
def test_disable_already_disabled(self, project_dir, pack_dir):
|
||||
"""Test disable on already disabled preset shows warning."""
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
runner = CliRunner()
|
||||
|
||||
# Install preset and disable it
|
||||
manager = PresetManager(project_dir)
|
||||
manager.install_from_directory(pack_dir, "0.1.5")
|
||||
manager.registry.update("test-pack", {"enabled": False})
|
||||
|
||||
with patch.object(Path, "cwd", return_value=project_dir):
|
||||
result = runner.invoke(app, ["preset", "disable", "test-pack"])
|
||||
|
||||
assert result.exit_code == 0, result.output
|
||||
assert "already disabled" in result.output.lower()
|
||||
|
||||
def test_enable_already_enabled(self, project_dir, pack_dir):
|
||||
"""Test enable on already enabled preset shows warning."""
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
runner = CliRunner()
|
||||
|
||||
# Install preset (enabled by default)
|
||||
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", "enable", "test-pack"])
|
||||
|
||||
assert result.exit_code == 0, result.output
|
||||
assert "already enabled" in result.output.lower()
|
||||
|
||||
def test_disable_not_installed(self, project_dir):
|
||||
"""Test disable 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", "disable", "nonexistent"])
|
||||
|
||||
assert result.exit_code == 1, result.output
|
||||
assert "not installed" in result.output.lower()
|
||||
|
||||
def test_enable_not_installed(self, project_dir):
|
||||
"""Test enable 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", "enable", "nonexistent"])
|
||||
|
||||
assert result.exit_code == 1, result.output
|
||||
assert "not installed" in result.output.lower()
|
||||
|
||||
def test_disabled_preset_excluded_from_resolution(self, project_dir, pack_dir):
|
||||
"""Test that disabled presets are excluded from template resolution."""
|
||||
# Install preset with a template
|
||||
manager = PresetManager(project_dir)
|
||||
manager.install_from_directory(pack_dir, "0.1.5")
|
||||
|
||||
# Create a template in the preset directory
|
||||
preset_template = project_dir / ".specify" / "presets" / "test-pack" / "templates" / "test-template.md"
|
||||
preset_template.parent.mkdir(parents=True, exist_ok=True)
|
||||
preset_template.write_text("# Template from test-pack")
|
||||
|
||||
resolver = PresetResolver(project_dir)
|
||||
|
||||
# Template should be found when enabled
|
||||
result = resolver.resolve("test-template", "template")
|
||||
assert result is not None
|
||||
assert "test-pack" in str(result)
|
||||
|
||||
# Disable the preset
|
||||
manager.registry.update("test-pack", {"enabled": False})
|
||||
|
||||
# Template should NOT be found when disabled
|
||||
resolver2 = PresetResolver(project_dir)
|
||||
result2 = resolver2.resolve("test-template", "template")
|
||||
assert result2 is None
|
||||
|
||||
def test_enable_corrupted_registry_entry(self, project_dir, pack_dir):
|
||||
"""Test enable fails gracefully for corrupted registry entry."""
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
runner = CliRunner()
|
||||
|
||||
# Install preset then corrupt the registry entry
|
||||
manager = PresetManager(project_dir)
|
||||
manager.install_from_directory(pack_dir, "0.1.5")
|
||||
manager.registry.data["presets"]["test-pack"] = "corrupted-string"
|
||||
manager.registry._save()
|
||||
|
||||
with patch.object(Path, "cwd", return_value=project_dir):
|
||||
result = runner.invoke(app, ["preset", "enable", "test-pack"])
|
||||
|
||||
assert result.exit_code == 1
|
||||
assert "corrupted state" in result.output.lower()
|
||||
|
||||
def test_disable_corrupted_registry_entry(self, project_dir, pack_dir):
|
||||
"""Test disable fails gracefully for corrupted registry entry."""
|
||||
from typer.testing import CliRunner
|
||||
from unittest.mock import patch
|
||||
from specify_cli import app
|
||||
|
||||
runner = CliRunner()
|
||||
|
||||
# Install preset then corrupt the registry entry
|
||||
manager = PresetManager(project_dir)
|
||||
manager.install_from_directory(pack_dir, "0.1.5")
|
||||
manager.registry.data["presets"]["test-pack"] = "corrupted-string"
|
||||
manager.registry._save()
|
||||
|
||||
with patch.object(Path, "cwd", return_value=project_dir):
|
||||
result = runner.invoke(app, ["preset", "disable", "test-pack"])
|
||||
|
||||
assert result.exit_code == 1
|
||||
assert "corrupted state" in result.output.lower()
|
||||
|
||||
Reference in New Issue
Block a user