fix(extensions): address reviewer feedback on update/rollback logic

- Hook rollback: handle empty backup_hooks by checking `is not None`
  instead of truthiness (falsy empty dict would skip hook cleanup)
- extension_info: use resolved_installed_id for catalog lookup when
  extension was found by display name (prevents wrong catalog match)
- Rollback: always remove extension dir first, then restore if backup
  exists (handles case when no original dir existed before update)
- Validate extension ID from ZIP before installing, not after
  (avoids side effects of installing wrong extension before rollback)
- Preserve enabled state during updates: re-apply disabled state and
  hook enabled flags after successful update
- Optimize _resolve_catalog_extension: pass query to catalog.search()
  instead of fetching all extensions
- update() now merges metadata with existing entry instead of replacing
  (preserves fields like registered_commands when only updating enabled)
- Add tests for ExtensionRegistry.update() and restore() methods:
  - test_update_preserves_installed_at
  - test_update_merges_with_existing
  - test_update_raises_for_missing_extension
  - test_restore_overwrites_completely
  - test_restore_can_recreate_removed_entry

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Author: iamaeroplane

src/specify_cli/__init__.py

@@ -1881,9 +1881,9 @@ def _resolve_catalog_extension(
         if ext_info:
             return (ext_info, None)
 
-        # Try by display name - search all extensions
-        all_extensions = catalog.search()
-        name_matches = [ext for ext in all_extensions if ext["name"].lower() == argument.lower()]
+        # Try by display name - search using argument as query, then filter for exact match
+        search_results = catalog.search(query=argument)
+        name_matches = [ext for ext in search_results if ext["name"].lower() == argument.lower()]
 
         if len(name_matches) == 1:
             return (name_matches[0], None)
@@ -2468,7 +2468,10 @@ def extension_info(
             raise typer.Exit(1)
 
     # Try catalog lookup (with error handling)
-    ext_info, catalog_error = _resolve_catalog_extension(extension, catalog, "info")
+    # If we resolved an installed extension by display name, use its ID for catalog lookup
+    # to ensure we get the correct catalog entry (not a different extension with same name)
+    lookup_key = resolved_installed_id if resolved_installed_id else extension
+    ext_info, catalog_error = _resolve_catalog_extension(lookup_key, catalog, "info")
 
     # Case 1: Found in catalog - show full catalog info
     if ext_info:
@@ -2729,7 +2732,7 @@ def extension_update(
 
             # Store backup state
             backup_registry_entry = None
-            backup_hooks = None
+            backup_hooks = {}  # Initialize to empty dict, not None
             backed_up_command_files = {}
 
             try:
@@ -2778,30 +2781,51 @@ def extension_update(
                         if ext_hooks:
                             backup_hooks[hook_name] = ext_hooks
 
-                # 5. Remove old extension (handles command file cleanup and registry removal)
-                manager.remove(extension_id, keep_config=True)
-
-                # 6. Download and install new version
+                # 5. Download new version
                 zip_path = catalog.download_extension(extension_id)
                 try:
-                    installed_manifest = manager.install_from_zip(zip_path, speckit_version)
-
-                    # 7. Verify extension ID matches
-                    if installed_manifest.id != extension_id:
-                        # Remove the wrongly installed extension before raising
+                    # 6. Validate extension ID from ZIP BEFORE modifying installation
+                    with zipfile.ZipFile(zip_path, "r") as zf:
                         try:
-                            manager.remove(installed_manifest.id)
-                        except Exception:
-                            pass  # Best effort cleanup
+                            with zf.open("extension.yml") as f:
+                                import yaml
+                                manifest_data = yaml.safe_load(f) or {}
+                        except KeyError:
+                            raise ValueError("Downloaded extension archive is missing 'extension.yml'")
+
+                    zip_extension_id = manifest_data.get("extension", {}).get("id")
+                    if zip_extension_id != extension_id:
                         raise ValueError(
-                            f"Extension ID mismatch: expected '{extension_id}', got '{installed_manifest.id}'"
+                            f"Extension ID mismatch: expected '{extension_id}', got '{zip_extension_id}'"
                         )
+
+                    # 7. Remove old extension (handles command file cleanup and registry removal)
+                    manager.remove(extension_id, keep_config=True)
+
+                    # 8. Install new version
+                    installed_manifest = manager.install_from_zip(zip_path, speckit_version)
+
+                    # 9. Restore enabled state from backup
+                    # If extension was disabled before update, disable it again
+                    if backup_registry_entry and not backup_registry_entry.get("enabled", True):
+                        new_metadata = manager.registry.get(extension_id)
+                        new_metadata["enabled"] = False
+                        manager.registry.update(extension_id, new_metadata)
+
+                        # Also disable hooks in extensions.yml to match
+                        config = hook_executor.get_project_config()
+                        if "hooks" in config:
+                            for hook_name in config["hooks"]:
+                                for hook in config["hooks"][hook_name]:
+                                    if hook.get("extension") == extension_id:
+                                        hook["enabled"] = False
+                            hook_executor.save_project_config(config)
                 finally:
                     # Clean up downloaded ZIP
                     if zip_path.exists():
                         zip_path.unlink()
 
-                # 8. Clean up backup on success
+                # 10. Clean up backup on success
                 if backup_base.exists():
                     shutil.rmtree(backup_base)
 
@@ -2819,10 +2843,13 @@ def extension_update(
 
                 try:
                     # Restore extension directory
+                    extension_dir = manager.extensions_dir / extension_id
+                    # Always remove any existing directory (from failed update)
+                    if extension_dir.exists():
+                        shutil.rmtree(extension_dir)
+                    # Restore from backup if it exists; otherwise leave absent
+                    # (matching pre-update state when there was no original dir)
                     if backup_ext_dir.exists():
-                        extension_dir = manager.extensions_dir / extension_id
-                        if extension_dir.exists():
-                            shutil.rmtree(extension_dir)
                         shutil.copytree(backup_ext_dir, extension_dir)
 
                     # Restore command files
@@ -2834,19 +2861,24 @@ def extension_update(
                             shutil.copy2(backup_file, original_file)
 
                     # Restore hooks in extensions.yml
-                    if backup_hooks:
+                    # Always remove any hooks for this extension (from failed install),
+                    # then restore backed-up hooks (even if empty)
+                    if backup_hooks is not None:
                         config = hook_executor.get_project_config()
                         if "hooks" not in config:
                             config["hooks"] = {}
-                        for hook_name, hooks in backup_hooks.items():
-                            if hook_name not in config["hooks"]:
-                                config["hooks"][hook_name] = []
-                            # Remove any existing hooks for this extension first
+
+                        # First remove any existing hooks for this extension across all hook groups
+                        for hook_name, hooks_list in config["hooks"].items():
                             config["hooks"][hook_name] = [
-                                h for h in config["hooks"][hook_name]
+                                h for h in hooks_list
                                 if h.get("extension") != extension_id
                             ]
-                            # Add back the backed up hooks
+
+                        # Then add back the backed up hooks (may be empty)
+                        for hook_name, hooks in backup_hooks.items():
+                            if hook_name not in config["hooks"]:
+                                config["hooks"][hook_name] = []
                             config["hooks"][hook_name].extend(hooks)
                         hook_executor.save_project_config(config)
 

src/specify_cli/extensions.py

@@ -229,27 +229,34 @@ def add(self, extension_id: str, metadata: dict):
         self._save()
 
     def update(self, extension_id: str, metadata: dict):
-        """Update extension metadata in registry, preserving installed_at.
+        """Update extension metadata in registry, merging with existing entry.
+
+        Merges the provided metadata with the existing entry, preserving any
+        fields not specified in the new metadata. The installed_at timestamp
+        is always preserved from the original entry.
 
         Use this method instead of add() when updating existing extension
         metadata (e.g., enabling/disabling) to preserve the original
-        installation timestamp.
+        installation timestamp and other existing fields.
 
         Args:
             extension_id: Extension ID
-            metadata: Extension metadata to update
+            metadata: Extension metadata fields to update (merged with existing)
 
         Raises:
             KeyError: If extension is not installed
         """
         if extension_id not in self.data["extensions"]:
             raise KeyError(f"Extension '{extension_id}' is not installed")
-        # Preserve the original installed_at timestamp
+        # Merge new metadata with existing, preserving original installed_at
         existing = self.data["extensions"][extension_id]
         original_installed_at = existing.get("installed_at")
-        self.data["extensions"][extension_id] = metadata
-        if original_installed_at and "installed_at" not in metadata:
-            self.data["extensions"][extension_id]["installed_at"] = original_installed_at
+        # Merge: existing fields preserved, new fields override
+        merged = {**existing, **metadata}
+        # Always preserve original installed_at
+        if original_installed_at:
+            merged["installed_at"] = original_installed_at
+        self.data["extensions"][extension_id] = merged
         self._save()
 
     def restore(self, extension_id: str, metadata: dict):

tests/test_extensions.py

@@ -277,6 +277,97 @@ def test_registry_persistence(self, temp_dir):
         assert registry2.is_installed("test-ext")
         assert registry2.get("test-ext")["version"] == "1.0.0"
 
+    def test_update_preserves_installed_at(self, temp_dir):
+        """Test that update() preserves the original installed_at timestamp."""
+        extensions_dir = temp_dir / "extensions"
+        extensions_dir.mkdir()
+
+        registry = ExtensionRegistry(extensions_dir)
+        registry.add("test-ext", {"version": "1.0.0", "enabled": True})
+
+        # Get original installed_at
+        original_data = registry.get("test-ext")
+        original_installed_at = original_data["installed_at"]
+
+        # Update with new metadata
+        registry.update("test-ext", {"version": "2.0.0", "enabled": False})
+
+        # Verify installed_at is preserved
+        updated_data = registry.get("test-ext")
+        assert updated_data["installed_at"] == original_installed_at
+        assert updated_data["version"] == "2.0.0"
+        assert updated_data["enabled"] is False
+
+    def test_update_merges_with_existing(self, temp_dir):
+        """Test that update() merges new metadata with existing fields."""
+        extensions_dir = temp_dir / "extensions"
+        extensions_dir.mkdir()
+
+        registry = ExtensionRegistry(extensions_dir)
+        registry.add("test-ext", {
+            "version": "1.0.0",
+            "enabled": True,
+            "registered_commands": {"claude": ["cmd1", "cmd2"]},
+        })
+
+        # Update with partial metadata (only enabled field)
+        registry.update("test-ext", {"enabled": False})
+
+        # Verify existing fields are preserved
+        updated_data = registry.get("test-ext")
+        assert updated_data["enabled"] is False
+        assert updated_data["version"] == "1.0.0"  # Preserved
+        assert updated_data["registered_commands"] == {"claude": ["cmd1", "cmd2"]}  # Preserved
+
+    def test_update_raises_for_missing_extension(self, temp_dir):
+        """Test that update() raises KeyError for non-installed extension."""
+        extensions_dir = temp_dir / "extensions"
+        extensions_dir.mkdir()
+
+        registry = ExtensionRegistry(extensions_dir)
+
+        with pytest.raises(KeyError, match="not installed"):
+            registry.update("nonexistent-ext", {"enabled": False})
+
+    def test_restore_overwrites_completely(self, temp_dir):
+        """Test that restore() overwrites the registry entry completely."""
+        extensions_dir = temp_dir / "extensions"
+        extensions_dir.mkdir()
+
+        registry = ExtensionRegistry(extensions_dir)
+        registry.add("test-ext", {"version": "2.0.0", "enabled": True})
+
+        # Restore with complete backup data
+        backup_data = {
+            "version": "1.0.0",
+            "enabled": False,
+            "installed_at": "2024-01-01T00:00:00+00:00",
+            "registered_commands": {"claude": ["old-cmd"]},
+        }
+        registry.restore("test-ext", backup_data)
+
+        # Verify entry is exactly as restored
+        restored_data = registry.get("test-ext")
+        assert restored_data == backup_data
+
+    def test_restore_can_recreate_removed_entry(self, temp_dir):
+        """Test that restore() can recreate an entry after remove()."""
+        extensions_dir = temp_dir / "extensions"
+        extensions_dir.mkdir()
+
+        registry = ExtensionRegistry(extensions_dir)
+        registry.add("test-ext", {"version": "1.0.0"})
+
+        # Save backup and remove
+        backup = registry.get("test-ext").copy()
+        registry.remove("test-ext")
+        assert not registry.is_installed("test-ext")
+
+        # Restore should recreate the entry
+        registry.restore("test-ext", backup)
+        assert registry.is_installed("test-ext")
+        assert registry.get("test-ext")["version"] == "1.0.0"
+
 
 # ===== ExtensionManager Tests =====