Resolve agent CLI executables in one place so checks match dispatch

[dhruv-15-03]

Problem

check_tool() decided whether an agent CLI was installed using per-tool special cases for claude, kiro-cli, rovodev and docker-agent. Dispatch did not use any of them. IntegrationBase.build_exec_args() calls _resolve_executable(), which returns the integration key unless an environment variable overrides it.

So the two paths could disagree, and on real machines they did:

• Claude installed by claude migrate-installer or the npm local installer lives under ~/.claude/local and is not on PATH. check_tool("claude") looked there and reported it available. _resolve_executable() returned "claude", so dispatch searched PATH and failed.
• A machine carrying only the legacy kiro binary passed check_tool("kiro-cli"), because that check accepted either name. Dispatch resolved "kiro-cli" and failed.

In both cases the preflight check says the tool is present and the run then fails to launch it.

Closes #2558.

What changed

Resolution moved onto the integrations, so the check and dispatch read one value instead of maintaining two rule sets that drift.

• ClaudeIntegration._resolve_executable() falls back to the known local install paths when the key is not on PATH. An explicit env override, or a real PATH install, still wins.
• KiroCliIntegration._resolve_executable() falls back to the legacy kiro binary on the same terms.
• IntegrationBase.is_cli_available() resolves the executable, then checks that path directly when it contains a separator, or looks the bare name up on PATH.
• DockerAgentIntegration.is_cli_available() overrides that, because Docker Agent is a docker CLI plugin rather than an executable on PATH. It uses the existing docker_agent_command() probe.
• check_tool() asks the integration when one is registered, and keeps the plain PATH lookup for non-integration tools such as git.

RovodevIntegration already overrode _resolve_executable() to return "acli", so dropping its special case from check_tool() is behaviour-preserving. It is the pattern the other two now follow.

The two workflow dispatch sites are not touched. They already do shutil.which(exec_args[0]) and substitute the result into argv, and exec_args[0] now carries the resolved executable, so correcting resolution fixes dispatch there without editing it.

Rebase and scope reduction

This branch had gone stale. main has since gained IntegrationBase._resolve_executable() independently, which is the hook this change needs, so the cli_executable property the PR originally added is no longer necessary. I reset the branch onto current main and rebuilt the change on top of what is already there. The diff is smaller as a result: no new property, only overrides of the existing one.

That reset also removed the dispatch-site migration I had pushed earlier in review. I have not restored it, and I have explained why on that thread rather than dropping it silently.

Testing

Python 3.12.9 on Windows, at 4d2ca34 with a clean tree.

pytest tests/integrations/test_base.py tests/specify_cli/test_check_tool.py -q
98 passed, 1 skipped in 3.15s

Two new tests in tests/integrations/test_base.py assert the property that was broken: a tool reported as available must yield an argv[0] that exists. One covers the Claude local install, one the legacy kiro binary. Each asserts check_tool(...), _resolve_executable() and build_exec_args(...)[0] together, so it is the dispatch argv that is being checked, not just a helper.

They fail without the source change. With only the tests applied:

git stash push -- src
pytest tests/integrations/test_base.py -k TestCliAvailabilityMatchesDispatch -q
2 failed    assert resolved == str(local_claude)
            assert resolved == "kiro"
git stash pop
2 passed

The 12 existing cases in tests/specify_cli/test_check_tool.py are unmodified and still pass. They are the contract for the behaviour being refactored.

ruff check on the touched files reports 22 findings on main and 21 with this change, so this introduces none. The earlier claim in this description that ruff was clean on the changed files was wrong; those files are not clean on main either.

I did not run the full suite to completion. It did not finish in about 35 minutes here, and neither did tests/integrations and tests/specify_cli in full, so the slow tests are not confined to one file. They appear to block on reaching the network from this machine. I would rather say that than quote a number I did not observe.

AI disclosure

This PR was authored autonomously by an AI coding agent (GitHub Copilot, Claude Sonnet 5) operating under my supervision via the dhruv-15-03 account, per this repo's disclosure guidelines in AGENTS.md.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Medium

[mnriem]

Please address Copilot feedback

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Review details

Comments suppressed due to low confidence (4)

