Update unknown tool retry logic

[geodavic]

Currently if an unknown tool name is provided by the model, a ModelRetry(f'Unknown tool name: {name!r}. {msg}') is raised and subsequently it will move to the retry step here. In this step, however, the max_retries will always be 1 because the tool name is unknown (i.e.tool is None). This causes the UnexpectedModelBehavior(f'Tool {name!r} exceeded max retries count of {max_retries}') to be raised, thus stopping the run.

It would be nice if there was retry logic on unknown tools, so that the model can correct itself if it accidentally returns a malformed tool name (I've seen this happen, for example, with gpt-oss-20b).

This PR adds a default_max_retries configuration to the tool manager that allows unknown tool attempts to be tried more than two times.

[geodavic]

One open question is: if the user doesn't pass unknown_tool_retries, should it default to retries? Or default to 1?

Currently it is set to default to 1 so as not to change existing behavior. But I think I prefer the UX of defaulting to retries (feels more correct).

EDIT: see conversation below

[DouweM]

however, the max_retries will always be 1 because the tool name is unknown (i.e.tool is None). This causes the UnexpectedModelBehavior(f'Tool {name!r} exceeded max retries count of {max_retries}') to be raised, thus stopping the run.

@geodavic Isn't this only the case if the model calls the same non-existent tool twice? Because max_retries will start as 1 and current_retry as 0.

But I think I prefer the UX of defaulting to retries (feels more correct).

I would prefer not to have a new setting and to inherit the existing default tool retries instead.

[geodavic]

@DouweM Yes you're right; it will have to hallucinate a tool name twice for this to happen.

So your suggestion is to not expose this as a configurable setting and just set it to retries? That's fair and I can update to reflect that. That will fix the problems I am trying to fix in my use-case at least.

[DouweM]

So your suggestion is to not expose this as a configurable setting and just set it to retries? That's fair and I can update to reflect that. That will fix the problems I am trying to fix in my use-case at least.

@geodavic Yep let's do that.

[geodavic]

@DouweM done!

[DouweM]

@geodavic Let's simplify it a bit; I'm not against having a dedicated exception class etc, but it's not necessary for the specific improvement we're trying to make here

[DouweM]

@geodavic Let's simplify it a bit; I'm not against having a dedicated exception class etc, but it's not necessary for the specific improvement we're trying to make here

[geodavic]

@DouweM resolved your comments