Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,11 @@ def node(state: SkillspectorState) -> AnalyzerNodeResponse:
"inspection_ledger": events,
"analyzer_status_events": [status],
"llm_call_log": [
llm_call_record(ANALYZER_ID, ok=bool(outcome.successful) or not outcome.failures)
# A record is ok only when every submitted batch succeeded. A
# partial batch failure (e.g. one file's batch 429'd while
# another's succeeded) is still lost coverage, so it must not
# read as ok=True just because some batches came back.
llm_call_record(ANALYZER_ID, ok=not outcome.failures)
],
"inference_usage": analyzer.inference_usage,
}
Expand Down
6 changes: 5 additions & 1 deletion src/skillspector/nodes/analyzers/semantic_quality_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,11 @@ def node(state: SkillspectorState) -> AnalyzerNodeResponse:
"inspection_ledger": events,
"analyzer_status_events": [status],
"llm_call_log": [
llm_call_record(ANALYZER_ID, ok=bool(outcome.successful) or not outcome.failures)
# A record is ok only when every submitted batch succeeded. A
# partial batch failure (e.g. one file's batch 429'd while
# another's succeeded) is still lost coverage, so it must not
# read as ok=True just because some batches came back.
llm_call_record(ANALYZER_ID, ok=not outcome.failures)
],
"inference_usage": analyzer.inference_usage,
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,11 @@ def node(state: SkillspectorState) -> AnalyzerNodeResponse:
"inspection_ledger": all_events,
"analyzer_status_events": [status],
"llm_call_log": [
llm_call_record(ANALYZER_ID, ok=bool(outcome.successful) or not outcome.failures)
# A record is ok only when every submitted batch succeeded. A
# partial batch failure (e.g. one file's batch 429'd while
# another's succeeded) is still lost coverage, so it must not
# read as ok=True just because some batches came back.
llm_call_record(ANALYZER_ID, ok=not outcome.failures)
],
"inference_usage": analyzer.inference_usage,
}
Expand Down
9 changes: 5 additions & 4 deletions src/skillspector/nodes/meta_analyzer.py
Original file line number Diff line number Diff line change
Expand Up @@ -716,10 +716,11 @@ def meta_analyzer(state: SkillspectorState) -> MetaAnalyzerResponse:
"inspection_ledger": ledger_events,
"analyzer_status_events": [status],
"llm_call_log": [
llm_call_record(
"meta_analyzer",
ok=bool(detailed.successful) or not detailed.failures,
)
# A record is ok only when every submitted batch succeeded. A
# partial batch failure (e.g. one file's batch 429'd while
# another's succeeded) is still lost coverage, so it must not
# read as ok=True just because some batches came back.
llm_call_record("meta_analyzer", ok=not detailed.failures)
],
"inference_usage": analyzer.inference_usage,
}
Expand Down
47 changes: 32 additions & 15 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 @@ -608,19 +610,33 @@ def _build_metadata(
) -> dict[str, object]:
"""Build the metadata section shared by all output formats."""
llm_call_log = llm_call_log or []
llm_available, llm_error = is_llm_available()
provider_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).
meta_analysis_applied = use_llm and llm_available and not degraded

# meta_analyzer's own record, independent of whether a DIFFERENT
# LLM-backed node (a semantic_* analyzer) lost coverage to a dropped
# batch. A missing record means meta_analyzer never ran (e.g. there were
# no findings to filter), which is not itself a failure, so it reads as
# vacuously ok. When it does run it always emits exactly one record.
meta_analyzer_records = [r for r in llm_call_log if r.get("node") == "meta_analyzer"]
meta_analyzer_ok = all(bool(r.get("ok")) for r in meta_analyzer_records)

# meta_analysis_applied / llm_available answer "did meta-analysis itself
# run": the provider was available and meta_analyzer's own call (if it
# ran) succeeded. A different analyzer's partial batch loss is a
# coverage gap, reported separately below via llm_degraded /
# llm_calls_attempted / llm_calls_succeeded, and must not flip these two
# fields false on its own - that conflated two independent contracts
# (meta-analysis ran vs. some coverage was lost) into one boolean.
meta_analysis_applied = use_llm and provider_available and meta_analyzer_ok

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).
"llm_available": llm_available and not degraded,
# available AND meta_analyzer's own call (if it ran) succeeded.
"llm_available": provider_available and meta_analyzer_ok,
"meta_analysis_applied": meta_analysis_applied,
# A list (including an empty list) makes observability explicit. Empty
# means the provider/transport supplied no counters; it is never an
Expand All @@ -638,11 +654,12 @@ 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:
elif use_llm and not provider_available:
meta["llm_error"] = llm_error
return meta

