fix(agent): bound multimodal outer-loop retries

[stigrunar]

Problem

A Codex/vision worker can preserve assistant content as OpenAI-style typed parts. During incomplete-response deduplication, _interim_assistant_visible_text() passed that list to _strip_think_blocks() / re.sub, raising:

TypeError: expected string or bytes-like object, got 'list'\n```\n\nThe broad outer conversation-loop retry then requested the same response again. Because the failure was deterministic local post-processing rather than a transient provider error, it could consume the full iteration budget while a headless worker continued to heartbeat without producing a verdict.\n\n## Fix\n\n- Flatten non-string assistant content with the existing multimodal-safe summarizer before think-block scrubbing. The original provider-shaped message is not mutated.\n- Add a circuit breaker for identical outer-loop failures: after three identical exception type/message pairs, stop the turn with `failed=True` and a diagnostic exit reason instead of spending the remaining provider-call budget.\n- Preserve the existing near-max-iterations behavior for non-repeated failures.\n\n## Verification\n\n- Regression: typed assistant content is flattened safely and remains unmodified.\n- Regression: a deterministic local post-processing error stops after 3 calls even when `max_iterations=80`.\n- `pytest -q tests/run_agent/test_run_agent.py tests/run_agent/test_outer_loop_circuit_breaker.py`\n- Result: **434 passed**\n

[alt-glitch]

This was generated by AI during triage.

Re-triage correction: the recut no longer changes typed-content coercion or adds a three-strike circuit breaker. It extracts the existing traceback local-processing classifier into a testable helper and adds focused coverage. Related to merged #66567 and open broader lifecycle work #67411; a maintainer should consolidate the remaining test/refactor scope.

[teknium1]

Thanks for isolating the retry-budget failure mode. The member comment correctly links this work to #66267.

Problems

• Current main already fixes the reported list-to-regex crash at the shared choke point: agent/agent_runtime_helpers.py:675-707 coerces list/dict content before regex processing and explicitly covers _interim_assistant_visible_text; tests/run_agent/test_run_agent.py:448-480 covers the regression (commit 296494db0). The run_agent.py hunk is therefore redundant.
• The proposed three-strike key at agent/conversation_loop.py:5616 is only exception type/message. It does not establish that a matching error is local processing, although the terminal response labels it an internal error.

Suggested changes

• If this is salvaged, retain only the independently useful retry-policy change and remove the now-duplicated typed-content hunk/tests.
• Scope the cap to a verified deterministic local-processing class, or add coverage for the intended behavior across repeated outer-handler failures.

Automated hermes-sweeper review.

[stigrunar]

Resolved in e460f71.

I recut the branch onto current upstream main (614dc19), removed the redundant run_agent.py/test_run_agent.py typed-content changes, and dropped the old type/message three-strike fingerprint. The remaining diff extracts current main's traceback-module classifier into a testable helper and adds focused production-loop coverage proving: allowlisted local-only tracebacks stop after one provider response; API/shared-module tracebacks and same-type/message nonlocal failures stay on the normal bounded retry path.

Verification:

• scripts/run_tests.sh tests/run_agent/test_outer_loop_circuit_breaker.py — 5 passed
• scripts/run_tests.sh tests/run_agent/test_66267_multimodal_interim.py — 8 passed
• python -m py_compile agent/conversation_loop.py tests/run_agent/test_outer_loop_circuit_breaker.py — passed
• ruff check agent/conversation_loop.py tests/run_agent/test_outer_loop_circuit_breaker.py — passed
• git diff --check — passed

[stigrunar]

The recut at e460f71 is ready for upstream re-review. GitHub reviewer-request mutation is unavailable to this fork author/token, so this comment is the review request receipt.

[hydraxman]

I re-reviewed the recut at e460f719577ef860d9a85037498c6f7e3d72e165. The earlier concern about a three-strike key based only on exception type/message no longer applies: that implementation is gone. The remaining diff cleanly extracts the current-main traceback classifier and gives it focused local-vs-API coverage.

The main question now is consolidation. #67411 retains the traceback classifier only as pre-terminal defense-in-depth and replaces traceback guessing with an attempt-scoped provider lifecycle after a terminal response. To avoid maintaining two competing retry-policy PRs, I suggest either:

• moving/cherry-picking the useful classifier unit tests from this PR into fix(agent): scope provider terminal lifecycle per attempt #67411; or
• documenting a narrow residual value for this PR that remains independent after fix(agent): scope provider terminal lifecycle per attempt #67411 lands.

If these integration tests are retained, one additional invariant is worth asserting: the terminal local-error path must not persist adjacent assistant messages, leave unanswered tool calls, or echo raw exception strings into the user-visible response/session. I posted the concrete locations and test suggestions on #67411 here: #67411 (comment)

So: the recut addresses the previous review correctly; my recommendation is consolidation rather than another parallel retry policy.

[stigrunar]

Rebased the focused salvage onto current upstream main (ac327dfa38f6418619e9d5aa72fded4319e98661) at 09b1f5547d201bc55156b08663b955966fa60d2b.

This keeps the earlier review repair intact: the redundant typed-content change and type/message fingerprint remain removed; the remaining production seam classifies only allowlisted local-processing tracebacks without API-call frames, with repeated nonlocal failures retaining the normal bounded retry path.

Verified on the pushed head:

• scripts/run_tests.sh tests/run_agent/test_outer_loop_circuit_breaker.py tests/run_agent/test_66267_multimodal_interim.py — 13 passed
• python -m py_compile agent/conversation_loop.py tests/run_agent/test_outer_loop_circuit_breaker.py — passed
• ruff check agent/conversation_loop.py tests/run_agent/test_outer_loop_circuit_breaker.py — passed
• git diff --check upstream/main...HEAD — passed

The PR remains open and unmerged; this exact head is ready for upstream re-review.

[stigrunar]

Upstream re-review requested for exact head 09b1f5547d201bc55156b08663b955966fa60d2b. The fork-author token cannot mutate this upstream repository’s requested-reviewers endpoint (HTTP 404), so this comment is the re-review request receipt.

[stigrunar]

Upstream re-review requested for exact head 09b1f5547d201bc55156b08663b955966fa60d2b.

The actionable findings from review 4729422944 are already repaired on this head: the redundant typed-content hunk/tests are absent, the exception type/message fingerprint is gone, and the remaining cap applies only when the traceback reaches an allowlisted local-processing helper without an API-call helper. Focused outer-loop coverage also proves matching nonlocal failures retain the normal bounded retry path.

Fresh verification on this exact head:

• scripts/run_tests.sh tests/run_agent/test_outer_loop_circuit_breaker.py — 5 passed
• scripts/run_tests.sh tests/run_agent/test_66267_multimodal_interim.py — 8 passed
• python -m py_compile agent/conversation_loop.py tests/run_agent/test_outer_loop_circuit_breaker.py — passed
• ruff check agent/conversation_loop.py tests/run_agent/test_outer_loop_circuit_breaker.py — passed
• git diff --check origin/main...HEAD — passed

@teknium1 please re-review the current exact head when convenient.

[stigrunar]

Upstream re-review requested for exact head e83e96c54590e24bf96c220d44439ac157f81e0d.

This focused follow-up keeps the local-processing diagnostic in logs while preventing raw exception details from entering the user-visible response, turn-exit metadata, or synthetic tool-result session rows. The regression test now also asserts a generic terminal response and strict user/assistant role alternation.

Fresh verification on this exact commit:

• scripts/run_tests.sh tests/run_agent/test_outer_loop_circuit_breaker.py tests/run_agent/test_66267_multimodal_interim.py — 13 passed
• ruff check agent/conversation_loop.py tests/run_agent/test_outer_loop_circuit_breaker.py — passed
• python -m py_compile agent/conversation_loop.py tests/run_agent/test_outer_loop_circuit_breaker.py — passed
• git diff --check HEAD^..HEAD — passed

The requested-reviewers endpoint remains unavailable to this fork-author token (HTTP 404), so this comment is the exact-head re-review request receipt. @teknium1 please re-review when convenient.

[stigrunar]

Upstream re-review requested for exact head c2ea66bb766aa1c203a5b2a8c221397cdfef9fbf.

Rebased the focused salvage onto current upstream main (7f4d155159e2a5d4098bb2f27d3fccb01ff84c3d) without widening scope. The blocking findings from review 4729422944 remain repaired: the superseded typed-content hunk/tests and type/message fingerprint are absent; only allowlisted local-processing tracebacks without API-call frames terminate immediately, while identical nonlocal failures retain the normal bounded retry path.

Fresh verification on this exact head:

• scripts/run_tests.sh tests/run_agent/test_outer_loop_circuit_breaker.py tests/run_agent/test_66267_multimodal_interim.py -q — 10 passed
• python -m py_compile agent/conversation_loop.py tests/run_agent/test_outer_loop_circuit_breaker.py — passed
• ruff check agent/conversation_loop.py tests/run_agent/test_outer_loop_circuit_breaker.py — passed
• git diff --check origin/main...HEAD — passed

The pre-rebase head is preserved at stigrunar:backup/pr-66510-e83e96c5. The fork-author token cannot use the upstream requested-reviewers endpoint, so this comment is the established re-review request receipt. @teknium1 please re-review when convenient.

[GottZ]

This was generated by AI during triage.

Summary

Two open PRs address repeated provider requests after local response-processing failures at different scopes: #66510 provides a focused traceback-based local-error classifier, while #67411 introduces an attempt-scoped provider-terminal lifecycle across multiple transports. The former stops allowlisted local failures without changing provider-shaped content; the latter prevents post-terminal retry, fallback, and continuation while hardening transcript finalization and error surfaces.

Related pull requests

• fix(agent): bound multimodal outer-loop retries #66510 related — (+194/-17) — n/a: Extracts the existing traceback classifier into _is_local_processing_error, immediately stops allowlisted local-processing failures, preserves bounded retries for nonlocal failures, and adds focused tests. Its current diff addresses the keep_open review by removing the redundant typed-content change and type/message fingerprint, but remains narrower than fix(agent): scope provider terminal lifecycle per attempt #67411 and still prints error_msg, including str(e), to the CLI even though the final response, exit reason, and session/tool rows are generic.
• fix(agent): scope provider terminal lifecycle per attempt #67411 related — (+3137/-38) — n/a: Adds per-attempt lifecycle tokens and explicit terminal markers across chat-completions, Anthropic, Codex, Bedrock, and MoA paths, preventing post-terminal failures from issuing another provider request while preserving pre-terminal retries. Its diff addresses the keep_open review's blocking Codex-watchdog finding by updating the affected fakes to accept attempt=None, and it adds role-safe generic failure finalization plus stale-worker and provider-path coverage.

Duplicates

#66510 and #67411 overlap on stopping retries after local response-processing failures, but are not exact duplicates: #66510 isolates the traceback fallback, whereas #67411 establishes the broader attempt-scoped lifecycle boundary and retains traceback classification as defense in depth.

Suggested consolidation

Keep #67411 open with a salvage path centered on the attempt-scoped lifecycle token, transport terminal markers, stale-worker isolation, and role-safe generic failure finalization. Keep #66510 open only for its focused classifier extraction and local-versus-nonlocal traceback tests; if #67411 absorbs that remaining scope, close #66510 as a duplicate of #67411, otherwise retain only the non-overlapping fallback-classifier work and fix its remaining CLI exception-detail exposure.

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 155 kB of PR diffs, 9 kB of issue/PR text, 12 kB of discussion (16 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[stigrunar]

Superseded by the broader attempt-scoped provider lifecycle in #67411, not rejected. The remaining useful scope from this PR — local-vs-nonlocal traceback classification, bounded fallback behavior, role-safe terminal persistence, and generic user-visible error surfaces — is already represented in #67411 and its focused lifecycle/classifier coverage. Keeping both open would maintain competing retry-policy paths. Closing this narrower PR so future work follows the authoritative #67411 exact-head lane.