fix: Responses API native content types in adapter - #19
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdapter mapping changed to emit OpenAI Responses API native content types (input_text, input_image, input_file); system-role messages are skipped and media content is normalized via a new mapping helper. Tests and changelog updated accordingly. Changes
Sequence Diagram(s)(omitted — change is a mapping/refactor within the adapter and does not add multi-component sequential flow requiring visualization) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Content blocks now use input_text, input_image, input_file instead of Chat Completions types (text, image_url, file) which OpenAI rejects.
6f3f72a to
21a2be0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/freeplay/resources/adapters.py (1)
385-395: Remove unnecessaryisinstancecheck flagged by pyright.After the preceding checks for
TextContent,dict,content.type == "audio", andMediaContentUrl, the only remaining possibility isMediaContentBase64. Theisinstance(content, MediaContentBase64)check is always true at this point.♻️ Proposed refactor
- if isinstance(content, MediaContentBase64): - if content.type == "file": - return { - "type": "input_file", - "filename": f"{content.slot_name}.{content.content_type.split('/')[-1]}", - "file_data": f"data:{content.content_type};base64,{content.data}", - } - return { - "type": "input_image", - "image_url": f"data:{content.content_type};base64,{content.data}", - } - raise ValueError(f"Unexpected content type {type(content)}") + # At this point, content must be MediaContentBase64 + if content.type == "file": + return { + "type": "input_file", + "filename": f"{content.slot_name}.{content.content_type.split('/')[-1]}", + "file_data": f"data:{content.content_type};base64,{content.data}", + } + return { + "type": "input_image", + "image_url": f"data:{content.content_type};base64,{content.data}", + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/freeplay/resources/adapters.py` around lines 385 - 395, The isinstance(content, MediaContentBase64) guard is redundant because earlier branches (TextContent, dict, content.type == "audio", MediaContentUrl) already exclude other types; remove the isinstance(...) check and unnest its body so the code directly handles the MediaContentBase64 case, keeping the existing branches that return the same dictionaries (the "input_file" branch when content.type == "file" and the "input_image" branch otherwise) and preserving the filename and data construction logic referencing content.slot_name and content.content_type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/freeplay/resources/adapters.py`:
- Around line 359-364: The conditional around appending to result is redundant:
both branches call result.append({"type": "message", **msg}), so remove the
if/else and perform a single append; in addition, if you intended to guard by
content's element types to satisfy the type checker, replace the untyped
all(...) comprehension with a typed check (e.g., annotate or cast content to
Sequence[Any] before the all(...) call, or narrow content's type to
Sequence[Mapping] earlier) so the TypeChecker knows what 'item' is—apply these
changes where content, result, and msg are used in this adapter function.
- Around line 373-374: The handler accepts a raw dict at runtime but the
function's type hint for the parameter content is declared as Union[TextContent,
MediaContentBase64, MediaContentUrl]; update the type signature to reflect the
passthrough by adding Dict[str, Any] (or Mapping[str, Any]) to the Union for the
content parameter, and keep the existing `if isinstance(content, dict): return
content` branch, or if dicts should be unreachable remove that branch and
validate/convert incoming dicts instead; target the function that declares the
content parameter and the `if isinstance(content, dict)` branch to make the
types consistent.
---
Nitpick comments:
In `@src/freeplay/resources/adapters.py`:
- Around line 385-395: The isinstance(content, MediaContentBase64) guard is
redundant because earlier branches (TextContent, dict, content.type == "audio",
MediaContentUrl) already exclude other types; remove the isinstance(...) check
and unnest its body so the code directly handles the MediaContentBase64 case,
keeping the existing branches that return the same dictionaries (the
"input_file" branch when content.type == "file" and the "input_image" branch
otherwise) and preserving the filename and data construction logic referencing
content.slot_name and content.content_type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21936091-1df0-4782-984a-d81b77d37b28
📒 Files selected for processing (3)
CHANGELOG.mdsrc/freeplay/resources/adapters.pytests/test_adapters.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/freeplay/resources/adapters.py (1)
343-343: Consider usingdict.getfor cleaner key access.Static analysis flags this as unnecessarily verbose.
♻️ Proposed refactor
- if "has_media" in message and message["has_media"]: + if message.get("has_media"):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/freeplay/resources/adapters.py` at line 343, Replace the verbose key check for the message dict with a get-based truthy check: in the block that currently uses "if 'has_media' in message and message['has_media']:" (the code handling the incoming message variable named message), use message.get('has_media') to simplify and make the intent clearer; ensure the truthiness semantics remain the same so falsy or missing values are handled identically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/freeplay/resources/adapters.py`:
- Around line 384-388: The return that builds the "input_image" dict is
currently unconditional and its closing brace is over-indented, making the
subsequent raise ValueError unreachable and causing Ruff to fail; fix this by
making the image branch conditional (e.g., if content is the expected image type
or matches content.content_type), ensure the closing brace alignment uses
8-space indentation to match the surrounding block, and move the raise
ValueError into the function's fallback path (after the conditional branches) so
it executes only when no handled content type matches; refer to the return that
produces {"type": "input_image", "image_url":
f"data:{content.content_type};base64,{content.data}"} and the raise
ValueError(f"Unexpected content type {type(content)}") to locate the lines to
change.
---
Nitpick comments:
In `@src/freeplay/resources/adapters.py`:
- Line 343: Replace the verbose key check for the message dict with a get-based
truthy check: in the block that currently uses "if 'has_media' in message and
message['has_media']:" (the code handling the incoming message variable named
message), use message.get('has_media') to simplify and make the intent clearer;
ensure the truthiness semantics remain the same so falsy or missing values are
handled identically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 764fe4c1-9d64-42b9-9509-94ac6fe13201
📒 Files selected for processing (1)
src/freeplay/resources/adapters.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/freeplay/resources/adapters.py (1)
343-343: Consider simplifying the key check withdict.get.The pattern
"has_media" in message and message["has_media"]can be simplified tomessage.get("has_media"), which returnsNone(falsy) if the key is missing.♻️ Suggested simplification
- if "has_media" in message and message["has_media"]: + if message.get("has_media"):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/freeplay/resources/adapters.py` at line 343, Replace the explicit key membership check and indexing ("has_media" in message and message["has_media"]) with the simpler truthy lookup message.get("has_media") to achieve the same behavior when the key is missing; locate the occurrence that uses the variable message in src/freeplay/resources/adapters.py and update that conditional to use message.get("has_media") so it remains falsy when absent or false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/freeplay/resources/adapters.py`:
- Around line 367-370: The code currently rejects audio inputs but allows video
passed as MediaContentBase64 to fall through and be treated as input_image;
update the same check that inspects content.type to also raise a ValueError for
"video" (e.g., if content.type == "audio" or content.type == "video": raise
ValueError("Video content is not supported by the Responses API") ), so any
MediaContentBase64 with content.type "video" is consistently rejected before it
reaches the input_image handling.
---
Nitpick comments:
In `@src/freeplay/resources/adapters.py`:
- Line 343: Replace the explicit key membership check and indexing ("has_media"
in message and message["has_media"]) with the simpler truthy lookup
message.get("has_media") to achieve the same behavior when the key is missing;
locate the occurrence that uses the variable message in
src/freeplay/resources/adapters.py and update that conditional to use
message.get("has_media") so it remains falsy when absent or false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9a4effb-5d1c-4ec6-879e-a345471f30a6
📒 Files selected for processing (1)
src/freeplay/resources/adapters.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/freeplay/resources/adapters.py`:
- Around line 341-342: The loop is discarding valid system messages
(role_support declares "system") by using `if message["role"] == "system":
continue`; instead collect system-role content and merge it into the prompt
instructions pipeline so system instructions are preserved. Replace the
`continue` with logic that appends `message["content"]` (or the full `message`)
to the same top-level `instructions`/`system_instructions` variable used for
prompt assembly (or create `system_instructions` and later prepend/merge it into
`instructions` before request send), ensuring existing handling for `developer`
and other roles remains unchanged and any downstream code that reads
`instructions` receives the system text.
- Around line 355-356: OpenAIResponsesAdapter currently lists "tool" in
role_support but later serializes messages as {"type": "message", **msg} (see
the msg = copy.deepcopy(message) / result.append(...) block), which yields
invalid OpenAI Conversations API payloads; fix by removing "tool" from the
adapter's role_support OR add explicit handling for messages with role == "tool"
in the message serialization path (e.g., skip them, raise a clear error, or
convert them to the proper conversation item type) so that tool messages never
get emitted as {"type":"message","role":"tool",...}; update
OpenAIResponsesAdapter.role_support and/or the serialization before
result.append to handle tool messages accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f9b7f0d-0897-43ee-9ce9-ccd4bb1fcad7
📒 Files selected for processing (1)
src/freeplay/resources/adapters.py
| msg = copy.deepcopy(message) | ||
| result.append({"type": "message", **msg}) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l src/freeplay/resources/adapters.pyRepository: freeplayai/freeplay-python
Length of output: 108
🏁 Script executed:
cat -n src/freeplay/resources/adapters.py | sed -n '340,370p'Repository: freeplayai/freeplay-python
Length of output: 1475
🏁 Script executed:
cat -n src/freeplay/resources/adapters.py | sed -n '1,100p'Repository: freeplayai/freeplay-python
Length of output: 3623
🏁 Script executed:
cat -n src/freeplay/resources/adapters.py | sed -n '320,360p'Repository: freeplayai/freeplay-python
Length of output: 1725
🏁 Script executed:
cat -n src/freeplay/resources/adapters.py | sed -n '200,320p'Repository: freeplayai/freeplay-python
Length of output: 5279
🏁 Script executed:
rg "tool" src/freeplay/resources/adapters.py -i -B 3 -A 3Repository: freeplayai/freeplay-python
Length of output: 410
🏁 Script executed:
rg "type.*message" src/freeplay/resources/ -i -B 2 -A 2Repository: freeplayai/freeplay-python
Length of output: 748
🏁 Script executed:
git ls-files | grep -E "(test|spec)" | head -20Repository: freeplayai/freeplay-python
Length of output: 770
🏁 Script executed:
find tests -name "*.py" -type f | xargs rg "OpenAIResponsesAdapter|tool" -B 3 -A 3Repository: freeplayai/freeplay-python
Length of output: 35639
🏁 Script executed:
rg "class OpenAI" src/freeplay/resources/adapters.py -A 50 | head -80Repository: freeplayai/freeplay-python
Length of output: 3023
🏁 Script executed:
cat -n tests/test_adapters.py | grep -A 30 "openai_responses"Repository: freeplayai/freeplay-python
Length of output: 5007
🏁 Script executed:
cat -n tests/test_adapters.py | grep -B 5 -A 20 "test_openai_responses"Repository: freeplayai/freeplay-python
Length of output: 4048
🏁 Script executed:
web_search
OpenAI Conversations API Responses create-item schema allowed roles message typeRepository: freeplayai/freeplay-python
Length of output: 165
🌐 Web query:
OpenAI Conversations API Responses message schema roles allowed types
💡 Result:
For the Responses API / Conversations-compat message items, the schema is:
- Item shape:
{"type":"message","role":..., "content":...}(with optionalphaseon assistant messages). Roles are used for instruction hierarchy. Allowedrolevalues:user,assistant,system,developer. [1] content: either a string or an array of content parts. [1]- Allowed input content-part
types (incontent[]):input_text[2]input_image[2]input_file[3]
- Common assistant output content-part
type:output_text[4]
Sources: [1] [2] [3] [4]
Remove tool from supported roles or implement separate handling for tool responses.
The OpenAIResponsesAdapter declares "tool" as a supported role (line 332), but the OpenAI Conversations API only accepts user, assistant, system, and developer roles in message items. Tool messages must be sent as separate conversation item types. If a tool message reaches lines 355-356, it will serialize as {"type": "message", "role": "tool", ...}, producing an invalid API payload.
Either remove "tool" from role_support or add logic to handle tool messages separately (e.g., skip them, raise an error, or transform them to the appropriate conversation item type for the API).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/freeplay/resources/adapters.py` around lines 355 - 356,
OpenAIResponsesAdapter currently lists "tool" in role_support but later
serializes messages as {"type": "message", **msg} (see the msg =
copy.deepcopy(message) / result.append(...) block), which yields invalid OpenAI
Conversations API payloads; fix by removing "tool" from the adapter's
role_support OR add explicit handling for messages with role == "tool" in the
message serialization path (e.g., skip them, raise a clear error, or convert
them to the proper conversation item type) so that tool messages never get
emitted as {"type":"message","role":"tool",...}; update
OpenAIResponsesAdapter.role_support and/or the serialization before
result.append to handle tool messages accordingly.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/freeplay/resources/adapters.py (1)
343-343: Simplify dict access pattern.The key existence check followed by access can be replaced with
dict.get()for cleaner code.♻️ Proposed fix
- if "has_media" in message and message["has_media"]: + if message.get("has_media"):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/freeplay/resources/adapters.py` at line 343, Replace the explicit key-existence check for the incoming message dict with dict.get to simplify the condition: change the if that currently reads checking "has_media" in message and then indexing message["has_media"] to use message.get("has_media") (e.g., inside the function/method handling incoming messages where the local variable message is used) so the condition is shorter and handles missing keys safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/freeplay/resources/adapters.py`:
- Line 343: Replace the explicit key-existence check for the incoming message
dict with dict.get to simplify the condition: change the if that currently reads
checking "has_media" in message and then indexing message["has_media"] to use
message.get("has_media") (e.g., inside the function/method handling incoming
messages where the local variable message is used) so the condition is shorter
and handles missing keys safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca2baada-49e5-4eb9-8be8-345852c9e383
📒 Files selected for processing (1)
src/freeplay/resources/adapters.py
2 participants
Summary
OpenAIResponsesAdapternow produces Responses API native content types (input_text,input_image,input_file) instead of inheriting Chat Completions types (text,image_url,file) fromOpenAIAdapterinputparameterNote
Fix
OpenAIResponsesAdapterto use Responses API native content typestext,image_url,file) with Responses API native types (input_text,input_image,input_file) inadapters.py._map_responses_contentstatic helper that converts internal content representations to the correct Responses API shapes, deriving filenames fromslot_nameandcontent_typefor file uploads.MediaContentUrlitems now raiseValueErrorinstead of being passed through.Macroscope summarized 4edac8f.
Summary by CodeRabbit
Bug Fixes
Tests