fix(bob): detect Spec Kit layout from managed artifacts, not any skills dir (review #3415)

Two related mis-detections from review 4723246468:

1. `BobIntegration.is_skills_mode` treated the mere presence of a
   `.bob/skills/` directory as proof the project is skills-based. A legacy
   Spec Kit install (managed `.bob/commands/speckit.*.md`) that also carried
   unrelated Bob 2 skills would be misclassified as skills, so
   `integration use bob` persisted `ai_skills` and rewrote shared refs.
   Now the layout is inferred from managed Spec Kit artifacts: legacy/command
   mode only when managed `speckit.*.md` command files exist and no managed
   `speckit-*` skill dirs do.

2. The `register_commands` separator for an inactive agent used a disk-based
   `effective_invoke_separator(None, project_root)` fallback that could pick
   the skills separator even though the registrar writes the static command
   layout (`.bob/commands/*.md`). Inactive agents now resolve the separator
   from the registrar's actual output layout (`extension == "/SKILL.md"`),
   so command-layout files keep `/speckit.*` refs regardless of sibling dirs.

Update the affected hook/E2E tests to use managed artifacts and add
regression tests for the mixed-layout and inactive-registrar scenarios.

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/agents.py

@@ -662,7 +662,21 @@ def register_commands(
                         is_ai_skills_enabled(_opts)
                     )
                 else:
-                    _sep = _integ.effective_invoke_separator(None, project_root)
+                    # 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
+                    )
         except Exception:
             pass
 

src/specify_cli/integrations/bob/__init__.py

