diff --git a/src/skillspector/llm_analyzer_base.py b/src/skillspector/llm_analyzer_base.py index 4ad6c558..2d55e2ab 100644 --- a/src/skillspector/llm_analyzer_base.py +++ b/src/skillspector/llm_analyzer_base.py @@ -34,7 +34,7 @@ from typing import Any, Literal, cast from langchain_core.messages import BaseMessage -from pydantic import BaseModel, Field, field_validator +from pydantic import BaseModel, Field, ValidationError, field_validator from skillspector.inspection_ledger import ( AnalyzerStatusEvent, @@ -567,6 +567,9 @@ def run_batches_detailed( response = _message_text(self._llm.invoke(prompt)) logger.debug("LLM response for %s", batch.file_label) outcome.successful.append((batch, self.parse_response(response, batch))) + except ValidationError as exc: + logger.warning("LLM batch failed for %s: %s", batch.file_label, exc) + outcome.failures.append(BatchFailure(batch=batch, error_class=type(exc).__name__)) except (ValueError, NotImplementedError): raise except Exception as exc: @@ -639,6 +642,12 @@ async def _process(batch: Batch) -> tuple[Batch, list]: results = await asyncio.gather(*[_process(b) for b in batches], return_exceptions=True) outcome = BatchExecutionResult() for batch, result in zip(batches, results, strict=True): + if isinstance(result, ValidationError): + logger.warning("LLM batch failed for %s: %s", batch.file_label, result) + outcome.failures.append( + BatchFailure(batch=batch, error_class=type(result).__name__) + ) + continue if isinstance(result, (ValueError, NotImplementedError)): raise result if isinstance(result, BaseException): diff --git a/tests/nodes/analyzers/test_semantic_security_discovery.py b/tests/nodes/analyzers/test_semantic_security_discovery.py index 4ce12117..dba29db3 100644 --- a/tests/nodes/analyzers/test_semantic_security_discovery.py +++ b/tests/nodes/analyzers/test_semantic_security_discovery.py @@ -23,7 +23,7 @@ import pytest from pydantic import ValidationError -from skillspector.llm_analyzer_base import LLMAnalysisResult, LLMFinding +from skillspector.llm_analyzer_base import Batch, LLMAnalysisResult, LLMFinding from skillspector.models import Finding from skillspector.nodes.analyzers.semantic_security_discovery import ( ANALYZER_ID, @@ -388,7 +388,11 @@ class TestLLMCallTelemetry: def test_success_records_ok_true(self, base_state) -> None: from skillspector.llm_analyzer_base import LLMAnalyzerBase - with patch.object(LLMAnalyzerBase, "run_batches", return_value=[]): + with patch.object( + LLMAnalyzerBase, + "run_batches", + return_value=[(Batch(file_path="SKILL.md", content="# Skill"), [])], + ): result = node(base_state) assert result["llm_call_log"] == [{"node": ANALYZER_ID, "ok": True, "error": None}] @@ -446,7 +450,6 @@ def _build_file_cache(skill_dir: Path) -> dict[str, str]: def _make_file_aware_run_batches(responses: dict[str, LLMAnalysisResult]): """Return a mock run_batches that dispatches based on file_path in each batch.""" - from skillspector.llm_analyzer_base import Batch def _run_batches(self_inner, batches: list[Batch], **_kwargs): results = [] diff --git a/tests/nodes/test_llm_analyzer_base.py b/tests/nodes/test_llm_analyzer_base.py index 1d9b6a23..683fcb21 100644 --- a/tests/nodes/test_llm_analyzer_base.py +++ b/tests/nodes/test_llm_analyzer_base.py @@ -404,6 +404,27 @@ def test_run_batches_uses_message_text_for_content_blocks(self) -> None: assert results[0][1] == ["chunk"] + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + def test_run_batches_isolates_raw_invoke_validation_error(self) -> None: + analyzer = _RawTextAnalyzer(base_prompt="test", model=self.MODEL) + + def _invoke(prompt: str) -> AIMessage: + if "b.py" in prompt: + LLMAnalysisResult.model_validate({"findings": 'We{"findings":[]}'}) + return AIMessage(content="ok") + + analyzer._llm.invoke.side_effect = _invoke + 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 = analyzer.run_batches(batches) + + assert {batch.file_path for batch, _ in results} == {"a.py", "c.py"} + assert [items for _, items in results] == [["ok"], ["ok"]] + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) async def test_arun_batches_uses_message_text_for_content_blocks(self) -> None: analyzer = _RawTextAnalyzer(base_prompt="test", model=self.MODEL) @@ -416,6 +437,72 @@ async def test_arun_batches_uses_message_text_for_content_blocks(self) -> None: assert results[0][1] == ["async chunk"] +# --------------------------------------------------------------------------- +# LLMAnalyzerBase.run_batches (sync execution) +# --------------------------------------------------------------------------- + + +class TestRunBatches: + MODEL = "nvidia/openai/gpt-oss-120b" + + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + def test_malformed_structured_batch_does_not_abort_the_others(self) -> None: + """A malformed structured response costs only its own batch.""" + + def _invoke(prompt: str) -> LLMAnalysisResult: + if "b.py" in prompt: + return LLMAnalysisResult.model_validate({"findings": 'We{"findings":[]}'}) + return LLMAnalysisResult( + findings=[ + LLMFinding(rule_id="T-1", message="hit", severity="LOW", start_line=1), + ] + ) + + analyzer = LLMAnalyzerBase(base_prompt="test", model=self.MODEL) + analyzer._structured_llm.invoke.side_effect = _invoke + + 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 = analyzer.run_batches(batches) + + assert {batch.file_path for batch, _ in results} == {"a.py", "c.py"} + assert [items[0].rule_id for _, items in results] == ["T-1", "T-1"] + + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + def test_value_error_still_propagates(self) -> None: + """ValueError signals misconfiguration, not a malformed model response.""" + analyzer = LLMAnalyzerBase(base_prompt="test", model=self.MODEL) + analyzer._structured_llm.invoke.side_effect = ValueError("no API key") + + with pytest.raises(ValueError, match="no API key"): + analyzer.run_batches([Batch(file_path="a.py", content="code")]) + + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + def test_parse_validation_error_does_not_abort_the_others(self) -> None: + analyzer = LLMAnalyzerBase(base_prompt="test", model=self.MODEL) + analyzer._structured_llm.invoke.return_value = LLMAnalysisResult(findings=[]) + original_parse = analyzer.parse_response + + def _parse(response: object, batch: Batch) -> list[Finding]: + if batch.file_path == "b.py": + LLMAnalysisResult.model_validate({"findings": 'We{"findings":[]}'}) + return original_parse(response, batch) + + analyzer.parse_response = _parse + 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 = analyzer.run_batches(batches) + + assert {batch.file_path for batch, _ in results} == {"a.py", "c.py"} + + # --------------------------------------------------------------------------- # LLMAnalyzerBase.arun_batches (async parallel execution) # --------------------------------------------------------------------------- @@ -622,6 +709,32 @@ async def _flaky_ainvoke(prompt: str) -> LLMAnalysisResult: 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_malformed_structured_batch_does_not_abort_the_others(self) -> None: + """A malformed structured response is isolated even though it is a ValueError.""" + + async def _ainvoke(prompt: str) -> LLMAnalysisResult: + if "b.py" in prompt: + return LLMAnalysisResult.model_validate({"findings": 'We{"findings":[]}'}) + return LLMAnalysisResult( + findings=[ + LLMFinding(rule_id="T-1", message="hit", severity="LOW", start_line=1), + ] + ) + + analyzer = LLMAnalyzerBase(base_prompt="test", model=self.MODEL) + analyzer._structured_llm.ainvoke = _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"} + assert [items[0].rule_id for _, items in results] == ["T-1", "T-1"] + @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) diff --git a/tests/nodes/test_semantic_quality_policy.py b/tests/nodes/test_semantic_quality_policy.py index e8ba916c..6b6f8fc6 100644 --- a/tests/nodes/test_semantic_quality_policy.py +++ b/tests/nodes/test_semantic_quality_policy.py @@ -22,7 +22,7 @@ import pytest -from skillspector.llm_analyzer_base import LLMAnalysisResult, LLMFinding +from skillspector.llm_analyzer_base import Batch, LLMAnalysisResult, LLMFinding from skillspector.models import Finding from skillspector.nodes.analyzers.semantic_quality_policy import ( ANALYZER_ID, @@ -272,7 +272,12 @@ class TestLLMCallTelemetry: def test_success_records_ok_true(self) -> None: from skillspector.llm_analyzer_base import LLMAnalyzerBase - with patch.object(LLMAnalyzerBase, "arun_batches", new_callable=AsyncMock, return_value=[]): + with patch.object( + LLMAnalyzerBase, + "arun_batches", + new_callable=AsyncMock, + return_value=[(Batch(file_path="SKILL.md", content="# Skill"), [])], + ): result = node({"file_cache": {"SKILL.md": "# Skill"}}) assert result["llm_call_log"] == [{"node": ANALYZER_ID, "ok": True, "error": None}]