fix: re-raise LLM guardrail execution errors instead of failed validation - #7151
fix: re-raise LLM guardrail execution errors instead of failed validation#7151BetterAndBetterII wants to merge 7 commits into
Conversation
…tion Provider/infra exceptions from LLMGuardrail were returned as (False, error-string), so callers treated them as output violations and retried. Raise GuardrailExecutionError so retries are reserved for real validation failures. Fixes crewAIInc#7150
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughChangesGuardrail execution errors
Sequence Diagram(s)sequenceDiagram
participant LLMGuardrail
participant process_guardrail
participant LLMGuardrailCompletedEvent
participant TaskExecution
LLMGuardrail->>process_guardrail: raise GuardrailExecutionError
process_guardrail->>LLMGuardrailCompletedEvent: emit failed completion event
process_guardrail-->>TaskExecution: re-raise GuardrailExecutionError
TaskExecution-->>TaskExecution: abort without retry
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Guardrail provider and infrastructure failures now propagate as execution errors instead of being mistaken for output violations and consuming validation retries, while genuine violations retain their existing retry behavior. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes address issue
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for picking this up so fast. This is the shape I had in mind when I filed #7150, and of the two options in the issue this is the one I would have gone for. One thing I checked while writing the issue that might save a reviewer some time. There are two separate retry loops that call process_guardrail, not one:
Neither of them wraps the process_guardrail call in a try/except, so re-raising from utilities/guardrail.py should propagate cleanly out of both and I do not think task.py needs a change for this to work. Worth someone confirming On the breaking change question, which I would guess is the main hesitation here: a provider outage already ended in an exception before this PR. It just got there the slow way, after three full agent re-executions, and then raised I also ran your branch against the three cases from the issue, using a small script that pulls LLMGuardrail.call out of the tree with ast and calls it with a stub _validate_output, so it needs no provider key. Comparing d600027 BEFORE (v1.15.1) AFTER (d600027) So the outage case stops returning a verdict, and both controls are untouched, which was the thing I would have worried about in a change like this. Say if the script is useful for the tests or the review and I will post it. |
Vidit-Ostwal
left a comment
Vidit-Ostwal
left a comment
There was a problem hiding this comment.
Two small follow-ups — the raise path itself looks correct and I confirmed neither retry loop wraps process_guardrail in try/except.
|
|
||
|
|
||
| class GuardrailExecutionError(Exception): | ||
| """The guardrail could not run. Not a statement about the output.""" |
There was a problem hiding this comment.
GuardrailExecutionError is a general “couldn’t run” error, but it lives next to LLMGuardrail and process_guardrail has to import it from here. Better home is guardrail_types.py (or next to GuardrailResult) so the utilities layer does not depend on the LLM-guardrail module. The closed duplicate #7156 already put it there.
| task.execute_sync(agent=agent) | ||
|
|
||
| assert agent.execute_task.call_count == 1 | ||
| assert task.retry_count == 0 |
There was a problem hiding this comment.
This covers the task retry loop well (one execute_task, retry_count == 0). There are two other callers of process_guardrail with the same shape: Agent._process_kickoff_guardrail and lite_agent.py. Neither wraps the call in try/except, so the raise should already skip retry and conversation append — a short test on each would confirm that, which is the gap the issue author called out.
|
Hey @BetterAndBetterII let me know if you require any help. |
3 participants
Summary
LLMGuardrail.__call__caught every exception (afterHookAborted) and returned(False, "Error while validating the task output: …"). Callers treat(False, …)as “output violated the guardrail” and retry viaguardrail_max_retries, stuffing the provider error into the conversation.An infrastructure/LLM failure is not a statement about the agent’s output. This change raises a distinct
GuardrailExecutionErrorso provider/infra errors propagate instead of being retried as validation failures. Real(False, feedback)violations still retry as before.process_guardrailstill emits a completed event before re-raising, matching theHookAbortedpath.Changes
lib/crewai/src/crewai/tasks/llm_guardrail.py— addGuardrailExecutionError; raise it from__call__on non-HookAbortedexceptionslib/crewai/src/crewai/utilities/guardrail.py— catch/re-raiseGuardrailExecutionErrorafter emittingLLMGuardrailCompletedEventlib/crewai/tests/test_task_guardrails.py— TDD coverage that provider errors are not validation failures and do not consume retriesTest plan
uv run pytest lib/crewai/tests/test_task_guardrails.py lib/crewai/tests/hooks/test_hook_abort_propagation.py -k "guardrail" -n 0(30 passed)Fixes #7150