@@ -155,18 +155,25 @@ def is_skills_mode(
         *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 ``.bob/commands/`` — silently switching its extension /
-        command-reference handling to the skills layout.  So when a
-        *project_root* is supplied, an already-installed legacy layout
-        (``.bob/commands/`` present, ``.bob/skills/`` absent) is preserved
-        until an explicit upgrade actually creates ``.bob/skills/``.  A fresh
-        project (no ``.bob/`` layout yet) still defaults to skills.
+        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
+        ``.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.
         """
         if (parsed_options or {}).get("legacy_commands", False):
             return False
         if project_root is not None:
             bob_dir = Path(project_root) / ".bob"
-            if (bob_dir / "commands").is_dir() and not (bob_dir / "skills").is_dir():
+            has_managed_skills = any((bob_dir / "skills").glob("speckit-*"))
+            has_managed_commands = any((bob_dir / "commands").glob("speckit.*.md"))
+            if has_managed_commands and not has_managed_skills:
                 return False
         return True
 

tests/integrations/test_integration_bob.py

@@ -85,32 +85,63 @@ def test_legacy_commands_disables_skills(self):
         assert bob.is_skills_mode({"legacy_commands": True}) is False
 
     def test_existing_commands_layout_preserved_on_use(self, tmp_path):
-        """Regression (review #3415): an existing Bob 1.x project (only
-        ``.bob/commands/`` on disk, no stored ``legacy_commands``) must NOT be
-        treated as skills mode when re-resolved with a project_root, so
-        ``use``/``switch``/``upgrade`` never silently migrate it to skills.
+        """Regression (review #3415): an existing Bob 1.x project (managed
+        ``.bob/commands/speckit.*.md`` on disk, no stored ``legacy_commands``)
+        must NOT be treated as skills mode when re-resolved with a
+        project_root, so ``use``/``switch``/``upgrade`` never silently migrate
+        it to skills.
         """
         bob = get_integration("bob")
-        (tmp_path / ".bob" / "commands").mkdir(parents=True)
+        cmds = tmp_path / ".bob" / "commands"
+        cmds.mkdir(parents=True)
+        (cmds / "speckit.plan.md").write_text("# plan", encoding="utf-8")
         # No parsed options at all — the pre-existing-install scenario.
         assert bob.is_skills_mode(None, project_root=tmp_path) is False
         assert bob.is_skills_mode({}, project_root=tmp_path) is False
 
     def test_existing_skills_layout_stays_skills_on_use(self, tmp_path):
-        """A ``.bob/skills/`` project resolves to skills mode."""
+        """A project with managed ``speckit-*`` skills resolves to skills mode."""
         bob = get_integration("bob")
-        (tmp_path / ".bob" / "skills").mkdir(parents=True)
+        (tmp_path / ".bob" / "skills" / "speckit-plan").mkdir(parents=True)
+        assert bob.is_skills_mode(None, project_root=tmp_path) is True
+
+    def test_managed_commands_with_unrelated_skills_dir_stays_legacy(
+        self, tmp_path
+    ):
+        """Regression (review #3415, 4723246468): a legacy Spec Kit install
+        (managed ``.bob/commands/speckit.*.md``) that *also* carries unrelated
+        Bob 2 skills (a ``.bob/skills/`` dir with no managed ``speckit-*``
+        skills) must stay in command mode — the mere presence of a skills
+        directory is not evidence that Spec Kit is skills-based.
+        """
+        bob = get_integration("bob")
+        cmds = tmp_path / ".bob" / "commands"
+        cmds.mkdir(parents=True)
+        (cmds / "speckit.plan.md").write_text("# plan", encoding="utf-8")
+        # An unrelated (non-Spec-Kit) skill the user authored.
+        (tmp_path / ".bob" / "skills" / "my-own-skill").mkdir(parents=True)
+        assert bob.is_skills_mode(None, project_root=tmp_path) is False
+        assert bob.effective_invoke_separator(None, project_root=tmp_path) == "."
+
+    def test_managed_skills_win_when_both_layouts_present(self, tmp_path):
+        """When managed Spec Kit skills exist, skills mode wins even if a stale
+        managed command file is still on disk (upgrade leftover)."""
+        bob = get_integration("bob")
+        cmds = tmp_path / ".bob" / "commands"
+        cmds.mkdir(parents=True)
+        (cmds / "speckit.plan.md").write_text("# plan", encoding="utf-8")
+        (tmp_path / ".bob" / "skills" / "speckit-plan").mkdir(parents=True)
         assert bob.is_skills_mode(None, project_root=tmp_path) is True
 
     def test_fresh_project_defaults_to_skills_with_project_root(self, tmp_path):
-        """A project with no ``.bob/`` layout yet still defaults to skills."""
+        """A project with no managed ``.bob/`` artifacts yet defaults to skills."""
         bob = get_integration("bob")
         assert bob.is_skills_mode(None, project_root=tmp_path) is True
 
     def test_explicit_legacy_flag_wins_over_disk_layout(self, tmp_path):
         """An explicit ``--legacy-commands`` overrides on-disk detection."""
         bob = get_integration("bob")
-        (tmp_path / ".bob" / "skills").mkdir(parents=True)
+        (tmp_path / ".bob" / "skills" / "speckit-plan").mkdir(parents=True)
         assert (
             bob.is_skills_mode({"legacy_commands": True}, project_root=tmp_path)
             is False
@@ -519,7 +550,9 @@ def test_update_init_options_preserves_legacy_commands_project(self, tmp_path):
         from specify_cli import load_init_options
 
         # Existing Bob 1.x project: legacy commands dir on disk, no ai_skills.
-        (tmp_path / ".bob" / "commands").mkdir(parents=True)
+        cmds = tmp_path / ".bob" / "commands"
+        cmds.mkdir(parents=True)
+        (cmds / "speckit.plan.md").write_text("# plan", encoding="utf-8")
         bob = get_integration("bob")
 
         # Simulate the use/switch path: no parsed options were stored.
@@ -538,7 +571,7 @@ def test_update_init_options_keeps_skills_project_as_skills(self, tmp_path):
         )
         from specify_cli import load_init_options
 
-        (tmp_path / ".bob" / "skills").mkdir(parents=True)
+        (tmp_path / ".bob" / "skills" / "speckit-plan").mkdir(parents=True)
         bob = get_integration("bob")
 
         _update_init_options_for_integration(tmp_path, bob, parsed_options=None)
@@ -556,7 +589,9 @@ def test_with_integration_setting_stores_dot_separator_for_legacy(self, tmp_path
         """
         from specify_cli.integration_runtime import with_integration_setting
 
-        (tmp_path / ".bob" / "commands").mkdir(parents=True)
+        cmds = tmp_path / ".bob" / "commands"
+        cmds.mkdir(parents=True)
+        (cmds / "speckit.plan.md").write_text("# plan", encoding="utf-8")
         bob = get_integration("bob")
 
         # Simulate the use/switch path: no parsed options stored.
@@ -694,3 +729,36 @@ def test_active_bob_skills_still_uses_hyphen(self, tmp_path):
         content = written[0].read_text(encoding="utf-8")
         assert "/speckit-plan" in content
         assert "/speckit.plan" not in content
+
+    def test_inactive_bob_command_output_uses_dot_even_with_skills_dir(
+        self, tmp_path
+    ):
+        """Regression (review #3415, 4723246468): for an inactive Bob install
+        the registrar's separator must match the layout it is actually writing
+        (``.bob/commands/*.md`` — command layout), not on-disk sibling dirs.
+        Even when a ``.bob/skills/`` directory (with managed ``speckit-*``
+        skills) coexists, command-layout files must keep ``/speckit.<cmd>``.
+        """
+        from specify_cli._init_options import save_init_options
+        from specify_cli.agents import CommandRegistrar
+
+        # Both layouts on disk; the active agent is something else entirely.
+        (tmp_path / ".bob" / "commands").mkdir(parents=True)
+        (tmp_path / ".bob" / "skills" / "speckit-plan").mkdir(parents=True)
+        save_init_options(tmp_path, {"ai": "claude", "ai_skills": True})
+
+        source_dir = tmp_path / "ext-src"
+        commands = self._write_command_ref_ext(source_dir)
+
+        registrar = CommandRegistrar()
+        registrar.register_commands("bob", commands, "ext", source_dir, tmp_path)
+
+        written = list((tmp_path / ".bob" / "commands").glob("*.md"))
+        assert written, "expected a rendered Bob command file"
+        content = written[0].read_text(encoding="utf-8")
+        assert "__SPECKIT_COMMAND_PLAN__" not in content
+        assert "/speckit.plan" in content, (
+            "Bob command-layout output must use the dot separator regardless "
+            "of a coexisting .bob/skills directory"
+        )
+        assert "/speckit-plan" not in content