-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Python: Fix FoundryAgent stripping model from PromptAgent requests #5526
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 |
|---|---|---|
|
|
@@ -200,7 +200,7 @@ def my_func() -> str: | |
|
|
||
|
|
||
| async def test_raw_foundry_agent_chat_client_prepare_options_strips_client_side_fields() -> None: | ||
| """Test that _prepare_options strips model and tool-loop fields from run_options.""" | ||
| """Test that _prepare_options strips tool-loop fields but preserves model for non-session requests.""" | ||
|
|
||
| mock_project = MagicMock() | ||
| mock_openai = MagicMock() | ||
|
|
@@ -232,11 +232,74 @@ def my_func() -> str: | |
| options={"tools": [my_func]}, | ||
| ) | ||
|
|
||
| assert "model" not in result | ||
| # model is preserved for non-session (PromptAgent) requests | ||
| assert result["model"] == "gpt-4.1" | ||
| assert "tools" not in result | ||
| assert "tool_choice" not in result | ||
| assert "parallel_tool_calls" not in result | ||
| assert result == {} | ||
|
|
||
|
|
||
| async def test_raw_foundry_agent_chat_client_prepare_options_strips_model_for_hosted_session() -> None: | ||
|
Member
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. I'm not sure if this covers everything, we have three scenario's that we need to cover:
and each has their own particular things it does and does not cover, for now the
Member
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. The gate
I validated all three live against a Foundry endpoint. The I've also added a new test ( |
||
| """Test that model is stripped when using a hosted agent session (not a PromptAgent).""" | ||
|
|
||
| mock_project = MagicMock() | ||
| mock_openai = MagicMock() | ||
| mock_project.get_openai_client.return_value = mock_openai | ||
|
|
||
| client = RawFoundryAgentChatClient( | ||
| project_client=mock_project, | ||
| agent_name="test-agent", | ||
| ) | ||
|
|
||
| with patch( | ||
| "agent_framework_openai._chat_client.RawOpenAIChatClient._prepare_options", | ||
| new_callable=AsyncMock, | ||
| return_value={ | ||
| "model": "gpt-4.1", | ||
| "previous_response_id": "resp_abc", | ||
| }, | ||
| ): | ||
| result = await client._prepare_options( | ||
| messages=[Message(role="user", contents="hi")], | ||
| options={"conversation_id": "agent-session-123"}, | ||
| ) | ||
|
|
||
| assert "model" not in result | ||
| assert "previous_response_id" not in result | ||
| assert result["extra_body"]["agent_session_id"] == "agent-session-123" | ||
|
|
||
|
|
||
| async def test_raw_foundry_agent_chat_client_prepare_options_preserves_model_for_resp_continuation() -> None: | ||
| """Test that model is preserved when conversation_id is a resp_* continuation (HostedAgent v1 / v2-no-session).""" | ||
|
|
||
| mock_project = MagicMock() | ||
| mock_openai = MagicMock() | ||
| mock_project.get_openai_client.return_value = mock_openai | ||
|
|
||
| client = RawFoundryAgentChatClient( | ||
| project_client=mock_project, | ||
| agent_name="test-agent", | ||
| ) | ||
|
|
||
| with patch( | ||
| "agent_framework_openai._chat_client.RawOpenAIChatClient._prepare_options", | ||
| new_callable=AsyncMock, | ||
| return_value={ | ||
| "model": "gpt-4.1", | ||
| "previous_response_id": "resp_abc123", | ||
| }, | ||
| ): | ||
| result = await client._prepare_options( | ||
| messages=[Message(role="user", contents="hi")], | ||
| options={"conversation_id": "resp_abc123"}, | ||
| ) | ||
|
|
||
| # model preserved — resp_* is standard Responses API continuity, not a hosted session | ||
| assert result["model"] == "gpt-4.1" | ||
| # previous_response_id preserved — not stripped outside hosted session path | ||
| assert result["previous_response_id"] == "resp_abc123" | ||
| # no agent_session_id injected | ||
| assert "extra_body" not in result or "agent_session_id" not in result.get("extra_body", {}) | ||
|
|
||
|
|
||
| async def test_raw_foundry_agent_chat_client_prepare_options_maps_agent_session_id_to_extra_body() -> None: | ||
|
|
||
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.
The fix correctly resolves the PromptAgent 400. One question on the gating choice: this uses
_uses_foundry_agent_session(conversation_id)as a proxy for "model is server-managed," which covers the two cases under test (PromptAgent with no session, HostedAgent with session) but not HostedAgent-without-session, which is supported per the docstring at_agent.py:848-853, 900-903(HostedAgents only need a session for preview-only APIs / isolation_key lazy creation).Two questions:
_uses_foundry_agent_session(...)here explaining why it's the right gate (like, "Foundry sessions manage model server-side; Responses-API path requires it"). Right now a future reader has to reverse-engineer the rationale from Python: update FoundryAgent for hosted agent sessions #5447 + this PR.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.
Yes, the platform tolerates
modelin the request for HostedAgent v1 and v2-no-session. Verified live against a Foundry endpoint. Typically users don't pass model for hosted agents (the server owns it), but if they do, the platform simply ignores it -- no error.The strip is only functionally necessary for the
agent_session_idpath where the wire format is fundamentally different (it's a routing envelope, not a real Responses API call).Live validation results with
modelexplicitly indefault_options:'model' is a required propertyallow_preview) + modelallow_preview=True, noisolation_key) + modelallow_preview=True+isolation_key) + modelSo to answer directly: yes, HostedAgent-without-session sends model to the Responses API, and the platform is fine with it. This PR preserves pre-#5447 behavior for that path (no regression), and the platform's tolerance means there's no correctness issue either.