Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions src/skillspector/nodes/report.py
Original file line number Diff line number Diff line change
Expand Up @@ -578,25 +578,27 @@ def _llm_runtime_status(
"""Return ``(attempted, succeeded, degraded)`` from the LLM call log.

``degraded`` is True when the LLM stage was requested and at least one call
was attempted, but every call failed at runtime — meaning the report
reflects static analysis only despite a deep scan being requested.
was attempted, but not every call succeeded: a dropped or throttled batch
(e.g. a 429) leaves the same coverage gap as a full failure, so a partial
pass is degraded too, not just a total one.
"""
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.

return attempted, succeeded, degraded


def _llm_degradation_notice(
use_llm: bool, llm_call_log: Sequence[Mapping[str, object]]
) -> str | None:
"""Return a human-readable degraded-scan warning, or None if not degraded."""
attempted, _succeeded, degraded = _llm_runtime_status(use_llm, llm_call_log)
attempted, succeeded, degraded = _llm_runtime_status(use_llm, llm_call_log)
if not degraded:
return None
failed = attempted - succeeded
return (
f"LLM analysis was requested but all {attempted} LLM call(s) failed - "
"results reflect STATIC analysis only."
f"LLM analysis was requested but {failed} of {attempted} LLM call(s) failed - "
"results reflect STATIC analysis only for the affected batch(es)."
)


Expand All @@ -611,15 +613,16 @@ def _build_metadata(
llm_available, llm_error = is_llm_available()
attempted, succeeded, degraded = _llm_runtime_status(use_llm, llm_call_log)
# 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.


meta: dict[str, object] = {
"has_executable_scripts": has_executable_scripts,
"skillspector_version": skillspector_version,
"llm_requested": use_llm,
# llm_available reflects runtime truth: the binary/credentials were
# available AND the stage was not fully degraded (every call failing).
# available AND every attempted call succeeded (a dropped batch is
# coverage the caller did not actually get, same as none at all).
"llm_available": llm_available and not degraded,
"meta_analysis_applied": meta_analysis_applied,
# A list (including an empty list) makes observability explicit. Empty
Expand All @@ -638,9 +641,10 @@ def _build_metadata(
{str(r.get("error")) for r in llm_call_log if not r.get("ok") and r.get("error")}
)
detail = f" Reasons: {'; '.join(reasons)}" if reasons else ""
failed = attempted - succeeded
meta["llm_error"] = (
f"LLM analysis was requested but all {attempted} LLM call(s) failed; "
f"results reflect static analysis only.{detail}"
f"LLM analysis was requested but {failed} of {attempted} LLM call(s) failed; "
f"results reflect static analysis only for the affected batch(es).{detail}"
)
elif use_llm and not llm_available:
meta["llm_error"] = llm_error
Expand Down
40 changes: 36 additions & 4 deletions tests/nodes/test_report.py
Original file line number Diff line number Diff line change
Expand Up @@ -781,8 +781,13 @@ def test_report_llm_degraded_when_all_calls_failed(monkeypatch: pytest.MonkeyPat
assert "static analysis only" in meta["llm_error"]


def test_report_not_degraded_when_some_calls_succeeded(monkeypatch: pytest.MonkeyPatch) -> None:
"""At least one successful LLM call -> not degraded, llm_available stays True."""
def test_report_degraded_when_some_calls_fail(monkeypatch: pytest.MonkeyPatch) -> None:
"""A dropped/throttled batch degrades the scan even though other calls succeeded.

A rate-limited provider can 429 one batch (e.g. the security-discovery
analyzer) while the rest of the fan-out succeeds; that is still a coverage
gap and must not read as a clean, fully-analyzed scan.
"""
monkeypatch.setattr("skillspector.nodes.report.is_llm_available", lambda: (True, None))
state: SkillspectorState = {
"filtered_findings": [],
Expand All @@ -797,10 +802,11 @@ def test_report_not_degraded_when_some_calls_succeeded(monkeypatch: pytest.Monke
],
}
meta = _meta_from_json_report(state)
assert meta["llm_available"] is True
assert "llm_degraded" not in meta
assert meta["llm_available"] is False # a dropped batch is not full coverage
assert meta["llm_degraded"] is True
assert meta["llm_calls_attempted"] == 2
assert meta["llm_calls_succeeded"] == 1
assert "1 of 2" in meta["llm_error"]


def test_report_not_degraded_when_no_llm_calls(monkeypatch: pytest.MonkeyPatch) -> None:
Expand Down Expand Up @@ -980,6 +986,32 @@ def test_degraded_scan_floors_recommendation_at_caution() -> None:
assert result["risk_recommendation"] == "CAUTION" # but never SAFE when degraded


def test_partial_llm_failure_also_floors_recommendation_at_caution() -> None:
"""A rate-limited provider dropping one batch must not read as a clean scan.

Matches the reported failure: llm_calls_attempted=4, llm_calls_succeeded=3
(one batch 429'd and was dropped), yet the report emitted a plain SAFE
verdict because only an all-calls-failed scan was treated as degraded.
"""
state: SkillspectorState = {
"filtered_findings": [], # static score 0 -> would be SAFE
"component_metadata": [],
"has_executable_scripts": False,
"manifest": {},
"output_format": "json",
"use_llm": True,
"llm_call_log": [
llm_call_record("semantic_security_discovery", ok=False, error="429 rate limited"),
llm_call_record("semantic_developer_intent", ok=True),
llm_call_record("semantic_quality_policy", ok=True),
llm_call_record("meta_analyzer", ok=True),
],
}
result = report(state)
assert result["risk_score"] == 0 # score is left honest
assert result["risk_recommendation"] == "CAUTION" # never SAFE on a partial pass


def test_non_degraded_clean_scan_stays_safe() -> None:
"""Without degradation, a clean scan still reports SAFE (no over-flooring)."""
state: SkillspectorState = {
Expand Down
Loading