Skip to content

fix(report): flag a partial LLM failure as degraded, not only a total one - #362

Open
AmirF194 wants to merge 2 commits into
NVIDIA:mainfrom
AmirF194:fix/303-partial-llm-failure-not-flagged-degraded
Open

fix(report): flag a partial LLM failure as degraded, not only a total one#362
AmirF194 wants to merge 2 commits into
NVIDIA:mainfrom
AmirF194:fix/303-partial-llm-failure-not-flagged-degraded

Conversation

@AmirF194

Copy link
Copy Markdown

What

report() derives degraded from _llm_runtime_status(), which only set it when every attempted LLM call failed (succeeded == 0). A rate-limited provider that drops one batch (e.g. a 429 on semantic_security_discovery) still has succeeded > 0, so the existing fail-closed floor (CAUTION instead of SAFE on a degraded scan) never triggered. The reported scenario is exactly this: llm_calls_attempted=4, llm_calls_succeeded=3, risk_assessment still SAFE, and the dropped batch happened to be the one analyzer that would have caught the malicious skill in the report.

This widens the condition to succeeded < attempted, so any dropped or throttled batch marks the scan degraded, not just a total failure. The two degraded-scan messages (_llm_degradation_notice, meta["llm_error"]) are updated to say how many of the calls failed instead of assuming all of them did, since that is no longer always true.

Test

Added test_partial_llm_failure_also_floors_recommendation_at_caution, matching the reported 3/4 scenario directly against report(). Renamed the test that pinned the old behavior (test_report_not_degraded_when_some_calls_succeeded -> test_report_degraded_when_some_calls_fail) to assert the corrected one. Full tests/nodes/test_report.py green (63 passed); make lint and make format-check clean.

Scope

This covers request 3 of #303 (surface incompleteness in the verdict). Request 1 (configurable concurrency) shipped in #305; request 2 (retry with backoff) is left to the already-open #29, which this PR does not touch or conflict with (it does not modify _llm_runtime_status).

Refs #303

… one

_llm_runtime_status() only set degraded when every LLM call failed
(succeeded == 0). A rate-limited provider that drops a single batch
(e.g. semantic_security_discovery hits a 429) still has succeeded > 0,
so the scan reported a normal risk_assessment even though the
security-critical analyzer never ran. Widen the condition to
succeeded < attempted, so any dropped batch degrades the scan and
the existing fail-closed floor (CAUTION instead of SAFE) applies to a
partial pass too. Updated the two degraded-scan messages to say how
many of the calls failed instead of assuming all of them did.

Covers request 3 of NVIDIA#303 (surface incompleteness in the verdict).
Request 1 (configurable concurrency) shipped in NVIDIA#305; request 2
(retry with backoff) is left to the already-open NVIDIA#29.

Refs NVIDIA#303

Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The report-level predicate now handles mixed analyzer outcomes, but the core partial-batch path is still recorded as a success upstream, so a multi-batch analyzer can drop work and still produce SAFE. The new predicate also conflates overall coverage with whether the meta-analyzer/provider actually ran. Please address the inline findings so the degraded verdict and metadata are accurate end to end.

attempted = len(llm_call_log)
succeeded = sum(1 for r in llm_call_log if r.get("ok"))
degraded = bool(use_llm and attempted > 0 and succeeded == 0)
degraded = bool(use_llm and attempted > 0 and succeeded < attempted)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Detect failures at batch granularity

llm_call_log is not a per-batch log today. The semantic analyzers and meta_analyzer emit one record with ok=bool(outcome.successful) or not outcome.failures, so a two-batch run with one success and one 429 is recorded as ok=True; test_partial_batch_failure_records_llm_success currently pins that behavior. In that exact multi-file/multi-batch case, succeeded == attempted here and the report remains SAFE, so this does not yet implement the advertised ‘any dropped batch’ behavior. Please either emit per-batch records or mark the analyzer record failed whenever outcome.failures is non-empty, then add an analyzer-to-report regression test.

Comment thread src/skillspector/nodes/report.py Outdated
# meta_analysis_applied reflects whether the LLM meta-analysis effectively
# ran: requested, available, and not fully degraded (every call failing).
# ran in full: requested, available, and every attempted call succeeded.
meta_analysis_applied = use_llm and llm_available and not degraded

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Keep coverage separate from meta-analysis/availability

