fix(bob): fail closed when preset registry is unreadable (review #3415)

Address review 4744636079:

- _migrate_commands: the preset guard previously failed *open* — a
  registry read/parse error returned an empty "no presets" list, so a
  --force layout-changing upgrade could delete preset-overridden command
  files while their registry state was unknown. Read the registry file
  directly and raise _PresetRegistryUnreadableError on any read/parse
  failure or malformed structure, rejecting the migration before any
  mutation. A genuinely absent registry still returns [] (safe).

- bob: correct the is_skills_mode docstring — upgrade *does* run setup();
  disk detection is needed because legacy Bob 1.x installs never persisted
  a legacy_commands option, so the stored mode is unavailable.

- tests: add fail-closed E2E (corrupted registry rejected, valid-empty
  allowed) plus a unit test for _installed_presets_affecting_agent covering
  absent / corrupted / malformed / valid / affecting-agent cases.

Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf

Author: mnriem

src/specify_cli/integrations/_migrate_commands.py

@@ -1,8 +1,9 @@
 """specify integration switch / upgrade command handlers."""
 from __future__ import annotations
 
+import json
 import os
-from pathlib import PurePath
+from pathlib import Path, PurePath
 
 import typer
 
@@ -53,6 +54,16 @@ def _manifest_tracks_skill_layout(manifest) -> bool:
     return any(str(rel).endswith("/SKILL.md") for rel in manifest.files)
 
 
+class _PresetRegistryUnreadableError(Exception):
+    """Raised when an existing preset registry cannot be read or parsed.
+
+    Distinct from a *genuinely absent* registry (no presets installed): an
+    unreadable registry means we cannot verify whether preset overrides would
+    be orphaned by a layout change, so the migration must be rejected rather
+    than proceeding on a false "no presets" assumption.
+    """
+
+
 def _installed_presets_affecting_agent(project_root, agent_key: str) -> list[str]:
     """Return IDs of installed presets with artifacts registered for *agent_key*.
 
@@ -64,18 +75,36 @@ def _installed_presets_affecting_agent(project_root, agent_key: str) -> list[str
     Callers use this to detect the unsafe case and reject the migration rather
     than silently orphaning preset files / leaving stale registry entries.
 
-    Best-effort: any error resolving preset state yields an empty list so a
-    broken/absent preset registry never blocks an otherwise-valid upgrade.
+    Fails **closed**: a genuinely absent registry (no presets ever installed)
+    returns an empty list, but if the registry file exists and cannot be read
+    or parsed (e.g. a permission error or corruption) this raises
+    :class:`_PresetRegistryUnreadableError`.  Reporting "no presets" in that
+    case would let a ``--force`` layout-changing upgrade delete
+    preset-overridden files while their registry state can't be reconciled —
+    the exact inconsistency the guard exists to prevent.
     """
-    try:
-        from ..presets import PresetManager
+    from ..presets import PresetRegistry
 
-        presets = PresetManager(project_root).registry.list()
-    except Exception:
+    registry_path = (
+        Path(project_root) / ".specify" / "presets" / PresetRegistry.REGISTRY_FILE
+    )
+    # Genuinely absent registry → no presets installed → safe to proceed.
+    if not registry_path.exists():
         return []
 
+    # The registry exists: any failure to read or parse it must surface as an
+    # error, not be swallowed into an empty ("no presets") result.
+    try:
+        data = json.loads(registry_path.read_text(encoding="utf-8"))
+    except (OSError, ValueError) as exc:
+        raise _PresetRegistryUnreadableError(str(exc)) from exc
+    if not isinstance(data, dict) or not isinstance(data.get("presets", {}), dict):
+        raise _PresetRegistryUnreadableError(
+            "preset registry structure is malformed"
+        )
+
     affected: list[str] = []
-    for preset_id, meta in presets.items():
+    for preset_id, meta in data.get("presets", {}).items():
         if not isinstance(meta, dict):
             continue
         registered_commands = meta.get("registered_commands", {})
@@ -463,7 +492,21 @@ def integration_upgrade(
     if _manifest_tracks_skill_layout(old_manifest) != integration.is_skills_mode(
         parsed_options, project_root
     ):
-        affected_presets = _installed_presets_affecting_agent(project_root, key)
+        try:
+            affected_presets = _installed_presets_affecting_agent(project_root, key)
+        except _PresetRegistryUnreadableError as exc:
+            console.print(
+                f"[red]Error:[/red] Cannot change '{key}' command layout: the "
+                f"preset registry could not be read to verify installed presets."
+            )
+            console.print(f"[dim]Details:[/dim] {_cli_error_detail(exc)}")
+            console.print(
+                "A layout change cannot reconcile preset artifacts, so the "
+                "migration is refused while the preset registry state is "
+                "unknown. Fix or restore "
+                "[cyan].specify/presets/.registry[/cyan] and retry."
+            )
+            raise typer.Exit(1)
         if affected_presets:
             preset_list = ", ".join(sorted(affected_presets))
             console.print(

src/specify_cli/integrations/bob/__init__.py

@@ -196,15 +196,19 @@ def is_skills_mode(
         4. A fresh project (no managed artifacts, no flags) defaults to skills.
 
         The disk-detection fallback exists because on ``use`` / ``switch`` /
-        ``upgrade`` (without ``--skills``) no ``setup()`` runs and
-        *parsed_options* is typically empty — existing Bob 1.x installs never
-        stored ``legacy_commands``.  Defaulting to skills there would rewrite
-        such a project's ``ai_skills`` flag to ``True`` even though it still
-        only contains a command layout, silently switching its extension /
-        command-reference handling.  So the layout is inferred from managed
-        Spec Kit artifacts, not the mere presence of a ``.bob/skills/``
-        directory: a user may keep unrelated Bob 2 skills in ``.bob/skills/``
-        while their Spec Kit commands still live in
+        ``upgrade`` (without an explicit ``--skills`` / ``--legacy-commands``)
+        *parsed_options* is typically empty: no flag was passed, and existing
+        Bob 1.x installs never persisted a ``legacy_commands`` option to
+        recover.  This is independent of whether ``setup()`` runs — ``upgrade``
+        *does* call :meth:`setup` (see ``_migrate_commands.integration_upgrade``),
+        but it passes those same empty *parsed_options*, so without disk
+        detection the mode would resolve to the skills default.  Defaulting to
+        skills there would rewrite such a project's ``ai_skills`` flag to
+        ``True`` even though it still only contains a command layout, silently
+        switching its extension / command-reference handling.  So the layout is
+        inferred from managed Spec Kit artifacts, not the mere presence of a
+        ``.bob/skills/`` directory: a user may keep unrelated Bob 2 skills in
+        ``.bob/skills/`` while their Spec Kit commands still live in
         ``.bob/commands/speckit.*.md``.  We therefore treat the project as
         legacy (command) mode only when managed Spec Kit command files exist
         and no managed Spec Kit skills (``speckit-*`` skill dirs) do.  Passing

tests/integrations/test_integration_subcommand.py

@@ -2662,6 +2662,58 @@ def test_upgrade_bob_layout_change_rejected_with_presets_installed(self, tmp_pat
             f"same-layout upgrade must not be blocked by presets: {result.output}"
         )
 
+    def test_upgrade_bob_layout_change_rejected_when_preset_registry_unreadable(
+        self, tmp_path
+    ):
+        """Regression (review #3415, 4744636079).
+
+        The preset guard must fail *closed*: if the preset registry exists but
+        cannot be read/parsed (corruption, permissions), the layout-changing
+        upgrade must be rejected before any mutation rather than proceeding on
+        a false "no presets installed" assumption (which would let ``--force``
+        delete preset-overridden command files while their registry state is
+        unknown). A genuinely absent registry must still be allowed.
+        """
+        project = _init_project(
+            tmp_path, "bob", integration_options="--legacy-commands"
+        )
+        commands = project / ".bob" / "commands"
+        skills = project / ".bob" / "skills"
+        assert sorted(commands.glob("speckit.*.md"))
+
+        # Corrupted (unparseable) registry: exists but cannot be read as JSON.
+        presets_dir = project / ".specify" / "presets"
+        presets_dir.mkdir(parents=True, exist_ok=True)
+        (presets_dir / ".registry").write_text("{ not valid json", encoding="utf-8")
+
+        result = _run_in_project(project, [
+            "integration", "upgrade", "bob",
+            "--integration-options", "--skills",
+            "--script", "sh", "--force",
+        ])
+        assert result.exit_code != 0, (
+            "layout change must be rejected when preset registry is unreadable"
+        )
+        assert "preset registry" in result.output.lower()
+        assert not skills.exists(), "no skills layout may be scaffolded on rejection"
+        assert sorted(commands.glob("speckit.*.md")), (
+            "legacy command files must be untouched when failing closed"
+        )
+
+        # A valid, empty registry must NOT block the migration.
+        (presets_dir / ".registry").write_text(
+            json.dumps({"presets": {}}), encoding="utf-8"
+        )
+        result = _run_in_project(project, [
+            "integration", "upgrade", "bob",
+            "--integration-options", "--skills",
+            "--script", "sh", "--force",
+        ])
+        assert result.exit_code == 0, (
+            f"valid empty preset registry must not block migration: {result.output}"
+        )
+        assert skills.exists(), "skills layout should be scaffolded once unblocked"
+
     def test_upgrade_secondary_bob_layout_change_preserves_active_agent_skills(
         self, tmp_path
     ):
@@ -2858,6 +2910,59 @@ def test_upgrade_non_active_agent_preserves_active_agent_skills(self, tmp_path):
             "deleted extension skill (#2886)"
         )
 
+    def test_installed_presets_affecting_agent_absent_vs_unreadable(self, tmp_path):
+        """Unit (review #3415, 4744636079): fail closed only when unreadable.
+
+        The preset guard helper must return an empty list for a genuinely
+        absent registry, but raise ``_PresetRegistryUnreadableError`` when the
+        registry exists yet cannot be read/parsed — so a layout-changing
+        upgrade never proceeds on a false "no presets" result.
+        """
+        from specify_cli.integrations._migrate_commands import (
+            _PresetRegistryUnreadableError,
+            _installed_presets_affecting_agent,
+        )
+
+        project = tmp_path / "proj"
+        project.mkdir()
+
+        # Genuinely absent registry → empty list (safe to proceed).
+        assert _installed_presets_affecting_agent(project, "bob") == []
+
+        presets_dir = project / ".specify" / "presets"
+        presets_dir.mkdir(parents=True)
+        registry = presets_dir / ".registry"
+
+        # Corrupted JSON → unreadable → raise.
+        registry.write_text("{ not json", encoding="utf-8")
+        with pytest.raises(_PresetRegistryUnreadableError):
+            _installed_presets_affecting_agent(project, "bob")
+
+        # Malformed structure (presets not a dict) → unreadable → raise.
+        registry.write_text(json.dumps({"presets": []}), encoding="utf-8")
+        with pytest.raises(_PresetRegistryUnreadableError):
+            _installed_presets_affecting_agent(project, "bob")
+
+        # Valid, empty registry → empty list.
+        registry.write_text(json.dumps({"presets": {}}), encoding="utf-8")
+        assert _installed_presets_affecting_agent(project, "bob") == []
+
+        # Valid registry with a preset registered for bob → reported.
+        registry.write_text(
+            json.dumps({
+                "presets": {
+                    "p1": {"registered_commands": {"bob": ["speckit.plan"]}},
+                    "p2": {"registered_commands": {"codex": ["speckit.plan"]}},
+                    "p3": {"registered_skills": ["speckit-x"]},
+                }
+            }),
+            encoding="utf-8",
+        )
+        assert sorted(_installed_presets_affecting_agent(project, "bob")) == [
+            "p1",
+            "p3",
+        ]
+
 
 # ── Full lifecycle ───────────────────────────────────────────────────