fix(guardrails): distinguish execution failure from invalid verdict - #7156
fix(guardrails): distinguish execution failure from invalid verdict#7156xu-jia-ming wants to merge 1 commit into
Conversation
LLMGuardrail.__call__ caught every exception and returned (False, "Error while validating the task output: ..."), the same shape as a genuine guardrail violation. A provider outage, expired key or rate limit was reported to the caller as a verdict about the agent's output, so the caller burned guardrail retries on it and finally raised a validation failure that never happened. An LLM failure now raises GuardrailExecutionError, which propagates through process_guardrail (emitting the terminal completed event first, like a hook abort) and out of the task/agent retry loops: no retry is spent, no error text is appended to the conversation, and the surfaced error names the real cause. A guardrail that runs and judges the output invalid still returns (False, feedback). 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 (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughLLM guardrail execution failures now raise ChangesGuardrail execution error handling
Sequence Diagram(s)sequenceDiagram
participant LLMGuardrail
participant process_guardrail
participant GuardrailCaller
participant LLMGuardrailCompletedEvent
LLMGuardrail->>process_guardrail: raise GuardrailExecutionError
process_guardrail->>LLMGuardrailCompletedEvent: emit failed completion event
process_guardrail-->>GuardrailCaller: re-raise execution error
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change separates guardrail execution failures from genuine invalid-output verdicts, preventing outages or provider errors from consuming validation retries or entering agent feedback. 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 satisfy 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 |
Vidit-Ostwal
left a comment
There was a problem hiding this comment.
Thanks for this — the shape is right (raise GuardrailExecutionError instead of returning (False, …)).
#7151 already does that fix and has stronger coverage (scoped event handlers, passing-output control, and a test that the task does not spend guardrail_max_retries). Please follow along there.
Worth folding into #7151 if they take it: putting GuardrailExecutionError on guardrail_types.py and the "The LLM guardrail could not run:" message prefix.
|
Closing as a duplicate of #7151. Same fix; follow along there. |
Summary
LLMGuardrail.__call__returned(False, "Error while validating the task output: ...")for any exception, the exact shape of a genuine guardrail violationguardrail_max_retries) and finally raised "guardrail failed validation" for a failure that never judged anythingGuardrailExecutionError(new, increwai.utilities.guardrail_types), which propagates out of the retry loops without spending a retry or appending the error to the conversationprocess_guardrailemits the terminalLLMGuardrailCompletedEventfor it before re-raising, mirroring the existingHookAbortedhandling, so the started event is always closed(False, feedback)— that path is unchangedProblem
Before, an infrastructure error and a validation verdict were indistinguishable:
Both feed
GuardrailResult(success=False), soTask._process_output/Agent._process_kickoff_guardrailappend the text to the conversation, spend retries re-running the agent, and afterguardrail_max_retriesraise a validation failure naming feedback that was really an outage.Root cause
LLMGuardrail.__call__'s blanketexcept Exceptionmapped "couldn't check" onto "check says it's bad". Plain callable guardrails already propagate exceptions throughprocess_guardrail(which only catchesHookAborted);LLMGuardrailwas the odd one swallowing them.Changes
utilities/guardrail_types.py: newGuardrailExecutionErrordocumenting the contract ("the guardrail could not run; not a statement about the output")tasks/llm_guardrail.py: the blanket except now raisesGuardrailExecutionError("The LLM guardrail could not run: ...") from einstead of returning a false verdict;HookAbortedstill re-raises untouchedutilities/guardrail.py:process_guardrailcatchesGuardrailExecutionError, emits the terminal completed event (success=False, real error), re-raises; docstring updatedtests/test_task_guardrails.py:test_llm_guardrail_outage_raises_execution_error— outage raisesGuardrailExecutionErrorwith the cause chainedtest_llm_guardrail_violation_still_returns_verdict— a real violation still returns(False, feedback)test_process_guardrail_propagates_execution_error_with_terminal_event— re-raise plus terminal eventtest_guardrail_when_an_error_occurs(which pinned the old conflation) to expectGuardrailExecutionErrorTesting
uv run pytest lib/crewai/tests/test_task_guardrails.py -q— 25 passed (3 new + updated test included)GuardrailExecutionError) and pass at the PR tipuv run pytest lib/crewai/tests/hooks/— 41 passed (2 Windows-only teardown errors fromTemporaryDirectorySQLite file locks, also present on clean checkout)uv run ruff check/ruff format --check/uv run mypyon the changed files — cleanTests not run: the full
uv run pytest lib/crewai/tests/ -x -qsuite (left to CI per time); locally on Windows the repo's default--block-networkaddopt breaks asyncio's socketpair fallback, so async-event tests were run with-o addoptsoverriding it.Related issue
Fixes #7150