After this change, degraded means any LLM-backed analyzer failed—not that the meta-analyzer failed or the provider was unavailable. In the new 3/4 scenario, meta_analyzer is explicitly successful, yet this forces meta_analysis_applied=False, adds filtering_mode="heuristic", and line 626 reports llm_available=False despite three successful calls. That misstates independent contracts (and #303 explicitly distinguishes throttled partial coverage from provider unavailability). Please derive meta-analysis from the meta_analyzer outcome, retain provider availability, and use llm_degraded/a coverage field for the partial loss.

…p meta-analysis fields inheriting other analyzers' failures

Two gaps from review on NVIDIA#362:

1. llm_call_log records were built with
   ok=bool(outcome.successful) or not outcome.failures, so an analyzer with
   one succeeded batch and one dropped/429'd batch still recorded ok=True.
   In that exact case succeeded == attempted at the report layer and the
   scan stayed SAFE, defeating the partial-coverage fix. Now the record is
   ok=not outcome.failures: any dropped batch marks the whole record failed.
   Applied identically in the three semantic analyzers and meta_analyzer,
   the four call sites that build this record.

2. meta_analysis_applied and the llm_available field were derived from the
   aggregate `degraded` flag, which pools every LLM-backed node together.
   That let a different analyzer's dropped batch force
   meta_analysis_applied=False, filtering_mode="heuristic" and
   llm_available=False even when meta_analyzer's own call fully succeeded,
   misstating two independent contracts (meta-analysis ran vs. some
   coverage was lost) as one boolean. Both fields now derive from
   is_llm_available() plus meta_analyzer's own llm_call_log record only;
   the coverage loss from other analyzers still surfaces through
   llm_degraded / llm_calls_attempted / llm_calls_succeeded, unchanged.

Verified: test_partial_batch_failure_records_llm_failure (renamed from
..._records_llm_success, now pins ok=False) and three new report-level
tests, run red against the pre-fix code (3 of 4 failed) and green after.
tests/nodes/test_report.py: 66 passed. Full suite in Docker
(python:3.12-slim): 1947 passed, 13 skipped, 4 xfailed, 0 failed. ruff
lint and format-check both pass.

Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>
@AmirF194

Copy link
Copy Markdown
Author

Pushed a follow-up commit for both findings.

P1 (line 587): llm_call_log records were built with ok=bool(outcome.successful) or not outcome.failures, so a batch that partially failed (one file's batch succeeds, another 429s) still recorded ok=True as long as any batch came back. Changed all four call sites (the three semantic analyzers and meta_analyzer) to ok=not outcome.failures, so any dropped batch marks the record failed. test_partial_batch_failure_records_llm_success in test_semantic_developer_intent.py pinned the old behavior; renamed it to test_partial_batch_failure_records_llm_failure and inverted the assertion. Added test_analyzer_partial_batch_failure_flows_through_to_report_degraded in test_report.py, which drives the real semantic_developer_intent.node() through a mocked one-success/one-timeout batch outcome and feeds its actual llm_call_log output into report(), asserting llm_degraded and the CAUTION floor. That's an end-to-end test, not just a predicate test.

P2 (line 617/626): meta_analysis_applied and llm_available were derived from the aggregate degraded flag, which pools every LLM-backed node together, so a different analyzer's dropped batch could force meta_analysis_applied=False and llm_available=False even when meta_analyzer itself fully succeeded. Both fields now derive from is_llm_available() plus meta_analyzer's own llm_call_log record specifically (a missing record, e.g. no findings to filter, reads as vacuously ok). The coverage loss from other analyzers is unchanged and still surfaces through llm_degraded / llm_calls_attempted / llm_calls_succeeded. Added two tests: one for the 3/4 scenario you described (meta_analyzer ok, one semantic analyzer's batch dropped) asserting meta_analysis_applied/llm_available stay True while llm_degraded stays True; one where meta_analyzer's own record is the failure, asserting both fields correctly go False regardless of the other analyzers.

I did not add a separate coverage field beyond the existing llm_degraded / llm_calls_attempted / llm_calls_succeeded trio; those already report exactly which fraction of calls dropped, so a new field seemed redundant, but happy to add one if you had something more specific in mind.

Verified: ran the new/changed tests against the pre-fix code first to confirm they fail (3 of 4 red, the meta_analyzer-failure case already passed under the old formula since that one case wasn't actually broken), then confirmed green after the fix. tests/nodes/test_report.py: 66 passed. Full suite in a clean python:3.12-slim Docker container: 1947 passed, 13 skipped, 4 xfailed, 0 failed. ruff check and ruff format --check both pass. I did not exercise a real 429 against a live provider; the batch failures are simulated via a mocked arun_batches outcome, same approach the existing test suite uses throughout this file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants