diff --git a/src/skillspector/llm_analyzer_base.py b/src/skillspector/llm_analyzer_base.py index cc661f307..47858e361 100644 --- a/src/skillspector/llm_analyzer_base.py +++ b/src/skillspector/llm_analyzer_base.py @@ -122,6 +122,19 @@ class _StructuredResponseValidationError(Exception): """Signal that provider output failed structured-response validation.""" +def _require_structured_response(response: object) -> None: + """Reject a structured-output call that produced no result. + + LangChain's tool-call parsers return ``None`` when the reply contains no + tool call, for example a forced tool call answered in prose. Treat that as + a malformed structured response so it gets the same bounded retries, + instead of passing ``None`` to ``parse_response``, which expects a parsed + schema object. + """ + if response is None: + raise StructuredOutputParseError("model returned no structured response") + + class LLMRuntimeLimitError(RuntimeError): """Signal that no shared scan time remains for an LLM operation.""" @@ -1156,6 +1169,7 @@ def _invoke_batch(self, batch: Batch, prompt: str) -> tuple[Batch, list]: if structured_llm: try: response = _invoke_with_usage(structured_llm, prompt, self._usage_collector) + _require_structured_response(response) except (StructuredOutputParseError, ValidationError) as exc: raise _StructuredResponseValidationError from exc else: @@ -1245,6 +1259,7 @@ async def _ainvoke_batch(self, batch: Batch, prompt: str) -> tuple[Batch, list]: if structured_llm: try: response = await _ainvoke_with_usage(structured_llm, prompt, self._usage_collector) + _require_structured_response(response) except (StructuredOutputParseError, ValidationError) as exc: raise _StructuredResponseValidationError from exc else: @@ -1417,8 +1432,9 @@ async def arun_batches( retries so each coordinator retry can re-check and cap its delay against remaining time. Unrecovered errors cost only their own batch and are omitted from the result. - Malformed structured responses (Pydantic ``ValidationError`` or CLI - JSON parse failures) receive three bounded exponential-backoff retries + Malformed structured responses (Pydantic ``ValidationError``, CLI + JSON parse failures, or no structured result at all) receive three + bounded exponential-backoff retries and are then isolated to their batch. A batch makes at most seven outer chat-model invocations even when both retry policies apply; native provider retries can make additional HTTP diff --git a/tests/nodes/test_llm_analyzer_base.py b/tests/nodes/test_llm_analyzer_base.py index f73418166..aa409fb66 100644 --- a/tests/nodes/test_llm_analyzer_base.py +++ b/tests/nodes/test_llm_analyzer_base.py @@ -1279,6 +1279,50 @@ def test_structured_validation_error_isolated_after_three_retries( ((2.0,), {}), ] + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + @patch("skillspector.llm_analyzer_base.time.sleep") + def test_missing_structured_response_recovers_on_retry(self, sleep: MagicMock) -> None: + """A tool-call parser returns None when the reply has no tool call.""" + analyzer = LLMAnalyzerBase(base_prompt="test", model=self.MODEL) + analyzer._structured_llm.invoke = MagicMock( + side_effect=[None, 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 + sleep.assert_called_once_with(0.5) + + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + @patch("skillspector.llm_analyzer_base.time.sleep") + def test_missing_structured_response_isolated_after_three_retries( + self, sleep: MagicMock + ) -> None: + analyzer = LLMAnalyzerBase(base_prompt="test", model=self.MODEL) + analyzer._structured_llm.invoke = MagicMock( + side_effect=[None, None, None, None, LLMAnalysisResult(findings=[])] + ) + batches = [ + Batch(file_path="missing.py", content="no tool call"), + Batch(file_path="clean.py", content="clean response"), + ] + + outcome = analyzer.run_batches_detailed(batches) + + assert [item[0].file_path for item in outcome.successful] == ["clean.py"] + assert [ + (failure.batch.file_path, failure.error_class, failure.reason) + for failure in outcome.failures + ] == [("missing.py", "ValidationError", LedgerReason.LLM_STRUCTURED_RESPONSE_INVALID)] + assert analyzer._structured_llm.invoke.call_count == 5 + assert sleep.call_args_list == [ + ((0.5,), {}), + ((1.0,), {}), + ((2.0,), {}), + ] + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) @patch("skillspector.llm_analyzer_base.time.sleep") def test_api_connection_error_recovers_with_bounded_backoff(self, sleep: MagicMock) -> None: @@ -1986,6 +2030,50 @@ async def test_structured_validation_error_isolated_after_three_retries( ((2.0,), {}), ] + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + @patch("skillspector.llm_analyzer_base.asyncio.sleep", new_callable=AsyncMock) + async def test_missing_structured_response_recovers_on_retry(self, sleep: AsyncMock) -> None: + """A tool-call parser returns None when the reply has no tool call.""" + analyzer = LLMAnalyzerBase(base_prompt="test", model=self.MODEL) + analyzer._structured_llm.ainvoke = AsyncMock( + side_effect=[None, 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 + sleep.assert_awaited_once_with(0.5) + + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) + @patch("skillspector.llm_analyzer_base.asyncio.sleep", new_callable=AsyncMock) + async def test_missing_structured_response_isolated_after_three_retries( + self, sleep: AsyncMock + ) -> None: + analyzer = LLMAnalyzerBase(base_prompt="test", model=self.MODEL) + analyzer._structured_llm.ainvoke = AsyncMock( + side_effect=[None, None, None, None, LLMAnalysisResult(findings=[])] + ) + batches = [ + Batch(file_path="missing.py", content="no tool call"), + Batch(file_path="clean.py", content="clean response"), + ] + + outcome = await analyzer.arun_batches_detailed(batches, max_concurrency=1) + + assert [item[0].file_path for item in outcome.successful] == ["clean.py"] + assert [ + (failure.batch.file_path, failure.error_class, failure.reason) + for failure in outcome.failures + ] == [("missing.py", "ValidationError", LedgerReason.LLM_STRUCTURED_RESPONSE_INVALID)] + assert analyzer._structured_llm.ainvoke.call_count == 5 + assert sleep.await_args_list == [ + ((0.5,), {}), + ((1.0,), {}), + ((2.0,), {}), + ] + @patch(MOCK_PATCH_TARGET, _mock_get_chat_model) @patch("skillspector.llm_analyzer_base.asyncio.sleep", new_callable=AsyncMock) async def test_api_connection_error_recovers_with_bounded_backoff( diff --git a/tests/test_mcp_tool_poisoning.py b/tests/test_mcp_tool_poisoning.py index 23932b54d..dcedfb006 100644 --- a/tests/test_mcp_tool_poisoning.py +++ b/tests/test_mcp_tool_poisoning.py @@ -29,7 +29,7 @@ import yaml from pydantic import BaseModel -from skillspector.inspection_ledger import LedgerOutcome, LedgerReason +from skillspector.inspection_ledger import LedgerOutcome, LedgerReason, finalize_ledger from skillspector.llm_analyzer_base import LLMAnalyzerBase from skillspector.llm_utils import AgentCLIChatModel from skillspector.nodes.analyzers import mcp_tool_poisoning @@ -1836,14 +1836,98 @@ def test_wrapped_json_schema_response_stays_incomplete( ) def test_tool_refusal_stays_incomplete(self, monkeypatch: pytest.MonkeyPatch) -> None: + """A structured call that keeps returning None ends degraded, not fatal (#843).""" state = _make_state("mcp_mismatched_skill", use_llm=True) - structured_llm = _mock_tp4_structured_llm(monkeypatch, [None]) + monkeypatch.setattr(LLMAnalyzerBase, "_asleep_before_retry", AsyncMock()) + structured_llm = _mock_tp4_structured_llm(monkeypatch, [None, None, None, None]) result = node(state) - assert structured_llm.calls == 1 - assert result["analyzer_status_events"][0]["status"] == "failed" + assert structured_llm.calls == 4 + assert result["analyzer_status_events"][0]["status"] == "degraded" + semantic = [event for event in result["inspection_ledger"] if event["phase"] == "semantic"] + assert [ + (event["outcome"], event["reason_code"], event["error_class"]) for event in semantic + ] == [ + ( + LedgerOutcome.SKIPPED, + LedgerReason.LLM_STRUCTURED_RESPONSE_INVALID, + "ValidationError", + ) + ] + assert result["llm_call_log"][0]["ok"] is False + completeness, _ = finalize_ledger( + { + "components": state["components"], + "findings": result["findings"], + "inspection_ledger": result["inspection_ledger"], + "analyzer_status_events": result["analyzer_status_events"], + } + ) + assert completeness["execution_successful"] is True + assert completeness["is_complete"] is False + + def test_missing_structured_response_is_retried(self, monkeypatch: pytest.MonkeyPatch): + state = _make_state("mcp_mismatched_skill", use_llm=True) + sleep = AsyncMock() + monkeypatch.setattr(LLMAnalyzerBase, "_asleep_before_retry", sleep) + structured_llm = _mock_tp4_structured_llm(monkeypatch, [None, {"is_mismatch": False}]) + + result = node(state) + + assert structured_llm.calls == 2 + sleep.assert_called_once_with(0.5) + assert result["llm_call_log"] == [{"node": "mcp_tool_poisoning", "ok": True, "error": None}] + assert result["analyzer_status_events"][0]["status"] == "completed" + + def test_bedrock_forced_tool_call_answered_in_prose_is_retried( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Bedrock's tool parser returns None for a reply with no tool call (#843).""" + monkeypatch.delenv("SKILLSPECTOR_STRUCTURED_OUTPUT_METHOD", raising=False) + from langchain_aws import ChatBedrockConverse + from langchain_core.messages import AIMessage + from langchain_core.outputs import ChatGeneration, ChatResult + + from skillspector import llm_utils + from skillspector.providers.bedrock import ( + BEDROCK_DEFAULT_MODEL, + BEDROCK_DEFAULT_REGION, + BedrockProvider, + ) + + assert BedrockProvider().forced_tool_choice_supported(BEDROCK_DEFAULT_MODEL) + calls = [] + + async def answer_in_prose(*_args, **_kwargs): + calls.append(1) + return ChatResult(generations=[ChatGeneration(message=AIMessage(content="refused"))]) + + monkeypatch.setattr(ChatBedrockConverse, "_agenerate", answer_in_prose) + monkeypatch.setattr(llm_utils, "get_active_provider", BedrockProvider) + monkeypatch.setattr( + "skillspector.llm_analyzer_base.get_chat_model", + lambda **kwargs: ChatBedrockConverse( + model=kwargs["model"], + client=MagicMock(), + bedrock_client=MagicMock(), + region_name=BEDROCK_DEFAULT_REGION, + ), + ) + monkeypatch.setattr(LLMAnalyzerBase, "_asleep_before_retry", AsyncMock()) + state = _make_state("mcp_mismatched_skill", use_llm=True) + state["model_config"] = {"default": BEDROCK_DEFAULT_MODEL} + + result = node(state) + + assert len(calls) == 4 + assert result["analyzer_status_events"][0]["status"] == "degraded" assert any( + event.get("reason_code") is LedgerReason.LLM_STRUCTURED_RESPONSE_INVALID + and event.get("error_class") == "ValidationError" + for event in result["inspection_ledger"] + ) + assert not any( event["outcome"] is LedgerOutcome.FAILED for event in result["inspection_ledger"] )