fix: address PR review feedback

- Replace empty except with cache cleanup in _fetch_single_catalog
- Log teardown failure warning instead of silent pass in upgrade
- Validate catalog_data and integrations are dicts before use
- Catch OSError/UnicodeError in IntegrationDescriptor._load
- Add isinstance checks for integration/requires/provides/commands
- Enforce semver (X.Y.Z) instead of PEP 440 for descriptor versions
- Fix docstring and CONTRIBUTING.md to match actual block-on-modified behavior
- Restore old manifest on upgrade failure for transactional safety

Author: mnriem

integrations/CONTRIBUTING.md

@@ -123,8 +123,8 @@ The `specify integration upgrade` command supports diff-aware upgrades:
 
 1. **Hash comparison** — the manifest records SHA-256 hashes of all installed files
 2. **Modified file detection** — files changed since installation are flagged
-3. **Safe default** — modified files are preserved unless `--force` is used
-4. **Clean reinstall** — unmodified files are replaced with the latest version
+3. **Safe default** — the upgrade blocks if any installed files were modified since installation
+4. **Forced reinstall** — passing `--force` overwrites modified files with the latest version
 
 ```bash
 # Upgrade current integration (blocks if files are modified)

src/specify_cli/__init__.py

@@ -2229,7 +2229,7 @@ def integration_upgrade(
     """Upgrade an integration by reinstalling with diff-aware file handling.
 
     Compares manifest hashes to detect locally modified files and
-    preserves them unless --force is used.
+    blocks the upgrade unless --force is used.
     """
     from .integrations import get_integration
     from .integrations.manifest import IntegrationManifest
@@ -2315,8 +2315,16 @@ def integration_upgrade(
     except Exception as exc:
         try:
             integration.teardown(project_root, new_manifest, force=True)
+        except Exception as teardown_exc:
+            console.print(
+                f"[yellow]Warning:[/yellow] Teardown during rollback also failed: {teardown_exc}"
+            )
+        # Attempt to restore the old manifest so the project is not left broken
+        try:
+            old_manifest.save()
+            _write_integration_json(project_root, key, selected_script)
         except Exception:
-            pass
+            pass  # Best-effort restoration; original error is more important
         console.print(f"[red]Error:[/red] Failed to upgrade integration: {exc}")
         raise typer.Exit(1)
 

src/specify_cli/integrations/catalog.py

@@ -19,7 +19,6 @@
 from typing import Any, Dict, List, Optional
 
 import yaml
-from packaging import version as pkg_version
 
 
 # ---------------------------------------------------------------------------
@@ -244,19 +243,29 @@ def _fetch_single_catalog(
                 if age < self.CACHE_DURATION:
                     return json.loads(cache_file.read_text())
             except (json.JSONDecodeError, ValueError, KeyError, TypeError):
-                pass
+                # Cache is invalid or stale metadata; delete and refetch from source.
+                cache_file.unlink(missing_ok=True)
+                cache_meta.unlink(missing_ok=True)
 
         try:
             with urllib.request.urlopen(entry.url, timeout=10) as resp:
                 catalog_data = json.loads(resp.read())
 
+            if not isinstance(catalog_data, dict):
+                raise IntegrationCatalogError(
+                    f"Invalid catalog format from {entry.url}: expected a JSON object"
+                )
             if (
                 "schema_version" not in catalog_data
                 or "integrations" not in catalog_data
             ):
                 raise IntegrationCatalogError(
                     f"Invalid catalog format from {entry.url}"
                 )
+            if not isinstance(catalog_data.get("integrations"), dict):
+                raise IntegrationCatalogError(
+                    f"Invalid catalog format from {entry.url}: 'integrations' must be a JSON object"
+                )
 
             self.cache_dir.mkdir(parents=True, exist_ok=True)
             cache_file.write_text(json.dumps(catalog_data, indent=2))
@@ -413,6 +422,10 @@ def _load(path: Path) -> dict:
             raise IntegrationDescriptorError(f"Invalid YAML in {path}: {exc}")
         except FileNotFoundError:
             raise IntegrationDescriptorError(f"Descriptor not found: {path}")
+        except (OSError, UnicodeError) as exc:
+            raise IntegrationDescriptorError(
+                f"Unable to read descriptor {path}: {exc}"
+            )
 
     # -- Validation -------------------------------------------------------
 
@@ -430,6 +443,10 @@ def _validate(self) -> None:
             )
 
         integ = self.data["integration"]
+        if not isinstance(integ, dict):
+            raise IntegrationDescriptorError(
+                "'integration' must be a mapping"
+            )
         for field in ("id", "name", "version", "description"):
             if field not in integ:
                 raise IntegrationDescriptorError(
@@ -442,20 +459,26 @@ def _validate(self) -> None:
                 "must be lowercase alphanumeric with hyphens only"
             )
 
-        try:
-            pkg_version.Version(integ["version"])
-        except pkg_version.InvalidVersion:
+        if not re.match(r"^\d+\.\d+\.\d+", integ["version"]):
             raise IntegrationDescriptorError(
-                f"Invalid version: {integ['version']}"
+                f"Invalid version '{integ['version']}': must use semantic versioning (e.g., 1.0.0)"
             )
 
         requires = self.data["requires"]
+        if not isinstance(requires, dict):
+            raise IntegrationDescriptorError(
+                "'requires' must be a mapping"
+            )
         if "speckit_version" not in requires:
             raise IntegrationDescriptorError(
                 "Missing requires.speckit_version"
             )
 
         provides = self.data["provides"]
+        if not isinstance(provides, dict):
+            raise IntegrationDescriptorError(
+                "'provides' must be a mapping"
+            )
         commands = provides.get("commands", [])
         scripts = provides.get("scripts", [])
         if "commands" in provides and not isinstance(commands, list):
@@ -471,6 +494,10 @@ def _validate(self) -> None:
                 "Integration must provide at least one command or script"
             )
         for cmd in commands:
+            if not isinstance(cmd, dict):
+                raise IntegrationDescriptorError(
+                    "Each command entry must be a mapping"
+                )
             if "name" not in cmd or "file" not in cmd:
                 raise IntegrationDescriptorError(
                     "Command entry missing 'name' or 'file'"