fix(llm): isolate Stage 2 batch failures and keep unanalysed findings#32
Conversation
One exception anywhere in the arun_batches fan-out aborted the whole Stage 2 pass: asyncio.gather without return_exceptions cancelled the remaining batches, the meta-analyzer's blanket except caught the propagated error, and every file silently fell back to static-only results while the CLI still exited 0 (NVIDIA#9). arun_batches now isolates failures per batch: a transient error (timeout, 429, oversized-chunk 400) is logged and costs only its own batch. ValueError and NotImplementedError still propagate, since they signal misconfiguration rather than infra trouble. With partial results possible, apply_filter could no longer treat a missing confirmation as a rejection: a finding whose batch never returned would be silently dropped — a false negative manufactured by an infrastructure error (NVIDIA#11). The meta-analyzer now partitions findings by whether a returned batch actually carried them: analysed findings go through the normal confirm-or-drop filter, unanalysed ones are kept via the existing fallback path, and a WARNING logs how many findings were kept unfiltered so the gap is visible. Net effect: an infra failure can only ever cost enrichment on a file, never the finding itself, and one bad call no longer turns off the semantic filter for the whole scan. Fixes NVIDIA#9, fixes NVIDIA#11 Signed-off-by: nyxst4ck <289980115+nyxst4ck@users.noreply.github.com>
d09497d to
0eeae6f
Compare
rng1995
left a comment
There was a problem hiding this comment.
Solid, well-reasoned fix. I traced both halves against the current code and the keep/drop semantics hold up, including the conservative (fail-closed) direction that matters for a security scanner.
Correctness — verified
get_batches(files_with_findings, file_cache, findings)builds batches from the sameFindinginstances (it appendsfdirectly / passes them throughfindings_in_range), andarun_batchesreturns those sameBatchobjects. So theid(f)partition inmeta_analyzeris sound: a finding is "analysed" only if a batch that actually returned carried it.- When no batch fails (
len(batch_results) == len(batches)), the code reduces toapply_filter(findings, batch_results)— byte-for-byte the previous behavior, so there's no regression to the strict confirm-or-drop path. The added node test (test_no_failures_keeps_strict_confirm_or_drop) pins this. analysedandunanalysedare a disjoint partition offindings, so combiningapply_filter(analysed, …)with_fallback_filtered(unanalysed)can't double-count or drop.apply_filteris pure confirm-or-drop keyed by(file, rule_id, start/end_line)and_fallback_filteredis keep-all, so routing un-analysed findings through the fallback genuinely prevents an infra failure from turning into a false negative. The all-batches-failed case collapses to today's "keep everything via fallback", as claimed.- Re-raising
ValueErrorfromarun_batchesis consistent with the caller's existingexcept ValueError: raise, so misconfiguration still surfaces instead of masquerading as a clean scan.
Tests
Good coverage of exactly the risky logic: per-batch isolation, all-failed → empty, ValueError propagation, and the three node-level keep/drop scenarios (confirmed kept + enriched, rejected dropped, unseen kept, all-failed keep-all, strict path preserved). Appreciate that the node tests are red on main / green with the fix.
Minor / optional (non-blocking)
- In
arun_batches,isinstance(result, BaseException)+continuewill also swallowasyncio.CancelledError(it derives fromBaseException, andreturn_exceptions=Truesurfaces child cancellations as results). Consider narrowing the swallow toException, or explicitly re-raisingasyncio.CancelledError, so cooperative cancellation isn't masked. Low severity since outer-task cancellation still propagates from thegatherawait itself. - The
id()-based partition is correct today but silently depends onget_batchesnot copying findings. A one-line comment noting that invariant (or matching on a stable finding key) would harden it against a future refactor. Worth noting the failure mode is itself fail-safe — if instances ever diverged, findings would be treated as unanalysed and kept, never dropped.
These are optional; the change is correct, conservative in the right direction, and well-tested. Approving.
|
@nyxst4ck - Please resolve the conflicts, minor issues and merge the PR. |
Resolve the meta_analyzer test conflict, keep the partial-batch coverage alongside upstream apply_filter tests, and propagate asyncio cancellation instead of treating it as a transient LLM batch failure.
|
Resolved the merge conflict by merging current upstream/main into this branch, and addressed the two minor review notes while I was there:
Validation:
|
Summary
Fixes the two coupled Stage 2 resilience bugs, which the reporter noted should be considered together because fixing the abort without fixing the drop would make the scanner less safe:
apply_filtersilently drops findings for files the LLM never analyzed (false negatives)Changes
llm_analyzer_base.py— per-batch failure isolation.arun_batchesnow gathers withreturn_exceptions=True: a transient error (timeout, 429, oversized-chunk 400) is logged with the batch label and costs only its own batch instead of cancelling the whole fan-out.ValueErrorandNotImplementedErrorstill propagate — they signal misconfiguration (missing API key, wrong response_schema), which skipping a batch cannot fix. This also benefits the two discovery analyzers (semantic_developer_intent,semantic_quality_policy), which previously lost all discovered findings when any single batch failed.meta_analyzer.py— no verdict means conservative keep, not delete. With partial results possible, the keep/drop loop must distinguish "the LLM rejected this" from "the LLM never saw this". The node now partitions findings by whether a returned batch actually carried them (eachBatchalready tracks its findings): analysed findings go through the normal confirm-or-drop filter; unanalysed ones are kept via the existing_fallback_filteredpath; a WARNING logs how many findings were kept unfiltered so the gap is visible rather than silent. When all batches fail, behaviour matches today's net result (everything kept via fallback).Net effect, as suggested in #11: an infra failure can only ever cost enrichment on a file, never the finding itself.
Tests
arun_batches: a failing batch doesn't abort the others; all-failed returns empty;ValueErrorstill propagates (5 new tests intest_llm_analyzer_base.py).tests/nodes/test_meta_analyzer.py: confirmed finding kept enriched + rejected finding dropped + unseen finding kept when one batch fails; all-batches-failed keeps everything; strict confirm-or-drop preserved when nothing fails.All commits are signed off per the DCO.