Repository navigation
fix: chat completions extra_args collision with omitted kwargs - #3192
SuperMarioYL wants to merge 2 commits into
Conversation
Mirror the Responses-path fix from openai#3185: when a key in ModelSettings.extra_args matches an entry in create_kwargs whose value is the OpenAI omit/NotGiven sentinel, the user has not actually set the first-class field, so the duplicate-detection check should let the extra_args entry through instead of raising TypeError. Adds two regression tests mirroring tests/models/test_openai_responses.py: one that succeeds when only extra_args supplies a key whose first-class value is omitted, and one that still rejects a true duplicate where both the first-class field and extra_args set the same key.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09555d52d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Can you fix the issue pointed out by Codex and add sufficient tests for covering the pattern?
Codex review noted that the previous duplicate-key filter — which ignores
create_kwargs entries whose value is the OpenAI omit sentinel — also lets
`extra_args={"stream": True}` slip through on the non-streaming path.
`get_response()` pins `create_kwargs["stream"]` to omit when stream=False
and then expects a `ChatCompletion`; if extra_args overrode it to True the
client would return an async stream that the non-streaming code path
cannot consume.
Mark `stream` as a reserved key that must always trigger duplicate-key
detection regardless of its current sentinel value. Other create_kwargs
slots (`reasoning_effort`, `tool_choice`, etc.) keep the omit-aware
behavior, so legitimate fall-through via extra_args still works.
Add two regression tests:
- extra_args={"stream": True} with stream=False must raise TypeError
- extra_args={"stream": False} with stream=True must raise TypeError
|
Thanks for the review @seratch and @codex. You're right — the previous filter let Fixed in 4a5f382 by marking Added two regression tests in
Verified |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
This PR is stale because it has been open for 10 days with no activity. |
|
Gentle bump on this @seratch — fix pushed 2026-05-09, codex re-review came back clean ('Didn't find any major issues'). Happy to rebase against current |
Summary
Mirrors the Responses-path fix from #3185 in the parallel chat-completions
path.
OpenAIChatCompletionsModel._fetch_responsebuildscreate_kwargswhere many fields go through
_non_null_or_omit(...)and become theOpenAI
omitsentinel when the user did not set them. The duplicate-keycheck then incorrectly counted those omitted entries as collisions with
ModelSettings.extra_args, raising:even though the first-class field was never explicitly set. As a result,
users could not legitimately route fields like
reasoning_effortthroughextra_argswhile leavingmodel_settings.reasoningunset.This patch ports the same one-clause filter introduced for Responses in
#3185: skip duplicate-detection on keys whose
create_kwargsvalue isthe OpenAI
Omit/NotGivensentinel, and only raise when both thefirst-class field and
extra_argsactually carry a value. The companion_is_openai_omitted_valuehelper is defined locally for parity withopenai_responses.py.Test plan
make formatmake lintuv run mypy src/agents/models/openai_chatcompletions.py tests/models/test_openai_chatcompletions.pyuv run pytest tests/models/test_openai_chatcompletions.py -qtests/models/test_openai_chatcompletions.py::test_fetch_response_chat_completions_allows_extra_arg_when_explicit_arg_is_omitted— suppliesextra_args={\"reasoning_effort\": \"high\"}whilemodel_settings.reasoningis unset; the underlyingchat.completions.create(**kwargs)call now receivesreasoning_effort=\"high\"instead of failing with a spurious duplicate-argument error.tests/models/test_openai_chatcompletions.py::test_fetch_response_chat_completions_rejects_duplicate_extra_args_keys— suppliestemperature=0.5together withextra_args={\"temperature\": 0.7}; the legitimate duplicate is still rejected with the originalTypeError.Issue number
N/A — parity follow-up to #3185.
Checks