feat(bob): add --skills migration opt-in; fix separator + manifest loss (review #3415)

Address review #3415 (4724160183):

- Comment 1: Add an explicit `--skills` opt-in to BobIntegration. It forces
  the skills layout over on-disk auto-detection, giving legacy Bob 1.x
  installs a supported migration path
  (`integration upgrade bob --integration-options="--skills"`). `--skills`
  and `--legacy-commands` are mutually exclusive (clean exit-1 error).

- Comment 2: In CommandRegistrar.register_commands, derive the command-ref
  separator from the output layout (agent_config["extension"]) for the
  active agent too, not the persisted ai_skills flag. A command-layout file
  (.bob/commands/*.md, .github/agents/*.agent.md) always renders /speckit.*;
  only a /SKILL.md scaffold uses /speckit-*. Dual-layout agents (Bob,
  Copilot) write skills via their own setup()/skills path, so
  register_commands only ever emits their command-layout files.

- Comment 3: Update docs/reference/integrations.md Bob entry to document the
  skills-based default (.bob/skills/), the deprecated --legacy-commands
  opt-out, and the --skills migration path.

Also fix a latent manifest-loss bug surfaced by the migration path: the
upgrade Phase 2 stale-file cleanup built a throwaway manifest sharing the
integration key and called uninstall(), which always deleted
{key}.manifest.json. Any layout-shrinking upgrade (e.g. legacy->skills)
thus wiped the freshly-saved manifest, leaving the project untracked and
un-upgradeable. uninstall() now takes remove_manifest (default True); the
stale-cleanup pass passes False.

Adds regression tests for the --skills opt-in, mutual exclusion, corrected
active-agent separator, remove_manifest=False, and an end-to-end
legacy->skills migration that verifies the manifest survives and the
project remains upgradeable. Full suite: 4555 passed, 5 skipped.

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

docs/reference/integrations.md

@@ -22,7 +22,7 @@ The Specify CLI supports a wide range of AI coding agents. When you run `specify
 | [Goose](https://goose-docs.ai/)                                                      | `goose`          | Uses YAML recipe format in `.goose/recipes/`                                                                                              |
 | [Grok Build](https://docs.x.ai/build/overview)                                       | `grok`           | Skills-based integration; installs skills into `.grok/skills` and invokes them as `/speckit-<command>`                                    |
 | [Hermes](https://github.com/NousResearch/hermes-agent)                               | `hermes`         | Skills-based integration; installs skills globally into `~/.hermes/skills/`                                                                |
-| [IBM Bob](https://www.ibm.com/products/bob)                                          | `bob`            | IDE-based agent                                                                                                                           |
+| [IBM Bob](https://www.ibm.com/products/bob)                                          | `bob`            | Skills-based integration by default; installs skills as `speckit-<command>/SKILL.md` under `.bob/skills/` and invokes them as `/speckit-<command>`. Pass `--integration-options="--legacy-commands"` to scaffold the deprecated Bob 1.x layout (`.bob/commands/*.md`) instead; that flag will be removed in a future release. Existing legacy installs can migrate with `specify integration upgrade bob --integration-options="--skills"`, which converts them to the skills layout and removes the old command files. |
 | [Junie](https://junie.jetbrains.com/)                                                | `junie`          |                                                                                                                                           |
 | [Kilo Code](https://github.com/Kilo-Org/kilocode)                                    | `kilocode`       |                                                                                                                                           |
 | [Kimi Code](https://code.kimi.com/)                                                  | `kimi`           | Skills-based integration; installs into `.kimi-code/skills/`. `--migrate-legacy` moves old `.kimi/skills/` installs to the new paths |

src/specify_cli/agents.py

@@ -637,46 +637,34 @@ def register_commands(
         is_cline_ext = agent_name == "cline" and source_id != "core"
         source_root = source_dir.resolve()
 
-        # Resolve the command-reference separator once for this agent/project.
-        # Dual-layout agents (e.g. Bob) use different separators for their
-        # skills vs command layouts, so we ask the integration to map the
-        # project's persisted skills state to the correct separator rather than
-        # relying on the single static AGENT_CONFIGS value.
+        # Resolve the command-reference separator for the file THIS registrar
+        # is about to write.  The separator must match the *output layout* the
+        # registrar produces for this agent — not the project's persisted
+        # ``ai_skills`` flag, and not unrelated sibling directories on disk.  A
+        # skill scaffold ("/SKILL.md") uses the skills separator; any
+        # command-layout output (".md", ".agent.md", ".toml", …) uses the
+        # command separator.
+        #
+        # This holds for the *active* agent too.  Dual-layout agents (Bob,
+        # Copilot) write their skills via their own setup()/skills path, so
+        # ``register_commands`` only ever emits their command-layout files.
+        # Deriving the separator from ``ai_skills`` would render such a
+        # ``.bob/commands/*.md`` (or ``.github/agents/*.agent.md``) file with
+        # ``/speckit-*`` whenever that agent is active in skills mode — even
+        # though a command-layout file must use ``/speckit.*``.  Deriving it
+        # from the agent's static output config avoids that mismatch and stays
+        # correct when a stale ``.bob/skills`` directory coexists with
+        # ``.bob/commands``.
         _sep = agent_config.get("invoke_separator", ".")
         try:
             from specify_cli.integrations import get_integration  # noqa: PLC0415
 
             _integ = get_integration(agent_name)
             if _integ is not None:
-                _opts = load_init_options(project_root)
-                # The persisted ``ai_skills`` flag describes only the active
-                # integration (``opts["ai"]``).  ``register_commands_for_all_agents``
-                # calls this for every detected agent, so trusting that flag for a
-                # different agent would, e.g., render a legacy ``.bob/commands``
-                # project's refs as ``/speckit-*`` just because Copilot is active in
-                # skills mode.  Only consult the flag for the agent it describes;
-                # otherwise resolve this agent's separator from its own project-aware
-                # detection.
-                if _opts.get("ai") == agent_name:
-                    _sep = _integ.invoke_separator_for_mode(
-                        is_ai_skills_enabled(_opts)
-                    )
-                else:
-                    # Inactive agent: the reference separator must match the
-                    # layout THIS registrar writes into — determined by the
-                    # agent's static output config (its command dir + file
-                    # extension), not by unrelated sibling directories on disk.
-                    # A skill-scaffold output ("/SKILL.md") uses the skills
-                    # separator; a command-layout output uses the command
-                    # separator. This avoids mislabeling a Bob command-layout
-                    # write as skills just because an unrelated .bob/skills
-                    # directory happens to exist.
-                    registrar_writes_skills = (
-                        agent_config.get("extension") == "/SKILL.md"
-                    )
-                    _sep = _integ.invoke_separator_for_mode(
-                        registrar_writes_skills
-                    )
+                registrar_writes_skills = (
+                    agent_config.get("extension") == "/SKILL.md"
+                )
+                _sep = _integ.invoke_separator_for_mode(registrar_writes_skills)
         except Exception:
             pass
 

src/specify_cli/integrations/_migrate_commands.py

@@ -496,7 +496,15 @@ def integration_upgrade(
     if stale_keys:
         stale_manifest = IntegrationManifest(key, project_root, version="stale-cleanup")
         stale_manifest._files = {k: old_files[k] for k in stale_keys}
-        stale_removed, _ = stale_manifest.uninstall(project_root, force=True)
+        # remove_manifest=False: this throwaway manifest shares ``key`` with the
+        # real one just saved above (new_manifest.save()).  Letting uninstall()
+        # delete ``{key}.manifest.json`` would wipe the freshly-written manifest
+        # whenever an upgrade shrinks the tracked file set (e.g. Bob migrating
+        # from the legacy commands layout to skills), leaving the integration
+        # untracked and un-upgradeable.
+        stale_removed, _ = stale_manifest.uninstall(
+            project_root, force=True, remove_manifest=False
+        )
         if stale_removed:
             console.print(f"  Removed {len(stale_removed)} stale file(s) from previous install")
 

src/specify_cli/integrations/bob/__init__.py

@@ -23,6 +23,8 @@
 from pathlib import Path
 from typing import Any
 
+import typer
+
 from ..base import (
     IntegrationBase,
     IntegrationOption,
@@ -32,6 +34,24 @@
 from ..manifest import IntegrationManifest
 
 
+def _validate_mode_options(parsed_options: dict[str, Any] | None) -> None:
+    """Reject ``--skills`` and ``--legacy-commands`` used together.
+
+    The two flags select opposite layouts, so combining them is ambiguous.
+    Fail fast with the same clean exit-1 UX as other bad-option paths rather
+    than silently letting one win.
+    """
+    opts = parsed_options or {}
+    if opts.get("skills") and opts.get("legacy_commands"):
+        from ..._console import console
+
+        console.print(
+            "[red]Error:[/red] --skills and --legacy-commands are mutually "
+            "exclusive; pass only one."
+        )
+        raise typer.Exit(1)
+
+
 def _warn_legacy_commands_deprecated() -> None:
     warnings.warn(
         "Bob legacy commands mode (.bob/commands/) is deprecated and will be "
@@ -132,6 +152,17 @@ class BobIntegration(IntegrationBase):
     @classmethod
     def options(cls) -> list[IntegrationOption]:
         return [
+            IntegrationOption(
+                "--skills",
+                is_flag=True,
+                default=False,
+                help=(
+                    "Force the default skills layout (.bob/skills/), overriding "
+                    "on-disk auto-detection. Use this to migrate a legacy "
+                    "commands install to skills, e.g. "
+                    "`integration upgrade bob --integration-options \"--skills\"`"
+                ),
+            ),
             IntegrationOption(
                 "--legacy-commands",
                 is_flag=True,
@@ -151,23 +182,39 @@ def is_skills_mode(
     ) -> bool:
         """Bob is skills-first; ``--legacy-commands`` opts out.
 
-        On ``use`` / ``switch`` / ``upgrade`` no ``setup()`` runs and
-        *parsed_options* is typically empty (existing Bob 1.x installs never
-        stored ``legacy_commands``).  Defaulting to skills there would rewrite
+        Precedence:
+
+        1. Explicit ``--skills`` wins — it *forces* skills mode regardless of
+           what is already on disk.  This is the supported migration / opt-in
+           path: ``integration upgrade bob --integration-options "--skills"``
+           converts a legacy commands install to the skills layout (setup()
+           scaffolds ``.bob/skills`` and the upgrade's stale-file pass removes
+           the old ``.bob/commands`` files).
+        2. Explicit ``--legacy-commands`` opts out to the Bob 1.x layout.
+        3. Otherwise, when a *project_root* is supplied, the layout is inferred
+           from **managed Spec Kit artifacts** (see below).
+        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 to the skills layout.
-
-        So when a *project_root* is supplied, the Spec Kit layout is inferred
-        from **managed Spec Kit artifacts**, not from 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
+        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.  A fresh
-        project (no managed artifacts yet) still defaults to skills.
+        and no managed Spec Kit skills (``speckit-*`` skill dirs) do.  Passing
+        ``--skills`` overrides this so users are never trapped in legacy mode.
         """
-        if (parsed_options or {}).get("legacy_commands", False):
+        opts = parsed_options or {}
+        _validate_mode_options(opts)
+        if opts.get("skills", False):
+            return True
+        if opts.get("legacy_commands", False):
             return False
         if project_root is not None:
             bob_dir = Path(project_root) / ".bob"

src/specify_cli/integrations/manifest.py

@@ -327,12 +327,18 @@ def uninstall(
         project_root: Path | None = None,
         *,
         force: bool = False,
+        remove_manifest: bool = True,
     ) -> tuple[list[Path], list[Path]]:
         """Remove tracked files whose hash still matches.
 
         Parameters:
-            project_root: Override for the project root.
-            force:        If ``True``, remove files even if modified.
+            project_root:    Override for the project root.
+            force:           If ``True``, remove files even if modified.
+            remove_manifest: If ``True`` (default), also delete this
+                integration's ``{key}.manifest.json``. Set ``False`` for
+                *partial* cleanups (e.g. the upgrade stale-file pass, which
+                builds a throwaway manifest over a subset of files) so the
+                real, freshly-saved manifest for the same key is not destroyed.
 
         Returns:
             ``(removed, skipped)`` — absolute paths.
@@ -393,7 +399,7 @@ def uninstall(
 
         # Remove the manifest file itself
         manifest = root / ".specify" / "integrations" / f"{self.key}.manifest.json"
-        if manifest.exists():
+        if remove_manifest and manifest.exists():
             manifest.unlink()
             parent = manifest.parent
             while parent != root: