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.

[koriyoshi2041]

Pushed a8cef79 for the review blockers.

What changed:

• async batch fan-out now treats Pydantic ValidationError as a malformed model response and isolates that batch instead of re-raising it through the ValueError guard
• raw-string sync invocation also gets the same ValidationError carve-out
• semantic nodes and meta analyzer now mark partial batch loss in llm_call_log instead of reporting ok=True when analysis coverage was dropped
• added an async regression where one malformed structured batch is skipped while neighboring batches still return findings

Verified with:

• uv run pytest tests/nodes/test_llm_analyzer_base.py tests/nodes/test_meta_analyzer.py tests/nodes/analyzers/test_semantic_security_discovery.py tests/nodes/test_semantic_quality_policy.py tests/nodes/analyzers/test_semantic_developer_intent.py -q -> 210 passed, 5 deselected
• uv run ruff check src/ tests/
• uv run ruff format --check src/ tests/
• git diff --check

[rng1995]

[Automated SkillSpector Review]

Re-review approved. I verified every blocker from the 2026-07-14 automated changes-requested review against only the subsequent commits and author response:

• Async malformed structured responses are now handled before the broader ValueError propagation guard, so a Pydantic ValidationError drops only its batch. The new async regression proves neighboring batches survive.
• Partial batch loss is now observable: the sync security node, both async semantic nodes, and the meta analyzer compare submitted and returned batches and emit llm_call_log degradation records instead of reporting full success.
• Raw invoke and parse-time validation paths now have matching isolation coverage, and the batch docstrings describe the contract.

All three commits include DCO sign-offs. No new correctness, security, schema, or test blocker was found in the post-review changes. GitHub currently reports no completed check runs for this head.

[koriyoshi2041]

Rebased onto current main and resolved the 2.5.x execution-ledger overlap. Malformed sync and async structured responses are still isolated per batch, and each dropped batch now also records a sanitized BatchFailure in the new detailed outcome so inspection accounting remains complete. Targeted analyzer tests pass (237 passed, 5 deselected); Ruff lint, format check, and diff check also pass.

[rng1995]

[Automated SkillSpector Review]

Re-review approved. The rebased implementation isolates Pydantic ValidationError failures in both sync and async batch execution before the broader ValueError propagation path, records sanitized batch failures for inspection accounting, and keeps configuration errors fatal. The focused exact-diff suites passed (186 tests), as did Ruff lint and format checks.