diff --git a/CHANGELOG.md b/CHANGELOG.md index e517a40e3..09dcd98cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ### 2.12.0 (Unreleased) ### Features/Bug Fixes +* fix(providers): cut per-call cost of CLI-provider scans and default their LLM fan-out to 2 (#808) * fix(taint): key TT1-TT6 taint by lexical scope and follow direct calls to functions and methods defined in the same file, so a tainted variable no longer taints same-named variables in other functions (#799). Message-glob baseline rules keep matching TT findings whose rule and message are unchanged; exact baseline fingerprints are bound to the scanner version and suppress nothing after an upgrade (see docs/SUPPRESSION.md), so they must be regenerated in any case. A TT message changes where the earlier message came from a same-named variable in another scope (it now names the variable, source and line that reach the sink); where a more severe source raises the rule (TT2 to TT3 or TT4, TT4 to TT3, TT2 to TT5 or TT6), and at such a sink, where a weaker finding is subsumed or now cites another variable; and where an identifier longer than 120 characters is now shortened. Re-accept those findings or regenerate the baseline. `match_fingerprint` changes only when the rule does. * fix(static): stop code comparisons after removal verbs from reading as unterminated tag-marker directives (#777) * fix(baseline): fingerprint every occurrence of a deduplicated finding so the next scan suppresses all of them (#633) diff --git a/skills/skill-inspector/SKILL.md b/skills/skill-inspector/SKILL.md index 25f071e28..a968a585f 100644 --- a/skills/skill-inspector/SKILL.md +++ b/skills/skill-inspector/SKILL.md @@ -22,6 +22,7 @@ Do not rely on the numeric score alone. A low score can miss semantic risk, and - Run SkillSpector first when the `skillspector` CLI is available. - If `skillspector` is missing, say so clearly and continue with manual source review. - Do not install tools, dependencies, or runtimes silently. +- Keep `--no-llm` unless the user asks for LLM analysis. Do not choose or change `SKILLSPECTOR_PROVIDER` on your own. The `claude_cli`, `codex_cli`, `gemini_cli`, and `opencode_cli` providers start a separate agent session for every LLM call, billed to the user's own plan, so ask before using them and do not run several such scans in parallel. - Do not execute scripts from the target skill. - Use read-only inspection commands such as `find`, `rg`, `sed`, `jq`, `file`, and `git diff`. - Read source around every high-signal finding instead of trusting the scanner summary alone. diff --git a/src/skillspector/llm_analyzer_base.py b/src/skillspector/llm_analyzer_base.py index cc661f307..470f6d3a9 100644 --- a/src/skillspector/llm_analyzer_base.py +++ b/src/skillspector/llm_analyzer_base.py @@ -71,12 +71,15 @@ from skillspector.logging_config import get_logger from skillspector.model_info import get_max_input_tokens from skillspector.models import Finding -from skillspector.providers import get_active_provider +from skillspector.providers import UnknownProviderError, get_active_provider, has_cli_capability from skillspector.providers.gemini import GeminiProvider logger = get_logger(__name__) DEFAULT_MAX_LLM_CONCURRENCY = 10 +# A CLI-provider call is a whole agent session on the user's own plan, so the +# default fan-out is much smaller than for an HTTP API. +DEFAULT_CLI_MAX_LLM_CONCURRENCY = 2 API_CONNECTION_MAX_RETRIES = 3 API_CONNECTION_RETRY_DELAYS_SECONDS = (0.5, 1.0, 2.0) RATE_LIMIT_RETRY_DELAYS_SECONDS = (5.0, 15.0, 30.0) @@ -536,10 +539,28 @@ def _shared_limiter(limit: int) -> _GlobalLLMLimiter: return limiter +def _default_max_concurrency() -> int: + """Return the fan-out default for the active provider. + + CLI providers get :data:`DEFAULT_CLI_MAX_LLM_CONCURRENCY` because every + request starts a separate agent session that counts against the user's + plan. An unknown provider keeps the HTTP default; provider selection + reports that error itself. + """ + try: + provider = get_active_provider() + except UnknownProviderError: + return DEFAULT_MAX_LLM_CONCURRENCY + if has_cli_capability(provider): + return DEFAULT_CLI_MAX_LLM_CONCURRENCY + return DEFAULT_MAX_LLM_CONCURRENCY + + def resolve_max_concurrency() -> int: """Resolve the LLM fan-out concurrency from ``SKILLSPECTOR_MAX_LLM_CONCURRENCY``. - Defaults to :data:`DEFAULT_MAX_LLM_CONCURRENCY`. Users on rate-limited + Defaults to :data:`DEFAULT_MAX_LLM_CONCURRENCY`, or to + :data:`DEFAULT_CLI_MAX_LLM_CONCURRENCY` for CLI providers. Users on rate-limited providers (free tiers with a low RPM) can set it to ``1`` to serialize requests across every analyzer instead of bursting up to 10 in parallel — a burst that otherwise guarantees 429s, and 429'd batches are dropped from the result (see the @@ -548,16 +569,17 @@ def resolve_max_concurrency() -> int: """ raw = os.environ.get("SKILLSPECTOR_MAX_LLM_CONCURRENCY", "").strip() if not raw: - return DEFAULT_MAX_LLM_CONCURRENCY + return _default_max_concurrency() try: value = int(raw) except ValueError: + default = _default_max_concurrency() logger.warning( "Invalid SKILLSPECTOR_MAX_LLM_CONCURRENCY=%r (not an int); using %d", raw, - DEFAULT_MAX_LLM_CONCURRENCY, + default, ) - return DEFAULT_MAX_LLM_CONCURRENCY + return default if value < 1: logger.warning("SKILLSPECTOR_MAX_LLM_CONCURRENCY=%d < 1; clamping to 1", value) return 1 diff --git a/src/skillspector/llm_utils.py b/src/skillspector/llm_utils.py index 6cbed29e4..4792103fa 100644 --- a/src/skillspector/llm_utils.py +++ b/src/skillspector/llm_utils.py @@ -614,6 +614,12 @@ def get_chat_model( """ provider = get_active_provider() if has_cli_capability(provider): + # Every CLI call is a full agent session on the user's plan. Warn here, + # before the analyzers fan out; the scan path only probes availability + # from the report node, after the calls have run. + warn_session_cost = getattr(provider, "warn_session_cost", None) + if callable(warn_session_cost): + warn_session_cost() resolved_model = model or provider.resolve_model() chat_model = AgentCLIChatModel( provider, diff --git a/src/skillspector/providers/_agent_cli.py b/src/skillspector/providers/_agent_cli.py index 7d2a44319..5eb29bc84 100644 --- a/src/skillspector/providers/_agent_cli.py +++ b/src/skillspector/providers/_agent_cli.py @@ -183,6 +183,15 @@ def _validate_model_label(model: str) -> str: # --------------------------------------------------------------------------- +# Fixed text, never derived from untrusted input. Short on purpose: it replaces +# Claude Code's default agent prompt, which is sent again on every call. +_CLAUDE_SYSTEM_PROMPT = ( + "You are a non-interactive text analysis component. You have no tools. " + "Follow the instructions in the user message and reply only with the " + "requested output." +) + + def _build_claude_argv(binary: str, model: str, max_output_tokens: int) -> list[str]: """Build the argv list for a capability-stripped ``claude -p`` call. @@ -231,6 +240,23 @@ def _build_claude_argv(binary: str, model: str, max_output_tokens: int) -> list[ ``--disable-slash-commands`` Prevents skill/plugin invocations from within the sandboxed call. + ``--tools ""`` + Removes every built-in tool definition from the model's context. + ``--allowed-tools ""`` only denies tool *use*; without ``--tools ""`` + each call still carries the full tool schemas, which a scan pays for + once per LLM call. + + ``--system-prompt <_CLAUDE_SYSTEM_PROMPT>`` + Replaces Claude Code's default agent system prompt, which describes + tools and coding workflows this call cannot use. The analyzer's own + instructions still arrive on stdin. Together with ``--tools ""`` this + is most of the fixed per-call startup cost. + + ``--no-session-persistence`` + Do not write a session transcript. Each call runs in a fresh temporary + cwd, so without this every call leaves a new project folder under + ``~/.claude/projects``. + Deliberately NOT included: - ``--dangerously-skip-permissions`` / ``--allow-dangerously-skip-permissions`` — explicitly forbidden. @@ -256,6 +282,11 @@ def _build_claude_argv(binary: str, model: str, max_output_tokens: int) -> list[ "--strict-mcp-config", "--setting-sources=", "--disable-slash-commands", + "--tools", + "", + "--system-prompt", + _CLAUDE_SYSTEM_PROMPT, + "--no-session-persistence", ] diff --git a/src/skillspector/providers/_agent_cli_base.py b/src/skillspector/providers/_agent_cli_base.py index 624e503d0..f10468498 100644 --- a/src/skillspector/providers/_agent_cli_base.py +++ b/src/skillspector/providers/_agent_cli_base.py @@ -28,8 +28,28 @@ import os +from skillspector.logging_config import get_logger from skillspector.providers import _agent_cli, registry +logger = get_logger(__name__) + +# CLIs already warned about in this process, so a scan warns once, not per probe. +_cost_warned: set[str] = set() + + +def _warn_session_cost_once(binary_name: str) -> None: + if binary_name in _cost_warned: + return + _cost_warned.add(binary_name) + logger.warning( + "LLM analysis via the local %r CLI starts a separate %s session for every " + "LLM call, and each one counts against your own plan. A scan can make dozens " + "of calls per skill. Use --no-llm for a static-only scan, or lower " + "SKILLSPECTOR_MAX_LLM_CONCURRENCY to slow the fan-out.", + binary_name, + binary_name, + ) + class AgentCLIProviderBase: """Base for providers that drive a local agent CLI (no API key needed).""" @@ -52,8 +72,25 @@ def resolve_credentials(self) -> tuple[str, str | None] | None: # -- Availability -------------------------------------------------------- def is_available(self) -> tuple[bool, str | None]: - """Binary on PATH AND authenticated (delegates to the registry probe).""" - return _agent_cli.is_available(self.BINARY_NAME) + """Binary on PATH AND authenticated (delegates to the registry probe). + + Warns once per process when the CLI is usable (see + :meth:`warn_session_cost`), so callers that probe up front, such as + the MCP server, warn before any scan starts. + """ + available, error = _agent_cli.is_available(self.BINARY_NAME) + if available: + self.warn_session_cost() + return available, error + + def warn_session_cost(self) -> None: + """Warn once per process that each LLM call is a full agent session. + + ``get_chat_model()`` calls this before the analyzers fan out, because + ``skillspector scan`` only probes :meth:`is_available` from the report + node, after every LLM call has run. + """ + _warn_session_cost_once(self.BINARY_NAME) # -- Transport ----------------------------------------------------------- diff --git a/tests/nodes/test_llm_analyzer_base.py b/tests/nodes/test_llm_analyzer_base.py index f73418166..77c21deaf 100644 --- a/tests/nodes/test_llm_analyzer_base.py +++ b/tests/nodes/test_llm_analyzer_base.py @@ -64,6 +64,7 @@ from skillspector.inspection_ledger import LedgerOutcome, LedgerReason, finalize_ledger from skillspector.llm_analyzer_base import ( API_CONNECTION_MAX_RETRIES, + DEFAULT_CLI_MAX_LLM_CONCURRENCY, DEFAULT_MAX_LLM_CONCURRENCY, OUTPUT_LANGUAGE_MAX_LENGTH, PROVIDER_RETRY_AFTER_MAX_SECONDS, @@ -131,6 +132,21 @@ def test_below_one_clamps_to_one(self, monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setenv("SKILLSPECTOR_MAX_LLM_CONCURRENCY", "0") assert resolve_max_concurrency() == 1 + def test_cli_provider_unset_uses_cli_default(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("SKILLSPECTOR_MAX_LLM_CONCURRENCY", raising=False) + monkeypatch.setenv("SKILLSPECTOR_PROVIDER", "claude_cli") + assert resolve_max_concurrency() == DEFAULT_CLI_MAX_LLM_CONCURRENCY + + def test_cli_provider_explicit_value_wins(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("SKILLSPECTOR_PROVIDER", "claude_cli") + monkeypatch.setenv("SKILLSPECTOR_MAX_LLM_CONCURRENCY", "6") + assert resolve_max_concurrency() == 6 + + def test_unknown_provider_uses_http_default(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("SKILLSPECTOR_MAX_LLM_CONCURRENCY", raising=False) + monkeypatch.setenv("SKILLSPECTOR_PROVIDER", "not-a-provider") + assert resolve_max_concurrency() == DEFAULT_MAX_LLM_CONCURRENCY + class TestOutputLanguage: def test_unset_is_disabled(self, monkeypatch: pytest.MonkeyPatch) -> None: diff --git a/tests/unit/test_agent_cli.py b/tests/unit/test_agent_cli.py index f371c28fc..cd7ded312 100644 --- a/tests/unit/test_agent_cli.py +++ b/tests/unit/test_agent_cli.py @@ -42,6 +42,7 @@ from skillspector.providers import _agent_cli from skillspector.providers._agent_cli import ( + _CLAUDE_SYSTEM_PROMPT, MAX_INPUT_BYTES, AgentCLIError, _build_claude_argv, @@ -210,6 +211,23 @@ def test_disable_slash_commands_present(self) -> None: argv = _build_claude_argv(CLAUDE_BINARY, MODEL, 4096) assert "--disable-slash-commands" in argv + def test_tools_flag_removes_all_builtin_tools(self) -> None: + argv = _build_claude_argv(CLAUDE_BINARY, MODEL, 4096) + # --allowed-tools "" only denies use; --tools "" also drops the schemas + # from context, which every call would otherwise pay for. + assert argv[argv.index("--tools") + 1] == "" + + def test_system_prompt_replaced_with_fixed_text(self) -> None: + argv = _build_claude_argv(CLAUDE_BINARY, MODEL, 4096) + assert argv[argv.index("--system-prompt") + 1] == _CLAUDE_SYSTEM_PROMPT + assert "--append-system-prompt" not in argv + + def test_no_session_persistence(self) -> None: + argv = _build_claude_argv(CLAUDE_BINARY, MODEL, 4096) + # Each call runs in a fresh temp cwd; persisting would leave one + # ~/.claude/projects folder per call. + assert "--no-session-persistence" in argv + def test_dangerously_skip_permissions_never_in_argv(self) -> None: argv = _build_claude_argv(CLAUDE_BINARY, MODEL, 4096) # Neither the short nor any variation may appear. diff --git a/tests/unit/test_providers.py b/tests/unit/test_providers.py index c743bf6f7..164b7e2cc 100644 --- a/tests/unit/test_providers.py +++ b/tests/unit/test_providers.py @@ -32,8 +32,11 @@ from pydantic import SecretStr import skillspector.providers as providers_module +import skillspector.providers._agent_cli as agent_cli_module +import skillspector.providers._agent_cli_base as agent_cli_base_module import skillspector.providers.anthropic.provider as anthropic_provider_module from skillspector.inference_usage import chat_model_controls, chat_model_requested_controls +from skillspector.llm_utils import get_chat_model from skillspector.providers import ( NO_LLM_API_KEY_MESSAGE, chat_models, @@ -1081,6 +1084,63 @@ def test_is_available_reports_not_ready(self) -> None: assert reason +class TestAgentCLISessionCostWarning: + """A usable CLI provider warns once that every LLM call is a full session.""" + + @pytest.fixture(autouse=True) + def _reset_warned(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(agent_cli_base_module, "_cost_warned", set()) + + def test_warns_once_when_available( + self, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture + ) -> None: + monkeypatch.setattr(agent_cli_module, "is_available", lambda name: (True, None)) + provider = ClaudeCLIProvider() + with caplog.at_level("WARNING", logger=agent_cli_base_module.logger.name): + assert provider.is_available() == (True, None) + assert provider.is_available() == (True, None) + warnings = [r for r in caplog.records if "separate claude session" in r.getMessage()] + assert len(warnings) == 1 + assert "--no-llm" in warnings[0].getMessage() + + def test_silent_when_unavailable( + self, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture + ) -> None: + monkeypatch.setattr(agent_cli_module, "is_available", lambda name: (False, "not logged in")) + with caplog.at_level("WARNING", logger=agent_cli_base_module.logger.name): + assert ClaudeCLIProvider().is_available() == (False, "not logged in") + assert not [r for r in caplog.records if "separate claude session" in r.getMessage()] + + def test_warns_before_first_cli_call_without_availability_probe( + self, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture + ) -> None: + # `skillspector scan` builds its chat model before any CLI call and only + # probes is_available() from the report node, after the calls have run. + def _no_probe(name: str) -> tuple[bool, str | None]: + raise AssertionError("the scan path must warn without probing availability") + + warned_before_call: list[bool] = [] + + def _run_agent_cli(binary: str, prompt: str, **kwargs: object) -> str: + warned_before_call.append( + any("separate claude session" in r.getMessage() for r in caplog.records) + ) + return "pong" + + monkeypatch.setattr(agent_cli_module, "is_available", _no_probe) + monkeypatch.setattr(agent_cli_module, "run_agent_cli", _run_agent_cli) + token = use_provider(ClaudeCLIProvider()) + try: + with caplog.at_level("WARNING", logger=agent_cli_base_module.logger.name): + assert get_chat_model().invoke("ping").content == "pong" + assert get_chat_model().invoke("ping").content == "pong" + finally: + reset_provider(token) + assert warned_before_call == [True, True] + warnings = [r for r in caplog.records if "separate claude session" in r.getMessage()] + assert len(warnings) == 1 + + class TestAgentCLIProviderMetadata: """Shared model-registry behavior for supported agent CLI providers."""