From fe484b2e212b0aaea6bab5e155440ae3c297ca5f Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Wed, 18 Mar 2026 07:59:15 +0100 Subject: [PATCH 01/13] 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 --- src/specify_cli/__init__.py | 81 ++++++++++ src/specify_cli/presets.py | 42 ++++- tests/test_presets.py | 298 ++++++++++++++++++++++++++++++++++++ 3 files changed, 416 insertions(+), 5 deletions(-) diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index ff2364d29a..83fa85808f 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -2405,6 +2405,87 @@ def preset_set_priority( console.print("\n[dim]Lower priority = higher precedence in template resolution[/dim]") +@preset_app.command("enable") +def preset_enable( + pack_id: str = typer.Argument(help="Preset ID to enable"), +): + """Enable a disabled 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) + + 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 or not isinstance(metadata, dict): + console.print(f"[red]Error:[/red] Preset '{pack_id}' not found in registry (corrupted state)") + raise typer.Exit(1) + + if metadata.get("enabled", True): + console.print(f"[yellow]Preset '{pack_id}' is already enabled[/yellow]") + raise typer.Exit(0) + + # Enable the preset + manager.registry.update(pack_id, {"enabled": True}) + + console.print(f"[green]✓[/green] Preset '{pack_id}' enabled") + console.print("\nTemplates from this preset will now be included in resolution.") + + +@preset_app.command("disable") +def preset_disable( + pack_id: str = typer.Argument(help="Preset ID to disable"), +): + """Disable a preset without removing it.""" + 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) + + 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 or not isinstance(metadata, dict): + console.print(f"[red]Error:[/red] Preset '{pack_id}' not found in registry (corrupted state)") + raise typer.Exit(1) + + if not metadata.get("enabled", True): + console.print(f"[yellow]Preset '{pack_id}' is already disabled[/yellow]") + raise typer.Exit(0) + + # Disable the preset + manager.registry.update(pack_id, {"enabled": False}) + + console.print(f"[green]✓[/green] Preset '{pack_id}' disabled") + console.print("\nTemplates from this preset will be skipped during resolution.") + console.print(f"To re-enable: specify preset enable {pack_id}") + + # ===== Preset Catalog Commands ===== diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 121d596178..f74cc6ba71 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -306,32 +306,61 @@ def update(self, pack_id: str, updates: dict): self.data["presets"][pack_id] = merged self._save() + def restore(self, pack_id: str, metadata: dict): + """Restore preset metadata to registry without modifying timestamps. + + Use this method for rollback scenarios where you have a complete backup + of the registry entry (including installed_at) and want to restore it + exactly as it was. + + Args: + pack_id: Preset ID + metadata: Complete preset metadata including installed_at + + Raises: + ValueError: If metadata is None or not a dict + """ + if metadata is None or not isinstance(metadata, dict): + raise ValueError(f"Cannot restore '{pack_id}': metadata must be a non-empty dict") + self.data["presets"][pack_id] = dict(metadata) + self._save() + def get(self, pack_id: str) -> Optional[dict]: """Get preset metadata from registry. + Returns a deep copy to prevent callers from accidentally mutating + nested internal registry state without going through the write path. + Args: pack_id: Preset ID Returns: - Pack metadata or None if not found + Deep copy of preset metadata, or None if not found """ - return self.data["presets"].get(pack_id) + entry = self.data["presets"].get(pack_id) + return copy.deepcopy(entry) if entry is not None else None def list(self) -> Dict[str, dict]: """Get all installed presets. + Returns a deep copy of the presets mapping to prevent callers + from accidentally mutating nested internal registry state. + Returns: - Dictionary of pack_id -> metadata + Dictionary of pack_id -> metadata (deep copies) """ - return self.data["presets"] + return copy.deepcopy(self.data["presets"]) - def list_by_priority(self) -> List[tuple]: + def list_by_priority(self, include_disabled: bool = False) -> 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. + Args: + include_disabled: If True, include disabled presets. Default False. + Returns: List of (pack_id, metadata_copy) tuples sorted by priority. Metadata is deep-copied to prevent accidental mutation. @@ -343,6 +372,9 @@ def list_by_priority(self) -> List[tuple]: for pack_id, meta in packs.items(): if not isinstance(meta, dict): continue + # Skip disabled presets unless explicitly requested + if not include_disabled and not meta.get("enabled", True): + continue metadata_copy = copy.deepcopy(meta) metadata_copy["priority"] = normalize_priority(metadata_copy.get("priority", 10)) sortable_packs.append((pack_id, metadata_copy)) diff --git a/tests/test_presets.py b/tests/test_presets.py index b6fe81d5ba..33d1e68f4a 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -369,6 +369,118 @@ def test_get_nonexistent(self, temp_dir): 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 non-empty 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 non-empty dict"): + registry.restore("test-pack", "not-a-dict") + + with pytest.raises(ValueError, match="metadata must be a non-empty dict"): + registry.restore("test-pack", ["list", "not", "dict"]) + + 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_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_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 ===== @@ -2001,3 +2113,189 @@ def test_mixed_legacy_and_new_presets_ordering(self, temp_dir): "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 or "test-pack" not in str(result2) + + 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() From 517afdb79ad76e0f753da63d3e35086ff19f9ffb Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Wed, 18 Mar 2026 08:18:09 +0100 Subject: [PATCH 02/13] 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 --- .claude/commands/defensive-check.md | 166 ++++++++++++++++++++++++++++ src/specify_cli/extensions.py | 7 +- src/specify_cli/presets.py | 4 +- tests/test_extensions.py | 42 +++++++ tests/test_presets.py | 27 ++++- 5 files changed, 240 insertions(+), 6 deletions(-) create mode 100644 .claude/commands/defensive-check.md diff --git a/.claude/commands/defensive-check.md b/.claude/commands/defensive-check.md new file mode 100644 index 0000000000..986fe29e3a --- /dev/null +++ b/.claude/commands/defensive-check.md @@ -0,0 +1,166 @@ +--- +description: Scan Python code for defensive programming gaps before PRs. Checks for missing None/type validation, shallow vs deep copy issues, error message accuracy, and parity between extension/preset systems. +--- + +## User Input + +```text +$ARGUMENTS +``` + +You **MUST** consider the user input before proceeding (if not empty). User can specify files or scope to review. + +## Goal + +Identify missing defensive programming checks that could lead to runtime errors, data corruption, or inconsistent behavior. This is especially important before creating PRs to catch issues that reviewers commonly flag. + +## Operating Constraints + +**READ-ONLY ANALYSIS**: Do **not** modify any files. Output a structured report with specific fix suggestions. + +**Parity Enforcement**: The extension system (`extensions.py`) and preset system (`presets.py`) MUST have equivalent defensive patterns. Asymmetry is always an issue. + +## Review Scope + +Default behavior: +1. If there are staged changes: review `git diff --cached` +2. If no staged changes: review `git diff` +3. If user specifies files: review those files + +For parity checks, always examine both `extensions.py` and `presets.py` when either is modified. + +## Defensive Programming Checklist + +### 1. Input Validation + +| Pattern | Bad | Good | +|---------|-----|------| +| None check | `data["key"]` | `if data is None: raise` | +| Type check | `data.get("x")` on unknown | `if not isinstance(data, dict): raise` | +| Empty check | error says "non-empty" but accepts `{}` | error message matches actual validation | + +### 2. Data Integrity + +| Pattern | Bad | Good | +|---------|-----|------| +| Return mutable | `return self.data[id]` | `return copy.deepcopy(self.data[id])` | +| Store mutable | `self.data[id] = dict(input)` | `self.data[id] = copy.deepcopy(input)` | +| List return | `return self.data` | `return copy.deepcopy(self.data)` | + +### 3. Error Handling + +| Pattern | Bad | Good | +|---------|-----|------| +| Silent failure | `pass` on exception | Log or re-raise with context | +| Missing Raises | No docstring for exceptions | Document all raised exceptions | +| Corrupted state | Assume dict structure | Check `isinstance()` before access | + +### 4. Code Parity (Extension/Preset) + +Methods that MUST match between `ExtensionRegistry` and `PresetRegistry`: +- `get()` - both must return deep copy +- `list()` - both must return deep copy +- `update()` - both must preserve `installed_at` +- `restore()` - both must validate input and use deep copy +- `list_by_priority()` - both must handle corrupted entries + +CLI commands that MUST match: +- `extension enable/disable` ↔ `preset enable/disable` +- `extension set-priority` ↔ `preset set-priority` +- Error messages, validation patterns, exit codes + +## Execution Steps + +### 1. Determine Review Scope + +```bash +# Check for staged changes +git diff --cached --name-only + +# If empty, check unstaged +git diff --name-only +``` + +### 2. Identify Files to Review + +Filter for Python files in: +- `src/specify_cli/presets.py` +- `src/specify_cli/extensions.py` +- `src/specify_cli/__init__.py` (CLI commands) +- `tests/test_presets.py` +- `tests/test_extensions.py` + +### 3. Run Detection Passes + +#### A. Input Validation Pass +Scan for functions that: +- Accept `dict`, `list`, or `Optional` parameters +- Access dict keys without None/type checks +- Have error messages that don't match validation logic + +#### B. Data Integrity Pass +Scan for methods that: +- Return internal data structures without deep copy +- Store input data with shallow copy (`dict()` instead of `deepcopy()`) +- Expose mutable state + +#### C. Parity Pass +When `presets.py` or `extensions.py` is in scope: +1. List all methods in changed file +2. Find equivalent methods in the other file +3. Compare defensive patterns +4. Flag asymmetries + +#### D. Test Coverage Pass +For each defensive check added: +- Verify corresponding test exists +- Check edge cases (None, wrong type, empty, corrupted) + +### 4. Severity Assignment + +- **CRITICAL**: Missing check that will cause crash or data corruption +- **IMPORTANT**: Missing parity, inconsistent error messages, missing deep copy +- **SUGGESTION**: Could be more defensive but unlikely to cause issues + +## Output Format + +## Defensive Programming Review + +### Summary +- Files reviewed: N +- Issues found: N (X critical, Y important, Z suggestions) + +### Critical Issues + +| Location | Issue | Fix | +|----------|-------|-----| +| presets.py:320 | `restore()` uses shallow copy | Use `copy.deepcopy(metadata)` | + +### Important Issues + +| Location | Issue | Fix | +|----------|-------|-----| +| presets.py:320 | Error says "non-empty" but accepts `{}` | Change to "must be a dict" | + +### Parity Issues + +| Extension Location | Preset Location | Issue | +|--------------------|-----------------|-------| +| extensions.py:300 | presets.py:320 | Extension has validation, preset missing | + +### Suggestions + +(Lower priority improvements) + +### Test Coverage + +| Defensive Check | Test Exists? | Test Location | +|-----------------|--------------|---------------| +| restore() rejects None | Yes | test_presets.py:392 | + +## Next Actions + +Based on findings, suggest: +1. Specific code changes with line numbers +2. Tests to add +3. Files to check for parity diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index 984ca83d64..b51aecbfb4 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -296,8 +296,13 @@ def restore(self, extension_id: str, metadata: dict): Args: extension_id: Extension ID metadata: Complete extension metadata including installed_at + + Raises: + ValueError: If metadata is None or not a dict """ - self.data["extensions"][extension_id] = dict(metadata) + if metadata is None or not isinstance(metadata, dict): + raise ValueError(f"Cannot restore '{extension_id}': metadata must be a dict") + self.data["extensions"][extension_id] = copy.deepcopy(metadata) self._save() def remove(self, extension_id: str): diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index f74cc6ba71..52e0e6c555 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -321,8 +321,8 @@ def restore(self, pack_id: str, metadata: dict): ValueError: If metadata is None or not a dict """ if metadata is None or not isinstance(metadata, dict): - raise ValueError(f"Cannot restore '{pack_id}': metadata must be a non-empty dict") - self.data["presets"][pack_id] = dict(metadata) + raise ValueError(f"Cannot restore '{pack_id}': metadata must be a dict") + self.data["presets"][pack_id] = copy.deepcopy(metadata) self._save() def get(self, pack_id: str) -> Optional[dict]: diff --git a/tests/test_extensions.py b/tests/test_extensions.py index c87ba5b533..1efbea12a3 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -420,6 +420,48 @@ def test_restore_can_recreate_removed_entry(self, temp_dir): assert registry.is_installed("test-ext") assert registry.get("test-ext")["version"] == "1.0.0" + def test_restore_rejects_none_metadata(self, temp_dir): + """Test restore() raises ValueError for None metadata.""" + extensions_dir = temp_dir / "extensions" + extensions_dir.mkdir() + registry = ExtensionRegistry(extensions_dir) + + with pytest.raises(ValueError, match="metadata must be a dict"): + registry.restore("test-ext", None) + + def test_restore_rejects_non_dict_metadata(self, temp_dir): + """Test restore() raises ValueError for non-dict metadata.""" + extensions_dir = temp_dir / "extensions" + extensions_dir.mkdir() + registry = ExtensionRegistry(extensions_dir) + + with pytest.raises(ValueError, match="metadata must be a dict"): + registry.restore("test-ext", "not-a-dict") + + with pytest.raises(ValueError, match="metadata must be a dict"): + registry.restore("test-ext", ["list", "not", "dict"]) + + def test_restore_uses_deep_copy(self, temp_dir): + """Test restore() deep copies metadata to prevent mutation.""" + extensions_dir = temp_dir / "extensions" + extensions_dir.mkdir() + registry = ExtensionRegistry(extensions_dir) + + original_metadata = { + "version": "1.0.0", + "nested": {"key": "original"}, + } + registry.restore("test-ext", 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-ext") + assert stored["version"] == "1.0.0" + assert stored["nested"]["key"] == "original" + def test_get_returns_deep_copy(self, temp_dir): """Test that get() returns deep copies for nested structures.""" extensions_dir = temp_dir / "extensions" diff --git a/tests/test_presets.py b/tests/test_presets.py index 33d1e68f4a..becca0ca04 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -396,7 +396,7 @@ def test_restore_rejects_none_metadata(self, temp_dir): packs_dir.mkdir() registry = PresetRegistry(packs_dir) - with pytest.raises(ValueError, match="metadata must be a non-empty dict"): + 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): @@ -405,12 +405,33 @@ def test_restore_rejects_non_dict_metadata(self, temp_dir): packs_dir.mkdir() registry = PresetRegistry(packs_dir) - with pytest.raises(ValueError, match="metadata must be a non-empty dict"): + 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 non-empty 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" From 6303497bb3e555ab57b6e4708812dd15604db251 Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Wed, 18 Mar 2026 09:04:06 +0100 Subject: [PATCH 03/13] revert: remove defensive-check command from PR --- .claude/commands/defensive-check.md | 166 ---------------------------- 1 file changed, 166 deletions(-) delete mode 100644 .claude/commands/defensive-check.md diff --git a/.claude/commands/defensive-check.md b/.claude/commands/defensive-check.md deleted file mode 100644 index 986fe29e3a..0000000000 --- a/.claude/commands/defensive-check.md +++ /dev/null @@ -1,166 +0,0 @@ ---- -description: Scan Python code for defensive programming gaps before PRs. Checks for missing None/type validation, shallow vs deep copy issues, error message accuracy, and parity between extension/preset systems. ---- - -## User Input - -```text -$ARGUMENTS -``` - -You **MUST** consider the user input before proceeding (if not empty). User can specify files or scope to review. - -## Goal - -Identify missing defensive programming checks that could lead to runtime errors, data corruption, or inconsistent behavior. This is especially important before creating PRs to catch issues that reviewers commonly flag. - -## Operating Constraints - -**READ-ONLY ANALYSIS**: Do **not** modify any files. Output a structured report with specific fix suggestions. - -**Parity Enforcement**: The extension system (`extensions.py`) and preset system (`presets.py`) MUST have equivalent defensive patterns. Asymmetry is always an issue. - -## Review Scope - -Default behavior: -1. If there are staged changes: review `git diff --cached` -2. If no staged changes: review `git diff` -3. If user specifies files: review those files - -For parity checks, always examine both `extensions.py` and `presets.py` when either is modified. - -## Defensive Programming Checklist - -### 1. Input Validation - -| Pattern | Bad | Good | -|---------|-----|------| -| None check | `data["key"]` | `if data is None: raise` | -| Type check | `data.get("x")` on unknown | `if not isinstance(data, dict): raise` | -| Empty check | error says "non-empty" but accepts `{}` | error message matches actual validation | - -### 2. Data Integrity - -| Pattern | Bad | Good | -|---------|-----|------| -| Return mutable | `return self.data[id]` | `return copy.deepcopy(self.data[id])` | -| Store mutable | `self.data[id] = dict(input)` | `self.data[id] = copy.deepcopy(input)` | -| List return | `return self.data` | `return copy.deepcopy(self.data)` | - -### 3. Error Handling - -| Pattern | Bad | Good | -|---------|-----|------| -| Silent failure | `pass` on exception | Log or re-raise with context | -| Missing Raises | No docstring for exceptions | Document all raised exceptions | -| Corrupted state | Assume dict structure | Check `isinstance()` before access | - -### 4. Code Parity (Extension/Preset) - -Methods that MUST match between `ExtensionRegistry` and `PresetRegistry`: -- `get()` - both must return deep copy -- `list()` - both must return deep copy -- `update()` - both must preserve `installed_at` -- `restore()` - both must validate input and use deep copy -- `list_by_priority()` - both must handle corrupted entries - -CLI commands that MUST match: -- `extension enable/disable` ↔ `preset enable/disable` -- `extension set-priority` ↔ `preset set-priority` -- Error messages, validation patterns, exit codes - -## Execution Steps - -### 1. Determine Review Scope - -```bash -# Check for staged changes -git diff --cached --name-only - -# If empty, check unstaged -git diff --name-only -``` - -### 2. Identify Files to Review - -Filter for Python files in: -- `src/specify_cli/presets.py` -- `src/specify_cli/extensions.py` -- `src/specify_cli/__init__.py` (CLI commands) -- `tests/test_presets.py` -- `tests/test_extensions.py` - -### 3. Run Detection Passes - -#### A. Input Validation Pass -Scan for functions that: -- Accept `dict`, `list`, or `Optional` parameters -- Access dict keys without None/type checks -- Have error messages that don't match validation logic - -#### B. Data Integrity Pass -Scan for methods that: -- Return internal data structures without deep copy -- Store input data with shallow copy (`dict()` instead of `deepcopy()`) -- Expose mutable state - -#### C. Parity Pass -When `presets.py` or `extensions.py` is in scope: -1. List all methods in changed file -2. Find equivalent methods in the other file -3. Compare defensive patterns -4. Flag asymmetries - -#### D. Test Coverage Pass -For each defensive check added: -- Verify corresponding test exists -- Check edge cases (None, wrong type, empty, corrupted) - -### 4. Severity Assignment - -- **CRITICAL**: Missing check that will cause crash or data corruption -- **IMPORTANT**: Missing parity, inconsistent error messages, missing deep copy -- **SUGGESTION**: Could be more defensive but unlikely to cause issues - -## Output Format - -## Defensive Programming Review - -### Summary -- Files reviewed: N -- Issues found: N (X critical, Y important, Z suggestions) - -### Critical Issues - -| Location | Issue | Fix | -|----------|-------|-----| -| presets.py:320 | `restore()` uses shallow copy | Use `copy.deepcopy(metadata)` | - -### Important Issues - -| Location | Issue | Fix | -|----------|-------|-----| -| presets.py:320 | Error says "non-empty" but accepts `{}` | Change to "must be a dict" | - -### Parity Issues - -| Extension Location | Preset Location | Issue | -|--------------------|-----------------|-------| -| extensions.py:300 | presets.py:320 | Extension has validation, preset missing | - -### Suggestions - -(Lower priority improvements) - -### Test Coverage - -| Defensive Check | Test Exists? | Test Location | -|-----------------|--------------|---------------| -| restore() rejects None | Yes | test_presets.py:392 | - -## Next Actions - -Based on findings, suggest: -1. Specific code changes with line numbers -2. Tests to add -3. Files to check for parity From 162573b3012b2da3ef84914021fd4db187aed509 Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Wed, 18 Mar 2026 09:22:11 +0100 Subject: [PATCH 04/13] 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 --- src/specify_cli/__init__.py | 2 ++ src/specify_cli/extensions.py | 8 +++++++- tests/test_extensions.py | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index 83fa85808f..8afbe90eb0 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -2443,6 +2443,7 @@ def preset_enable( console.print(f"[green]✓[/green] Preset '{pack_id}' enabled") console.print("\nTemplates from this preset will now be included in resolution.") + console.print("[dim]Note: Previously registered commands/skills remain active.[/dim]") @preset_app.command("disable") @@ -2483,6 +2484,7 @@ def preset_disable( console.print(f"[green]✓[/green] Preset '{pack_id}' disabled") console.print("\nTemplates from this preset will be skipped during resolution.") + console.print("[dim]Note: Previously registered commands/skills remain active until preset removal.[/dim]") console.print(f"To re-enable: specify preset enable {pack_id}") diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index b51aecbfb4..75bb3506a1 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -352,13 +352,16 @@ def is_installed(self, extension_id: str) -> bool: """ return extension_id in self.data["extensions"] - def list_by_priority(self) -> List[tuple]: + def list_by_priority(self, include_disabled: bool = False) -> 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. + Args: + include_disabled: If True, include disabled extensions. Default False. + Returns: List of (extension_id, metadata_copy) tuples sorted by priority. Metadata is deep-copied to prevent accidental mutation. @@ -370,6 +373,9 @@ def list_by_priority(self) -> List[tuple]: for ext_id, meta in extensions.items(): if not isinstance(meta, dict): continue + # Skip disabled extensions unless explicitly requested + if not include_disabled and not meta.get("enabled", True): + continue metadata_copy = copy.deepcopy(meta) metadata_copy["priority"] = normalize_priority(metadata_copy.get("priority", 10)) sortable_extensions.append((ext_id, metadata_copy)) diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 1efbea12a3..9eea75e0c3 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -2542,6 +2542,40 @@ def test_list_by_priority_invalid_priority_defaults(self, temp_dir): assert [item[0] for item in result] == ["ext-high", "ext-invalid"] assert result[1][1]["priority"] == 10 + def test_list_by_priority_excludes_disabled(self, temp_dir): + """Test that list_by_priority excludes disabled extensions by default.""" + extensions_dir = temp_dir / "extensions" + extensions_dir.mkdir() + + registry = ExtensionRegistry(extensions_dir) + registry.add("ext-enabled", {"version": "1.0.0", "enabled": True, "priority": 5}) + registry.add("ext-disabled", {"version": "1.0.0", "enabled": False, "priority": 1}) + registry.add("ext-default", {"version": "1.0.0", "priority": 10}) # no enabled field = True + + # Default: exclude disabled + by_priority = registry.list_by_priority() + ext_ids = [p[0] for p in by_priority] + assert "ext-enabled" in ext_ids + assert "ext-default" in ext_ids + assert "ext-disabled" not in ext_ids + + def test_list_by_priority_includes_disabled_when_requested(self, temp_dir): + """Test that list_by_priority includes disabled extensions when requested.""" + extensions_dir = temp_dir / "extensions" + extensions_dir.mkdir() + + registry = ExtensionRegistry(extensions_dir) + registry.add("ext-enabled", {"version": "1.0.0", "enabled": True, "priority": 5}) + registry.add("ext-disabled", {"version": "1.0.0", "enabled": False, "priority": 1}) + + # Include disabled + by_priority = registry.list_by_priority(include_disabled=True) + ext_ids = [p[0] for p in by_priority] + assert "ext-enabled" in ext_ids + assert "ext-disabled" in ext_ids + # Disabled ext has lower priority number, so it comes first when included + assert ext_ids[0] == "ext-disabled" + def test_install_with_priority(self, extension_dir, project_dir): """Test that install_from_directory stores priority.""" manager = ExtensionManager(project_dir) From 697823818622ae431da030d6c35cc28b9d6f23f0 Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Wed, 18 Mar 2026 12:11:57 +0100 Subject: [PATCH 05/13] 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 --- src/specify_cli/extensions.py | 7 +++-- src/specify_cli/presets.py | 18 ++++++++--- tests/test_extensions.py | 20 ++++++++++++ tests/test_presets.py | 57 +++++++++++++++++++++++++++++++++++ 4 files changed, 95 insertions(+), 7 deletions(-) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index 75bb3506a1..7a0effe50f 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -325,10 +325,13 @@ def get(self, extension_id: str) -> Optional[dict]: extension_id: Extension ID Returns: - Deep copy of extension metadata, or None if not found + Deep copy of extension metadata, or None if not found or corrupted """ entry = self.data["extensions"].get(extension_id) - return copy.deepcopy(entry) if entry is not None else None + # Return None for missing or corrupted (non-dict) entries + if entry is None or not isinstance(entry, dict): + return None + return copy.deepcopy(entry) def list(self) -> Dict[str, dict]: """Get all installed extensions. diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 52e0e6c555..78d82919b1 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -335,10 +335,13 @@ def get(self, pack_id: str) -> Optional[dict]: pack_id: Preset ID Returns: - Deep copy of preset metadata, or None if not found + Deep copy of preset metadata, or None if not found or corrupted """ entry = self.data["presets"].get(pack_id) - return copy.deepcopy(entry) if entry is not None else None + # Return None for missing or corrupted (non-dict) entries + if entry is None or not isinstance(entry, dict): + return None + return copy.deepcopy(entry) def list(self) -> Dict[str, dict]: """Get all installed presets. @@ -1498,12 +1501,17 @@ def _get_all_extensions_by_priority(self) -> list[tuple[int, str, dict | None]]: return [] registry = ExtensionRegistry(self.extensions_dir) - registered_extensions = registry.list_by_priority() - registered_extension_ids = {ext_id for ext_id, _ in registered_extensions} + # Get ALL registered extensions (including disabled) to know which dirs are tracked + all_registered = registry.list_by_priority(include_disabled=True) + registered_extension_ids = {ext_id for ext_id, _ in all_registered} all_extensions: list[tuple[int, str, dict | None]] = [] - for ext_id, metadata in registered_extensions: + # Only include enabled extensions in the result + for ext_id, metadata in all_registered: + # Skip disabled extensions + if not metadata.get("enabled", True): + continue priority = normalize_priority(metadata.get("priority") if metadata else None) all_extensions.append((priority, ext_id, metadata)) diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 9eea75e0c3..ddb5d94792 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -481,6 +481,26 @@ def test_get_returns_deep_copy(self, temp_dir): internal = registry.data["extensions"]["test-ext"] assert internal["registered_commands"] == {"claude": ["cmd1"]} + def test_get_returns_none_for_corrupted_entry(self, temp_dir): + """Test that get() returns None for corrupted (non-dict) entries.""" + extensions_dir = temp_dir / "extensions" + extensions_dir.mkdir() + + registry = ExtensionRegistry(extensions_dir) + + # Directly corrupt the registry with non-dict entries + registry.data["extensions"]["corrupted-string"] = "not a dict" + registry.data["extensions"]["corrupted-list"] = ["not", "a", "dict"] + registry.data["extensions"]["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 for nested structures.""" extensions_dir = temp_dir / "extensions" diff --git a/tests/test_presets.py b/tests/test_presets.py index becca0ca04..9688fb001d 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -450,6 +450,25 @@ def test_get_returns_deep_copy(self, temp_dir): 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" @@ -840,6 +859,44 @@ def test_resolve_extension_provided_templates(self, project_dir): 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 From 71dd58a07cd7061139818af4467db3fbc0a6559b Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Wed, 18 Mar 2026 20:32:33 +0100 Subject: [PATCH 06/13] 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 --- src/specify_cli/extensions.py | 7 +++++-- src/specify_cli/presets.py | 7 +++++-- tests/test_extensions.py | 14 ++++++++++++++ tests/test_presets.py | 14 ++++++++++++++ 4 files changed, 38 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index 7a0effe50f..c9243253a2 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -340,9 +340,12 @@ def list(self) -> Dict[str, dict]: from accidentally mutating nested internal registry state. Returns: - Dictionary of extension_id -> metadata (deep copies) + Dictionary of extension_id -> metadata (deep copies), empty dict if corrupted """ - return copy.deepcopy(self.data["extensions"]) + extensions = self.data.get("extensions", {}) or {} + if not isinstance(extensions, dict): + return {} + return copy.deepcopy(extensions) def is_installed(self, extension_id: str) -> bool: """Check if extension is installed. diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 78d82919b1..4c897674c9 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -350,9 +350,12 @@ def list(self) -> Dict[str, dict]: from accidentally mutating nested internal registry state. Returns: - Dictionary of pack_id -> metadata (deep copies) + Dictionary of pack_id -> metadata (deep copies), empty dict if corrupted """ - return copy.deepcopy(self.data["presets"]) + packs = self.data.get("presets", {}) or {} + if not isinstance(packs, dict): + return {} + return copy.deepcopy(packs) def list_by_priority(self, include_disabled: bool = False) -> List[tuple]: """Get all installed presets sorted by priority. diff --git a/tests/test_extensions.py b/tests/test_extensions.py index ddb5d94792..ef13e9410b 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -520,6 +520,20 @@ def test_list_returns_deep_copy(self, temp_dir): internal = registry.data["extensions"]["test-ext"] assert internal["registered_commands"] == {"claude": ["cmd1"]} + def test_list_returns_empty_dict_for_corrupted_registry(self, temp_dir): + """Test that list() returns empty dict when extensions is not a dict.""" + extensions_dir = temp_dir / "extensions" + extensions_dir.mkdir() + registry = ExtensionRegistry(extensions_dir) + + # Corrupt the registry - extensions is a list instead of dict + registry.data["extensions"] = ["not", "a", "dict"] + registry._save() + + # list() should return empty dict, not crash + result = registry.list() + assert result == {} + # ===== ExtensionManager Tests ===== diff --git a/tests/test_presets.py b/tests/test_presets.py index 9688fb001d..8ac2fe9724 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -487,6 +487,20 @@ def test_list_returns_deep_copy(self, temp_dir): 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" From d7c14c5819cd66f6897b63b3386ba967862add82 Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Wed, 18 Mar 2026 21:34:12 +0100 Subject: [PATCH 07/13] 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 --- src/specify_cli/extensions.py | 8 +++++++- src/specify_cli/presets.py | 8 +++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index c9243253a2..d561c11c00 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -302,6 +302,9 @@ def restore(self, extension_id: str, metadata: dict): """ if metadata is None or not isinstance(metadata, dict): raise ValueError(f"Cannot restore '{extension_id}': metadata must be a dict") + # Ensure extensions dict exists (handle corrupted registry) + if not isinstance(self.data.get("extensions"), dict): + self.data["extensions"] = {} self.data["extensions"][extension_id] = copy.deepcopy(metadata) self._save() @@ -327,7 +330,10 @@ def get(self, extension_id: str) -> Optional[dict]: Returns: Deep copy of extension metadata, or None if not found or corrupted """ - entry = self.data["extensions"].get(extension_id) + extensions = self.data.get("extensions") + if not isinstance(extensions, dict): + return None + entry = extensions.get(extension_id) # Return None for missing or corrupted (non-dict) entries if entry is None or not isinstance(entry, dict): return None diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 4c897674c9..23f79e34bb 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -322,6 +322,9 @@ def restore(self, pack_id: str, metadata: dict): """ if metadata is None or not isinstance(metadata, dict): raise ValueError(f"Cannot restore '{pack_id}': metadata must be a dict") + # Ensure presets dict exists (handle corrupted registry) + if not isinstance(self.data.get("presets"), dict): + self.data["presets"] = {} self.data["presets"][pack_id] = copy.deepcopy(metadata) self._save() @@ -337,7 +340,10 @@ def get(self, pack_id: str) -> Optional[dict]: Returns: Deep copy of preset metadata, or None if not found or corrupted """ - entry = self.data["presets"].get(pack_id) + packs = self.data.get("presets") + if not isinstance(packs, dict): + return None + entry = packs.get(pack_id) # Return None for missing or corrupted (non-dict) entries if entry is None or not isinstance(entry, dict): return None From 90a0feafce9d675748c4d9ca4cbcbe2db93a755f Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Thu, 19 Mar 2026 07:07:58 +0100 Subject: [PATCH 08/13] 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 --- src/specify_cli/extensions.py | 16 +++++++++++++--- src/specify_cli/presets.py | 16 +++++++++++++--- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index d561c11c00..27b6e1bfb1 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -222,7 +222,14 @@ def _load(self) -> dict: try: with open(self.registry_path, 'r') as f: - return json.load(f) + data = json.load(f) + # Validate loaded data is a dict (handles corrupted registry files) + if not isinstance(data, dict): + return { + "schema_version": self.SCHEMA_VERSION, + "extensions": {} + } + return data except (json.JSONDecodeError, FileNotFoundError): # Corrupted or missing registry, start fresh return { @@ -360,9 +367,12 @@ def is_installed(self, extension_id: str) -> bool: extension_id: Extension ID Returns: - True if extension is installed + True if extension is installed, False if not or registry corrupted """ - return extension_id in self.data["extensions"] + extensions = self.data.get("extensions") + if not isinstance(extensions, dict): + return False + return extension_id in extensions def list_by_priority(self, include_disabled: bool = False) -> List[tuple]: """Get all installed extensions sorted by priority. diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 23f79e34bb..57f8a3d69a 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -238,7 +238,14 @@ def _load(self) -> dict: try: with open(self.registry_path, 'r') as f: - return json.load(f) + data = json.load(f) + # Validate loaded data is a dict (handles corrupted registry files) + if not isinstance(data, dict): + return { + "schema_version": self.SCHEMA_VERSION, + "presets": {} + } + return data except (json.JSONDecodeError, FileNotFoundError): return { "schema_version": self.SCHEMA_VERSION, @@ -402,9 +409,12 @@ def is_installed(self, pack_id: str) -> bool: pack_id: Preset ID Returns: - True if pack is installed + True if pack is installed, False if not or registry corrupted """ - return pack_id in self.data["presets"] + packs = self.data.get("presets") + if not isinstance(packs, dict): + return False + return pack_id in packs class PresetManager: From 340edb0cd314ab42a2f5278ac0a8504043c60073 Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Thu, 19 Mar 2026 07:13:58 +0100 Subject: [PATCH 09/13] 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 --- src/specify_cli/extensions.py | 3 +++ src/specify_cli/presets.py | 3 +++ 2 files changed, 6 insertions(+) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index 27b6e1bfb1..189fe3cf8b 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -229,6 +229,9 @@ def _load(self) -> dict: "schema_version": self.SCHEMA_VERSION, "extensions": {} } + # Normalize extensions field (handles corrupted extensions value) + if not isinstance(data.get("extensions"), dict): + data["extensions"] = {} return data except (json.JSONDecodeError, FileNotFoundError): # Corrupted or missing registry, start fresh diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 57f8a3d69a..0e0c317ee0 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -245,6 +245,9 @@ def _load(self) -> dict: "schema_version": self.SCHEMA_VERSION, "presets": {} } + # Normalize presets field (handles corrupted presets value) + if not isinstance(data.get("presets"), dict): + data["presets"] = {} return data except (json.JSONDecodeError, FileNotFoundError): return { From e91f614447bf1d2f2add245bbd69692f3e27770e Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Thu, 19 Mar 2026 07:47:48 +0100 Subject: [PATCH 10/13] 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 --- src/specify_cli/presets.py | 7 +++++-- tests/test_extensions.py | 17 +++++++++-------- tests/test_presets.py | 2 +- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 0e0c317ee0..9d15a46929 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -1523,9 +1523,12 @@ def _get_all_extensions_by_priority(self) -> list[tuple[int, str, dict | None]]: return [] registry = ExtensionRegistry(self.extensions_dir) - # Get ALL registered extensions (including disabled) to know which dirs are tracked + # Use raw registry keys to track ALL extensions (including corrupted entries) + # This prevents corrupted entries from being picked up as "unregistered" dirs + registered_extension_ids = set(registry.list().keys()) + + # Get enabled extensions for resolution (list_by_priority skips corrupted/disabled) all_registered = registry.list_by_priority(include_disabled=True) - registered_extension_ids = {ext_id for ext_id, _ in all_registered} all_extensions: list[tuple[int, str, dict | None]] = [] diff --git a/tests/test_extensions.py b/tests/test_extensions.py index ef13e9410b..d99295ea80 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -2651,8 +2651,8 @@ 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.""" + def test_corrupted_extension_entry_not_picked_up_as_unregistered(self, project_dir): + """Corrupted registry entries are still tracked and NOT picked up as unregistered.""" extensions_dir = project_dir / ".specify" / "extensions" valid_dir = extensions_dir / "valid-ext" / "templates" @@ -2665,20 +2665,21 @@ def test_resolve_uses_unregistered_extension_dirs_when_registry_partially_corrup registry = ExtensionRegistry(extensions_dir) registry.add("valid-ext", {"version": "1.0.0", "priority": 10}) + # Corrupt the entry - should still be tracked, not picked up as unregistered registry.data["extensions"]["broken-ext"] = "corrupted" registry._save() from specify_cli.presets import PresetResolver resolver = PresetResolver(project_dir) + # Corrupted extension templates should NOT be resolved resolved = resolver.resolve("target-template") - sourced = resolver.resolve_with_source("target-template") + assert resolved is None - 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)" + # Valid extension template should still resolve + valid_resolved = resolver.resolve("other-template") + assert valid_resolved is not None + assert "Valid" in valid_resolved.read_text() class TestExtensionPriorityCLI: diff --git a/tests/test_presets.py b/tests/test_presets.py index 8ac2fe9724..2716b73dc7 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -2350,7 +2350,7 @@ def test_disabled_preset_excluded_from_resolution(self, project_dir, pack_dir): # Template should NOT be found when disabled resolver2 = PresetResolver(project_dir) result2 = resolver2.resolve("test-template", "template") - assert result2 is None or "test-pack" not in str(result2) + assert result2 is None def test_enable_corrupted_registry_entry(self, project_dir, pack_dir): """Test enable fails gracefully for corrupted registry entry.""" From c0d7c2e5e5cb94a36aa53ea71e6d2a6bebca2b87 Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Thu, 19 Mar 2026 08:07:16 +0100 Subject: [PATCH 11/13] 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 --- src/specify_cli/extensions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index 189fe3cf8b..c0aa50d8d5 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -669,7 +669,7 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: # Get registered commands before removal metadata = self.registry.get(extension_id) - registered_commands = metadata.get("registered_commands", {}) + registered_commands = metadata.get("registered_commands", {}) if metadata else {} extension_dir = self.extensions_dir / extension_id From 9001975f610ce53c1cca2d9e418b63c783e9509e Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Thu, 19 Mar 2026 08:54:54 +0100 Subject: [PATCH 12/13] 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 --- src/specify_cli/extensions.py | 27 +++++++++++++++++++++++---- src/specify_cli/presets.py | 33 ++++++++++++++++++++++++++------- 2 files changed, 49 insertions(+), 11 deletions(-) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index c0aa50d8d5..ff4fda1c15 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -350,10 +350,10 @@ def get(self, extension_id: str) -> Optional[dict]: return copy.deepcopy(entry) def list(self) -> Dict[str, dict]: - """Get all installed extensions. + """Get all installed extensions with valid metadata. - Returns a deep copy of the extensions mapping to prevent callers - from accidentally mutating nested internal registry state. + Returns a deep copy of extensions with dict metadata only. + Corrupted entries (non-dict values) are filtered out. Returns: Dictionary of extension_id -> metadata (deep copies), empty dict if corrupted @@ -361,7 +361,26 @@ def list(self) -> Dict[str, dict]: extensions = self.data.get("extensions", {}) or {} if not isinstance(extensions, dict): return {} - return copy.deepcopy(extensions) + # Filter to only valid dict entries to match type contract + return { + ext_id: copy.deepcopy(meta) + for ext_id, meta in extensions.items() + if isinstance(meta, dict) + } + + def keys(self) -> set: + """Get all extension IDs including corrupted entries. + + Lightweight method that returns IDs without deep-copying metadata. + Use this when you only need to check which extensions are tracked. + + Returns: + Set of extension IDs (includes corrupted entries) + """ + extensions = self.data.get("extensions", {}) or {} + if not isinstance(extensions, dict): + return set() + return set(extensions.keys()) def is_installed(self, extension_id: str) -> bool: """Check if extension is installed. diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 9d15a46929..c6fe1c64bf 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -360,10 +360,10 @@ def get(self, pack_id: str) -> Optional[dict]: return copy.deepcopy(entry) def list(self) -> Dict[str, dict]: - """Get all installed presets. + """Get all installed presets with valid metadata. - Returns a deep copy of the presets mapping to prevent callers - from accidentally mutating nested internal registry state. + Returns a deep copy of presets with dict metadata only. + Corrupted entries (non-dict values) are filtered out. Returns: Dictionary of pack_id -> metadata (deep copies), empty dict if corrupted @@ -371,7 +371,26 @@ def list(self) -> Dict[str, dict]: packs = self.data.get("presets", {}) or {} if not isinstance(packs, dict): return {} - return copy.deepcopy(packs) + # Filter to only valid dict entries to match type contract + return { + pack_id: copy.deepcopy(meta) + for pack_id, meta in packs.items() + if isinstance(meta, dict) + } + + def keys(self) -> set: + """Get all preset IDs including corrupted entries. + + Lightweight method that returns IDs without deep-copying metadata. + Use this when you only need to check which presets are tracked. + + Returns: + Set of preset IDs (includes corrupted entries) + """ + packs = self.data.get("presets", {}) or {} + if not isinstance(packs, dict): + return set() + return set(packs.keys()) def list_by_priority(self, include_disabled: bool = False) -> List[tuple]: """Get all installed presets sorted by priority. @@ -1523,11 +1542,11 @@ def _get_all_extensions_by_priority(self) -> list[tuple[int, str, dict | None]]: return [] registry = ExtensionRegistry(self.extensions_dir) - # Use raw registry keys to track ALL extensions (including corrupted entries) + # Use keys() to track ALL extensions (including corrupted entries) without deep copy # This prevents corrupted entries from being picked up as "unregistered" dirs - registered_extension_ids = set(registry.list().keys()) + registered_extension_ids = registry.keys() - # Get enabled extensions for resolution (list_by_priority skips corrupted/disabled) + # Get all registered extensions including disabled; we filter disabled manually below all_registered = registry.list_by_priority(include_disabled=True) all_extensions: list[tuple[int, str, dict | None]] = [] From 8c88c55767963721574fe6ee211f1595c181a85a Mon Sep 17 00:00:00 2001 From: iamaeroplane Date: Thu, 19 Mar 2026 09:14:48 +0100 Subject: [PATCH 13/13] 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 --- src/specify_cli/__init__.py | 6 ++---- src/specify_cli/extensions.py | 16 ++++++++++------ src/specify_cli/presets.py | 16 ++++++++++------ 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/src/specify_cli/__init__.py b/src/specify_cli/__init__.py index 8afbe90eb0..9bcf11b755 100644 --- a/src/specify_cli/__init__.py +++ b/src/specify_cli/__init__.py @@ -3924,8 +3924,7 @@ def extension_enable( console.print(f"[yellow]Extension '{display_name}' is already enabled[/yellow]") raise typer.Exit(0) - metadata["enabled"] = True - manager.registry.update(extension_id, metadata) + manager.registry.update(extension_id, {"enabled": True}) # Enable hooks in extensions.yml config = hook_executor.get_project_config() @@ -3972,8 +3971,7 @@ def extension_disable( console.print(f"[yellow]Extension '{display_name}' is already disabled[/yellow]") raise typer.Exit(0) - metadata["enabled"] = False - manager.registry.update(extension_id, metadata) + manager.registry.update(extension_id, {"enabled": False}) # Disable hooks in extensions.yml config = hook_executor.get_project_config() diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index ff4fda1c15..1e5125c904 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -254,7 +254,7 @@ def add(self, extension_id: str, metadata: dict): metadata: Extension metadata (version, source, etc.) """ self.data["extensions"][extension_id] = { - **metadata, + **copy.deepcopy(metadata), "installed_at": datetime.now(timezone.utc).isoformat() } self._save() @@ -277,10 +277,11 @@ def update(self, extension_id: str, metadata: dict): Raises: KeyError: If extension is not installed """ - if extension_id not in self.data["extensions"]: + extensions = self.data.get("extensions") + if not isinstance(extensions, dict) or extension_id not in extensions: raise KeyError(f"Extension '{extension_id}' is not installed") # Merge new metadata with existing, preserving original installed_at - existing = self.data["extensions"][extension_id] + existing = extensions[extension_id] # Handle corrupted registry entries (e.g., string/list instead of dict) if not isinstance(existing, dict): existing = {} @@ -293,7 +294,7 @@ def update(self, extension_id: str, metadata: dict): else: # If not present in existing, explicitly remove from merged if caller provided it merged.pop("installed_at", None) - self.data["extensions"][extension_id] = merged + extensions[extension_id] = merged self._save() def restore(self, extension_id: str, metadata: dict): @@ -324,8 +325,11 @@ def remove(self, extension_id: str): Args: extension_id: Extension ID """ - if extension_id in self.data["extensions"]: - del self.data["extensions"][extension_id] + extensions = self.data.get("extensions") + if not isinstance(extensions, dict): + return + if extension_id in extensions: + del extensions[extension_id] self._save() def get(self, extension_id: str) -> Optional[dict]: diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index c6fe1c64bf..4274ca5dab 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -269,7 +269,7 @@ def add(self, pack_id: str, metadata: dict): metadata: Pack metadata (version, source, etc.) """ self.data["presets"][pack_id] = { - **metadata, + **copy.deepcopy(metadata), "installed_at": datetime.now(timezone.utc).isoformat() } self._save() @@ -280,8 +280,11 @@ def remove(self, pack_id: str): Args: pack_id: Preset ID """ - if pack_id in self.data["presets"]: - del self.data["presets"][pack_id] + packs = self.data.get("presets") + if not isinstance(packs, dict): + return + if pack_id in packs: + del packs[pack_id] self._save() def update(self, pack_id: str, updates: dict): @@ -298,9 +301,10 @@ def update(self, pack_id: str, updates: dict): Raises: KeyError: If preset is not installed """ - if pack_id not in self.data["presets"]: + packs = self.data.get("presets") + if not isinstance(packs, dict) or pack_id not in packs: raise KeyError(f"Preset '{pack_id}' not found in registry") - existing = self.data["presets"][pack_id] + existing = packs[pack_id] # Handle corrupted registry entries (e.g., string/list instead of dict) if not isinstance(existing, dict): existing = {} @@ -313,7 +317,7 @@ def update(self, pack_id: str, updates: dict): 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 + packs[pack_id] = merged self._save() def restore(self, pack_id: str, metadata: dict):