diff --git a/AGENTS.md b/AGENTS.md index 4f0c9912a8..c52096ba15 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -147,12 +147,12 @@ class CodexIntegration(SkillsIntegration): | Field | Location | Purpose | |---|---|---| -| `key` | Class attribute | Unique identifier; for CLI-based integrations (`requires_cli: True`), must match the CLI executable name | +| `key` | Class attribute | Unique identifier; for most CLI-based integrations this matches the executable name, but see `cli_executable` below for exceptions | | `config` | Class attribute (dict) | Agent metadata: `name`, `folder`, `commands_subdir`, `install_url`, `requires_cli` | | `registrar_config` | Class attribute (dict) | Command output config: `dir`, `format`, `args` placeholder, file `extension` | | `context_file` | Class attribute (str or None) | Path to agent context/instructions file (e.g., `"CLAUDE.md"`, `".github/copilot-instructions.md"`) | -**Key design rule:** For CLI-based integrations (`requires_cli: True`), `key` must be the actual executable name (e.g., `"cursor-agent"` not `"cursor"`). This ensures `shutil.which(key)` works for CLI-tool checks without special-case mappings. IDE-based integrations (`requires_cli: False`) should use their canonical identifier (e.g., `"windsurf"`, `"copilot"`). +**Key design rule:** For CLI-based integrations (`requires_cli: True`), `key` should generally match the CLI executable name so that the default `is_cli_available()` check works without any override. When the executable name differs from the key (e.g., RovoDev's key is `"rovodev"` but the binary is `"acli"`), override the `cli_executable` property or `is_cli_available()` method — see [§6 Optional overrides](#6-optional-overrides) below. IDE-based integrations (`requires_cli: False`) should use their canonical identifier (e.g., `"windsurf"`, `"copilot"`). ### 3. Register it @@ -222,11 +222,37 @@ The base classes handle most work automatically. Override only when the agent de | Override | When to use | Example | |---|---|---| +| `cli_executable` | Binary name differs from `key` | RovoDev: key `"rovodev"`, binary `"acli"` → override returns `"acli"` | +| `is_cli_available()` | Multiple binary names or non-PATH installs | Claude checks `~/.claude/local/`; Kiro accepts both `kiro-cli` and `kiro` | | `command_filename(template_name)` | Custom file naming or extension | Copilot → `speckit.{name}.agent.md` | | `options()` | Integration-specific CLI flags via `--integration-options` | Codex → `--skills` flag, Copilot → `--skills` flag | | `setup()` | Custom install logic (companion files, settings merge) | Copilot → `.agent.md` + `.prompt.md` + `.vscode/settings.json` (default) or `speckit-/SKILL.md` (skills mode) | | `teardown()` | Custom uninstall logic | Rarely needed; base handles manifest-tracked files | +**`cli_executable` property** — Return the binary name to look up on `PATH` for tool-availability checks. The default implementation returns `self.key`. Override when the executable name differs from the integration key: + +```python +@property +def cli_executable(self) -> str: + return "acli" # e.g. RovoDev: key="rovodev", binary="acli" +``` + +**`is_cli_available()` method** — Return `True` if the integration's CLI tool is installed. The default implementation calls `shutil.which(self.cli_executable)`. Override for more complex detection: + +```python +def is_cli_available(self) -> bool: + # Multiple binary names (Kiro): + return shutil.which("kiro-cli") is not None or shutil.which("kiro") is not None + + # Non-PATH install locations (Claude): + import specify_cli._utils as _utils_mod + if _utils_mod.CLAUDE_LOCAL_PATH.is_file() or _utils_mod.CLAUDE_NPM_LOCAL_PATH.is_file(): + return True + return shutil.which(self.cli_executable) is not None +``` + +`is_cli_available()` is used by `check_tool()` in `_utils.py` and by both `CommandStep` and `PromptStep` workflow steps to gate CLI dispatch. No hardcoded special cases should be added to those callers — encode detection logic in the integration class instead. + **Example — Copilot (fully custom `setup`):** Copilot extends `IntegrationBase` directly because it creates `.agent.md` commands, companion `.prompt.md` files, and merges `.vscode/settings.json`. It also supports a `--skills` mode that scaffolds `speckit-/SKILL.md` under `.github/skills/` using composition with an internal `_CopilotSkillsHelper`. See `src/specify_cli/integrations/copilot/__init__.py` for the full implementation. @@ -436,7 +462,7 @@ When an issue exists, include its number immediately after the prefix — this i ## Common Pitfalls -1. **Using shorthand keys for CLI-based integrations**: For CLI-based integrations (`requires_cli: True`), the `key` must match the executable name (e.g., `"cursor-agent"` not `"cursor"`). `shutil.which(key)` is used for CLI tool checks — mismatches require special-case mappings. IDE-based integrations (`requires_cli: False`) are not subject to this constraint. +1. **Using shorthand keys for CLI-based integrations**: For CLI-based integrations (`requires_cli: True`), `key` should generally match the executable name. When it cannot (e.g., the binary name differs), override `cli_executable` or `is_cli_available()` on the integration class. Do **not** add special-case mappings to `check_tool()`, `CommandStep`, or `PromptStep`. 2. **Forgetting context configuration**: The bundled `agent-context` extension reads from `.specify/extensions/agent-context/agent-context-config.yml`. New integrations only need to set `context_file` on the class — markers and dispatcher scripts are managed centrally. 3. **Incorrect `requires_cli` value**: Set to `True` only for agents that have a CLI tool; set to `False` for IDE-based agents. 4. **Wrong argument format**: Use `$ARGUMENTS` for Markdown agents, `{{args}}` for TOML agents. diff --git a/src/specify_cli/_utils.py b/src/specify_cli/_utils.py index beae253593..6d25dea20c 100644 --- a/src/specify_cli/_utils.py +++ b/src/specify_cli/_utils.py @@ -38,35 +38,44 @@ def run_command(cmd: list[str], check_return: bool = True, capture: bool = False def check_tool(tool: str, tracker=None) -> bool: """Check if a tool is installed. Optionally update tracker. + For tools that correspond to a registered integration the check is + delegated to ``IntegrationBase.is_cli_available()`` so that each + integration can encode its own detection logic (e.g. multiple + binary names, non-PATH install locations). Unknown tools fall back + to a plain ``shutil.which`` look-up. + Args: - tool: Name of the tool to check + tool: Name of the tool to check (typically an integration key) tracker: StepTracker | None to update with results Returns: True if tool is found, False otherwise """ - # Special handling for Claude CLI local installs - # See: https://github.com/github/spec-kit/issues/123 - # See: https://github.com/github/spec-kit/issues/550 - # Claude Code can be installed in two local paths: - # 1. ~/.claude/local/claude (after `claude migrate-installer`) - # 2. ~/.claude/local/node_modules/.bin/claude (npm-local install, e.g. via nvm) - # Neither path may be on the system PATH, so we check them explicitly. - if tool == "claude": - if CLAUDE_LOCAL_PATH.is_file() or CLAUDE_NPM_LOCAL_PATH.is_file(): + found: bool + + # Delegate to the integration's is_cli_available() when the tool + # key matches a registered integration. This removes the need for + # hard-coded special cases here (e.g. Claude local paths, kiro dual + # binaries, rovodev/acli mismatch). See issue #2597. + try: + from specify_cli.integrations import get_integration + + impl = get_integration(tool) + if impl is not None: + found = impl.is_cli_available() if tracker: - tracker.complete(tool, "available") - return True - - # Per-integration executable resolution. - if tool == "kiro-cli": - # Kiro currently supports both executable names. Prefer kiro-cli and - # accept kiro as a compatibility fallback. - found = shutil.which("kiro-cli") is not None or shutil.which("kiro") is not None - elif tool == "rovodev": - found = shutil.which("acli") is not None - else: - found = shutil.which(tool) is not None + if found: + tracker.complete(tool, "available") + else: + tracker.error(tool, "not found") + return found + except ImportError as exc: + # Integrations module is unavailable in this environment; fall back + # to PATH-based detection below for non-integration tools. + _ = exc + + # Fallback for non-integration tools (e.g. "git"). + found = shutil.which(tool) is not None if tracker: if found: diff --git a/src/specify_cli/integrations/base.py b/src/specify_cli/integrations/base.py index def5ad20ba..00db1ae856 100644 --- a/src/specify_cli/integrations/base.py +++ b/src/specify_cli/integrations/base.py @@ -162,6 +162,45 @@ def build_exec_args( """ return None + @property + def cli_executable(self) -> str: + """Executable name used for CLI availability detection. + + Defaults to ``self.key``. Integrations whose CLI binary name + differs from the integration key should override this property. + For example, RovoDev's key is ``"rovodev"`` but the binary is + ``"acli"``, so its override returns ``"acli"``. + + This property is used by :meth:`is_cli_available` and by + ``check_tool()`` when checking whether the integration's CLI + tool is installed. It intentionally does **not** honour the + ``SPECKIT_INTEGRATION__EXECUTABLE`` env-var override — that + variable controls which binary is *executed* at runtime (see + :meth:`_resolve_executable`), whereas ``cli_executable`` names + the tool to *detect* on ``PATH``. + + See issue #2597. + """ + return self.key + + def is_cli_available(self) -> bool: + """Return ``True`` if this integration's CLI tool is installed. + + The default implementation checks ``shutil.which(self.cli_executable)``. + Integrations with non-standard install locations or multiple + possible binary names should override this method. + + Examples of integrations that override this: + + * **ClaudeIntegration** — also checks ``~/.claude/local/`` paths + that are not on ``PATH``. + * **KiroCliIntegration** — accepts both ``kiro-cli`` and the + legacy ``kiro`` binary name. + + See issue #2597. + """ + return shutil.which(self.cli_executable) is not None + def _resolve_executable(self) -> str: """Return the executable for this integration's CLI tool. diff --git a/src/specify_cli/integrations/claude/__init__.py b/src/specify_cli/integrations/claude/__init__.py index 57ecb354ad..b6893ce215 100644 --- a/src/specify_cli/integrations/claude/__init__.py +++ b/src/specify_cli/integrations/claude/__init__.py @@ -2,6 +2,7 @@ from __future__ import annotations +import shutil from pathlib import Path from typing import Any @@ -45,6 +46,27 @@ class ClaudeIntegration(SkillsIntegration): context_file = "CLAUDE.md" multi_install_safe = True + def is_cli_available(self) -> bool: + """Return ``True`` if the Claude Code CLI is installed. + + Claude Code can be installed in multiple locations, not all of + which are on ``PATH``: + + 1. ``~/.claude/local/claude`` — ``claude migrate-installer`` + 2. ``~/.claude/local/node_modules/.bin/claude`` — npm-local install (nvm) + 3. Anywhere on ``PATH`` — global npm install + + See issues #123, #550, and #2597. + """ + import specify_cli._utils as _utils_mod + + if ( + _utils_mod.CLAUDE_LOCAL_PATH.is_file() + or _utils_mod.CLAUDE_NPM_LOCAL_PATH.is_file() + ): + return True + return shutil.which(self.cli_executable) is not None + @staticmethod def inject_argument_hint(content: str, hint: str) -> str: """Insert ``argument-hint`` after the first ``description:`` in YAML frontmatter. diff --git a/src/specify_cli/integrations/kiro_cli/__init__.py b/src/specify_cli/integrations/kiro_cli/__init__.py index 4571b54f90..81a1103e51 100644 --- a/src/specify_cli/integrations/kiro_cli/__init__.py +++ b/src/specify_cli/integrations/kiro_cli/__init__.py @@ -1,5 +1,7 @@ """Kiro CLI integration.""" +import shutil + from ..base import MarkdownIntegration @@ -27,3 +29,17 @@ class KiroCliIntegration(MarkdownIntegration): "extension": ".md", } context_file = "AGENTS.md" + + def is_cli_available(self) -> bool: + """Return ``True`` if the Kiro CLI is installed. + + Kiro ships under two binary names: ``kiro-cli`` (preferred) and + the legacy ``kiro`` alias. Either name satisfies the availability + check so existing installations continue to work. + + See issue #2597. + """ + return ( + shutil.which("kiro-cli") is not None + or shutil.which("kiro") is not None + ) diff --git a/src/specify_cli/integrations/rovodev/__init__.py b/src/specify_cli/integrations/rovodev/__init__.py index f8879424ac..88ea00678d 100644 --- a/src/specify_cli/integrations/rovodev/__init__.py +++ b/src/specify_cli/integrations/rovodev/__init__.py @@ -43,6 +43,19 @@ class RovodevIntegration(SkillsIntegration): # -- CLI dispatch ------------------------------------------------------ + @property + def cli_executable(self) -> str: + """Executable name for CLI availability detection (``acli``). + + RovoDev is invoked as ``acli rovodev …`` — ``acli`` is the + host binary; ``rovodev`` is a sub-command. The integration key + is ``"rovodev"``, but the binary to detect on ``PATH`` is + ``"acli"``. + + See issue #2597. + """ + return "acli" + def _resolve_executable(self) -> str: """Return the binary to invoke (``acli``). diff --git a/src/specify_cli/workflows/steps/command/__init__.py b/src/specify_cli/workflows/steps/command/__init__.py index 891b9da4e7..f7f7f86968 100644 --- a/src/specify_cli/workflows/steps/command/__init__.py +++ b/src/specify_cli/workflows/steps/command/__init__.py @@ -2,7 +2,6 @@ from __future__ import annotations -import shutil from pathlib import Path from typing import Any @@ -126,15 +125,10 @@ def _try_dispatch( if impl is None: return None - # Build sample args for fallback executable detection when impl.key is not executable. - exec_args = impl.build_exec_args("test") - - # Check if the CLI tool is actually installed. - # Try the integration key first (covers most agents), then fall back - # to exec_args[0] for agents whose executable differs. - cli_path = shutil.which(impl.key) - fallback_cli_path = shutil.which(exec_args[0]) if exec_args else None - if cli_path is None and fallback_cli_path is None: + # Check if the CLI tool is actually installed via the integration's + # own availability check (honours custom executables, dual binaries, + # and non-PATH install paths). See issue #2597. + if not impl.is_cli_available(): return None project_root = Path(context.project_root) if context.project_root else None diff --git a/src/specify_cli/workflows/steps/prompt/__init__.py b/src/specify_cli/workflows/steps/prompt/__init__.py index 5ec99b794d..c479da8d6d 100644 --- a/src/specify_cli/workflows/steps/prompt/__init__.py +++ b/src/specify_cli/workflows/steps/prompt/__init__.py @@ -2,7 +2,6 @@ from __future__ import annotations -import shutil from pathlib import Path from typing import Any @@ -116,12 +115,10 @@ def _try_dispatch( exec_args = impl.build_exec_args(prompt, model=model, output_json=False) - # Check if the CLI tool is actually installed. - # Try the integration key first (covers most agents), then fall back - # to exec_args[0] for agents whose executable differs. - cli_path = shutil.which(impl.key) - fallback_cli_path = shutil.which(exec_args[0]) if exec_args else None - if cli_path is None and fallback_cli_path is None: + # Check if the CLI tool is actually installed via the integration's + # own availability check (honours custom executables, dual binaries, + # and non-PATH install paths). See issue #2597. + if not impl.is_cli_available(): return None # Prompt dispatch executes exec_args directly; require a non-empty argv. diff --git a/tests/test_check_tool.py b/tests/test_check_tool.py index 9520046168..7bbebb7383 100644 --- a/tests/test_check_tool.py +++ b/tests/test_check_tool.py @@ -10,6 +10,7 @@ from typer.testing import CliRunner from specify_cli import app, check_tool +from specify_cli.integrations import get_integration from tests.conftest import strip_ansi @@ -121,6 +122,98 @@ def fake_which(name): assert check_tool("rovodev") is True +class TestIsCliAvailable: + """Integration.is_cli_available() must encode correct detection logic.""" + + def test_rovodev_cli_executable_is_acli(self): + """RovodevIntegration.cli_executable should return 'acli'.""" + impl = get_integration("rovodev") + assert impl.cli_executable == "acli" + + def test_rovodev_is_cli_available_uses_acli(self): + """RovodevIntegration.is_cli_available() checks for 'acli', not 'rovodev'.""" + impl = get_integration("rovodev") + + with patch("shutil.which", side_effect=lambda name: "/usr/bin/acli" if name == "acli" else None): + assert impl.is_cli_available() is True + + with patch("shutil.which", return_value=None): + assert impl.is_cli_available() is False + + def test_kiro_is_cli_available_accepts_kiro_cli(self): + """KiroCliIntegration.is_cli_available() returns True for 'kiro-cli' binary.""" + impl = get_integration("kiro-cli") + + with patch("shutil.which", side_effect=lambda name: "/usr/bin/kiro-cli" if name == "kiro-cli" else None): + assert impl.is_cli_available() is True + + def test_kiro_is_cli_available_accepts_legacy_kiro(self): + """KiroCliIntegration.is_cli_available() accepts the legacy 'kiro' binary.""" + impl = get_integration("kiro-cli") + + with patch("shutil.which", side_effect=lambda name: "/usr/bin/kiro" if name == "kiro" else None): + assert impl.is_cli_available() is True + + def test_kiro_is_cli_available_false_when_neither(self): + """KiroCliIntegration.is_cli_available() returns False when neither binary exists.""" + impl = get_integration("kiro-cli") + + with patch("shutil.which", return_value=None): + assert impl.is_cli_available() is False + + def test_claude_is_cli_available_local_path(self, tmp_path): + """ClaudeIntegration.is_cli_available() finds claude via local path.""" + impl = get_integration("claude") + fake_claude = tmp_path / "claude" + fake_claude.touch() + fake_missing = tmp_path / "nonexistent" / "claude" + + with patch("specify_cli._utils.CLAUDE_LOCAL_PATH", fake_claude), \ + patch("specify_cli._utils.CLAUDE_NPM_LOCAL_PATH", fake_missing), \ + patch("shutil.which", return_value=None): + assert impl.is_cli_available() is True + + def test_claude_is_cli_available_npm_local_path(self, tmp_path): + """ClaudeIntegration.is_cli_available() finds claude via npm-local path.""" + impl = get_integration("claude") + fake_npm = tmp_path / "node_modules" / ".bin" / "claude" + fake_npm.parent.mkdir(parents=True) + fake_npm.touch() + fake_missing = tmp_path / "nonexistent" / "claude" + + with patch("specify_cli._utils.CLAUDE_LOCAL_PATH", fake_missing), \ + patch("specify_cli._utils.CLAUDE_NPM_LOCAL_PATH", fake_npm), \ + patch("shutil.which", return_value=None): + assert impl.is_cli_available() is True + + def test_claude_is_cli_available_path(self, tmp_path): + """ClaudeIntegration.is_cli_available() finds claude via PATH.""" + impl = get_integration("claude") + fake_missing = tmp_path / "nonexistent" / "claude" + + with patch("specify_cli._utils.CLAUDE_LOCAL_PATH", fake_missing), \ + patch("specify_cli._utils.CLAUDE_NPM_LOCAL_PATH", fake_missing), \ + patch("shutil.which", return_value="/usr/local/bin/claude"): + assert impl.is_cli_available() is True + + def test_claude_is_cli_available_not_found(self, tmp_path): + """ClaudeIntegration.is_cli_available() returns False when not installed.""" + impl = get_integration("claude") + fake_missing = tmp_path / "nonexistent" / "claude" + + with patch("specify_cli._utils.CLAUDE_LOCAL_PATH", fake_missing), \ + patch("specify_cli._utils.CLAUDE_NPM_LOCAL_PATH", fake_missing), \ + patch("shutil.which", return_value=None): + assert impl.is_cli_available() is False + + def test_default_integration_uses_key(self): + """Integrations without an override use key as cli_executable.""" + # Use a non-CLI integration to test the default; any MarkdownIntegration + # without a cli_executable override works. + impl = get_integration("gemini") + assert impl.cli_executable == impl.key + + class TestCheckTip: """`specify check` should point users to the existing version check."""