fix(learn): harden analyzer JSON parsing against malformed LLM output

headroom/learn/analyzer.py

@@ -514,7 +514,12 @@ def _strip_fenced_json(raw: str) -> dict:
 
     # Nothing parsed as an object: re-raise the natural error on the raw text
     # so callers see a JSONDecodeError, preserving the documented contract.
-    result: dict = json.loads(text)
+    # json.loads raises for invalid JSON, but valid JSON that is not an object
+    # (e.g. a bare array the model emitted) would otherwise be returned as a
+    # non-dict and crash callers that do `.get(...)`, so reject it explicitly.
+    result = json.loads(text)
+    if not isinstance(result, dict):
+        raise json.JSONDecodeError("Expected a JSON object", text, 0)
     return result
 
 
@@ -820,11 +825,16 @@ def _parse_llm_response(raw: dict) -> list[Recommendation]:
     """Convert LLM structured output into Recommendation objects."""
     recommendations: list[Recommendation] = []
 
-    for rule in raw.get("context_file_rules", []):
+    # `.get(key, [])` / `.get(key, "")` only fall back when the key is absent;
+    # the model controls this JSON, so a present-but-null `*_file_rules` list or
+    # a non-string `section`/`content` would crash the iteration / `.strip()`.
+    # `_as_str` coerces any non-string (null, number, list, object) to "" so a
+    # malformed field is treated like an absent one instead of raising.
+    for rule in raw.get("context_file_rules") or []:
         if not isinstance(rule, dict):
             continue
-        section = rule.get("section", "").strip()
-        content = rule.get("content", "").strip()
+        section = _as_str(rule.get("section")).strip()
+        content = _as_str(rule.get("content")).strip()
         if not section or not content:
             continue
         recommendations.append(
@@ -838,11 +848,11 @@ def _parse_llm_response(raw: dict) -> list[Recommendation]:
             )
         )
 
-    for rule in raw.get("memory_file_rules", []):
+    for rule in raw.get("memory_file_rules") or []:
         if not isinstance(rule, dict):
             continue
-        section = rule.get("section", "").strip()
-        content = rule.get("content", "").strip()
+        section = _as_str(rule.get("section")).strip()
+        content = _as_str(rule.get("content")).strip()
         if not section or not content:
             continue
         recommendations.append(
@@ -862,6 +872,17 @@ def _parse_llm_response(raw: dict) -> list[Recommendation]:
     return recommendations
 
 
+def _as_str(val: object) -> str:
+    """Return ``val`` if it is a string, else ``""``.
+
+    Fields like ``section``/``content`` come straight from model-controlled JSON,
+    where a number/list/object is as possible as a string. Coercing a non-string
+    to empty keeps ``.strip()`` from raising and lets the caller's emptiness check
+    drop the malformed rule.
+    """
+    return val if isinstance(val, str) else ""
+
+
 def _safe_int(val: object) -> int:
     """Safely convert a value to int."""
     if isinstance(val, int):

tests/test_learn/test_analyzer.py

@@ -361,6 +361,40 @@ def test_handles_non_dict_entries(self):
         recs = _parse_llm_response(raw)
         assert recs == []
 
+    def test_handles_null_rule_lists_and_fields(self):
+        # The model controls this JSON. A present-but-null rules list or a null
+        # section/content must be treated like an absent value, not crash the
+        # iteration / `.strip()`.
+        assert _parse_llm_response({"context_file_rules": None, "memory_file_rules": None}) == []
+        assert (
+            _parse_llm_response({"context_file_rules": [{"section": None, "content": "x"}]}) == []
+        )
+        assert _parse_llm_response({"memory_file_rules": [{"section": "s", "content": None}]}) == []
+
+        # A truthy non-string section/content (number, list, object) is also
+        # model-controlled and must not crash `.strip()`; it is dropped like null.
+        assert _parse_llm_response({"context_file_rules": [{"section": 123, "content": "x"}]}) == []
+        assert _parse_llm_response({"context_file_rules": [{"section": "s", "content": 123}]}) == []
+        assert (
+            _parse_llm_response({"memory_file_rules": [{"section": ["s"], "content": "x"}]}) == []
+        )
+        assert (
+            _parse_llm_response({"memory_file_rules": [{"section": "s", "content": {"a": 1}}]})
+            == []
+        )
+
+        # A valid rule alongside the null shapes is still parsed.
+        recs = _parse_llm_response(
+            {
+                "context_file_rules": [
+                    None,
+                    {"section": "Large Files", "content": "use offset", "evidence_count": 3},
+                ]
+            }
+        )
+        assert [r.section for r in recs] == ["Large Files"]
+        assert recs[0].evidence_count == 3
+
 
 # =============================================================================
 # Full Analyzer Integration Tests (mocked LLM)
@@ -592,6 +626,19 @@ def test_invalid_json_raises(self):
         with pytest.raises(json.JSONDecodeError):
             _strip_fenced_json("not json at all")
 
+    def test_non_object_json_raises(self):
+        # A model may return valid JSON that is not an object (a bare array,
+        # string, number, ...). The contract is to raise JSONDecodeError so
+        # callers do not receive a non-dict and crash on `.get`.
+        for raw in ("[1, 2, 3]", '["a", "b"]', '[{"a": 1}, {"b": 2}]', '"a string"', "42", "true"):
+            with pytest.raises(json.JSONDecodeError):
+                _strip_fenced_json(raw)
+
+    def test_array_wrapping_a_single_object_is_extracted(self):
+        # The lenient first-{ .. last-} slice still recovers a single object a
+        # model wrapped in an array; only genuinely object-less JSON raises.
+        assert _strip_fenced_json('[{"key": "value"}]') == {"key": "value"}
+
     def test_prose_preamble_before_fence(self):
         # Models sometimes add a preamble before the fence despite being told
         # to return JSON only (e.g. "Here it is:\n\n```json ...").