Prevent silent data loss from malformed JSON in batch analysis - #5
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The restructuring into guard clauses is a readable improvement, and the isinstance(parsed, list) / empty-array / non-dict-element checks are reasonable hardening. But a few things in the diff work against the PR's stated goal of preventing silent data loss.
1. vulntester/llm/analyzer.py:307 — response[:500] truncates the only surviving copy of the response.
This is the parse-failure path, so raw_analysis is the only record of what the LLM said, and report/generator.py:327 renders it verbatim into the HTML report. A batch prompt covers up to _MAX_FINDINGS_PER_BATCH (25) findings, so a real response is many kilobytes; capping at 500 chars discards nearly all of it with no truncation marker, and the report shows a fragment cut off mid-sentence. That is new, deliberate data loss in a PR titled "prevent silent data loss," and it isn't mentioned in the description. Please keep the full response, or if a cap is genuinely wanted, make it a named constant and append an explicit "...[truncated]" marker plus log the original length.
2. vulntester/llm/analyzer.py:292-299 — an array of all non-dict elements contributes zero results and zero error entries.
If the model returns e.g. [\"finding 1 is fine\", \"finding 2 is fine\"], every element is skipped by the new isinstance(item, dict) guard, the batch appends nothing to results, and the caller has no way to tell the batch was dropped. Every other failure mode here appends a {\"finding_title\": \"Batch Analysis\", \"error\": ...} marker; this one should too — e.g. track whether any dict was appended and, if not, record an error for the batch.
3. vulntester/llm/analyzer.py:296 — non-int finding_index raises TypeError, which the except clause does not catch.
item.get(\"finding_index\", 0) - 1 raises TypeError when the model emits \"finding_index\": \"1\" (a common LLM formatting slip). TypeError is not a subclass of ValueError, so it escapes the try at line 301, propagates out of _analyze_findings_batch, and aborts analyze_report — discarding the already-completed executive summary and every prior batch's results. This is pre-existing, but it is precisely the malformed-JSON data-loss class this PR is scoped to, and the diff touches this exact line, so it should be handled here (coerce via int(...) inside a try, or add TypeError to the except tuple).
Minor: line 302 truncating str(e) to 100 chars is fine for JSONDecodeError but will clip longer ValueError messages in the log for no real benefit. The new blank lines at 291/300 (and the others in the diff) carry trailing whitespace — CI has no linter so it won't fail, but it's worth cleaning up.
No new tests accompany the change; given tests/test_llm_analyzer.py already covers this function's failure modes closely, cases for non-dict elements and the empty-extraction path would fit naturally.
…erce finding_index - raw_analysis is capped at a named _MAX_RAW_ANALYSIS_CHARS (4000) with an explicit "...[truncated, N chars total]" marker and the original length logged, instead of a silent 500-char clip. - A JSON array whose elements are all non-dicts now records a "Batch Analysis" error entry rather than contributing nothing at all. - finding_index is coerced with int(), so a string index no longer raises TypeError out of _analyze_findings_batch and aborts analyze_report. - Log the full parse error; drop trailing whitespace. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Resolved the review. All three points checked out against the code, so all three are fixed; each fix has a regression test that fails without it. 1. 2. All-non-dict array contributed nothing — fixed. Confirmed by reading the loop: every element hitting the 3. Non-int Minor points: both taken. The parse-error log now records the full Nothing pushed back on this round. Checks: |
There was a problem hiding this comment.
Traced every branch of the rewritten parse path in vulntester/llm/analyzer.py:293-354 against the new tests. The control flow is equivalent to the old extend behavior on the happy path, and each new early-continue (empty extraction, non-array, empty array, all-elements-skipped) appends a marker dict so a batch can no longer vanish from finding_analysis without a trace. _finding_index correctly contains the TypeError that "1" - 1 used to raise — that exception was not covered by the (JSONDecodeError, ValueError) handler and would have aborted the whole report, so this is a real fix rather than defensive noise; unusable indices degrade to index 0 -> -1, which falls outside the range check and leaves the item untitled instead of mistitling it.
Checked the report side too: raw_analysis and error are rendered through a Jinja Environment(autoescape=True) (vulntester/report/generator.py:386), so the newly-retained raw model text cannot inject markup, and _raw_analysis_text always leaves a "...[truncated, N chars total]" marker plus an error-level log rather than clipping silently. Non-dict elements in a mixed array are dropped with a warning — that is a deliberate narrowing (the template could not render them anyway) and is pinned by a test. Scope matches the title and description; no unrelated changes bundled in.
1 participant
What
Add defensive validation and error handling for malformed or truncated JSON responses from LLM to prevent silent data loss in batch analysis results.
Why
Malformed JSON responses from the LLM can currently cause data loss when they fail to parse. This change validates JSON extraction completeness, checks for empty strings after extraction, ensures parsed JSON is a non-empty list, validates array elements are dicts, and captures parse errors with context rather than silently dropping results.