Expand Down
11 changes: 9 additions & 2 deletions tests/nodes/analyzers/test_semantic_developer_intent.py
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,14 @@ def test_success_records_ok_true(self) -> None:
assert result["llm_call_log"] == [{"node": ANALYZER_ID, "ok": True, "error": None}]

@patch(MOCK_PATCH_TARGET, _mock_get_chat_model)
def test_partial_batch_failure_records_llm_success(self) -> None:
def test_partial_batch_failure_records_llm_failure(self) -> None:
"""One batch succeeding does not hide another batch's dropped coverage.

Regression for the case where a two-file run has one file batch
succeed and the other 429 / time out: the record must be ok=False so
the report can detect the coverage gap, not ok=True just because
`outcome.successful` was non-empty.
"""
from skillspector.llm_analyzer_base import LLMAnalyzerBase

async def partially_succeeds(self, batches, **_kwargs):
Expand All @@ -267,7 +274,7 @@ async def partially_succeeds(self, batches, **_kwargs):
with patch.object(LLMAnalyzerBase, "arun_batches", partially_succeeds):
result = node({"file_cache": {"first.py": "print(1)", "second.py": "print(2)"}})

assert result["llm_call_log"] == [{"node": ANALYZER_ID, "ok": True, "error": None}]
assert result["llm_call_log"] == [{"node": ANALYZER_ID, "ok": False, "error": None}]

@patch(MOCK_PATCH_TARGET)
def test_exception_records_ok_false(self, mock_get_model: MagicMock) -> None:
Expand Down
183 changes: 178 additions & 5 deletions tests/nodes/test_report.py
Original file line number Diff line number Diff line change
Expand Up @@ -754,7 +754,14 @@ def _meta_from_json_report(state: SkillspectorState) -> dict:


