[cosminacho]

Add run_limits.max_turn_retries to bound retries of an entire agent communication. Leaving it unset preserves the existing error-category policy. Setting it to 0 makes one attempt; positive values cap retries without enabling non-retryable errors. The shared orchestrator applies the limit to single-shot and dialog turns, and retains failed partial records.

A task that must preserve its authored questions can now report a failed attempt without replaying that question. Agent/provider request retries remain separate. The retry executor also stops before logging or sleeping for an attempt the cap forbids.

The lockfile includes the same pyasn1 0.6.5 update as #246, which clears the existing OSV dependency gate on the base branch.

Validation:

• 146 focused and 415 expanded tests passed, including zero/default/positive limits, non-retryable errors, partial retention and dialog stopping.
• 741 architecture checks, Ruff/format/prose checks and Pyright passed (one existing warning).
• Full offline gate: 6,606 passed, 4 skipped, 93.24% coverage. Two Docker preflight tests failed identically on the pristine base because this host has no Docker CLI; no tests were loosened.
• Independent code review found no actionable issues. No live model calls.

[github-actions]

Claude finished @cosminacho's task in 1m 26s —— View job

---

Code Review in Progress

Todo List:

• Read .github/code_review.md for review standards
• Read CLAUDE.md for project conventions
• Analyze the full PR diff
• Read each changed file in full context
• Perform cross-file consistency checks
• Check for missing elements
• Provide comprehensive review feedback

Starting review now...

[uipreliga]

Review: feat: configure whole-turn agent retry limits

PR #247 by @cosminacho · codex/turn-retry-limit → main · OPEN · reviewed against d3e45b5

Change class: complex — changes the whole-turn retry control flow in execute_with_retry and adds a user-facing RunLimits schema field (max_turn_retries)

PR #247 is safe to merge: the new run_limits.max_turn_retries cap adds no new risk. Unset keeps the category policy, 0 stops replay as documented, the cap is checked before backoff, and it can never make a non-retryable error retryable. None of the seven Low findings can change a task's score or final_status for identical agent output. The remaining gaps are documentation and observability: the parity table, the recorded config and the terminal log can each give a reader a wrong picture of how many replays occurred.

Summary

Axis	Score	🔴	🟠	🟡	🔵	Top Issue
1. Code Quality & Style	9.9 / 10	0	0	0	1	The contract of the new field is restated in four places, and the notes paragraph is mostly contract, not rationale
2. Type Safety	10 / 10	0	0	0	0	—
3. Test Health	9.9 / 10	0	0	0	1	The changed retry-warning denominator (attempt N/min(policy, cap)) has no assertion
4. Security	10 / 10	0	0	0	0	—
5. Architecture & Design	9.9 / 10	0	0	0	1	HARNESS_PARITY row claims the same behavior on all harnesses but ignores Pi/Codex in-call replay
6. Error Handling & Resilience	9.8 / 10	0	0	0	2	Terminal retry log does not show that run_limits.max_turn_retries capped the attempts
7. API Surface & Maintainability	9.9 / 10	0	0	0	1	max_turn_retries gives 'turn' a different meaning from max_turns in RunLimits
8. Evaluation Harness Quality	9.9 / 10	0	0	0	1	A positive max_turn_retries above the category caps is silently accepted and runs exactly as if unset, but the recorded config still shows the number the author typed

Overall Score: 9.9 / 10 · Weakest Axis: Error Handling & Resilience at 9.8 / 10
Totals: 🔴 0 · 🟠 0 · 🟡 0 · 🔵 7 across 8 axes.

Blockers

None.

Non-blocking, but please consider before merge

None.

Nits

