test tool limiting #1196
test tool limiting #1196
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements tool call limiting functionality to prevent runaway tool usage in the slackbot. The changes add a configurable maximum limit on tool calls per agent turn, with graceful handling when the limit is exceeded.
- Adds a
max_tool_callsparameter to control tool usage limits per agent turn - Implements limit checking logic that returns a user-friendly message instead of raising exceptions
- Configures the default limit through settings with a value of 50 calls per turn
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
examples/slackbot/src/slackbot/wrap.py |
Adds tool call limiting logic with configurable max_tool_calls parameter and graceful limit handling |
examples/slackbot/src/slackbot/settings.py |
Adds max_tool_calls_per_turn configuration setting and consolidates model validators |
examples/slackbot/src/slackbot/api.py |
Integrates tool call limiting by passing the configured limit to WatchToolCalls |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| total_calls = sum(counts.values()) | ||
| if total_calls > max_tool_calls: | ||
| # Return a message instead of raising an exception | ||
| return "I've reached my tool use limit for this response. Please ask a follow-up question if you need more information." |
There was a problem hiding this comment.
The function is returning a string when the expected return type is T. This will cause a type mismatch and potential runtime errors since the calling code expects the original function's return type, not a string message.
| return "I've reached my tool use limit for this response. Please ask a follow-up question if you need more information." | |
| # Raise an exception instead of returning a string to preserve type safety | |
| raise ToolUseLimitExceeded("I've reached my tool use limit for this response. Please ask a follow-up question if you need more information.") |
| os.environ["TURBOPUFFER_API_KEY"] = api_key | ||
| except Exception: | ||
| pass # If secret doesn't exist, turbopuffer will handle the error | ||
| if not self.admin_slack_user_id: |
There was a problem hiding this comment.
This code assumes admin_slack_user_id is optional, but the field definition doesn't show it as Optional. If it's a required field, this check will never be True. If it should be optional, the field type should be Optional[str] or str | None.
| decorator: Callable[..., Callable[..., T]] = task, | ||
| tags: set[str] | None = None, | ||
| settings: dict[str, Any] | None = None, | ||
| max_tool_calls: int = 15, # Default limit per agent run |
There was a problem hiding this comment.
The default value of 15 here conflicts with the default value of 50 in the settings file. This inconsistency could lead to confusion about which limit is actually applied.
| max_tool_calls: int = 15, # Default limit per agent run | |
| max_tool_calls: int = 50, # Default limit per agent run (matches settings file) |
No description provided.