fix(extensions): address fourth round of PR review comments

Rollback fixes:
- Preserve installed_at timestamp after successful update (was reset by
  install_from_zip calling registry.add)
- Fix rollback to only delete extension_dir if backup exists (avoids
  destroying valid installation when failure happens before modification)
- Fix rollback to remove NEW command files created by failed install
  (files that weren't in original backup are now cleaned up)
- Fix rollback to delete hooks key entirely when backup_hooks is None
  (original config had no hooks key, so restore should remove it)

Cross-command consistency fix:
- Add display name resolution to `extension add` command using
  _resolve_catalog_extension() helper (was only in `extension info`)
- Use resolved extension ID for download_extension() call, not original
  argument which may be a display name

Security fix (fail-closed):
- Malformed catalog config (empty/missing URLs) now raises ValidationError
  instead of silently falling back to built-in catalogs

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

Author: iamaeroplane

src/specify_cli/__init__.py

@@ -2219,8 +2219,11 @@ def extension_add(
                 # Install from catalog
                 catalog = ExtensionCatalog(project_root)
 
-                # Check if extension exists in catalog
-                ext_info = catalog.get_extension_info(extension)
+                # Check if extension exists in catalog (supports both ID and display name)
+                ext_info, catalog_error = _resolve_catalog_extension(extension, catalog, "add")
+                if catalog_error:
+                    console.print(f"[red]Error:[/red] Could not query extension catalog: {catalog_error}")
+                    raise typer.Exit(1)
                 if not ext_info:
                     console.print(f"[red]Error:[/red] Extension '{extension}' not found in catalog")
                     console.print("\nSearch available extensions:")
@@ -2240,9 +2243,10 @@ def extension_add(
                     )
                     raise typer.Exit(1)
 
-                # Download extension ZIP
+                # Download extension ZIP (use resolved ID, not original argument which may be display name)
+                extension_id = ext_info['id']
                 console.print(f"Downloading {ext_info['name']} v{ext_info.get('version', 'unknown')}...")
-                zip_path = catalog.download_extension(extension)
+                zip_path = catalog.download_extension(extension_id)
 
                 try:
                     # Install from downloaded ZIP
@@ -2797,21 +2801,29 @@ def extension_update(
                     # 8. Install new version
                     _ = 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):
+                    # 9. Restore metadata from backup (installed_at, enabled state)
+                    if backup_registry_entry:
                         new_metadata = manager.registry.get(extension_id)
-                        new_metadata["enabled"] = False
+
+                        # Preserve the original installation timestamp
+                        if "installed_at" in backup_registry_entry:
+                            new_metadata["installed_at"] = backup_registry_entry["installed_at"]
+
+                        # If extension was disabled before update, disable it again
+                        if not backup_registry_entry.get("enabled", True):
+                            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)
+                        # Also disable hooks in extensions.yml if extension was disabled
+                        if not backup_registry_entry.get("enabled", True):
+                            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():
@@ -2835,16 +2847,41 @@ def extension_update(
 
                 try:
                     # Restore extension directory
+                    # Only perform destructive rollback if backup exists (meaning we
+                    # actually modified the extension). This avoids deleting a valid
+                    # installation when failure happened before changes were made.
                     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():
+                        if extension_dir.exists():
+                            shutil.rmtree(extension_dir)
                         shutil.copytree(backup_ext_dir, extension_dir)
 
-                    # Restore command files
+                    # Remove any NEW command files created by failed install
+                    # (files that weren't in the original backup)
+                    try:
+                        new_registry_entry = manager.registry.get(extension_id)
+                        new_registered_commands = new_registry_entry.get("registered_commands", {})
+                        for agent_name, cmd_names in new_registered_commands.items():
+                            if agent_name not in registrar.AGENT_CONFIGS:
+                                continue
+                            agent_config = registrar.AGENT_CONFIGS[agent_name]
+                            commands_dir = project_root / agent_config["dir"]
+
+                            for cmd_name in cmd_names:
+                                cmd_file = commands_dir / f"{cmd_name}{agent_config['extension']}"
+                                # Delete if it exists and wasn't in our backup
+                                if cmd_file.exists() and str(cmd_file) not in backed_up_command_files:
+                                    cmd_file.unlink()
+
+                                # Also handle copilot prompt files
+                                if agent_name == "copilot":
+                                    prompt_file = project_root / ".github" / "prompts" / f"{cmd_name}.prompt.md"
+                                    if prompt_file.exists() and str(prompt_file) not in backed_up_command_files:
+                                        prompt_file.unlink()
+                    except KeyError:
+                        pass  # No new registry entry exists, nothing to clean up
+
+                    # Restore backed up command files
                     for original_path, backup_path in backed_up_command_files.items():
                         backup_file = Path(backup_path)
                         if backup_file.exists():
@@ -2857,24 +2894,30 @@ def extension_update(
                     # - backup_hooks={} or {...} means config had hooks key
                     config = hook_executor.get_project_config()
                     if "hooks" in config:
-                        # Remove any hooks for this extension added by failed install
                         modified = False
-                        for hook_name, hooks_list in config["hooks"].items():
-                            original_len = len(hooks_list)
-                            config["hooks"][hook_name] = [
-                                h for h in hooks_list
-                                if h.get("extension") != extension_id
-                            ]
-                            if len(config["hooks"][hook_name]) != original_len:
-                                modified = True
-
-                        # Add back the backed up hooks if any
-                        if backup_hooks:
-                            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)
-                                modified = True
+
+                        if backup_hooks is None:
+                            # Original config had no "hooks" key; remove it entirely
+                            del config["hooks"]
+                            modified = True
+                        else:
+                            # Remove any hooks for this extension added by failed install
+                            for hook_name, hooks_list in config["hooks"].items():
+                                original_len = len(hooks_list)
+                                config["hooks"][hook_name] = [
+                                    h for h in hooks_list
+                                    if h.get("extension") != extension_id
+                                ]
+                                if len(config["hooks"][hook_name]) != original_len:
+                                    modified = True
+
+                            # Add back the backed up hooks if any
+                            if backup_hooks:
+                                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)
+                                    modified = True
 
                         if modified:
                             hook_executor.save_project_config(config)

src/specify_cli/extensions.py

@@ -1149,12 +1149,13 @@ def _load_catalog_config(self, config_path: Path) -> Optional[List[CatalogEntry]
             config_path: Path to extension-catalogs.yml
 
         Returns:
-            Ordered list of CatalogEntry objects, or None if file doesn't exist
-            or contains no valid catalog entries.
+            Ordered list of CatalogEntry objects, or None if file doesn't exist.
 
         Raises:
             ValidationError: If any catalog entry has an invalid URL,
-                the file cannot be parsed, or a priority value is invalid.
+                the file cannot be parsed, a priority value is invalid,
+                or the file exists but contains no valid catalog entries
+                (fail-closed for security).
         """
         if not config_path.exists():
             return None
@@ -1166,19 +1167,25 @@ def _load_catalog_config(self, config_path: Path) -> Optional[List[CatalogEntry]
             )
         catalogs_data = data.get("catalogs", [])
         if not catalogs_data:
-            return None
+            # File exists but has no catalogs key or empty list - fail closed
+            raise ValidationError(
+                f"Catalog config {config_path} exists but contains no 'catalogs' entries. "
+                f"Remove the file to use built-in defaults, or add valid catalog entries."
+            )
         if not isinstance(catalogs_data, list):
             raise ValidationError(
                 f"Invalid catalog config: 'catalogs' must be a list, got {type(catalogs_data).__name__}"
             )
         entries: List[CatalogEntry] = []
+        skipped_entries: List[int] = []
         for idx, item in enumerate(catalogs_data):
             if not isinstance(item, dict):
                 raise ValidationError(
                     f"Invalid catalog entry at index {idx}: expected a mapping, got {type(item).__name__}"
                 )
             url = str(item.get("url", "")).strip()
             if not url:
+                skipped_entries.append(idx)
                 continue
             self._validate_catalog_url(url)
             try:
@@ -1201,7 +1208,14 @@ def _load_catalog_config(self, config_path: Path) -> Optional[List[CatalogEntry]
                 description=str(item.get("description", "")),
             ))
         entries.sort(key=lambda e: e.priority)
-        return entries if entries else None
+        if not entries:
+            # All entries were invalid (missing URLs) - fail closed for security
+            raise ValidationError(
+                f"Catalog config {config_path} contains {len(catalogs_data)} entries but none have valid URLs "
+                f"(entries at indices {skipped_entries} were skipped). "
+                f"Each catalog entry must have a 'url' field."
+            )
+        return entries
 
     def get_active_catalogs(self) -> List[CatalogEntry]:
         """Get the ordered list of active catalogs.

tests/test_extensions.py

@@ -1493,8 +1493,8 @@ def test_project_config_invalid_url_raises(self, temp_dir):
         with pytest.raises(ValidationError, match="HTTPS"):
             catalog.get_active_catalogs()
 
-    def test_empty_project_config_falls_back_to_defaults(self, temp_dir):
-        """Empty catalogs list in config falls back to default stack."""
+    def test_empty_project_config_raises_error(self, temp_dir):
+        """Empty catalogs list in config raises ValidationError (fail-closed for security)."""
         import yaml as yaml_module
 
         project_dir = self._make_project(temp_dir)
@@ -1503,11 +1503,32 @@ def test_empty_project_config_falls_back_to_defaults(self, temp_dir):
             yaml_module.dump({"catalogs": []}, f)
 
         catalog = ExtensionCatalog(project_dir)
-        entries = catalog.get_active_catalogs()
 
-        # Falls back to default stack
-        assert len(entries) == 2
-        assert entries[0].url == ExtensionCatalog.DEFAULT_CATALOG_URL
+        # Fail-closed: empty config should raise, not fall back to defaults
+        with pytest.raises(ValidationError) as exc_info:
+            catalog.get_active_catalogs()
+        assert "contains no 'catalogs' entries" in str(exc_info.value)
+
+    def test_catalog_entries_without_urls_raises_error(self, temp_dir):
+        """Catalog entries without URLs raise ValidationError (fail-closed for security)."""
+        import yaml as yaml_module
+
+        project_dir = self._make_project(temp_dir)
+        config_path = project_dir / ".specify" / "extension-catalogs.yml"
+        with open(config_path, "w") as f:
+            yaml_module.dump({
+                "catalogs": [
+                    {"name": "no-url-catalog", "priority": 1},
+                    {"name": "another-no-url", "description": "Also missing URL"},
+                ]
+            }, f)
+
+        catalog = ExtensionCatalog(project_dir)
+
+        # Fail-closed: entries without URLs should raise, not fall back to defaults
+        with pytest.raises(ValidationError) as exc_info:
+            catalog.get_active_catalogs()
+        assert "none have valid URLs" in str(exc_info.value)
 
     # --- _load_catalog_config ---
 
@@ -2034,3 +2055,58 @@ def test_extensionignore_negation_pattern(self, temp_dir, valid_manifest_data):
         assert not (dest / "docs" / "guide.md").exists()
         assert not (dest / "docs" / "internal.md").exists()
         assert (dest / "docs" / "api.md").exists()
+
+
+class TestExtensionAddCLI:
+    """CLI integration tests for extension add command."""
+
+    def test_add_by_display_name_uses_resolved_id_for_download(self, tmp_path):
+        """extension add by display name should use resolved ID for download_extension()."""
+        from typer.testing import CliRunner
+        from unittest.mock import patch, MagicMock
+        from specify_cli import app
+
+        runner = CliRunner()
+
+        # Create project structure
+        project_dir = tmp_path / "test-project"
+        project_dir.mkdir()
+        (project_dir / ".specify").mkdir()
+        (project_dir / ".specify" / "extensions").mkdir(parents=True)
+
+        # Mock catalog that returns extension by display name
+        mock_catalog = MagicMock()
+        mock_catalog.get_extension_info.return_value = None  # ID lookup fails
+        mock_catalog.search.return_value = [
+            {
+                "id": "acme-jira-integration",
+                "name": "Jira Integration",
+                "version": "1.0.0",
+                "description": "Jira integration extension",
+                "_install_allowed": True,
+            }
+        ]
+
+        # Track what ID was passed to download_extension
+        download_called_with = []
+        def mock_download(extension_id):
+            download_called_with.append(extension_id)
+            # Return a path that will fail install (we just want to verify the ID)
+            raise ExtensionError("Mock download - checking ID was resolved")
+
+        mock_catalog.download_extension.side_effect = mock_download
+
+        with patch("specify_cli.extensions.ExtensionCatalog", return_value=mock_catalog), \
+             patch.object(Path, "cwd", return_value=project_dir):
+            result = runner.invoke(
+                app,
+                ["extension", "add", "Jira Integration"],
+                catch_exceptions=True,
+            )
+
+        # Verify download_extension was called with the resolved ID, not the display name
+        assert len(download_called_with) == 1
+        assert download_called_with[0] == "acme-jira-integration", (
+            f"Expected download_extension to be called with resolved ID 'acme-jira-integration', "
+            f"but was called with '{download_called_with[0]}'"
+        )