[codex-apps] Omit internal fields from file payloads#31330
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
![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: ea12126f62
ℹ️ 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".
| if optional_fields | ||
| .iter() | ||
| .any(|optional_field| optional_field == "mime_type") | ||
| && let Some(mime_type) = uploaded.mime_type |
There was a problem hiding this comment.
When a Codex Apps file param schema does not declare mime_type/file_name (the existing AppsTestServer fixture only declares file_id), this branch now sends only download_url and file_id, but core/tests/suite/openai_file_mcp.rs still expects uploaded_file() with mime_type, file_name, uri, and file_size_bytes for both the MCP call and the PostToolUse hook. That existing integration suite will fail once it reaches those assertions and no longer documents the intended user-facing payload shape, so please update the suite fixture/expectations alongside this payload change.
AGENTS.md reference: AGENTS.md:L114-L118
Useful? React with 👍 / 👎.
jacobzhou-oai
force-pushed
the
codex/schema-aware-file-param-payloads
branch
from
ea12126 to
8e0a76d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00a35b1bf7
ℹ️ 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".
| .get(keyword) | ||
| .and_then(JsonValue::as_array) | ||
| .is_some_and(|variants| { | ||
| variants.iter().any(|variant| { |
There was a problem hiding this comment.
For a file parameter schema that uses anyOf/oneOf to allow either a provided-file object or another image/file shape, this any marks mime_type or file_name as accepted when any variant declares it. If the provided-file variant is strict, e.g. {download_url,file_id} with additionalProperties:false, while another variant declares mime_type but also requires unrelated fields, the rewritten upload payload adds mime_type and then no union branch validates, so the Apps tool is rejected before execution. The optional-field scan should only include fields from a branch that can accept the uploaded provided-file payload, rather than any union branch.
Useful? React with 👍 / 👎.
| let first_segment = segments.next()?.replace("~1", "/").replace("~0", "~"); | ||
| let mut referenced_schema = root_schema.get(&first_segment)?; | ||
|
|
||
| for segment in segments { | ||
| let segment = segment.replace("~1", "/").replace("~0", "~"); |
There was a problem hiding this comment.
When an Apps schema uses a valid local $ref whose JSON Pointer fragment contains percent-encoded characters, for example #/$defs/Provided%20File for a $defs key named Provided File, this resolver splits and looks up the raw %20 text instead of URI-decoding the fragment first. In that case the optional file fields declared behind the ref are missed, so a strict tool that requires mime_type or file_name via such a ref still receives only download_url/file_id and can fail schema validation.
Useful? React with 👍 / 👎.
| let properties = tool | ||
| .input_schema | ||
| .get("properties") | ||
| .and_then(JsonValue::as_object); |
There was a problem hiding this comment.
When an Apps tool defines its parameter object through a root $ref (for example inputSchema: { "$ref": "#/$defs/Params", "$defs": { ... } }) and declares openai/fileParams, this direct properties lookup returns None, so every file param is recorded with no accepted optional fields. A strict tool whose referenced file payload schema requires or accepts mime_type/file_name then receives only download_url/file_id, whereas the previous rewrite sent those fields, causing schema validation failures for otherwise valid referenced schemas.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31fe678000
ℹ️ 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".
| if schema.get("type").and_then(JsonValue::as_str) == Some("array") { | ||
| if let Some(items) = schema.get("items") { | ||
| pending.push((items, true)); | ||
| } | ||
| continue; |
There was a problem hiding this comment.
When a Codex Apps file parameter schema encodes a nullable array as type: ["array", "null"] with items (a standard JSON Schema form used elsewhere in this repo), this check does not recognize it as an array because as_str() returns None. openai_file_schema_info therefore leaves is_array false, and rewrite_input_property_schema_as_local_file_path masks the parameter as a single string even though the app will validate an array of uploaded-file objects, causing multi-file tools with this schema shape to receive scalar arguments and fail validation. Consider also treating a type array containing "array" (or the presence of items) as an array.
Useful? React with 👍 / 👎.
| continue; | ||
| } | ||
|
|
||
| info.is_array |= inside_array; |
There was a problem hiding this comment.
For a declared openai/fileParams argument whose schema is still an array but whose items are not one of the newly recognized canonical provided-file objects (for example a legacy/generic { "type": "array", "items": { "type": "object" } } schema), is_array stays false because it is only set after the item schema passes the canonical download_url/file_id property check. The subsequent masking then exposes the parameter as a single string instead of an array of paths, so the model will send a scalar while the app still validates an array; this regresses schemas the previous top-level type == "array" || items check handled.
Useful? React with 👍 / 👎.
| for keyword in ["anyOf", "oneOf", "allOf"] { | ||
| if let Some(variants) = schema.get(keyword).and_then(JsonValue::as_array) { | ||
| pending.extend(variants.iter().map(|variant| (variant, inside_array))); | ||
| } |
There was a problem hiding this comment.
When a provided-file schema uses allOf, each branch must validate the same payload, but this traversal treats allOf the same as anyOf/oneOf and then ORs accepts_mime_type/accepts_file_name across the discovered branches. In a schema where one allOf branch is a strict {download_url,file_id} object (additionalProperties: false) and another branch mentions mime_type, Codex will now include mime_type, even though the strict branch rejects it before the app runs. For allOf, optional fields need to be intersected/merged with the constraints from every branch rather than accepted from any branch.
Useful? React with 👍 / 👎.
jacobzhou-oai
changed the title
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ 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". |
|
I have read the CLA Document and I hereby sign the CLA |
3 participants
Summary
Codex Apps file parameters are exposed to the model as local paths, uploaded at execution time, and rewritten into provided-file payloads before the MCP tool call.
The rewrite currently forwards two internal upload fields,
uriandfile_size_bytes, even though they are not part of the documented app file-reference shape. Strict app schemas can reject those extra fields before execution.Changes
uriandfile_size_bytesinto app-facing MCP arguments.UploadedOpenAiFileresult unchanged.download_url,file_id,mime_type, andfile_namebehavior for scalar and array file inputs.additionalProperties: falseschema.This intentionally does not add schema inspection or change how
openai/fileParamsnames are discovered.Validation
just test -p codex-core mcp_openai_file(6 passed)just test -p codex-core codex_apps_file_params_(2 passed)just fix -p codex-corejust fmtgit diff --check