src/specify_cli/integrations/claude/init.py:72

• is_cli_available() returns True if the local-path file exists, even if it is not executable. That can produce false positives (tool reported available, but dispatch fails with OSError). Consider checking executability (e.g., os.access(path, os.X_OK)) or using shutil.which(str(path)) for these absolute paths so the contract matches “runnable CLI” rather than “file exists”.

    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()

tests/test_workflows.py:1308

• This test verifies that preflight no longer blocks when PATH lookup fails, but it doesn’t assert that dispatch actually uses the local Claude executable path. To ensure the #2558 scenario is genuinely supported (not just preflight), assert on the subprocess.run call args (or whatever ultimately executes) that the executable resolved to fake_claude_local when shutil.which returns None.

    def test_dispatch_honors_claude_non_path_local_install(self, tmp_path):
        """Preflight must go through ``is_cli_available()`` so a Claude
        install at ``~/.claude/local/claude`` (not on ``PATH``, see #2558)
        is still detected — a bare ``shutil.which("claude")`` check would
        miss it and the step would wrongly report the CLI as absent."""
        from unittest.mock import MagicMock, patch
        from specify_cli.workflows.steps.command import CommandStep
        from specify_cli.workflows.base import StepContext, StepStatus

        fake_claude_local = tmp_path / "claude"
        fake_claude_local.touch()
        fake_missing = tmp_path / "nonexistent" / "claude"

        step = CommandStep()
        ctx = StepContext(
            inputs={"name": "login"},
            default_integration="claude",
            project_root=str(tmp_path),
        )
        config = {
            "id": "test",
            "command": "speckit.specify",
            "input": {"args": "{{ inputs.name }}"},
        }

        mock_result = MagicMock()
        mock_result.returncode = 0
        mock_result.stdout = '{"result": "done"}'
        mock_result.stderr = ""

        with patch("specify_cli._utils.CLAUDE_LOCAL_PATH", fake_claude_local), \
             patch("specify_cli._utils.CLAUDE_NPM_LOCAL_PATH", fake_missing), \
             patch("specify_cli.integrations.base.shutil.which", return_value=None), \
             patch("subprocess.run", return_value=mock_result):
            result = step.execute(config, ctx)

        assert result.status == StepStatus.COMPLETED
        assert result.output["dispatched"] is True

tests/test_workflows.py:1388

