Prevent silent data loss from malformed JSON in batch analysis

tests/test_llm_analyzer.py

@@ -15,7 +15,7 @@
 import pytest
 
 from vulntester.config import Severity
-from vulntester.llm.analyzer import LLMAnalyzer
+from vulntester.llm.analyzer import _MAX_RAW_ANALYSIS_CHARS, LLMAnalyzer
 from vulntester.llm.client import LLMClient, LLMError, LLMProvider
 from vulntester.report.models import Finding, ScanReport, ZeroDayAnomaly
 
@@ -191,6 +191,89 @@ def test_analyzer_uses_the_client_request_timeout(analyzer):
     assert kwargs["timeout"] == LLMClient._REQUEST_TIMEOUT
 
 
+# --- a malformed array must not vanish, and must not abort the run -----------
+
+
+def _responding(text: str):
+    return lambda *a, **k: _FakeResponse(_claude_text(text))
+
+
+def test_array_of_only_non_dicts_records_an_error(analyzer):
+    """Every element skipped used to append nothing at all."""
+    payload = json.dumps(["finding 1 is fine", "finding 2 is fine"])
+    with patch("urllib.request.urlopen", side_effect=_responding(payload)):
+        results = analyzer._analyze_findings_batch([_finding()])
+
+    assert len(results) == 1
+    assert "no analysis objects" in results[0]["error"]
+
+
+def test_non_dict_elements_are_skipped_without_flagging_a_usable_batch(analyzer):
+    payload = json.dumps(["noise", {"finding_index": 1, "risk_rating": 4}])
+    with patch("urllib.request.urlopen", side_effect=_responding(payload)):
+        results = analyzer._analyze_findings_batch([_finding()])
+
+    assert results == [
+        {"finding_index": 1, "risk_rating": 4, "finding_title": "Finding 1"}
+    ]
+
+
+def test_empty_json_extraction_records_an_error(analyzer):
+    with patch("urllib.request.urlopen", side_effect=_responding("```json\n\n```")):
+        results = analyzer._analyze_findings_batch([_finding()])
+
+    assert len(results) == 1
+    assert "no valid JSON" in results[0]["error"]
+
+
+def test_string_finding_index_is_coerced_not_raised(analyzer):
+    """``"1" - 1`` raises TypeError, which the except clause does not catch."""
+    payload = json.dumps([{"finding_index": "1", "risk_rating": 6}])
+    with patch("urllib.request.urlopen", side_effect=_responding(payload)):
+        results = analyzer._analyze_findings_batch([_finding()])
+
+    assert results == [
+        {"finding_index": "1", "risk_rating": 6, "finding_title": "Finding 1"}
+    ]
+
+
+def test_unusable_finding_index_leaves_the_item_untitled(analyzer):
+    payload = json.dumps([{"finding_index": None, "risk_rating": 6}])
+    with patch("urllib.request.urlopen", side_effect=_responding(payload)):
+        results = analyzer._analyze_findings_batch([_finding()])
+
+    assert results == [{"finding_index": None, "risk_rating": 6}]
+
+
+def test_a_bad_index_does_not_discard_the_rest_of_the_report(analyzer):
+    payload = json.dumps([{"finding_index": "1", "risk_rating": 6}])
+    with patch("urllib.request.urlopen", side_effect=_responding(payload)):
+        results = analyzer.analyze_report(_report())
+
+    assert results["finding_analysis"][0]["risk_rating"] == 6
+
+
+# --- an unparseable response must not be silently clipped --------------------
+
+
+def test_short_unparseable_response_is_kept_whole(analyzer):
+    text = "Sorry, I cannot produce JSON for these findings."
+    with patch("urllib.request.urlopen", side_effect=_responding(text)):
+        results = analyzer._analyze_findings_batch([_finding()])
+
+    assert results[0]["raw_analysis"] == text
+
+
+def test_long_unparseable_response_is_marked_as_truncated(analyzer):
+    text = "x" * (_MAX_RAW_ANALYSIS_CHARS + 5000)
+    with patch("urllib.request.urlopen", side_effect=_responding(text)):
+        results = analyzer._analyze_findings_batch([_finding()])
+
+    raw = results[0]["raw_analysis"]
+    assert raw.startswith("x" * _MAX_RAW_ANALYSIS_CHARS)
+    assert f"...[truncated, {len(text)} chars total]" in raw
+
+
 # --- successful paths still work --------------------------------------------
 
 

vulntester/llm/analyzer.py

