From deb698b42e6fc64363de11894ccf6fd2868f134f Mon Sep 17 00:00:00 2001 From: Steven Moy Date: Wed, 10 Jun 2026 14:33:44 -0700 Subject: [PATCH 001/104] add openai project header --- src/skillspector/llm_utils.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/skillspector/llm_utils.py b/src/skillspector/llm_utils.py index 1e03fc18..d24fa75f 100644 --- a/src/skillspector/llm_utils.py +++ b/src/skillspector/llm_utils.py @@ -64,6 +64,13 @@ def _resolve_llm_credentials() -> tuple[str, str | None]: return resolved_key, resolved_base +def _resolve_openai_project_header_value() -> str | None: + project_id = os.environ.get("OPENAI_PROJECT_ID", "").strip() + if not project_id: + return None + return project_id + + def is_llm_available() -> tuple[bool, str | None]: """Return ``(available, error_message)`` describing LLM credential status.""" try: @@ -87,12 +94,18 @@ def get_chat_model(model: str | None = None) -> ChatOpenAI: resolved_key, resolved_base = _resolve_llm_credentials() model = model or MODEL_CONFIG["default"] + default_headers = {} + project_header_value = _resolve_openai_project_header_value() + if project_header_value is not None: + default_headers["OpenAI-Project"] = project_header_value + return ChatOpenAI( model=model, base_url=resolved_base, api_key=resolved_key, max_tokens=get_max_output_tokens(model), timeout=120, + default_headers=default_headers, ) From 76eea23789889ea8c66e731a771ee08deaa33b5b Mon Sep 17 00:00:00 2001 From: CharmingGroot <70020572+CharmingGroot@users.noreply.github.com> Date: Fri, 12 Jun 2026 10:44:07 +0900 Subject: [PATCH 002/104] fix(mcp): treat allowed-tools as a permission declaration for LP3 LP3 fired on standard-compliant skills that declare capabilities via the Agent Skills `allowed-tools` field: _parse_manifest dropped allowed-tools, and the analyzer only read `permissions`, so a skill declaring `allowed-tools: [Bash, Read]` was reported as having no declared permissions. Parse and preserve `allowed-tools` (list or comma-separated string) in _parse_manifest, and treat a non-empty allowed-tools alongside `permissions` when evaluating LP3's "no declared permissions" condition. LP1/LP4 continue to use the explicit `permissions` list only. Add regression tests for allowed-tools parsing (both forms) and for LP3 no longer firing when allowed-tools is declared. Signed-off-by: CharmingGroot <70020572+CharmingGroot@users.noreply.github.com> --- .../nodes/analyzers/mcp_least_privilege.py | 20 +++++++++-- src/skillspector/nodes/build_context.py | 12 +++++-- tests/nodes/test_build_context.py | 34 +++++++++++++++++++ tests/test_mcp_least_privilege.py | 26 ++++++++++++++ 4 files changed, 88 insertions(+), 4 deletions(-) diff --git a/src/skillspector/nodes/analyzers/mcp_least_privilege.py b/src/skillspector/nodes/analyzers/mcp_least_privilege.py index a79ee0dc..73d57454 100644 --- a/src/skillspector/nodes/analyzers/mcp_least_privilege.py +++ b/src/skillspector/nodes/analyzers/mcp_least_privilege.py @@ -125,6 +125,19 @@ def _is_test_file(path: str) -> bool: return name.startswith("test_") or stem.endswith("_test") +def _normalize_allowed_tools(value: object) -> list[str]: + """Coerce a manifest ``allowed-tools`` value into a list of tool names. + + Accepts the list form (``[Bash, Read]``) and the comma-separated string + form (``"Bash, Read"``). Anything else yields an empty list. + """ + if isinstance(value, list): + return [str(t).strip() for t in value if str(t).strip()] + if isinstance(value, str): + return [t.strip() for t in value.split(",") if t.strip()] + return [] + + def _detect_capabilities(content: str) -> set[str]: """Return set of capability categories found in *content*.""" found: set[str] = set() @@ -189,6 +202,9 @@ def node(state: SkillspectorState) -> AnalyzerNodeResponse: else: permissions = None # treat missing or non-list as None + # `allowed-tools` (Agent Skills standard) is also a permission declaration. + allowed_tools = _normalize_allowed_tools(manifest.get("allowed-tools")) + # --- LP2: Wildcard permission --- if isinstance(permissions, list) and _has_wildcard(permissions): logger.debug("%s: LP2 wildcard permission detected", ANALYZER_ID) @@ -232,8 +248,8 @@ def node(state: SkillspectorState) -> AnalyzerNodeResponse: for caps in file_capabilities.values(): all_caps.update(caps) - # LP3: emit when permissions is None or empty list AND capabilities detected - permissions_absent = permissions is None or permissions == [] + # LP3: no declaration via `permissions` or `allowed-tools`, yet caps detected. + permissions_absent = (permissions is None or permissions == []) and not allowed_tools if permissions_absent and all_caps: logger.debug("%s: LP3 no permissions declared but capabilities detected", ANALYZER_ID) cap_names = ", ".join(sorted(all_caps)) diff --git a/src/skillspector/nodes/build_context.py b/src/skillspector/nodes/build_context.py index 694a0487..4b9705a0 100644 --- a/src/skillspector/nodes/build_context.py +++ b/src/skillspector/nodes/build_context.py @@ -164,8 +164,8 @@ def _read_file_cache(skill_dir: Path, components: list[str]) -> dict[str, str]: def _parse_manifest(skill_dir: Path) -> dict[str, object]: """Parse SKILL.md or skill.md YAML frontmatter into a manifest dict. - Returns dict with name, description, triggers (list), permissions (list). - Returns {} if no file or parse fails. + Returns dict with name, description, triggers (list), permissions (list), + allowed-tools (list), parameters (list). Returns {} if no file or parse fails. """ for name in ("SKILL.md", "skill.md"): path = skill_dir / name @@ -200,6 +200,14 @@ def _parse_manifest(skill_dir: Path) -> dict[str, object]: manifest["permissions"] = ( [str(p) for p in permissions] if isinstance(permissions, list) else [] ) + # `allowed-tools` (Agent Skills standard) — accept list or comma string. + allowed_tools = data.get("allowed-tools", []) + if isinstance(allowed_tools, list): + manifest["allowed-tools"] = [str(t).strip() for t in allowed_tools if str(t).strip()] + elif isinstance(allowed_tools, str): + manifest["allowed-tools"] = [t.strip() for t in allowed_tools.split(",") if t.strip()] + else: + manifest["allowed-tools"] = [] # Preserve parameter definitions as dicts so the MCP tool-poisoning # analyzer (TP1/TP2/TP3 parameter checks) can inspect them. Without # this, those checks never fire on real scans because the manifest diff --git a/tests/nodes/test_build_context.py b/tests/nodes/test_build_context.py index a0fc16b9..7ff92e3a 100644 --- a/tests/nodes/test_build_context.py +++ b/tests/nodes/test_build_context.py @@ -70,6 +70,7 @@ def test_build_context_real_directory_with_skill_md(tmp_path: Path) -> None: "description": "For tests", "triggers": ["a", "b"], "permissions": ["read"], + "allowed-tools": [], "parameters": [], } assert result["ast_cache"] == {} @@ -211,3 +212,36 @@ def test_build_context_parses_parameters_from_frontmatter(tmp_path: Path) -> Non assert result["manifest"]["parameters"] == [ {"name": "path", "description": "file path to read"} ] + + +def test_build_context_parses_allowed_tools_list(tmp_path: Path) -> None: + """`allowed-tools` list form is preserved so LP3 treats it as a declaration.""" + (tmp_path / "SKILL.md").write_text( + "---\nname: deployer\ndescription: deploys services\nallowed-tools: [Bash, Read]\n---\n", + encoding="utf-8", + ) + state: SkillspectorState = {"skill_path": str(tmp_path)} + result = build_context(state) + assert result["manifest"]["allowed-tools"] == ["Bash", "Read"] + + +def test_build_context_allowed_tools_malformed_value(tmp_path: Path) -> None: + """A non-list, non-string `allowed-tools` value normalizes to an empty list.""" + (tmp_path / "SKILL.md").write_text( + "---\nname: deployer\ndescription: deploys services\nallowed-tools: 42\n---\n", + encoding="utf-8", + ) + state: SkillspectorState = {"skill_path": str(tmp_path)} + result = build_context(state) + assert result["manifest"]["allowed-tools"] == [] + + +def test_build_context_parses_allowed_tools_comma_string(tmp_path: Path) -> None: + """`allowed-tools` comma-separated string form is normalized to a list.""" + (tmp_path / "SKILL.md").write_text( + "---\nname: deployer\ndescription: deploys services\nallowed-tools: Bash, Read\n---\n", + encoding="utf-8", + ) + state: SkillspectorState = {"skill_path": str(tmp_path)} + result = build_context(state) + assert result["manifest"]["allowed-tools"] == ["Bash", "Read"] diff --git a/tests/test_mcp_least_privilege.py b/tests/test_mcp_least_privilege.py index 9e7852e6..e134fca1 100644 --- a/tests/test_mcp_least_privilege.py +++ b/tests/test_mcp_least_privilege.py @@ -258,6 +258,32 @@ def test_no_permissions_field(self): assert lp3.file == "SKILL.md" +class TestLP3AllowedTools: + def test_allowed_tools_list_no_lp3(self): + """allowed-tools list form ([Bash, Read]) is a declaration → no LP3.""" + state = _make_state("mcp_underdeclared_skill") + state["manifest"]["permissions"] = None + state["manifest"]["allowed-tools"] = ["Bash", "Read"] + result = mcp_least_privilege.node(state) + findings = result["findings"] + lp3_findings = [f for f in findings if f.rule_id == "LP3"] + assert lp3_findings == [], ( + f"allowed-tools should satisfy LP3, got: {[f.rule_id for f in findings]}" + ) + + def test_allowed_tools_comma_string_no_lp3(self): + """allowed-tools comma-string form ('Bash, Read') is also a declaration → no LP3.""" + state = _make_state("mcp_underdeclared_skill") + state["manifest"]["permissions"] = None + state["manifest"]["allowed-tools"] = "Bash, Read" + result = mcp_least_privilege.node(state) + findings = result["findings"] + lp3_findings = [f for f in findings if f.rule_id == "LP3"] + assert lp3_findings == [], ( + f"allowed-tools should satisfy LP3, got: {[f.rule_id for f in findings]}" + ) + + class TestLP4OverDeclared: def test_over_declared_detected(self): """mcp_overprivileged_skill declares bash/network/write but code only reads → LP4 for network and write.""" From 0eeae6ff7dbc3f1d04b1617051dbd04dd184c711 Mon Sep 17 00:00:00 2001 From: nyxst4ck <289980115+nyxst4ck@users.noreply.github.com> Date: Fri, 12 Jun 2026 09:21:29 -0300 Subject: [PATCH 003/104] fix(llm): isolate batch failures in Stage 2 and keep unanalysed findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One exception anywhere in the arun_batches fan-out aborted the whole Stage 2 pass: asyncio.gather without return_exceptions cancelled the remaining batches, the meta-analyzer's blanket except caught the propagated error, and every file silently fell back to static-only results while the CLI still exited 0 (#9). arun_batches now isolates failures per batch: a transient error (timeout, 429, oversized-chunk 400) is logged and costs only its own batch. ValueError and NotImplementedError still propagate, since they signal misconfiguration rather than infra trouble. With partial results possible, apply_filter could no longer treat a missing confirmation as a rejection: a finding whose batch never returned would be silently dropped — a false negative manufactured by an infrastructure error (#11). The meta-analyzer now partitions findings by whether a returned batch actually carried them: analysed findings go through the normal confirm-or-drop filter, unanalysed ones are kept via the existing fallback path, and a WARNING logs how many findings were kept unfiltered so the gap is visible. Net effect: an infra failure can only ever cost enrichment on a file, never the finding itself, and one bad call no longer turns off the semantic filter for the whole scan. Fixes #9, fixes #11 Signed-off-by: nyxst4ck <289980115+nyxst4ck@users.noreply.github.com> --- src/skillspector/llm_analyzer_base.py | 19 ++- src/skillspector/nodes/meta_analyzer.py | 23 +++- tests/nodes/test_llm_analyzer_base.py | 36 ++++++ tests/nodes/test_meta_analyzer.py | 148 ++++++++++++++++++++++++ 4 files changed, 224 insertions(+), 2 deletions(-) create mode 100644 tests/nodes/test_meta_analyzer.py diff --git a/src/skillspector/llm_analyzer_base.py b/src/skillspector/llm_analyzer_base.py index a678e4ec..f5933ff5 100644 --- a/src/skillspector/llm_analyzer_base.py +++ b/src/skillspector/llm_analyzer_base.py @@ -374,6 +374,14 @@ async def arun_batches( *max_concurrency* LLM requests in parallel. Both cross-file and cross-chunk batches are parallelized in a single gather call. + Failures are isolated per batch: a transient error (timeout, 429, + oversized-chunk 400, ...) costs only its own batch, which is logged + and omitted from the result, so one bad call cannot cancel the rest + of the fan-out. Callers can detect partial results by comparing the + returned batches against the submitted ones. ``ValueError`` and + ``NotImplementedError`` signal misconfiguration rather than infra + trouble and keep propagating. + The return type mirrors :meth:`run_batches`. """ sem = asyncio.Semaphore(max_concurrency) @@ -394,7 +402,16 @@ async def _process(batch: Batch) -> tuple[Batch, list]: logger.debug("LLM response for %s", batch.file_label) return (batch, self.parse_response(response, batch)) - return list(await asyncio.gather(*[_process(b) for b in batches])) + results = await asyncio.gather(*[_process(b) for b in batches], return_exceptions=True) + successful: list[tuple[Batch, list]] = [] + for batch, result in zip(batches, results, strict=True): + if isinstance(result, (ValueError, NotImplementedError)): + raise result + if isinstance(result, BaseException): + logger.warning("LLM batch failed for %s: %s", batch.file_label, result) + continue + successful.append(result) + return successful # -- Convenience -------------------------------------------------------- diff --git a/src/skillspector/nodes/meta_analyzer.py b/src/skillspector/nodes/meta_analyzer.py index 8f2b5410..74f8c99e 100644 --- a/src/skillspector/nodes/meta_analyzer.py +++ b/src/skillspector/nodes/meta_analyzer.py @@ -392,7 +392,28 @@ def meta_analyzer(state: SkillspectorState) -> MetaAnalyzerResponse: ) batch_results = asyncio.run(analyzer.arun_batches(batches, metadata_text=metadata_text)) - filtered = analyzer.apply_filter(findings, batch_results) + + if len(batch_results) < len(batches): + # Some batches never returned. A finding the LLM never saw has no + # verdict — keep it via the fallback path instead of letting + # apply_filter treat the missing confirmation as a rejection. + analysed_ids = {id(f) for batch, _ in batch_results for f in batch.findings} + analysed = [f for f in findings if id(f) in analysed_ids] + unanalysed = [f for f in findings if id(f) not in analysed_ids] + else: + analysed, unanalysed = findings, [] + + filtered = analyzer.apply_filter(analysed, batch_results) + if unanalysed: + logger.warning( + "Meta-analyzer: %d/%d batches failed; keeping %d findings in %d " + "files unfiltered (no LLM verdict)", + len(batches) - len(batch_results), + len(batches), + len(unanalysed), + len({f.file for f in unanalysed}), + ) + filtered.extend(_fallback_filtered(unanalysed)) logger.debug( "LLM filtering done: %d findings -> %d after filter", diff --git a/tests/nodes/test_llm_analyzer_base.py b/tests/nodes/test_llm_analyzer_base.py index 9899a7ff..4125b112 100644 --- a/tests/nodes/test_llm_analyzer_base.py +++ b/tests/nodes/test_llm_analyzer_base.py @@ -484,6 +484,42 @@ async def _delayed_ainvoke(prompt: str) -> LLMAnalysisResult: assert seen_files == {f"file_{i}.py" for i in range(num_batches)} + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + async def test_failed_batch_does_not_abort_the_others(self) -> None: + """A transient failure costs only its own batch, not the whole fan-out.""" + + async def _flaky_ainvoke(prompt: str) -> LLMAnalysisResult: + if "b.py" in prompt: + raise RuntimeError("429 Too Many Requests") + return LLMAnalysisResult(findings=[]) + + analyzer = LLMAnalyzerBase(base_prompt="test", model=self.MODEL) + analyzer._structured_llm.ainvoke = _flaky_ainvoke + + batches = [ + Batch(file_path="a.py", content="code a"), + Batch(file_path="b.py", content="code b"), + Batch(file_path="c.py", content="code c"), + ] + results = await analyzer.arun_batches(batches) + assert {batch.file_path for batch, _ in results} == {"a.py", "c.py"} + + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + async def test_all_batches_failed_returns_empty(self) -> None: + analyzer = LLMAnalyzerBase(base_prompt="test", model=self.MODEL) + analyzer._structured_llm.ainvoke = AsyncMock(side_effect=RuntimeError("boom")) + batches = [Batch(file_path="a.py", content="code")] + assert await analyzer.arun_batches(batches) == [] + + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + async def test_value_error_still_propagates(self) -> None: + """ValueError signals misconfiguration, not infra trouble — never swallowed.""" + analyzer = LLMAnalyzerBase(base_prompt="test", model=self.MODEL) + analyzer._structured_llm.ainvoke = AsyncMock(side_effect=ValueError("no API key")) + batches = [Batch(file_path="a.py", content="code")] + with pytest.raises(ValueError, match="no API key"): + await analyzer.arun_batches(batches) + # --------------------------------------------------------------------------- # _format_findings_for_prompt (per-file, no truncation) diff --git a/tests/nodes/test_meta_analyzer.py b/tests/nodes/test_meta_analyzer.py new file mode 100644 index 00000000..e7a3c591 --- /dev/null +++ b/tests/nodes/test_meta_analyzer.py @@ -0,0 +1,148 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Tests for the meta_analyzer node: batch-failure resilience of the filter. + +Covers the keep/drop semantics when Stage 2 batches fail (issues #9/#11): +a finding the LLM rejected is dropped, a finding the LLM never saw is kept +via the fallback path, and one failed batch must not disable filtering for +the batches that succeeded. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, patch + +from skillspector.llm_analyzer_base import Batch +from skillspector.models import Finding +from skillspector.nodes.meta_analyzer import LLMMetaAnalyzer, meta_analyzer + +MOCK_PATCH_TARGET = "skillspector.llm_analyzer_base.get_chat_model" + + +def _mock_get_chat_model(*_args, **_kwargs): + from unittest.mock import MagicMock + + mock_llm = MagicMock() + mock_llm.with_structured_output.return_value = MagicMock() + return mock_llm + + +def _confirm(pattern_id: str, file: str, start_line: int) -> dict[str, object]: + """LLM item confirming a finding, as parse_response would emit it.""" + return { + "pattern_id": pattern_id, + "is_vulnerability": True, + "confidence": 0.9, + "explanation": "confirmed by llm", + "remediation": "fix it", + "_file": file, + "start_line": start_line, + "end_line": None, + } + + +@patch(MOCK_PATCH_TARGET, _mock_get_chat_model) +class TestMetaAnalyzerPartialBatchFailure: + def _state(self, findings: list[Finding]) -> dict[str, object]: + return { + "findings": findings, + "use_llm": True, + "file_cache": {"a.py": "code a", "b.py": "code b"}, + "manifest": {}, + "model_config": {}, + } + + def test_unanalysed_findings_survive_a_failed_batch(self) -> None: + """Findings whose batch failed are kept (no verdict != rejection).""" + f_confirmed = Finding(rule_id="R1", message="m", file="a.py", start_line=1) + f_rejected = Finding(rule_id="R2", message="m", file="a.py", start_line=5) + f_unseen = Finding(rule_id="R1", message="m", file="b.py", start_line=3) + + batch_a = Batch(file_path="a.py", content="code a", findings=[f_confirmed, f_rejected]) + batch_b = Batch(file_path="b.py", content="code b", findings=[f_unseen]) + + # batch_b never returned (timeout/429): only batch_a's verdicts exist, + # and the LLM confirmed R1 but stayed silent on R2 (= rejection). + partial_results = [(batch_a, [_confirm("R1", "a.py", 1)])] + + with ( + patch.object(LLMMetaAnalyzer, "get_batches", return_value=[batch_a, batch_b]), + patch.object( + LLMMetaAnalyzer, + "arun_batches", + new_callable=AsyncMock, + return_value=partial_results, + ), + ): + result = meta_analyzer(self._state([f_confirmed, f_rejected, f_unseen])) + + filtered = result["filtered_findings"] + kept = {(f.file, f.rule_id) for f in filtered} + + # the real filter still applies to the batch that came back + assert ("a.py", "R1") in kept + assert ("a.py", "R2") not in kept + # the finding the LLM never saw must NOT be silently dropped + assert ("b.py", "R1") in kept + + confirmed = next(f for f in filtered if f.file == "a.py") + assert confirmed.explanation == "confirmed by llm" + + def test_all_batches_failed_keeps_everything_via_fallback(self) -> None: + f1 = Finding(rule_id="R1", message="m", file="a.py", start_line=1) + f2 = Finding(rule_id="R2", message="m", file="b.py", start_line=2) + batch_a = Batch(file_path="a.py", content="code a", findings=[f1]) + batch_b = Batch(file_path="b.py", content="code b", findings=[f2]) + + with ( + patch.object(LLMMetaAnalyzer, "get_batches", return_value=[batch_a, batch_b]), + patch.object( + LLMMetaAnalyzer, + "arun_batches", + new_callable=AsyncMock, + return_value=[], + ), + ): + result = meta_analyzer(self._state([f1, f2])) + + kept = {(f.file, f.rule_id) for f in result["filtered_findings"]} + assert kept == {("a.py", "R1"), ("b.py", "R2")} + + def test_no_failures_keeps_strict_confirm_or_drop(self) -> None: + """When every batch returns, unconfirmed findings are dropped as before.""" + f_confirmed = Finding(rule_id="R1", message="m", file="a.py", start_line=1) + f_rejected = Finding(rule_id="R2", message="m", file="b.py", start_line=2) + batch_a = Batch(file_path="a.py", content="code a", findings=[f_confirmed]) + batch_b = Batch(file_path="b.py", content="code b", findings=[f_rejected]) + + full_results = [ + (batch_a, [_confirm("R1", "a.py", 1)]), + (batch_b, []), + ] + + with ( + patch.object(LLMMetaAnalyzer, "get_batches", return_value=[batch_a, batch_b]), + patch.object( + LLMMetaAnalyzer, + "arun_batches", + new_callable=AsyncMock, + return_value=full_results, + ), + ): + result = meta_analyzer(self._state([f_confirmed, f_rejected])) + + kept = {(f.file, f.rule_id) for f in result["filtered_findings"]} + assert kept == {("a.py", "R1")} From 358cbb6e15a74221ca0f6ec9af825c4041e826a6 Mon Sep 17 00:00:00 2001 From: Ram Dwivedi Date: Sun, 14 Jun 2026 08:49:30 -0400 Subject: [PATCH 004/104] docs: correct stale analyzer status and dangling references The MCP, semantic, and taint-tracking analyzers are implemented, but DEVELOPMENT.md still described them as stubs. Update the package-layout and "Stub analyzers" sections to reflect actual status (only mcp_rug_pull remains a stub), fix an invalid `//` comment in the Python example, and replace dangling internal "SADD" references in graph.py with neutral roadmap notes. Also refresh two stale "stub" docstrings. Docs and comments only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Ram Dwivedi --- docs/DEVELOPMENT.md | 13 +++++++------ src/skillspector/graph.py | 7 ++++--- src/skillspector/nodes/analyzers/__init__.py | 2 +- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/docs/DEVELOPMENT.md b/docs/DEVELOPMENT.md index 0795f093..d80db177 100644 --- a/docs/DEVELOPMENT.md +++ b/docs/DEVELOPMENT.md @@ -151,16 +151,17 @@ There are no conditional edges: after `resolve_input` → `build_context`, all a | `report.py` | Report node | | **nodes/analyzers/** | | | `__init__.py` | Registry: `ANALYZER_NODE_IDS`, `ANALYZER_NODES` | -| `common.py` | Helpers (e.g. `make_dummy_finding`) for stub analyzers | +| `common.py` | Shared analyzer helpers (line/context extraction, AST name resolution) | | `static_runner.py` | Runs static patterns; converts `AnalyzerFinding` → `Finding` | | `pattern_defaults.py` | Shared pattern metadata (category, explanation, remediation) | | `static_yara.py` | YARA-based static analyzer | | `osv_client.py` | OSV.dev API client for live vulnerability lookups (SC4); batch queries with caching and fallback | | `static_patterns_*.py` | 11 pattern-based analyzers (prompt_injection, data_exfiltration, etc.) | | `behavioral_ast.py` | AST-based behavioral analyzer (AST1–AST8): detects exec, eval, subprocess, os.system, compile, dynamic import/getattr, and dangerous execution chains | -| `behavioral_taint_tracking.py` | Taint-tracking behavioral analyzer (stub) | -| `mcp_least_privilege.py`, `mcp_tool_poisoning.py`, `mcp_rug_pull.py` | MCP analyzer stubs | -| `semantic_security_discovery.py`, `semantic_developer_intent.py`, `semantic_quality_policy.py` | Semantic (LLM) analyzer stubs | +| `behavioral_taint_tracking.py` | Taint-tracking behavioral analyzer (TT1–TT5): source→sink data-flow analysis over Python AST | +| `mcp_least_privilege.py`, `mcp_tool_poisoning.py` | MCP analyzers (LP1–LP4 least-privilege; TP1–TP4 tool poisoning) | +| `mcp_rug_pull.py` | MCP rug-pull analyzer (stub; RP1–RP3 planned) | +| `semantic_security_discovery.py`, `semantic_developer_intent.py`, `semantic_quality_policy.py` | Semantic (LLM) analyzers; emit findings only when `use_llm` is enabled | --- @@ -195,7 +196,7 @@ The CLI passes `input_path` to the graph. The **resolve_input** node (using [inp from skillspector import graph result = graph.invoke({ - "input_path": "/path/to/skill", // or "skill_path" for local dir only + "input_path": "/path/to/skill", # or use "skill_path" for a local dir "output_format": "json", # optional: terminal, json, markdown, sarif (default sarif) "use_llm": True, # optional: False to skip LLM in meta_analyzer }) @@ -248,7 +249,7 @@ Use [pattern_defaults](../src/skillspector/nodes/analyzers/pattern_defaults.py) ### Stub analyzers -Return `{"findings": []}`. The behavioral, MCP, and semantic analyzer nodes are currently stubs (placeholders for future implementation). +Return `{"findings": []}`. Most analyzer nodes are implemented; `mcp_rug_pull` remains a stub (returns no findings) pending rug-pull detection (RP1–RP3). Use this pattern for any new placeholder analyzer. The LLM-backed semantic analyzers also return `{"findings": []}` when `use_llm` is False. --- diff --git a/src/skillspector/graph.py b/src/skillspector/graph.py index 277f9634..e034ffe3 100644 --- a/src/skillspector/graph.py +++ b/src/skillspector/graph.py @@ -13,10 +13,11 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""LangGraph workflow for Skillspector stub analyzers.""" +"""LangGraph workflow wiring Skillspector's analyzer nodes.""" -# TODO(SADD A.2.1–A.2.4): Analyzer discovery, stage-as-category with meta last, wire registry; respect requires_api_key/is_available() and skip or warn when API key missing or analyzer unavailable. See SADD for skillspector § A.2. -# TODO(SADD A.5.1): Implement skillspector serve (FastAPI): POST /scan (zip), GET /results/{id}, GET /health. See SADD for skillspector § A.5.1. +# Roadmap: analyzer auto-discovery and stage-as-category ordering (meta_analyzer last); respect +# requires_api_key / is_available() so analyzers are skipped or warned when unavailable. +# Roadmap: optional `skillspector serve` HTTP API (POST /scan, GET /results/{id}, GET /health). from __future__ import annotations diff --git a/src/skillspector/nodes/analyzers/__init__.py b/src/skillspector/nodes/analyzers/__init__.py index 58b3e934..71fdac3e 100644 --- a/src/skillspector/nodes/analyzers/__init__.py +++ b/src/skillspector/nodes/analyzers/__init__.py @@ -13,7 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""Analyzer node registry for Skillspector v2 stub workflow.""" +"""Analyzer node registry for the Skillspector workflow.""" from __future__ import annotations From 20a330d5babf361fa06ac2d4a7e6b866a239dbc6 Mon Sep 17 00:00:00 2001 From: Ram Dwivedi Date: Sun, 14 Jun 2026 09:26:56 -0400 Subject: [PATCH 005/104] ci: add GitHub Actions CI/CD workflow Add a GitHub Actions workflow that runs on pull_request and push to main: - Lint and format checks with ruff - Unit tests with pytest (integration tests excluded to avoid LLM secrets) - Matrix over Python 3.12 and 3.13 on ubuntu-latest - DCO sign-off verification on PRs Windows is excluded because the test suite has known path-separator failures in build_context that are out of scope for this change. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Ram Dwivedi --- .github/workflows/ci.yml | 84 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 .github/workflows/ci.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 00000000..56f734b3 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,84 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +name: CI + +on: + pull_request: + branches: ["main"] + push: + branches: ["main"] + +jobs: + lint-and-test: + name: Lint & Test (Python ${{ matrix.python-version }}) + runs-on: ubuntu-latest + # Windows is excluded: the test suite has known path-separator failures + # in build_context that are out of scope for this workflow. + strategy: + fail-fast: false + matrix: + python-version: ["3.12", "3.13"] + + steps: + - uses: actions/checkout@v4 + + - name: Set up uv + uses: astral-sh/setup-uv@v5 + with: + enable-cache: true + python-version: ${{ matrix.python-version }} + + - name: Install dependencies + run: uv sync --all-extras + + - name: Lint with ruff + run: uv run ruff check src/ tests/ + + - name: Check formatting with ruff + run: uv run ruff format --check src/ tests/ + + - name: Run unit tests + run: uv run pytest -m "not integration" + + - name: Run unit tests with coverage + run: uv run pytest -m "not integration" --cov=src/skillspector --cov-report=term-missing + + dco: + name: DCO Check + runs-on: ubuntu-latest + if: github.event_name == 'pull_request' + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Verify DCO sign-off on all commits + run: | + BASE=${{ github.event.pull_request.base.sha }} + HEAD=${{ github.event.pull_request.head.sha }} + MISSING=$(git log --pretty=format:"%H %s" "${BASE}..${HEAD}" | while read -r sha msg; do + if ! git log -1 --format="%B" "$sha" | grep -q "^Signed-off-by:"; then + echo " $sha: $msg" + fi + done) + if [ -n "$MISSING" ]; then + echo "The following commits are missing Signed-off-by trailers:" + echo "$MISSING" + echo "" + echo "Please add a DCO sign-off (git commit -s) to all commits." + exit 1 + fi + echo "All commits have DCO sign-off." From c54d6e7c3b7489dbc6d01a04298c22568db1a36b Mon Sep 17 00:00:00 2001 From: Ram Dwivedi Date: Sun, 14 Jun 2026 10:32:58 -0400 Subject: [PATCH 006/104] security(meta_analyzer): add severity-gated floor to apply_filter CRITICAL and HIGH static findings are now always kept in the output of apply_filter(), even when the LLM does not confirm them (omitted, denies the finding, or returns confidence < 0.6). MEDIUM/LOW findings continue to be filtered by the LLM for false-positive reduction as before. Motivation: the LLM receives attacker-controlled skill content. A prompt- injection payload embedded in a scanned skill could cause the LLM to drop a real CRITICAL or HIGH static finding, hiding it from the security report (a false negative in a security gate). This invariant applies to all providers. Implementation: - LLMMetaAnalyzer._HIGH_SEVERITY_FLOOR = frozenset({"CRITICAL", "HIGH"}). - In the "not confirmed" branch of apply_filter(), if the finding's severity is in the floor set, emit the original static finding unchanged and append the tag "llm-unconfirmed" so consumers can distinguish it from LLM-validated findings. The tag is not duplicated if already present. - Confirmed CRITICAL/HIGH findings are still enriched with LLM explanation/ remediation/confidence as before (no regression). - Finding.to_dict() now includes "tags", so the "llm-unconfirmed" marker is visible in the JSON report (tags were previously not serialized). Tests in tests/nodes/test_llm_analyzer_base.py: - CRITICAL/HIGH finding unconfirmed -> kept, "llm-unconfirmed" tag, original data. - MEDIUM/LOW finding unconfirmed -> still dropped (existing behaviour). - Confirmed CRITICAL -> enriched normally, tag absent. - Duplicate-tag guard -> "llm-unconfirmed" not appended twice. - to_dict surfacing -> marker present in JSON output. Signed-off-by: Ram Dwivedi Co-Authored-By: Claude Opus 4.8 (1M context) --- src/skillspector/models.py | 3 + src/skillspector/nodes/meta_analyzer.py | 48 ++++++ tests/nodes/test_llm_analyzer_base.py | 187 ++++++++++++++++++++++++ 3 files changed, 238 insertions(+) diff --git a/src/skillspector/models.py b/src/skillspector/models.py index a26a78be..9a478219 100644 --- a/src/skillspector/models.py +++ b/src/skillspector/models.py @@ -101,6 +101,9 @@ def to_dict(self) -> dict[str, object]: "remediation": self.remediation, "code_snippet": self.code_snippet or self.context, "intent": self.intent, + # Tags surface markers like "llm-unconfirmed" (a high-severity static + # finding the LLM filter did not confirm but which is preserved anyway). + "tags": list(self.tags), } def __str__(self) -> str: diff --git a/src/skillspector/nodes/meta_analyzer.py b/src/skillspector/nodes/meta_analyzer.py index 8f2b5410..61908f31 100644 --- a/src/skillspector/nodes/meta_analyzer.py +++ b/src/skillspector/nodes/meta_analyzer.py @@ -266,6 +266,14 @@ def parse_response( # -- Apply filter (keyed by file + rule_id + start/end_line) ------------- + # Severities that must never be silently dropped by LLM filtering. + # Because the LLM receives attacker-controlled skill content, a prompt-injection + # payload could cause it to omit or deny a real CRITICAL/HIGH static finding. + # For these severities a false-negative (hiding a real vulnerability) is far + # worse than a false-positive, so we keep the original static finding regardless + # of what the LLM says and mark it "llm-unconfirmed" via the tags field. + _HIGH_SEVERITY_FLOOR = frozenset({"CRITICAL", "HIGH"}) + def apply_filter( self, findings: list[Finding], @@ -279,6 +287,15 @@ def apply_filter( is included in the key when provided but falls back to ``None`` so callers that omit it still match. Falls back to coarse ``(file, rule_id)`` keying for LLM responses that omit ``start_line``. + + Severity-gated floor (security invariant) + ------------------------------------------ + CRITICAL and HIGH static findings are **always** kept in the output even + if the LLM did not confirm them. When the LLM omits or denies such a + finding the original static finding is preserved unchanged and the tag + ``"llm-unconfirmed"`` is appended so consumers can distinguish it from + LLM-validated findings. MEDIUM and LOW findings continue to be filtered + by the LLM as before (false-positive reduction). """ _enrichment = tuple[str, str, float] confirmed_granular: dict[tuple[str, str, int, int | None], _enrichment] = {} @@ -323,6 +340,37 @@ def apply_filter( elif coarse_key in confirmed_coarse: expl, rem, conf = confirmed_coarse[coarse_key] else: + # Security: CRITICAL/HIGH static findings must survive LLM filtering. + # A prompt-injection payload in the scanned skill could cause the LLM + # to deny or omit a real high-severity finding; silently dropping it + # would be a false-negative in a security gate. Keep the original + # finding and tag it so consumers know it was not LLM-validated. + if f.severity in self._HIGH_SEVERITY_FLOOR: + unconfirmed_tags = list(f.tags) + if "llm-unconfirmed" not in unconfirmed_tags: + unconfirmed_tags.append("llm-unconfirmed") + result.append( + Finding( + rule_id=f.rule_id, + message=f.message, + severity=f.severity, + confidence=f.confidence, + file=f.file, + start_line=f.start_line, + end_line=f.end_line, + remediation=f.remediation or get_remediation(f.rule_id), + tags=unconfirmed_tags, + context=f.context, + matched_text=f.matched_text, + category=getattr(f, "category", None), + pattern=getattr(f, "pattern", None), + finding=getattr(f, "finding", None), + explanation=getattr(f, "explanation", None), + code_snippet=getattr(f, "code_snippet", None) or f.context, + intent=None, + ) + ) + # MEDIUM/LOW: preserve existing behaviour (LLM may filter as false-positive). continue result.append( Finding( diff --git a/tests/nodes/test_llm_analyzer_base.py b/tests/nodes/test_llm_analyzer_base.py index 9899a7ff..08e96c48 100644 --- a/tests/nodes/test_llm_analyzer_base.py +++ b/tests/nodes/test_llm_analyzer_base.py @@ -1085,6 +1085,193 @@ def test_end_line_used_when_provided(self) -> None: assert result[0].explanation == "Long block is dangerous" +# --------------------------------------------------------------------------- +# LLMMetaAnalyzer.apply_filter — severity-gated suppression floor +# +# Security invariant: CRITICAL and HIGH static findings must survive LLM +# filtering even if the LLM (operating on attacker-controlled skill content) +# omits or denies them. MEDIUM/LOW findings are still filtered normally. +# --------------------------------------------------------------------------- + + +class TestApplyFilterSeverityFloor: + """Tests for the severity-gated suppression floor in apply_filter. + + Verifies that CRITICAL/HIGH static findings are never silently dropped + by the LLM filter (adversarial prompt-injection defence), while MEDIUM/LOW + findings continue to be filtered as before. + """ + + MODEL = "nvidia/openai/gpt-oss-120b" + + def _make_finding( + self, + rule_id: str, + severity: str, + file: str = "skill.md", + line: int = 1, + tags: list[str] | None = None, + ) -> Finding: + return Finding( + rule_id=rule_id, + message=f"original message for {rule_id}", + severity=severity, + confidence=0.8, + file=file, + start_line=line, + tags=tags or [], + ) + + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + def test_critical_unconfirmed_kept_with_llm_unconfirmed_tag(self) -> None: + """A CRITICAL static finding NOT confirmed by the LLM must be kept. + + The finding must appear in the output with its original severity and + message, and the tag 'llm-unconfirmed' must be present to let consumers + know it was not LLM-validated (and may represent an adversarial suppression). + """ + analyzer = LLMMetaAnalyzer(model=self.MODEL) + finding = self._make_finding("CRIT-001", "CRITICAL", line=10) + batch = Batch(file_path="skill.md", content="code", findings=[finding]) + # LLM response: is_vulnerability=False — the LLM denies the finding + llm_items = [ + { + "pattern_id": "CRIT-001", + "start_line": 10, + "is_vulnerability": False, + "confidence": 0.2, + "_file": "skill.md", + } + ] + result = analyzer.apply_filter([finding], [(batch, llm_items)]) + + assert len(result) == 1, "CRITICAL finding must NOT be dropped by LLM filtering" + kept = result[0] + assert kept.severity == "CRITICAL" + assert kept.rule_id == "CRIT-001" + assert kept.message == "original message for CRIT-001" + assert kept.confidence == 0.8 # original confidence preserved + assert "llm-unconfirmed" in kept.tags + + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + def test_high_unconfirmed_kept_with_llm_unconfirmed_tag(self) -> None: + """A HIGH static finding NOT confirmed by the LLM must be kept. + + Same invariant as CRITICAL: the original finding is preserved and + tagged 'llm-unconfirmed'. + """ + analyzer = LLMMetaAnalyzer(model=self.MODEL) + finding = self._make_finding("HIGH-001", "HIGH", line=5) + batch = Batch(file_path="skill.md", content="code", findings=[finding]) + # LLM response: finding completely omitted (empty list) — simulates + # a prompt-injection payload making the LLM silently drop the finding + llm_items: list[dict] = [] + result = analyzer.apply_filter([finding], [(batch, llm_items)]) + + assert len(result) == 1, "HIGH finding must NOT be dropped by LLM filtering" + kept = result[0] + assert kept.severity == "HIGH" + assert kept.rule_id == "HIGH-001" + assert kept.message == "original message for HIGH-001" + assert kept.confidence == 0.8 # original confidence preserved + assert "llm-unconfirmed" in kept.tags + + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + def test_medium_unconfirmed_still_dropped(self) -> None: + """A MEDIUM static finding NOT confirmed by the LLM must still be dropped. + + The severity floor only applies to CRITICAL/HIGH. MEDIUM and LOW + findings remain subject to normal LLM filtering (false-positive reduction). + """ + analyzer = LLMMetaAnalyzer(model=self.MODEL) + finding = self._make_finding("MED-001", "MEDIUM", line=3) + batch = Batch(file_path="skill.md", content="code", findings=[finding]) + llm_items = [ + { + "pattern_id": "MED-001", + "start_line": 3, + "is_vulnerability": False, + "confidence": 0.1, + "_file": "skill.md", + } + ] + result = analyzer.apply_filter([finding], [(batch, llm_items)]) + + assert len(result) == 0, "MEDIUM finding must be dropped when LLM does not confirm it" + + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + def test_low_unconfirmed_still_dropped(self) -> None: + """A LOW static finding NOT confirmed by the LLM must still be dropped.""" + analyzer = LLMMetaAnalyzer(model=self.MODEL) + finding = self._make_finding("LOW-001", "LOW", line=7) + batch = Batch(file_path="skill.md", content="code", findings=[finding]) + llm_items: list[dict] = [] # LLM omits the finding entirely + result = analyzer.apply_filter([finding], [(batch, llm_items)]) + + assert len(result) == 0, "LOW finding must be dropped when LLM does not confirm it" + + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + def test_critical_confirmed_uses_llm_enrichment(self) -> None: + """A CRITICAL finding confirmed by the LLM is still enriched as before. + + The floor does not interfere with the normal happy path: when the LLM + confirms a CRITICAL/HIGH finding, the enriched version (with LLM + explanation/remediation/confidence) is used and 'llm-unconfirmed' is + NOT added. + """ + analyzer = LLMMetaAnalyzer(model=self.MODEL) + finding = self._make_finding("CRIT-002", "CRITICAL", line=20) + batch = Batch(file_path="skill.md", content="code", findings=[finding]) + llm_items = [ + { + "pattern_id": "CRIT-002", + "start_line": 20, + "is_vulnerability": True, + "confidence": 0.95, + "explanation": "LLM-confirmed dangerous pattern", + "remediation": "Remove immediately", + "_file": "skill.md", + } + ] + result = analyzer.apply_filter([finding], [(batch, llm_items)]) + + assert len(result) == 1 + kept = result[0] + assert kept.severity == "CRITICAL" + assert kept.rule_id == "CRIT-002" + assert kept.explanation == "LLM-confirmed dangerous pattern" + assert kept.confidence == 0.95 + assert "llm-unconfirmed" not in kept.tags + + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + def test_llm_unconfirmed_tag_not_duplicated(self) -> None: + """If the original finding already has 'llm-unconfirmed' in its tags, + apply_filter must not append it again.""" + analyzer = LLMMetaAnalyzer(model=self.MODEL) + finding = self._make_finding( + "CRIT-003", "CRITICAL", line=1, tags=["llm-unconfirmed", "existing-tag"] + ) + batch = Batch(file_path="skill.md", content="code", findings=[finding]) + llm_items: list[dict] = [] # LLM omits finding + result = analyzer.apply_filter([finding], [(batch, llm_items)]) + + assert len(result) == 1 + assert result[0].tags.count("llm-unconfirmed") == 1 + + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + def test_llm_unconfirmed_tag_surfaced_in_to_dict(self) -> None: + """The 'llm-unconfirmed' marker must be visible in the JSON output + (Finding.to_dict), so consumers can see a high-severity finding the LLM + did not confirm.""" + analyzer = LLMMetaAnalyzer(model=self.MODEL) + finding = self._make_finding("CRIT-004", "CRITICAL", line=1) + batch = Batch(file_path="skill.md", content="code", findings=[finding]) + result = analyzer.apply_filter([finding], [(batch, [])]) + + assert len(result) == 1 + assert "llm-unconfirmed" in result[0].to_dict()["tags"] + + # --------------------------------------------------------------------------- # LLMMetaAnalyzer.run_batches (mocked LLM) # --------------------------------------------------------------------------- From 4eee3c02c177e682925c885f68886693832468f8 Mon Sep 17 00:00:00 2001 From: Ram Dwivedi Date: Sun, 14 Jun 2026 08:51:59 -0400 Subject: [PATCH 007/104] docs: document the integration contract and trust model Add an "Integrating SkillSpector" section (exit codes, JSON shape, severity/recommendation enums, and a recommended install-gate mapping) and a "Trust model and data egress" section (no skill execution; LLM sends file contents unless --no-llm; SC4 sends dependency names to OSV.dev by design). Cross-link from DEVELOPMENT.md. Documentation only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Ram Dwivedi --- README.md | 64 +++++++++++++++++++++++++++++++++++++++++++++ docs/DEVELOPMENT.md | 2 +- 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index cca07240..b278ea08 100644 --- a/README.md +++ b/README.md @@ -418,6 +418,61 @@ Options: --help Show this message and exit ``` +## Integrating SkillSpector + +SkillSpector is built to be driven by other tools (CI pipelines, install gates, editor integrations). Its exit code and JSON output are a stable contract. + +### Exit codes + +`skillspector scan` exits with: + +| Code | Meaning | +|------|---------| +| `0` | Scan completed, `risk_score` ≤ 50 (recommendation `SAFE` or `CAUTION`) | +| `1` | Scan completed, `risk_score` > 50 (recommendation `DO_NOT_INSTALL`) | +| `2` | Error (bad input, unreadable source, internal failure) | + +> The exit code collapses `SAFE` and `CAUTION` into `0`. To act differently on them (e.g. *warn* on `CAUTION` but *block* on `DO_NOT_INSTALL`), read the `recommendation` field from the JSON output rather than relying on the exit code. + +### Machine-readable output + +`--format json` produces a JSON report; with no `--output`/`-o` it is written to stdout: + +```bash +skillspector scan ./my-skill/ --format json +``` + +The top-level shape is (this example shows a full LLM-backed scan; with `--no-llm`, `metadata.llm_requested` is `false`): + +```json +{ + "skill": { "name": "...", "source": "...", "scanned_at": "" }, + "risk_assessment": { "score": 0, "severity": "LOW", "recommendation": "SAFE" }, + "components": [ { "path": "...", "type": "...", "lines": 0, "executable": false, "size_bytes": 0 } ], + "issues": [ { "id": "...", "category": "...", "severity": "...", "confidence": 0.0, "location": { "file": "...", "start_line": 0 } } ], + "metadata": { "has_executable_scripts": false, "skillspector_version": "...", "llm_requested": true, "llm_available": true } +} +``` + +- `risk_assessment.severity` ∈ `LOW | MEDIUM | HIGH | CRITICAL`. +- `risk_assessment.recommendation` ∈ `SAFE | CAUTION | DO_NOT_INSTALL`, mapped from severity: `LOW → SAFE`, `MEDIUM → CAUTION`, `HIGH`/`CRITICAL → DO_NOT_INSTALL`. +- `metadata.llm_error` appears only when LLM analysis was requested but unavailable. +- The full per-issue shape is defined by `Finding.to_dict()` in [models.py](src/skillspector/models.py); rely on the fields above and treat any additional fields as best-effort. + +For CI/IDE tooling, `--format sarif` emits SARIF 2.1.0. + +### Recommended gate mapping + +When using SkillSpector as an install gate, map the recommendation to an action: + +| `recommendation` | Suggested action | +|------------------|------------------| +| `SAFE` | allow | +| `CAUTION` | prompt / warn the user | +| `DO_NOT_INSTALL` | block | + +SkillSpector computes the score band and recommendation; how strict the gate is (e.g. whether `CAUTION` blocks in CI) is a policy decision for the integrating tool. + ## Development ### Setup @@ -476,6 +531,15 @@ SC4 uses the [OSV.dev](https://osv.dev) API to check dependencies against the fu The tool requires outbound HTTPS access to `api.osv.dev` for live vulnerability data. When that is not available, findings are limited to the static fallback list. +## Trust model and data egress + +SkillSpector is defense-in-depth, not a sandbox. Know what it does and does not do before relying on it: + +- **It never executes the scanned skill.** All analysis is static (regex, Python AST, YARA) plus optional LLM evaluation of file *contents* — the skill's code is never run. +- **LLM analysis sends file contents to the configured provider.** When LLM analysis is enabled (the default), file contents are sent to the active `SKILLSPECTOR_PROVIDER` endpoint. Use `--no-llm` to keep contents local (static analysis only). +- **SC4 sends dependency names to OSV.dev.** The supply-chain check queries [OSV.dev](https://osv.dev) with the package names and versions the skill declares, to look up known CVEs. This is fundamental to the check and runs even with `--no-llm`. It sends dependency coordinates (not file contents), requires no API key, and falls back to a bundled list when OSV.dev is unreachable. +- **It does not sandbox the host.** SkillSpector flags risky patterns *before* you install a skill; it does not contain or isolate a skill you choose to install anyway. + ## Limitations - **Non-English content**: May miss patterns in other languages diff --git a/docs/DEVELOPMENT.md b/docs/DEVELOPMENT.md index d80db177..f48da46d 100644 --- a/docs/DEVELOPMENT.md +++ b/docs/DEVELOPMENT.md @@ -188,7 +188,7 @@ skillspector scan ./skill.zip --no-llm # static analysis only skillspector --version ``` -The CLI passes `input_path` to the graph. The **resolve_input** node (using [input_handler.py](../src/skillspector/input_handler.py)) resolves Git URL, file URL, .zip, single .md file, or directory to a local directory and sets `skill_path` (and `temp_dir_for_cleanup` when a temp dir was created). The CLI cleans up `temp_dir_for_cleanup` after invoke. Exit code 1 if risk_score > 50; exit code 2 on error. +The CLI passes `input_path` to the graph. The **resolve_input** node (using [input_handler.py](../src/skillspector/input_handler.py)) resolves Git URL, file URL, .zip, single .md file, or directory to a local directory and sets `skill_path` (and `temp_dir_for_cleanup` when a temp dir was created). The CLI cleans up `temp_dir_for_cleanup` after invoke. Exit code 1 if risk_score > 50; exit code 2 on error. See [Integrating SkillSpector](../README.md#integrating-skillspector) for the full exit-code and JSON contract. ### Programmatic From cd539bff6e6597b8f5180b693d07ff127338d317 Mon Sep 17 00:00:00 2001 From: Jiaying Huang Date: Mon, 15 Jun 2026 17:02:57 +0800 Subject: [PATCH 008/104] fix(meta-analyzer): keep LLM-confirmed findings when model returns end_line apply_filter dropped LLM-confirmed findings whose static end_line is None when the model populated end_line (e.g. end_line == start_line, as DeepSeek does): the granular, start_only and coarse lookups all missed and the finding was silently filtered out. This turned a CRITICAL skill (live OSV CVEs) into SAFE once LLM analysis was enabled. Add an end_line-agnostic fallback keyed by (file, rule_id, start_line), gated on the static finding having end_line is None so findings deliberately distinguished by end_line keep exact matching. Add regression tests. Fixes #67 Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Jiaying Huang --- src/skillspector/nodes/meta_analyzer.py | 6 ++ tests/nodes/test_meta_analyzer.py | 100 ++++++++++++++++++++++++ 2 files changed, 106 insertions(+) create mode 100644 tests/nodes/test_meta_analyzer.py diff --git a/src/skillspector/nodes/meta_analyzer.py b/src/skillspector/nodes/meta_analyzer.py index 8f2b5410..87e478bd 100644 --- a/src/skillspector/nodes/meta_analyzer.py +++ b/src/skillspector/nodes/meta_analyzer.py @@ -282,6 +282,8 @@ def apply_filter( """ _enrichment = tuple[str, str, float] confirmed_granular: dict[tuple[str, str, int, int | None], _enrichment] = {} + # Fallback index keyed without end_line (see lookup below). Issue #67. + confirmed_by_start: dict[tuple[str, str, int], _enrichment] = {} confirmed_coarse: dict[tuple[str, str], _enrichment] = {} for batch, llm_items in batch_results: @@ -308,6 +310,7 @@ def apply_filter( int(end_line) if end_line is not None else None, ) ] = enrichment + confirmed_by_start[(file_path, pattern_id, int(start_line))] = enrichment else: confirmed_coarse[(file_path, pattern_id)] = enrichment @@ -316,10 +319,13 @@ def apply_filter( exact_key = (f.file, f.rule_id, f.start_line, f.end_line) start_only_key = (f.file, f.rule_id, f.start_line, None) coarse_key = (f.file, f.rule_id) + start_key = (f.file, f.rule_id, f.start_line) if f.start_line is not None else None if exact_key in confirmed_granular: expl, rem, conf = confirmed_granular[exact_key] elif start_only_key in confirmed_granular: expl, rem, conf = confirmed_granular[start_only_key] + elif f.end_line is None and start_key is not None and start_key in confirmed_by_start: + expl, rem, conf = confirmed_by_start[start_key] elif coarse_key in confirmed_coarse: expl, rem, conf = confirmed_coarse[coarse_key] else: diff --git a/tests/nodes/test_meta_analyzer.py b/tests/nodes/test_meta_analyzer.py new file mode 100644 index 00000000..0087f463 --- /dev/null +++ b/tests/nodes/test_meta_analyzer.py @@ -0,0 +1,100 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Unit tests for LLMMetaAnalyzer.apply_filter (no LLM / no network).""" + +from skillspector.llm_analyzer_base import Batch +from skillspector.models import Finding +from skillspector.nodes.meta_analyzer import LLMMetaAnalyzer + + +def _analyzer() -> LLMMetaAnalyzer: + # Skip __init__ so no LLM client / API key is needed; apply_filter is pure. + return LLMMetaAnalyzer.__new__(LLMMetaAnalyzer) + + +def _finding(rule_id: str, start_line: int, end_line: int | None = None) -> Finding: + return Finding( + rule_id=rule_id, + message=f"static finding {rule_id}", + severity="CRITICAL", + confidence=0.9, + file="requirements.txt", + start_line=start_line, + end_line=end_line, + ) + + +def _llm_item(rule_id: str, start_line: int, **kw: object) -> dict[str, object]: + item: dict[str, object] = { + "pattern_id": rule_id, + "is_vulnerability": True, + "confidence": 1.0, + "start_line": start_line, + "_file": "requirements.txt", + } + item.update(kw) + return item + + +def test_confirmed_finding_kept_when_model_returns_end_line() -> None: + """Regression: a static finding with end_line=None must still match a + confirmation whose end_line is populated (e.g. end_line == start_line, as + some models return). Previously these confirmed findings were silently + dropped. See issue #67.""" + findings = [_finding("SC4", 4), _finding("SC4", 5)] + items = [_llm_item("SC4", 4, end_line=4), _llm_item("SC4", 5, end_line=5)] + batch = Batch(file_path="requirements.txt", content="", findings=findings) + + kept = _analyzer().apply_filter(findings, [(batch, items)]) + + assert {f.start_line for f in kept} == {4, 5} + assert len(kept) == 2 + + +def test_rejected_finding_still_dropped() -> None: + """The end_line-agnostic fallback must not resurrect findings the LLM + rejected (is_vulnerability=False).""" + findings = [_finding("SC4", 4)] + items = [_llm_item("SC4", 4, end_line=4, is_vulnerability=False)] + batch = Batch(file_path="requirements.txt", content="", findings=findings) + + kept = _analyzer().apply_filter(findings, [(batch, items)]) + + assert kept == [] + + +def test_low_confidence_finding_dropped() -> None: + """Confirmations below the confidence threshold are not kept.""" + findings = [_finding("SC4", 4)] + items = [_llm_item("SC4", 4, end_line=4, confidence=0.3)] + batch = Batch(file_path="requirements.txt", content="", findings=findings) + + kept = _analyzer().apply_filter(findings, [(batch, items)]) + + assert kept == [] + + +def test_exact_end_line_match_still_works() -> None: + """Existing behaviour: when both sides carry the same concrete end_line, + the finding is kept (no regression from the new fallback).""" + findings = [_finding("AST1", 21, end_line=21)] + items = [_llm_item("AST1", 21, end_line=21)] + batch = Batch(file_path="requirements.txt", content="", findings=findings) + + kept = _analyzer().apply_filter(findings, [(batch, items)]) + + assert len(kept) == 1 + assert kept[0].rule_id == "AST1" From 7f4a69574b139738b95a819b6527e9f06583c781 Mon Sep 17 00:00:00 2001 From: dc995 Date: Tue, 16 Jun 2026 10:58:21 -0400 Subject: [PATCH 009/104] Fix Windows path separators and console encoding build_context emitted component/file-cache paths using the OS-native separator (e.g. scripts\helper.py on Windows), which broke SARIF output and tests that expect POSIX-style paths. Normalize relative paths with Path.as_posix() so analysis output is identical across platforms. Apply the same s_posix() normalization to the five test helpers that build file caches from fixture directories. The CLI terminal report also crashed on Windows consoles (cp1252) with UnicodeEncodeError when rendering box-drawing characters and icons. Reconfigure stdout/stderr to UTF-8 (errors="replace") at startup so the report renders without crashing on any platform. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/skillspector/cli.py | 21 +++++++++++++++++++ src/skillspector/nodes/build_context.py | 2 +- .../test_semantic_developer_intent.py | 2 +- .../test_semantic_security_discovery.py | 2 +- tests/nodes/test_semantic_quality_policy.py | 2 +- tests/test_mcp_least_privilege.py | 2 +- tests/test_mcp_tool_poisoning.py | 2 +- 7 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/skillspector/cli.py b/src/skillspector/cli.py index 83e3224a..75a5c14f 100644 --- a/src/skillspector/cli.py +++ b/src/skillspector/cli.py @@ -24,6 +24,7 @@ import json import os import shutil +import sys from enum import StrEnum from pathlib import Path from typing import Annotated @@ -38,6 +39,26 @@ logger = get_logger(__name__) + +def _ensure_utf8_streams() -> None: + """Reconfigure stdout/stderr to UTF-8 so Unicode report output does not crash. + + On Windows the default console encoding (e.g. cp1252) cannot encode the + box-drawing characters and icons used in the terminal report, which raises + UnicodeEncodeError. Reconfiguring with errors="replace" makes output robust + across platforms without crashing. + """ + for stream in (sys.stdout, sys.stderr): + reconfigure = getattr(stream, "reconfigure", None) + if reconfigure is not None: + try: + reconfigure(encoding="utf-8", errors="replace") + except (ValueError, OSError): + logger.debug("Could not reconfigure %s to UTF-8", stream) + + +_ensure_utf8_streams() + app = typer.Typer( name="skillspector", help="Security scanner for AI agent skills (LangGraph). Detect vulnerabilities before installation.", diff --git a/src/skillspector/nodes/build_context.py b/src/skillspector/nodes/build_context.py index 694a0487..947ab885 100644 --- a/src/skillspector/nodes/build_context.py +++ b/src/skillspector/nodes/build_context.py @@ -87,7 +87,7 @@ def _walk_skill_files(skill_dir: Path) -> list[str]: continue try: rel = item.relative_to(skill_dir) - paths.append(str(rel)) + paths.append(rel.as_posix()) except ValueError: logger.debug("Skipping path (not under skill_dir): %s", item) continue diff --git a/tests/nodes/analyzers/test_semantic_developer_intent.py b/tests/nodes/analyzers/test_semantic_developer_intent.py index 0ddc7044..097b9b1f 100644 --- a/tests/nodes/analyzers/test_semantic_developer_intent.py +++ b/tests/nodes/analyzers/test_semantic_developer_intent.py @@ -342,7 +342,7 @@ def _build_file_cache(skill_dir: Path) -> dict[str, str]: for item in sorted(skill_dir.rglob("*")): if not item.is_file(): continue - rel = str(item.relative_to(skill_dir)) + rel = item.relative_to(skill_dir).as_posix() try: cache[rel] = item.read_text(encoding="utf-8", errors="replace") except OSError: diff --git a/tests/nodes/analyzers/test_semantic_security_discovery.py b/tests/nodes/analyzers/test_semantic_security_discovery.py index 85209f63..4883fec6 100644 --- a/tests/nodes/analyzers/test_semantic_security_discovery.py +++ b/tests/nodes/analyzers/test_semantic_security_discovery.py @@ -317,7 +317,7 @@ def _build_file_cache(skill_dir: Path) -> dict[str, str]: for item in sorted(skill_dir.rglob("*")): if not item.is_file(): continue - rel = str(item.relative_to(skill_dir)) + rel = item.relative_to(skill_dir).as_posix() try: cache[rel] = item.read_text(encoding="utf-8", errors="replace") except OSError: diff --git a/tests/nodes/test_semantic_quality_policy.py b/tests/nodes/test_semantic_quality_policy.py index f80960b8..7cce2377 100644 --- a/tests/nodes/test_semantic_quality_policy.py +++ b/tests/nodes/test_semantic_quality_policy.py @@ -289,7 +289,7 @@ def _build_file_cache(skill_dir: Path) -> dict[str, str]: for item in sorted(skill_dir.rglob("*")): if not item.is_file(): continue - rel = str(item.relative_to(skill_dir)) + rel = item.relative_to(skill_dir).as_posix() try: cache[rel] = item.read_text(encoding="utf-8", errors="replace") except OSError: diff --git a/tests/test_mcp_least_privilege.py b/tests/test_mcp_least_privilege.py index 9e7852e6..d9a045fc 100644 --- a/tests/test_mcp_least_privilege.py +++ b/tests/test_mcp_least_privilege.py @@ -98,7 +98,7 @@ def _make_state(fixture_name: str) -> dict: continue if item.name.startswith(".") and not item.name.startswith(".claude"): continue - rel = str(item.relative_to(fixture_dir)) + rel = item.relative_to(fixture_dir).as_posix() components.append(rel) components.sort() diff --git a/tests/test_mcp_tool_poisoning.py b/tests/test_mcp_tool_poisoning.py index 7d5b5241..9e3e25f7 100644 --- a/tests/test_mcp_tool_poisoning.py +++ b/tests/test_mcp_tool_poisoning.py @@ -121,7 +121,7 @@ def _make_state( continue if item.name.startswith(".") and not item.name.startswith(".claude"): continue - rel = str(item.relative_to(fixture_dir)) + rel = item.relative_to(fixture_dir).as_posix() components.append(rel) components.sort() From bc6c542b88daed8ac33ec4d8f9b5c035693217c8 Mon Sep 17 00:00:00 2001 From: Ram Dwivedi Date: Tue, 16 Jun 2026 23:39:01 -0400 Subject: [PATCH 010/104] fix(build_context): use forward-slash component paths (cross-platform) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit build_context emitted OS-native path separators for relative component paths, producing backslashes on Windows (e.g. `references\guide.md`). These paths are used as dict keys (components / file_cache / component_metadata) and as SARIF physicalLocation URIs, which are meant to be portable forward-slash paths — so on Windows the report locations were non-portable and downstream lookups (and the test suite) mismatched. Use Path.as_posix() so component paths are forward-slash on every OS, in build_context and the handful of test helpers that mirror the same relative_to logic. Behaviour-preserving on Linux/macOS (str() already yields forward slashes). Fixes #86 Co-Authored-By: Claude Opus 4.8 Signed-off-by: Ram Dwivedi --- src/skillspector/nodes/build_context.py | 5 ++++- tests/nodes/analyzers/test_semantic_developer_intent.py | 2 +- tests/nodes/analyzers/test_semantic_security_discovery.py | 2 +- tests/nodes/test_semantic_quality_policy.py | 2 +- tests/test_mcp_least_privilege.py | 2 +- tests/test_mcp_tool_poisoning.py | 2 +- 6 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/skillspector/nodes/build_context.py b/src/skillspector/nodes/build_context.py index 694a0487..225c26d5 100644 --- a/src/skillspector/nodes/build_context.py +++ b/src/skillspector/nodes/build_context.py @@ -87,7 +87,10 @@ def _walk_skill_files(skill_dir: Path) -> list[str]: continue try: rel = item.relative_to(skill_dir) - paths.append(str(rel)) + # Use forward slashes on every OS: these relative paths are dict keys + # and SARIF/URI locations, so they must be portable (not OS-specific + # backslashes on Windows). + paths.append(rel.as_posix()) except ValueError: logger.debug("Skipping path (not under skill_dir): %s", item) continue diff --git a/tests/nodes/analyzers/test_semantic_developer_intent.py b/tests/nodes/analyzers/test_semantic_developer_intent.py index 0ddc7044..408daa94 100644 --- a/tests/nodes/analyzers/test_semantic_developer_intent.py +++ b/tests/nodes/analyzers/test_semantic_developer_intent.py @@ -342,7 +342,7 @@ def _build_file_cache(skill_dir: Path) -> dict[str, str]: for item in sorted(skill_dir.rglob("*")): if not item.is_file(): continue - rel = str(item.relative_to(skill_dir)) + rel = item.relative_to(skill_dir).as_posix() # forward slashes on every OS try: cache[rel] = item.read_text(encoding="utf-8", errors="replace") except OSError: diff --git a/tests/nodes/analyzers/test_semantic_security_discovery.py b/tests/nodes/analyzers/test_semantic_security_discovery.py index 85209f63..5b0c53b5 100644 --- a/tests/nodes/analyzers/test_semantic_security_discovery.py +++ b/tests/nodes/analyzers/test_semantic_security_discovery.py @@ -317,7 +317,7 @@ def _build_file_cache(skill_dir: Path) -> dict[str, str]: for item in sorted(skill_dir.rglob("*")): if not item.is_file(): continue - rel = str(item.relative_to(skill_dir)) + rel = item.relative_to(skill_dir).as_posix() # forward slashes on every OS try: cache[rel] = item.read_text(encoding="utf-8", errors="replace") except OSError: diff --git a/tests/nodes/test_semantic_quality_policy.py b/tests/nodes/test_semantic_quality_policy.py index f80960b8..9d52d335 100644 --- a/tests/nodes/test_semantic_quality_policy.py +++ b/tests/nodes/test_semantic_quality_policy.py @@ -289,7 +289,7 @@ def _build_file_cache(skill_dir: Path) -> dict[str, str]: for item in sorted(skill_dir.rglob("*")): if not item.is_file(): continue - rel = str(item.relative_to(skill_dir)) + rel = item.relative_to(skill_dir).as_posix() # forward slashes on every OS try: cache[rel] = item.read_text(encoding="utf-8", errors="replace") except OSError: diff --git a/tests/test_mcp_least_privilege.py b/tests/test_mcp_least_privilege.py index 9e7852e6..8195986f 100644 --- a/tests/test_mcp_least_privilege.py +++ b/tests/test_mcp_least_privilege.py @@ -98,7 +98,7 @@ def _make_state(fixture_name: str) -> dict: continue if item.name.startswith(".") and not item.name.startswith(".claude"): continue - rel = str(item.relative_to(fixture_dir)) + rel = item.relative_to(fixture_dir).as_posix() # forward slashes on every OS components.append(rel) components.sort() diff --git a/tests/test_mcp_tool_poisoning.py b/tests/test_mcp_tool_poisoning.py index 7d5b5241..428897c0 100644 --- a/tests/test_mcp_tool_poisoning.py +++ b/tests/test_mcp_tool_poisoning.py @@ -121,7 +121,7 @@ def _make_state( continue if item.name.startswith(".") and not item.name.startswith(".claude"): continue - rel = str(item.relative_to(fixture_dir)) + rel = item.relative_to(fixture_dir).as_posix() # forward slashes on every OS components.append(rel) components.sort() From c4a92ff565d1b41aca41cf9408328a0e7165a26e Mon Sep 17 00:00:00 2001 From: Ram Dwivedi Date: Wed, 17 Jun 2026 06:42:05 -0400 Subject: [PATCH 011/104] feat(providers): local agent-CLI providers (claude/codex/gemini), no API key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add four agent-CLI LLM providers driven by locally-installed, already-authenticated CLI binaries (claude, codex, gemini, agy) instead of metered HTTP endpoints: SKILLSPECTOR_PROVIDER=claude_cli -> local `claude` OAuth session, no API key SKILLSPECTOR_PROVIDER=codex_cli -> local `codex` session SKILLSPECTOR_PROVIDER=gemini_cli -> local `gemini` session SKILLSPECTOR_PROVIDER=antigravity_cli -> registered but DISABLED (fail-closed; agy is TTY-only, uncapturable on a pipe) Security chokepoint: all subprocess I/O goes through run_agent_cli() in _agent_cli.py (shell=False, prompt via stdin only, capability-stripped argv, env scrub, temp CWD, bounded streaming, fail-closed). _agent_cli_base.py is the shared provider base class; the four concrete providers are ~5-line subclasses. No pinned model: CLI providers forward no --model by default so the user's own configured model is used; SKILLSPECTOR_MODEL overrides. model_registry.yaml files are absent; metadata methods return None and fall through to default token budgets. The AgentCLIChatModel adapter in llm_utils.py mimics the ChatOpenAI interface (.invoke / .ainvoke / .with_structured_output) backed by the provider's complete() subprocess transport, so existing LLM analyzers (meta_analyzer, semantic_*) work with no code changes. Rebased and adapted onto upstream provider refactor (a5092dd): - providers/base.py: adds AgentCLICapable + has_cli_capability alongside upstream's new ChatModelProvider / LLMProvider protocols. - providers/__init__.py: registers CLI providers in _select_active_provider; preserves upstream's create_chat_model / resolve_chat_model_credentials / NO_LLM_API_KEY_MESSAGE / raise_no_llm_api_key_configured; CLI branch skips create_chat_model (no HTTP transport) and calls raise_no_llm_api_key_configured. - llm_utils.py: get_chat_model branches on has_cli_capability — returns AgentCLIChatModel for CLI providers; delegates to providers.create_chat_model (which uses upstream's native-client path, e.g. ChatAnthropic) for HTTP ones. - Tests: merged upstream's ChatAnthropic/create_chat_model/NO_LLM_API_KEY_MESSAGE coverage with PR#52's CLI dispatch/adapter/antigravity/no-pinned-model tests. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Ram Dwivedi --- README.md | 16 +- docs/DEVELOPMENT.md | 20 +- src/skillspector/llm_utils.py | 193 +++- src/skillspector/providers/__init__.py | 86 +- src/skillspector/providers/_agent_cli.py | 821 ++++++++++++++++++ src/skillspector/providers/_agent_cli_base.py | 92 ++ .../providers/antigravity_cli/__init__.py | 22 + .../providers/antigravity_cli/provider.py | 48 + src/skillspector/providers/base.py | 54 +- .../providers/claude_cli/__init__.py | 25 + .../providers/claude_cli/provider.py | 43 + .../providers/codex_cli/__init__.py | 29 + .../providers/codex_cli/provider.py | 45 + .../providers/gemini_cli/__init__.py | 22 + .../providers/gemini_cli/provider.py | 36 + tests/integration/test_agent_cli_live.py | 135 +++ tests/unit/test_agent_cli.py | 722 +++++++++++++++ tests/unit/test_llm_utils.py | 131 +++ tests/unit/test_providers.py | 122 +++ 19 files changed, 2622 insertions(+), 40 deletions(-) create mode 100644 src/skillspector/providers/_agent_cli.py create mode 100644 src/skillspector/providers/_agent_cli_base.py create mode 100644 src/skillspector/providers/antigravity_cli/__init__.py create mode 100644 src/skillspector/providers/antigravity_cli/provider.py create mode 100644 src/skillspector/providers/claude_cli/__init__.py create mode 100644 src/skillspector/providers/claude_cli/provider.py create mode 100644 src/skillspector/providers/codex_cli/__init__.py create mode 100644 src/skillspector/providers/codex_cli/provider.py create mode 100644 src/skillspector/providers/gemini_cli/__init__.py create mode 100644 src/skillspector/providers/gemini_cli/provider.py create mode 100644 tests/integration/test_agent_cli_live.py create mode 100644 tests/unit/test_agent_cli.py diff --git a/README.md b/README.md index 6984998b..9d25273e 100644 --- a/README.md +++ b/README.md @@ -149,6 +149,8 @@ inference gateways. | `openai` | `OPENAI_API_KEY` (+ optional `OPENAI_BASE_URL`) | api.openai.com (or any OpenAI-compatible URL) | `gpt-5.4` | | `anthropic` | `ANTHROPIC_API_KEY` | api.anthropic.com | `claude-opus-4-6` | | `nv_build` | `NVIDIA_INFERENCE_KEY` | build.nvidia.com | `deepseek-ai/deepseek-v4-flash` | +| `claude_cli` | _(none — uses local CLI auth)_ | local `claude` binary | `claude-sonnet-4-6` | +| `codex_cli` | _(none — uses local CLI auth)_ | local `codex` binary | `o4-mini` | ```bash # Stock OpenAI @@ -166,6 +168,16 @@ export SKILLSPECTOR_PROVIDER=nv_build export NVIDIA_INFERENCE_KEY=nvapi-... skillspector scan ./my-skill/ +# Local Claude CLI — no API key; uses your existing `claude auth login` session +# Requires: claude CLI installed and authenticated (claude auth login) +export SKILLSPECTOR_PROVIDER=claude_cli +skillspector scan ./my-skill/ + +# Local Codex CLI — no API key; uses your existing `codex login` session +# Requires: codex CLI installed and authenticated +export SKILLSPECTOR_PROVIDER=codex_cli +skillspector scan ./my-skill/ + # Local Ollama or any OpenAI-compatible endpoint export SKILLSPECTOR_PROVIDER=openai export OPENAI_API_KEY=ollama @@ -396,7 +408,7 @@ Issues (2) | Variable | Description | Required | |----------|-------------|----------| -| `SKILLSPECTOR_PROVIDER` | Active LLM provider: `openai`, `anthropic`, or `nv_build`. Each provider has its own bundled `model_registry.yaml` and default model (see the LLM Analysis table above). Defaults to `nv_build`. | Optional | +| `SKILLSPECTOR_PROVIDER` | Active LLM provider: `openai`, `anthropic`, `nv_build`, `claude_cli`, or `codex_cli`. Each provider has its own bundled `model_registry.yaml` and default model (see the LLM Analysis table above). Defaults to `nv_build`. | Optional | | `NVIDIA_INFERENCE_KEY` | Credential for the `nv_build` provider (build.nvidia.com). | Required for LLM analysis when `SKILLSPECTOR_PROVIDER=nv_build` | | `OPENAI_API_KEY` | Credential for the OpenAI provider (`SKILLSPECTOR_PROVIDER=openai`). Also serves as the tier-2 fallback in the credential waterfall when the active provider returns no credentials. | Required for LLM analysis when `SKILLSPECTOR_PROVIDER=openai` | | `OPENAI_BASE_URL` | Override the OpenAI endpoint (e.g. point at Ollama). | Optional | @@ -405,6 +417,8 @@ Issues (2) | `SKILLSPECTOR_MODEL_REGISTRY` | Override the bundled per-provider YAML registry (`src/skillspector/providers//model_registry.yaml`) with a custom path. | Optional | | `SKILLSPECTOR_LOG_LEVEL` | Log level: `DEBUG`, `INFO`, `WARNING`, `ERROR` (default: `WARNING`). | Optional | +> **CLI providers** (`claude_cli`, `codex_cli`): No API key is needed. Authentication is managed entirely by the agent CLI's own login session (`claude auth login` / `codex login`). SkillSpector never reads or forwards API keys when these providers are active. The subprocess is run in a hardened sandbox: tools disabled, no MCP, read-only sandbox mode (codex), and untrusted skill content is delivered only via stdin. + ### CLI Options ```bash diff --git a/docs/DEVELOPMENT.md b/docs/DEVELOPMENT.md index 82387fae..96224f18 100644 --- a/docs/DEVELOPMENT.md +++ b/docs/DEVELOPMENT.md @@ -260,12 +260,14 @@ Copy [.env.example](../.env.example) to `.env` in the project root and set value | Variable | Description | Example | |----------|-------------|---------| -| `SKILLSPECTOR_PROVIDER` | Active LLM provider: `openai` \| `anthropic` \| `nv_build`. Defaults to `nv_build`. | `openai` | +| `SKILLSPECTOR_PROVIDER` | Active LLM provider: `openai` \| `anthropic` \| `nv_build` \| `claude_cli` \| `codex_cli`. Defaults to `nv_build`. | `claude_cli` | | `NVIDIA_INFERENCE_KEY` | Credential for `nv_build`. | `nvapi-...` | | `OPENAI_API_KEY` | Credential for `SKILLSPECTOR_PROVIDER=openai`. Also tier-2 fallback for non-OpenAI providers. | `sk-...` | | `OPENAI_BASE_URL` | Override the OpenAI endpoint (e.g. point at Ollama). | `http://localhost:11434/v1` | | `ANTHROPIC_API_KEY` | Credential for `SKILLSPECTOR_PROVIDER=anthropic`. | `sk-ant-...` | -| `SKILLSPECTOR_MODEL` | Override the active provider's bundled default model (see [README.md](../README.md) for per-provider defaults). | `gpt-5.2` | +| `SKILLSPECTOR_MODEL` | Override the active provider's bundled default model (see [README.md](../README.md) for per-provider defaults). For `claude_cli`, this is passed as `--model` to the `claude` binary. | `gpt-5.2` | + +> **CLI providers** (`claude_cli`, `codex_cli`): no credential env var is needed. Authentication is managed by the agent CLI's own session (`claude auth login` / `codex login`). The subprocess is heavily sandboxed — see [providers/_agent_cli.py](../src/skillspector/providers/_agent_cli.py). ### Live provider tests @@ -286,8 +288,18 @@ Base URL env vars are not needed for live provider tests; the tests intentionall - **`get_max_input_tokens(model)`** — input budget per LLM request (75% of resolved context window). - **`get_max_output_tokens(model)`** — output budget per LLM request (min of 25% context, registry's `max_output_tokens` cap if set). - Batch budget overhead is computed per-prompt via `estimate_tokens(base_prompt)` rather than a fixed constant. -- **Providers** ([providers/](../src/skillspector/providers/)): pluggable credential + token-budget resolvers. Each provider is a subpackage with its own `provider.py` and bundled `model_registry.yaml`; [registry.py](../src/skillspector/providers/registry.py) exposes `lookup_context_length` / `lookup_max_output_tokens` utilities the providers call directly. The active provider is chosen by `SKILLSPECTOR_PROVIDER` (default: `nv_build`) — see [providers/`__init__`.py](../src/skillspector/providers/__init__.py): `nv_build/` (build.nvidia.com), `openai/`, or `anthropic/`. -- **LLM calls** ([llm_utils.py](../src/skillspector/llm_utils.py)): **`get_chat_model()`** and **`chat_completion()`** resolve credentials in two tiers — active NVIDIA provider (`NVIDIA_INFERENCE_KEY` → endpoint) → standard `OPENAI_API_KEY` / `OPENAI_BASE_URL` — against any OpenAI-compatible endpoint. `max_tokens` is auto-bound to `get_max_output_tokens(model)` from `model_info`. +- **Providers** ([providers/](../src/skillspector/providers/)): pluggable credential + token-budget resolvers. Each provider is a subpackage with its own `provider.py` and bundled `model_registry.yaml`; [registry.py](../src/skillspector/providers/registry.py) exposes `lookup_context_length` / `lookup_max_output_tokens` utilities the providers call directly. The active provider is chosen by `SKILLSPECTOR_PROVIDER` (default: `nv_build`): + - `nv_build/` — build.nvidia.com (HTTP, `NVIDIA_INFERENCE_KEY`) + - `openai/` — api.openai.com or any OpenAI-compatible URL (`OPENAI_API_KEY`) + - `anthropic/` — api.anthropic.com (`ANTHROPIC_API_KEY`) + - `claude_cli/` — **local `claude` binary; no API key**. Uses the CLI's own auth session (`claude auth login`). Set `SKILLSPECTOR_PROVIDER=claude_cli`. + - `codex_cli/` — **local `codex` binary; no API key**. Uses the CLI's own auth session (`codex login`). Set `SKILLSPECTOR_PROVIDER=codex_cli`. + + CLI providers (`claude_cli`, `codex_cli`) implement the optional `AgentCLICapable` interface (`is_available()` + `complete()`) defined in [providers/base.py](../src/skillspector/providers/base.py). `has_cli_capability(provider)` detects this at runtime. All subprocess calls go through the hardened helper [providers/_agent_cli.py](../src/skillspector/providers/_agent_cli.py) which enforces: no shell (`shell=False`), untrusted content via stdin only, capability stripping (tools disabled / sandboxed), environment scrubbing (no API keys forwarded), per-call timeout, and fail-closed error handling. + +- **LLM calls** ([llm_utils.py](../src/skillspector/llm_utils.py)): **`get_chat_model()`** and **`chat_completion()`** dispatch based on the active provider: + - **HTTP providers**: resolve credentials in two tiers — active provider (`NVIDIA_INFERENCE_KEY` / `ANTHROPIC_API_KEY` / `OPENAI_API_KEY` → endpoint) — against any OpenAI-compatible endpoint. `max_tokens` is auto-bound to `get_max_output_tokens(model)` from `model_info`. + - **CLI providers** (`claude_cli`, `codex_cli`): `get_chat_model()` returns an `AgentCLIChatModel` adapter backed by `provider.complete()`, so the analyzers' `.invoke()` / `.with_structured_output(schema).invoke()` calls work with no API key (structured output is produced by prompting for JSON, then Pydantic-validating). `chat_completion()` routes through `get_chat_model()` as well. `is_llm_available()` calls `provider.is_available()` instead of credential resolution. - **LLM analyzer base** ([llm_analyzer_base.py](../src/skillspector/nodes/llm_analyzer_base.py)): `LLMAnalyzerBase` provides per-file/per-chunk batching, token-budget-aware chunking, and a run loop for all LLM-based analyzers. `LLMMetaAnalyzer` extends it for filter/enrich (meta_analyzer node). Future semantic analyzers extend `LLMAnalyzerBase` for discovery mode. --- diff --git a/src/skillspector/llm_utils.py b/src/skillspector/llm_utils.py index ab6e5518..5e7dd47c 100644 --- a/src/skillspector/llm_utils.py +++ b/src/skillspector/llm_utils.py @@ -13,13 +13,17 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""Shared LLM utilities. +"""Shared LLM utilities (OpenAI-compatible chat models + agent CLI transports). Credentials are resolved in this order: - 1. The active SkillSpector provider (see :mod:`skillspector.providers`) — - reads its own credential env var and supplies the matching client. + 1. The active provider (see :mod:`skillspector.providers`): + - CLI providers (``claude_cli``, ``codex_cli``, ``gemini_cli``): use + ``is_available()`` and ``complete()`` — no API key needed. + - HTTP providers (``anthropic``, ``openai``, ``nv_build``): read their + respective credential env vars and supply a base URL. 2. ``OPENAI_API_KEY`` / ``OPENAI_BASE_URL`` (the langchain-openai - defaults). + defaults) — only consulted for HTTP providers when the provider's + own credential env var is unset. There is no SkillSpector-specific credential env var: setting ``NVIDIA_INFERENCE_KEY`` configures whichever NVIDIA endpoint the @@ -30,13 +34,17 @@ from __future__ import annotations +import asyncio +import json + from langchain_core.language_models.chat_models import BaseChatModel -from langchain_core.messages import BaseMessage from skillspector.constants import MODEL_CONFIG from skillspector.model_info import get_max_input_tokens, get_max_output_tokens from skillspector.providers import ( create_chat_model, + get_active_provider, + has_cli_capability, raise_no_llm_api_key_configured, resolve_chat_model_credentials, ) @@ -45,8 +53,9 @@ def _resolve_llm_credentials() -> tuple[str, str | None]: """Return ``(api_key, base_url)`` resolved from the environment. - Tries the active NVIDIA provider first; falls back to ``OPENAI_API_KEY`` - / ``OPENAI_BASE_URL`` when the provider is not configured. + Tries the active SkillSpector provider first; falls back to + ``OPENAI_API_KEY`` / ``OPENAI_BASE_URL`` when the provider is not + configured. Raises: ValueError: when no API key can be resolved from any source. @@ -58,7 +67,15 @@ def _resolve_llm_credentials() -> tuple[str, str | None]: def is_llm_available() -> tuple[bool, str | None]: - """Return ``(available, error_message)`` describing LLM credential status.""" + """Return ``(available, error_message)`` describing LLM availability. + + For CLI providers (``claude_cli``, ``codex_cli``, ``gemini_cli``) the check + delegates to the provider's ``is_available()`` method (binary on PATH + + auth). For HTTP providers, it falls back to credential resolution. + """ + provider = get_active_provider() + if has_cli_capability(provider): + return provider.is_available() # type: ignore[attr-defined] try: _resolve_llm_credentials() except ValueError as exc: @@ -71,24 +88,160 @@ def fetch_model_token_limits(model_label: str) -> tuple[int, int]: return get_max_input_tokens(model_label), get_max_output_tokens(model_label) -def get_chat_model(model: str | None = None) -> BaseChatModel: - """Return the active provider's native LangChain chat model. +# --------------------------------------------------------------------------- +# Agent CLI chat-model adapter +# --------------------------------------------------------------------------- +# +# The LLM analyzers (meta_analyzer, semantic_*) obtain a model from +# ``get_chat_model()`` and call ``.invoke()`` / ``.with_structured_output( +# schema).invoke()`` on it (see ``llm_analyzer_base``) — they never go through +# ``chat_completion``. To support CLI providers there, ``get_chat_model`` +# returns this minimal adapter, which mimics the slice of the ``ChatOpenAI`` +# interface the analyzers rely on, backed by the provider's ``complete()`` +# subprocess transport. + + +class _AgentCLIMessage: + """Minimal stand-in for a LangChain message: exposes ``.content``.""" + + def __init__(self, content: str) -> None: + self.content = content + + +def _extract_json_object(raw: str) -> dict: + """Extract a single JSON object from a CLI model's text response. + + Tolerates markdown code fences and surrounding prose. Raises ``ValueError`` + (fail-closed) when no JSON object can be parsed. + """ + text = raw.strip() + if text.startswith("```"): + # Drop the opening fence line (``` or ```json) and any closing fence. + text = text.split("\n", 1)[1] if "\n" in text else "" + fence = text.rfind("```") + if fence != -1: + text = text[:fence] + text = text.strip() + try: + obj = json.loads(text) + if isinstance(obj, dict): + return obj + except json.JSONDecodeError: + pass + start, end = text.find("{"), text.rfind("}") + if start != -1 and end > start: + try: + obj = json.loads(text[start : end + 1]) + if isinstance(obj, dict): + return obj + except json.JSONDecodeError: + pass + raise ValueError(f"could not extract a JSON object from CLI response: {raw[:200]!r}") + + +class _StructuredAgentCLIModel: + """Mimics ``ChatOpenAI.with_structured_output(schema)`` for a CLI provider. + + ``invoke`` augments the prompt with the schema, calls the provider's + ``complete()``, then parses and validates the response into *schema*. + """ + + def __init__(self, provider: object, model: str, max_output_tokens: int, schema: type) -> None: + self._provider = provider + self._model = model + self._max_output_tokens = max_output_tokens + self._schema = schema + + def _augment(self, prompt: str) -> str: + schema_json = json.dumps(self._schema.model_json_schema(), indent=2) + return ( + f"{prompt}\n\n" + "Respond with ONLY a single JSON object conforming to the JSON Schema " + "below. Do not wrap it in markdown code fences and do not add any prose " + f"before or after the JSON.\n\nJSON Schema:\n{schema_json}" + ) + + def invoke(self, prompt: str) -> object: + raw = self._provider.complete( # type: ignore[attr-defined] + self._augment(prompt), + model=self._model, + max_output_tokens=self._max_output_tokens, + ) + return self._schema.model_validate(_extract_json_object(raw)) + + async def ainvoke(self, prompt: str) -> object: + return await asyncio.to_thread(self.invoke, prompt) + + +class AgentCLIChatModel: + """Minimal ``ChatOpenAI``-compatible adapter backed by a CLI provider. + + Implements only the surface the analyzers use: ``invoke`` (returns an + object with ``.content``), ``ainvoke``, and ``with_structured_output``. + """ + + def __init__(self, provider: object, model: str, max_output_tokens: int) -> None: + self._provider = provider + self._model = model + self._max_output_tokens = max_output_tokens + + def invoke(self, prompt: str) -> _AgentCLIMessage: + text = self._provider.complete( # type: ignore[attr-defined] + prompt, + model=self._model, + max_output_tokens=self._max_output_tokens, + ) + return _AgentCLIMessage(text) + + async def ainvoke(self, prompt: str) -> _AgentCLIMessage: + return await asyncio.to_thread(self.invoke, prompt) + + def with_structured_output(self, schema: type) -> _StructuredAgentCLIModel: + return _StructuredAgentCLIModel( + self._provider, self._model, self._max_output_tokens, schema + ) + + +def get_chat_model(model: str | None = None) -> BaseChatModel | AgentCLIChatModel: + """Return a chat model for the active provider. + + For CLI providers (``claude_cli``, ``codex_cli``, ``gemini_cli``) this + returns an :class:`AgentCLIChatModel` adapter backed by the provider's + ``complete()`` subprocess transport — so the LLM analyzers (which use + ``.invoke()`` and ``.with_structured_output()``) work with no API key. + + For HTTP providers it delegates to + :func:`skillspector.providers.create_chat_model`, which uses the + provider's own native client (e.g. ``ChatAnthropic`` for Anthropic) with + an ``OPENAI_API_KEY`` / ``ChatOpenAI`` fallback. Raises: - ValueError: when no API key is configured (see ``is_llm_available``). + ValueError: when an HTTP provider has no API key configured. """ - model = model or MODEL_CONFIG["default"] + resolved_model = model or MODEL_CONFIG["default"] + + provider = get_active_provider() + if has_cli_capability(provider): + return AgentCLIChatModel(provider, resolved_model, get_max_output_tokens(resolved_model)) + return create_chat_model( - model=model, - max_tokens=get_max_output_tokens(model), + model=resolved_model, + max_tokens=get_max_output_tokens(resolved_model), timeout=120, ) def chat_completion(prompt: str, *, model: str | None = None) -> str: - """Request a single chat completion and return the assistant text.""" - llm = get_chat_model(model=model) - response = llm.invoke(prompt) - if not isinstance(response, BaseMessage): - raise TypeError(f"Expected BaseMessage from chat model, got {type(response).__name__}") - return str(response.text) + """Request a single chat completion and return the assistant content. + + Routes through :func:`get_chat_model`, which dispatches to the CLI adapter + for CLI providers and to the provider's native chat model for HTTP providers. + + Uses ``.text`` when available (real LangChain ``BaseMessage`` objects, + which normalise content blocks to a single string) and falls back to + ``.content`` for the CLI adapter's ``_AgentCLIMessage``. + """ + response = get_chat_model(model=model).invoke(prompt) + if hasattr(response, "text"): + return response.text # type: ignore[union-attr] + return response.content or "" # type: ignore[union-attr] diff --git a/src/skillspector/providers/__init__.py b/src/skillspector/providers/__init__.py index bf1522e6..9197bc08 100644 --- a/src/skillspector/providers/__init__.py +++ b/src/skillspector/providers/__init__.py @@ -25,8 +25,20 @@ openai → OpenAIProvider (api.openai.com) anthropic → AnthropicProvider (api.anthropic.com) nv_build → NvBuildProvider (build.nvidia.com) + claude_cli → ClaudeCLIProvider (local ``claude`` binary, no API key) + codex_cli → CodexCLIProvider (local ``codex`` binary, no API key) + gemini_cli → GeminiCLIProvider (local ``gemini`` binary, no API key) + antigravity_cli → AntigravityCLIProvider (local ``agy`` binary; registered + but disabled — agy is TTY-only and + can't be captured; use gemini_cli) When unset, the selector defaults to ``nv_build``. + +CLI providers (``claude_cli``, ``codex_cli``, ``gemini_cli``) implement the +optional :class:`~skillspector.providers.base.AgentCLICapable` interface — they +expose ``is_available()`` and ``complete()`` so that +:func:`skillspector.llm_utils.get_chat_model` uses the local CLI subprocess +instead of the ``ChatOpenAI`` HTTP transport. """ from __future__ import annotations @@ -36,7 +48,14 @@ from langchain_core.language_models.chat_models import BaseChatModel -from .base import ChatModelProvider, CredentialsProvider, LLMProvider, ModelMetadataProvider +from .base import ( + AgentCLICapable, + ChatModelProvider, + CredentialsProvider, + LLMProvider, + ModelMetadataProvider, + has_cli_capability, +) from .nv_build import NvBuildProvider NO_LLM_API_KEY_MESSAGE = ( @@ -66,6 +85,22 @@ def _select_active_provider() -> LLMProvider: return AnthropicProvider() if name == "nv_build": return NvBuildProvider() + if name == "claude_cli": + from .claude_cli import ClaudeCLIProvider + + return ClaudeCLIProvider() + if name == "codex_cli": + from .codex_cli import CodexCLIProvider + + return CodexCLIProvider() + if name == "gemini_cli": + from .gemini_cli import GeminiCLIProvider + + return GeminiCLIProvider() + if name == "antigravity_cli": + from .antigravity_cli import AntigravityCLIProvider + + return AntigravityCLIProvider() if name in ("nv_inference", ""): # Try the optional nv_inference subpackage if it's bundled with # this installation; otherwise fall through to nv_build. @@ -78,7 +113,8 @@ def _select_active_provider() -> LLMProvider: raise ValueError( f"Unknown SKILLSPECTOR_PROVIDER: {name!r}. " - "Expected one of: openai, anthropic, nv_build (or unset)." + "Expected one of: openai, anthropic, nv_build, claude_cli, codex_cli, " + "gemini_cli, antigravity_cli (or unset)." ) @@ -87,11 +123,22 @@ def get_metadata_provider() -> ModelMetadataProvider: return _select_active_provider() +def get_active_provider() -> ModelMetadataProvider: + """Return the active provider (alias for :func:`get_metadata_provider`). + + Preferred over :func:`get_metadata_provider` when callers also need to + check for optional capabilities (e.g. :func:`has_cli_capability`). + """ + return _select_active_provider() + + def resolve_provider_credentials() -> tuple[str, str | None] | None: """Return ``(api_key, base_url)`` from the active provider. Returns ``None`` when the provider's credential env var is unset, so - callers can fall through to other credential sources. + callers can fall through to other credential sources. CLI providers + always return ``None`` from this method; availability is checked via + ``is_available()`` instead. """ return _select_active_provider().resolve_credentials() @@ -120,37 +167,48 @@ def create_chat_model( ) -> BaseChatModel: """Create the active provider's native LangChain chat model. + CLI providers (``claude_cli``, ``codex_cli``, ``gemini_cli``) do not have + a native LangChain chat model — callers that need CLI transport should use + :func:`skillspector.llm_utils.get_chat_model` instead (which returns an + :class:`~skillspector.llm_utils.AgentCLIChatModel` adapter). + If the active provider is not configured, fall back to standard OpenAI environment variables. This preserves the historical ``OPENAI_API_KEY`` escape hatch while letting configured providers choose their own client. """ provider = _select_active_provider() - llm = provider.create_chat_model(model, max_tokens=max_tokens, timeout=timeout) - if llm is not None: - return llm - - from .openai import OpenAIProvider - if not isinstance(provider, OpenAIProvider): - llm = _openai_fallback_provider().create_chat_model( - model, - max_tokens=max_tokens, - timeout=timeout, - ) + # CLI providers don't participate in the create_chat_model path. + if not has_cli_capability(provider): + llm = provider.create_chat_model(model, max_tokens=max_tokens, timeout=timeout) if llm is not None: return llm + from .openai import OpenAIProvider + + if not isinstance(provider, OpenAIProvider): + llm = _openai_fallback_provider().create_chat_model( + model, + max_tokens=max_tokens, + timeout=timeout, + ) + if llm is not None: + return llm + raise_no_llm_api_key_configured() __all__ = [ + "AgentCLICapable", "ChatModelProvider", "CredentialsProvider", "LLMProvider", "ModelMetadataProvider", "NO_LLM_API_KEY_MESSAGE", "create_chat_model", + "get_active_provider", "get_metadata_provider", + "has_cli_capability", "raise_no_llm_api_key_configured", "resolve_chat_model_credentials", "resolve_provider_credentials", diff --git a/src/skillspector/providers/_agent_cli.py b/src/skillspector/providers/_agent_cli.py new file mode 100644 index 00000000..3d4d2c75 --- /dev/null +++ b/src/skillspector/providers/_agent_cli.py @@ -0,0 +1,821 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Hardened subprocess helper for agent CLI providers (claude, codex, gemini). + +This is the single security chokepoint for all agent-CLI calls. Per-CLI +knowledge (argv, output parsing, auth check) lives in a small ``CliSpec`` +registry (see ``_REGISTRY`` / "HOW TO ADD A NEW AGENT CLI" below); the +security core is CLI-agnostic. Every call goes through :func:`run_agent_cli` +which enforces: + +- **No shell**: ``shell=False`` with an explicit argv list. +- **Untrusted content via stdin only**: the prompt (which may contain + adversarial skill content) is written to the process stdin, never + injected into argv. +- **Capability stripping** (per-binary): tools disabled, MCP disabled, + no extra directories, deny permission mode (claude); read-only sandbox + (codex). ``--dangerously-skip-permissions`` is NEVER used. +- **Environment scrubbing**: API keys, SSH keys, cloud credentials, and + other secrets are stripped from the child environment. +- **Timeout enforcement**: the call raises ``TimeoutError`` rather than + hanging indefinitely. +- **Input / output caps**: prompt exceeding ``MAX_INPUT_BYTES`` is + rejected; stdout is capped at ``MAX_OUTPUT_BYTES``. +- **Fail-closed**: non-zero exit, timeout, missing binary, or bad + output all raise ``AgentCLIError``. +- **Prompt-layer hardening**: the caller wraps untrusted content in + clear DATA delimiters before passing it here (defense-in-depth on top + of capability removal). + +The JSON output envelope (``claude -p --output-format json``) is parsed +and the assistant text is returned. ``codex exec --json`` produces +JSONL events; the last assistant message is extracted. +""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +import tempfile +import threading +from collections.abc import Callable +from dataclasses import dataclass +from typing import Any + +from skillspector.logging_config import get_logger + +logger = get_logger(__name__) + +# --------------------------------------------------------------------------- +# Constants +# --------------------------------------------------------------------------- + +# Reuse the same cap as static_runner so a skill that's too big for static +# analysis is also too big to send to the CLI. +MAX_INPUT_BYTES = 1_000_000 # 1 MB — mirrors MAX_FILE_BYTES in static_runner.py +MAX_OUTPUT_BYTES = 10_000_000 # 10 MB safety cap on stdout +MAX_STDERR_BYTES = 64_000 # stderr is only used for error snippets +CLI_TIMEOUT_SECONDS = 300 # 5-minute per-call hard limit + +# Environment variables that must NOT be forwarded to child processes. +# Includes API keys, cloud creds, SSH agent, and SkillSpector's own keys. +_SECRET_ENV_PREFIXES: tuple[str, ...] = ( + "ANTHROPIC_API_KEY", + "OPENAI_API_KEY", + "NVIDIA_INFERENCE_KEY", + "NVIDIA_INFERENCE_METADATA_KEY", + "AWS_", + "AZURE_", + "GOOGLE_", + "GCLOUD_", + "GCP_", + "SSH_", + "GPG_", + "GITHUB_TOKEN", + "GITLAB_TOKEN", + "HUGGINGFACE_TOKEN", + "HF_TOKEN", + "COHERE_API_KEY", + "REPLICATE_API_TOKEN", + "MISTRAL_API_KEY", + "TOGETHER_API_KEY", + "GROQ_API_KEY", + "FIREWORKS_API_KEY", + "LANGCHAIN_API_KEY", + "LANGSMITH_API_KEY", +) + + +class AgentCLIError(RuntimeError): + """Raised when an agent CLI call fails for any reason (fail-closed).""" + + +# --------------------------------------------------------------------------- +# Environment scrubbing +# --------------------------------------------------------------------------- + + +def _scrub_env() -> dict[str, str]: + """Return a copy of ``os.environ`` with secret variables removed. + + Any variable whose name starts with a prefix in ``_SECRET_ENV_PREFIXES`` + is stripped. The resulting environment is passed to the subprocess. + """ + clean: dict[str, str] = {} + for key, val in os.environ.items(): + upper = key.upper() + if any(upper.startswith(p.upper()) for p in _SECRET_ENV_PREFIXES): + continue + clean[key] = val + return clean + + +# --------------------------------------------------------------------------- +# Binary lookup +# --------------------------------------------------------------------------- + + +def find_binary(name: str) -> str | None: + """Return the absolute path of *name* on PATH, or ``None`` if absent.""" + return shutil.which(name) + + +# --------------------------------------------------------------------------- +# Argument validation +# --------------------------------------------------------------------------- + + +def _validate_model_label(model: str) -> str: + """Ensure *model* cannot be used as an argument injection vector. + + Model labels come from ``SKILLSPECTOR_MODEL`` (user-controlled) or the + provider's defaults. We verify the label does not start with ``-`` + (which would look like a flag to the CLI) and contains only safe + characters. + + Raises: + AgentCLIError: when the label fails validation. + """ + if not model: + raise AgentCLIError("model label must be a non-empty string") + if model.startswith("-"): + raise AgentCLIError( + f"model label {model!r} starts with '-'; this looks like an argument injection attempt" + ) + # Allow alphanumeric, dash, dot, slash, colon, underscore (covers all + # known claude/codex model identifiers). + allowed = set("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-./: _") + bad = [c for c in model if c not in allowed] + if bad: + raise AgentCLIError(f"model label {model!r} contains disallowed characters: {bad!r}") + return model + + +# --------------------------------------------------------------------------- +# Claude CLI invocation +# --------------------------------------------------------------------------- + + +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. + + Flags chosen (verified end-to-end against ``claude`` v2.1.177 — each was + confirmed to parse AND to authenticate and return a result): + + ``-p`` / ``--print`` + Non-interactive single-shot mode. The prompt is read from stdin; + the response is written to stdout and the process exits. + + ``--output-format json`` + Emit a single JSON object (not a stream) so we can parse it + deterministically. + + ``--model