-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Python: fix(python/openai): use full-history mode by default to fix tool call streaming with OpenRouter #5111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
886f288
4b96724
7b6b59b
d14a021
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2780,6 +2780,24 @@ def test_parse_response_with_store_false() -> None: | |||||||||
| assert conversation_id is None | ||||||||||
|
|
||||||||||
|
|
||||||||||
| def test_parse_response_with_store_none_returns_none() -> None: | ||||||||||
| """Test _get_conversation_id returns None when store=None (default). | ||||||||||
|
|
||||||||||
| Regression for https://github.com/microsoft/agent-framework/issues/5105 — | ||||||||||
| ensures full-history mode is used by default, which works with providers | ||||||||||
| that don't support previous_response_id (e.g. OpenRouter). | ||||||||||
| """ | ||||||||||
| client = OpenAIChatClient(model="test-model", api_key="test-key") | ||||||||||
|
|
||||||||||
| mock_response = MagicMock() | ||||||||||
| mock_response.id = "resp_123" | ||||||||||
| mock_response.conversation = MagicMock() | ||||||||||
| mock_response.conversation.id = "conv_456" | ||||||||||
|
|
||||||||||
| assert client._get_conversation_id(mock_response, store=None) is None | ||||||||||
| assert client._get_conversation_id(mock_response, store=False) is None | ||||||||||
|
|
||||||||||
|
|
||||||||||
| def test_parse_response_uses_response_id_when_no_conversation() -> None: | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test covers the
Suggested change
|
||||||||||
| """Test _get_conversation_id returns response ID when no conversation exists.""" | ||||||||||
| client = OpenAIChatClient(model="test-model", api_key="test-key") | ||||||||||
|
|
@@ -3259,9 +3277,8 @@ def test_streaming_response_basic_structure() -> None: | |||||||||
|
|
||||||||||
|
|
||||||||||
| def test_streaming_response_created_type() -> None: | ||||||||||
| """Test streaming response with created type""" | ||||||||||
| """Test streaming response with created type sets conversation_id only when store=True.""" | ||||||||||
| client = OpenAIChatClient(model="test-model", api_key="test-key") | ||||||||||
| chat_options = ChatOptions() | ||||||||||
| function_call_ids: dict[int, tuple[str, str]] = {} | ||||||||||
|
|
||||||||||
| mock_event = MagicMock() | ||||||||||
|
|
@@ -3271,16 +3288,20 @@ def test_streaming_response_created_type() -> None: | |||||||||
| mock_event.response.conversation = MagicMock() | ||||||||||
| mock_event.response.conversation.id = "conv_5678" | ||||||||||
|
|
||||||||||
| response = client._parse_chunk_from_openai(mock_event, chat_options, function_call_ids) | ||||||||||
|
|
||||||||||
| # store=True: server-managed mode — conversation_id is returned | ||||||||||
| response = client._parse_chunk_from_openai(mock_event, ChatOptions(store=True), function_call_ids) | ||||||||||
| assert response.response_id == "resp_1234" | ||||||||||
| assert response.conversation_id == "conv_5678" | ||||||||||
|
|
||||||||||
| # store=None (default): full-history mode — conversation_id is NOT returned | ||||||||||
| response = client._parse_chunk_from_openai(mock_event, ChatOptions(), function_call_ids) | ||||||||||
| assert response.response_id == "resp_1234" | ||||||||||
| assert response.conversation_id is None | ||||||||||
|
|
||||||||||
|
|
||||||||||
| def test_streaming_response_in_progress_type() -> None: | ||||||||||
| """Test streaming response with in_progress type""" | ||||||||||
| """Test streaming response with in_progress type sets conversation_id only when store=True.""" | ||||||||||
| client = OpenAIChatClient(model="test-model", api_key="test-key") | ||||||||||
| chat_options = ChatOptions() | ||||||||||
| function_call_ids: dict[int, tuple[str, str]] = {} | ||||||||||
|
|
||||||||||
| mock_event = MagicMock() | ||||||||||
|
|
@@ -3290,11 +3311,16 @@ def test_streaming_response_in_progress_type() -> None: | |||||||||
| mock_event.response.conversation = MagicMock() | ||||||||||
| mock_event.response.conversation.id = "conv_5678" | ||||||||||
|
|
||||||||||
| response = client._parse_chunk_from_openai(mock_event, chat_options, function_call_ids) | ||||||||||
|
|
||||||||||
| # store=True: server-managed mode — conversation_id is returned | ||||||||||
| response = client._parse_chunk_from_openai(mock_event, ChatOptions(store=True), function_call_ids) | ||||||||||
| assert response.response_id == "resp_1234" | ||||||||||
| assert response.conversation_id == "conv_5678" | ||||||||||
|
|
||||||||||
| # store=None (default): full-history mode — conversation_id is NOT returned | ||||||||||
| response = client._parse_chunk_from_openai(mock_event, ChatOptions(), function_call_ids) | ||||||||||
| assert response.response_id == "resp_1234" | ||||||||||
| assert response.conversation_id is None | ||||||||||
|
|
||||||||||
|
|
||||||||||
| def test_streaming_annotation_added_with_file_path() -> None: | ||||||||||
| """Streaming `file_path` should attach as a text annotation, matching non-streaming.""" | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this is not the way, this client is designed to connect to the
Responses APIand that API (at least in both OpenAI and Foundry) uses service side storage by default, so only whenstoreis explicitly set to False do we not store anything, the assumption for these clients is that whenstoreis not set, the value isTrue, hence the check forstore=Falseabove. In order to use this as expected with other providers, either create a issue on their end that they adhere to the responses API which means they would have to support the store=True mechanism, or that they raise when store=None, or in your own code, set the options tostore=False