diff --git a/AGENTS.md b/AGENTS.md index 061b8b12b1..55b735cbb1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -151,6 +151,22 @@ class CodexIntegration(SkillsIntegration): **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., `"kilocode"`, `"copilot"`). +**When `key` and the CLI executable genuinely differ** (issue [#2558](https://github.com/github/spec-kit/issues/2558)): sometimes the executable name legitimately cannot match `key` — e.g. RovoDev's `key` is `"rovodev"` but it's invoked via the `acli` binary (`acli rovodev …`). Don't hack around this — override `IntegrationBase`'s `cli_executable` property (or its underlying `_resolve_executable()` hook) instead: + +```python +class RovodevIntegration(SkillsIntegration): + key = "rovodev" + ... + + def _resolve_executable(self) -> str: + # cli_executable delegates here by default; override the fallback + # instead of self.key while still honoring SPECKIT_INTEGRATION_ROVODEV_EXECUTABLE. + env_name = f"SPECKIT_INTEGRATION_{self.key.upper().replace('-', '_')}_EXECUTABLE" + return os.environ.get(env_name, "").strip() or "acli" +``` + +`check_tool()` in `_utils.py` detects installed CLIs by calling `get_integration(tool).is_cli_available()`, which defaults to `shutil.which(self.cli_executable) is not None`. Override `is_cli_available()` directly (rather than just `cli_executable`) when detection needs more than a single PATH lookup — e.g. `ClaudeIntegration` also checks local install paths (`~/.claude/local/claude`, npm-local), and `KiroCliIntegration` accepts both `kiro-cli` and the legacy `kiro` binary name. Tools with no registered integration (e.g. `"git"`) fall back to a plain `shutil.which(tool)` check. + ### 3. Register it In `src/specify_cli/integrations/__init__.py`, add one import and one `_register()` call inside `_register_builtins()`. Both lists are alphabetical: @@ -502,7 +518,7 @@ Disclosure is **continuous**, not a one-time event. A single AI-disclosure parag ## 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`), the `key` must match the executable name (e.g., `"cursor-agent"` not `"cursor"`) whenever possible. `shutil.which(key)` is used for CLI tool checks by default. If the executable genuinely can't match `key` (e.g. RovoDev's `key="rovodev"` but binary is `acli`), override `cli_executable` / `_resolve_executable()` — or `is_cli_available()` for multi-path detection — instead of adding a hardcoded special case to `check_tool()` (see [#2558](https://github.com/github/spec-kit/issues/2558)). IDE-based integrations (`requires_cli: False`) are not subject to this constraint. 2. **Reintroducing context handling into the CLI**: The opt-in `agent-context` extension owns everything about context files — including the per-agent default mapping in `agent-context-defaults.json`. Integration classes must **not** declare a `context_file`, and no CLI code should read, write, resolve, or migrate context files. All context-file logic lives in `.specify/extensions/agent-context/` and its bundled scripts. 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 de7836698e..e03e81b717 100644 --- a/src/specify_cli/_utils.py +++ b/src/specify_cli/_utils.py @@ -104,28 +104,17 @@ def check_tool(tool: str, tracker=None) -> bool: 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(): - 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 + # Integrations declare their own CLI-detection contract via + # `IntegrationBase.is_cli_available()` (see issue #2558), which + # subsumes what used to be hardcoded special cases here for Claude's + # non-PATH local installs, kiro-cli's dual executable names, and + # rovodev's `acli`-backed dispatch. Fall back to a plain `shutil.which` + # for tool names that are not registered integrations (e.g. "git", + # "code", "code-insiders"). + from .integrations import get_integration + + impl = get_integration(tool) + found = impl.is_cli_available() if impl is not None else 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 ccc8587709..3e7d51ca0f 100644 --- a/src/specify_cli/integrations/base.py +++ b/src/specify_cli/integrations/base.py @@ -260,6 +260,41 @@ def _resolve_executable(self) -> str: override = os.environ.get(env_name, "").strip() return override if override else self.key + @property + def cli_executable(self) -> str: + """Executable name used to detect this integration's CLI tool. + + Defaults to whatever ``_resolve_executable()`` returns (the + ``SPECKIT_INTEGRATION__EXECUTABLE`` override, else ``self.key``), + so integrations whose executable differs from their key only need to + override ``_resolve_executable()`` — as ``RovodevIntegration`` already + does for ``acli`` — to get correct CLI detection for free. + + Integrations that need to accept more than one candidate binary name, + or check non-``PATH`` install locations, should override + ``is_cli_available()`` instead of (or in addition to) this property. + + See issue #2558. + """ + return self._resolve_executable() + + def is_cli_available(self) -> bool: + """Return whether this integration's CLI tool is installed. + + The default implementation checks ``shutil.which(self.cli_executable)``. + Detection call sites (``check_tool()``, workflow command/prompt + dispatch) should call this instead of hardcoding + ``shutil.which(self.key)`` or maintaining a per-agent special case. + + Override for agents whose detection can't be expressed as a single + executable name — e.g. ``KiroCliIntegration`` accepts either + ``kiro-cli`` or the legacy ``kiro`` binary, and Claude Code also + checks non-``PATH`` local install locations. + + See issue #2558. + """ + return shutil.which(self.cli_executable) is not None + def _apply_extra_args_env_var(self, args: list[str]) -> None: """Append `SPECKIT_INTEGRATION__EXTRA_ARGS` env-var value to *args*. diff --git a/src/specify_cli/integrations/claude/__init__.py b/src/specify_cli/integrations/claude/__init__.py index 923a77607a..6b0db57a6e 100644 --- a/src/specify_cli/integrations/claude/__init__.py +++ b/src/specify_cli/integrations/claude/__init__.py @@ -5,6 +5,7 @@ from typing import Any from ..base import SkillsIntegration +from ... import _utils from ..._utils import dump_frontmatter # Mapping of command template stem → argument-hint text shown inline @@ -54,6 +55,22 @@ class ClaudeIntegration(SkillsIntegration): } multi_install_safe = True + def is_cli_available(self) -> bool: + """Claude Code can be installed in two local paths that may not be + on the system ``PATH``: + + 1. ``~/.claude/local/claude`` (after ``claude migrate-installer``) + 2. ``~/.claude/local/node_modules/.bin/claude`` (npm-local install, + e.g. via nvm) + + Checked here (rather than a hardcoded special case in + ``check_tool()``) so any future detection call site gets the same + behavior for free. See issues #123, #550, #2558. + """ + if _utils.CLAUDE_LOCAL_PATH.is_file() or _utils.CLAUDE_NPM_LOCAL_PATH.is_file(): + return True + return super().is_cli_available() + @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 4c90d030a1..a571a8a258 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 @@ -34,3 +36,14 @@ class KiroCliIntegration(MarkdownIntegration): "args": _KIRO_ARG_FALLBACK, "extension": ".md", } + + def is_cli_available(self) -> bool: + """Kiro currently supports both executable names. + + Prefer ``kiro-cli`` and accept the legacy ``kiro`` binary as a + compatibility fallback (see issue #2558). + """ + return ( + shutil.which(self.cli_executable) is not None + or shutil.which("kiro") is not None + ) diff --git a/tests/integrations/test_base.py b/tests/integrations/test_base.py index e78ef23613..26de86155d 100644 --- a/tests/integrations/test_base.py +++ b/tests/integrations/test_base.py @@ -3,6 +3,7 @@ import shlex import sys from types import SimpleNamespace +from unittest.mock import patch import pytest @@ -97,6 +98,42 @@ def test_uninstall_delegates_to_teardown(self, tmp_path): assert skipped == [] +class TestCliExecutableDetection: + """cli_executable / is_cli_available() — issue #2558.""" + + def test_cli_executable_defaults_to_key(self): + i = StubIntegration() + assert i.cli_executable == "stub" + + def test_cli_executable_honors_executable_env_override(self, monkeypatch): + monkeypatch.setenv("SPECKIT_INTEGRATION_STUB_EXECUTABLE", "stub-bin") + i = StubIntegration() + assert i.cli_executable == "stub-bin" + + def test_is_cli_available_true_when_executable_on_path(self): + i = StubIntegration() + with patch("specify_cli.integrations.base.shutil.which", return_value="/usr/bin/stub"): + assert i.is_cli_available() is True + + def test_is_cli_available_false_when_not_on_path(self): + i = StubIntegration() + with patch("specify_cli.integrations.base.shutil.which", return_value=None): + assert i.is_cli_available() is False + + def test_is_cli_available_uses_cli_executable_not_key(self, monkeypatch): + """An integration overriding cli_executable should be detected by + that name, not by ``self.key`` (mirrors RovoDev: key='rovodev', + executable='acli').""" + monkeypatch.setenv("SPECKIT_INTEGRATION_STUB_EXECUTABLE", "stub-bin") + i = StubIntegration() + + def fake_which(name): + return "/usr/bin/stub-bin" if name == "stub-bin" else None + + with patch("specify_cli.integrations.base.shutil.which", side_effect=fake_which): + assert i.is_cli_available() is True + + class TestMarkdownIntegration: def test_is_subclass_of_base(self): assert issubclass(MarkdownIntegration, IntegrationBase)