• Asserting the exact number/order of shutil.which calls is brittle (internal dispatch resolution details can change without affecting behavior). Prefer asserting the key behavioral outcome (e.g., that \"/opt/claude\" was checked at least once, and that the dispatched argv[0] is \"/opt/claude\") rather than an exact seen_which list.

        # is_cli_available() resolves the override via cli_executable and
        # checks it directly — a single shutil.which("/opt/claude") call for
        # the preflight, plus dispatch_command()'s own PATHEXT resolution.
        assert seen_which == ["/opt/claude", "/opt/claude"]

src/specify_cli/integrations/kiro_cli/init.py:49

• This duplicates the base-class detection logic. Consider return super().is_cli_available() or shutil.which(\"kiro\") is not None to keep the primary detection behavior centralized (so future changes to the default detection contract don’t need to be mirrored here).

    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
        )

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Low

[mnriem]

Please resolve conflicts

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 3
• Review effort level: Balanced

[mnriem]

Please address Copilot feedback

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot review overview

🟡 Changes recommended

Availability checks still accept non-executable files and nonexistent Docker Agent overrides.

Review effort: Balanced
Findings: 2 · 2

Open (4)

• The legacy fallback is only used for the boolean preflight. When only kiro exists, inherited…
• This reports a non-PATH install as dispatchable, but it does not make that path the executable used…
• Treat non-executable path overrides as unavailable · New
• Validate explicit docker-agent overrides before reporting availability · New

Resolved since last review (1)

• The PR description still says both workflow dispatch sites are intentionally left unchanged and…

[mnriem]

Please address Copilot feedback and fix test & lint errors

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[dhruv-15-03]

The three failing Claude-install fixtures now mark their fake binaries executable, matching the stricter availability check. Pushed the fixture-only fix; the CI-pinned Ruff command passes locally. Waiting for the new CI results.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Explicit default-name overrides are not preserved, and the linked documentation criterion remains unmet.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Skip non-executable candidates during CLI resolution

Previously missed (3)

In code that hasn't changed since last review

Explicit default executable override incorrectly falls back to local path

src/​specify_cli/​integrations/​claude/​__init__.py:84

An explicit override equal to the default key is indistinguishable from an unset override here. For example, with SPECKIT_INTEGRATION_CLAUDE_EXECUTABLE=claude, no claude on PATH, and a local install present, this falls through and returns the local path even though the base override contract and PR description say the explicit value wins. Check whether the environment variable is set before applying local fallbacks.

Explicit kiro-cli override incorrectly falls back to legacy kiro

src/​specify_cli/​integrations/​kiro_cli/​__init__.py:51

This comparison does not preserve an explicit override when its value is kiro-cli: if only legacy kiro is on PATH, _resolve_executable() silently replaces the requested executable with kiro. That contradicts the inherited executable-override contract and prevents operators from explicitly requiring the modern binary. Detect a nonblank SPECKIT_INTEGRATION_KIRO_CLI_EXECUTABLE value before applying the legacy fallback.

Document executable resolution and availability override mechanisms

src/​specify_cli/​integrations/​base.py:315

The linked issue's acceptance criteria explicitly require AGENTS.md to document the cli_executable/is_cli_available() override mechanism, but this new extension point is not documented there (nor in design/integration.md). Add contributor guidance explaining when integrations should override executable resolution versus availability.

[mnriem]

Please address the previously missed Copilot feedback

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[dhruv-15-03]

Addressed the three previously missed items at 1c2a2b7a.

Root cause: _resolve_executable() returned override or self.key, so claude and kiro_cli inferred "an override was set" from resolved != self.key. That inference is lossy exactly when the override equals the integration key, so SPECKIT_INTEGRATION_CLAUDE_EXECUTABLE=claude was silently replaced by the ~/.claude local install, and SPECKIT_INTEGRATION_KIRO_CLI_EXECUTABLE=kiro-cli by the legacy kiro binary.

Added IntegrationBase._executable_override(), which returns the override or None, and both subclasses now ask it instead of comparing strings. Whitespace-only values still count as unset, and PATH, default and no-override fallbacks are unchanged, as is the executable-candidate ordering check. copilot and rovodev also read the override, but each falls back to a different default, so the comparison is not lossy there; checked and left untouched.

Regression tests cover both default-key override cases and fail before this change, plus non-default and whitespace-override coverage. The resolution order and the override contract are now documented in design/integration.md, cross-referenced from AGENTS.md.

Locally: 138 passed / 4 skipped across tests/integrations/test_base.py, tests/integrations/test_extra_args.py and tests/specify_cli/test_check_tool.py; 70 passed across the Claude and Kiro CLI integration suites; the CI-pinned ruff 0.15.0 check src tests reports no findings. The new head still needs its own CI result.

[Copilot]

Copilot review overview

🟡 Changes recommended

Docker Agent still ignores an explicit override when its value equals the integration key.

Review effort: Balanced
Findings: 1

Open (1)

• Honor explicit docker-agent executable override · New

[mnriem]

Please address Copilot feedback

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Path-valued overrides can be incorrectly rejected on Windows, and the documented resolution order contradicts the implementation.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Honor explicit docker-agent executable override

Previously missed (1)

In code that hasn't changed since last review

Prioritize PATH-installed keys before integration fallbacks

design/​integration.md:64

This resolution order is opposite to both implementations: Claude and Kiro first keep self.key when it is found on PATH, and only use their local/legacy fallback when the key is absent. As written, this contract tells future integrations to let a fallback supersede a normal PATH install. Document the PATH-installed key before the integration fallback, retain the unresolved key as the final default, and update the subsequent step reference.

[Copilot]

Copilot review overview

🟡 Changes recommended

The documented resolution precedence contradicts the new Claude and Kiro implementations.

Review effort: Balanced
Findings: 1

Open (1)

• Documented key precedence contradicts Claude and Kiro implementations · New

[mnriem]

Please address Copilot feedback and fix test & lint errors