Improve openai_ask tool-use behavior#719
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens and hardens openai_ask’s Chat Completions + tool-use loop behavior, with a focus on safer logging, stricter tool validation, and clearer iteration limiting semantics.
Changes:
- Switch Chat Completions requests to
max_completion_tokensand opt out of request storage viastore: false. - Add validation for tool definitions and returned tool-call payloads, preventing malformed/unsupported tool calls from invoking local handlers.
- Adjust the tool-use loop to cap local tool execution rounds while still allowing one final model turn to produce an answer; reduce sensitive data exposure in logs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/fastlane/plugin/wpmreleasetoolkit/actions/common/openai_ask_action.rb |
Updates request parameters, adds tool/max-iteration validation, hardens tool-call execution and logging, and refines loop termination semantics. |
spec/openai_ask_action_spec.rb |
Adds/updates specs for new request fields, iteration cap behavior, tool validation, and sensitive logging guarantees. |
CHANGELOG.md |
Documents the openai_ask tool-use behavior improvements under Trunk bug fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| module Actions | ||
| class OpenaiAskAction < Action | ||
| OPENAI_API_ENDPOINT = URI('https://api.openai.com/v1/chat/completions').freeze | ||
| DEFAULT_MAX_COMPLETION_TOKENS = 2048 |
There was a problem hiding this comment.
It would be good to document where this number comes from. What's its rationale?
I feel the 5 in the max tool iterations is much easier to accept at face value than 2048 tokens.
There was a problem hiding this comment.
Addressed in 1e9c94c by documenting that DEFAULT_MAX_COMPLETION_TOKENS preserves the previous max_tokens ceiling while moving to the current API field.
There was a problem hiding this comment.
Thanks! Also, only later in the review I realized that this was a previously introduced magic number, I should have acknowledge that in the comment.
|
|
||
| validate_tools_array!(tools) | ||
| validate_max_tool_iterations!(max_tool_iterations) | ||
| validate_tools!(tools) |
There was a problem hiding this comment.
Nitpick: What do you think of an empty line to visually separate the validation from the execution?
| validate_tools!(tools) | |
| validate_tools!(tools) | |
| end | ||
|
|
||
| def self.tool_type(tool) | ||
| return '' unless tool.is_a?(Hash) |
There was a problem hiding this comment.
I wonder if returning nil would be clearer here.
There was a problem hiding this comment.
Agreed. Changed tool_type to return nil for missing/non-hash definitions in 1e9c94c.
| ### Bug Fixes | ||
|
|
||
| _None_ | ||
| - `openai_ask`: improve tool-use handling by requiring named function tools, using `max_completion_tokens`, opting out of OpenAI request storage, omitting sensitive tool diagnostics from logs, and refusing to execute additional tool calls after `max_tool_iterations`. [#719] |
There was a problem hiding this comment.
Is this a bug fix then? All of the entries seem improvements to me. Should this be an internal changes or new feature entry?
There was a problem hiding this comment.
I think it's both 😅 omitting sensitive tool diagnostics from logs and not running additional tool calls after max_tool_iterations are actually bug fixes. But indeed other points are improvements.
What does it do?
Improves
openai_askfollow-up behavior after the initial tool-use support:max_completion_tokensfor Chat Completions requests.store: falseto opt out of OpenAI request storage.Checklist before requesting a review
bundle exec rubocopto test for code style violations and recommendations.specs/*_spec.rb) if applicable.bundle exec rspecto run the whole test suite and ensure all your tests pass.CHANGELOG.mdfile to describe your changes under the appropriate existing###subsection of the existing## Trunksection.MIGRATION.mdfile to describe how the changes will affect the migration from the previous major version and what the clients will need to change and consider.