Fix Assistants IChatClient handling of unrelated tool calls in history #6891
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the OpenAI Assistants IChatClient implementation where it incorrectly assumed that any tool results in message history required a thread ID. The fix addresses scenarios where message history contains historical function calls/responses (like in agent scenarios) rather than active tool calls requiring a thread.
Key changes:
- Removes validation that incorrectly throws when tool results are present without a thread ID
- Updates logic to only process function results when the message role is Tool
- Improves comments to clarify the distinction between active and historical tool results
src/Libraries/Microsoft.Extensions.AI.OpenAI/OpenAIAssistantsChatClient.cs
Show resolved
Hide resolved
The implementation is assuming that any tool results in the incoming message history mean a thread ID is required, which would be necessary if those tool results were for an active run. But that's not the case if the history just contains historical function calls/responses, as is the case in agent scenarios where the message history from one agent is passed to another. The fix is to just stop throwing based on this incorrect assumption.
stephentoub
force-pushed
the
fixassistantshistory
branch
from
d52009a to
0590e3b
Compare
|
|
||
| // Get the thread ID. | ||
| string? threadId = options?.ConversationId ?? _defaultThreadId; | ||
| if (threadId is null && toolResults is not null) |
There was a problem hiding this comment.
Is this change related to the fix?
There was a problem hiding this comment.
This change is the fix. The other line is housekeeping.
| break; | ||
|
|
||
| case FunctionResultContent result: | ||
| case FunctionResultContent result when chatMessage.Role == ChatRole.Tool: |
There was a problem hiding this comment.
Should we add a test validating the fix?
There was a problem hiding this comment.
We don't have good tests for assistants because of the multi-request nature making it a lot harder to mock well.
The implementation is assuming that any tool results in the incoming message history mean a thread ID is required, which would be necessary if those tool results were for an active run. But that's not the case if the history just contains historical function calls/responses, as is the case in agent scenarios where the message history from one agent is passed to another. The fix is to just stop throwing based on this incorrect assumption.
Microsoft Reviewers: Open in CodeFlow