From e2892df50b27216926996c3ceaeeab8354468799 Mon Sep 17 00:00:00 2001 From: keshavp <32313895+keshprad@users.noreply.github.com> Date: Fri, 7 Aug 2026 19:22:13 -0700 Subject: [PATCH] chore: public OSS release 2.8.2 Signed-off-by: keshavp <32313895+keshprad@users.noreply.github.com> --- CHANGELOG.md | 4 + docs/release/skillspector-2.8.2.md | 47 ++++++ pyproject.toml | 2 +- src/skillspector/llm_analyzer_base.py | 8 +- src/skillspector/llm_utils.py | 8 +- .../nodes/analyzers/mcp_tool_poisoning.py | 122 ++++++++------ tests/integration/test_graph.py | 23 ++- .../test_semantic_developer_intent.py | 40 ++++- tests/nodes/test_llm_analyzer_base.py | 48 +++++- tests/test_mcp_tool_poisoning.py | 158 +++++++++++++----- tests/unit/test_llm_utils.py | 5 +- uv.lock | 4 +- 12 files changed, 364 insertions(+), 105 deletions(-) create mode 100644 docs/release/skillspector-2.8.2.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 22eecbeac..6a203be7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +### 2.8.2 (Friday, August 07, 2026) +### Features/Bug Fixes +* fix(mcp): retry malformed TP4 responses +--- ### 2.8.1 (Thursday, August 06, 2026) ### Features/Bug Fixes * fix(llm): isolate malformed structured responses per batch diff --git a/docs/release/skillspector-2.8.2.md b/docs/release/skillspector-2.8.2.md new file mode 100644 index 000000000..540905d32 --- /dev/null +++ b/docs/release/skillspector-2.8.2.md @@ -0,0 +1,47 @@ +# SkillSpector v2.8.2 + +Released: 2026-08-07 + +## Summary + +This patch release improves the resilience of MCP tool-poisoning analysis when a model returns malformed structured output. Affected assessments are retried through the shared structured-output analyzer lifecycle rather than causing an immediate scan failure. + +## Highlights + +- MCP tool-poisoning TP4 checks now use the shared typed structured-output analyzer lifecycle. + +## Added + +- None. + +## Changed + +- Structured TP4 responses are validated through a typed schema with the same per-batch isolation behavior used by other LLM analyzers. + +## Fixed + +- Malformed structured model responses in MCP tool-poisoning checks are retried once and then isolated to the affected batch. + +## Security + +- None. + +## Breaking Changes and Migration + +- None. + +## Deprecations + +- None. + +## Validation + +- `git diff --check -- docs/release/skillspector-2.8.2.md` — passed. + +## Known Limitations + +- None. + +## References + +- `CHANGELOG.md` diff --git a/pyproject.toml b/pyproject.toml index 38d791710..eb9e0b9dd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "skillspector" -version = "2.8.1" +version = "2.8.2" description = "SkillSpector: Security scanner for AI agent skills (Claude Code, Cursor, and similar). Scans skills for vulnerabilities, malicious patterns, and security risks before installation. Supports Git repos, URLs, zips, and local directories; runs static pattern checks and optional LLM semantic analysis; outputs terminal, JSON, and Markdown reports with risk scoring." readme = "README.md" license = "Apache-2.0" diff --git a/src/skillspector/llm_analyzer_base.py b/src/skillspector/llm_analyzer_base.py index 9aff5ed96..0e46e622f 100644 --- a/src/skillspector/llm_analyzer_base.py +++ b/src/skillspector/llm_analyzer_base.py @@ -45,6 +45,7 @@ ledger_event, ) from skillspector.llm_utils import ( + StructuredOutputParseError, _AgentCLIMessage, _ainvoke_with_usage, _invoke_with_usage, @@ -575,7 +576,7 @@ def _invoke_batch(self, batch: Batch, prompt: str) -> tuple[Batch, list]: if self._structured_llm: try: response = _invoke_with_usage(self._structured_llm, prompt, self._usage_collector) - except ValidationError as exc: + except (StructuredOutputParseError, ValidationError) as exc: raise _StructuredResponseValidationError from exc else: response = _raw_response_text( @@ -597,7 +598,7 @@ async def _ainvoke_batch(self, batch: Batch, prompt: str) -> tuple[Batch, list]: response = await _ainvoke_with_usage( self._structured_llm, prompt, self._usage_collector ) - except ValidationError as exc: + except (StructuredOutputParseError, ValidationError) as exc: raise _StructuredResponseValidationError from exc else: response = _raw_response_text( @@ -678,7 +679,8 @@ async def arun_batches( 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. Malformed structured responses (Pydantic - ``ValidationError``) are retried once and then isolated to their batch. + ``ValidationError`` or CLI JSON parse failures) are retried once and + then isolated to their batch. Callers can detect partial results by comparing the returned batches against the submitted ones. Other ``ValueError`` instances and ``NotImplementedError`` signal misconfiguration rather than infra trouble diff --git a/src/skillspector/llm_utils.py b/src/skillspector/llm_utils.py index 8b5ca4bb6..2194e15d2 100644 --- a/src/skillspector/llm_utils.py +++ b/src/skillspector/llm_utils.py @@ -178,6 +178,10 @@ def __init__(self, content: str) -> None: self.content = content +class StructuredOutputParseError(ValueError): + """Raised when a structured-output response does not contain a JSON object.""" + + def _extract_json_object(raw: str) -> dict: """Extract a single JSON object from a CLI model's text response. @@ -206,7 +210,9 @@ def _extract_json_object(raw: str) -> dict: return obj except json.JSONDecodeError: pass - raise ValueError(f"could not extract a JSON object from CLI response: {raw[:200]!r}") + raise StructuredOutputParseError( + f"could not extract a JSON object from CLI response: {raw[:200]!r}" + ) class _StructuredAgentCLIModel: diff --git a/src/skillspector/nodes/analyzers/mcp_tool_poisoning.py b/src/skillspector/nodes/analyzers/mcp_tool_poisoning.py index 9898854a6..a233ee168 100644 --- a/src/skillspector/nodes/analyzers/mcp_tool_poisoning.py +++ b/src/skillspector/nodes/analyzers/mcp_tool_poisoning.py @@ -18,19 +18,21 @@ from __future__ import annotations import base64 -import json import logging import re import unicodedata +from typing import cast -from skillspector.inference_usage import InferenceUsageCollector, InferenceUsageRecord +from pydantic import BaseModel, Field, field_validator + +from skillspector.inference_usage import InferenceUsageRecord from skillspector.inspection_ledger import ( LedgerOutcome, LedgerReason, analyzer_status_event, ledger_event, ) -from skillspector.llm_utils import chat_completion, new_inference_usage_collector +from skillspector.llm_analyzer_base import Batch, LLMAnalyzerBase from skillspector.models import Finding from skillspector.providers import get_active_provider from skillspector.state import ( @@ -690,6 +692,44 @@ def _check_tp3(params: list[dict]) -> list[Finding]: ) +class _TP4AnalysisResult(BaseModel): + """Validated response from the description-behavior mismatch check.""" + + is_mismatch: bool + confidence: float = 0.0 + declared_purpose_summary: str = "" + actual_behavior_summary: str = "" + mismatched_capabilities: list[str] = Field(default_factory=list) + explanation: str = "" + + @field_validator("confidence") + @classmethod + def _validate_confidence(cls, value: float) -> float: + if not 0.0 <= value <= 1.0: + raise ValueError("confidence must be between 0.0 and 1.0") + return value + + +class _TP4Analyzer(LLMAnalyzerBase): + """Run TP4 through the shared structured-output analyzer lifecycle.""" + + response_schema = _TP4AnalysisResult + + def __init__(self, model: str) -> None: + super().__init__(base_prompt="", model=model, node=ANALYZER_ID) + + def build_prompt(self, batch: Batch, **_kwargs: object) -> str: + """Use TP4's purpose-built prompt without the generic file wrapper.""" + return batch.content + + def parse_response( # type: ignore[override] # TP4 returns its typed assessment. + self, response: object, _batch: Batch + ) -> list[_TP4AnalysisResult]: + if isinstance(response, _TP4AnalysisResult): + return [response] + raise NotImplementedError("TP4 requires a structured assessment response") + + def _check_tp4( state: SkillspectorState, ) -> tuple[ @@ -707,7 +747,7 @@ def _check_tp4( emitted only when the provider response supplied it. """ attempted = False - usage_collector: InferenceUsageCollector | None = None + analyzer: _TP4Analyzer | None = None try: manifest: dict = state.get("manifest") or {} description = manifest.get("description") @@ -740,11 +780,7 @@ def _check_tp4( model_config: dict = state.get("model_config") or {} model = model_config.get(ANALYZER_ID) or model_config.get("default") - usage_collector = new_inference_usage_collector( - node=ANALYZER_ID, - request_kind="chat_completion", - model=model or get_active_provider().resolve_model(), - ) + model = model or get_active_provider().resolve_model() prompt = f"""You are a security auditor. Your task: determine whether a skill's declared description accurately represents what its code actually does. @@ -773,52 +809,42 @@ def _check_tp4( - Utility code that supports the declared purpose (logging, error handling) - Over-declared permissions (covered by a separate analyzer) -Respond in JSON matching this exact schema: -{{ - "is_mismatch": true/false, - "confidence": 0.0-1.0, - "declared_purpose_summary": "one-sentence summary of what the description claims", - "actual_behavior_summary": "one-sentence summary of what the code actually does", - "mismatched_capabilities": ["list of capabilities in code but not in description"], - "explanation": "why this is or is not a mismatch" -}}""" +Return the assessment using the provided structured output schema.""" + analyzer = _TP4Analyzer(model) attempted = True - response = chat_completion( - prompt, - model=model, - usage_collector=usage_collector, - node=ANALYZER_ID, - ) - - # Parse JSON — handle optional ```json code blocks - json_text = response.strip() - if json_text.startswith("```"): - # Strip opening fence (```json or ```) - first_newline = json_text.find("\n") - if first_newline != -1: - json_text = json_text[first_newline + 1 :] - # Strip closing fence - if json_text.rstrip().endswith("```"): - json_text = json_text.rstrip()[:-3].rstrip() - - result = json.loads(json_text) + outcome = analyzer.run_batches_detailed([Batch(file_path="SKILL.md", content=prompt)]) + if outcome.failures: + failure = outcome.failures[0] + return ( + [], + llm_call_record( + ANALYZER_ID, + ok=False, + error=f"TP4 LLM batch failed: {failure.error_class}", + ), + failure.error_class, + cast(list[InferenceUsageRecord], analyzer.inference_usage), + ) + result = outcome.successful[0][1][0] + if not isinstance(result, _TP4AnalysisResult): + raise RuntimeError("TP4 returned an unexpected structured response type") ok_record = llm_call_record(ANALYZER_ID, ok=True) - if not result.get("is_mismatch"): - return [], ok_record, None, usage_collector.snapshot() + if not result.is_mismatch: + return [], ok_record, None, cast(list[InferenceUsageRecord], analyzer.inference_usage) - confidence = float(result.get("confidence", 0.0)) + confidence = result.confidence if confidence < 0.5: - return [], ok_record, None, usage_collector.snapshot() + return [], ok_record, None, cast(list[InferenceUsageRecord], analyzer.inference_usage) severity = "HIGH" if confidence >= 0.7 else "MEDIUM" - mismatched = result.get("mismatched_capabilities") or [] + mismatched = result.mismatched_capabilities mismatched_str = ", ".join(mismatched) if mismatched else "unspecified" - explanation = result.get("explanation", "") - declared = result.get("declared_purpose_summary", description[:80]) - actual = result.get("actual_behavior_summary", "") + explanation = result.explanation + declared = result.declared_purpose_summary or description[:80] + actual = result.actual_behavior_summary return ( [ @@ -842,7 +868,7 @@ def _check_tp4( ], ok_record, None, - usage_collector.snapshot(), + cast(list[InferenceUsageRecord], analyzer.inference_usage), ) except Exception as exc: @@ -854,7 +880,9 @@ def _check_tp4( [], llm_call_record(ANALYZER_ID, ok=False, error=str(exc)), type(exc).__name__, - usage_collector.snapshot() if usage_collector is not None else [], + cast(list[InferenceUsageRecord], analyzer.inference_usage) + if analyzer is not None + else [], ) return [], None, None, [] diff --git a/tests/integration/test_graph.py b/tests/integration/test_graph.py index 963888e54..8ec668e34 100644 --- a/tests/integration/test_graph.py +++ b/tests/integration/test_graph.py @@ -168,10 +168,26 @@ def test_graph_surfaces_degraded_llm_stage(tmp_path: Path, monkeypatch: pytest.M def boom(*_a: object, **_k: object) -> object: raise RuntimeError("simulated LLM transport failure") - # Fail both LLM transports: get_chat_model (semantic analyzers + meta) and - # chat_completion (mcp_tool_poisoning TP4). + class FailingTP4Analyzer: + """Simulate an attempted TP4 request failing after analyzer setup.""" + + @property + def inference_usage(self) -> list[object]: + return [] + + def __init__(self, _model: str) -> None: + pass + + def run_batches_detailed(self, _batches: object) -> object: + raise RuntimeError("simulated LLM transport failure") + + # Semantic analyzers and meta_analyzer fail while constructing their shared + # transport. TP4's analyzer construction is deliberately not an attempted + # LLM call, so fail it at batch execution to assert its ledger projection. monkeypatch.setattr("skillspector.llm_analyzer_base.get_chat_model", boom) - monkeypatch.setattr("skillspector.nodes.analyzers.mcp_tool_poisoning.chat_completion", boom) + monkeypatch.setattr( + "skillspector.nodes.analyzers.mcp_tool_poisoning._TP4Analyzer", FailingTP4Analyzer + ) result = graph.invoke({"skill_path": str(tmp_path), "use_llm": True, "output_format": "json"}) @@ -186,6 +202,7 @@ def boom(*_a: object, **_k: object) -> object: "semantic_developer_intent", "semantic_quality_policy", "meta_analyzer", + "mcp_tool_poisoning", } <= nodes meta = json.loads(result["report_body"])["metadata"] diff --git a/tests/nodes/analyzers/test_semantic_developer_intent.py b/tests/nodes/analyzers/test_semantic_developer_intent.py index f28a2bce1..10ab00f28 100644 --- a/tests/nodes/analyzers/test_semantic_developer_intent.py +++ b/tests/nodes/analyzers/test_semantic_developer_intent.py @@ -388,6 +388,31 @@ def test_list_permissions_joined(self) -> None: _sdi_fixture_test = pytest.mark.integration +def _mock_sdi_structured_llm(monkeypatch: pytest.MonkeyPatch, rule_id: str | None) -> MagicMock: + """Return a deterministic structured response for an SDI fixture case.""" + mock_llm = MagicMock() + structured_llm = MagicMock() + findings = ( + [] + if rule_id is None + else [ + LLMFinding( + rule_id=rule_id, + message="Fixture response identifies the declared-behavior mismatch.", + severity="HIGH", + start_line=1, + confidence=0.9, + explanation="The fixture response is a valid structured LLM result.", + remediation="Update the declared behavior to match the implementation.", + ) + ] + ) + structured_llm.ainvoke = AsyncMock(return_value=LLMAnalysisResult(findings=findings)) + mock_llm.with_structured_output.return_value = structured_llm + monkeypatch.setattr(MOCK_PATCH_TARGET, lambda **_kwargs: mock_llm) + return structured_llm + + def _build_file_cache(skill_dir: Path) -> dict[str, str]: cache: dict[str, str] = {} for item in sorted(skill_dir.rglob("*")): @@ -422,7 +447,8 @@ def _load_manifest(skill_dir: Path) -> dict: class TestSdi1Mismatch: """SDI-1: skill claiming local-only but making network calls → findings.""" - def test_mismatch_produces_finding(self) -> None: + def test_mismatch_produces_finding(self, monkeypatch: pytest.MonkeyPatch) -> None: + _mock_sdi_structured_llm(monkeypatch, "SDI-1") skill_dir = _SDI_FIXTURES / "sdi1_mismatch" if not skill_dir.is_dir(): pytest.skip("sdi1_mismatch fixture not present") @@ -446,7 +472,8 @@ def test_mismatch_produces_finding(self) -> None: class TestSdi2Inappropriate: """SDI-2: formatter skill using subprocess → findings.""" - def test_inappropriate_capability_flagged(self) -> None: + def test_inappropriate_capability_flagged(self, monkeypatch: pytest.MonkeyPatch) -> None: + _mock_sdi_structured_llm(monkeypatch, "SDI-2") skill_dir = _SDI_FIXTURES / "sdi2_inappropriate" if not skill_dir.is_dir(): pytest.skip("sdi2_inappropriate fixture not present") @@ -470,7 +497,8 @@ def test_inappropriate_capability_flagged(self) -> None: class TestSdi3ScopeCreep: """SDI-3: read-only permissions declared but code writes files → findings.""" - def test_scope_creep_flagged(self) -> None: + def test_scope_creep_flagged(self, monkeypatch: pytest.MonkeyPatch) -> None: + _mock_sdi_structured_llm(monkeypatch, "SDI-3") skill_dir = _SDI_FIXTURES / "sdi3_scope_creep" if not skill_dir.is_dir(): pytest.skip("sdi3_scope_creep fixture not present") @@ -494,7 +522,8 @@ def test_scope_creep_flagged(self) -> None: class TestSdi4Divergence: """SDI-4: docstrings contradict what the code does → findings.""" - def test_divergence_flagged(self) -> None: + def test_divergence_flagged(self, monkeypatch: pytest.MonkeyPatch) -> None: + _mock_sdi_structured_llm(monkeypatch, "SDI-4") skill_dir = _SDI_FIXTURES / "sdi4_divergence" if not skill_dir.is_dir(): pytest.skip("sdi4_divergence fixture not present") @@ -519,7 +548,8 @@ def test_divergence_flagged(self) -> None: class TestSdiClean: """Shared clean fixture: well-formed skill → no SDI findings.""" - def test_clean_skill_produces_no_sdi_findings(self) -> None: + def test_clean_skill_produces_no_sdi_findings(self, monkeypatch: pytest.MonkeyPatch) -> None: + _mock_sdi_structured_llm(monkeypatch, None) skill_dir = _SDI_FIXTURES / "sdi_clean" if not skill_dir.is_dir(): pytest.skip("sdi_clean fixture not present") diff --git a/tests/nodes/test_llm_analyzer_base.py b/tests/nodes/test_llm_analyzer_base.py index 6a0815214..fa4e652bc 100644 --- a/tests/nodes/test_llm_analyzer_base.py +++ b/tests/nodes/test_llm_analyzer_base.py @@ -40,7 +40,7 @@ number_lines, resolve_max_concurrency, ) -from skillspector.llm_utils import AgentCLIChatModel +from skillspector.llm_utils import AgentCLIChatModel, StructuredOutputParseError from skillspector.models import Finding from skillspector.nodes.meta_analyzer import ( LLMMetaAnalyzer, @@ -474,6 +474,36 @@ def test_structured_validation_error_recovers_on_retry(self) -> None: assert outcome.failures == [] assert analyzer._structured_llm.invoke.call_count == 2 + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + def test_structured_parse_error_recovers_on_retry(self) -> None: + analyzer = LLMAnalyzerBase(base_prompt="test", model=self.MODEL) + analyzer._structured_llm.invoke = MagicMock( + side_effect=[ + StructuredOutputParseError("could not extract JSON"), + LLMAnalysisResult(findings=[]), + ] + ) + + outcome = analyzer.run_batches_detailed([Batch(file_path="a.py", content="code")]) + + assert [item[0].file_path for item in outcome.successful] == ["a.py"] + assert outcome.failures == [] + assert analyzer._structured_llm.invoke.call_count == 2 + + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + def test_cli_structured_parse_error_recovers_on_retry(self) -> None: + analyzer = LLMAnalyzerBase(base_prompt="test", model=self.MODEL) + provider = MagicMock() + provider.complete.side_effect = ["not JSON", '{"findings": []}'] + analyzer._llm = AgentCLIChatModel(provider, self.MODEL, 1024) + analyzer._structured_llm = analyzer._llm.with_structured_output(LLMAnalysisResult) + + outcome = analyzer.run_batches_detailed([Batch(file_path="a.py", content="code")]) + + assert [item[0].file_path for item in outcome.successful] == ["a.py"] + assert outcome.failures == [] + assert provider.complete.call_count == 2 + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) def test_structured_validation_error_isolated_after_retry(self) -> None: analyzer = LLMAnalyzerBase(base_prompt="test", model=self.MODEL) @@ -594,6 +624,22 @@ async def test_structured_validation_error_recovers_on_retry(self) -> None: assert outcome.failures == [] assert analyzer._structured_llm.ainvoke.call_count == 2 + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + async def test_structured_parse_error_recovers_on_retry(self) -> None: + analyzer = LLMAnalyzerBase(base_prompt="test", model=self.MODEL) + analyzer._structured_llm.ainvoke = AsyncMock( + side_effect=[ + StructuredOutputParseError("could not extract JSON"), + LLMAnalysisResult(findings=[]), + ] + ) + + outcome = await analyzer.arun_batches_detailed([Batch(file_path="a.py", content="code")]) + + assert [item[0].file_path for item in outcome.successful] == ["a.py"] + assert outcome.failures == [] + assert analyzer._structured_llm.ainvoke.call_count == 2 + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) async def test_structured_validation_error_isolated_after_retry(self) -> None: analyzer = LLMAnalyzerBase(base_prompt="test", model=self.MODEL) diff --git a/tests/test_mcp_tool_poisoning.py b/tests/test_mcp_tool_poisoning.py index e79d8efc3..063577390 100644 --- a/tests/test_mcp_tool_poisoning.py +++ b/tests/test_mcp_tool_poisoning.py @@ -20,10 +20,13 @@ import base64 import re from pathlib import Path +from unittest.mock import MagicMock import pytest import yaml +from pydantic import BaseModel +from skillspector.llm_utils import AgentCLIChatModel from skillspector.nodes.analyzers import mcp_tool_poisoning # --------------------------------------------------------------------------- @@ -187,6 +190,46 @@ def _make_state( } +class _FakeStructuredLLM: + """Minimal structured model double for TP4 response handling tests.""" + + def __init__(self, responses: list[object]) -> None: + self.responses = list(responses) + self.calls = 0 + self.response_schema: type[BaseModel] | None = None + + def invoke_with_usage(self, _prompt: str, collector: object) -> object: + self.calls += 1 + response = self.responses.pop(0) + if isinstance(response, BaseException): + raise response + collector.mark_response_received() # type: ignore[attr-defined] + if isinstance(response, dict): + assert self.response_schema is not None + return self.response_schema.model_validate(response) + return response + + +class _FakeChatModel: + def __init__(self, structured_llm: _FakeStructuredLLM) -> None: + self.structured_llm = structured_llm + + def with_structured_output(self, schema: type[BaseModel]) -> _FakeStructuredLLM: + self.structured_llm.response_schema = schema + return self.structured_llm + + +def _mock_tp4_structured_llm( + monkeypatch: pytest.MonkeyPatch, responses: list[object] +) -> _FakeStructuredLLM: + structured_llm = _FakeStructuredLLM(responses) + monkeypatch.setattr( + "skillspector.llm_analyzer_base.get_chat_model", + lambda **_kwargs: _FakeChatModel(structured_llm), + ) + return structured_llm + + # Alias used by node import at module level node = mcp_tool_poisoning.node @@ -616,14 +659,28 @@ def test_fixture_triggers_tp1_tp2_tp3(self): @pytest.mark.integration class TestTP4DescriptionBehaviorMismatch: - def test_mismatch_detected(self): + def test_mismatch_detected(self, monkeypatch: pytest.MonkeyPatch): + _mock_tp4_structured_llm( + monkeypatch, + [ + { + "is_mismatch": True, + "confidence": 0.9, + "declared_purpose_summary": "Local text transformation", + "actual_behavior_summary": "Sends source data to a remote endpoint", + "mismatched_capabilities": ["network access"], + "explanation": "The declared purpose does not disclose its network behavior.", + } + ], + ) state = _make_state("mcp_mismatched_skill", use_llm=True) result = node(state) tp4 = [f for f in result["findings"] if f.rule_id == "TP4"] assert len(tp4) >= 1 assert tp4[0].severity in {"HIGH", "MEDIUM"} - def test_no_mismatch_clean(self): + def test_no_mismatch_clean(self, monkeypatch: pytest.MonkeyPatch): + _mock_tp4_structured_llm(monkeypatch, [{"is_mismatch": False}]) state = _make_state("mcp_clean_skill", use_llm=True) result = node(state) tp4 = [f for f in result["findings"] if f.rule_id == "TP4"] @@ -643,59 +700,84 @@ def test_skipped_no_description(self): tp4 = [f for f in result["findings"] if f.rule_id == "TP4"] assert len(tp4) == 0 - def test_llm_call_failure_returns_empty(self): - from unittest.mock import patch - + def test_llm_call_failure_returns_empty(self, monkeypatch: pytest.MonkeyPatch): state = _make_state("mcp_mismatched_skill", use_llm=True) - with patch( - "skillspector.nodes.analyzers.mcp_tool_poisoning.chat_completion", - side_effect=RuntimeError("timeout"), - ): - result = node(state) + _mock_tp4_structured_llm(monkeypatch, [RuntimeError("timeout")]) + result = node(state) tp4 = [f for f in result["findings"] if f.rule_id == "TP4"] assert len(tp4) == 0 - def test_unparseable_response_returns_empty(self): - from unittest.mock import patch - + def test_persistently_malformed_response_returns_empty(self, monkeypatch: pytest.MonkeyPatch): state = _make_state("mcp_mismatched_skill", use_llm=True) - with patch( - "skillspector.nodes.analyzers.mcp_tool_poisoning.chat_completion", - return_value="this is not json at all {{{", - ): - result = node(state) + structured_llm = _mock_tp4_structured_llm( + monkeypatch, + [{}, {}], + ) + result = node(state) tp4 = [f for f in result["findings"] if f.rule_id == "TP4"] assert len(tp4) == 0 + assert structured_llm.calls == 2 + assert result["inspection_ledger"][1]["error_class"] == "ValidationError" + + def test_malformed_response_is_retried(self, monkeypatch: pytest.MonkeyPatch): + state = _make_state("mcp_mismatched_skill", use_llm=True) + structured_llm = _mock_tp4_structured_llm( + monkeypatch, + [{}, {"is_mismatch": False}], + ) + + result = node(state) + + assert structured_llm.calls == 2 + assert result["llm_call_log"] == [{"node": "mcp_tool_poisoning", "ok": True, "error": None}] + assert result["analyzer_status_events"][0]["status"] == "completed" + + def test_cli_parse_error_is_retried(self, monkeypatch: pytest.MonkeyPatch): + state = _make_state("mcp_mismatched_skill", use_llm=True) + provider = MagicMock() + provider.complete.side_effect = ["not JSON", '{"is_mismatch": false}'] + monkeypatch.setattr( + "skillspector.llm_analyzer_base.get_chat_model", + lambda **kwargs: AgentCLIChatModel(provider, kwargs["model"], 1024), + ) + + result = node(state) + + assert provider.complete.call_count == 2 + assert result["llm_call_log"] == [{"node": "mcp_tool_poisoning", "ok": True, "error": None}] + + def test_out_of_range_confidence_is_retried(self, monkeypatch: pytest.MonkeyPatch): + state = _make_state("mcp_mismatched_skill", use_llm=True) + structured_llm = _mock_tp4_structured_llm( + monkeypatch, + [{"is_mismatch": True, "confidence": 1.7}, {"is_mismatch": False}], + ) + + result = node(state) + + assert structured_llm.calls == 2 + assert [finding for finding in result["findings"] if finding.rule_id == "TP4"] == [] + assert result["llm_call_log"] == [{"node": "mcp_tool_poisoning", "ok": True, "error": None}] class TestTP4Telemetry: """TP4 records llm_call_log so the report's degradation detector counts it consistently with the semantic analyzers and the meta-analyzer.""" - def test_successful_call_records_ok_true(self): - from unittest.mock import patch - + def test_successful_call_records_ok_true(self, monkeypatch: pytest.MonkeyPatch): state = _make_state("mcp_mismatched_skill", use_llm=True) - with patch( - "skillspector.nodes.analyzers.mcp_tool_poisoning.chat_completion", - return_value='{"is_mismatch": false}', - ): - result = node(state) + _mock_tp4_structured_llm(monkeypatch, [{"is_mismatch": False}]) + result = node(state) assert result["llm_call_log"] == [{"node": "mcp_tool_poisoning", "ok": True, "error": None}] - def test_failed_call_records_ok_false(self): - from unittest.mock import patch - + def test_failed_call_records_ok_false(self, monkeypatch: pytest.MonkeyPatch): state = _make_state("mcp_mismatched_skill", use_llm=True) - with patch( - "skillspector.nodes.analyzers.mcp_tool_poisoning.chat_completion", - side_effect=RuntimeError("timeout"), - ): - result = node(state) + _mock_tp4_structured_llm(monkeypatch, [RuntimeError("timeout")]) + result = node(state) log = result["llm_call_log"] assert log[0]["node"] == "mcp_tool_poisoning" assert log[0]["ok"] is False - assert "timeout" in log[0]["error"] + assert "RuntimeError" in log[0]["error"] status = result["analyzer_status_events"][0] assert status["status"] == "failed" assert [work["work_id"] for work in status["planned_work"]] == [ @@ -735,11 +817,7 @@ def test_static_work_is_completed_when_tp4_is_not_applicable(self): ] def test_successful_tp4_plans_static_and_semantic_work(self, monkeypatch): - monkeypatch.setattr( - mcp_tool_poisoning, - "chat_completion", - lambda *_args, **_kwargs: '{"is_mismatch": false}', - ) + _mock_tp4_structured_llm(monkeypatch, [{"is_mismatch": False}]) result = mcp_tool_poisoning.node(_make_state("mcp_mismatched_skill", use_llm=True)) diff --git a/tests/unit/test_llm_utils.py b/tests/unit/test_llm_utils.py index b9ca2bd5f..211286ddf 100644 --- a/tests/unit/test_llm_utils.py +++ b/tests/unit/test_llm_utils.py @@ -35,6 +35,7 @@ from skillspector.inference_usage import InferenceUsageCollector from skillspector.llm_utils import ( AgentCLIChatModel, + StructuredOutputParseError, _ainvoke_with_usage, _extract_json_object, _invoke_with_usage, @@ -521,8 +522,8 @@ def test_fenced_json(self) -> None: def test_prose_wrapped_json(self) -> None: assert _extract_json_object('Here you go:\n{"a": 1}\nDone.') == {"a": 1} - def test_garbage_raises(self) -> None: - with pytest.raises(ValueError): + def test_garbage_raises_structured_output_parse_error(self) -> None: + with pytest.raises(StructuredOutputParseError): _extract_json_object("not json") diff --git a/uv.lock b/uv.lock index 80c5f2f2c..24a35b144 100644 --- a/uv.lock +++ b/uv.lock @@ -1,5 +1,5 @@ version = 1 -revision = 3 +revision = 2 requires-python = ">=3.12, <3.15" resolution-markers = [ "python_full_version >= '3.14' and sys_platform == 'win32'", @@ -2675,7 +2675,7 @@ wheels = [ [[package]] name = "skillspector" -version = "2.8.1" +version = "2.8.2" source = { editable = "." } dependencies = [ { name = "boto3" },