diff --git a/docs/reference/integrations.md b/docs/reference/integrations.md index 8b03b5c6c4..62dcc5c083 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. If preset overrides are installed, the migration is rejected with an actionable error (preset artifacts cannot yet be reconciled across a layout change) — remove the preset(s), migrate, then reinstall them. | | [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..9c9a17e2b9 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,18 @@ 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. ``_sep`` (resolved above) is derived from the *output + # layout* this registrar writes — a "/SKILL.md" scaffold uses the skills + # separator, any command-layout file uses the command separator — not + # the project's persisted ai_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..e9330c69b0 100644 --- a/src/specify_cli/integrations/_migrate_commands.py +++ b/src/specify_cli/integrations/_migrate_commands.py @@ -40,6 +40,55 @@ ) +def _manifest_tracks_skill_layout(manifest) -> bool: + """Return True when *manifest* tracks any skills-layout artifact. + + A skill scaffold is written as ``.../speckit-/SKILL.md``, so a + manifest whose tracked files include a ``/SKILL.md`` key is in the skills + layout; otherwise it is in the command layout. Used by ``upgrade`` to + detect a dual-mode agent (e.g. Bob) flipping between the legacy commands + layout and the skills layout so orphaned extension artifacts from the old + layout can be reconciled. + """ + return any(str(rel).endswith("/SKILL.md") for rel in manifest.files) + + +def _installed_presets_affecting_agent(project_root, agent_key: str) -> list[str]: + """Return IDs of installed presets with artifacts registered for *agent_key*. + + Presets register command overrides for every detected agent and mirror + skills for the active skills agent, tracking the result in each preset's + ``registered_commands`` / ``registered_skills`` metadata. There is no + agent-scoped preset re-registration mechanism, so a command↔skills *layout + change* cannot reconcile those artifacts (see ``integration_upgrade``). + Callers use this to detect the unsafe case and reject the migration rather + than silently orphaning preset files / leaving stale registry entries. + + Best-effort: any error resolving preset state yields an empty list so a + broken/absent preset registry never blocks an otherwise-valid upgrade. + """ + try: + from ..presets import PresetManager + + presets = PresetManager(project_root).registry.list() + except Exception: + return [] + + affected: list[str] = [] + for preset_id, meta in presets.items(): + if not isinstance(meta, dict): + continue + registered_commands = meta.get("registered_commands", {}) + has_commands = ( + isinstance(registered_commands, dict) + and bool(registered_commands.get(agent_key)) + ) + has_skills = bool(meta.get("registered_skills")) + if has_commands or has_skills: + affected.append(preset_id) + return affected + + @integration_app.command("switch") def integration_switch( target: str = typer.Argument(help="Integration key to switch to"), @@ -236,7 +285,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 " @@ -398,6 +448,42 @@ def integration_upgrade( integration, current, key, integration_options ) + # Guard: reject a command↔skills layout change while preset overrides are + # installed for this agent (review #3415). A dual-mode agent (e.g. Bob) + # can flip layout across an upgrade (``--skills`` / ``--legacy-commands``). + # Extension artifacts are reconciled after the flip (see below), but preset + # artifacts cannot be: there is no agent-scoped preset re-registration + # anywhere in the CLI, so migrating would delete a preset's old-layout + # files without recreating them in the new layout and leave the preset + # registry claiming artifacts that no longer exist. Detect the intended + # layout (``is_skills_mode`` reflects the resolved flags/disk state, so a + # plain same-layout upgrade is unaffected) and bail out *before* any + # mutation with an actionable error so the project is never left in a + # half-migrated, inconsistent state. + if _manifest_tracks_skill_layout(old_manifest) != integration.is_skills_mode( + parsed_options, project_root + ): + affected_presets = _installed_presets_affecting_agent(project_root, key) + if affected_presets: + preset_list = ", ".join(sorted(affected_presets)) + console.print( + f"[red]Error:[/red] Cannot change '{key}' command layout while " + f"preset override(s) are installed: [bold]{preset_list}[/bold]." + ) + console.print( + "Preset artifacts cannot yet be reconciled across a command↔skills " + "layout change, so the migration would orphan their files and leave " + "the preset registry inconsistent." + ) + console.print( + "Remove the preset(s), run the upgrade, then reinstall them:\n" + f" [cyan]specify preset remove [/cyan]\n" + f" [cyan]specify integration upgrade {key} " + f"--integration-options \"...\"[/cyan]\n" + f" [cyan]specify preset add [/cyan]" + ) + raise typer.Exit(1) + # Ensure shared infrastructure is up to date; --force overwrites existing files. infra_integration = integration infra_key = key @@ -415,7 +501,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 +528,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 +536,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 +552,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 +581,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") @@ -497,6 +599,40 @@ def integration_upgrade( # Done after the upgrade has fully settled (Phase 2 included) and outside # the try/except above so this best-effort step cannot affect upgrade # success. + # + # Layout-change reconciliation: a dual-mode agent (e.g. Bob) can flip + # between the legacy commands layout and the skills layout across an + # upgrade (``upgrade bob --integration-options "--skills"`` / reverse + # ``--legacy-commands``). Phase 2 above only removes stale files tracked by + # the *integration* manifest (core commands); extension artifacts are + # tracked separately in the extension registry, so the old layout's + # extension command/skill files would otherwise linger as orphans. When the + # layout actually changed, first unregister the agent's extension artifacts + # (removing old-layout files and clearing per-agent registry entries) so the + # re-registration below recreates them in the new layout. ``upgrade``s that + # don't change layout skip this to avoid needless remove/re-add churn. + # + # Known limitation: preset command/skill artifacts are NOT reconciled on a + # layout change. There is no agent-scoped preset re-registration mechanism + # anywhere in the CLI — ``use`` / ``switch`` / ``upgrade`` never reconcile + # presets for any agent (presets are only (un)registered at preset + # install/remove time). Rather than silently orphan them, the guard near + # the top of this function rejects a layout-changing upgrade while preset + # overrides are installed, so control only reaches here (with a changed + # layout) when no preset artifacts are at stake. Full preset reconciliation + # would require a new cross-cutting PresetManager subsystem affecting every + # dual-layout agent, which is out of scope for this Bob migration. + if _manifest_tracks_skill_layout(old_manifest) != _manifest_tracks_skill_layout( + new_manifest + ): + _unregister_extensions_for_agent( + project_root, + key, + continuing=( + "The integration layout changed, but old-layout extension " + "artifacts may need manual cleanup." + ), + ) _register_extensions_for_agent( project_root, key, 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 fa498b9604..4af03853c4 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..672e9c10da 100644 --- a/tests/integrations/test_integration_subcommand.py +++ b/tests/integrations/test_integration_subcommand.py @@ -2477,6 +2477,191 @@ 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_bob_layout_change_reconciles_extension_artifacts(self, tmp_path): + """Regression (review #3415, 4725829110). + + When a dual-mode agent (Bob) flips layout across an upgrade, the old + layout's *extension* artifacts must be reconciled — not left orphaned. + A legacy Bob install renders enabled extensions as ``.bob/commands/`` + command files; migrating to skills via ``--skills`` must remove those + command files, recreate the extension as ``.bob/skills/`` skills, and + update the extension registry accordingly (and vice-versa for the + reverse ``--legacy-commands`` migration). + """ + project = _init_project( + tmp_path, "bob", integration_options="--legacy-commands" + ) + + result = _run_in_project(project, ["extension", "add", "git"]) + assert result.exit_code == 0, f"extension add failed: {result.output}" + + commands = project / ".bob" / "commands" + skills = project / ".bob" / "skills" + registry_path = project / ".specify" / "extensions" / ".registry" + + def _git_registry(): + data = json.loads(registry_path.read_text(encoding="utf-8")) + g = data["extensions"]["git"] + return list(g.get("registered_commands", {})), g.get( + "registered_skills", [] + ) + + # Legacy precondition: git renders as command files under .bob/commands. + assert sorted(commands.glob("speckit.git.*.md")), ( + "legacy Bob should render the git extension as command files" + ) + assert not list(skills.glob("speckit-git-*")) if skills.exists() else True + cmds_agents, skill_names = _git_registry() + assert "bob" in cmds_agents and not skill_names + + # Migrate legacy -> skills. + result = _run_in_project(project, [ + "integration", "upgrade", "bob", + "--integration-options", "--skills", + "--script", "sh", "--force", + ]) + assert result.exit_code == 0, f"--skills migration failed: {result.output}" + + # Old-layout git command files removed; skills recreated. + assert not sorted(commands.glob("speckit.git.*.md")), ( + "git extension command files must be removed after --skills migration" + ) + assert sorted(skills.glob("speckit-git-*")), ( + "git extension must be recreated as skills after --skills migration" + ) + cmds_agents, skill_names = _git_registry() + assert "bob" not in cmds_agents, ( + "extension registry must drop the stale bob command entry" + ) + assert skill_names, "extension registry must record the migrated skills" + + # Migrate skills -> legacy: the reverse reconciliation must also hold. + result = _run_in_project(project, [ + "integration", "upgrade", "bob", + "--integration-options", "--legacy-commands", + "--script", "sh", "--force", + ]) + assert result.exit_code == 0, ( + f"--legacy-commands migration failed: {result.output}" + ) + assert not sorted(skills.glob("speckit-git-*")), ( + "git extension skills must be removed after --legacy-commands migration" + ) + assert sorted(commands.glob("speckit.git.*.md")), ( + "git extension command files must be recreated in legacy layout" + ) + cmds_agents, skill_names = _git_registry() + assert "bob" in cmds_agents and not skill_names + + def test_upgrade_bob_layout_change_rejected_with_presets_installed(self, tmp_path): + """Regression (review #3415, 4726193915). + + A command↔skills layout change cannot reconcile preset artifacts (no + agent-scoped preset re-registration exists). Rather than silently + orphaning preset files / leaving the registry inconsistent, a + layout-changing ``upgrade`` must reject the migration with an + actionable error *before any mutation* when preset overrides are + installed for the agent. A same-layout upgrade must still succeed. + """ + project = _init_project( + tmp_path, "bob", integration_options="--legacy-commands" + ) + commands = project / ".bob" / "commands" + skills = project / ".bob" / "skills" + assert sorted(commands.glob("speckit.*.md")) + + # Simulate an installed preset that registered command overrides for bob. + presets_dir = project / ".specify" / "presets" + presets_dir.mkdir(parents=True, exist_ok=True) + (presets_dir / ".registry").write_text( + json.dumps({ + "presets": { + "my-preset": { + "version": "1.0.0", + "enabled": True, + "registered_commands": {"bob": ["speckit.plan"]}, + "registered_skills": [], + } + } + }), + encoding="utf-8", + ) + + # Layout-changing upgrade is rejected, and nothing is mutated. + result = _run_in_project(project, [ + "integration", "upgrade", "bob", + "--integration-options", "--skills", + "--script", "sh", "--force", + ]) + assert result.exit_code != 0, "layout change with presets must be rejected" + assert "preset" in result.output.lower() + assert "my-preset" in result.output + assert not skills.exists(), "no skills layout must be scaffolded on rejection" + assert sorted(commands.glob("speckit.*.md")), ( + "legacy command files must be left untouched on rejection" + ) + + # A same-layout upgrade (no flag) must still succeed with presets present. + result = _run_in_project(project, [ + "integration", "upgrade", "bob", "--script", "sh", "--force", + ]) + assert result.exit_code == 0, ( + f"same-layout upgrade must not be blocked by presets: {result.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(