1. [Axis 1] The contract of the new field is restated in four places, and the notes paragraph is mostly contract, not rationale (.claude/notes/agents.md:95) — The same rules ('unset keeps the category policy', 'a positive value cannot make a non-retryable error retryable', 'does not cover agent/provider SDK retries') appear in four places. They are the Field description (limits.py:64-69), the guide table row (TASK_DEFINITION_GUIDE.md:276), a second guide paragraph after the table (TASK_DEFINITION_GUIDE.md:286-291, 'does not control retries inside an agent or provider SDK'), and the notes paragraph at agents.md:95-101 ('Unset preserves the existing error-category policy ... A larger cap cannot make a rejected error category retryable. Agent/provider internal retries and setup retries keep their own policies.'). CLAUDE.md asks for docs to be defined once in the Pydantic model, and says .claude/notes/ holds the why. In the notes, keep only the rationale (that replay changes the measured trajectory, and why the cap is checked before backoff and after the partial is drained) and drop the restated contract. Fold the extra guide paragraph's unique content (the -D example and 'exhausted limit stops the dialog') into the table row, or keep only that unique content. n/a
2. [Axis 3] The changed retry-warning denominator (attempt N/min(policy, cap)) has no assertion (src/coder_eval/errors/executor.py:109) — The PR changes the user-visible retry warning at executor.py:109-111 (attempt_limit = min(config.max_retries + 1, max_attempts) / f"... failed (attempt {attempt + 1}/{attempt_limit}): "), but no test checks it: grep -rn "Retrying in\|attempt_limit" tests returns nothing. The new tests (test_attempt_cap_has_no_terminal_backoff with max_attempts in [1, 2] on AgentCrashError, whose policy is 2 retries) assert only await counts. So if the min() is wrong (for example, it shows 3 when the cap is 2), no test fails. Add a caplog assertion to the max_attempts=2 case: assert that the record contains attempt 1/2. Also add a case where the cap is above the policy (for example, max_attempts=5 on AgentCrashError) and assert attempt 1/3, so both sides of the min() are covered at the executor level. This is only log text and does not change the score, so it is filed as Low.
3. [Axis 5] HARNESS_PARITY row claims the same behavior on all harnesses but ignores Pi/Codex in-call replay (docs/agents/HARNESS_PARITY.md:18) — Line 18 reads | run_limits.max_turn_retries | orchestrator-level cap on whole-turn retries | ... | with the same cell for claude-code, codex, antigravity, opencode, pi and delegate. CLAUDE.md requires that a shared config field means the same thing on every backend, or that the divergence is documented. The notes give the purpose of the knob (.claude/notes/agents.md:96): Benchmarks can set it to zero when replay would change the measured trajectory. Two harnesses do not meet that purpose. Pi 'retries a transient/provider error internally (another agent_start cycle in the same invocation, flagged willRetry: true)' (HARNESS_PARITY.md:768-771; pi_agent.py:10-13). Codex also 'retries a stalled or failed provider stream internally' (models/telemetry.py:121-122). With max_turn_retries: 0, both harnesses can still replay part of the trajectory inside one communicate(). TASK_DEFINITION_GUIDE.md:286-287 has a general disclaimer, but the parity table is the contract surface, and this row does not say that. Fix: keep the orchestrator-level wording, and add to the pi and codex cells '(does not cover Pi's in-invocation auto-retry, see below)' and '(does not cover Codex will_retry provider-stream retries)'. Link the existing 'Built-in auto-retry' bullet. This finding is about documentation parity only. The code path is the same for every harness.
4. [Axis 6] Terminal retry log does not show that run_limits.max_turn_retries capped the attempts (src/coder_eval/errors/executor.py:95) — The new exit if not should_retry(category, attempt) or attempt + 1 >= max_attempts: (line 95) shares one log branch for both stop reasons. When the cap stops a retryable category, line 100 logs f"[{task_id}] {operation_name} failed after {attempt + 1} attempts: {category.value} - {e}". Example: with max_turn_retries: 0, a transient AGENT_CRASH (category policy max_retries=2) logs "failed after 1 attempts: agent_crash". The log gives no sign that a configured cap suppressed retries the policy allows, so an operator can think the retry policy is broken. Recommendation: when should_retry(category, attempt) is still True but attempt + 1 >= max_attempts, add the cap to the log (for example "... failed after 1 attempt (attempt cap 1 reached): ..."). Also add a caplog assertion to test_attempt_cap_has_no_terminal_backoff that pins the terminal message for the capped case.
5. [Axis 6] The post-loop raise last_error fallback is unreachable, and max_attempts < 1 is not validated (src/coder_eval/errors/executor.py:119) — After this change, every loop iteration returns, raises (on the last iteration attempt + 1 >= max_attempts always holds), or continues to another iteration. So # Should never reach here, but just in case / if last_error: raise last_error (lines 119-121) cannot run. Only raise RuntimeError(f"Unexpected retry loop exit for {operation_name}") (line 122) can run, and only when max_attempts <= 0. In that case the operation is never called and the caller gets a misleading 'unexpected exit' error, not a precondition error. The orchestrator cannot trigger this (RunLimits.max_turn_retries has ge=0, so max_attempts >= 1), but execute_with_retry is a generic API. Recommendation: remove the dead if last_error fallback. Add an explicit precondition at the top, for example if max_attempts < 1: raise ValueError("max_attempts must be >= 1"), so the failure is loud and correctly worded.
6. [Axis 7] max_turn_retries gives 'turn' a different meaning from max_turns in RunLimits (docs/TASK_DEFINITION_GUIDE.md:276) — In the same run_limits table, line 272 defines a turn as a model API call: max_turns is the "Hard cap on main-thread model API calls per iteration, Claude Code's turn". Line 276 then defines max_turn_retries as "Max retries after the first failed agent communicate() attempt, per single-shot or dialog turn". That is a whole communicate() call, and the row then calls it a "dialog turn". Because the two keys share the max_turn prefix, a reader can take max_turn_retries: 2 to mean that each model API call (the max_turns unit) is retried twice. The field follows the turn_timeout precedent, so this is a naming paper cut, not a defect. Suggested fix, cheapest first: start the table cell (and the limits.py:65 description) with "Unit: one whole communicate() call (the turn_timeout unit), not a max_turns model call". If renaming is still cheap before release, a name such as max_communicate_retries would remove the clash.
7. [Axis 8] A positive max_turn_retries above the category caps is silently accepted and runs exactly as if unset, but the recorded config still shows the number the author typed (src/coder_eval/models/limits.py:61) — The field is max_turn_retries: int | None = Field(default=None, ge=0, ...) with the description "A positive value caps that policy and never makes a non-retryable error retryable." There is no upper bound and no warning. RETRY_CONFIG (errors/categories.py) allows at most 5 retries (AGENT_RATE_LIMIT), 3 (AGENT_API_ERROR) and 2 (AGENT_CRASH). The cap is min(config.max_retries + 1, max_attempts) (executor.py:109), so max_turn_retries: 99 (a case the PR's own test parametrizes as (99, 3)) and any value >= 5 behave exactly like unset. A value of 3 changes only rate-limit retries. task.json records task_config.resolved.run_limits.max_turn_retries: 99, so anyone comparing runs reads it as "99 replays allowed" when the real limit was 2 crash retries. This is not a scoring defect: unset still behaves as before, and 0 does what the docs say. It is a small YAML/UX paper cut that weakens benchmark comparison. Fix: in the docs/TASK_DEFINITION_GUIDE.md:276 row, say plainly that the effective limit per error category is min(value, category limit), give the current category limits (crash 2, API 3, rate-limit 5), and say that values >= 5 are the same as unset. Optionally, have validate_run_limits (orchestration/run_limits.py) warn when the value is above the largest category limit, the same way it already warns about task_timeout vs turn_timeout. Do not add an le= bound taken from RETRY_CONFIG, because that would make models/ depend on errors/.

What's Missing

Parallel paths:

• 🟡 The PR does not reconcile the cross-field run-limit validator with the new knob. orchestration/run_limits.py's validate_run_limits / INEFFECTIVE_TASK_TIMEOUT_WARNING (and the guide paragraph right after the run_limits table, TASK_DEFINITION_GUIDE.md:293-296) still says a task_timeout above turn_timeout is ineffective because 'the agent budget is turn_timeout'. With max_turn_retries > 0 (or unset), the agent can use up to (N+1) x turn_timeout plus backoff (AGENT_RATE_LIMIT starts at 60s and multiplies by 3). The orchestrator-level task_timeout watchdog bounds all of that. So the warning tells authors to lower the one cap that makes room for their configured retries. A config such as turn_timeout: 300, task_timeout: 300, max_turn_retries: 2 gets no warning, and its retries can never finish. Fix: make the warning conditional on max_turn_retries == 0, or add a sibling warning when task_timeout < (max_turn_retries+1) x turn_timeout. Also add one sentence to the guide that retries share the task_timeout budget. (trigger: src/coder_eval/models/limits.py)
• 🔵 experiments/default.yaml documents every run_limits key in its block, with commented-out examples for the token and USD caps. The PR does not add an entry for max_turn_retries there, so the default experiment, which is the place authors copy from, does not show the new cap. Add a commented # max_turn_retries: 0 line that says unset keeps the category policy. (trigger: src/coder_eval/models/limits.py)
• 🔵 The new parity row says the cap behaves the same on all six harnesses. It does not link the existing Pi 'Built-in auto-retry' bullet or the Codex will_retry provider-stream retry. On those two harnesses, max_turn_retries: 0 does not prevent replay inside one communicate() call. (trigger: docs/agents/HARNESS_PARITY.md) (restates: Axis 5: HARNESS_PARITY row claims the same behavior on all harnesses but ignores Pi/Codex in-call replay)

Tests:

• 🔵 No caplog test covers the new attempt_limit = min(config.max_retries + 1, max_attempts) warning denominator, or the terminal log in the capped case. test_attempt_cap_has_no_terminal_backoff asserts only await counts, so neither side of the min() is tested at the executor level. (trigger: src/coder_eval/errors/executor.py) (restates: Axis 3: The changed retry-warning denominator (attempt N/min(policy, cap)) has no assertion)
• 🔵 Dialog-mode coverage only tests max_turn_retries: 0, which stops before the next question. No test checks that a positive cap in dialog mode (for example, max_turn_retries=1 with a crash and then recovery on dialog turn 2) retries that dialog turn, keeps the crashed partial, and then continues to simulator.next_user_message. The PR body and guide claim the cap applies 'per single-shot or dialog turn', but only the single-shot path is tested with a positive value. (trigger: tests/test_run_limits_orchestrator.py)
• 🔵 The changed exit condition in execute_with_retry is shared with the two sandbox-setup callers (orchestrator.py:1530 and :1559), which pass no max_attempts and fall back to 10. No test asserts that the default path still allows the full category budget on the largest policy, AGENT_RATE_LIMIT (5 retries, so 6 attempts). Without it, a later change to the default 10 or to the attempt + 1 >= max_attempts comparison could silently cut setup or rate-limit retries. (trigger: src/coder_eval/errors/executor.py)

Downstream consumers:

• 🔵 Cross-run comparison consumers (the evalboard, /coder-eval:analyze, coder-eval-uipath) can now see runs that differ only in max_turn_retries, and the cap changes final_status distributions: an ERROR row versus a row recovered after a replay. The cap is recorded only in task.json task_config.resolved.run_limits, and the recorded value is not the effective per-category cap. run.json rows carry iterations[].crashed, so the replay count can be derived. But REPORT_SCHEMA.md does not say that several iterations with the same iteration number are whole-turn replays bounded by this cap. Add that sentence, or surface the effective cap the way expected_turns is surfaced in run_record.py. (trigger: src/coder_eval/models/limits.py) (restates: Axis 8: A positive max_turn_retries above the category caps is silently accepted and runs exactly as if unset, but the recorded config still shows the number the author typed)

Nightly pipeline:

• 🔵 The PR changes the shared retry executor on the production run path: agent communicate plus the sandbox-setup retries. It does not say explicitly what happens to the nightly run. The real impact appears to be nil: the default is None, so max_attempts stays 10, which is above every RETRY_CONFIG cap (5 at most), and the warning denominator is unchanged. The PR also brings in an unrelated uv.lock pyasn1 0.6.4->0.6.5 bump. One line in the PR body should confirm both points: there is no default-path change for nightly or DockerRunner runs, and the lockfile bump is shared with #246. (trigger: src/coder_eval/errors/executor.py)

Harness & Lint Improvements

Static checks (lint / type):

• [ce-lint] CE070 (CE069 is already taken by the in-flight TestCE069NoCostLiteral branch, so CE070 is the next free number): generate the per-category whole-turn retry limits from errors/categories.py::RETRY_CONFIG into a marker block in docs/TASK_DEFINITION_GUIDE.md, next to the max_turn_retries row. Use <!-- retry-limits:start --> / <!-- retry-limits:end -->, and generate the block with a make retry-limits target, the same way make docs-indexes and make pricing-mirror work. The block lists category, max_retries, the effective attempt limit min(value+1, policy+1), and the 'values >= max category limit behave as unset' line. The rule fails make lint on drift. It is a doc-surface rule, so wire it as a @pytest.mark.lint class in tests/test_custom_lint.py, with a helper in tests/lint/retry_limits_doc.py. Do not add it to tests/lint/runner.py, because the rule reads Markdown and is not an AST check. It cannot be a pyright or ruff setting: the defect is drift between a Python dict and Markdown prose. A Field le= bound is not an option either, because it would make models/ import errors/ (the CE017 layering). Prevents: Finding 7 (limits.py:61 / TASK_DEFINITION_GUIDE.md:276): max_turn_retries: 99 is accepted silently and runs exactly like unset, while the docs give no category limits. CE070 also stops the guide's numbers from drifting when RETRY_CONFIG changes. It partly reduces finding 1, because the numbers come from RETRY_CONFIG and are not typed again by hand. Static analysis does not cover the other findings (2-6), because they need runtime behavior or semantic judgment. Each one has a harness_improvements entry that says why.

Harness improvements (not statically reachable):

• Add a table-driven executor retry test in tests/test_retry_logic_comprehensive.py. Parametrize it over every RETRY_CONFIG category and max_attempts in {1, 2, policy+1, 99}. For each case, assert (a) the number of operation calls, (b) the number of asyncio.sleep calls (no sleep after the last attempt), (c) the caplog warning denominator attempt k/min(policy+1, max_attempts), and (d) that the terminal error names the attempt cap when the cap stopped the retry, not the category policy. Because the matrix comes from RETRY_CONFIG, a new category gets coverage with no extra work. Why not static: The denominator, call counts and terminal message are runtime results of the retry loop. They depend on the value of max_attempts and the category policy. No grep or AST pattern can tell that min(config.max_retries + 1, max_attempts) produces the wrong number, or that a log branch hides the reason the loop stopped. Prevents: Finding 2 (executor.py:109: the warning denominator has no assertion) and finding 4 (executor.py:95: the terminal log does not show that max_turn_retries capped the attempts).
• Add a check to orchestration/run_limits.py::validate_run_limits that warns when the resolved max_turn_retries is greater than max(c.max_retries for c in RETRY_CONFIG.values()). The warning says that the effective limit per category is min(value, category limit), so the value acts the same as unset. Use the same channel as the existing task_timeout vs turn_timeout warning. Add a resolver test that sets the value to 99 and asserts the warning. Why not static: The value is user-authored YAML or -D input. It is resolved at runtime through the 5-layer merge. A Field le= bound taken from RETRY_CONFIG would make models/ depend on errors/, which breaks the CE017 layering. So the orchestration layer must make the check after resolution. Prevents: Finding 7: task.json records run_limits.max_turn_retries: 99 while the run behaved as if the field were unset.
• At the top of errors/executor.py::execute_with_retry, add if max_attempts < 1: raise ValueError(...). Delete the unreachable post-loop if last_error: raise last_error fallback, and keep one RuntimeError as an explicit safety net. Add a unit test that passes max_attempts=0 and asserts ValueError is raised and the operation is never called. Why not static: The fallback is unreachable only because on the last iteration attempt + 1 >= max_attempts is always true, and that branch raises. Proving this needs integer reasoning about range(max_attempts). ruff's PLW0101 (preview) and pyright's reportUnreachable use only control flow and type narrowing, so they cannot see it. Python also has no pyright-enforced refinement type for int >= 1. Prevents: Finding 5 (executor.py:119: dead fallback, and a misleading 'unexpected exit' error when max_attempts <= 0).
• Record Pi's in-invocation auto-retry (the willRetry: true agent_start cycle in pi_agent.py) as ProviderError(will_retry=True) in the same provider_errors field that Codex already fills. A max_turn_retries: 0 benchmark can then see the replay that still happens inside one communicate() on both harnesses. Also add an item to the HARNESS_PARITY authoring checklist: a new run_limits.* row that has the same cell for every harness must be checked against the 'Built-in auto-retry' section before merge. Why not static: A lint rule cannot decide whether a harness's internal retry defeats the purpose of a knob. That needs semantic knowledge of each agent's provider loop. The residual replay can only be observed in a live event stream. Prevents: Finding 3 (HARNESS_PARITY.md:18: the row claims the same behavior on all harnesses but ignores Pi and Codex in-call replay).
• Add a review checklist item for new user-facing RunLimits fields, in docs/EXTENDING.md or .claude/notes/README.md. (1) The Field description is the contract SSOT. The guide row may add only links and examples. The .claude/notes/ paragraph must hold only rationale and must not restate the contract. (2) If the new field reuses a unit word that a sibling field already defines differently (for example, 'turn' means a model API call in max_turns), the description must state its unit at the start. Why not static: The restatement is a paraphrase ('Unset preserves the existing error-category policy' vs 'None keeps the error-category retry policy'), so n-gram or shingle overlap checks would miss it. Whether two uses of 'turn' mean different units is also a semantic question. Both need reviewer judgment. Prevents: Finding 1 (.claude/notes/agents.md:95: the contract is restated in four places) and finding 6 (TASK_DEFINITION_GUIDE.md:276 / limits.py:61: 'turn' in max_turn_retries has a different meaning from 'turn' in max_turns).

Top 5 Priority Actions

1. Correct the max_turn_retries row at docs/agents/HARNESS_PARITY.md:18. In the pi and codex cells, say that the cap does not cover Pi's in-invocation auto-retry (willRetry) or Codex will_retry provider-stream retries, and link the existing 'Built-in auto-retry' bullet. Without this, a benchmark that sets max_turn_retries: 0 can still get replayed trajectories on those two harnesses.
2. In the docs/TASK_DEFINITION_GUIDE.md:276 row and the src/coder_eval/models/limits.py:61-69 description, state three things. The effective limit per error category is min(value, category limit), with crash 2, API 3 and rate-limit 5. A value of 5 or more behaves the same as unset. The unit is one whole communicate() call (the turn_timeout unit), not a max_turns model call. Optionally, have validate_run_limits warn when the value is above the largest category limit, so that a recorded 99 is not read as 99 replays.
3. At src/coder_eval/errors/executor.py:95-100, add a separate terminal log message for the case where should_retry() is still True but the attempt cap is reached, for example 'attempt cap 1 reached'. Add caplog assertions that pin this message and the attempt N/min(policy, cap) warning at executor.py:109-111. Cover both sides of the min(): max_attempts=2 must give 'attempt 1/2' and max_attempts=5 on AgentCrashError must give 'attempt 1/3'.
4. In src/coder_eval/errors/executor.py, remove the unreachable post-loop 'if last_error: raise last_error' fallback at lines 119-121. Replace the misleading RuntimeError at line 122 with an explicit 'if max_attempts < 1: raise ValueError(...)' check at the top of execute_with_retry.
5. Make the limits.py Field description the single statement of the contract. Remove the restated contract from .claude/notes/agents.md:95-101 and keep only the rationale (replay changes the measured trajectory, and the reason for the cap-before-backoff and drain-partial ordering). Move the unique content of TASK_DEFINITION_GUIDE.md:286-291 (the -D example and 'an exhausted limit stops the dialog') into the table row.

---

Stats: 0 🔴 · 0 🟠 · 0 🟡 · 7 🔵 across 8 axes reviewed.

[bai-uipath]

Approve. Clean and focused: unset keeps today's policy, the cap is checked before the backoff, and it can't make a non-retryable error retryable.

Fix what you agree with:

• A cap above the category policy is a silent no-op: a positive value larger than the error category's own retry count runs exactly as if unset, while the recorded config shows the number the author typed. Fix: reject or clamp it, or log the effective cap.
• The terminal log hides the cap: when the run_limits cap ends the retries, the error line still reads like the category policy stopped them. Fix: say the cap ended it.

Minor: the same contract is restated in the field description, the guide table, the guide paragraph and the agents notes; trim the notes to the rationale.