fix(tool runner): use latest message for tool response generation #878
Conversation
…rrectly invalidating cache
|
@yjp20 would you be able to take a look? |
src/lib/tools/BetaToolRunner.ts
Outdated
| return this.#generateToolResponse(message); | ||
| return this.#generateToolResponse({ | ||
| role: message.role, | ||
| content: message.content, | ||
| }); |
There was a problem hiding this comment.
It was needed for better comparison with JSON.stringify, but it doesn’t look very reliable anyway 😞, especially without stable stringifying.
I think we can just go with reference comparison—I’ve changed that behavior.
There was a problem hiding this comment.
If we can, I think we should encode these in the tests rather than an example file and keep the examples a bit more minimal (there's a lot of them already)
There was a problem hiding this comment.
I’ve moved pushMessage to the advanced example to avoid adding another example script, and I’ve also added some basic tests for pushMessages.
helpers.md
Outdated
| - [`examples/tools-helpers-zod.ts`](examples/tools-helpers-zod.ts) - Zod-based tools | ||
| - [`examples/tools-helpers-json-schema.ts`](examples/tools-helpers-json-schema.ts) - JSON Schema tools | ||
| - [`examples/tools-helpers-advanced.ts`](examples/tools-helpers-advanced.ts) - Advanced tool runner patterns | ||
| - [`examples/tools-helpers-pushmessages.ts`](examples/tools-helpers-pushmessages.ts) - Using `pushMessages()` to guide based on tool results |
There was a problem hiding this comment.
we should delete this now, right?
There was a problem hiding this comment.
Yep, good catch, thanks
| */ | ||
| async generateToolResponse() { | ||
| const message = (await this.#message) ?? this.params.messages.at(-1); | ||
| const message = (this.#mutated ? undefined : await this.#message) ?? this.params.messages.at(-1); |
There was a problem hiding this comment.
Isn't this a breaking change?
I am missing the motivation for this change, could you please add a PR description?
There was a problem hiding this comment.
this.#message points to the last API response. When the user adds new messages (via pushMessages or setMessagesParams), we shouldn’t use the last API response for generating the tool response, since we might end up putting tool_result blocks somewhere other than right after the tool_use blocks.
I've also updated the PR description
stainless-app
bot
force-pushed
the
next
branch
2 times, most recently
from
eeb7fab to
7b4849b
Compare
3 participants
Fix tool runner pushMessages and tool response caching
Summary
This PR fixes a bug in the
BetaToolRunnerwhere tool response caching was not properly invalidated whenpushMessages()was called, causing stale cached responses to be reused. The fix ensures that when the conversation is mutated viapushMessages(), the tool response cache is correctly invalidated.Changes
Core Fix (
src/lib/tools/BetaToolRunner.ts)#lastProcessedMessagefield to track the last message processed for tool response generationgenerateToolResponse()to check mutation state (this.#mutated) before using cached message#generateToolResponse()to use reference equality check to invalidate cache when message reference changespushMessages()adds new messages, subsequent calls togenerateToolResponse()correctly process the new conversation stateBehavior
Before this fix, calling
pushMessages()after a tool use would still result in stale cached tool responses being used. Now, the cache is properly invalidated based on message reference changes, ensuring the runner correctly handles dynamically modified conversation history.