Repository navigation
fix(agent): keep one cause one streak class in the media envelopes - #502
Conversation
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: batched media HTTP 403 failures lose their proxy recovery hint.
I found one inline blocker. The stable error/detail split otherwise preserves the intended streak behavior and keeps diagnostics model-visible.
Covered: the full diff; surrounding envelope consumers and the agent-loop caller; relevant history; backward compatibility of the JSON envelopes; whether tests were weakened; and the AGENTS.md, CLAUDE.md, CONTEXT-MAP.md, and Runtime architecture constraints.
Verification: the affected suite passed with 307 tests and 17 warnings. The direct batch-reducer reproduction confirmed the missing hint. The source-language gate passed. The full suite did not pass in this checkout: 18,312 passed, 126 skipped, 67 failed, and 235 errored; the output includes missing optional/dev components such as raven_ppt, raven_design, raven_everos, and websockets.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the batch reducer still drops the HTTP 403 recovery hint.
The new revision adds useful coverage for the images, speech, and video routes, but it does not change raven/agent/tools/media_gen.py:609: batch results still omit the hint field introduced by _format_http_error. The existing inline finding remains current, so I have not duplicated it or resolved its thread.
I reviewed the new delta and rechecked the full branch diff, affected callers, relevant history, backward compatibility, test strength, and the repository and Runtime architecture constraints. I found no additional issues.
Verification: uv run pytest tests/test_media_gen_tool.py -q passed with 95 tests. uv run python scripts/check_source_language.py github/main...HEAD passed. Current GitHub checks are otherwise green or still in progress.
failure_class keys the loop's tool-failure streak on an envelope's "error" string alone, which holds only while that string comes from the small fixed vocabulary its producers are meant to keep. The media tools interpolated the vendor body, the unreadable reference path and the exception's own text into it, and the URL gate in front of both copies of the reader interpolated the address it refused. One dead endpoint, one bad reference or one blocked range therefore produced a class per call, the streak never reached the break threshold, and the stop-repeating nudge was unreachable for every tool here. "error" now carries only what a reader could enumerate; the variable part moves to "detail", which keeps it in front of the model and inside is_hard_tool_failure's transient-marker scan. A batch result forwards "detail" so a per-picture reason is not lost with it. Distinct causes stay distinct, which is the failure mode this trades against: two HTTP statuses, two exception types and the four vendor refusals each keep their own class. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
…r takes The diff-coverage gate named four lines: a dedicated image model goes to the Images API and never touches chat/completions, so that route carries its own copies of both handlers, and speech and video each carry their own catch-all. The streak key is (tool, failure_class), so each of those tools accumulates its own streak and each one could lose the nudge on its own. Each new test makes the same claim as the ones it joins rather than only walking the line: two failures from one cause are one class, and the host or path that varies is still in front of the model under "detail". Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Splitting "error" into a class plus the variable part gave the single call two new keys: "hint", where a 403 names the proxy the media calls can be routed through, and "url", where a guarded fetch names the reference it refused. The batch reducer was taught to forward "detail" and not those two, so the same 403 answered a batched picture with the status and the vendor body alone while the single call still told the operator how to get past it, and a refused reference reached the model with nothing saying which address was refused. Both keys now travel with "error" out of the fold. The batched picture is told what the single call is told, which is the property the split is supposed to preserve. Each test pins one key: reverting the fold to "detail" alone turns both red, restoring "hint" alone leaves the reference test red, and restoring "url" alone leaves the 403 test red. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
ca1afc9 to
b24eb75
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The revised batch fold preserves every field split out of error by this change: detail, the 403 recovery hint, and the refused-reference url. The new tests cover the real batched call paths, and my prior blocker is fixed and resolved. I found no new issues.
Covered: the current full diff and revision delta; media-envelope producers, the batch consumer, and failure-streak caller; relevant history; backward compatibility; test strength; and the AGENTS.md, CLAUDE.md, CONTEXT-MAP.md, and Runtime architecture constraints.
Verification: uv run pytest tests/test_agent_loop_tool_loop_break.py tests/test_agent_tools_web_providers.py tests/test_agents_research_tools.py tests/test_media_gen_tool.py tests/test_security_web_ssrf.py -q passed with 312 tests and 17 warnings. uv run python scripts/check_source_language.py github/main...HEAD passed. GitHub checks are green or still in progress.
|
Not a blocker -- the model-facing envelope is unaffected. This is about the row that lands on disk.
Measured at the seam, handing the real Same function, same call; only the envelope differs. The row is written on the production path, not a window built for the probe: Why this is more than cosmetic: Scope, in fairness to the change: the sibling handlers at 1852 and 1865 split the same way before One entry in that tuple closes it: |
|
No blockers; suggestions only, and they are marked inline. Confirmed at the production seam: Verification: the direct |
) ## Summary `failure_class` keys the loop's tool-failure streak on an envelope's `error` string alone. That works only while the string comes from the small fixed vocabulary its producers are meant to keep, and it is a convention the module cannot enforce - the docstring added in #410 names exactly this failure mode: one cause counting as many. Every media tool broke it. `_format_http_error` interpolated up to 400 characters of vendor body (which carries a request id) into `error`; the unreadable-reference handlers interpolated the path; the four catch-alls put `str(e)` there, which spells the host. So a dead endpoint produced a class per call, `(tool, failure_class)` never repeated, the streak never reached `_LOOP_BREAK_THRESHOLD`, and the stop-repeating nudge was unreachable for `image_generate`, `text_to_speech` and `video_generate` alike. The URL gate in front of `web_fetch` had it too, in both copies of the tool: most reasons `validate_url_target` composes name the address they refused, so a model walking an internal range got one class per address. #410 fixed the handlers behind that gate and left the gate itself. `error` now carries only what a reader could enumerate. The variable part moves to `detail`, which keeps it in front of the model and inside `is_hard_tool_failure`'s transient-marker scan. Where the split gave the single call a further key, the batch reducer forwards that too: `hint`, which names the proxy a 403 can be routed through, and `url`, which names the reference a guarded fetch refused. A batched picture is told what the single call is told, which is the property the split has to preserve. Distinct causes stay distinct, which is the failure mode this trades against: two HTTP statuses, two exception types and the four vendor refusals each keep their own class, and the tests assert that direction too. Found by sweeping for the shape rather than by reading the code: every `"error":` whose value is not a literal is a candidate, and that grep is the cheapest guard against this recurring. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification - `uv run pytest tests/ -q -p no:randomly` -> 23443 passed, 109 skipped - `make lint-python` -> ruff check and format clean - `make lint-types` -> all checks passed - `make lint-imports` -> 10 contracts kept, 0 broken - `make check-source-language`, `make check-large-files` -> clean - Revert-to-red, per fix, restoring the source from a backup afterwards: reverting `media_gen.py` alone turns the four new class tests red and leaves `test_two_different_statuses_stay_two_classes` green; reverting either copy of `web.py` turns only that copy's gate test red and leaves the two #410 streak tests green. For the batch fold, each key is pinned on its own: reverting it to `detail` alone turns both new batch tests red, restoring `hint` alone leaves the reference test red, restoring `url` alone leaves the 403 test red. - After rebase onto 45203a4: `uv run pytest tests/test_media_gen_tool.py tests/test_security_web_ssrf.py -q -p no:randomly` -> 100 passed; `make lint-python`, `make lint-types`, `make check-source-language`, `make check-large-files` -> clean. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed ## Risk Tool output shape changes: a caller reading the reason out of `error` now finds it in `detail`. The readers in this repo are the model itself and the tests updated here; `tests/test_security_web_ssrf.py` is the one behavioural assertion that moved, and it still pins that a private address is refused for being private. Rollback is a plain revert of this commit - no state, no migration, no config. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues N/A. Finishes the envelope work started in #410, which fixed the reader's handlers and left the gate in front of them and every media tool. --------- Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Summary
failure_classkeys the loop's tool-failure streak on an envelope'serrorstring alone. That works only while the string comes from the small fixed
vocabulary its producers are meant to keep, and it is a convention the module
cannot enforce - the docstring added in #410 names exactly this failure mode:
one cause counting as many.
Every media tool broke it.
_format_http_errorinterpolated up to 400characters of vendor body (which carries a request id) into
error; theunreadable-reference handlers interpolated the path; the four catch-alls put
str(e)there, which spells the host. So a dead endpoint produced a class percall,
(tool, failure_class)never repeated, the streak never reached_LOOP_BREAK_THRESHOLD, and the stop-repeating nudge was unreachable forimage_generate,text_to_speechandvideo_generatealike.The URL gate in front of
web_fetchhad it too, in both copies of the tool:most reasons
validate_url_targetcomposes name the address they refused, soa model walking an internal range got one class per address. #410 fixed the
handlers behind that gate and left the gate itself.
errornow carries only what a reader could enumerate. The variable part movesto
detail, which keeps it in front of the model and insideis_hard_tool_failure's transient-marker scan. Where the split gave the singlecall a further key, the batch reducer forwards that too:
hint, which names theproxy a 403 can be routed through, and
url, which names the reference aguarded fetch refused. A batched picture is told what the single call is told,
which is the property the split has to preserve. Distinct causes stay distinct,
which is the failure mode this trades against: two HTTP statuses, two exception
types and the four vendor refusals each keep their own class, and the tests
assert that direction too.
Found by sweeping for the shape rather than by reading the code: every
"error":whose value is not a literal is a candidate, and that grep is thecheapest guard against this recurring.
Type
Verification
uv run pytest tests/ -q -p no:randomly-> 23443 passed, 109 skippedmake lint-python-> ruff check and format cleanmake lint-types-> all checks passedmake lint-imports-> 10 contracts kept, 0 brokenmake check-source-language,make check-large-files-> cleanRevert-to-red, per fix, restoring the source from a backup afterwards:
reverting
media_gen.pyalone turns the four new class tests red and leavestest_two_different_statuses_stay_two_classesgreen; reverting either copyof
web.pyturns only that copy's gate test red and leaves the two feat(*): dead-end predicates, verbatim tool bodies and envelope failures #410streak tests green. For the batch fold, each key is pinned on its own:
reverting it to
detailalone turns both new batch tests red, restoringhintalone leaves the reference test red, restoringurlalone leaves the403 test red.
After rebase onto 45203a4:
uv run pytest tests/test_media_gen_tool.py tests/test_security_web_ssrf.py -q -p no:randomly-> 100 passed;make lint-python,make lint-types,make check-source-language,make check-large-files-> clean.Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
Tool output shape changes: a caller reading the reason out of
errornowfinds it in
detail. The readers in this repo are the model itself and thetests updated here;
tests/test_security_web_ssrf.pyis the one behaviouralassertion that moved, and it still pins that a private address is refused for
being private. Rollback is a plain revert of this commit - no state, no
migration, no config.
Related Issues
N/A. Finishes the envelope work started in #410, which fixed the reader's
handlers and left the gate in front of them and every media tool.