diff --git a/src/specify_cli/integrations/base.py b/src/specify_cli/integrations/base.py index 7ce107caec..50ea6e95fb 100644 --- a/src/specify_cli/integrations/base.py +++ b/src/specify_cli/integrations/base.py @@ -13,7 +13,9 @@ from __future__ import annotations +import os import re +import shlex import shutil from abc import ABC from dataclasses import dataclass @@ -138,6 +140,43 @@ def build_exec_args( """ return None + def _apply_extra_args_env_var(self, args: list[str]) -> None: + """Append `SPECKIT_INTEGRATION__EXTRA_ARGS` env-var value to *args*. + + Operators can inject extra CLI flags into the spawned agent + subprocess by setting an env var named for the integration key, + e.g. `SPECKIT_INTEGRATION_CLAUDE_EXTRA_ARGS="--dangerously-skip-permissions"`. + The `INTEGRATION` segment scopes the variable to this subsystem + so it does not collide with other Spec Kit env-var namespaces. + Hyphens in the integration key are replaced with underscores + and the key is uppercased + (e.g. `kiro-cli` → `SPECKIT_INTEGRATION_KIRO_CLI_EXTRA_ARGS`). + + Useful in CI / non-interactive contexts where the spawned agent + needs flags that change its prompt-handling behaviour. + Default behaviour (env var unset or whitespace-only) is a no-op + — *args* is unchanged. Multi-token values are parsed via + `shlex.split`. + + See issue #2595. + """ + env_name = ( + f"SPECKIT_INTEGRATION_{self.key.upper().replace('-', '_')}_EXTRA_ARGS" + ) + extra = os.environ.get(env_name, "").strip() + if not extra: + return + try: + tokens = shlex.split(extra) + except ValueError as exc: + raise ValueError( + f"{env_name} is not parseable as a POSIX-quoted command line " + f"(value: {extra!r}). shlex reported: {exc}. " + f"Use single or double quotes to group multi-word values, e.g. " + f'{env_name}=\'--flag "value with spaces"\'.' + ) from exc + args.extend(tokens) + def build_command_invocation(self, command_name: str, args: str = "") -> str: """Build the native slash-command invocation for a Spec Kit command. @@ -851,6 +890,7 @@ def build_exec_args( if not self.config or not self.config.get("requires_cli"): return None args = [self.key, "-p", prompt] + self._apply_extra_args_env_var(args) if model: args.extend(["--model", model]) if output_json: @@ -938,6 +978,7 @@ def build_exec_args( if not self.config or not self.config.get("requires_cli"): return None args = [self.key, "-p", prompt] + self._apply_extra_args_env_var(args) if model: args.extend(["-m", model]) if output_json: @@ -1356,6 +1397,7 @@ def build_exec_args( if not self.config or not self.config.get("requires_cli"): return None args = [self.key, "-p", prompt] + self._apply_extra_args_env_var(args) if model: args.extend(["--model", model]) if output_json: diff --git a/src/specify_cli/integrations/codex/__init__.py b/src/specify_cli/integrations/codex/__init__.py index 5efdcf877a..2e4f0be0ae 100644 --- a/src/specify_cli/integrations/codex/__init__.py +++ b/src/specify_cli/integrations/codex/__init__.py @@ -52,7 +52,12 @@ def build_exec_args( output_json: bool = True, ) -> list[str] | None: # Codex uses ``codex exec "prompt"`` for non-interactive mode. - args: list[str] = ["codex", "exec", prompt] + # Use ``self.key`` so the executable name stays coupled to the + # env-var lookup (which also derives from ``self.key``), matching + # the pattern in Devin/Opencode and avoiding drift if the key + # ever changes. + args: list[str] = [self.key, "exec", prompt] + self._apply_extra_args_env_var(args) if model: args.extend(["--model", model]) if output_json: diff --git a/src/specify_cli/integrations/copilot/__init__.py b/src/specify_cli/integrations/copilot/__init__.py index 4e8f6e16b9..2808bac83f 100644 --- a/src/specify_cli/integrations/copilot/__init__.py +++ b/src/specify_cli/integrations/copilot/__init__.py @@ -149,6 +149,7 @@ def build_exec_args( # (default: enabled). The deprecated SPECKIT_ALLOW_ALL_TOOLS # is also honoured as a fallback. args = [_copilot_executable(), "-p", prompt] + self._apply_extra_args_env_var(args) if _allow_all(): args.append("--yolo") if model: @@ -217,6 +218,11 @@ def dispatch_command( prompt = args or "" cli_args = [_copilot_executable(), "-p", prompt] + # Honour SPECKIT_INTEGRATION_COPILOT_EXTRA_ARGS for real workflow + # runs. `dispatch_command` builds cli_args inline rather than + # going through `build_exec_args`, so the hook must be invoked + # here too — otherwise the env var is silently ignored. + self._apply_extra_args_env_var(cli_args) if not skills_mode: cli_args.extend(["--agent", agent_name]) if _allow_all(): diff --git a/src/specify_cli/integrations/devin/__init__.py b/src/specify_cli/integrations/devin/__init__.py index f5656e4aef..f8a57d9ceb 100644 --- a/src/specify_cli/integrations/devin/__init__.py +++ b/src/specify_cli/integrations/devin/__init__.py @@ -49,6 +49,7 @@ def build_exec_args( kept on the integration for tool detection. """ args = [self.key, "-p", prompt] + self._apply_extra_args_env_var(args) if model: args.extend(["--model", model]) return args diff --git a/src/specify_cli/integrations/opencode/__init__.py b/src/specify_cli/integrations/opencode/__init__.py index 4fa9c724ac..c5dfe03e07 100644 --- a/src/specify_cli/integrations/opencode/__init__.py +++ b/src/specify_cli/integrations/opencode/__init__.py @@ -29,6 +29,11 @@ def build_exec_args( output_json: bool = True, ) -> list[str] | None: args = [self.key, "run"] + # Apply operator-injected extra args before the prompt-derived + # --command and the canonical --format/-m flags so Spec Kit's + # later appends remain authoritative under repeated-flag CLI + # semantics. + self._apply_extra_args_env_var(args) message = prompt if prompt.startswith("/"): diff --git a/tests/integrations/test_extra_args.py b/tests/integrations/test_extra_args.py new file mode 100644 index 0000000000..0fc9196f5c --- /dev/null +++ b/tests/integrations/test_extra_args.py @@ -0,0 +1,480 @@ +"""Tests for the per-integration `SPECKIT_INTEGRATION__EXTRA_ARGS` env-var hook. + +The hook is implemented in `IntegrationBase._apply_extra_args_env_var` +and wired into every concrete `build_exec_args` — +`MarkdownIntegration`, `TomlIntegration`, `SkillsIntegration`, plus the +overrides in Codex, Devin, Opencode and Copilot. These tests cover both +the shared mechanism (via `SkillsIntegration` stubs near the top of the +file) and each override integration end-to-end (further down). See +issue #2595.""" + +import os + +import pytest + +from specify_cli.integrations.base import ( + MarkdownIntegration, + SkillsIntegration, + TomlIntegration, +) + + +class _ClaudeStub(SkillsIntegration): + """Minimal Claude-like SkillsIntegration for testing.""" + + key = "claude" + config = { + "name": "Claude (test stub)", + "folder": ".claude/", + "commands_subdir": "skills", + "install_url": None, + "requires_cli": True, + } + registrar_config = { + "dir": ".claude/skills", + "format": "markdown", + "args": "$ARGUMENTS", + "extension": "/SKILL.md", + } + context_file = "CLAUDE.md" + + +class _KiroCliStub(SkillsIntegration): + """SkillsIntegration with a hyphenated key to exercise key + normalization (`kiro-cli` → `KIRO_CLI`).""" + + key = "kiro-cli" + config = { + "name": "Kiro CLI (test stub)", + "folder": ".kiro/", + "commands_subdir": "commands", + "install_url": None, + "requires_cli": True, + } + registrar_config = { + "dir": ".kiro/commands", + "format": "markdown", + "args": "$ARGUMENTS", + "extension": ".md", + } + context_file = "KIRO.md" + + +class _NoCliStub(SkillsIntegration): + """SkillsIntegration with requires_cli=False — build_exec_args + must return None and the env-var hook must not fire.""" + + key = "no-cli" + config = { + "name": "No-CLI agent (test stub)", + "folder": ".no-cli/", + "commands_subdir": "commands", + "install_url": None, + "requires_cli": False, + } + registrar_config = { + "dir": ".no-cli/commands", + "format": "markdown", + "args": "$ARGUMENTS", + "extension": ".md", + } + context_file = "NOCLI.md" + + +class _MarkdownAgentStub(MarkdownIntegration): + """Bare MarkdownIntegration subclass — does NOT override + `build_exec_args`. Locks the base implementation in + `MarkdownIntegration.build_exec_args` for the common case + (most concrete integrations: Amp, Auggie, Generic, …).""" + + key = "md-agent" + config = { + "name": "Markdown agent (test stub)", + "folder": ".md-agent/", + "commands_subdir": "commands", + "install_url": None, + "requires_cli": True, + } + registrar_config = { + "dir": ".md-agent/commands", + "format": "markdown", + "args": "$ARGUMENTS", + "extension": ".md", + } + context_file = "MDAGENT.md" + + +class _TomlAgentStub(TomlIntegration): + """Bare TomlIntegration subclass — does NOT override + `build_exec_args`. Locks the base implementation in + `TomlIntegration.build_exec_args` (Gemini, Tabnine).""" + + key = "toml-agent" + config = { + "name": "TOML agent (test stub)", + "folder": ".toml-agent/", + "commands_subdir": "commands", + "install_url": None, + "requires_cli": True, + } + registrar_config = { + "dir": ".toml-agent/commands", + "format": "toml", + "args": "$ARGUMENTS", + "extension": ".toml", + } + context_file = "TOMLAGENT.md" + + +@pytest.fixture(autouse=True) +def _clean_extra_args_env(monkeypatch): + """Strip any leaked SPECKIT_INTEGRATION_*_EXTRA_ARGS from the test + env so a developer's shell setting doesn't pollute results.""" + for key in list(os.environ): + if key.startswith("SPECKIT_INTEGRATION_") and key.endswith( + "_EXTRA_ARGS" + ): + monkeypatch.delenv(key, raising=False) + + +def test_env_var_unset_byte_identical_argv(): + """Default behaviour: env var unset → no extra args inserted. + + Locks the backward-compatibility guarantee that existing + operators see no change. + """ + args = _ClaudeStub().build_exec_args("hello prompt") + assert args == ["claude", "-p", "hello prompt", "--output-format", "json"] + + +def test_env_var_set_flag_inserted_before_model_and_output_format( + monkeypatch, +): + monkeypatch.setenv( + "SPECKIT_INTEGRATION_CLAUDE_EXTRA_ARGS", "--dangerously-skip-permissions" + ) + args = _ClaudeStub().build_exec_args("hello prompt", model="sonnet") + assert args == [ + "claude", + "-p", + "hello prompt", + "--dangerously-skip-permissions", + "--model", + "sonnet", + "--output-format", + "json", + ] + + +def test_env_var_multi_token_parsed_via_shlex(monkeypatch): + monkeypatch.setenv( + "SPECKIT_INTEGRATION_CLAUDE_EXTRA_ARGS", + "--dangerously-skip-permissions --max-turns 3", + ) + args = _ClaudeStub().build_exec_args("p") + assert args == [ + "claude", + "-p", + "p", + "--dangerously-skip-permissions", + "--max-turns", + "3", + "--output-format", + "json", + ] + + +def test_malformed_quoting_raises_actionable_value_error(monkeypatch): + """An unmatched quote in the env-var value must surface a clear + error naming the offending env var and showing the invalid value, + rather than crashing workflow dispatch with a bare shlex traceback.""" + monkeypatch.setenv( + "SPECKIT_INTEGRATION_CLAUDE_EXTRA_ARGS", + '--flag "unterminated', + ) + with pytest.raises(ValueError) as excinfo: + _ClaudeStub().build_exec_args("p") + msg = str(excinfo.value) + assert "SPECKIT_INTEGRATION_CLAUDE_EXTRA_ARGS" in msg + assert "--flag \"unterminated" in msg + + +def test_env_var_empty_or_whitespace_is_noop(monkeypatch): + """An env var set to '' or ' ' is treated as unset.""" + monkeypatch.setenv("SPECKIT_INTEGRATION_CLAUDE_EXTRA_ARGS", " ") + args = _ClaudeStub().build_exec_args("p") + assert args == ["claude", "-p", "p", "--output-format", "json"] + + +def test_other_integration_env_var_ignored(monkeypatch): + """`SPECKIT_INTEGRATION_GEMINI_EXTRA_ARGS` set must NOT leak into + Claude's argv (per-integration scoping).""" + monkeypatch.setenv("SPECKIT_INTEGRATION_GEMINI_EXTRA_ARGS", "--gemini-only-flag") + args = _ClaudeStub().build_exec_args("p") + assert args == ["claude", "-p", "p", "--output-format", "json"] + + +def test_key_normalization_hyphen_to_underscore_uppercase(monkeypatch): + """`kiro-cli` key looks up `SPECKIT_INTEGRATION_KIRO_CLI_EXTRA_ARGS` + (hyphens replaced with underscores, then uppercased).""" + monkeypatch.setenv( + "SPECKIT_INTEGRATION_KIRO_CLI_EXTRA_ARGS", "--some-kiro-flag" + ) + args = _KiroCliStub().build_exec_args("p") + assert args == [ + "kiro-cli", + "-p", + "p", + "--some-kiro-flag", + "--output-format", + "json", + ] + + +def test_requires_cli_false_returns_none(monkeypatch): + """`requires_cli: False` short-circuits to None — the env-var + hook is never reached and no argv is built.""" + monkeypatch.setenv("SPECKIT_INTEGRATION_NO_CLI_EXTRA_ARGS", "--should-not-appear") + assert _NoCliStub().build_exec_args("p") is None + + +# --------------------------------------------------------------------------- +# Base-class coverage +# +# Most integrations inherit `build_exec_args` from `MarkdownIntegration` +# or `TomlIntegration` without overriding it. The tests above use +# `SkillsIntegration` stubs (which share the same hook mechanism) — these +# tests exercise the two other base implementations directly so all three +# concrete bases are covered. +# --------------------------------------------------------------------------- + + +def test_markdown_integration_base_honours_extra_args(monkeypatch): + """A bare `MarkdownIntegration` subclass — which does not override + `build_exec_args` — must honour the env var via the base + implementation. Covers the most common integration pattern.""" + monkeypatch.setenv( + "SPECKIT_INTEGRATION_MD_AGENT_EXTRA_ARGS", "--debug --max-tokens 100" + ) + args = _MarkdownAgentStub().build_exec_args("p") + assert args == [ + "md-agent", + "-p", + "p", + "--debug", + "--max-tokens", + "100", + "--output-format", + "json", + ] + + +def test_toml_integration_base_honours_extra_args(monkeypatch): + """A bare `TomlIntegration` subclass — which does not override + `build_exec_args` — must honour the env var via the base + implementation. Covers Gemini/Tabnine-style integrations.""" + monkeypatch.setenv( + "SPECKIT_INTEGRATION_TOML_AGENT_EXTRA_ARGS", "--yolo" + ) + args = _TomlAgentStub().build_exec_args("p", model="gemini-pro") + # TomlIntegration uses `-m` for model (vs Markdown's `--model`). + assert args == [ + "toml-agent", + "-p", + "p", + "--yolo", + "-m", + "gemini-pro", + "--output-format", + "json", + ] + + +# --------------------------------------------------------------------------- +# Override-integration coverage +# +# CodexIntegration, DevinIntegration, OpencodeIntegration and +# CopilotIntegration each override `build_exec_args` rather than using the +# base implementations. The env-var hook must be wired into every override +# so the documented behaviour ("works for every requires_cli integration") +# is honoured. These tests lock that contract per integration. +# --------------------------------------------------------------------------- + + +def test_codex_integration_honours_extra_args(monkeypatch): + from specify_cli.integrations.codex import CodexIntegration + + monkeypatch.setenv("SPECKIT_INTEGRATION_CODEX_EXTRA_ARGS", "--sandbox read-only") + args = CodexIntegration().build_exec_args("p", model="gpt-5") + assert args == [ + "codex", + "exec", + "p", + "--sandbox", + "read-only", + "--model", + "gpt-5", + "--json", + ] + + +def test_devin_integration_honours_extra_args(monkeypatch): + from specify_cli.integrations.devin import DevinIntegration + + monkeypatch.setenv("SPECKIT_INTEGRATION_DEVIN_EXTRA_ARGS", "--no-confirm") + args = DevinIntegration().build_exec_args("p") + assert args == ["devin", "-p", "p", "--no-confirm"] + + +def test_opencode_integration_honours_extra_args(monkeypatch): + from specify_cli.integrations.opencode import OpencodeIntegration + + monkeypatch.setenv("SPECKIT_INTEGRATION_OPENCODE_EXTRA_ARGS", "--quiet") + args = OpencodeIntegration().build_exec_args("p") + assert args == [ + "opencode", + "run", + "--quiet", + "--format", + "json", + "p", + ] + + +def test_opencode_extra_args_cannot_clobber_prompt_derived_command( + monkeypatch, +): + """Operator-injected extra args must appear BEFORE the prompt-derived + ``--command `` so that Spec Kit's command selection wins under + repeated-flag CLI semantics (last value typically takes precedence). + + Locks against the regression where an operator setting + ``SPECKIT_INTEGRATION_OPENCODE_EXTRA_ARGS="--command malicious"`` could redirect + a slash-prefixed prompt to a different command. + """ + from specify_cli.integrations.opencode import OpencodeIntegration + + monkeypatch.setenv( + "SPECKIT_INTEGRATION_OPENCODE_EXTRA_ARGS", "--command operator-override" + ) + args = OpencodeIntegration().build_exec_args("/speckit body text") + # Prompt-derived "--command speckit" appears AFTER the + # operator-injected one, so a CLI that resolves repeated flags + # last-wins will honour Spec Kit's choice. + assert args == [ + "opencode", + "run", + "--command", + "operator-override", + "--command", + "speckit", + "--format", + "json", + "body text", + ] + + +def test_copilot_integration_honours_extra_args(monkeypatch): + from specify_cli.integrations.copilot import ( + CopilotIntegration, + _copilot_executable, + ) + + # Disable --yolo so the argv shape stays deterministic. + monkeypatch.setenv("SPECKIT_COPILOT_ALLOW_ALL_TOOLS", "0") + monkeypatch.setenv( + "SPECKIT_INTEGRATION_COPILOT_EXTRA_ARGS", "--allow-tool 'shell(echo)'" + ) + args = CopilotIntegration().build_exec_args("p") + # `_copilot_executable()` returns "copilot.cmd" on Windows and + # "copilot" elsewhere; the test must mirror that to stay portable. + assert args == [ + _copilot_executable(), + "-p", + "p", + "--allow-tool", + "shell(echo)", + "--output-format", + "json", + ] + + +# --------------------------------------------------------------------------- +# `dispatch_command` end-to-end coverage +# +# Workflow execution calls `impl.dispatch_command(...)`, not +# `build_exec_args` directly. `IntegrationBase.dispatch_command` delegates +# to `build_exec_args` (so the override fixes above flow through), but +# `CopilotIntegration` overrides `dispatch_command` and constructs +# `cli_args` inline — the hook must be invoked there too or the env var +# is silently ignored at workflow runtime. These tests monkeypatch +# `subprocess.run` and assert the env-var args reach the executed argv. +# --------------------------------------------------------------------------- + + +class _RunCapture: + """Test double that captures argv passed to subprocess.run.""" + + def __init__(self): + self.captured_args: list[str] | None = None + + def __call__(self, args, **kwargs): + self.captured_args = list(args) + + class _Result: + returncode = 0 + stdout = "" + stderr = "" + + return _Result() + + +def test_copilot_dispatch_command_includes_extra_args(monkeypatch): + """Locks the bypass fix: `CopilotIntegration.dispatch_command` + must honour `SPECKIT_INTEGRATION_COPILOT_EXTRA_ARGS`, not just `build_exec_args`. + """ + import subprocess + + from specify_cli.integrations.copilot import CopilotIntegration + + capture = _RunCapture() + monkeypatch.setattr(subprocess, "run", capture) + monkeypatch.setenv("SPECKIT_COPILOT_ALLOW_ALL_TOOLS", "0") + monkeypatch.setenv( + "SPECKIT_INTEGRATION_COPILOT_EXTRA_ARGS", "--allow-tool 'shell(echo)'" + ) + + CopilotIntegration().dispatch_command( + "speckit.plan", args="body", stream=False + ) + + assert capture.captured_args is not None + # Hook inserted between `-p prompt` and the canonical Copilot flags. + p_idx = capture.captured_args.index("-p") + agent_idx = capture.captured_args.index("--agent") + extra_idx = capture.captured_args.index("--allow-tool") + assert p_idx < extra_idx < agent_idx + assert "shell(echo)" in capture.captured_args + + +def test_codex_dispatch_command_includes_extra_args(monkeypatch): + """Lock the inherited `IntegrationBase.dispatch_command` path: + Codex (and by transitivity Devin, Opencode) flow through + `build_exec_args`, so the env var must reach argv at workflow + runtime. + """ + import subprocess + + from specify_cli.integrations.codex import CodexIntegration + + capture = _RunCapture() + monkeypatch.setattr(subprocess, "run", capture) + monkeypatch.setenv("SPECKIT_INTEGRATION_CODEX_EXTRA_ARGS", "--sandbox read-only") + + CodexIntegration().dispatch_command( + "speckit.plan", args="body", stream=False + ) + + assert capture.captured_args is not None + assert "--sandbox" in capture.captured_args + assert "read-only" in capture.captured_args