fix: validate Tool max_retries and timeout parameters - #6375
Conversation
max_retries < 0 would silently cause unbounded retries. timeout <= 0 causes immediate failures. Add UserError validation for both.
github-actions
Bot
added
size: S
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis change adds constructor-time validation to 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR fixes a configuration footgun in the Tool API by validating max_retries and timeout at Tool construction time, preventing negative retry budgets and non-positive timeouts that can lead to unbounded retry loops or immediate timeouts.
Changes:
- Add runtime validation in
Tool.__init__to rejectmax_retries < 0andtimeout <= 0(when notNone). - Add regression tests asserting accepted/rejected values for
max_retriesandtimeout.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
pydantic_ai_slim/pydantic_ai/tools.py |
Raises UserError on invalid max_retries/timeout values during Tool instantiation. |
tests/test_tools.py |
Adds tests covering the new validation behavior for Tool parameters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Mirror the `concurrency.py` `_validate_max_running` pattern by moving the `max_retries`/`timeout` checks into module-level `_validate_max_retries` and `_validate_timeout` helpers, and validate the local parameters instead of the assigned instance attributes. Co-Authored-By: Claude Opus 4.8 <[email protected]>
| self.metadata = metadata | ||
| self.timeout = timeout | ||
| _validate_max_retries(max_retries) | ||
| _validate_timeout(timeout) | ||
| self.defer_loading = defer_loading |
There was a problem hiding this comment.
Good call — moved both validations to the top of Tool.__init__, before schema generation and any attribute assignment, so invalid max_retries/timeout fail fast without partially initializing the instance. Done in 251f8d9.
Move the `max_retries`/`timeout` validation to the top of `Tool.__init__`, before schema generation and attribute assignments, so invalid configuration fails fast without partially initializing the instance. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Fixes #6374