fix: forward OpenAI Responses parse params#2341
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| async with openai.AsyncOpenAI(**self._resolve_client_args()) as client: | ||
| try: | ||
| request = self._format_request(prompt, system_prompt=system_prompt) | ||
| parse_kwargs = {key: value for key, value in request.items() if key not in {"model", "input", "stream"}} |
There was a problem hiding this comment.
Issue: The exclusion-list approach (key not in {"model", "input", "stream"}) is fragile. If _format_request is later extended to add more keys that shouldn't reach responses.parse() (or if users put keys like tools in params for built-in tool use), those will be forwarded silently.
Suggestion: Consider using an allowlist of known safe parameters to forward, or additionally exclude keys that are only relevant to streaming/stateful flows (e.g., previous_response_id, tools, tool_choice):
_PARSE_EXCLUDE_KEYS = {"model", "input", "stream", "tools", "tool_choice", "previous_response_id"}
parse_kwargs = {k: v for k, v in request.items() if k not in _PARSE_EXCLUDE_KEYS}This would prevent built-in tools configured via params (e.g., web_search) from leaking into structured output calls where they're unlikely to be intended.
| assert kwargs["reasoning"] == {"effort": "high"} | ||
| assert kwargs["instructions"] == "Follow the schema" | ||
| assert kwargs["store"] is False | ||
| assert "stream" not in kwargs |
There was a problem hiding this comment.
Issue: The test asserts individual fields but doesn't verify there are no unexpected extra keys being forwarded. If a future change to _format_request adds a new key, this test wouldn't catch it being unintentionally passed through.
Suggestion: Add an assertion on the complete set of forwarded keys to catch unintended leakage:
expected_keys = {"model", "input", "text_format", "max_output_tokens", "reasoning", "instructions", "store"}
assert set(kwargs.keys()) == expected_keys|
Assessment: Comment Good targeted fix that correctly addresses the issue of dropped parameters in Review Themes
Clean, minimal fix with good regression coverage. |
Motivation
OpenAIResponsesModel.structured_output()builds the request through_format_request(), but then only forwardsinputtoclient.responses.parse(). That drops regular Responses API parameters from model config, includingmax_output_tokens,reasoning, and systeminstructions.Closes #1908.
Changes
_format_request()result.model,input, andtext_formatexplicitly.stream, since_format_request()sets it for streaming calls andresponses.parse()should not receive it.max_output_tokens,reasoning,instructions, andstore.Testing
.\.venv\Scripts\python.exe -m pytest tests\strands\models\test_openai_responses.py::test_structured_output tests\strands\models\test_openai_responses.py::test_structured_output_forwards_formatted_request_params -q.\.venv\Scripts\python.exe -m ruff check src\strands\models\openai_responses.py tests\strands\models\test_openai_responses.py.\.venv\Scripts\python.exe -m ruff format --check src\strands\models\openai_responses.py tests\strands\models\test_openai_responses.py.\.venv\Scripts\python.exe -m mypy src\strands\models\openai_responses.py.\strands-py\.venv\Scripts\python.exe -m py_compile strands-py\src\strands\models\openai_responses.py strands-py\tests\strands\models\test_openai_responses.pygit diff --checkBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.