diff --git a/docs/reference/integrations.md b/docs/reference/integrations.md index 72044ec684..67596f6b58 100644 --- a/docs/reference/integrations.md +++ b/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-` | | [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-/SKILL.md` under `.bob/skills/` and invokes them as `/speckit-`. 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 | diff --git a/integrations/catalog.json b/integrations/catalog.json index 40dc13d84c..1bdaf861ec 100644 --- a/integrations/catalog.json +++ b/integrations/catalog.json @@ -177,11 +177,11 @@ "bob": { "id": "bob", "name": "IBM Bob", - "version": "1.0.0", - "description": "IBM Bob IDE integration", + "version": "2.0.0", + "description": "IBM Bob 2.0 IDE skills-based integration", "author": "spec-kit-core", "repository": "https://github.com/github/spec-kit", - "tags": ["ide", "ibm"] + "tags": ["ide", "ibm", "skills"] }, "trae": { "id": "trae", diff --git a/src/specify_cli/_invocation_style.py b/src/specify_cli/_invocation_style.py index 874903abec..2b5f189488 100644 --- a/src/specify_cli/_invocation_style.py +++ b/src/specify_cli/_invocation_style.py @@ -18,6 +18,7 @@ CONDITIONAL_SLASH_AGENTS: frozenset[str] = frozenset( { "agy", + "bob", "claude", "copilot", "cursor-agent", diff --git a/src/specify_cli/agents.py b/src/specify_cli/agents.py index e4d09ffe99..b46464348e 100644 --- a/src/specify_cli/agents.py +++ b/src/specify_cli/agents.py @@ -637,6 +637,37 @@ def register_commands( is_cline_ext = agent_name == "cline" and source_id != "core" source_root = source_dir.resolve() + # 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: + registrar_writes_skills = ( + agent_config.get("extension") == "/SKILL.md" + ) + _sep = _integ.invoke_separator_for_mode(registrar_writes_skills) + except Exception: + pass + for cmd_info in commands: cmd_name = cmd_info["name"] aliases = cmd_info.get("aliases", []) @@ -709,13 +740,15 @@ def register_commands( ) # Resolve __SPECKIT_COMMAND_*__ tokens using the agent's invoke separator. - # The separator is sourced from agent_config (populated by _build_agent_configs, - # which propagates each integration's invoke_separator class attribute). + # For dual-layout agents (e.g. Bob) the separator differs between the + # skills and command layouts, so a single static AGENT_CONFIGS value is + # insufficient. Resolve it from the integration using the project's + # persisted skills state; single-layout agents fall back to the static + # AGENT_CONFIGS value unchanged (invoke_separator_for_mode default). # Deferred import of IntegrationBase avoids a circular import at module load # (base.py itself imports CommandRegistrar lazily). from specify_cli.integrations.base import IntegrationBase # noqa: PLC0415 - _sep = agent_config.get("invoke_separator", ".") body = IntegrationBase.resolve_command_refs(body, _sep) output_name = self._compute_output_name(agent_name, cmd_name, agent_config) diff --git a/src/specify_cli/commands/init.py b/src/specify_cli/commands/init.py index 1a1dd046a8..f820d6646f 100644 --- a/src/specify_cli/commands/init.py +++ b/src/specify_cli/commands/init.py @@ -458,6 +458,7 @@ def init( script_type=selected_script, raw_options=integration_options, parsed_options=integration_parsed_options or None, + project_root=project_path, ) _write_integration_json( project_path, @@ -478,7 +479,7 @@ def init( tracker=tracker, force=force, invoke_separator=resolved_integration.effective_invoke_separator( - integration_parsed_options + integration_parsed_options, project_root=project_path ), ) tracker.complete( @@ -532,10 +533,8 @@ def init( "feature_numbering": "sequential", "speckit_version": get_speckit_version(), } - from ..integrations.base import SkillsIntegration as _SkillsPersist - - if isinstance(resolved_integration, _SkillsPersist) or getattr( - resolved_integration, "_skills_mode", False + if resolved_integration.is_skills_mode( + integration_parsed_options or None, project_root=project_path ): init_opts["ai_skills"] = True save_init_options(project_path, init_opts) @@ -683,11 +682,9 @@ def init( steps_lines.append("1. You're already in the project directory!") step_num = 2 - from ..integrations.base import SkillsIntegration as _SkillsInt - - _is_skills_integration = isinstance( - resolved_integration, _SkillsInt - ) or getattr(resolved_integration, "_skills_mode", False) + _is_skills_integration = resolved_integration.is_skills_mode( + integration_parsed_options or None, project_root=project_path + ) codex_skill_mode = selected_ai == "codex" and _is_skills_integration zcode_skill_mode = selected_ai == "zcode" and _is_skills_integration @@ -703,6 +700,7 @@ def init( zed_skill_mode = selected_ai == "zed" and _is_skills_integration grok_skill_mode = selected_ai == "grok" and _is_skills_integration cline_skill_mode = selected_ai == "cline" + bob_skill_mode = selected_ai == "bob" and _is_skills_integration native_skill_mode = ( codex_skill_mode or zcode_skill_mode @@ -715,6 +713,7 @@ def init( or devin_skill_mode or zed_skill_mode or grok_skill_mode + or bob_skill_mode ) if codex_skill_mode: @@ -752,6 +751,11 @@ def init( f"{step_num}. Start Grok Build in this project directory; spec-kit skills were installed to [cyan].grok/skills[/cyan]" ) step_num += 1 + if bob_skill_mode: + steps_lines.append( + f"{step_num}. Start Bob in this project directory; spec-kit skills were installed to [cyan].bob/skills[/cyan]" + ) + step_num += 1 usage_label = "skills" if native_skill_mode else "slash commands" from .._invocation_style import ( diff --git a/src/specify_cli/integration_runtime.py b/src/specify_cli/integration_runtime.py index a36dcc672c..9bf8aeac96 100644 --- a/src/specify_cli/integration_runtime.py +++ b/src/specify_cli/integration_runtime.py @@ -46,6 +46,7 @@ def with_integration_setting( script_type: str | None = None, raw_options: str | None = None, parsed_options: dict[str, Any] | None = None, + project_root: Any = None, ) -> dict[str, dict[str, Any]]: """Return integration settings with *key* updated.""" settings = integration_settings(state) @@ -63,7 +64,9 @@ def with_integration_setting( elif raw_options is not None: current.pop("parsed_options", None) - current["invoke_separator"] = integration.effective_invoke_separator(parsed_options) + current["invoke_separator"] = integration.effective_invoke_separator( + parsed_options, project_root + ) settings[key] = current return settings @@ -73,10 +76,11 @@ def invoke_separator_for_integration( state: dict[str, Any], key: str, parsed_options: dict[str, Any] | None = None, + project_root: Any = None, ) -> str: """Resolve the invocation separator for stored/default integration state.""" if parsed_options is not None: - return integration.effective_invoke_separator(parsed_options) + return integration.effective_invoke_separator(parsed_options, project_root) setting = integration_setting(state, key) stored_separator = setting.get("invoke_separator") @@ -85,6 +89,6 @@ def invoke_separator_for_integration( stored_parsed = setting.get("parsed_options") if isinstance(stored_parsed, dict): - return integration.effective_invoke_separator(stored_parsed) + return integration.effective_invoke_separator(stored_parsed, project_root) - return integration.effective_invoke_separator(None) + return integration.effective_invoke_separator(None, project_root) diff --git a/src/specify_cli/integrations/_helpers.py b/src/specify_cli/integrations/_helpers.py index 07a62efeed..d27ca7ef4d 100644 --- a/src/specify_cli/integrations/_helpers.py +++ b/src/specify_cli/integrations/_helpers.py @@ -272,24 +272,19 @@ def _update_init_options_for_integration( load_init_options, save_init_options, ) - from .base import SkillsIntegration opts = load_init_options(project_root) opts["integration"] = integration.key opts["ai"] = integration.key opts["speckit_version"] = _get_speckit_version() if script_type: opts["script"] = script_type - # Skills mode is either intrinsic (SkillsIntegration), set on the instance - # during setup() (_skills_mode), or requested via parsed options (e.g. - # Copilot's --skills, persisted as parsed_options["skills"]). The latter is - # the only signal available on the `use` path, where no setup() runs and a - # fresh integration instance has _skills_mode == False (issue #3550). - skills_mode = ( - isinstance(integration, SkillsIntegration) - or getattr(integration, "_skills_mode", False) - or bool((parsed_options or {}).get("skills")) - ) - if skills_mode: + # Whether skills mode is active is owned by each integration via the + # ``is_skills_mode`` hook (base default honors ``--skills``; + # SkillsIntegration returns True; skills-first integrations with a legacy + # opt-out such as Bob override it). This keeps shared code free of + # ``isinstance`` / ``_skills_mode`` probing. Passing parsed_options lets it + # work on the ``use``/``install`` path where no setup() runs (issue #3550). + if integration.is_skills_mode(parsed_options, project_root=project_root): opts["ai_skills"] = True else: opts.pop("ai_skills", None) @@ -325,6 +320,7 @@ def _set_default_integration( script_type=resolved_script, raw_options=raw_options, parsed_options=parsed_options, + project_root=project_root, ) if refresh_templates: @@ -333,7 +329,8 @@ def _set_default_integration( project_root, resolved_script, invoke_separator=_invoke_separator_for_integration( - integration, {"integration_settings": settings}, key, parsed_options + integration, {"integration_settings": settings}, key, parsed_options, + project_root=project_root, ), force=refresh_templates_force, refresh_managed=True, diff --git a/src/specify_cli/integrations/_install_commands.py b/src/specify_cli/integrations/_install_commands.py index 66fd2b2d26..740d8de708 100644 --- a/src/specify_cli/integrations/_install_commands.py +++ b/src/specify_cli/integrations/_install_commands.py @@ -127,7 +127,8 @@ def integration_install( project_root, selected_script, invoke_separator=_invoke_separator_for_integration( - infra_integration, current, infra_key, infra_parsed + infra_integration, current, infra_key, infra_parsed, + project_root=project_root, ), ) if os.name != "nt": @@ -155,10 +156,16 @@ def integration_install( script_type=selected_script, raw_options=raw_options, parsed_options=parsed_options, + project_root=project_root, ) _write_integration_json(project_root, new_default, new_installed, settings) if new_default == integration.key: - _update_init_options_for_integration(project_root, integration, script_type=selected_script) + _update_init_options_for_integration( + project_root, + integration, + script_type=selected_script, + parsed_options=parsed_options, + ) else: _refresh_init_options_speckit_version(project_root) diff --git a/src/specify_cli/integrations/_migrate_commands.py b/src/specify_cli/integrations/_migrate_commands.py index 6568d1af18..6518db8fdc 100644 --- a/src/specify_cli/integrations/_migrate_commands.py +++ b/src/specify_cli/integrations/_migrate_commands.py @@ -236,7 +236,8 @@ def integration_switch( force=refresh_shared_infra, refresh_managed=True, invoke_separator=_invoke_separator_for_integration( - target_integration, current, target, parsed_options + target_integration, current, target, parsed_options, + project_root=project_root, ), refresh_hint=( "To overwrite customizations, re-run with " @@ -415,7 +416,8 @@ def integration_upgrade( selected_script, force=force, invoke_separator=_invoke_separator_for_integration( - infra_integration, current, infra_key, infra_parsed + infra_integration, current, infra_key, infra_parsed, + project_root=project_root, ), ) if os.name != "nt": @@ -441,6 +443,7 @@ def integration_upgrade( script_type=selected_script, raw_options=raw_options, parsed_options=parsed_options, + project_root=project_root, ) if installed_key == key: try: @@ -448,7 +451,8 @@ def integration_upgrade( project_root, selected_script, invoke_separator=_invoke_separator_for_integration( - integration, {"integration_settings": settings}, key, parsed_options + integration, {"integration_settings": settings}, key, parsed_options, + project_root=project_root, ), force=force, refresh_managed=True, @@ -463,7 +467,12 @@ def integration_upgrade( new_manifest.save() _write_integration_json(project_root, installed_key, installed_keys, settings) if installed_key == key: - _update_init_options_for_integration(project_root, integration, script_type=selected_script) + _update_init_options_for_integration( + project_root, + integration, + script_type=selected_script, + parsed_options=parsed_options, + ) else: _refresh_init_options_speckit_version(project_root) except Exception as exc: @@ -487,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") diff --git a/src/specify_cli/integrations/base.py b/src/specify_cli/integrations/base.py index a425d4e012..3d316cc158 100644 --- a/src/specify_cli/integrations/base.py +++ b/src/specify_cli/integrations/base.py @@ -160,17 +160,66 @@ def options(cls) -> list[IntegrationOption]: return [] def effective_invoke_separator( - self, parsed_options: dict[str, Any] | None = None + self, + parsed_options: dict[str, Any] | None = None, + project_root: Path | None = None, ) -> str: """Return the invoke separator for the given options. Subclasses whose separator depends on runtime options (e.g. Copilot in ``--skills`` mode) should override this method. - The default implementation ignores *parsed_options* and returns - the class-level ``invoke_separator``. + The default implementation ignores *parsed_options* and + *project_root* and returns the class-level ``invoke_separator``. """ return self.invoke_separator + def invoke_separator_for_mode(self, skills_enabled: bool) -> str: + """Command-ref separator given the project's *resolved* skills state. + + Registration paths (extension / preset command rendering) have no CLI + ``parsed_options`` — only the persisted ``ai_skills`` flag — so they + resolve the command-reference separator through this hook rather than + the static ``AGENT_CONFIGS[key]["invoke_separator"]`` value, which + cannot represent an agent whose separator differs between its skills + and command layouts. + + The default is mode-independent and returns exactly what + ``_build_agent_configs`` would place in ``AGENT_CONFIGS`` (the + ``registrar_config`` override if present, else the class-level + ``invoke_separator``), so single-layout agents are unaffected. + Dual-mode agents whose separator depends on the layout (e.g. Bob: + ``-`` for skills, ``.`` for legacy commands) override this. + """ + cfg = self.registrar_config or {} + return cfg.get("invoke_separator", self.invoke_separator) + + def is_skills_mode( + self, + parsed_options: dict[str, Any] | None = None, + project_root: Path | None = None, + ) -> bool: + """Return whether this integration scaffolds skills for these options. + + This is the single, well-defined hook the shared init/install/upgrade + machinery consults to decide whether to persist ``ai_skills=True`` and + render skill invocations. It replaces ad-hoc ``isinstance`` / + ``getattr(self, "_skills_mode", ...)`` probing so an integration's + internal representation never has to leak into shared dispatch code. + + *project_root* is optional context for the ``use`` / ``switch`` / + ``upgrade`` path, where no ``setup()`` runs and *parsed_options* may be + empty: dual-mode integrations can consult the already-installed + on-disk layout to avoid silently migrating an existing project to a + different mode. The default ignores it. + + The default (command-first integrations, e.g. Copilot's default + layout) is skills mode only when ``--skills`` was requested. + ``SkillsIntegration`` overrides this to return ``True`` by default; + skills-first integrations that expose a legacy opt-out (e.g. Bob) + override it to honor their own flag. + """ + return bool((parsed_options or {}).get("skills")) + def build_exec_args( self, prompt: str, @@ -1376,6 +1425,14 @@ class SkillsIntegration(IntegrationBase): invoke_separator = "-" + def is_skills_mode( + self, + parsed_options: dict[str, Any] | None = None, + project_root: Path | None = None, + ) -> bool: + """Skills-native integrations scaffold skills unconditionally.""" + return True + def build_exec_args( self, prompt: str, diff --git a/src/specify_cli/integrations/bob/__init__.py b/src/specify_cli/integrations/bob/__init__.py index b953151bd2..f0c85f3b45 100644 --- a/src/specify_cli/integrations/bob/__init__.py +++ b/src/specify_cli/integrations/bob/__init__.py @@ -1,10 +1,105 @@ -"""IBM Bob integration.""" +"""IBM Bob integration. -from ..base import MarkdownIntegration +Bob 2.0 uses the ``.bob/skills/speckit-/SKILL.md`` layout by default. +The legacy ``.bob/commands/*.md`` layout (Bob 1.x) remains available as an +opt-in via ``--integration-options "--legacy-commands"``. +Bob is a *dual-mode* integration: whether it scaffolds skills or commands is +a per-project **configuration** decision (the ``--legacy-commands`` option, +persisted as ``ai_skills`` in init-options), not a property of the class. +It therefore extends :class:`IntegrationBase` (like Copilot, the other +dual-mode agent) and resolves the mode through the ``is_skills_mode`` hook, +delegating the actual scaffolding to a per-layout helper. + +Deprecation cycle: + This release: Skills layout is the default; legacy ``.bob/commands/`` is + opt-in via ``--legacy-commands``. + Next cycle: ``--legacy-commands`` flag removed. +""" + +from __future__ import annotations + +import warnings +from pathlib import Path +from typing import Any + +import typer + +from ..base import ( + IntegrationBase, + IntegrationOption, + MarkdownIntegration, + SkillsIntegration, +) +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 " + "removed in a future Spec Kit release. Omit --legacy-commands to use " + "the default skills layout (.bob/skills/).", + UserWarning, + stacklevel=3, + ) + + +class _BobSkillsHelper(SkillsIntegration): + """Default-mode helper: ``.bob/skills/speckit-/SKILL.md``. + + Not registered in the integration registry — used only as a delegate by + :class:`BobIntegration` for skills-mode ``setup()``. + """ -class BobIntegration(MarkdownIntegration): key = "bob" + config = { + "name": "IBM Bob", + "folder": ".bob/", + "commands_subdir": "skills", + "install_url": None, + "requires_cli": False, + } + registrar_config = { + "dir": ".bob/skills", + "format": "markdown", + "args": "$ARGUMENTS", + "extension": "/SKILL.md", + } + + def post_process_skill_content(self, content: str) -> str: + """Bob skills are intent-activated; no slash-command note is needed.""" + return content + + +class _BobMarkdownHelper(MarkdownIntegration): + """Legacy-mode helper: ``.bob/commands/speckit..md`` (Bob 1.x). + + Not registered in the integration registry — used only as a delegate by + :class:`BobIntegration` when ``--legacy-commands`` is passed. Declares + ``invoke_separator="."`` so command-reference tokens render as Bob 1.x + ``/speckit.`` invocations. + """ + + key = "bob" + invoke_separator = "." config = { "name": "IBM Bob", "folder": ".bob/", @@ -17,4 +112,168 @@ class BobIntegration(MarkdownIntegration): "format": "markdown", "args": "$ARGUMENTS", "extension": ".md", + "invoke_separator": ".", } + + +class BobIntegration(IntegrationBase): + """Integration for IBM Bob IDE (dual-mode; skills by default). + + Whether a project uses the skills or the legacy commands layout is a + configuration choice resolved by :meth:`is_skills_mode`, not the class + hierarchy. ``setup()`` delegates to the matching helper. + + ``registrar_config`` mirrors the *commands* layout (``extension: ".md"``, + ``dir: ".bob/commands"``) — the same pattern Copilot uses — so that + ``CommandRegistrar.AGENT_CONFIGS["bob"]`` drives extension/preset + registration into ``.bob/commands/`` for legacy-mode projects, while + skills-mode projects have that command registration transparently skipped + (``skills_mode_active`` becomes ``True`` because ``ai_skills=True`` and + ``extension != "/SKILL.md"``) and receive extension skills instead. + ``invoke_separator = "-"`` matches the default (skills) layout. + """ + + key = "bob" + invoke_separator = "-" + config = { + "name": "IBM Bob", + "folder": ".bob/", + "commands_subdir": "commands", + "install_url": None, + "requires_cli": False, + } + registrar_config = { + "dir": ".bob/commands", + "format": "markdown", + "args": "$ARGUMENTS", + "extension": ".md", + } + + @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, + default=False, + help=( + "Scaffold commands as legacy .bob/commands/*.md files " + "(Bob 1.x layout, deprecated) instead of the default " + "skills layout" + ), + ), + ] + + def is_skills_mode( + self, + parsed_options: dict[str, Any] | None = None, + project_root: Path | None = None, + ) -> bool: + """Bob is skills-first; ``--legacy-commands`` opts out. + + 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. 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 + ``--skills`` overrides this so users are never trapped in legacy mode. + """ + 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" + 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 + + def effective_invoke_separator( + self, + parsed_options: dict[str, Any] | None = None, + project_root: Path | None = None, + ) -> str: + """``"."`` for the legacy commands layout, ``"-"`` for skills. + + *project_root* lets the ``use`` / ``switch`` / ``upgrade`` path — which + refreshes shared infrastructure *before* persisting init-options — + detect an already-installed legacy layout, so core command references + are rendered with the correct separator instead of defaulting to the + skills ``-``. + """ + return "-" if self.is_skills_mode(parsed_options, project_root) else "." + + def invoke_separator_for_mode(self, skills_enabled: bool) -> str: + """Resolve the command-ref separator from a project's persisted mode. + + Skills projects render ``/speckit-``; legacy command projects + render Bob 1.x ``/speckit.``. Extension/preset registration + consults this (via the persisted ``ai_skills`` flag) so both layouts + get the correct separator despite sharing one static ``AGENT_CONFIGS`` + entry. + """ + return "-" if skills_enabled else "." + + def post_process_skill_content(self, content: str) -> str: + """Bob skills are intent-activated; no slash-command note is injected. + + Preset/extension skill generators call this on the *registered* + ``BobIntegration`` instance, not on :class:`_BobSkillsHelper`, so the + no-op must be repeated here (delegating to the helper) — otherwise + those paths would inherit ``IntegrationBase``'s default and inject + ``/speckit-*`` hook guidance that core Bob skills intentionally omit. + """ + return _BobSkillsHelper().post_process_skill_content(content) + + def setup( + self, + project_root: Path, + manifest: IntegrationManifest, + parsed_options: dict[str, Any] | None = None, + **opts: Any, + ) -> list[Path]: + parsed_options = parsed_options or {} + if self.is_skills_mode(parsed_options, project_root): + return _BobSkillsHelper().setup( + project_root, manifest, parsed_options, **opts + ) + _warn_legacy_commands_deprecated() + return MarkdownIntegration.setup( + _BobMarkdownHelper(), project_root, manifest, parsed_options, **opts + ) diff --git a/src/specify_cli/integrations/copilot/__init__.py b/src/specify_cli/integrations/copilot/__init__.py index 44bd47f353..ce41c00d6f 100644 --- a/src/specify_cli/integrations/copilot/__init__.py +++ b/src/specify_cli/integrations/copilot/__init__.py @@ -122,7 +122,9 @@ class CopilotIntegration(IntegrationBase): _skills_mode: bool = False def effective_invoke_separator( - self, parsed_options: dict[str, Any] | None = None + self, + parsed_options: dict[str, Any] | None = None, + project_root: Path | None = None, ) -> str: """Return ``"-"`` when skills mode is requested, ``"."`` otherwise.""" if parsed_options and parsed_options.get("skills"): @@ -131,6 +133,33 @@ def effective_invoke_separator( return "-" return self.invoke_separator + def is_skills_mode( + self, + parsed_options: dict[str, Any] | None = None, + project_root: Path | None = None, + ) -> bool: + """Copilot is skills mode when ``--skills`` was requested. + + On the init path ``setup()`` has already recorded the choice in + ``self._skills_mode``; on the ``use``/``install`` path (where no + ``setup()`` runs) the signal comes from *parsed_options* (#3550), which + round-trips because ``--skills`` is persisted in the stored options. + """ + if parsed_options and parsed_options.get("skills"): + return True + return self._skills_mode + + def invoke_separator_for_mode(self, skills_enabled: bool) -> str: + """Skills projects render ``/speckit-``; default markdown ``.``. + + Copilot is dual-layout, so — like Bob — the command-reference + separator depends on the persisted ``ai_skills`` state rather than a + single static value. This keeps preset/extension command refs in a + Copilot skills project consistent with ``build_command_invocation`` + (which emits ``/speckit-``). + """ + return "-" if skills_enabled else self.invoke_separator + @classmethod def options(cls) -> list[IntegrationOption]: return [ diff --git a/src/specify_cli/integrations/manifest.py b/src/specify_cli/integrations/manifest.py index 8c98243c9a..a318b990cb 100644 --- a/src/specify_cli/integrations/manifest.py +++ b/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: diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index 97e15aa648..5773bf979c 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -1171,7 +1171,7 @@ def _reconcile_skills(self, command_names: List[str]) -> None: selected_ai, fm, body, self.project_root ) body = self._resolve_skill_command_refs( - body, registrar, selected_ai + body, registrar, selected_ai, self.project_root ) from ..integrations import get_integration integration = get_integration(selected_ai) if isinstance(selected_ai, str) else None @@ -1252,7 +1252,10 @@ def _skill_title_from_command(cmd_name: str) -> str: @staticmethod def _resolve_skill_command_refs( - body: str, registrar: "CommandRegistrar", selected_ai: str + body: str, + registrar: "CommandRegistrar", + selected_ai: str, + project_root: "Path | None" = None, ) -> str: """Render ``__SPECKIT_COMMAND_*__`` tokens in a skill body as invocations. @@ -1261,10 +1264,30 @@ def _resolve_skill_command_refs( slash-command invocation — ``/speckit-`` for a ``-`` separator, ``/speckit.`` for ``.`` — the same rendering the command layer applies via ``CommandRegistrar.register_commands()``. + + For dual-layout agents (e.g. Bob) the separator depends on the + project's persisted skills state, so — when *project_root* is provided + — the separator is resolved from the integration via + ``invoke_separator_for_mode`` rather than the single static + ``AGENT_CONFIGS`` value. """ - separator = registrar.AGENT_CONFIGS.get(selected_ai, {}).get( - "invoke_separator", "." - ) + separator = None + if project_root is not None and isinstance(selected_ai, str): + try: + from .. import load_init_options + from ..integrations import get_integration + + integration = get_integration(selected_ai) + if integration is not None: + separator = integration.invoke_separator_for_mode( + is_ai_skills_enabled(load_init_options(project_root)) + ) + except Exception: + separator = None + if separator is None: + separator = registrar.AGENT_CONFIGS.get(selected_ai, {}).get( + "invoke_separator", "." + ) return IntegrationBase.resolve_command_refs(body, separator) def _build_extension_skill_restore_index(self) -> Dict[str, Dict[str, Any]]: @@ -1445,7 +1468,7 @@ def _register_skills( body = registrar.resolve_skill_placeholders( selected_ai, frontmatter, body, self.project_root ) - body = self._resolve_skill_command_refs(body, registrar, selected_ai) + body = self._resolve_skill_command_refs(body, registrar, selected_ai, self.project_root) for target_skill_name in target_skill_names: skill_subdir = skills_dir / target_skill_name @@ -1540,7 +1563,7 @@ def _unregister_skills(self, skill_names: List[str], preset_dir: Path) -> None: selected_ai, frontmatter, body, self.project_root ) body = self._resolve_skill_command_refs( - body, registrar, selected_ai + body, registrar, selected_ai, self.project_root ) original_desc = frontmatter.get("description", "") @@ -1592,7 +1615,7 @@ def _unregister_skills(self, skill_names: List[str], preset_dir: Path) -> None: selected_ai, frontmatter, body, self.project_root ) body = self._resolve_skill_command_refs( - body, registrar, selected_ai + body, registrar, selected_ai, self.project_root ) command_name = extension_restore["command_name"] diff --git a/tests/integrations/test_integration_bob.py b/tests/integrations/test_integration_bob.py index 8e0e72f0bd..52a25ae2c6 100644 --- a/tests/integrations/test_integration_bob.py +++ b/tests/integrations/test_integration_bob.py @@ -1,10 +1,927 @@ """Tests for BobIntegration.""" -from .test_integration_base_markdown import MarkdownIntegrationTests +import os +import warnings +import pytest +import yaml -class TestBobIntegration(MarkdownIntegrationTests): - KEY = "bob" - FOLDER = ".bob/" - COMMANDS_SUBDIR = "commands" - REGISTRAR_DIR = ".bob/commands" +from specify_cli.integrations import INTEGRATION_REGISTRY, get_integration +from specify_cli.integrations.base import SkillsIntegration +from specify_cli.integrations.manifest import IntegrationManifest + + +class TestBobIntegrationRegistration: + def test_registered(self): + assert "bob" in INTEGRATION_REGISTRY + assert get_integration("bob") is not None + + def test_is_integration_base_not_skills_integration(self): + """BobIntegration extends IntegrationBase directly — not SkillsIntegration. + + Bob is dual-mode (skills by default, legacy commands via + ``--legacy-commands``), so its skills-ness is a per-project config + decision resolved by the ``is_skills_mode`` hook — not a class-hierarchy + property. It therefore must NOT be a ``SkillsIntegration`` (which is + reserved for statically skills-only agents); shared code consults + ``is_skills_mode(parsed_options)`` instead of ``isinstance``. + ``invoke_separator='-'`` is set explicitly on the class to match the + default (skills) layout. + """ + from specify_cli.integrations.base import IntegrationBase + bob = get_integration("bob") + assert isinstance(bob, IntegrationBase) + assert not isinstance(bob, SkillsIntegration) + assert bob.invoke_separator == "-" + + def test_key_and_config(self): + bob = get_integration("bob") + assert bob.key == "bob" + assert bob.config["folder"] == ".bob/" + # registrar_config mirrors the legacy commands layout so that + # CommandRegistrar.AGENT_CONFIGS["bob"] follows the Copilot pattern: + # extension registration writes to .bob/commands/ for legacy-mode + # projects and is skipped for skills-mode projects (skills_mode_active). + assert bob.config["commands_subdir"] == "commands" + assert bob.registrar_config["dir"] == ".bob/commands" + assert bob.registrar_config["extension"] == ".md" + + def test_invoke_separator_is_hyphen(self): + """Class-level invoke_separator must be '-' so CommandRegistrar.AGENT_CONFIGS + generates correct /speckit- refs without calling effective_invoke_separator.""" + bob = get_integration("bob") + assert bob.invoke_separator == "-" + + +class TestBobOptionsFlag: + def test_options_include_legacy_commands_flag(self): + bob = get_integration("bob") + opts = bob.options() + legacy_opts = [o for o in opts if o.name == "--legacy-commands"] + assert len(legacy_opts) == 1 + opt = legacy_opts[0] + assert opt.is_flag is True + # Legacy must be OPT-IN (default=False) — skills are the default + assert opt.default is False + + def test_options_include_skills_migration_flag(self): + """Review #3415, 4724160183, comment 1: a ``--skills`` opt-in exists as + the supported migration path from legacy commands to the skills layout. + It is distinct from the pre-skills-default ``--skills`` flag: here it + *forces* skills mode over on-disk auto-detection. + """ + bob = get_integration("bob") + opts = bob.options() + skills_opts = [o for o in opts if o.name == "--skills"] + assert len(skills_opts) == 1 + opt = skills_opts[0] + assert opt.is_flag is True + # Opt-in: disk auto-detection remains the default behavior. + assert opt.default is False + + +class TestBobIsSkillsModeHook: + """The is_skills_mode hook is the single source of truth for the mode.""" + + def test_default_is_skills(self): + bob = get_integration("bob") + assert bob.is_skills_mode(None) is True + assert bob.is_skills_mode({}) is True + + def test_legacy_commands_disables_skills(self): + bob = get_integration("bob") + 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 (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") + 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 project with managed ``speckit-*`` skills resolves to skills mode.""" + bob = get_integration("bob") + (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 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" / "speckit-plan").mkdir(parents=True) + assert ( + bob.is_skills_mode({"legacy_commands": True}, project_root=tmp_path) + is False + ) + + def test_explicit_skills_flag_forces_skills_over_legacy_disk_layout( + self, tmp_path + ): + """Regression (review #3415, 4724160183, comment 1). + + ``--skills`` is the supported migration / opt-in: it must force skills + mode even when a managed legacy ``.bob/commands`` layout is on disk + (which otherwise auto-detects to legacy). This gives + ``integration upgrade bob --integration-options="--skills"`` a path out + of legacy mode instead of being trapped by disk detection. + """ + bob = get_integration("bob") + cmds = tmp_path / ".bob" / "commands" + cmds.mkdir(parents=True) + (cmds / "speckit.plan.md").write_text("# plan", encoding="utf-8") + assert bob.is_skills_mode({"skills": True}, project_root=tmp_path) is True + assert ( + bob.effective_invoke_separator({"skills": True}, project_root=tmp_path) + == "-" + ) + + def test_skills_and_legacy_flags_are_mutually_exclusive(self): + """Passing both ``--skills`` and ``--legacy-commands`` exits cleanly.""" + import typer + + bob = get_integration("bob") + with pytest.raises(typer.Exit): + bob.is_skills_mode({"skills": True, "legacy_commands": True}) + + def test_effective_invoke_separator_tracks_mode(self): + bob = get_integration("bob") + assert bob.effective_invoke_separator(None) == "-" + assert bob.effective_invoke_separator({"legacy_commands": True}) == "." + assert bob.effective_invoke_separator({"skills": True}) == "-" + + def test_invoke_separator_for_mode_tracks_persisted_state(self): + """Registration paths resolve the separator from persisted ai_skills.""" + bob = get_integration("bob") + assert bob.invoke_separator_for_mode(True) == "-" + assert bob.invoke_separator_for_mode(False) == "." + + def test_no_skills_mode_method_leaks(self): + """The old callable _skills_mode method must be gone; consumers use the hook.""" + bob = get_integration("bob") + assert not callable(getattr(bob, "_skills_mode", None)) + + +class TestBobDefaultSkillsMode: + """Default mode: .bob/skills/speckit-/SKILL.md layout.""" + + def test_setup_creates_skill_files(self, tmp_path): + bob = get_integration("bob") + m = IntegrationManifest("bob", tmp_path) + created = bob.setup(tmp_path, m) + assert len(created) > 0 + for f in created: + assert f.exists() + assert f.name == "SKILL.md" + assert f.parent.name.startswith("speckit-") + + def test_setup_writes_to_correct_directory(self, tmp_path): + bob = get_integration("bob") + m = IntegrationManifest("bob", tmp_path) + bob.setup(tmp_path, m) + skills_dir = tmp_path / ".bob" / "skills" + assert skills_dir.is_dir() + + def test_setup_does_not_warn(self, tmp_path): + bob = get_integration("bob") + m = IntegrationManifest("bob", tmp_path) + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + bob.setup(tmp_path, m) + assert not any( + "legacy" in str(item.message).lower() for item in caught + ) + + def test_setup_no_commands_dir(self, tmp_path): + bob = get_integration("bob") + m = IntegrationManifest("bob", tmp_path) + bob.setup(tmp_path, m) + assert not (tmp_path / ".bob" / "commands").exists() + + def test_skill_directory_structure(self, tmp_path): + """Each command produces speckit-/SKILL.md.""" + bob = get_integration("bob") + m = IntegrationManifest("bob", tmp_path) + created = bob.setup(tmp_path, m) + + expected_commands = { + "analyze", "clarify", "constitution", "converge", "implement", + "plan", "checklist", "specify", "tasks", "taskstoissues", + } + actual_commands = {f.parent.name.removeprefix("speckit-") for f in created} + assert actual_commands == expected_commands + + def test_skill_frontmatter_structure(self, tmp_path): + """SKILL.md must have name, description, compatibility, metadata.""" + bob = get_integration("bob") + m = IntegrationManifest("bob", tmp_path) + created = bob.setup(tmp_path, m) + for f in created: + content = f.read_text(encoding="utf-8") + assert content.startswith("---\n"), f"{f} missing frontmatter" + parts = content.split("---", 2) + fm = yaml.safe_load(parts[1]) + assert "name" in fm + assert "description" in fm + assert "compatibility" in fm + assert "metadata" in fm + assert fm["metadata"]["author"] == "github-spec-kit" + + def test_templates_are_processed(self, tmp_path): + bob = get_integration("bob") + m = IntegrationManifest("bob", tmp_path) + created = bob.setup(tmp_path, m) + for f in created: + content = f.read_text(encoding="utf-8") + assert "{SCRIPT}" not in content, f"{f.name} has unprocessed {{SCRIPT}}" + assert "__AGENT__" not in content, f"{f.name} has unprocessed __AGENT__" + assert "{ARGS}" not in content, f"{f.name} has unprocessed {{ARGS}}" + assert "__SPECKIT_COMMAND_" not in content, f"{f.name} has unprocessed __SPECKIT_COMMAND_*__" + + def test_command_refs_use_hyphen_separator(self, tmp_path): + """Default skills layout must use /speckit-, not /speckit..""" + bob = get_integration("bob") + m = IntegrationManifest("bob", tmp_path) + created = bob.setup(tmp_path, m) + for f in created: + content = f.read_text(encoding="utf-8") + assert "/speckit." not in content, ( + f"{f.name} contains dot-notation /speckit. reference; " + "skills must use /speckit-" + ) + + def test_all_files_tracked_in_manifest(self, tmp_path): + bob = get_integration("bob") + m = IntegrationManifest("bob", tmp_path) + created = bob.setup(tmp_path, m) + for f in created: + rel = f.resolve().relative_to(tmp_path.resolve()).as_posix() + assert rel in m.files, f"{rel} not tracked in manifest" + + def test_install_uninstall_roundtrip(self, tmp_path): + bob = get_integration("bob") + m = IntegrationManifest("bob", tmp_path) + created = bob.install(tmp_path, m) + assert len(created) > 0 + m.save() + for f in created: + assert f.exists() + removed, skipped = bob.uninstall(tmp_path, m) + assert len(removed) == len(created) + assert skipped == [] + + +class TestBobLegacyCommandsMode: + """Legacy opt-in mode: .bob/commands/speckit..md layout.""" + + def test_setup_legacy_creates_markdown_files(self, tmp_path): + from specify_cli.integrations.bob import BobIntegration + bob = BobIntegration() + m = IntegrationManifest("bob", tmp_path) + created = bob.setup(tmp_path, m, parsed_options={"legacy_commands": True}) + assert len(created) > 0 + for f in created: + assert f.exists() + assert f.suffix == ".md" + assert f.name.startswith("speckit.") + assert f.parent == tmp_path / ".bob" / "commands" + + def test_setup_legacy_warns_deprecated(self, tmp_path): + from specify_cli.integrations.bob import BobIntegration + bob = BobIntegration() + m = IntegrationManifest("bob", tmp_path) + with pytest.warns(UserWarning, match="Bob legacy commands mode"): + bob.setup(tmp_path, m, parsed_options={"legacy_commands": True}) + + def test_setup_legacy_no_skills_dir(self, tmp_path): + from specify_cli.integrations.bob import BobIntegration + bob = BobIntegration() + m = IntegrationManifest("bob", tmp_path) + bob.setup(tmp_path, m, parsed_options={"legacy_commands": True}) + assert not (tmp_path / ".bob" / "skills").exists() + + def test_setup_legacy_templates_are_processed(self, tmp_path): + from specify_cli.integrations.bob import BobIntegration + bob = BobIntegration() + m = IntegrationManifest("bob", tmp_path) + bob.setup(tmp_path, m, parsed_options={"legacy_commands": True}) + commands_dir = tmp_path / ".bob" / "commands" + for md_file in commands_dir.glob("speckit.*.md"): + content = md_file.read_text(encoding="utf-8") + assert "{SCRIPT}" not in content + assert "__AGENT__" not in content + assert "{ARGS}" not in content + assert "__SPECKIT_COMMAND_" not in content + + def test_setup_legacy_all_files_tracked(self, tmp_path): + from specify_cli.integrations.bob import BobIntegration + bob = BobIntegration() + m = IntegrationManifest("bob", tmp_path) + created = bob.setup(tmp_path, m, parsed_options={"legacy_commands": True}) + for f in created: + rel = f.resolve().relative_to(tmp_path.resolve()).as_posix() + assert rel in m.files, f"{rel} not tracked in manifest" + + def test_setup_legacy_uninstall_roundtrip(self, tmp_path): + from specify_cli.integrations.bob import BobIntegration + bob = BobIntegration() + m = IntegrationManifest("bob", tmp_path) + created = bob.install(tmp_path, m, parsed_options={"legacy_commands": True}) + assert len(created) > 0 + m.save() + removed, skipped = bob.uninstall(tmp_path, m) + assert len(removed) == len(created) + assert skipped == [] + + +class TestBobInitFlowDefault: + """CLI init creates skills by default.""" + + def test_init_default_creates_skills(self, tmp_path): + from typer.testing import CliRunner + from specify_cli import app + + target = tmp_path / "test-proj" + result = CliRunner().invoke(app, [ + "init", str(target), "--integration", "bob", + "--ignore-agent-tools", "--script", "sh", + ]) + assert result.exit_code == 0, f"init --integration bob failed: {result.output}" + assert (target / ".bob" / "skills" / "speckit-plan" / "SKILL.md").exists() + assert not (target / ".bob" / "commands").exists() + + def test_init_default_complete_file_inventory_sh(self, tmp_path): + from typer.testing import CliRunner + from specify_cli import app + + project = tmp_path / "inventory-sh-bob" + project.mkdir() + old_cwd = os.getcwd() + try: + os.chdir(project) + result = CliRunner().invoke(app, [ + "init", "--here", "--integration", "bob", "--script", "sh", + "--ignore-agent-tools", + ], catch_exceptions=False) + finally: + os.chdir(old_cwd) + assert result.exit_code == 0, f"init failed: {result.output}" + + commands = [ + "analyze", "clarify", "constitution", "converge", "implement", + "plan", "checklist", "specify", "tasks", "taskstoissues", + ] + for cmd in commands: + assert (project / ".bob" / "skills" / f"speckit-{cmd}" / "SKILL.md").exists(), ( + f"Missing .bob/skills/speckit-{cmd}/SKILL.md" + ) + + +class TestBobInitFlowLegacy: + """CLI init with --legacy-commands produces .bob/commands/*.md.""" + + def test_init_legacy_creates_commands(self, tmp_path): + from typer.testing import CliRunner + from specify_cli import app + + target = tmp_path / "test-proj" + result = CliRunner().invoke(app, [ + "init", str(target), "--integration", "bob", + "--integration-options", "--legacy-commands", + "--ignore-agent-tools", "--script", "sh", + ]) + assert result.exit_code == 0, f"init --integration bob --legacy-commands failed: {result.output}" + assert (target / ".bob" / "commands" / "speckit.plan.md").exists() + assert not (target / ".bob" / "skills").exists() + + def test_init_legacy_does_not_set_ai_skills(self, tmp_path): + """Legacy install must NOT write ai_skills=True to init-options.json. + + Behavioral guard for the dual-mode contract: with --legacy-commands, + BobIntegration.is_skills_mode(parsed_options) returns False, so + _update_init_options_for_integration must not persist ai_skills=True. + (Regression origin: shared code previously probed a bound _skills_mode + method object, which is always truthy, and wrongly enabled skills for + legacy projects.) + """ + from typer.testing import CliRunner + from specify_cli import app + from specify_cli import load_init_options + + target = tmp_path / "test-proj" + result = CliRunner().invoke(app, [ + "init", str(target), "--integration", "bob", + "--integration-options", "--legacy-commands", + "--ignore-agent-tools", "--script", "sh", + ]) + assert result.exit_code == 0, f"init failed: {result.output}" + init_opts = load_init_options(target) + assert init_opts.get("ai_skills") is not True, ( + "Legacy Bob project must not have ai_skills=True in init-options.json" + ) + + +class TestBobRegistrarConfig: + """Verify AGENT_CONFIGS["bob"] follows the Copilot pattern for extension registration.""" + + def test_registrar_config_uses_commands_layout(self): + """AGENT_CONFIGS["bob"] must use the legacy .md layout (not /SKILL.md). + + This mirrors Copilot: the static registrar config targets the non-skills + format so that: + - skills_mode_active becomes True when ai_skills=True, preventing + extension registration from writing SKILL.md files into .bob/skills/ + on projects that never asked for legacy files. + - legacy-mode projects receive extension .md files in .bob/commands/. + """ + from specify_cli.agents import CommandRegistrar + registrar = CommandRegistrar() + bob_cfg = registrar.AGENT_CONFIGS.get("bob") + assert bob_cfg is not None, "bob must be in AGENT_CONFIGS" + assert bob_cfg["extension"] == ".md", ( + "AGENT_CONFIGS['bob']['extension'] must be '.md' so that " + "skills_mode_active=True suppresses extension registration on " + "skills-mode projects (mirrors the Copilot pattern)" + ) + assert bob_cfg["dir"] == ".bob/commands" + + def test_skills_mode_project_extension_registration_skipped(self, tmp_path): + """Extension registrar skips Bob on skills-mode projects (no .bob/commands dir).""" + from specify_cli.agents import CommandRegistrar + # Simulate a skills-mode Bob project: .bob/skills exists, .bob/commands does not + (tmp_path / ".bob" / "skills").mkdir(parents=True) + + registrar = CommandRegistrar() + results = registrar.register_commands_for_all_agents( + commands=[{"name": "speckit.test-cmd", "file": "test.md"}], + source_id="test", + source_dir=tmp_path, + project_root=tmp_path, + ) + # Bob must not appear in results — .bob/commands doesn't exist + assert "bob" not in results + + def test_legacy_mode_project_extension_registration_runs(self, tmp_path): + """Extension registrar writes to .bob/commands/ for legacy-mode projects.""" + import textwrap + from specify_cli.agents import CommandRegistrar + + # Simulate a legacy-mode Bob project: .bob/commands exists, .bob/skills does not + commands_dir = tmp_path / ".bob" / "commands" + commands_dir.mkdir(parents=True) + + # Provide a minimal command source file + cmd_file = tmp_path / "test.md" + cmd_file.write_text( + textwrap.dedent("""\ + --- + description: "Test command" + --- + Test body. + """), + encoding="utf-8", + ) + + registrar = CommandRegistrar() + results = registrar.register_commands_for_all_agents( + commands=[{"name": "speckit.test-cmd", "file": "test.md"}], + source_id="test", + source_dir=tmp_path, + project_root=tmp_path, + ) + assert "bob" in results, "bob must appear in results for legacy-mode project" + registered_file = commands_dir / "speckit.test-cmd.md" + assert registered_file.exists(), f"Expected {registered_file} to be written" + + def test_legacy_extension_command_refs_use_dot_separator(self, tmp_path): + """Regression (review #3415): legacy .bob/commands/ extension commands must + render Bob 1.x ``/speckit.`` refs, not the skills-layout ``/speckit-``. + + The single static AGENT_CONFIGS["bob"]["invoke_separator"] is "-" (the + default skills layout); register_commands must instead resolve the + separator from the project's persisted mode via + BobIntegration.invoke_separator_for_mode(False) -> ".". + """ + import textwrap + from specify_cli.agents import CommandRegistrar + + # Legacy-mode project: .bob/commands exists, ai_skills is NOT set. + commands_dir = tmp_path / ".bob" / "commands" + commands_dir.mkdir(parents=True) + cmd_file = tmp_path / "test.md" + cmd_file.write_text( + textwrap.dedent("""\ + --- + description: "Test command" + --- + See __SPECKIT_COMMAND_SPECIFY__ for details. + """), + encoding="utf-8", + ) + + registrar = CommandRegistrar() + registrar.register_commands_for_all_agents( + commands=[{"name": "speckit.test-cmd", "file": "test.md"}], + source_id="test", + source_dir=tmp_path, + project_root=tmp_path, + ) + rendered = (commands_dir / "speckit.test-cmd.md").read_text(encoding="utf-8") + assert "/speckit.specify" in rendered, ( + "legacy Bob extension commands must render /speckit.specify (dot)" + ) + assert "/speckit-specify" not in rendered + + +class TestBobUseFlowPreservesLegacyLayout: + """Regression (review #3415): re-activating an existing Bob 1.x project + must not silently migrate it to the skills layout. + """ + + def test_update_init_options_preserves_legacy_commands_project(self, tmp_path): + """``use``/``switch``/``upgrade`` on a ``.bob/commands``-only project + (no stored ``legacy_commands``) must not write ``ai_skills=True``. + """ + from specify_cli.integrations._helpers import ( + _update_init_options_for_integration, + ) + from specify_cli import load_init_options + + # Existing Bob 1.x project: legacy commands dir on disk, no ai_skills. + 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. + _update_init_options_for_integration(tmp_path, bob, parsed_options=None) + + opts = load_init_options(tmp_path) + assert opts.get("ai") == "bob" + assert opts.get("ai_skills") is not True, ( + "an existing .bob/commands project must stay legacy on re-activation" + ) + + def test_update_init_options_keeps_skills_project_as_skills(self, tmp_path): + """A ``.bob/skills`` project stays skills on re-activation.""" + from specify_cli.integrations._helpers import ( + _update_init_options_for_integration, + ) + from specify_cli import load_init_options + + (tmp_path / ".bob" / "skills" / "speckit-plan").mkdir(parents=True) + bob = get_integration("bob") + + _update_init_options_for_integration(tmp_path, bob, parsed_options=None) + + opts = load_init_options(tmp_path) + assert opts.get("ai_skills") is True + + def test_with_integration_setting_stores_dot_separator_for_legacy(self, tmp_path): + """Regression (review #3415): shared-infra refresh on the use/switch + path resolves the command-ref separator *before* init-options are + rewritten, via ``effective_invoke_separator``. For an existing + ``.bob/commands`` project with no stored options this must resolve to + ``"."`` (project-aware), not the skills-layout ``"-"``; otherwise core + command references get rewritten to ``/speckit-*``. + """ + from specify_cli.integration_runtime import with_integration_setting + + 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. + settings = with_integration_setting( + {}, "bob", bob, parsed_options=None, project_root=tmp_path + ) + assert settings["bob"]["invoke_separator"] == ".", ( + "legacy .bob/commands project must persist the dot separator so " + "shared templates render Bob 1.x /speckit. references" + ) + + def test_use_force_keeps_legacy_command_refs_in_shared_templates(self, tmp_path): + """End-to-end (review #3415): ``integration use bob --force`` on an + existing Bob 1.x project (legacy layout on disk, stored options + stripped as a pre-PR install would be) must re-render shared templates + with ``/speckit.`` (dot), not ``/speckit-``. + """ + import json + from typer.testing import CliRunner + from specify_cli import app + + # Create a real legacy Bob project (renders shared templates). + target = tmp_path / "proj" + runner = CliRunner() + result = runner.invoke(app, [ + "init", str(target), "--integration", "bob", + "--integration-options", "--legacy-commands", + "--ignore-agent-tools", "--script", "sh", + ]) + assert result.exit_code == 0, f"init failed: {result.output}" + + template = target / ".specify" / "templates" / "plan-template.md" + assert template.is_file(), "expected a rendered shared plan template" + assert "/speckit.plan" in template.read_text(encoding="utf-8") + + # Simulate a pre-PR Bob 1.x install: no stored options/separator. + integ_json = target / ".specify" / "integration.json" + data = json.loads(integ_json.read_text(encoding="utf-8")) + bob_settings = data["integration_settings"]["bob"] + for stale in ("raw_options", "parsed_options", "invoke_separator"): + bob_settings.pop(stale, None) + integ_json.write_text(json.dumps(data, indent=2), encoding="utf-8") + + # Re-activate with --force so shared templates are re-rendered. + import os + old_cwd = os.getcwd() + try: + os.chdir(target) + result = runner.invoke( + app, ["integration", "use", "bob", "--force"] + ) + finally: + os.chdir(old_cwd) + assert result.exit_code == 0, f"use failed: {result.output}" + + rendered = template.read_text(encoding="utf-8") + assert "/speckit.plan" in rendered, ( + "legacy Bob project must keep /speckit.plan (dot) after refresh" + ) + assert "/speckit-plan" not in rendered, ( + "shared templates must not be rewritten to the skills /speckit-plan" + ) + # And the persisted separator must reflect the legacy layout. + data = json.loads(integ_json.read_text(encoding="utf-8")) + assert data["integration_settings"]["bob"].get("invoke_separator") == "." + + +class TestBobCommandRefScopedToActiveAgent: + """Regression (review #3415, 4716424313). + + ``CommandRegistrar.register_commands`` runs once per detected agent, but the + persisted ``ai_skills`` flag describes only the *active* integration + (``opts["ai"]``). When another agent (e.g. Copilot) is active in skills + mode while a legacy ``.bob/commands`` layout is also present, Bob's command + references must still render with the ``.`` separator (Bob 1.x + ``/speckit.``) rather than inheriting Copilot's ``ai_skills=True`` and + rendering ``/speckit-``. + """ + + def _write_command_ref_ext(self, source_dir): + source_dir.mkdir(parents=True, exist_ok=True) + cmd = source_dir / "run.md" + cmd.write_text( + "---\ndescription: Run\n---\n\nUse __SPECKIT_COMMAND_PLAN__ first.\n", + encoding="utf-8", + ) + return [{"name": "speckit.ext.run", "file": "run.md"}] + + def test_legacy_bob_ref_not_rewritten_when_other_agent_active_in_skills( + self, tmp_path + ): + from specify_cli._init_options import save_init_options + from specify_cli.agents import CommandRegistrar + + # Legacy Bob layout on disk; skills layout absent. + (tmp_path / ".bob" / "commands").mkdir(parents=True) + # A different agent (Copilot) is the active integration, in skills mode. + save_init_options(tmp_path, {"ai": "copilot", "ai_skills": True}) + + source_dir = tmp_path / "ext-src" + commands = self._write_command_ref_ext(source_dir) + + registrar = CommandRegistrar() + registered = registrar.register_commands( + "bob", commands, "ext", source_dir, tmp_path, + ) + assert "speckit.ext.run" in registered + + 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, ( + "legacy Bob command refs must use the dot separator even when " + "another agent is active in skills mode" + ) + assert "/speckit-plan" not in content + + def test_active_bob_skills_command_output_uses_dot(self, tmp_path): + """Regression (review #3415, 4724160183, comment 2). + + The separator must match the *output layout* the registrar writes, not + the project's persisted ``ai_skills`` flag. Even when Bob itself is the + active agent in skills mode, a ``.bob/commands/*.md`` file is a + command-layout artifact and must render Bob 1.x ``/speckit.``. + Rendering ``/speckit-`` into a command file (as the old + ``ai_skills``-driven active-agent branch did) produced an invocation the + command layout can't resolve. Bob skills are written via its own + skills path, so ``register_commands`` only ever emits command-layout + files for Bob. + """ + from specify_cli._init_options import save_init_options + from specify_cli.agents import CommandRegistrar + + (tmp_path / ".bob" / "skills").mkdir(parents=True) + save_init_options(tmp_path, {"ai": "bob", "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, ( + "a .bob/commands/*.md command-layout file must use the dot " + "separator even when Bob is the active agent in skills mode" + ) + 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.``. + """ + 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 + + +class TestBobSetupPreservesLegacyOnUpgrade: + """Regression (review #3415, 4723782860, comment 1). + + ``setup()`` must apply the same managed-artifact detection as ``use`` so + that ``integration upgrade bob`` on a Bob 1.x install (managed + ``.bob/commands/speckit.*.md`` on disk, no stored options) preserves the + command layout instead of silently generating skills and stale-deleting + the legacy commands. + """ + + def test_setup_without_options_preserves_existing_command_layout( + self, tmp_path + ): + from specify_cli.integrations.bob import BobIntegration + + # Pre-existing Bob 1.x install: managed command files, no options. + cmds = tmp_path / ".bob" / "commands" + cmds.mkdir(parents=True) + (cmds / "speckit.plan.md").write_text("# plan", encoding="utf-8") + + bob = BobIntegration() + m = IntegrationManifest("bob", tmp_path) + with pytest.warns(UserWarning, match="Bob legacy commands mode"): + created = bob.setup(tmp_path, m, parsed_options=None) + + # Command layout regenerated; no skills layout introduced. + assert not (tmp_path / ".bob" / "skills").exists(), ( + "upgrade must not migrate an existing legacy Bob project to skills" + ) + assert created, "expected command files to be regenerated" + for f in created: + assert f.parent == tmp_path / ".bob" / "commands" + assert f.suffix == ".md" + + def test_setup_fresh_project_still_defaults_to_skills(self, tmp_path): + """A fresh project (no managed artifacts) still defaults to skills.""" + from specify_cli.integrations.bob import BobIntegration + + bob = BobIntegration() + m = IntegrationManifest("bob", tmp_path) + created = bob.setup(tmp_path, m, parsed_options=None) + + assert (tmp_path / ".bob" / "skills").is_dir() + assert not (tmp_path / ".bob" / "commands").exists() + assert created + + def test_setup_with_skills_flag_migrates_legacy_to_skills(self, tmp_path): + """Review #3415, 4724160183, comment 1: ``--skills`` on an existing + legacy install forces the skills layout (the migration opt-in), instead + of preserving the auto-detected legacy layout. ``setup()`` scaffolds the + skills layout; the ``integration upgrade`` stale-file pass removes the + old command files. + """ + from specify_cli.integrations.bob import BobIntegration + + # Pre-existing Bob 1.x install on disk. + cmds = tmp_path / ".bob" / "commands" + cmds.mkdir(parents=True) + (cmds / "speckit.plan.md").write_text("# plan", encoding="utf-8") + + bob = BobIntegration() + m = IntegrationManifest("bob", tmp_path) + # No deprecation warning — the user opted into skills, not legacy. + with warnings.catch_warnings(): + warnings.simplefilter("error", UserWarning) + created = bob.setup(tmp_path, m, parsed_options={"skills": True}) + + assert (tmp_path / ".bob" / "skills").is_dir(), ( + "--skills must force the skills layout even when a legacy commands " + "layout is already on disk" + ) + assert created + for f in created: + assert f.name == "SKILL.md" + assert f.parent.name.startswith("speckit-") + + +class TestBobPostProcessSkillContent: + """Regression (review #3415, 4723782860, comment 2). + + Preset/extension skill generators call ``post_process_skill_content`` on + the *registered* ``BobIntegration`` instance. Core Bob skills are + intent-activated and intentionally omit the shared slash-command hook note, + so the registered class must expose the same no-op the skills helper does + (not inherit a note-injecting default) to keep every skill path consistent. + """ + + def test_registered_bob_has_post_process_hook(self): + bob = get_integration("bob") + assert hasattr(bob, "post_process_skill_content") + + def test_post_process_is_noop_no_hook_note_injected(self): + bob = get_integration("bob") + sample = ( + "---\nname: speckit-plan\n---\n\n" + "Run /speckit.plan then /speckit.tasks.\n" + ) + assert bob.post_process_skill_content(sample) == sample + + def test_post_process_matches_skills_helper(self): + from specify_cli.integrations.bob import _BobSkillsHelper + + bob = get_integration("bob") + sample = "---\nname: speckit-analyze\n---\n\nSome body with /speckit.plan.\n" + assert ( + bob.post_process_skill_content(sample) + == _BobSkillsHelper().post_process_skill_content(sample) + ) diff --git a/tests/integrations/test_integration_copilot.py b/tests/integrations/test_integration_copilot.py index 5b3a5712ad..e9cb33f66d 100644 --- a/tests/integrations/test_integration_copilot.py +++ b/tests/integrations/test_integration_copilot.py @@ -575,6 +575,17 @@ def test_skills_mode_invoke_separator(self): assert copilot.effective_invoke_separator({"skills": True}) == "-" assert copilot.effective_invoke_separator({"skills": False}) == "." + def test_invoke_separator_for_mode_tracks_persisted_state(self): + """Regression (review #3415): registration paths (preset/extension + command refs) must resolve the separator from the persisted ai_skills + state. A Copilot skills project renders ``/speckit-`` (hyphen), + matching ``build_command_invocation``; the default markdown layout + renders ``/speckit.`` (dot). + """ + copilot = self._make_copilot() + assert copilot.invoke_separator_for_mode(True) == "-" + assert copilot.invoke_separator_for_mode(False) == "." + def test_skill_body_has_content(self, tmp_path): """Each SKILL.md body should contain template content.""" copilot = self._make_copilot() diff --git a/tests/integrations/test_integration_subcommand.py b/tests/integrations/test_integration_subcommand.py index a6a3807498..7c189e23d9 100644 --- a/tests/integrations/test_integration_subcommand.py +++ b/tests/integrations/test_integration_subcommand.py @@ -2477,6 +2477,59 @@ def test_upgrade_migrates_opencode_legacy_dir(self, tmp_path): f"found: {[f.name for f in core_remaining]}" ) + def test_upgrade_bob_skills_migration_preserves_manifest(self, tmp_path): + """Regression (review #3415, 4724160183, comment 1). + + ``integration upgrade bob --integration-options="--skills"`` migrates a + legacy Bob 1.x install (``.bob/commands/*.md``) to the skills layout + (``.bob/skills/speckit-*/SKILL.md``) and stale-removes the old command + files. Because that stale-file pass shrinks the tracked set, the + upgrade's Phase 2 must NOT delete the freshly-saved ``bob.manifest.json`` + — otherwise the migrated project is left untracked and un-upgradeable. + """ + project = _init_project( + tmp_path, "bob", integration_options="--legacy-commands" + ) + + commands = project / ".bob" / "commands" + skills = project / ".bob" / "skills" + manifest_path = ( + project / ".specify" / "integrations" / "bob.manifest.json" + ) + assert commands.is_dir() and sorted(commands.glob("speckit.*.md")) + assert not skills.exists() + assert manifest_path.is_file() + + result = _run_in_project(project, [ + "integration", "upgrade", "bob", + "--integration-options", "--skills", + "--script", "sh", "--force", + ]) + assert result.exit_code == 0, f"migration upgrade failed: {result.output}" + + # Skills layout scaffolded; legacy core command files removed. + assert skills.is_dir(), ".bob/skills/ must exist after --skills migration" + assert sorted(skills.glob("speckit-*")), "expected migrated skill dirs" + core_commands = [ + f for f in commands.glob("speckit.*.md") + if "agent-context" not in f.name + ] if commands.exists() else [] + assert core_commands == [], ( + f"legacy core command files should be removed, found: " + f"{[f.name for f in core_commands]}" + ) + + # The manifest must survive so the project stays tracked/upgradeable. + assert manifest_path.is_file(), ( + "bob.manifest.json must survive a layout-shrinking migration" + ) + reupgrade = _run_in_project(project, [ + "integration", "upgrade", "bob", "--script", "sh", "--force", + ]) + assert reupgrade.exit_code == 0, ( + f"migrated project must remain upgradeable: {reupgrade.output}" + ) + def test_upgrade_preserves_existing_vscode_settings(self, tmp_path): """Regression: copilot upgrade must not stale-delete .vscode/settings.json. diff --git a/tests/integrations/test_manifest.py b/tests/integrations/test_manifest.py index 06c1fd398d..32b769769a 100644 --- a/tests/integrations/test_manifest.py +++ b/tests/integrations/test_manifest.py @@ -220,6 +220,28 @@ def test_removes_manifest_file(self, tmp_path): m.uninstall() assert not m.manifest_path.exists() + def test_remove_manifest_false_preserves_manifest_file(self, tmp_path): + """Regression (review #3415, 4724160183): a partial cleanup must not + delete ``{key}.manifest.json``. + + The upgrade stale-file pass builds a throwaway manifest sharing the + integration's key over a subset of files and uninstalls it. With + ``remove_manifest=False`` the tracked files are still removed but the + real, freshly-saved manifest for that key survives — otherwise a + layout-shrinking upgrade (e.g. Bob migrating legacy commands → skills) + would leave the integration untracked and un-upgradeable. + """ + m = IntegrationManifest("test", tmp_path, version="1.0") + m.record_file("f.txt", "content") + m.save() + assert m.manifest_path.exists() + removed, skipped = m.uninstall(remove_manifest=False) + assert len(removed) == 1 + assert not (tmp_path / "f.txt").exists() + assert m.manifest_path.exists(), ( + "remove_manifest=False must keep the manifest file on disk" + ) + def test_cleans_empty_parent_dirs(self, tmp_path): m = IntegrationManifest("test", tmp_path) m.record_file("a/b/c/f.txt", "content") diff --git a/tests/test_extension_skills.py b/tests/test_extension_skills.py index dea42a3852..799629b076 100644 --- a/tests/test_extension_skills.py +++ b/tests/test_extension_skills.py @@ -978,6 +978,7 @@ def test_skill_registration_rewrites_extension_subdir_paths(self, project_dir, t ("codex", "$speckit-plan"), ("kimi", "/skill:speckit-plan"), ("zcode", "$speckit-plan"), + ("bob", "/speckit-plan"), ], ) def test_skill_registration_resolves_command_ref_tokens(