@@ -13,6 +13,41 @@
 # Maximum findings to send per LLM request to avoid token limits
 _MAX_FINDINGS_PER_BATCH = 25
 
+# Cap on the unparseable response text kept for the report. A batch covers up to
+# _MAX_FINDINGS_PER_BATCH findings, so the raw text can be many kilobytes; the cap
+# keeps the report readable and always leaves a marker saying what was dropped.
+_MAX_RAW_ANALYSIS_CHARS = 4000
+
+
+def _raw_analysis_text(response: str) -> str:
+    """Return ``response`` for the report, marked and logged if it had to be cut."""
+    if len(response) <= _MAX_RAW_ANALYSIS_CHARS:
+        return response
+    logger.error(
+        "Unparseable LLM response truncated for the report: kept %d of %d chars",
+        _MAX_RAW_ANALYSIS_CHARS,
+        len(response),
+    )
+    return (
+        response[:_MAX_RAW_ANALYSIS_CHARS]
+        + f"\n...[truncated, {len(response)} chars total]"
+    )
+
+
+def _finding_index(item: dict) -> int:
+    """Return the 1-based ``finding_index``, or 0 when it is missing or not a number.
+
+    Models sometimes emit the index as a string (``"finding_index": "1"``).
+    Subtracting from that raises ``TypeError``, which would abort the whole run,
+    so coerce here and let one bad element degrade to "untitled" instead.
+    """
+    raw = item.get("finding_index", 0)
+    try:
+        return int(raw)
+    except (TypeError, ValueError):
+        logger.warning("Ignoring non-numeric finding_index: %r", raw)
+        return 0
+
 
 class LLMAnalyzer:
     """Uses an LLM to analyze scan results and provide detailed remediation guidance."""
@@ -262,32 +297,60 @@ def _analyze_findings_batch(self, findings: list[Finding]) -> list[dict]:
                     m = re.search(r"```(?:json)?\s*\n?(.*?)\n?```", response, re.DOTALL)
                     if m:
                         json_text = m.group(1)
+
+                if not json_text or not json_text.strip():
+                    logger.error("JSON extraction yielded empty string for findings batch")
+                    results.append({
+                        "finding_title": "Batch Analysis",
+                        "error": "LLM response contained no valid JSON",
+                    })
+                    continue
+
                 parsed = json.loads(json_text)
-                if isinstance(parsed, list):
-                    if not parsed:
-                        logger.warning("LLM returned empty JSON array for findings batch")
-                        results.append({
-                            "finding_title": "Batch Analysis",
-                            "error": "LLM returned empty array",
-                        })
-                    else:
-                        for item in parsed:
-                            idx = item.get("finding_index", 0) - 1
-                            if 0 <= idx < len(batch):
-                                item["finding_title"] = batch[idx].title
-                        results.extend(parsed)
-                else:
+
+                if not isinstance(parsed, list):
                     logger.error(f"LLM JSON response is not an array: {type(parsed)}")
                     results.append({
                         "finding_title": "Batch Analysis",
                         "error": "LLM response JSON is not an array",
                     })
+                    continue
+
+                if not parsed:
+                    logger.warning("LLM returned empty JSON array for findings batch")
+                    results.append({
+                        "finding_title": "Batch Analysis",
+                        "error": "LLM returned empty array",
+                    })
+                    continue
+
+                analyzed = 0
+                for item in parsed:
+                    if not isinstance(item, dict):
+                        logger.warning(f"Skipping non-dict item in LLM JSON array: {type(item)}")
+                        continue
+                    idx = _finding_index(item) - 1
+                    if 0 <= idx < len(batch):
+                        item["finding_title"] = batch[idx].title
+                    results.append(item)
+                    analyzed += 1
+
+                if not analyzed:
+                    # Every element was skipped: without a marker the caller would
+                    # see the batch simply vanish.
+                    logger.error("LLM JSON array held no analysis objects for findings batch")
+                    results.append({
+                        "finding_title": "Batch Analysis",
+                        "error": "LLM response JSON array contained no analysis objects",
+                    })
+
             except (json.JSONDecodeError, ValueError) as e:
-                logger.error(f"Failed to parse LLM JSON response: {str(e)}")
-                # If JSON parsing fails, store raw text
+                logger.error(f"Failed to parse LLM JSON response: {e}")
+                # If JSON parsing fails, store raw text with error context
                 results.append({
                     "finding_title": "Batch Analysis",
-                    "raw_analysis": response,
+                    "error": f"JSON parse failed: {type(e).__name__}",
+                    "raw_analysis": _raw_analysis_text(response),
                 })
 
         return results