Skip to content

Fix uncaught pydantic ValidationError crash on malformed LLM response #255

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Ruanyuxi1337 wants to merge 1 commit into
base: main
Choose a base branch
from
+33 −6
Open

Fix uncaught pydantic ValidationError crash on malformed LLM response #255

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
39 changes: 33 additions & 6 deletions src/skillspector/llm_analyzer_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,12 +428,39 @@ async def _process(batch: Batch) -> tuple[Batch, list]:
estimate_tokens(prompt),

@rng1995 rng1995 Jul 9, 2026

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.

Blocking: only the async path is wrapped. The synchronous run_batches() path used by semantic security discovery remains fail-fast, so a malformed middle response discards earlier successes. Apply equivalent per-batch containment to both paths and test it.

len(batch.findings),
)
if self._structured_llm:
response = await self._structured_llm.ainvoke(prompt)
else:
response = _message_text(await self._llm.ainvoke(prompt))
logger.debug("LLM response for %s", batch.file_label)
return (batch, self.parse_response(response, batch))
try:
if self._structured_llm:
response = await self._structured_llm.ainvoke(prompt)
else:
response = _message_text(await self._llm.ainvoke(prompt))
logger.debug("LLM response for %s", batch.file_label)
parsed = self.parse_response(response, batch)
return (batch, parsed)
except NotImplementedError as exc:
# Do not retry on configuration / code errors
raise exc
except Exception as exc:

@keshprad keshprad Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This broad catch changes the documented ValueError contract. If the first call raises an ordinary configuration ValueError (for example, a missing API key) and the retry succeeds, arun_batches() returns successfully instead of propagating the configuration error. Please catch Pydantic ValidationError explicitly for retry and let non-Pydantic ValueError bypass retry.

@rng1995 rng1995 Jul 9, 2026

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.

Blocking: broad except Exception retries ordinary configuration and programming failures, including unrelated ValueErrors. Catch the intended transient/structured-parse exceptions explicitly so real misconfiguration still propagates immediately.

logger.warning(
"LLM call/parse failed for %s: %s (attempting retry)",
batch.file_label,
exc,
)
# Retry once before throwing
try:
if self._structured_llm:
response = await self._structured_llm.ainvoke(prompt)
else:
response = _message_text(await self._llm.ainvoke(prompt))
logger.debug("LLM response for %s on retry", batch.file_label)
parsed = self.parse_response(response, batch)
return (batch, parsed)
except Exception as retry_exc:
logger.warning(
"LLM call/parse retry failed for %s: %s",
batch.file_label,
retry_exc,
)
raise retry_exc

@keshprad keshprad Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A persistent malformed response still reproduces #250 here. Pydantic ValidationError subclasses ValueError, so after the retry re-raises it, the aggregation loop matches it as ValueError and aborts the entire scan. Please handle exhausted Pydantic validation failures as an isolated batch failure while preserving propagation for ordinary configuration ValueError, and add a regression where both attempts are malformed.

@rng1995 rng1995 Jul 9, 2026

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.

Blocking: after retries are exhausted this re-raises. Pydantic ValidationError is a ValueError, so result aggregation can still abort the whole scan instead of containing only the malformed batch. Return/record an isolated batch failure and add a persistent-malformed-response regression.


results = await asyncio.gather(*[_process(b) for b in batches], return_exceptions=True)
successful: list[tuple[Batch, list]] = []
Expand Down