[GATE-93] Normalize PDF image parts to file content - #455
Ken Jiang (knjiang) wants to merge 4 commits into
Conversation
Ken Jiang (knjiang)
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 4750008833
ℹ️ 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 (@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 (@codex) address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 090ee3eddd
ℹ️ 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 (@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 (@codex) address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a4be8ed25
ℹ️ 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 (@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 (@codex) address that feedback".
| .ok() | ||
| .and_then(|bytes| String::from_utf8(bytes).ok()); | ||
| .and_then(|bytes| String::from_utf8(bytes).ok()) | ||
| .filter(|_| !block.media_type.eq_ignore_ascii_case("application/pdf")); |
There was a problem hiding this comment.
When an OpenAI input_file supplies canonical raw base64 rather than a data URL, an ASCII-compatible PDF such as JVBERi0xLjQ= bypasses this new filter and the following raw-base64 branch decodes it as UTF-8, changes its media type to text/plain, and replaces the base64 with decoded text. A Responses/Chat Completions request transformed to Anthropic therefore still loses or invalidates the PDF. The fresh evidence beyond the prior PDF-source comment is that the new guard only covers the data-URL branch at universal_file_payload_from_openai, while the adjacent raw-base64 branch remains unguarded; apply the same PDF exclusion there using the media type inferred from the filename.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
| } else if media_type == "application/pdf" { | ||
| generated::Base64ImageSourceType::Base64 |
There was a problem hiding this comment.
When a PDF reaches this converter with a valid MIME case variant such as Application/PDF—for example from an OpenAI data URL, whose newly added import guard explicitly accepts case variants—the exact comparison falls through to source.type: "text", and the preceding exact media-type match also emits text/plain. The resulting Anthropic document is invalid or lossy instead of a base64 PDF. This is fresh evidence beyond the prior normalized-router case because direct OpenAI file conversion preserves the original MIME casing; normalize the media type or use case-insensitive comparisons for both Anthropic enum decisions.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
| @@ -0,0 +1,11 @@ | |||
| # Emit base64 Anthropic PDF document sources | |||
There was a problem hiding this comment.
might want to delete
Erin McNulty (erin2722)
left a comment
Erin McNulty (erin2722)
left a comment
There was a problem hiding this comment.
can we test this w an integration test?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8be34bad4
ℹ️ 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 (@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 (@codex) address that feedback".
| let (filename, mut media) = if let Some(media) = parse_base64_data_url(&image.url) { | ||
| if normalized_media_type(&media.media_type) != "application/pdf" { | ||
| continue; | ||
| } | ||
| ("file.pdf".to_string(), media) | ||
| } else if let Some(metadata) = pdf_url_metadata(&image.url) { |
There was a problem hiding this comment.
This introduces new provider-transform behavior for Chat Completions PDF image parts, but the diff adds only Rust unit tests and no behavior-named case in payloads/cases/params.ts or captured transforms. Consequently, the payload and cross-provider test matrix does not exercise this route across the affected OpenAI, Anthropic, Vertex Anthropic, and Responses adapters, allowing provider-specific regressions to pass those checks; add the required payload case and capture its transformations.
AGENTS.md reference: AGENTS.md:L123-L129
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
im gonna add a integation test in the main repo
2 participants
Preserve PDF MIME types and base64 data through OpenAI Chat Completions, Responses, and Anthropic conversion.