Isolate malformed LLM batch responses

[koriyoshi2041]

Summary

• skip only the malformed LLM batch when structured response parsing fails
• keep configuration errors such as missing API keys propagating
• add sync run_batches regression coverage for a stray-text structured response

Fixes #250.

Verification

• uv run pytest tests/nodes/test_llm_analyzer_base.py -k "RunBatches or failed_batch or value_error"
• uv run pytest tests/nodes/test_llm_analyzer_base.py
• git diff --check upstream/main...HEAD
• uv run ruff check src/ tests/
• uv run ruff format --check src/ tests/

Note: make lint and make format fail in this shell because the Makefile calls bare ruff, but the equivalent uv run ruff ... commands above pass.

[rng1995]

[Automated SkillSpector Review]

Summary: Wraps the sync run_batches loop in per-batch exception handling so a malformed structured LLM response (pydantic ValidationError) skips only its own batch instead of propagating, while plain ValueError/NotImplementedError (misconfiguration) still propagate. Adds two sync regression tests. Claims to fix #250.

Blockers

1. The fix does not cover the paths that actually crash the scan — #250 remains reproducible. Pydantic v2 ValidationError subclasses ValueError. The untouched async arun_batches re-raises anything matching isinstance(result, (ValueError, NotImplementedError)) (src/skillspector/llm_analyzer_base.py:441), so a malformed structured response still escapes it as a ValidationError. Three of the four LLM stages call arun_batches (semantic_quality_policy, semantic_developer_intent, meta_analyzer), and each node re-raises via except ValueError: raise, killing the whole scan with no report — the exact symptom in #250. I verified this empirically against this PR's head: feeding arun_batches the same We{"findings":[]} payload used in the new sync test aborts the entire fan-out. Meanwhile the only run_batches caller (semantic_security_discovery) already caught ValidationError at node level on main and degraded gracefully. The same except ValidationError carve-out (placed before the ValueError re-raise check) is needed in arun_batches, with an async regression test.

2. Silent-degradation regression: dropped batches no longer surface in llm_call_log or the report. On main, a malformed response in the sync path reached semantic_security_discovery's except ValidationError handler, which recorded llm_call_record(ok=False, error="malformed LLM response: ...") — feeding the report's _llm_degradation_notice. After this PR that handler is dead code: batches are dropped inside run_batches with only a log warning, and the node reports ok=True even if every batch failed to parse. For a security scanner, unannotated loss of analysis coverage is a detection gap (and #250 explicitly asked for clearly annotated partial results). run_batches callers should be able to detect dropped batches (compare submitted vs returned, as meta_analyzer does for the async path) and the node should record a degraded/partial llm_call_record.

Non-blocking

• The raw-string branch (_message_text(self._llm.invoke(prompt))) has no ValidationError carve-out, so structured mode skips a ValidationError while raw mode propagates it. If intentional, a comment would help; otherwise align the two branches.
• The three near-identical try/except blocks in run_batches could be factored into a small helper so the exception policy (skip vs propagate) lives in one place — ideally shared with arun_batches so the two paths cannot drift again (which is exactly how blocker 1 happened).
• run_batches's docstring was not updated; arun_batches documents its failure-isolation semantics in detail — mirror that.
• Tests cover only the structured-invoke leg; the parse_response leg and raw-string leg of the new handling are untested.

Tests: the two new sync tests are well-constructed (the ValidationError is raised from inside invoke, matching how langchain structured output fails) and pass locally along with the rest of the file (113 passed).

[rng1995]

This carve-out is correct here, but the same fix is missing in arun_batches: pydantic ValidationError subclasses ValueError, so isinstance(result, (ValueError, NotImplementedError)) at line 441 re-raises it, and all three async callers (semantic_quality_policy, semantic_developer_intent, meta_analyzer) re-raise ValueError at node level — the whole-scan crash from #250 is still reproducible through those stages (verified against this branch with the same We{"findings":[]} payload). Please add an isinstance(result, ValidationError) check before the ValueError re-raise in arun_batches, plus an async regression test.

[rng1995]

Dropping the batch with only a log warning gives callers no signal. Previously a ValidationError reached semantic_security_discovery's node-level handler, which recorded llm_call_record(ok=False, error="malformed LLM response: ...") and fed the report's degradation notice; that handler is now dead code, so a scan where every batch fails to parse still reports the LLM stage as fully OK. Consider having the node compare submitted vs returned batches (as meta_analyzer does) and record a degraded call, so partial results stay annotated in the report.

[rng1995]

Inconsistency: this raw-mode branch has no ValidationError carve-out, so a ValidationError raised here (or in a subclass's raw-mode pipeline) propagates via the ValueError re-raise, while the structured branch above skips it. If intentional, add a comment; otherwise align the two branches.

[rng1995]

Maintainability: this is the first of three near-identical try/except blocks in the loop. Extracting the policy into a small helper (skip on ValidationError/other Exceptions, propagate ValueError/NotImplementedError) would keep it in one place and let arun_batches share it, preventing the sync/async drift that left the async path unfixed. Also, the run_batches docstring should document the new failure-isolation semantics the way arun_batches's docstring does.

[rng1995]

Good regression test for the sync path. Please add the async counterpart: an arun_batches test where ainvoke raises this same ValidationError and the other batches survive — it currently fails on this branch because arun_batches re-raises ValidationError via the (ValueError, NotImplementedError) isinstance check. Coverage for the parse_response leg and raw-string mode of the new sync handling would also be worthwhile.