fix(agent): propagate OutputCheckError to caller#7418
fix(agent): propagate OutputCheckError to caller#7418Bahtya wants to merge 4 commits intoagno-agi:mainfrom
Conversation
OutputCheckError raised in output_validation_post_hook was caught internally and logged as ERROR, but never propagated to the caller. This made the try/except OutputCheckError pattern shown in the docs ineffective. The exception was caught in 8 locations across _run.py (4 sync, 4 async), where cleanup and logging was performed before silently returning the run_response. Now the exception is re-raised after cleanup so callers can handle it as documented. Fixes agno-agi#7414 Signed-off-by: bahtya <bahtyar153@qq.com>
PR TriageMissing tests: This PR modifies source code but does not include any test changes. Please add or update tests to cover your changes. |
Adds regression tests for agno-agi#7414 verifying that OutputCheckError raised in post-hooks propagates to the caller for both sync and async agent runs. Signed-off-by: bahtya <bahtyar153@qq.com>
|
Hi team, just wanted to follow up on this PR. Would appreciate any feedback! |
|
Hi team, following up on this PR. The bot flagged missing tests — I'd be happy to add tests if the maintainers think the approach is correct. Would appreciate initial feedback on the direction before I add test coverage. Thanks! |
Remove unreachable `raise` after `break` in streaming functions. Update existing tests to expect propagated exceptions instead of captured-in-response behavior, matching the new error propagation. Bahtya
|
Hi, following up on the bot's test flag. Tests were already added in commit
Summary of test coverage:
|
VANDRANKI
left a comment
There was a problem hiding this comment.
The fix is correct for the four non-streaming paths (_run, _arun, _continue_run, _acontinue_run). return run_response silently hid the exception from the caller; raise is the right replacement.
PR #7632 covers the same bug more completely
PR #7632 (already reviewed) applies the identical fix to all 8 exception-handler sites: the 4 non-streaming paths this PR covers plus the 4 streaming variants (_run_stream, _arun_stream, _continue_run_stream, _acontinue_run_stream), which also swallowed exceptions via break. The two PRs conflict on the same lines in _run.py and cannot both be merged.
The integration tests in this PR are stronger
The tests in test_hooks.py drive the fix through the full agent.run() call and assert that InputCheckError/OutputCheckError actually propagates to the caller. PR #7632's tests only exercise execute_pre_hooks at the hook layer, which was already re-raising before either PR. The integration tests here are what's actually needed to verify the _run.py change.
A merged solution should combine #7632's code changes (all 8 paths) with this PR's integration tests.
|
Thanks for the thorough review @VANDRANKI. I agree that the streaming paths (4 locations where the catch block does For the streaming generators, simply replacing I can extend this PR to cover all 8 paths and add corresponding streaming tests, or we can coordinate with #7632 as you suggested. What's your preference? If combining: I'll add the 4 streaming |
|
@VANDRANKI Thanks for the thorough review! I agree that extending the fix to cover the 4 streaming paths ( |
Change break to raise in _run_stream, _arun_stream, _continue_run_stream, and _acontinue_run_stream so InputCheckError/OutputCheckError propagate to callers instead of being silently swallowed. Add streaming integration tests. Bahtya
VANDRANKI
left a comment
There was a problem hiding this comment.
The extended fix is correct and complete. All 8 paths are now covered with a single consistent change in each.
_run / _arun / _continue_run / _acontinue_run (non-streaming, 4 paths): return run_response -> raise. Clean. The partially-built response is discarded and the exception reaches the caller directly.
_run_stream / _arun_stream / _continue_run_stream / _acontinue_run_stream (streaming, 4 paths): break -> raise after the existing yield run_error. This is the right sequence: the error event is yielded to the caller first (preserving observability for any consumer reading the event stream), then on the caller's next next() call, execution resumes at raise inside the still-active except block, and the exception propagates out. Python's generator machinery correctly restores exception context after a yield inside an except block, so bare raise re-raises the original OutputCheckError / InputCheckError. No need to use raise e explicitly.
Tests:
- The 4 new streaming tests cover sync/async x input/output combinations correctly. The
for _ in agent.run(stream=True): passpattern is the right way to drive the generator to the point where the raise fires. - The new regression tests
test_output_check_error_propagates_to_callerandtest_output_check_error_propagates_to_caller_asynclock in the non-streaming contract and will catch any future reversion. - The existing tests are correctly updated: assertions against
result.status == RunStatus.errorare replaced withpytest.raises(...), which is the right contract now that exceptions propagate instead of being swallowed.
One gap: _continue_run_stream and _acontinue_run_stream are fixed in the code but have no dedicated tests. The fix is mechanically identical to the other streaming paths, so the risk is low — but a test that does a second agent.run() within an existing session (triggering the continue path) with a failing hook would close the loop completely. Not a blocker.
Coordination with #7632: as noted earlier, both PRs modify the same 8 locations in _run.py. They cannot both be merged. This PR now has stronger integration test coverage (streaming tests + existing test updates) so it is the stronger candidate. The maintainer should close #7632 in favor of this one, or cherry-pick its any-unique parts here.
LGTM. The PR is ready to merge as-is once the #7632 conflict is resolved.
Problem
When using
output_validation_post_hook,OutputCheckErroris caught internally and logged as ERROR, but never propagated to the caller. Thetry/except OutputCheckErrorpattern shown in the docs is ineffective.Root Cause
8 locations in
_run.pycatch(InputCheckError, OutputCheckError), perform cleanup/logging, thenreturn run_response(sync) oryield run_error; break(streaming) without re-raising.Solution
Add
raiseafter cleanup in all 8 catch blocks so the exception propagates to the caller as documented.return run_responsewithraiseraiseafterbreakFixes #7414