def test_report_llm_degraded_when_all_calls_failed(monkeypatch: pytest.MonkeyPatch) -> None:
"""use_llm requested + every LLM call failed -> llm_available False, llm_degraded True."""
"""use_llm requested + every semantic-analyzer call failed -> llm_degraded True.

llm_available/meta_analysis_applied are about the provider and
meta_analyzer's OWN call specifically (see the meta_analysis_applied
tests below); none of these three failures is a meta_analyzer record,
so those two fields stay True here and the failure surfaces only via
llm_degraded / llm_calls_attempted / llm_calls_succeeded / llm_error.
"""
# Pre-flight reports available (binary/creds present); the failure is at runtime.
monkeypatch.setattr("skillspector.nodes.report.is_llm_available", lambda: (True, None))
state: SkillspectorState = {
Expand All @@ -772,7 +779,7 @@ def test_report_llm_degraded_when_all_calls_failed(monkeypatch: pytest.MonkeyPat
}
meta = _meta_from_json_report(state)
assert meta["llm_requested"] is True
assert meta["llm_available"] is False # degraded -> not actually available
assert meta["llm_available"] is True # provider ok; meta_analyzer never ran/failed
assert meta["llm_degraded"] is True
assert meta["llm_calls_attempted"] == 3
assert meta["llm_calls_succeeded"] == 0
Expand All @@ -781,8 +788,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 +809,77 @@ def test_report_not_degraded_when_some_calls_succeeded(monkeypatch: pytest.Monke
],
}
meta = _meta_from_json_report(state)
# Neither record is meta_analyzer, so llm_available/meta_analysis_applied
# are untouched by this coverage gap; llm_degraded is the signal for it.
assert meta["llm_available"] is True
assert "llm_degraded" not in meta
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_meta_analysis_applied_survives_other_analyzer_partial_failure(
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""meta_analyzer succeeding is independent of a different analyzer's coverage loss.

Regression for the reviewed gap: a semantic analyzer dropping a batch
forced meta_analysis_applied=False and llm_available=False even though
meta_analyzer's own call succeeded in full, which misstated two
independent contracts (meta-analysis ran vs. some coverage was lost) as
one boolean. Matches the reported 3/4 scenario: 3 calls succeed
(including meta_analyzer), 1 semantic-analyzer batch is dropped.
"""
monkeypatch.setattr("skillspector.nodes.report.is_llm_available", lambda: (True, None))
state: SkillspectorState = {
"filtered_findings": [],
"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),
],
}
meta = _meta_from_json_report(state)
assert meta["meta_analysis_applied"] is True
assert meta["llm_available"] is True
assert "filtering_mode" not in meta
# The lost coverage is still visible, just not through these two fields.
assert meta["llm_degraded"] is True
assert meta["llm_calls_attempted"] == 4
assert meta["llm_calls_succeeded"] == 3


def test_report_meta_analysis_not_applied_when_meta_analyzer_itself_fails(
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""meta_analyzer's own failure still zeros meta_analysis_applied/llm_available.

This is the other half of the independent-contracts fix: the two fields
are not blind to meta_analyzer - they just ignore everyone ELSE.
"""
monkeypatch.setattr("skillspector.nodes.report.is_llm_available", lambda: (True, None))
state: SkillspectorState = {
"filtered_findings": [],
"component_metadata": [],
"has_executable_scripts": False,
"manifest": {},
"output_format": "json",
"use_llm": True,
"llm_call_log": [
llm_call_record("semantic_security_discovery", ok=True),
llm_call_record("meta_analyzer", ok=False, error="claude empty stdout"),
],
}
meta = _meta_from_json_report(state)
assert meta["meta_analysis_applied"] is False
assert meta["llm_available"] is False
assert meta["filtering_mode"] == "heuristic"


def test_report_not_degraded_when_no_llm_calls(monkeypatch: pytest.MonkeyPatch) -> None:
Expand Down Expand Up @@ -980,6 +1059,100 @@ 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_analyzer_partial_batch_failure_flows_through_to_report_degraded(
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""End-to-end: an analyzer-level partial batch failure reaches the report as degraded.

Regression for the reviewed gap: llm_call_log records were built with
``ok=bool(outcome.successful) or not outcome.failures``, so a batch
dropping while a sibling batch in the same analyzer succeeded still
recorded ok=True and the report never saw the coverage loss (the exact
two-file/one-429 case ``test_partial_batch_failure_records_llm_failure``
in test_semantic_developer_intent.py now pins). This drives the real
``semantic_developer_intent`` node through a mocked partial-batch outcome
and feeds its ACTUAL llm_call_log output into report(), rather than
hand-constructing the log the way the report-only tests above do.
"""
from unittest.mock import MagicMock, patch

from skillspector.llm_analyzer_base import (
BatchExecutionResult,
BatchFailure,
LLMAnalyzerBase,
)
from skillspector.nodes.analyzers.semantic_developer_intent import node as di_node

monkeypatch.setattr("skillspector.nodes.report.is_llm_available", lambda: (True, None))

def _mock_get_chat_model(*_args: object, **_kwargs: object) -> MagicMock:
mock_llm = MagicMock()
mock_llm.with_structured_output.return_value = MagicMock()
return mock_llm

async def partially_succeeds(self: LLMAnalyzerBase, batches: list, **_kwargs: object) -> list:
successful = [(batches[0], [])]
self._last_batch_outcome = BatchExecutionResult(
successful=successful,
failures=[BatchFailure(batches[1], "TimeoutError")],
)
return successful

with (
patch("skillspector.llm_analyzer_base.get_chat_model", _mock_get_chat_model),
patch.object(LLMAnalyzerBase, "arun_batches", partially_succeeds),
):
analyzer_result = di_node({"file_cache": {"first.py": "print(1)", "second.py": "print(2)"}})

# The analyzer's own record reflects the dropped batch...
assert analyzer_result["llm_call_log"] == [
{"node": "semantic_developer_intent", "ok": False, "error": None}
]

state: SkillspectorState = {
"filtered_findings": [],
"component_metadata": [],
"has_executable_scripts": False,
"manifest": {},
"output_format": "json",
"use_llm": True,
"llm_call_log": analyzer_result["llm_call_log"],
}
result = report(state)
meta = json.loads(result["report_body"])["metadata"]

# ...and the report-level verdict reflects it too: never a plain SAFE on
# a multi-batch analyzer that silently lost coverage.
assert meta["llm_degraded"] is True
assert result["risk_recommendation"] == "CAUTION"


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