fix(agent): walk error cause chain in image-shrink recovery helper (follow-up to #53582) - #53884
fix(agent): walk error cause chain in image-shrink recovery helper (follow-up to #53582)#53884r266-tech wants to merge 1 commit into
Conversation
Follow-up to NousResearch#53582. That PR taught error_classifier to walk the __cause__/__context__ chain, so a *wrapped* image-dimension 400 now classifies as FailoverReason.image_too_large. But the recovery-side helper _image_error_max_dimension still inspected only the top-level exception, so on a wrapped error it returned None and the caller fell back to an 8000px cap — the image was shrunk to the wrong ceiling and the provider re-rejected it, the exact brick NousResearch#53582 set out to fix. Walk the same cause chain (depth-bounded to 5, breaking on None/self-cycle, mirroring error_classifier._extract_status_code/_extract_error_body). Each exception in the chain is evaluated independently: the trigger terms and the pixel ceiling must come from the same error, so a generic wrapper plus an unrelated cause can't be concatenated into a spurious match. Unwrapped errors behave exactly as before. Adds regression tests for the wrapped __cause__ path, an implicit __context__ chain, and the split-across-levels non-match.
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Thorough fix for image-shrink recovery not finding pixel ceilings on wrapped errors (#53582). The _image_error_max_dimension helper now walks the __cause__/__context__ chain, matching the same traversal that classify_api_error already uses.
Key design points:
- Each exception in the chain is evaluated independently — trigger terms and pixel ceiling must come from the same error
- Depth bound of 5 matches existing
_extract_status_code()traversal - Cycle guard (
cause is current) prevents infinite loops
3 dedicated tests: wrapped error cause chain, deep context chain, cross-chain trigger term isolation.
No concerns. Ready to merge.
Reviewed by Hermes Agent
|
Thanks for the focused follow-up. The current-main gap is confirmed: The proposed five-level Automated hermes-sweeper review. |
Follow-up to #53582.
The gap #53582 left
#53582 made
error_classifierwalk the__cause__/__context__chain, so a wrapped image-dimension 400 (the common case when a provider/gateway re-wraps the SDK error) now correctly classifies asFailoverReason.image_too_largeand enters image-shrink recovery.But the recovery-side helper that decides how small to shrink —
_image_error_max_dimensioninconversation_loop.py— still read only the top-level exception:On a wrapped error the provider's
max allowed size: N pixelslives on__cause__, so the helper returnedNone, and the caller falls back to an 8000px cap:So the recovery #53582 just enabled fires, but shrinks the image to the wrong ceiling, the provider re-rejects it, and the session bricks — arguably worse than before #53582, because recovery now triggers but still fails.
Fix
Walk the same cause chain in
_image_error_max_dimension, depth-bounded to 5 and breaking onNone/self-cycle, mirroringerror_classifier._extract_status_code/_extract_error_body. Each exception in the chain is evaluated independently — the trigger terms (image+dimension+max allowed size) and the pixel ceiling must come from the same error, so a generic wrapper plus an unrelated cause can't be concatenated into a spurious match. Unwrapped errors behave exactly as before.Scope is intentionally just this one recovery helper, so it reads as completing #53582's chain-walk on the recovery side. The sibling image-rejection matcher and
retry_utils._error_textare left untouched as separate follow-ups.Tests
tests/run_agent/test_image_shrink_recovery.py::TestImageTooLargeClassificationpasses, with new cases for the wrapped__cause__path, an implicit__context__chain, and a split-across-levels non-match (terms spread across different exceptions must not synthesize a false ceiling).If this has already been handled via a separate patch, feel free to close — happy to defer.