-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Python: fix(openai): report the background cause when a tool result is rejected #7603
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
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 |
|---|---|---|
|
|
@@ -1374,6 +1374,116 @@ async def test_bad_request_error_non_content_filter() -> None: | |
| assert "failed to complete the prompt" in str(exc_info.value) | ||
|
|
||
|
|
||
| def _tool_output_pairing_error() -> BadRequestError: | ||
| """Build the 400 the Responses API returns for an unpaired function_call_output.""" | ||
| message = "No tool call found for function call output with call_id call_abc123." | ||
| error = BadRequestError( | ||
| message=message, | ||
| response=MagicMock(), | ||
| body={"error": {"code": None, "message": message, "param": "input"}}, | ||
| ) | ||
| error.code = None | ||
| return error | ||
|
|
||
|
|
||
| async def test_background_tool_output_pairing_error_reports_background_limitation() -> None: | ||
|
Author
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. Added. |
||
| """A background predecessor rejecting a tool result is reported with the actionable cause.""" | ||
| client = OpenAIChatClient(model="test-model", api_key="test-key") | ||
|
|
||
| with ( | ||
| patch.object(client.client.responses.with_raw_response, "create", side_effect=_tool_output_pairing_error()), | ||
| pytest.raises(ChatClientException) as exc_info, | ||
| ): | ||
| await client.get_response( | ||
| messages=[Message(role="user", contents=["Test message"])], | ||
| options={"background": True, "conversation_id": "resp_abc123"}, | ||
| ) | ||
|
|
||
| message = str(exc_info.value) | ||
| assert "background=True" in message | ||
| assert "background=False" in message | ||
| assert "microsoft/agent-framework#7538" in message | ||
|
|
||
|
|
||
| async def test_background_tool_output_pairing_error_does_not_assert_predecessor_was_background() -> None: | ||
| """The guidance is conditional, since an orphaned call_id reaches this branch identically. | ||
|
|
||
| `background` in the run options describes this request, not the response named by | ||
| `previous_response_id`. A caller submitting a genuinely orphaned `call_id` on a background | ||
| continuation produces the same three signals, so the message must state the background | ||
| limitation as a condition to check and name the orphaned-call_id alternative, rather than | ||
| claim the predecessor was a background response. | ||
| """ | ||
| client = OpenAIChatClient(model="test-model", api_key="test-key") | ||
|
|
||
| with ( | ||
| patch.object(client.client.responses.with_raw_response, "create", side_effect=_tool_output_pairing_error()), | ||
| pytest.raises(ChatClientException) as exc_info, | ||
| ): | ||
| await client.get_response( | ||
| messages=[Message(role="user", contents=["Test message"])], | ||
| options={"background": True, "conversation_id": "resp_abc123"}, | ||
| ) | ||
|
|
||
| message = str(exc_info.value) | ||
| assert "If the preceding response was created with background=True" in message | ||
| assert "If the preceding response was not a background response" in message | ||
| assert "does not match a function_call on it" in message | ||
|
|
||
|
|
||
| async def test_streaming_background_tool_output_pairing_error_reports_background_limitation() -> None: | ||
| """The streaming path reports the same background limitation as the non-streaming path.""" | ||
| client = OpenAIChatClient(model="test-model", api_key="test-key") | ||
|
|
||
| with ( | ||
| patch.object(client.client.responses.with_raw_response, "create", side_effect=_tool_output_pairing_error()), | ||
| pytest.raises(ChatClientException, match="background=False"), | ||
| ): | ||
| response_stream = client.get_response( | ||
| stream=True, | ||
| messages=[Message(role="user", contents=["Test message"])], | ||
| options={"background": True, "conversation_id": "resp_abc123"}, | ||
| ) | ||
| async for _ in response_stream: | ||
| break | ||
|
|
||
|
|
||
| async def test_foreground_tool_output_pairing_error_keeps_generic_message() -> None: | ||
| """A foreground predecessor keeps the generic message, since that chaining is supported.""" | ||
| client = OpenAIChatClient(model="test-model", api_key="test-key") | ||
|
|
||
| with ( | ||
| patch.object(client.client.responses.with_raw_response, "create", side_effect=_tool_output_pairing_error()), | ||
| pytest.raises(ChatClientException) as exc_info, | ||
| ): | ||
| await client.get_response( | ||
| messages=[Message(role="user", contents=["Test message"])], | ||
| options={"conversation_id": "resp_abc123"}, | ||
| ) | ||
|
|
||
| message = str(exc_info.value) | ||
| assert "failed to complete the prompt" in message | ||
| assert "background=False" not in message | ||
|
|
||
|
|
||
| async def test_background_without_previous_response_keeps_generic_message() -> None: | ||
| """A background request that is not a continuation keeps the generic message.""" | ||
| client = OpenAIChatClient(model="test-model", api_key="test-key") | ||
|
|
||
| with ( | ||
| patch.object(client.client.responses.with_raw_response, "create", side_effect=_tool_output_pairing_error()), | ||
| pytest.raises(ChatClientException) as exc_info, | ||
| ): | ||
| await client.get_response( | ||
| messages=[Message(role="user", contents=["Test message"])], | ||
| options={"background": True}, | ||
| ) | ||
|
|
||
| message = str(exc_info.value) | ||
| assert "failed to complete the prompt" in message | ||
| assert "background=False" not in message | ||
|
|
||
|
|
||
| async def test_streaming_content_filter_exception_handling() -> None: | ||
| """Test that content filter errors in get_response(..., stream=True) are properly handled.""" | ||
| client = OpenAIChatClient(model="test-model", api_key="test-key") | ||
|
|
||
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.
Correct, and it changed the patch — though the fix belongs in the message rather than the gate.
You are right that
backgrounddescribes this request, not the response named byprevious_response_id. A caller who submits a genuinely orphanedcall_idon a background continuation produces all three signals and reaches this branch identically, including the foreground-predecessor case you name.Gating on tracked predecessor provenance is not available to this client, though. It is stateless with respect to
previous_response_id: it holds no map from a response id to how that response was created, and a continuation may be built by a different process, or a different client instance, than the one that created the predecessor. State carried forward from background polling would cover only the subset of continuations this instance itself created, so the branch would still be reachable without it — and would then be inconsistent about when it fires.So the message no longer asserts anything about the predecessor. It states the background limitation as a condition the caller can check, and names the orphaned-
call_idalternative explicitly:Both readings are correct whichever case the caller is actually in, and the original error text is still included either way.
test_background_tool_output_pairing_error_does_not_assert_predecessor_was_backgroundpins the wording, and the helper's docstring records why the distinction is not decidable here.