This test should be removed. It imports from the private pydantic_ai._output module and directly inspects toolset.max_retries — both of which violate the testing guidelines around testing through public APIs and not accessing private internals.

The second test (test_tool_output_max_retries_overrides_agent_retries) already covers the same behavior through the public API by verifying that ctx.max_retries reflects the ToolOutput value at runtime, which is the meaningful assertion.

ToolOutput is already imported at line 70 — remove this inline import. Imports should be at the top of the file.

Following the pattern of the existing retry tests above (e.g. test_tool_output_function_retries), this test should also track and assert retries_log (the ctx.retry values) alongside max_retries_seen, and should also assert on result.all_messages() using snapshot() to validate the complete execution trace.

@DouweM This change makes ToolOutput.max_retries control the per-tool retry limit (via ToolsetTool.max_retries), but the global output retry counter in _agent_graph.py still uses GraphAgentDeps.max_result_retries (always from the agent's _max_result_retries). This means:

• ToolOutput(Foo, max_retries=7) with Agent(retries=1) → per-tool limit is 7, but the global increment_retries check fires at retry 1, so the effective limit is still 1.
• ToolOutput(Foo, max_retries=2) with Agent(retries=10) → per-tool limit fires at 2, global at 10, so effective limit is 2.

Before this PR both values were always the same (agent set both). Now they can diverge, but only the "lower wins" direction actually works. A user who sets ToolOutput(Foo, max_retries=5) without also bumping the agent's retries/output_retries to at least 5 will still get the agent default of 1.

Is this the intended semantic, or should ToolOutput.max_retries also flow into (or override) GraphAgentDeps.max_result_retries?

This pattern was applied here and in the ToolRetryError handler below (line 1027), but the two UnexpectedModelBehavior handlers in this same function (lines 982 and 1019) still use ctx.deps.max_result_retries directly. The one at line 1019 has validated in scope and should use the same pattern for consistency. The one at line 982 fires before validated exists, but could look up the tool via tool_manager.get_tool_def(call.tool_name).

This means the fix only works for validation errors and ModelRetry/ToolRetryError, but not for UnexpectedModelBehavior errors on output tools — those will still be capped by the agent-level retries.

That said, as flagged in the other comment, the deeper question is whether this piecemeal approach is the right fix, or whether ToolOutput.max_retries should also flow into GraphAgentDeps.max_result_retries. @DouweM

The hardcoded fallback else 1 here means that even when OutputToolset.max_retries is None (no ToolOutput.max_retries was set), the ToolsetTool gets max_retries=1 before the agent has a chance to set OutputToolset.max_retries to the agent's default. This works because get_tools() is called after the agent sets OutputToolset.max_retries in __init__, but it's fragile — the else 1 is a dead code path since the agent always fills in None before this method runs. Consider removing the fallback and making the type int (not int | None) by the time get_tools() is called, to make the invariant explicit.

max(max_retries or 0, output.max_retries) treats max_retries=0 (if a previous ToolOutput set it explicitly) the same as None. This is probably fine since max_retries=0 doesn't make practical sense, but the idiomatic way to handle this for int | None is: max_retries = max(max_retries, output.max_retries) if max_retries is not None else output.max_retries — which is actually what the commenter on the issue suggested.

This test should be removed or rewritten. It imports from pydantic_ai._output (a private module) and directly inspects OutputToolset.build() internals like toolset.max_retries — both of which violate the testing guideline of testing through public APIs, not private methods or helpers.

The behavioral test below (test_tool_output_max_retries_overrides_agent_retries) already covers the important case: that ToolOutput(max_retries=N) actually results in N retries at runtime. That's what matters. The extraction logic in build() is an implementation detail.

If you want to keep coverage of the "max across multiple ToolOutputs" case, add a behavioral test with output_type=[ToolOutput(Foo, max_retries=2), ToolOutput(Bar, max_retries=5)] and verify that 5 retries are actually allowed at runtime.

max(max_retries or 0, output.max_retries) conflates max_retries=0 with None. While max_retries=0 isn't practically useful, the idiomatic int | None pattern is:

max_retries = max(max_retries, output.max_retries) if max_retries is not None else output.max_retries

(This was also suggested in the issue comment.)

The hardcoded else 1 fallback means that if OutputToolset.max_retries is still None when get_tools() is called, each ToolsetTool gets max_retries=1 regardless of what the agent's retries/output_retries was set to.

In practice, the agent sets output_toolset.max_retries = self._max_result_retries before get_tools() is called, so this fallback shouldn't be hit in normal usage. But if it ever is, 1 is the wrong default — it should match whatever the agent would have set.

Consider making the fallback more robust by asserting that max_retries is not None at this point (since the agent is supposed to have filled it in), or at least documenting why 1 is acceptable here.

The pattern max_retries = (...).tool.max_retries if (...).tool else ctx.deps.max_result_retries is repeated four times in this function (lines 983, 998, 1019, 1026). Per the repo's code style guidelines, duplicated logic should be consolidated into a helper. Consider extracting a small helper like:

def _get_max_retries(tool: ToolsetTool[DepsT] | None, ctx: GraphRunContext[...]) -> int:
    return tool.max_retries if tool else ctx.deps.max_result_retries

Or at minimum, since two of the four sites have validated.tool and two have a tool looked up from tool_manager.tools, unify the lookup approach — the first handler (line 982) fetches the tool via tool_manager.tools.get(call.tool_name) while the other three use validated.tool. Using validated.tool consistently (where validated is available) would be cleaner.

As flagged in an earlier review, this test should also track and assert ctx.retry values (a retries_log list), matching the pattern of the existing test_tool_output_function_retries test right above. This verifies that the retry counter is incrementing correctly on each attempt, not just that max_retries is visible.

@DouweM This is the key change in this PR that still needs your input: all four increment_retries calls have been removed from the output tool retry paths in process_tool_calls. The retry limiting for output tools is now handled entirely by ToolManager._check_max_retries, which uses per-tool retry tracking via RunContext.retries[tool_name] — the same mechanism that function tools already use.

This is a sound approach and makes output tools consistent with function tools, but it's a significant behavioral change beyond the original scope of the bug (which was just that ToolOutput.max_retries was silently ignored):

1. The global GraphAgentState.retries counter is no longer incremented for output tool retries — it now only tracks text output retries (via _build_retry_node) and unknown tool calls.
2. The error message changes from "Exceeded maximum retries (N) for output validation" to "Tool 'final_result' exceeded max retries count of N".
3. For the UnexpectedModelBehavior cases (lines 1294 and 1323), the old code would call increment_retries before re-raising, which could replace the original error with a max-retries error if the counter was already at the limit. Now the original UnexpectedModelBehavior always propagates directly.

The earlier auto-review comments on this topic (from the previous code revision) are now outdated, so I'm re-raising the question here on the current code.

This test covers the single-output-tool case well — good use of retries_log, max_retries_log, and the all_messages() snapshot.

However, as flagged in earlier reviews, there should also be a test with multiple output tools that have different max_retries values, e.g. output_type=[ToolOutput(A, max_retries=5), ToolOutput(B, max_retries=2)]. This would:

1. Verify that each tool's limit is respected independently (the whole point of the _max_retries_overrides dict).
2. Confirm that a failure on tool A doesn't exhaust tool B's retry budget (since the per-tool tracking in ToolManager is per tool name).
3. Exercise the code path in get_tools() where _max_retries_overrides.get(tool_def.name, self.max_retries) returns different values for different tools.

The _max_retries_overrides dict creates a parallel data structure that maps tool names to their max_retries overrides, separate from the existing processors dict that also maps tool names to their processing info. Consider whether this could be simplified by storing the max_retries override on the ObjectOutputProcessor or alongside the ToolDefinition instead, to avoid needing to keep two name-keyed dicts in sync.

That said, this is a minor concern — the current approach works and the sync between the two dicts is straightforward since they're both built in the same loop in build().

@DouweM This PR removes all four increment_retries calls from the output tool retry paths in process_tool_calls, making output tool retries entirely governed by the per-tool mechanism in ToolManager._check_max_retries (which uses ToolsetTool.max_retries and RunContext.retries).

This is a significant behavioral change beyond just "respecting ToolOutput.max_retries":

1. The global retry counter (GraphAgentState.retries) is no longer incremented for output tool failures. This means the "Exceeded maximum retries (N) for output validation" error from increment_retries will never be raised for output tool retries — the error is now always "Tool 'X' exceeded max retries count of N" from ToolManager._check_max_retries. The test changes in test_model_test.py confirm this change in error messaging.

2. GraphAgentDeps.max_result_retries no longer acts as a cap on output tool retries in process_tool_calls — only the per-tool ToolsetTool.max_retries matters now.

3. Text output retries still use the global counter (via _build_retry_node at line 883 and _handle_text_response at line 1146), creating an inconsistency in how retries are tracked between text output and tool output.

The original issue (#4678) was that ToolOutput.max_retries was silently ignored. A more conservative fix would have been to pass the per-tool max_retries to increment_retries (as was done in earlier iterations of this PR) rather than removing the calls entirely. Could you weigh in on which approach is correct?

Adding # pragma: no cover to work around reduced coverage violates the repo's testing guidelines — the preferred approach is to write tests that exercise this branch, or to remove the branch if it's truly unreachable. If removing the increment_retries calls from the output tool paths makes this branch unreachable, that's worth understanding and documenting rather than suppressing with pragmas.

This assert will crash at runtime if max_retries hasn't been set by the Agent. Since max_retries starts as None and is only set in Agent.__init__, this creates a fragile coupling where OutputToolset can't be used independently (e.g. in tests or by advanced users building custom graphs). Per the repo guidelines, prefer a default over an assertion for this kind of invariant — for example, falling back to self.max_retries or 1 in the get() call on line 978 would be more resilient.

This test is a good addition for the multi-tool case, but it should include an assert result.all_messages() == snapshot(...) assertion to match the pattern established by test_tool_output_max_retries_overrides_agent_retries above and the existing test_tool_output_function_retries test. The all_messages() snapshot validates the complete execution trace (tool calls, retry prompts, etc.), not just the final output — this is important for catching regressions in the retry flow.

The _max_retries_overrides dict duplicates the key space of processors — both are keyed by tool name and built in the same loop. Consider storing the max_retries override on the ObjectOutputProcessor (or alongside it as a tuple) rather than maintaining a parallel dict. This would keep per-tool data co-located and avoid needing to keep two name-keyed structures in sync.

That said, this is a minor concern compared to the higher-level design question about removing increment_retries from the output tool paths.

# pragma: no cover on output_b — if this function is truly never called (the model always picks output_a), this test doesn't actually exercise the per-tool isolation. A stronger test would have the model call output_b at some point and verify it hits its retry limit (2) independently of output_a's limit (5). As written, this test only demonstrates that output_a gets its override; it doesn't verify that output_b gets a different override.

📝 Info: Behavioral change: output tool retries no longer affect the global retry counter

This PR removes all ctx.state.increment_retries() calls for output tools in process_tool_calls (_agent_graph.py:1289-1333). Previously, output tool failures incremented BOTH the per-tool counter (via ToolManager._check_max_retries) AND the global counter (GraphAgentState.retries). Now only per-tool tracking applies.

This means:

• Old: 3 output tools each failing once → global counter reaches 3, could trigger max_result_retries
• New: each tool's failures tracked independently; global counter unaffected

The global counter (ctx.state.retries) is still incremented for empty responses (line 985), text response retries (line 1057), model-level retries (line 876), and unknown tools (line 1359). This is an intentional semantic change confirmed by the updated tests, but worth noting for reviewers that the max_result_retries limit no longer constrains output tool retries — only ToolOutput.max_retries (or the agent-level default) does.

Was this helpful? React with 👍 or 👎 to provide feedback.